Add Kaggle models integration plan and scaffold - #346
Open
MasumRab wants to merge 9 commits into
Open
Conversation
* Update docs/tasks/06_GEMMA_INTEGRATION.md to include steps for Kagglehub and KerasNLP. * Add KaggleGemmaClient reference implementation to backend/examples/gemma_providers.py. 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. |
|
Merging to
|
Reviewer's GuideAdds a Kaggle-based Gemma client example using Kagglehub + KerasNLP and updates the Gemma integration task doc to include Kaggle as a first-class provider with required dependencies and configuration keys. Sequence diagram for KaggleGemmaClient initialization and text generationsequenceDiagram
actor Developer
participant App as Application
participant Config as AppConfig
participant Client as KaggleGemmaClient
participant OS as os_environ
participant KH as kagglehub
participant KCfg as keras_config
participant KNLP as keras_nlp_models
participant LLM as GemmaCausalLM
Developer->>Config: Set GEMMA_PROVIDER=kaggle
Developer->>Config: Set GEMMA_MODEL_NAME=google/gemma-2/keras/gemma2-2b-en
Developer->>OS: Set KAGGLE_USERNAME and KAGGLE_KEY
App->>Client: new KaggleGemmaClient(model_handle)
activate Client
Client->>OS: get(KAGGLE_USERNAME), get(KAGGLE_KEY)
OS-->>Client: values or None
Client->>Client: print warning if missing
Client->>KH: model_download(model_handle)
KH-->>Client: model_path
Client->>KCfg: set_floatx(bfloat16)
KCfg-->>Client: ok
Client->>KNLP: GemmaCausalLM.from_preset(model_path)
KNLP-->>Client: LLM instance
Client->>Client: store llm
deactivate Client
App-->>Developer: KaggleGemmaClient ready
Developer->>App: Request completion(prompt)
App->>Client: generate(prompt, max_length)
activate Client
Client->>LLM: generate(prompt, max_length)
LLM-->>Client: generated_text
Client-->>App: generated_text
deactivate Client
App-->>Developer: generated_text
Class diagram for KaggleGemmaClient Gemma providerclassDiagram
class KaggleGemmaClient {
- str model_handle
- str model_path
- any llm
+ KaggleGemmaClient(model_handle: str)
+ generate(prompt: str, max_length: int, **kwargs) str
}
class kagglehub {
+ model_download(model_handle: str) str
}
class keras_nlp_models {
+ GemmaCausalLM_from_preset(preset: str) any
}
class keras_config {
+ set_floatx(dtype: str) void
}
KaggleGemmaClient ..> kagglehub : uses
KaggleGemmaClient ..> keras_nlp_models : uses
KaggleGemmaClient ..> keras_config : configures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR adds CI/workflow and pre-commit configs, modernizes type hints, refactors and extends the agent/research/RAG/search stacks (new DeepResearchAgent, kg_enrich, compression, dual-write RAG/Chroma support), expands config/.env examples (LangSmith placeholders, MAX_RESEARCH_LOOPS), introduces new Gemma providers, and applies broad frontend stylistic and UI refinements. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/UI
participant Agent as DeepResearchAgent
participant LLM as LLM (Gemma/Gemini)
participant Search as WebSearcher/SearchProviders
participant RAG as RAG/VectorStore (FAISS/Chroma)
participant Synth as Synthesizer
Client->>Agent: run(topic)
Agent->>LLM: _plan_queries(topic) (prompt -> JSON queries)
LLM-->>Agent: list of queries
Agent->>Search: search(query) [parallel per query]
Search-->>Agent: results (title,url,snippet,content)
Agent->>RAG: ingest_research_results(results, subgoal_id)
RAG-->>Agent: ingestion confirmations / embeddings
Agent->>LLM: synthesize results (SYNTHESIS_PROMPT + context)
LLM-->>Agent: synthesized answer (with citations)
Agent-->>Client: final answer + sources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
🧪 CI InsightsHere's what we observed from your CI run for 8171dc9. 🟢 All jobs passed!But CI Insights is watching 👀 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider avoiding side-effectful work (downloading the model, global
keras.config.set_floatx) in the constructor; a lazy-loading pattern or explicitload()method would give callers more control over when these expensive/global operations occur. - Replace
printstatements inKaggleGemmaClientwith the project’s logging mechanism so logs can be controlled consistently across providers and environments. - Ensure
KaggleGemmaClient.generatereturns the same type/shape as other Gemma clients (e.g., plain string vs tensor/list) so the higher-levelLLMClient-style interface can treat all providers uniformly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding side-effectful work (downloading the model, global `keras.config.set_floatx`) in the constructor; a lazy-loading pattern or explicit `load()` method would give callers more control over when these expensive/global operations occur.
- Replace `print` statements in `KaggleGemmaClient` with the project’s logging mechanism so logs can be controlled consistently across providers and environments.
- Ensure `KaggleGemmaClient.generate` returns the same type/shape as other Gemma clients (e.g., plain string vs tensor/list) so the higher-level `LLMClient`-style interface can treat all providers uniformly.
## Individual Comments
### Comment 1
<location path="backend/examples/gemma_providers.py" line_range="252" />
<code_context>
+ print(f"Model path: {self.model_path}")
+
+ # Set floatx for efficiency if desired
+ keras.config.set_floatx("bfloat16")
+
+ # Initialize the causal language model via Keras NLP
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid mutating global floatx inside a library-style client, or make it configurable.
Calling `keras.config.set_floatx("bfloat16")` in the constructor changes global process-wide config and may unexpectedly affect other code that relies on the default float type. Instead, consider accepting the desired dtype as a constructor parameter so callers can opt in, or omit this change and rely on existing global/environment configuration.
Suggested implementation:
```python
# Download the model weights and assets via kagglehub
print(f"Downloading/Locating model {model_handle} via kagglehub...")
self.model_path = kagglehub.model_download(model_handle)
print(f"Model path: {self.model_path}")
# Initialize the causal language model via Keras NLP
# Since keras_nlp uses presets, we load it using the downloaded path
print(f"Loading Gemma model via Keras NLP from {self.model_path}...")
# Note: For custom downloaded paths, from_preset can point to a local directory
self.llm = keras_nlp.models.GemmaCausalLM.from_preset(self.model_path)
```
If you want to make the dtype configurable instead of relying solely on global/environment configuration, you can:
1. Add a constructor parameter to the `KaggleGemmaClient` (e.g., `dtype: str | None = None`) and store it as `self.dtype`.
2. Document that callers can configure Keras globally before instantiating this client (e.g., `keras.config.set_floatx("bfloat16")` in application code), or use presets that already specify the desired dtype.
</issue_to_address>
### Comment 2
<location path="backend/examples/gemma_providers.py" line_range="274" />
<code_context>
+ max_length: Maximum length of the generated sequence.
+ """
+ # KerasNLP GemmaCausalLM 'generate' takes the prompt and max_length
+ output = self.llm.generate(prompt, max_length=max_length)
+ return output
</code_context>
<issue_to_address>
**issue (bug_risk):** Forward `**kwargs` to the underlying `llm.generate` to avoid silently discarding options.
`**kwargs` are accepted but not passed to `self.llm.generate`, so caller-specified options (e.g., temperature, top_p) are silently ignored. Please update the call to `self.llm.generate(prompt, max_length=max_length, **kwargs)` so all generation parameters are honored.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 MasumRab your PR is conflicting and needs to be updated to be merged |
Resolve formatting conflicts and address PR comments on KaggleGemmaClient implementation. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Owner
Author
|
@jules pull lates update and address comments and conflits Overall Comments
Individual CommentsComment 1+ print(f"Model path: {self.model_path}") + + # Set floatx for efficiency if desired + keras.config.set_floatx("bfloat16") + + # Initialize the causal language model via Keras NLP **suggestion (bug_risk):** Avoid mutating global floatx inside a library-style client, or make it configurable.Calling Suggested implementation: # Download the model weights and assets via kagglehub
print(f"Downloading/Locating model {model_handle} via kagglehub...")
self.model_path = kagglehub.model_download(model_handle)
print(f"Model path: {self.model_path}")
# Initialize the causal language model via Keras NLP
# Since keras_nlp uses presets, we load it using the downloaded path
print(f"Loading Gemma model via Keras NLP from {self.model_path}...")
# Note: For custom downloaded paths, from_preset can point to a local directory
self.llm = keras_nlp.models.GemmaCausalLM.from_preset(self.model_path)If you want to make the dtype configurable instead of relying solely on global/environment configuration, you can:
Comment 2+ max_length: Maximum length of the generated sequence. + """ + # KerasNLP GemmaCausalLM 'generate' takes the prompt and max_length + output = self.llm.generate(prompt, max_length=max_length) + return output **issue (bug_risk):** Forward `**kwargs` to the underlying `llm.generate` to avoid silently discarding options.
|
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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
backend/src/search/providers/google_adapter.py (1)
17-20:⚠️ Potential issue | 🟡 Minor
Clientis instantiated even whenapi_keyisNone.Unlike
TavilyAdapterwhich conditionally creates the client only when the API key exists, this adapter always instantiatesClient(api_key=self.api_key)even after logging a warning that the key is missing. This may cause an immediate exception or a confusing deferred failure.Consider guarding the client creation:
Suggested fix
self.api_key = api_key or os.getenv("GEMINI_API_KEY") if not self.api_key: logger.warning("GEMINI_API_KEY not found. Google Search may fail.") - self.client = Client(api_key=self.api_key) + self.client = None + else: + self.client = Client(api_key=self.api_key)Then add an early return in
search:def search(self, ...): if not self.client: logger.error("Google Search client not initialized (missing API key).") return [] # ... rest of method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/providers/google_adapter.py` around lines 17 - 20, The adapter currently instantiates Client(api_key=self.api_key) even when api_key is None, risking exceptions; change GoogleAdapter to only create self.client when self.api_key is truthy (mirroring TavilyAdapter), e.g. set self.client = None if no key and otherwise Client(...), and update the search method to early-return an empty list (and log an error) when self.client is None; reference symbols: Client, self.api_key, search, and TavilyAdapter.backend/src/agent/state.py (1)
23-31:⚠️ Potential issue | 🔴 CriticalThe two
Tododefinitions are intentional but reveal a critical bug in the transformation logic.The
Todoinstate.py(TypedDict withid,task,status,result) is correctly separated from theTodointools_and_schemas.py(Pydantic withtitle,description,status) as they serve different purposes—one for LLM schema validation, one for internal state.However, the conversion between them is inconsistent and buggy:
generate_plan(line 382) correctly maps:item.title→"task",item.status→"status", adds"result": Noneupdate_plan(line 860) incorrectly maps:item.title→"title",item.description→"description", adds"query"— none of which match the expectedstate.TodoschemaThis inconsistency will cause the
plan_todosinupdate_planto contain dict objects that don't conform to thestate.Todostructure, leading to potential runtime errors when the state'stodo_listis accessed.Align the field mappings in
update_plan(line 860+) to match the correct transformation used ingenerate_plan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/state.py` around lines 23 - 31, The update_plan function currently builds plan_todos with the LLM/Pydantic fields (e.g., item.title→"title", item.description→"description", adding "query"), which mismatches the internal state.Todo shape; change update_plan's mapping to mirror generate_plan: map item.title → "task", item.status → "status", set "result": None (and include "id" if available or generate one), producing dicts that conform to state.Todo so plan_todos and todo_list consume the same schema as generate_plan.backend/src/agent/rag.py (2)
211-212:⚠️ Potential issue | 🔴 CriticalChroma-only mode can crash on uninitialized
next_id.
start_id = self.next_idexecutes even when FAISS is disabled, butself.next_idis only created in the FAISS init branch.Proposed fix
- start_id = self.next_id + start_id = self.next_id if self.use_faiss else 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/rag.py` around lines 211 - 212, The line "start_id = self.next_id" assumes self.next_id exists even when FAISS is disabled; update the initialization logic so self.next_id is always defined (e.g., set self.next_id = 0 in the non-FAISS initialization branch) or guard the assignment before use (e.g., in the method containing start_id = self.next_id, check hasattr(self, 'next_id') and fallback to 0), ensuring any code paths in the class (e.g., the RAG constructor or the method that sets start_id) reference a valid next_id regardless of whether FAISS was initialized.
520-547:⚠️ Potential issue | 🟠 MajorStub config currently routes traffic into a guaranteed failure path.
_RAGConfig.enabled = Trueroutes to RAG, butcreate_rag_tool()always returnsNone. This creates predictable error logging and wasted hops each run.Proposed fix
class _RAGConfig: - enabled = True - enable_fallback = True + enabled = False + enable_fallback = False max_documents = 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/rag.py` around lines 520 - 547, The config currently advertises RAG as enabled but the legacy stub always returns None, causing guaranteed failures; change _RAGConfig.enabled default to False and make is_rag_enabled() return rag_config.enabled (not a hardcoded True) so runtime checks reflect config, and update create_rag_tool(resources) to only log a warning or attempt creation when rag_config.enabled is True (otherwise return None silently or with a debug-level message); reference _RAGConfig.enabled, rag_config, is_rag_enabled, and create_rag_tool when making these fixes.backend/src/agent/configuration.py (1)
31-35:⚠️ Potential issue | 🟡 Minor
answer_modeldescription is inconsistent with the configured default.Line 32 defaults to
DEFAULT_ANSWER_MODEL(currently Gemma), but Line 34 says it uses Gemini 2.5 Pro. This metadata will mislead users and UI consumers.🔧 Proposed fix
answer_model: str = Field( default=DEFAULT_ANSWER_MODEL, json_schema_extra={ - "description": "The name of the language model to use for the agent's answer. Uses Gemini 2.5 Pro for highest quality synthesis with advanced reasoning." + "description": "The name of the language model to use for the agent's answer. Defaults to the configured DEFAULT_ANSWER_MODEL." }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/configuration.py` around lines 31 - 35, The json_schema_extra description for the Field answer_model currently claims "Uses Gemini 2.5 Pro", which is inconsistent with the configured DEFAULT_ANSWER_MODEL; update the description in the answer_model Field (where json_schema_extra is defined) to accurately reflect the actual default (DEFAULT_ANSWER_MODEL) or change DEFAULT_ANSWER_MODEL to match the described model—ensure the description references the DEFAULT_ANSWER_MODEL symbol or the correct model name so UI/consumers are not misled.backend/src/agent/models.py (1)
193-198:⚠️ Potential issue | 🟡 MinorTighten Gemini detection to avoid model-family misclassification.
Line 198 currently treats any model containing
"google"as Gemini. That can route non-Gemini Google models through Gemini-specific handling.🔧 Proposed fix
def is_gemini_model(model_name: str) -> bool: """Check if the model is a Gemini model (supports native tool binding).""" if not model_name: return False name = model_name.lower() - return "gemini" in name or "google" in name + return "gemini" in name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/models.py` around lines 193 - 198, The current is_gemini_model function is too broad because it treats any model_name containing "google" as Gemini; update is_gemini_model to only return True when the name clearly indicates the Gemini family by keeping the "gemini" check and replacing the naive "google" substring check with a stricter match (e.g., use a regex to match "google" as a vendor token or as a prefix with a delimiter such as '-', '/', ':' — for example match r'(^|[^a-zA-Z0-9])google(?=[^a-zA-Z0-9]|$)' or require model_name.startswith("google-")/startswith("google/") ) so non-Google models that merely contain the string "google" are not misclassified.backend/src/agent/kg.py (1)
49-53:⚠️ Potential issue | 🟠 MajorAllowlist domain check is too permissive (substring match).
any(allowed in domain ...)admits unintended domains. Use parsed host comparison with exact/suffix matching.💡 Proposed fix
+from urllib.parse import urlparse @@ - domain = url.split("//")[-1].split("/")[0] - - if not any(allowed in domain for allowed in app_config.kg_allowlist): + host = (urlparse(url).hostname or "").lower() + allowlist = tuple(d.lower() for d in app_config.kg_allowlist) + if not any(host == d or host.endswith(f".{d}") for d in allowlist): continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/kg.py` around lines 49 - 53, The current allowlist uses substring matching on domain (see url_match, domain, app_config.kg_allowlist) which permits unintended hosts; instead parse the URL host (use urllib.parse.urlparse and hostname/netloc) and compare against the allowlist using exact equality or a proper suffix check that enforces dot-boundaries (e.g., allowed == hostname or hostname.endswith("." + allowed)) to avoid substring matches that cross label boundaries and strip ports before comparison; replace the any(allowed in domain ...) logic with this hostname-based exact/suffix matching.backend/src/agent/router.py (1)
24-34:⚠️ Potential issue | 🟠 Major
agent_modeselection is effectively unreachable with current config parsing.
Configuration.from_runnable_configonly materializes declared model fields. Sinceagent_modeis not part of that model (per providedbackend/src/agent/configuration.pysnippet), this branch defaults to"parallel"and never routes to linear/supervisor.💡 Proposed fix
def select_agent(state: OverallState, config: RunnableConfig) -> str: - configurable = Configuration.from_runnable_config(config) - # Default to parallel if not specified - mode = getattr(configurable, "agent_mode", "parallel") + configurable = config.get("configurable", {}) if config else {} + mode = str(configurable.get("agent_mode", "parallel")).lower() if mode == "linear": return "linear_agent" elif mode == "supervisor": return "supervisor_agent" else: return "parallel_agent"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/router.py` around lines 24 - 34, select_agent currently reads agent_mode from Configuration.from_runnable_config which never includes agent_mode, so it always falls back to "parallel"; fix by reading agent_mode from the original RunnableConfig first (e.g., check getattr(config, "agent_mode", None)) and only fall back to Configuration.from_runnable_config or the default if not present, or alternatively add agent_mode as a declared field on the Configuration model so Configuration.from_runnable_config(materializes it); update select_agent to use the config-provided agent_mode (or the updated Configuration) to correctly route to "linear_agent" or "supervisor_agent".backend/src/agent/graphs/supervisor.py (1)
43-53:⚠️ Potential issue | 🟠 Major
compress_contextcannot prevent context bloat with additive reducer semantics.
web_research_resultis configured withoperator.add(additive reducer), so any returned list appends to existing state rather than replacing it. The function deduplicates results before returning, but the deduplication is negated when the additive reducer applies. For example:
- Existing state:
[A, B]- Function dedupes and returns:
[A, B, C]- After additive:
[A, B, A, B, C]← duplicates reappearThis defeats the node's stated purpose ("prevent context bloat" and "instead of appending to an ever-growing list"). Either replace the additive reducer with a custom reducer that truly replaces state, or redesign the node to work with append semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/graphs/supervisor.py` around lines 43 - 53, The current compress_context logic deduplicates combined = current_results + new_results but returns the full unique_results which, given the node's operator.add reducer, will be appended back and reintroduce duplicates; fix by either changing the reducer for the web_research_result node to a replacing reducer (so returned unique_results replaces prior state) or change compress_context to return only the delta (items from validated_web_research_result not already present in web_research_result) so additive semantics don't re-add prior items; locate the code handling state.get("validated_web_research_result") and state.get("web_research_result") in compress_context (and respect app_config.compression_enabled) and implement one of these two options.backend/src/observability/langfuse.py (1)
121-129:⚠️ Potential issue | 🟡 MinorUse bare
raiseto preserve traceback context.
raise eresets the traceback to this handler line, making it harder to trace where the exception actually originated. Since this is a simple re-raise with no exception transformation, use bareraiseinstead to keep the original stack intact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/observability/langfuse.py` around lines 121 - 129, In the except Exception as e handler (the block that comments about Langfuse's observe() re-raising exceptions), replace the current "raise e" with a bare "raise" so the original traceback is preserved when re-raising the exception from the observe()/yield path; keep the surrounding comment and behavior but use bare raise to avoid losing the original stack context.backend/src/agent/utils.py (1)
13-22:⚠️ Potential issue | 🟠 MajorHandle dict-style messages in
get_research_topic.
Line [14]assumes.contentexists; dict-backed messages can fail here. Also, dict entries are ignored in the multi-message loop, which can drop context.💡 Suggested fix
def get_research_topic(messages: List[AnyMessage]) -> str: """Get the research topic from the messages.""" + if not messages: + return "" + # check if request has a history and combine the messages into a single string if len(messages) == 1: - research_topic = messages[-1].content + message = messages[-1] + if isinstance(message, dict): + return str(message.get("content", "")) + return str(getattr(message, "content", "")) else: research_topic = "" for message in messages: - if isinstance(message, HumanMessage): + if isinstance(message, dict): + role = str(message.get("role", "user")).lower() + prefix = "Assistant" if role == "assistant" else "User" + research_topic += f"{prefix}: {message.get('content', '')}\n" + elif isinstance(message, HumanMessage): research_topic += f"User: {message.content}\n" elif isinstance(message, AIMessage): research_topic += f"Assistant: {message.content}\n" return research_topic🤖 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 13 - 22, get_research_topic currently assumes messages are objects with a .content attribute and ignores dict-style messages, which can raise exceptions and drop context; update the logic in get_research_topic to handle both object messages (HumanMessage, AIMessage) and dict messages by checking isinstance(message, dict) and extracting content via message.get('content') (and optionally role via message.get('role') to map to "User"/"Assistant"), and build the research_topic string for every message type so no entries are skipped and no attribute errors occur.backend/src/agent/orchestration.py (2)
120-128:⚠️ Potential issue | 🟠 MajorGate Tavily tool registration on API key availability.
Currently this can register a tool that fails at first invocation when
TAVILY_API_KEYis absent.Proposed guard
- from agent.research_tools import TAVILY_AVAILABLE, tavily_search_multiple + from agent.research_tools import ( + TAVILY_AVAILABLE, + get_tavily_api_key, + tavily_search_multiple, + ) if TAVILY_AVAILABLE: - self.register( - "tavily_search", - tavily_search_multiple, - description="Deep web search using Tavily API", - category="search", - ) + try: + get_tavily_api_key() + except ValueError: + logger.info("TAVILY_API_KEY not set; tavily_search tool not registered") + else: + self.register( + "tavily_search", + tavily_search_multiple, + description="Deep web search using Tavily API", + category="search", + )Based on learnings Integrate
tavilyweb research tool for enhanced capabilities (requires API key configuration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/orchestration.py` around lines 120 - 128, The current registration of the Tavily tool may succeed even when the API key is missing; update the guard around registering tavily_search_multiple so it only registers when both TAVILY_AVAILABLE and the Tavily API key/config is present (e.g., check the environment/config var used for the key such as TAVILY_API_KEY or the helper that validates it) before calling self.register("tavily_search", tavily_search_multiple, ...). Ensure the check is performed in orchestration.py near the existing TAVILY_AVAILABLE import so registration is skipped if the key is absent.
333-348:⚠️ Potential issue | 🟠 MajorCoordinator can choose
use_tool, but router never returnstools.This makes tool routing dead even when the coordinator explicitly selects a tool.
Proposed routing fix
-def create_task_router(agents: AgentPool): +def create_task_router(agents: AgentPool, tools: ToolRegistry | None = None): @@ def router(state: OverallState) -> str: decision = state.get("coordinator_decision", "delegate_agent") target = state.get("coordinator_target", "planner") @@ + if decision == "use_tool" and tools and target in tools.get_tool_names(): + return "tools" + # Map target to node name if target in agents.get_agent_names(): return f"agent_{target}" - - return "agent_planner" # Default + agent_names = agents.get_agent_names() + return f"agent_{agent_names[0]}" if agent_names else "finalize" @@ builder.add_conditional_edges( "coordinator", - create_task_router(agents), + create_task_router(agents, tools), route_targets, )Also applies to: 383-399, 473-477
backend/src/config/app_config.py (1)
58-63:⚠️ Potential issue | 🟠 MajorAvoid dual CORS config fields with different parsing semantics.
cors_originsandCORS_ORIGINSboth readCORS_ORIGINS, but they can produce different values (empty entries/whitespace handling differ). This creates two sources of truth for a security-critical setting.Proposed consolidation
- cors_origins: Tuple[str, ...] = tuple( - filter(None, os.getenv("CORS_ORIGINS", "http://localhost:5173").split(",")) - ) + cors_origins: Tuple[str, ...] = tuple( + origin.strip() + for origin in os.getenv("CORS_ORIGINS", "http://localhost:5173").split(",") + if origin.strip() + ) @@ - # Security Configuration - CORS_ORIGINS: List[str] = field( - default_factory=lambda: os.getenv( - "CORS_ORIGINS", "http://localhost:5173" - ).split(",") - ) + `@property` + def CORS_ORIGINS(self) -> List[str]: + # Backward-compatible alias for older call sites + return list(self.cors_origins)Also applies to: 80-84
🤖 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 58 - 63, The file defines two CORS parsing locations that read the same env var with different semantics causing inconsistent values; consolidate parsing by implementing a single helper (e.g., normalize_env_list) and use it for cors_origins and allowed_hosts so both read their respective env vars with identical trimming/empty-entry filtering behavior; update references to cors_origins and allowed_hosts to call that helper (and apply the same fix to the similar code at the other occurrence around the block referenced on lines 80-84) so whitespace is stripped and empty entries are removed consistently.backend/src/agent/tools_and_schemas.py (1)
101-104:⚠️ Potential issue | 🟡 MinorUse project logger in MCP error path instead of
Proposed logging fix
+import logging @@ +logger = logging.getLogger(__name__) @@ except Exception as e: # Log error or return empty list so app startup doesn't crash on optional tool load - print(f"Error loading MCP tools: {e}") + logger.exception("Error loading MCP tools: %s", e) return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/tools_and_schemas.py` around lines 101 - 104, Replace the print in the MCP tool error handler with the project logger: in the except Exception as e block (the "Error loading MCP tools" path) call the module's logger (e.g., logger.exception or logger.error with exc_info=True) to log the message and stack trace instead of print, and ensure the module imports/obtains that logger (e.g., from your logging setup or use process_logger if available) before returning [].
🟡 Minor comments (12)
backend/src/search/providers/duckduckgo_adapter.py-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorDefault value for
regiondiffers from base class signature.The base class
SearchProvider.searchdefinesregion: str | None = None, but this implementation usesregion: str | None = "wt-wt". This signature mismatch could cause unexpected behavior when callers rely on the abstract interface's default.Since line 33 already handles
None→"wt-wt"conversion, consider aligning the signature with the base class:Suggested fix
def search( self, query: str, max_results: int = 5, - region: str | None = "wt-wt", + region: str | None = None, time_range: str | None = None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/providers/duckduckgo_adapter.py` around lines 22 - 23, The duckduckgo_adapter.SearchProvider.search implementation sets region: str | None = "wt-wt" which conflicts with the base class SearchProvider.search default of None; change the parameter default to region: str | None = None and rely on the existing None → "wt-wt" conversion inside the function (around the code at line handling region fallback) so the subclass signature matches the abstract interface and callers see consistent defaults.backend/src/search/providers/bing_adapter.py-54-60 (1)
54-60:⚠️ Potential issue | 🟡 MinorHandle missing
'y'(year) time range mapping.The base class documents
time_rangesupporting"d","w","m", and"y", but this implementation only maps the first three. Whentime_range == "y", the freshness parameter is silently ignored. While Bing's API doesn't provide a native "Year" freshness value, you should explicitly handle this case by omitting the freshness parameter (which searches all time).if time_range: if time_range == "d": params["freshness"] = "Day" elif time_range == "w": params["freshness"] = "Week" elif time_range == "m": params["freshness"] = "Month" + elif time_range == "y": + # Bing doesn't have a direct "Year" option; omit freshness to search all time + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/providers/bing_adapter.py` around lines 54 - 60, The mapping for time_range in backend/src/search/providers/bing_adapter.py currently handles "d","w","m" but omits "y"; update the logic around the params dict (the time_range variable and params["freshness"] mapping) to explicitly handle time_range == "y" by leaving out the freshness parameter (i.e., do not set params["freshness"] for "y" or remove it if previously set) so that a year request falls back to searching all time.frontend/src/hooks/useAgentState.ts-112-114 (1)
112-114:⚠️ Potential issue | 🟡 MinorNormalize unknown error shapes before storing error state.
error.messageis not guaranteed for non-Errorthrow values; this can silently setundefinedand hide failures.Suggested fix
- onError: (error: any) => { - setError(error.message) - }, + onError: (error: any) => { + setError(error?.message ?? String(error)) + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useAgentState.ts` around lines 112 - 114, The onError handler in the useAgentState hook currently does setError(error.message) which can become undefined for non-Error throw values; change the handler to normalize the thrown value before storing it by deriving a safe string (e.g., check for error?.message, error?.toString(), or fall back to String(error)) and pass that normalized message into setError so unknown error shapes never set undefined. Reference: the onError callback in useAgentState and the setError state setter.frontend/src/components/ArtifactView.tsx-116-120 (1)
116-120:⚠️ Potential issue | 🟡 MinorHandle clipboard failures explicitly in copy action.
navigator.clipboard.writeTextreturns a promise that can reject; currently this can fail silently with an unhandled rejection.Suggested fix
- const handleCopy = () => { - navigator.clipboard.writeText(content) - setCopied(true) - setTimeout(() => setCopied(false), 2000) - } + const handleCopy = async () => { + try { + await navigator.clipboard.writeText(content) + setCopied(true) + setTimeout(() => setCopied(false), 2000) + } catch { + setCopied(false) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ArtifactView.tsx` around lines 116 - 120, The handleCopy function calls navigator.clipboard.writeText(content) but ignores the returned promise; update handleCopy to await or .then() the writeText call, only call setCopied(true) on success, and catch failures to handle them (e.g., set an error state, call a provided onError/logging function, or fallback to a legacy copy method), ensuring navigator.clipboard.writeText rejection is not left unhandled; reference the handleCopy function, the content variable, and setCopied state when implementing this change.backend/src/agent/rag.py-129-131 (1)
129-131:⚠️ Potential issue | 🟡 MinorWarning message contradicts the active code path.
This branch runs only when Chroma is available and initialized, but the log says Chroma is missing.
Proposed fix
- logger.warning( - "Dual write enabled but ChromaDB is missing. Writing to FAISS only." - ) + logger.info("Initialized Chroma store at %s", persist_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/rag.py` around lines 129 - 131, The warning message in rag.py ("Dual write enabled but ChromaDB is missing. Writing to FAISS only.") contradicts the branch it sits in (this branch executes when Chroma is available); update the log in the logger.warning call to accurately describe the active path (e.g., indicate dual-write is enabled and writes will go to both FAISS and Chroma/ChromaDB), keeping the log call at the same location so it reflects the real state used by the dual-write logic.backend/src/agent/deep_search_agent.py-75-79 (1)
75-79:⚠️ Potential issue | 🟡 MinorTighten JSON boundary detection before parsing.
The current end-index check can pass malformed bracket ranges and rely on broad exception fallback.
Proposed fix
- start = response.find('[') - end = response.rfind(']') + 1 - if start != -1 and end != -1: - return json.loads(response[start:end]) + start = response.find('[') + end = response.rfind(']') + if start != -1 and end != -1 and end > start: + return json.loads(response[start : end + 1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/deep_search_agent.py` around lines 75 - 79, Tighten the JSON boundary detection before parsing by ensuring the found ']' comes after the found '[' and by attempting to parse within a safe try/except; specifically, in the block that computes start = response.find('[') and end = response.rfind(']') and currently returns json.loads(response[start:end]) or fallback return [topic], change the checks to require start != -1, end != -1 and end > start, build the slice including the closing bracket (end+1) and wrap json.loads(...) in a try/except that falls back to return [topic] on any parsing error so malformed bracket ranges or bad JSON won't raise.frontend/src/components/__tests__/ArtifactView.test.tsx-41-53 (1)
41-53:⚠️ Potential issue | 🟡 MinorUse label-based button queries instead of positional indices, but the suggested diff needs refinement.
Lines 41, 51, and 61 couple tests to button DOM order; this is fragile since the maximize button (
hidden md:flex) may not exist on all screen sizes. The suggested refactor improves two buttons but has issues:
- Copy button: Use
screen.getByRole('button', { name: /copy content/i })✓- Close button: Use
screen.getByRole('button', { name: /close/i })✓- Maximize button: The aria-label is dynamic (
"Maximize"→"Restore size"after toggle), so the suggested pattern won't reliably find it post-click. UsegetByRole('button', { name: /maximize|restore size/i })or the stabletitleattribute instead.- className assertions: The suggested diff removes
md:w-1/2/left-0checks entirely, but these assertions are necessary to verify the maximize actually works—not just that the button exists.Also applies to: 61-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/__tests__/ArtifactView.test.tsx` around lines 41 - 53, The tests in ArtifactView.test.tsx are brittle because they select buttons by position; update the tests to query by accessible names instead: replace positional queries with screen.getByRole('button', { name: /copy content/i }) for the copy button and screen.getByRole('button', { name: /close/i }) for the close button; for the maximize toggle use a tolerant matcher like screen.getByRole('button', { name: /maximize|restore size/i }) or query by a stable title attribute to handle the dynamic aria-label; after clicking the maximize/restore control assert the actual DOM layout changes by checking the container or element className (e.g., verify presence/absence of md:w-1/2 and left-0) rather than only asserting the button exists, and update any tests referencing buttons[0] / buttons[buttons.length - 1] to use these named queries and class assertions for ArtifactView..github/workflows/validate-env.yml-27-30 (1)
27-30:⚠️ Potential issue | 🟡 MinorBackend env check should validate key assignment, not just string presence.
Line 29 can pass on comments or incidental text. Use an assignment-style regex (optionally commented) for a more reliable guard.
🔧 Proposed fix
- name: Check backend/.env.example has GEMINI_API_KEY run: | - if ! grep -q "GEMINI_API_KEY" backend/.env.example; then + if ! grep -Eq "^[[:space:]#]*GEMINI_API_KEY=" backend/.env.example; then echo "ERROR: backend/.env.example is missing GEMINI_API_KEY." exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/validate-env.yml around lines 27 - 30, The current check only looks for the string GEMINI_API_KEY and can hit comments; update the grep to use an assignment-style regex that matches a key assignment (optionally commented). Replace the simple grep in the "Check backend/.env.example has GEMINI_API_KEY" step with a regex check such as using grep -Eq and pattern '^\s*(#\s*)?GEMINI_API_KEY\s*=' so the script only passes when GEMINI_API_KEY is actually assigned (or an assignment is commented out).backend/src/agent/persistence.py-13-14 (1)
13-14:⚠️ Potential issue | 🟡 MinorSanitized thread IDs can collapse to an empty filename stem.
If
thread_idcontains only filtered characters, all such requests map toplans/.json.💡 Proposed fix
def _get_plan_path(thread_id: str) -> str: @@ safe_id = "".join(c for c in thread_id if c.isalnum() or c in ("-", "_")) + if not safe_id: + raise ValueError( + "thread_id must include at least one alphanumeric, '-' or '_' character" + ) return os.path.join(PLAN_DIR, f"{safe_id}.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/persistence.py` around lines 13 - 14, The sanitization can yield an empty safe_id (so all such calls map to "plans/.json"); in the function that builds the filename (using thread_id and safe_id and returning os.path.join(PLAN_DIR, f"{safe_id}.json")), detect when safe_id == "" and replace it with a safe fallback (e.g., a UUID, a hashed version of the original thread_id, or raise a clear ValueError) before joining the path; update any callers or docs if you choose a deterministic fallback so other code can locate the file reliably.backend/src/agent/persistence.py-28-28 (1)
28-28:⚠️ Potential issue | 🟡 Minor
updated_atis computed from the old file mtime, not the current save operation.This stores stale metadata on updates. Set it to current write time (or compute mtime after writing).
💡 Proposed fix
+import time @@ - "updated_at": os.path.getmtime(path) if os.path.exists(path) else None, + "updated_at": time.time(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/persistence.py` at line 28, The current code sets "updated_at" from os.path.getmtime(path) which may reflect the previous file mtime; update the save/update logic in backend/src/agent/persistence.py so "updated_at" is assigned the actual current write time by either calling os.path.getmtime(path) after the file write completes (and verifying os.path.exists(path)) or by using time.time() at the moment of the successful write; locate the code that writes to the file (references to path and the "updated_at" dict key) and replace the stale mtime read with a post-write mtime or current timestamp.backend/src/agent/mcp_server.py-172-185 (1)
172-185:⚠️ Potential issue | 🟡 MinorReport actual bytes written, not character count.
Line [184]currently returnslen(content), which is chars. For UTF-8 multibyte text, this misreports written bytes.💡 Suggested fix
return ToolResult( - success=True, data={"path": path, "bytes_written": len(content)} + success=True, data={"path": path, "bytes_written": len(content_bytes)} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/mcp_server.py` around lines 172 - 185, The code in the save/write routine currently reports bytes_written using len(content) (character count), which misreports UTF-8 multibyte data; change it to compute bytes_written = len(content_bytes) (the already-created UTF-8 encoded bytes) and return that value in the ToolResult (replace the use of len(content) with bytes_written) after p.write_text(..., encoding="utf-8") in the same block (refer to variables content_bytes, p.write_text, and the ToolResult return).frontend/src/components/ChatMessagesView.tsx-232-238 (1)
232-238:⚠️ Potential issue | 🟡 MinorCopy flow assumes
message.idalways exists, but rendering already handles missing IDs.The non-null assertion at Line 237 can push
undefinedinto copy-state tracking for messages that use fallback keys.Proposed safe ID threading
interface AiMessageBubbleProps { message: Message + messageId: string @@ const AiMessageBubble: React.FC<AiMessageBubbleProps> = memo( ({ message, + messageId, @@ typeof message.content === 'string' ? message.content : JSON.stringify(message.content), - message.id! + messageId ) } @@ interface MessageItemProps { message: Message + messageId: string @@ ({ message, + messageId, @@ <AiMessageBubble message={message} + messageId={messageId} @@ {messages.map((message, index) => { const isLast = index === messages.length - 1 + const messageId = message.id ?? `msg-${index}` @@ - key={message.id || `msg-${index}`} + key={messageId} message={message} + messageId={messageId}Also applies to: 305-312, 512-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChatMessagesView.tsx` around lines 232 - 238, The copy handler call currently uses a non-null assertion on message.id (handleCopy(..., message.id!)), which can pass undefined into your copy-state tracking; change the call to pass the same stable key used for rendering instead of forcing non-null—e.g., replace message.id! with (message.id ?? fallbackKey) where fallbackKey is the fallback/render key you already derive for the message (or the same stable key variable used as the React key), and remove the non-null assertion; apply the same pattern to the other handleCopy sites that mirror this call so copy-state always receives a defined, stable identifier.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (100)
.env.example.github/workflows/pr-check.yml.github/workflows/push-check.yml.github/workflows/validate-env.yml.pre-commit-config.yamlbackend/.env.examplebackend/examples/gemma_providers.pybackend/pyproject.tomlbackend/src/agent/__init__.pybackend/src/agent/_graph.pybackend/src/agent/app.pybackend/src/agent/configuration.pybackend/src/agent/deep_search_agent.pybackend/src/agent/gemma_client.pybackend/src/agent/graph.pybackend/src/agent/graph_builder.pybackend/src/agent/graphs/linear.pybackend/src/agent/graphs/parallel.pybackend/src/agent/graphs/planning.pybackend/src/agent/graphs/supervisor.pybackend/src/agent/graphs/upstream.pybackend/src/agent/kg.pybackend/src/agent/llm_client.pybackend/src/agent/mcp_client.pybackend/src/agent/mcp_config.pybackend/src/agent/mcp_persistence.pybackend/src/agent/mcp_server.pybackend/src/agent/memory_tools.pybackend/src/agent/models.pybackend/src/agent/nodes.pybackend/src/agent/orchestration.pybackend/src/agent/persistence.pybackend/src/agent/planning_router.pybackend/src/agent/rag.pybackend/src/agent/rag_nodes.pybackend/src/agent/rate_limiter.pybackend/src/agent/registry.pybackend/src/agent/research_tools.pybackend/src/agent/router.pybackend/src/agent/scoping_schema.pybackend/src/agent/security.pybackend/src/agent/state.pybackend/src/agent/tool_adapter.pybackend/src/agent/tools_and_schemas.pybackend/src/agent/utils.pybackend/src/config/__init__.pybackend/src/config/app_config.pybackend/src/config/validation.pybackend/src/evaluation/bench.pybackend/src/evaluation/data.pybackend/src/evaluation/deep_research_bench.pybackend/src/evaluation/metrics.pybackend/src/evaluation/mle_bench.pybackend/src/observability/config.pybackend/src/observability/langfuse.pybackend/src/rag/chroma_store.pybackend/src/search/__init__.pybackend/src/search/provider.pybackend/src/search/providers/bing_adapter.pybackend/src/search/providers/brave_adapter.pybackend/src/search/providers/duckduckgo_adapter.pybackend/src/search/providers/google_adapter.pybackend/src/search/providers/tavily_adapter.pybackend/src/search/router.pydocs/tasks/06_GEMMA_INTEGRATION.mdfrontend/.prettierrc.jsonfrontend/eslint.config.jsfrontend/package.jsonfrontend/src/App.tsxfrontend/src/components/ActivityTimeline.test.tsxfrontend/src/components/ActivityTimeline.tsxfrontend/src/components/ArtifactView.test.tsxfrontend/src/components/ArtifactView.tsxfrontend/src/components/ChatMessagesView.test.tsxfrontend/src/components/ChatMessagesView.tsxfrontend/src/components/ChatMessagesView_Accessibility.test.tsxfrontend/src/components/ChatMessagesView_Log.test.tsxfrontend/src/components/ChatMessagesView_Markdown.test.tsxfrontend/src/components/InputForm.test.tsxfrontend/src/components/InputForm.tsxfrontend/src/components/WelcomeScreen.tsxfrontend/src/components/__tests__/ActivityTimeline_Semantics.test.tsxfrontend/src/components/__tests__/ArtifactView.test.tsxfrontend/src/components/__tests__/WelcomeScreen.test.tsxfrontend/src/components/ui/badge.tsxfrontend/src/components/ui/button.tsxfrontend/src/components/ui/card.tsxfrontend/src/components/ui/input.tsxfrontend/src/components/ui/scroll-area.tsxfrontend/src/components/ui/select.tsxfrontend/src/components/ui/tabs.tsxfrontend/src/components/ui/textarea.tsxfrontend/src/global.cssfrontend/src/hooks/useAgentState.test.tsfrontend/src/hooks/useAgentState.tsfrontend/src/lib/utils.tsfrontend/src/main.tsxfrontend/src/test/setup.tspatch_rag.pyruff.toml
💤 Files with no reviewable changes (1)
- frontend/src/hooks/useAgentState.test.ts
Comment on lines
+94
to
+97
| for query in queries: | ||
| results = self.searcher.search(query) | ||
| for r in results: | ||
| notes.append(f"Source: {r.get('url', 'unknown')}\n{r.get('content', '')}") |
There was a problem hiding this comment.
Avoid blocking sync calls inside async research flow.
self.searcher.search(query) is synchronous and runs on the event loop thread. Offload to threads (or async provider) to prevent loop stalls.
Proposed fix
- # Parallel search
- for query in queries:
- results = self.searcher.search(query)
- for r in results:
- notes.append(f"Source: {r.get('url', 'unknown')}\n{r.get('content', '')}")
+ # Parallel search
+ search_tasks = [asyncio.to_thread(self.searcher.search, query) for query in queries]
+ search_results = await asyncio.gather(*search_tasks, return_exceptions=True)
+ for query, result in zip(queries, search_results):
+ if isinstance(result, Exception):
+ logger.error("Search failed for '%s': %s", query, result)
+ continue
+ for r in result:
+ notes.append(f"Source: {r.get('url', 'unknown')}\n{r.get('content', '')}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/deep_search_agent.py` around lines 94 - 97, The loop in
DeepSearchAgent (in deep_search_agent.py) calls the synchronous
self.searcher.search(query) inside an async flow, which can block the event
loop; change that call to run in a thread via asyncio.to_thread (or
loop.run_in_executor) so the blocking work is offloaded — e.g., replace results
= self.searcher.search(query) with results = await
asyncio.to_thread(self.searcher.search, query) (import asyncio at top) and keep
the rest of the logic that appends to notes unchanged.
Comment on lines
286
to
+289
| if read_source == "chroma" and self.use_chroma and CHROMA_AVAILABLE: | ||
| return self.retrieve_from_chroma(query, top_k, query_embedding=query_embedding) | ||
| return self.retrieve_from_chroma( | ||
| query, top_k, query_embedding=query_embedding | ||
| ) |
There was a problem hiding this comment.
Chroma retrieval path ignores subgoal_filter and min_score.
This breaks contract parity with FAISS retrieval and can return cross-subgoal / below-threshold chunks.
Proposed fix
- if read_source == "chroma" and self.use_chroma and CHROMA_AVAILABLE:
- return self.retrieve_from_chroma(
- query, top_k, query_embedding=query_embedding
- )
+ if read_source == "chroma" and self.use_chroma and CHROMA_AVAILABLE:
+ chroma_results = self.retrieve_from_chroma(
+ query, top_k=top_k * 2, query_embedding=query_embedding
+ )
+ filtered: List[Tuple[EvidenceChunk, float]] = []
+ for evidence, score in chroma_results:
+ if subgoal_filter and evidence.subgoal_id != subgoal_filter:
+ continue
+ if score < min_score:
+ continue
+ filtered.append((evidence, score))
+ return filtered[:top_k]📝 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 read_source == "chroma" and self.use_chroma and CHROMA_AVAILABLE: | |
| return self.retrieve_from_chroma(query, top_k, query_embedding=query_embedding) | |
| return self.retrieve_from_chroma( | |
| query, top_k, query_embedding=query_embedding | |
| ) | |
| if read_source == "chroma" and self.use_chroma and CHROMA_AVAILABLE: | |
| chroma_results = self.retrieve_from_chroma( | |
| query, top_k=top_k * 2, query_embedding=query_embedding | |
| ) | |
| filtered: List[Tuple[EvidenceChunk, float]] = [] | |
| for evidence, score in chroma_results: | |
| if subgoal_filter and evidence.subgoal_id != subgoal_filter: | |
| continue | |
| if score < min_score: | |
| continue | |
| filtered.append((evidence, score)) | |
| return filtered[:top_k] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/rag.py` around lines 286 - 289, The Chroma branch currently
calls retrieve_from_chroma without forwarding subgoal_filter and min_score,
causing it to ignore those constraints; update the call in the read_source ==
"chroma" branch to pass subgoal_filter and min_score (e.g.,
self.retrieve_from_chroma(query, top_k, query_embedding=query_embedding,
subgoal_filter=subgoal_filter, min_score=min_score)) and, if needed, adjust
retrieve_from_chroma's signature/implementation to accept and apply
subgoal_filter and min_score the same way FAISS retrieval does so results
respect subgoal membership and score threshold.
Comment on lines
18
to
21
| def router_node(state: OverallState, config: RunnableConfig): | ||
| """Routes to the appropriate sub-agent based on configuration.""" | ||
| # We simply pass the state through; routing happens in the conditional edge. | ||
| return {"messages": state.get("messages", [])} |
There was a problem hiding this comment.
Router pass-through is mutating messages instead of preserving state.
Returning {"messages": state.get("messages", [])} will re-emit existing messages and can duplicate them when reducers append state updates. Return an empty update from the router node instead.
💡 Proposed fix
def router_node(state: OverallState, config: RunnableConfig):
"""Routes to the appropriate sub-agent based on configuration."""
- # We simply pass the state through; routing happens in the conditional edge.
- return {"messages": state.get("messages", [])}
+ # Routing happens in the conditional edge; no state mutation needed here.
+ return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/router.py` around lines 18 - 21, The router_node function
is incorrectly re-emitting existing messages by returning {"messages":
state.get("messages", [])}, which can cause duplicate messages; modify
router_node (function symbol: router_node, types: OverallState, RunnableConfig)
to return an empty update (e.g., an empty dict) instead of returning the
messages so the router does not mutate or re-emit state—ensure you do not modify
or copy state inside router_node and simply return no-op update.
Comment on lines
+261
to
264
| # Check if path is protected. | ||
| # If protected_paths is empty, nothing is rate-limited (explicit opt-in). | ||
| is_protected = any(path.startswith(prefix) for prefix in self.protected_paths) | ||
|
|
There was a problem hiding this comment.
Default path check currently turns rate limiting off globally.
Line 263 returns False when protected_paths is empty (the default), so no requests are rate-limited. That conflicts with the constructor contract and weakens DoS protection.
🔒 Proposed fix
- # Check if path is protected.
- # If protected_paths is empty, nothing is rate-limited (explicit opt-in).
- is_protected = any(path.startswith(prefix) for prefix in self.protected_paths)
+ # If protected_paths is empty, rate-limit all paths.
+ is_protected = (
+ True
+ if not self.protected_paths
+ else any(path.startswith(prefix) for prefix in self.protected_paths)
+ )🤖 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 261 - 264, The current check sets
is_protected = any(path.startswith(prefix) for prefix in self.protected_paths)
which returns False when self.protected_paths is empty and effectively disables
rate limiting; change the logic so an empty self.protected_paths means “protect
all paths” (e.g., if not self.protected_paths: is_protected = True else evaluate
the startswith any-check) to satisfy the constructor contract and restore DoS
protection for self.protected_paths and the is_protected variable.
Comment on lines
157
to
+169
| if isinstance(arguments, str): | ||
| try: | ||
| arguments = json.loads(arguments) | ||
| except Exception: | ||
| pass | ||
| except json.JSONDecodeError: | ||
| logger.warning("Could not parse tool arguments as JSON; using raw string") | ||
|
|
||
| import uuid | ||
|
|
||
| call_id = f"call_{uuid.uuid4().hex[:8]}" | ||
|
|
||
| tool_calls.append({ | ||
| "name": name, | ||
| "args": arguments, | ||
| "id": call_id, | ||
| "type": "tool_call" | ||
| }) | ||
| tool_calls.append( | ||
| {"name": name, "args": arguments, "id": call_id, "type": "tool_call"} | ||
| ) |
There was a problem hiding this comment.
Drop malformed string arguments instead of passing them through.
If argument JSON parsing fails, the current code preserves a raw string. That can break downstream logic that expects dict arguments.
💡 Suggested fix
if isinstance(arguments, str):
try:
arguments = json.loads(arguments)
except json.JSONDecodeError:
logger.warning("Could not parse tool arguments as JSON; using raw string")
+ continue
+
+ if not isinstance(arguments, dict):
+ continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/tool_adapter.py` around lines 157 - 169, When
json.loads(arguments) raises json.JSONDecodeError in the parsing block around
the variable arguments, do not keep the raw string; instead replace arguments
with an empty dict (arguments = {}) or otherwise normalize to a dict before
appending to tool_calls so downstream logic that expects dicts won't break.
Update the except block (where logger.warning is called) to set arguments = {}
and include the original malformed value in the warning via logger.warning so
callers can see what failed; keep the rest of the logic that builds call_id and
appends to tool_calls unchanged (references: arguments, json.loads,
logger.warning, call_id, tool_calls, name).
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @MasumRab. The following files were modified: * `backend/examples/gemma_providers.py` * `backend/src/agent/_graph.py` * `backend/src/agent/app.py` * `backend/src/agent/configuration.py` * `backend/src/agent/deep_search_agent.py` * `backend/src/agent/gemma_client.py` * `backend/src/agent/graph.py` * `backend/src/agent/graph_builder.py` * `backend/src/agent/graphs/linear.py` * `backend/src/agent/graphs/supervisor.py` * `backend/src/agent/kg.py` * `backend/src/agent/llm_client.py` * `backend/src/agent/mcp_client.py` * `backend/src/agent/mcp_config.py` * `backend/src/agent/mcp_persistence.py` * `backend/src/agent/mcp_server.py` * `backend/src/agent/memory_tools.py` * `backend/src/agent/models.py` * `backend/src/agent/nodes.py` * `backend/src/agent/orchestration.py` * `backend/src/agent/persistence.py` * `backend/src/agent/planning_router.py` * `backend/src/agent/rag.py` * `backend/src/agent/rag_nodes.py` * `backend/src/agent/rate_limiter.py` * `backend/src/agent/registry.py` * `backend/src/agent/research_tools.py` * `backend/src/agent/router.py` * `backend/src/agent/security.py` * `backend/src/agent/state.py` * `backend/src/agent/tool_adapter.py` * `backend/src/agent/tools_and_schemas.py` * `backend/src/agent/utils.py` * `backend/src/config/app_config.py` * `backend/src/config/validation.py` * `backend/src/evaluation/bench.py` * `backend/src/evaluation/data.py` * `backend/src/evaluation/metrics.py` * `backend/src/evaluation/mle_bench.py` * `backend/src/observability/config.py` * `backend/src/observability/langfuse.py` * `backend/src/rag/chroma_store.py` * `backend/src/search/provider.py` * `backend/src/search/providers/bing_adapter.py` * `backend/src/search/providers/brave_adapter.py` * `backend/src/search/providers/duckduckgo_adapter.py` * `backend/src/search/providers/google_adapter.py` * `backend/src/search/providers/tavily_adapter.py` * `backend/src/search/router.py` * `frontend/src/App.tsx` * `frontend/src/components/ChatMessagesView.tsx` * `frontend/src/components/ui/badge.tsx` * `frontend/src/components/ui/button.tsx` * `frontend/src/components/ui/card.tsx` * `frontend/src/components/ui/input.tsx` * `frontend/src/components/ui/scroll-area.tsx` * `frontend/src/components/ui/select.tsx` * `frontend/src/components/ui/tabs.tsx` * `frontend/src/components/ui/textarea.tsx` * `frontend/src/hooks/useAgentState.ts` * `frontend/src/lib/utils.ts` These files were kept as they were: * `backend/src/evaluation/deep_research_bench.py` These files were ignored: * `frontend/src/components/ActivityTimeline.test.tsx` * `frontend/src/components/ArtifactView.test.tsx` * `frontend/src/components/ChatMessagesView.test.tsx` * `frontend/src/components/ChatMessagesView_Accessibility.test.tsx` * `frontend/src/components/ChatMessagesView_Log.test.tsx` * `frontend/src/components/ChatMessagesView_Markdown.test.tsx` * `frontend/src/components/InputForm.test.tsx` * `frontend/src/components/__tests__/ActivityTimeline_Semantics.test.tsx` * `frontend/src/components/__tests__/ArtifactView.test.tsx` * `frontend/src/components/__tests__/WelcomeScreen.test.tsx` * `frontend/src/hooks/useAgentState.test.ts` These file types are not supported: * `.env.example` * `.github/workflows/pr-check.yml` * `.github/workflows/push-check.yml` * `.github/workflows/validate-env.yml` * `.pre-commit-config.yaml` * `backend/.env.example` * `backend/pyproject.toml` * `docs/tasks/06_GEMMA_INTEGRATION.md` * `frontend/.prettierrc.json` * `frontend/package.json` * `frontend/src/global.css` * `ruff.toml`
|
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
backend/src/observability/langfuse.py (1)
125-133:⚠️ Potential issue | 🟡 MinorRe-raise with bare
raiseto keep original traceback.Current
raise eloses stack fidelity from the original failure site.♻️ Proposed fix
- except Exception as e: + except Exception: @@ - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/observability/langfuse.py` around lines 125 - 133, In the exception handler in backend/src/observability/langfuse.py (the except Exception as e block that currently does "raise e"), replace the explicit re-raise with a bare "raise" so the original traceback is preserved; keep the surrounding comments/logic intact and do not add additional exception handling so observe() exceptions continue to bubble with their original stack trace.backend/src/search/providers/tavily_adapter.py (1)
103-106:⚠️ Potential issue | 🟡 MinorUse bare
raiseto preserve the original traceback.Re-raising with
raise etruncates useful stack context.♻️ Proposed fix
- except Exception as e: + except Exception as e: logger.error(f"Tavily Search failed: {e}") # If tuned failed, we could try basic, but the router handles fallback. - raise e + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/providers/tavily_adapter.py` around lines 103 - 106, In the exception handler inside tavily_adapter (the try/except block that logs "Tavily Search failed"), replace the current re-raise using "raise e" with a bare "raise" so the original traceback is preserved; update the except block where logger.error(f"Tavily Search failed: {e}") is called to use bare raise after logging.backend/src/search/providers/google_adapter.py (2)
85-87:⚠️ Potential issue | 🟡 MinorPreserve traceback when propagating errors.
Use bare
raiseinstead ofraise eto keep original stack context.♻️ Proposed fix
- except Exception as e: + except Exception as e: logger.error(f"Google Search 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/search/providers/google_adapter.py` around lines 85 - 87, In the except block that currently does "except Exception as e: logger.error(f'Google Search failed: {e}') raise e" in google_adapter.py, replace the "raise e" with a bare "raise" so the original traceback is preserved when re-raising the exception; keep the logger.error call but do not change the caught variable name (e) so the message remains the same.
22-26:⚠️ Potential issue | 🟠 MajorGuard client construction and fix exception handling pattern.
The SDK will raise a
ValueErrorduringClient()initialization ifapi_keyisNoneandGOOGLE_API_KEY/GEMINI_API_KEYare not set, causing the adapter to fail at setup time rather than gracefully. For defensive programming, guard the client creation to fail fast with clear messaging.Additionally, line 87 uses the anti-pattern
raise e, which should be replaced with a bareraiseto preserve the exception chain.♻️ Proposed fix
self.api_key = api_key or os.getenv("GEMINI_API_KEY") if not self.api_key: logger.warning("GEMINI_API_KEY not found. Google Search may fail.") - self.client = Client(api_key=self.api_key) + self.client = Client(api_key=self.api_key) if self.api_key else Noneexcept Exception as e: logger.error(f"Google Search 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/search/providers/google_adapter.py` around lines 22 - 26, Guard Client construction in GoogleAdapter.__init__: check for a non-empty api_key (use api_key or os.getenv("GEMINI_API_KEY") as already assigned) and if missing log an error and raise a clear ValueError before calling Client(...) instead of letting Client() raise; only construct self.client = Client(api_key=self.api_key) when api_key is present. Also replace the anti-pattern "raise e" in the method that currently re-raises exceptions (referenced near the handler that catches exceptions around line 87) with a bare "raise" to preserve the original traceback/exception chain.backend/src/rag/chroma_store.py (1)
82-88:⚠️ Potential issue | 🟠 MajorProtect reserved metadata keys from being overridden.
Current merge order allows
e.metadatato overwrite canonical fields likesubgoal_idandsource_url, which can break retrieval semantics.♻️ Proposed fix
meta = { + **e.metadata, "source_url": e.source_url, "subgoal_id": e.subgoal_id, "relevance_score": float(e.relevance_score), "timestamp": float(e.timestamp), - **e.metadata, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/rag/chroma_store.py` around lines 82 - 88, The merge currently does meta = { ..., **e.metadata } which lets user metadata override reserved canonical fields (source_url, subgoal_id, relevance_score, timestamp) and breaks semantics; fix by preventing overrides—either merge e.metadata first and then set the canonical fields last (so canonical values win) or filter e.metadata to remove reserved keys before merging (e.g., drop keys "source_url", "subgoal_id", "relevance_score", "timestamp"), and update the meta construction in chroma_store.py (the meta dict creation that references e.metadata and fields like source_url, subgoal_id, relevance_score, timestamp) accordingly.backend/src/agent/tools_and_schemas.py (1)
139-141:⚠️ Potential issue | 🟡 MinorUse
os.makedirs(workspace_path, exist_ok=True)for robustness.The current code checks existence then creates, which has a TOCTOU race condition if multiple processes run concurrently. Using
exist_ok=Trueis atomic and handles this safely.Proposed fix
- if not os.path.exists(workspace_path): - os.makedirs(workspace_path) + os.makedirs(workspace_path, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/tools_and_schemas.py` around lines 139 - 141, Replace the two-step existence check and creation for the workspace directory with a single atomic call; remove the os.path.exists(workspace_path) conditional and the os.makedirs(workspace_path) call and instead call os.makedirs(workspace_path, exist_ok=True) where workspace_path is defined in tools_and_schemas.py to avoid TOCTOU races when creating the workspace directory.backend/src/agent/mcp_persistence.py (1)
34-49:⚠️ Potential issue | 🟡 MinorAvoid false success on missing
thread_id.At Lines 47-49, this returns success even when
save_plan()may no-op for an emptythread_id. Add an explicit upfront validation in this wrapper.💡 Proposed fix
def save_thread_plan( thread_id: str, todo_list: List[Dict[str, Any]], artifacts: Dict[str, Any] ) -> str: @@ + if not thread_id: + return "Error saving plan: missing thread_id" + try: save_plan(thread_id, todo_list, artifacts) return f"Plan saved successfully for thread {thread_id}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/mcp_persistence.py` around lines 34 - 49, The wrapper save_thread_plan should validate the thread_id before calling save_plan to avoid returning a false success when save_plan no-ops for an empty id; in save_thread_plan check that thread_id is non-empty/valid (e.g., if not thread_id: raise ValueError("thread_id is required") or return an error message), then proceed to call save_plan(thread_id, todo_list, artifacts) and return the success string only after validation and successful save; reference the save_thread_plan function and the save_plan call when making the change.backend/src/agent/configuration.py (1)
88-97:⚠️ Potential issue | 🟠 MajorAdd compatibility mapping for legacy config keys.
At Line 96, only exact field names are read. Existing callers using
model,num_queries, ormax_loopswill be ignored and silently fall back to defaults.💡 Proposed fix
- configurable = ( - config["configurable"] if config and "configurable" in config else {} - ) + configurable = ( + dict(config["configurable"]) if config and "configurable" in config else {} + ) + + legacy_aliases = { + "model": "query_generator_model", + "num_queries": "number_of_initial_queries", + "max_loops": "max_research_loops", + } + for legacy_key, new_key in legacy_aliases.items(): + if legacy_key in configurable and new_key not in configurable: + configurable[new_key] = configurable[legacy_key]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/configuration.py` around lines 88 - 97, The loop that reads values from configurable/os.environ only checks exact field names (using cls.model_fields), so legacy keys like "model", "num_queries", and "max_loops" are ignored; add a compatibility mapping step before the for-loop that translates legacy keys to current field names (e.g., map "model" -> "base_model" or the current field name, "num_queries" -> "num_retrievals" or the current field, and "max_loops" -> "max_iterations") and use that mapped dictionary when looking up values in configurable and environment, updating the lookup in the loop that references cls.model_fields and configurable so legacy keys are honored.backend/src/agent/graph.py (1)
43-43: 🛠️ Refactor suggestion | 🟠 MajorReplace
print()with logger for consistency.The module uses
print()for MCP-related messages while other parts of the codebase use the logging module. This creates inconsistent log levels and formatting.Suggested fix
+import logging + +logger = logging.getLogger(__name__) + # ... try: validate(mcp_settings) except ValueError as e: - print(f"WARN: MCP Configuration invalid: {e}") + logger.warning(f"MCP Configuration invalid: {e}") # ... if mcp_settings.enabled: - print(f"INFO: MCP Enabled with endpoint {mcp_settings.endpoint}") + logger.info(f"MCP Enabled with endpoint {mcp_settings.endpoint}")Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/graph.py` at line 43, Replace the plain print() calls used for MCP configuration errors in graph.py with the module logger: obtain or use the existing logger (e.g. logger = logging.getLogger(__name__)) and change the print(f"WARN: MCP Configuration invalid: {e}") occurrences to logger.warning("MCP Configuration invalid: %s", e) (and similarly replace the other print occurrence). Ensure both instances in the file that currently call print() for MCP-related messages are updated to use logger.warning for consistent log levels and formatting.backend/src/agent/rag.py (1)
170-199:⚠️ Potential issue | 🟠 Major
retrieve_from_chromashould accept and forwardsubgoal_filterandmin_scoreto maintain parity with FAISS retrieval.The method signature lacks
subgoal_filterandmin_scoreparameters, and the call toself.chroma.retrieve()on line 199 doesn't pass them. SinceChromaStore.retrievealready supports these parameters (per relevant code snippet), forwarding them here is cleaner than post-hoc filtering in the caller.Proposed fix
def retrieve_from_chroma( - self, query: str, top_k: int, query_embedding: List[float] | None = None + self, query: str, top_k: int, query_embedding: List[float] | None = None, + subgoal_filter: str | None = None, min_score: float = 0.0, ) -> List[Tuple[EvidenceChunk, float]]: ... # Map ChromaEvidenceChunk back to EvidenceChunk - results = self.chroma.retrieve(query, top_k=top_k, query_embedding=embedding) + results = self.chroma.retrieve( + query, top_k=top_k, query_embedding=embedding, + subgoal_filter=subgoal_filter, min_score=min_score, + ) return [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/rag.py` around lines 170 - 199, The retrieve_from_chroma method currently omits subgoal_filter and min_score and doesn't forward them to the Chroma store; update the function signature of retrieve_from_chroma to accept subgoal_filter: str | None = None and min_score: float | None = None (in addition to query_embedding), update the docstring to document these params, and pass them through to self.chroma.retrieve(...) so the call becomes self.chroma.retrieve(query, top_k=top_k, query_embedding=embedding, subgoal_filter=subgoal_filter, min_score=min_score); keep existing behavior when those args are None.
♻️ Duplicate comments (7)
backend/src/evaluation/metrics.py (1)
1-8:⚠️ Potential issue | 🔴 Critical
loggeris still undefined and will crash exception handling paths.Line 241 and Line 286 call
logger.warning(...), but no module-level logger is defined in this file. This raisesNameErrorexactly when fallback handling is needed.🐛 Proposed fix
import json +import logging import re from collections import Counter from difflib import SequenceMatcher from typing import Dict, List import numpy as np + +logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/evaluation/metrics.py` around lines 1 - 8, Add a module-level logger so the existing exception paths that call logger.warning(...) won't raise NameError: import the logging module at top and create logger = logging.getLogger(__name__) (or getLogger("evaluation.metrics")); ensure any existing logger.warning(...) uses that module-level logger and remove any undefined local references. This change will fix the calls around the existing warning sites (the logger.warning usages in the file) without altering the surrounding exception handling logic.backend/examples/gemma_providers.py (1)
252-279:⚠️ Potential issue | 🟠 MajorLazy model initialization is still not thread-safe.
Concurrent
generate()calls can race through_lazy_load()and trigger duplicate model download/load work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/examples/gemma_providers.py` around lines 252 - 279, The lazy loader in _lazy_load can race when generate() is called concurrently; add a per-instance lock (e.g., self._load_lock) created in __init__ and use double-checked locking inside _lazy_load: first check if self.llm is not None and return, then acquire self._load_lock, re-check self.llm, perform the kagglehub download and keras_nlp load only while holding the lock, set self.model_path and self.llm, and finally release the lock; this prevents duplicate downloads/loads while preserving the existing self.llm check and behavior.backend/src/agent/router.py (1)
18-26:⚠️ Potential issue | 🟠 MajorRouter node should not re-emit
messages.At Line 26, returning existing
messagescan duplicate entries through reducers. Router should return a no-op update.💡 Proposed fix
def router_node(state: OverallState, config: RunnableConfig): @@ - # We simply pass the state through; routing happens in the conditional edge. - return {"messages": state.get("messages", [])} + # Routing happens in the conditional edge; no state mutation here. + return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/router.py` around lines 18 - 26, The router_node currently re-emits the existing "messages" by returning {"messages": state.get("messages", [])}, which causes duplicate entries; update router_node (function name: router_node, types: OverallState, RunnableConfig) to perform a no-op update instead (e.g., return an empty dict or otherwise avoid returning the "messages" key) so routing decisions happen via edges only and reducers won't get duplicate messages.backend/src/agent/persistence.py (1)
47-49:⚠️ Potential issue | 🟠 MajorReplace
At Lines 48 and 72,
💡 Proposed fix
import json +import logging import os from typing import Any, Dict, List PLAN_DIR = "plans" +logger = logging.getLogger(__name__) @@ - except Exception as e: - print(f"Error saving plan for thread {thread_id}: {e}") + except Exception: + logger.exception("Error saving plan for thread %s", thread_id) @@ - except Exception as e: - print(f"Error loading plan for thread {thread_id}: {e}") + except Exception: + logger.exception("Error loading plan for thread %s", thread_id) return NoneAlso applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/persistence.py` around lines 47 - 49, Replace the bare print calls in the exception handlers in backend/src/agent/persistence.py with structured logging: ensure a module-level logger (e.g., logger = logging.getLogger(__name__)) is defined and change the except blocks that now do print(f"Error saving plan for thread {thread_id}: {e}") to use logger.exception(f"Error saving plan for thread {thread_id}") (or logger.error(..., exc_info=True)) so the error message and stack trace are captured; apply the same replacement to the other except block that prints errors (the one referencing thread_id / plan persistence).backend/src/agent/orchestration.py (1)
510-530:⚠️ Potential issue | 🔴 Critical
tools=Noneis accepted but dereferenced unconditionally.If
toolsisNone, line 566 (tools.get_tools()) will raiseAttributeError. Add a default initialization similar toagents.Proposed fix
def build_orchestrated_graph( tools: ToolRegistry | None = None, agents: AgentPool | None = None, coordinator_model: str = GEMINI_PRO, name: str = "orchestrated-agent", ) -> StateGraph: + if tools is None: + tools = ToolRegistry() if agents is None: agents = AgentPool()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/orchestration.py` around lines 510 - 530, The function build_orchestrated_graph accepts tools=None but later calls tools.get_tools(), causing an AttributeError when tools is None; mirror the agents handling by initializing a default ToolRegistry when tools is None (e.g., set tools = ToolRegistry()) before any dereference, so subsequent uses like tools.get_tools() or references to tools are safe; update the beginning of build_orchestrated_graph to ensure tools is a valid ToolRegistry instance whenever it is used.backend/src/agent/deep_search_agent.py (2)
128-132:⚠️ Potential issue | 🟠 MajorAvoid blocking sync calls inside async research flow.
self.searcher.search(query)is synchronous and runs on the event loop thread. Offload to threads to prevent loop stalls.Proposed fix
# Parallel search - for query in queries: - results = self.searcher.search(query) - for r in results: - notes.append(f"Source: {r.get('url', 'unknown')}\n{r.get('content', '')}") + search_tasks = [asyncio.to_thread(self.searcher.search, query) for query in queries] + search_results = await asyncio.gather(*search_tasks, return_exceptions=True) + for query, result in zip(queries, search_results): + if isinstance(result, Exception): + logger.error("Search failed for '%s': %s", query, result) + continue + for r in result: + notes.append(f"Source: {r.get('url', 'unknown')}\n{r.get('content', '')}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/deep_search_agent.py` around lines 128 - 132, The loop in deep_search_agent.py is calling the synchronous searcher.search(query) inside an async flow, blocking the event loop; change the code that iterates queries to offload the blocking call to a thread (e.g., use asyncio.to_thread or loop.run_in_executor) when invoking searcher.search, await the threaded task, then process results into notes as before (references: method using self.searcher.search and the loop that appends to notes). Ensure exceptions from the threaded call are propagated/handled and preserve existing extraction of 'url' and 'content' from each result.
58-60:⚠️ Potential issue | 🟠 MajorDo not fabricate search evidence when the provider is unavailable.
Returning
"dummy"results introduces false citations into downstream synthesis. Return an empty list (or explicit error metadata) instead.Proposed fix
if not self.tool: - logger.warning("Search tool not initialized. Returning dummy results.") - return [{"url": "dummy", "content": f"Dummy result for '{query}'"}] + logger.warning("Search tool not initialized; returning no results.") + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/deep_search_agent.py` around lines 58 - 60, The current branch in the method that checks self.tool logs a warning and returns fabricated dummy results (logger.warning + returning [{"url": "dummy", ...}]); change this to not fabricate evidence by returning an empty list ([]) or a clear error object instead of dummy results, and update the logger.warning message to reflect that no results are available because the search provider (self.tool) is uninitialized; ensure callers of this method can handle the empty list or error metadata returned.
🧹 Nitpick comments (19)
frontend/src/components/ChatMessagesView.tsx (1)
291-308: Comparator currently re-renders all items on copy changes.On Line 303, direct
copiedMessageIdcomparison forces everyMessageItemto re-render whenever any message is copied. Compare per-item copied state instead.💡 Proposed optimization
function areMessageItemPropsEqual(prev: MessageItemProps, next: MessageItemProps) { @@ const prevActivity = prev.message.id ? prev.historicalActivities[prev.message.id] : undefined const nextActivity = next.message.id ? next.historicalActivities[next.message.id] : undefined + const prevIsCopied = prev.copiedMessageId != null && prev.copiedMessageId === prev.message.id + const nextIsCopied = next.copiedMessageId != null && next.copiedMessageId === next.message.id return ( @@ - prev.copiedMessageId === next.copiedMessageId && + prevIsCopied === nextIsCopied &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ChatMessagesView.tsx` around lines 291 - 308, The comparator areMessageItemPropsEqual currently compares the global copiedMessageId which causes every MessageItem to re-render when any message is copied; change the copied check to a per-item boolean by comparing whether prev.copiedMessageId === prev.message.id and next.copiedMessageId === next.message.id (i.e., compute prevIsCopied and nextIsCopied) and use that in the return instead of prev.copiedMessageId === next.copiedMessageId so only the affected MessageItem re-renders.backend/src/evaluation/bench.py (1)
221-258: Avoid recomputing Pass@1 in the same query evaluation.Line 253 recomputes
pass_at_1_accuracy(...)even though the same metric is already computed earlier in the same method. Reuse the first result to reduce duplicate work.♻️ Proposed refactor
# Compute metrics + pass_at_1_result = self.metrics.pass_at_1_accuracy( + generated_answer=generated_answer, + reference_answer=reference["reference_answer"], + key_facts=reference["key_facts"], + ) + results = { "query_id": query_id, "query": query, "generated_answer": generated_answer, # Metric 1: Pass@1 Accuracy - "pass_at_1": self.metrics.pass_at_1_accuracy( - generated_answer=generated_answer, - reference_answer=reference["reference_answer"], - key_facts=reference["key_facts"], - ), + "pass_at_1": pass_at_1_result, # Metric 2: Evidence Quality "evidence_quality": self.metrics.evidence_quality_score( retrieved_docs=retrieved_docs, required_sources=reference["required_sources"], min_evidence_count=reference["min_evidence_count"], @@ "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=pass_at_1_result["score"], ), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/evaluation/bench.py` around lines 221 - 258, The code currently calls self.metrics.pass_at_1_accuracy(...) twice (once for "pass_at_1" and again when computing context_efficiency), so change it to call pass_at_1_accuracy once into a local variable (e.g., pass_at_1_result = self.metrics.pass_at_1_accuracy(...)), assign that variable to the "pass_at_1" field and pass pass_at_1_result["score"] to context_efficiency (instead of recomputing), leaving other metric calls unchanged; reference the functions pass_at_1_accuracy and context_efficiency and the variables generated_answer and reference["reference_answer"]/reference["key_facts"] to locate where to make the change.backend/src/agent/utils.py (1)
29-34: Non-human/non-assistant messages are silently dropped.In multi-message mode, message types outside the two explicit branches are omitted, which can lose context in the assembled topic string.
♻️ Proposed fix
for message in messages: if isinstance(message, HumanMessage): research_topic += f"User: {message.content}\n" elif isinstance(message, AIMessage): research_topic += f"Assistant: {message.content}\n" + else: + research_topic += f"{message.content}\n"🤖 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 29 - 34, The loop that builds research_topic currently only handles HumanMessage and AIMessage so other message types get dropped; in the messages iteration (the for message in messages loop) add a fallback branch that appends non-human/non-assistant messages to research_topic (include the message class name and its content or a safe stringified fallback) so types other than HumanMessage and AIMessage are preserved; update the logic around the research_topic construction to use message.__class__.__name__ and getattr(message, "content", str(message)) in the fallback.backend/examples/gemma_providers.py (1)
300-301: Normalizegenerate()output to ensure consistent string return across initialization variants.With
GemmaCausalLM.from_preset(), a preprocessor is attached by default, sogenerate()returnsstrfor single prompts. However, callingstr()directly is redundant and will produce unwanted text representation if the preprocessor is ever disabled (preprocessor=None) or if batch inputs are passed. The proposed fix is partially correct but incomplete—it handles list and numpy arrays but not the dict case that occurs withpreprocessor=None.output = self.llm.generate(prompt, max_length=max_length, **kwargs) - return str(output) + if isinstance(output, str): + return output + if isinstance(output, (list, tuple)) and output: + return str(output[0]) + if isinstance(output, dict) and "token_ids" in output: + # Handle preprocessor=None case (raw token output) + token_ids = output["token_ids"] + if hasattr(token_ids, "numpy"): + token_ids = token_ids.numpy() + if hasattr(token_ids, "tolist"): + token_ids = token_ids.tolist() + return str(token_ids) + if hasattr(output, "numpy"): + arr = output.numpy() + if hasattr(arr, "tolist"): + arr = arr.tolist() + if isinstance(arr, list) and arr: + return str(arr[0]) + return str(output)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/examples/gemma_providers.py` around lines 300 - 301, The generate() return value should be normalized to a consistent string: in the wrapper where you call self.llm.generate (used by GemmaCausalLM.from_preset and other initializers), detect common output shapes and convert them to a single string—if the result is a list or numpy.ndarray, take the first element (or join elements for multi-input cases if you intend batch-to-single behavior); if it is a dict (happens when preprocessor=None), extract the text field(s) (e.g., 'generated_text' or 'text') or take the first value; otherwise stringify safely. Update the code around the output = self.llm.generate(...) return str(output) to perform these type checks and normalize to a single string before returning.backend/src/agent/tools_and_schemas.py (1)
148-154: Add docstrings toread_file_wrapperandwrite_file_wrapperfor consistency.The
list_directory_wrapperhas a docstring, but the other two wrappers do not. Consider adding brief docstrings for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/tools_and_schemas.py` around lines 148 - 154, Add concise docstrings to both read_file_wrapper and write_file_wrapper matching the style of list_directory_wrapper: one-line summary of purpose, brief param descriptions for path (and content for write_file_wrapper), and return description indicating it returns file contents or an error string on failure; place the docstrings immediately under each async def to maintain consistency with existing wrappers and usage.backend/src/config/app_config.py (2)
40-48: Inconsistent use ofdefault_factoryvs direct assignment.Lines 40-48 use
field(default_factory=lambda: int(...))while other fields (e.g., line 14) use directos.getenv()calls. Both patterns work, but the inconsistency is confusing. For afrozendataclass where fields are set once at instantiation, direct assignment suffices:Proposed simplification
# Budgets & Performance - token_budget: int = field( - default_factory=lambda: int(os.getenv("TOKEN_BUDGET", "50000")) - ) - call_budget: int = field( - default_factory=lambda: int(os.getenv("CALL_BUDGET", "50")) - ) - latency_target_ms: int = field( - default_factory=lambda: int(os.getenv("LATENCY_TARGET_MS", "8000")) - ) + token_budget: int = int(os.getenv("TOKEN_BUDGET", "50000")) + call_budget: int = int(os.getenv("CALL_BUDGET", "50")) + latency_target_ms: int = int(os.getenv("LATENCY_TARGET_MS", "8000"))🤖 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 40 - 48, The token_budget, call_budget, and latency_target_ms fields use field(default_factory=...) while other fields use direct assignment; simplify by replacing these with direct assignments using int(os.getenv(...)) (e.g., set token_budget = int(os.getenv("TOKEN_BUDGET", "50000"))), keeping behavior identical for the frozen dataclass and referencing the same symbols token_budget, call_budget, and latency_target_ms so instantiation remains unchanged.
100-102: Use bareraiseinstead ofraise e.Consistent with other files, prefer bare
raiseto preserve the full exception traceback.Proposed fix
except ValueError as e: logger.error(f"Configuration loading 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/config/app_config.py` around lines 100 - 102, The except block currently does "except ValueError as e: logger.error(...); raise e" which re-raises the exception with a new traceback; replace the explicit "raise e" with a bare "raise" so the original traceback is preserved — update the except ValueError as e handler in app_config.py (the block that logs "Configuration loading failed: {e}") to use bare raise.backend/src/search/providers/bing_adapter.py (1)
114-116: Use bareraiseinstead ofraise e.Using
raise einstead of bareraisecan obscure the original traceback in some Python versions. Prefer bareraiseto preserve the full exception context.Proposed fix
except Exception as e: logger.error(f"Bing Search 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/search/providers/bing_adapter.py` around lines 114 - 116, In the except block inside the Bing adapter (where it currently does "except Exception as e:" and calls logger.error(f\"Bing Search failed: {e}\")), replace the explicit "raise e" with a bare "raise" so the original traceback/context is preserved; keep the logger.error call but change the re-raise to a bare raise to maintain full exception chaining in the function that performs the Bing search.backend/src/agent/llm_client.py (2)
64-66: Use bareraiseinstead ofraise e.Consistent with other files in this PR, prefer bare
raiseto preserve the full exception traceback.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 64 - 66, In the except Exception handler in backend/src/agent/llm_client.py (the block that logs "LLM call failed (attempting retry): {e}" and currently does "raise e"), change the re-raise to a bare "raise" so the original traceback is preserved; locate the except Exception as e block surrounding the LLM call (the code that uses logger.warning) and replace the explicit re-raise with a bare re-raise.
88-91: Consider adding a comment explaining the deferred import.The import inside
__init__is likely to avoid circular dependencies. Adding a brief comment would clarify this intent for future maintainers.Proposed clarification
+ # Deferred import to avoid circular dependency with tool_adapter from agent.tool_adapter import ( GEMMA_TOOL_INSTRUCTION, format_tools_to_json_schema, )🤖 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 88 - 91, Add a brief inline comment above the deferred import in LLMAgent.__init__ (the import of GEMMA_TOOL_INSTRUCTION and format_tools_to_json_schema from agent.tool_adapter) stating that the import is deferred to avoid circular imports and to ensure dependencies (e.g., agent.tool_adapter) are only loaded at runtime; reference the symbols GEMMA_TOOL_INSTRUCTION and format_tools_to_json_schema and mention that this prevents import-time cycles when initializing the LLMAgent.backend/src/agent/tool_adapter.py (1)
186-188: Moveuuidimport to module level.The
import uuidstatement is inside the loop, which runs on every iteration. While Python caches imports, this is non-idiomatic. Move the import to the top of the file with other imports.Proposed fix
At the top of the file (after line 6):
import uuidThen update line 186-188:
- import uuid - call_id = f"call_{uuid.uuid4().hex[:8]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/tool_adapter.py` around lines 186 - 188, The uuid import is inside the loop where call_id is created (call_id = f"call_{uuid.uuid4().hex[:8]}"); move the import to module level with the other imports by adding a top-of-file "import uuid" and remove the inline "import uuid" so the code uses uuid.uuid4() without importing inside the loop.backend/src/search/providers/brave_adapter.py (2)
56-57: Inconsistent error handling compared to BingAdapter.
BraveSearchAdapter.searchraisesValueErrorwhen API key is missing, whileBingAdapter.searchreturns an empty list. Consider aligning the behavior for consistency across search providers, or document this intentional difference clearly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/search/providers/brave_adapter.py` around lines 56 - 57, BraveSearchAdapter.search currently raises ValueError when self.api_key is missing, which is inconsistent with BingAdapter.search that returns an empty list; update BraveSearchAdapter.search to mirror BingAdapter.search behavior by returning an empty list ([]) when self.api_key is falsy, or if you prefer raising an error, update BingAdapter.search to raise the same ValueError — change the conditional in BraveSearchAdapter.search (the check of self.api_key) to return [] instead of raising, and ensure any callers expecting a list are unaffected.
93-95: Use bareraiseinstead ofraise e.Same as BingAdapter - prefer bare
raiseto preserve the full exception traceback.Proposed fix
except Exception as e: logger.error(f"Brave Search 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/search/providers/brave_adapter.py` around lines 93 - 95, The except block that currently logs with logger.error(f"Brave Search failed: {e}") and then does "raise e" should be changed to re-raise the original exception with a bare "raise" to preserve the full traceback; locate the except handling in brave_adapter.py (the block that logs "Brave Search failed") and replace the explicit re-raise with a bare raise while keeping the existing logging call.backend/src/agent/models.py (1)
105-111: Unify valid-model definitions to avoid drift.
is_valid_model()andALL_VALID_MODELScurrently duplicate the same model set. Keep one source of truth so future model additions don’t diverge.♻️ Proposed refactor
def is_valid_model(model_name: str) -> bool: @@ - valid_models = { - GEMINI_FLASH, - GEMINI_FLASH_LITE, - GEMINI_PRO, - GEMMA_2_27B_IT, - GEMMA_3_27B_IT, - } + valid_models = set(ALL_VALID_MODELS)Also applies to: 176-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/models.py` around lines 105 - 111, The code duplicates the same model set in multiple places (valid_models, ALL_VALID_MODELS, and is_valid_model()), causing drift; refactor to a single source of truth by defining one canonical constant (e.g., ALL_VALID_MODELS or VALID_MODELS) and have is_valid_model() and any other consumers reference that constant instead of duplicating the set; update references in the file (including the blocks around valid_models and the is_valid_model() implementation) to use the unified constant and remove the redundant set definitions.backend/src/search/providers/duckduckgo_adapter.py (1)
71-73: Bareraise eis redundant.Using
raise einstead of bareraiseprevents the interpreter from preserving the original traceback context in some edge cases. Consider using bareraise.Suggested fix
except Exception as e: logger.error(f"DuckDuckGo search 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/search/providers/duckduckgo_adapter.py` around lines 71 - 73, The except block in the DuckDuckGo adapter (see the exception handler in duckduckgo_adapter.py, around the logger.error call) currently uses "raise e" which can lose original traceback context; change it to a bare "raise" so the original exception traceback is preserved while still logging the error (i.e., keep logger.error(f\"DuckDuckGo search failed: {e}\") and replace "raise e" with a bare "raise").backend/src/search/router.py (1)
144-148: Fallback provider lacks retry logic.The primary provider gets two attempts (tuned=True, then tuned=False), but the fallback provider only gets a single attempt with default parameters. If the fallback also experiences a transient error, it will fail without retry.
Consider applying the same retry pattern to the fallback for consistency, or document this as intentional (single-shot fallback to avoid excessive latency).
Optional: Apply retry logic to fallback
if fallback_provider: - # Fallback gets the same retry logic or just a single shot? - # For simplicity, fallback is usually single shot untuned or standard. - # Let's try standard (tuned=True default) - return fallback_provider.search(query, max_results=max_results) + try: + return fallback_provider.search(query, max_results=max_results, tuned=True) + except Exception as e3: + logger.warning(f"Fallback search failed (tuned=True): {e3}") + return fallback_provider.search(query, max_results=max_results, tuned=False)🤖 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 144 - 148, The fallback_provider.search call currently uses a single default invocation; apply the same two-attempt retry pattern as the primary provider by calling fallback_provider.search(query, max_results=max_results, tuned=True) first and if that raises/returns an error then call fallback_provider.search(query, max_results=max_results, tuned=False) before propagating the error; update the error handling in the same function (where fallback_provider is used) to mirror the primary provider's try/except/logic so transient failures on fallback are retried once.backend/src/agent/gemma_client.py (1)
92-94: Bareraise eis redundant.Same pattern as in duckduckgo_adapter.py. Using bare
raisepreserves the original traceback better.Suggested fix
except Exception as e: logger.error(f"Vertex AI prediction 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 92 - 94, In the exception handler inside gemma_client.py (the except Exception as e block handling Vertex AI prediction failures), replace the redundant "raise e" with a bare "raise" so the original traceback is preserved; update the handler that currently logs via logger.error(f"Vertex AI prediction failed: {e}") to re-raise the exception using "raise" instead of re-raising the bound variable (mirroring the fix applied in duckduckgo_adapter.py).backend/src/agent/nodes.py (1)
992-993: Remove redundantimport jsonstatement.
jsonis already imported at module level (line 14). This local import is unnecessary.♻️ Proposed fix
# Heuristic: Find first { and last } - import json - start = content.find("{")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/agent/nodes.py` around lines 992 - 993, Remove the redundant local import of json in backend/src/agent/nodes.py: delete the inner "import json" (the one shown in the diff) since json is already imported at module level; ensure no other local references rely on a re-import and run tests/linters to confirm the unused import is gone.backend/src/agent/app.py (1)
354-360: Replacetraceback.print_exc()with structured logging.Using
traceback.print_exc()writes to stderr which may not be captured by the project's logging infrastructure. For consistent, configurable logging across environments, uselogger.exception()which automatically includes the traceback.♻️ Proposed fix
except Exception: - traceback.print_exc() + logger.exception("Error during SSE stream processing") # Security: Don't leak exception details to client yield ( f"event: error\n"Apply the same pattern at lines 390 and 427.
🤖 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 354 - 360, Replace traceback.print_exc() with structured logging by calling logger.exception(...) so the traceback is captured by the project's logging system; keep the same SSE error yield payload (the f"event: error..." block) to avoid leaking details to clients. Specifically, in the except Exception handlers that produce the SSE error response (the generator/function containing the yield of the error event), remove traceback.print_exc() and call logger.exception("Stream processing error") or similar contextual message. Apply the same replacement to the other identical except Exception blocks in this module that emit the SSE error (the two other catch blocks with the same yield pattern).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (61)
backend/examples/gemma_providers.pybackend/src/agent/_graph.pybackend/src/agent/app.pybackend/src/agent/configuration.pybackend/src/agent/deep_search_agent.pybackend/src/agent/gemma_client.pybackend/src/agent/graph.pybackend/src/agent/graph_builder.pybackend/src/agent/graphs/linear.pybackend/src/agent/graphs/supervisor.pybackend/src/agent/kg.pybackend/src/agent/llm_client.pybackend/src/agent/mcp_client.pybackend/src/agent/mcp_config.pybackend/src/agent/mcp_persistence.pybackend/src/agent/mcp_server.pybackend/src/agent/memory_tools.pybackend/src/agent/models.pybackend/src/agent/nodes.pybackend/src/agent/orchestration.pybackend/src/agent/persistence.pybackend/src/agent/planning_router.pybackend/src/agent/rag.pybackend/src/agent/rag_nodes.pybackend/src/agent/rate_limiter.pybackend/src/agent/registry.pybackend/src/agent/research_tools.pybackend/src/agent/router.pybackend/src/agent/security.pybackend/src/agent/state.pybackend/src/agent/tool_adapter.pybackend/src/agent/tools_and_schemas.pybackend/src/agent/utils.pybackend/src/config/app_config.pybackend/src/config/validation.pybackend/src/evaluation/bench.pybackend/src/evaluation/data.pybackend/src/evaluation/metrics.pybackend/src/evaluation/mle_bench.pybackend/src/observability/config.pybackend/src/observability/langfuse.pybackend/src/rag/chroma_store.pybackend/src/search/provider.pybackend/src/search/providers/bing_adapter.pybackend/src/search/providers/brave_adapter.pybackend/src/search/providers/duckduckgo_adapter.pybackend/src/search/providers/google_adapter.pybackend/src/search/providers/tavily_adapter.pybackend/src/search/router.pyfrontend/src/App.tsxfrontend/src/components/ChatMessagesView.tsxfrontend/src/components/ui/badge.tsxfrontend/src/components/ui/button.tsxfrontend/src/components/ui/card.tsxfrontend/src/components/ui/input.tsxfrontend/src/components/ui/scroll-area.tsxfrontend/src/components/ui/select.tsxfrontend/src/components/ui/tabs.tsxfrontend/src/components/ui/textarea.tsxfrontend/src/hooks/useAgentState.tsfrontend/src/lib/utils.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/ui/select.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/src/lib/utils.ts
- backend/src/evaluation/mle_bench.py
- backend/src/agent/security.py
- frontend/src/App.tsx
- backend/src/config/validation.py
- backend/src/observability/config.py
- frontend/src/hooks/useAgentState.ts
- backend/src/agent/graphs/linear.py
- frontend/src/components/ui/input.tsx
Comment on lines
383
to
390
| 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 in truncated response handling.
The while finish_reason == "length" loop will continue indefinitely if the model keeps returning truncated responses. Consider adding a maximum iteration limit to prevent runaway loops.
Suggested fix: Add iteration limit
# Handle truncated responses
response_metadata = getattr(result, "response_metadata", {})
finish_reason = response_metadata.get("finish_reason")
+ continuation_count = 0
+ max_continuations = 5
- while finish_reason == "length":
+ while finish_reason == "length" and continuation_count < max_continuations:
+ 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)
+
+ if continuation_count >= max_continuations:
+ logger.warning("Max continuations reached; answer may be truncated.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/_graph.py` around lines 383 - 390, The loop that handles
truncated LLM responses (while finish_reason == "length") can run forever if the
model keeps returning length-truncated continuations; add a maximum retry
counter (e.g., max_retries) and increment it each iteration, breaking the loop
and logging or raising an error after exceeding the limit. Modify the loop
around llm.invoke, continuation, final_answer and response_metadata to track
attempts, use a configurable cap, and ensure you still append the last
continuation before breaking so final_answer remains consistent.
Comment on lines
+66
to
+67
| ) -> StateGraph: | ||
| """Build a customized agent graph based on feature flags. | ||
|
|
||
| Args: | ||
| enable_planning: Include planning mode for user review (default: False) | ||
| enable_reflection: Include reflection loop for follow-up queries (default: True) | ||
| enable_validation: Include web result validation (default: True) | ||
| enable_compression: Include result compression (default: False) | ||
| enable_rag: Include RAG retrieval node (default: False) | ||
| enable_kg: Include Knowledge Graph enrichment (default: False) | ||
| parallel_search: Use parallel Send for web research (default: True) | ||
| name: Name for the compiled graph | ||
|
|
||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n backend/src/agent/graph_builder.py | head -80Repository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 3261
🏁 Script executed:
cat backend/src/agent/graph_builder.py | sed -n '160,180p' | cat -nRepository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 764
🏁 Script executed:
find . -name "*.txt" -o -name "*.toml" -o -name "*.cfg" | grep -E "(requirements|pyproject|setup)" | head -5Repository: MasumRab/gemini-fullstack-langgraph-quickstart
Length of output: 114
🌐 Web query:
langgraph 0.2.6 StateGraph.compile() return type CompiledStateGraph
💡 Result:
In LangGraph Python v0.2.6, StateGraph.compile() returns a CompiledStateGraph instance (the implementation constructs CompiledStateGraph(...) and returns it after validate()), even though some docs/type hints describe it more generically as a CompiledGraph. [1]
So you can treat the return value as:
- Runtime type:
CompiledStateGraph - Type relationship:
CompiledStateGraphis a specialized “compiled graph” (i.e., it behaves like a compiled runnable graph /CompiledGraph-style object). [1]
Typing example (Python):
from langgraph.graph import StateGraph
from langgraph.graph.state import CompiledStateGraph # where available in your version
compiled: CompiledStateGraph = StateGraph(State).compile()Source: LangGraph StateGraph.compile() reference + shown source snippet returning CompiledStateGraph. [1]
References:
[1] (baihezi.com)
Citations:
Return annotation should match the compiled graph type.
build_graph() returns builder.compile(...), which is a CompiledStateGraph instance in langgraph 0.2.6, not a raw StateGraph. Update the type annotation to reflect the actual return type.
♻️ Fix
from langgraph.graph import END, START, StateGraph
+from langgraph.graph.state import CompiledStateGraph
@@
-) -> StateGraph:
+) -> CompiledStateGraph:Also applies to: 172-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/graph_builder.py` around lines 66 - 67, The return type
annotation on build_graph (and the other occurrence noted) is incorrect: change
the annotated return type from StateGraph to CompiledStateGraph to match
builder.compile(...)'s actual return value; import CompiledStateGraph from the
langgraph package (or the module where it's exported) and update the function
signatures (e.g., build_graph) to use CompiledStateGraph so the type matches the
compiled graph instance.
Comment on lines
+44
to
+46
| results = state.get("validated_web_research_result", []) or state.get( | ||
| "web_research_result", [] | ||
| ) |
There was a problem hiding this comment.
Do not fall back to unvalidated results when validated output is empty.
At Line 44, using or means an empty validated_web_research_result falls back to raw web_research_result. That can bypass validation filtering and enrich KG with content that was intentionally rejected.
💡 Proposed fix
- results = state.get("validated_web_research_result", []) or state.get(
- "web_research_result", []
- )
+ if "validated_web_research_result" in state:
+ results = state.get("validated_web_research_result") or []
+ else:
+ results = state.get("web_research_result", [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/kg.py` around lines 44 - 46, The current assignment to
results uses "or" so an empty validated_web_research_result will fall back to
web_research_result; change the logic to prefer the validated key only if it
exists in state (even if it's an empty list) and otherwise fall back to
web_research_result. Concretely, replace the expression that computes results
with a conditional that checks for the presence of the
"validated_web_research_result" key in state (e.g., using "if
'validated_web_research_result' in state") and assigns
state["validated_web_research_result"] when present, otherwise assigns
state.get("web_research_result", []); reference the results variable and the
keys "validated_web_research_result" and "web_research_result" to locate where
to change it.
Comment on lines
+226
to
229
| p.write_text(content, encoding="utf-8") | ||
| return ToolResult( | ||
| success=True, | ||
| data={"path": path, "bytes_written": len(content)} | ||
| success=True, data={"path": path, "bytes_written": len(content)} | ||
| ) |
There was a problem hiding this comment.
bytes_written reports character count, not actual bytes written.
Line 228 uses len(content) which counts characters, but content_bytes (which may differ for multi-byte UTF-8 characters) was already computed on line 216. For accuracy, report the actual bytes written.
🔧 Proposed fix
p.write_text(content, encoding="utf-8")
return ToolResult(
- success=True, data={"path": path, "bytes_written": len(content)}
+ success=True, data={"path": path, "bytes_written": len(content_bytes)}
)📝 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
| p.write_text(content, encoding="utf-8") | |
| return ToolResult( | |
| success=True, | |
| data={"path": path, "bytes_written": len(content)} | |
| success=True, data={"path": path, "bytes_written": len(content)} | |
| ) | |
| p.write_text(content, encoding="utf-8") | |
| return ToolResult( | |
| success=True, data={"path": path, "bytes_written": len(content_bytes)} | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/mcp_server.py` around lines 226 - 229, The return value
currently reports character count using len(content); change it to report actual
bytes written by using the already-computed content_bytes size (e.g., use
len(content_bytes) or content_bytes.__len__()) in the ToolResult for
bytes_written so multi-byte UTF-8 characters are counted correctly; update the
return in the function/method that calls p.write_text (referenced in
mcp_server.py around the write block) to use content_bytes instead of content.
Comment on lines
282
to
+287
| async def start(self): | ||
| # In a real MCP server, this would start stdio/sse transport. | ||
| # For this implementation (in-process usage in notebooks), it's a no-op | ||
| # or sets up internal state. | ||
| pass | ||
| """Start the MCP server. | ||
|
|
||
| In a real MCP server, this would start stdio/sse transport. | ||
| For this implementation (in-process usage), it is intentionally a no-op. | ||
| """ |
There was a problem hiding this comment.
Missing method body causes IndentationError.
The start method has a docstring but no executable statement. Python requires at least a pass statement for empty method bodies.
🐛 Proposed fix
async def start(self):
"""Start the MCP server.
In a real MCP server, this would start stdio/sse transport.
For this implementation (in-process usage), it is intentionally a no-op.
"""
+ pass📝 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
| async def start(self): | |
| # In a real MCP server, this would start stdio/sse transport. | |
| # For this implementation (in-process usage in notebooks), it's a no-op | |
| # or sets up internal state. | |
| pass | |
| """Start the MCP server. | |
| In a real MCP server, this would start stdio/sse transport. | |
| For this implementation (in-process usage), it is intentionally a no-op. | |
| """ | |
| async def start(self): | |
| """Start the MCP server. | |
| In a real MCP server, this would start stdio/sse transport. | |
| For this implementation (in-process usage), it is intentionally a no-op. | |
| """ | |
| pass |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/agent/mcp_server.py` around lines 282 - 287, The async start
method currently contains only a docstring and no executable statements, which
raises an IndentationError; add a minimal body (e.g., a single pass or return)
inside the async def start(self): method in backend/src/agent/mcp_server.py (the
start coroutine) so the method is syntactically valid while preserving the
intended no-op behavior.
Owner
Author
|
@jules Resolve conflicts: Avoid full repo diff - focus only on your changed paths. |
Owner
Author
|
Jules Session Analysis: This PR has merge conflicts. Recommended fix: |
Owner
Author
|
@jules your gemini and gemma integration changes may need to be updated |
Resolved conflicts: - For docs/tasks/06_GEMMA_INTEGRATION.md: kept PR version with Kaggle integration - For backend/examples/gemma_providers.py: kept PR version with KaggleGemmaClient - For all other files: accepted upstream/main version Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add thread-safety to KaggleGemmaClient with double-checked locking - Fix validate-env.yml path pattern to include subdirectories - Fix deep_search_agent to return empty list instead of dummy results - Use asyncio.to_thread for non-blocking search calls in deep_search_agent - Add dedicated scoping_wait node in graph.py - Apply confidence_threshold in metrics.py when counting completed subgoals - Coerce metadata values to float in chroma_store.py - Populate raw_content field in SearchResult for tavily_adapter - Fix copy button gating and timeout race in ChatMessagesView.tsx Co-authored-by: openhands <openhands@all-hands.dev>
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 📃 Configuration Change RequirementsMergify configuration change
|
…h fixes - Add thread-safety to KaggleGemmaClient with double-checked locking - Fix validate-env.yml path pattern to include subdirectories - Fix deep_search_agent to return empty list instead of dummy results - Use asyncio.to_thread for non-blocking search calls in deep_search_agent - Add dedicated scoping_wait node in graph.py - Apply confidence_threshold in metrics.py when counting completed subgoals - Coerce metadata values to float in chroma_store.py - Populate raw_content field in SearchResult for tavily_adapter - Fix copy button gating and timeout race in ChatMessagesView.tsx Co-authored-by: openhands <openhands@all-hands.dev>
The SubGoal class was accidentally removed during merge conflict resolution but is still referenced in the research() method. Co-authored-by: openhands <openhands@all-hands.dev>
|
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
2 participants
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.
PR created automatically by Jules for task 9750286105324430980 started by @MasumRab
Summary by Sourcery
Add a Kaggle-based Gemma client example and extend the Gemma integration plan to cover Kaggle Models and related configuration.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores