Revamp: agents revamped!! #269
Conversation
WalkthroughThis pull request restructures the chat and agent framework by introducing an abstract Changes
Sequence Diagram(s)Chat Agent Execution FlowsequenceDiagram
participant C as Client
participant AC as AgentsController
participant AS as AgentsService
participant AR as AutoRouterAgent
participant CA as ChatAgent (e.g., AdaptiveAgent)
C->>AC: Send chat query with context
AC->>AS: Delegate query execution
AS->>AR: Route query based on classification
AR->>CA: Invoke run/run_stream
CA-->>AR: Return ChatAgentResponse
AR-->>AS: Forward response
AS-->>AC: Return aggregated result
AC-->>C: Deliver final response
Custom Agent Execution FlowsequenceDiagram
participant C as Client
participant CAC as CustomAgentController
participant CAS as CustomAgentService
participant EX as AgentExecutor
participant LLM as LanguageModel
C->>CAC: Request custom agent execution
CAC->>CAS: Validate and fetch custom agent
CAS->>EX: Initialize AgentExecutor (if not already)
EX->>LLM: Process query with tools and context
LLM-->>EX: Stream response chunks
EX-->>CAS: Return full agent output
CAS-->>CAC: Provide execution result
CAC-->>C: Deliver custom agent response
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (2)
app/modules/intelligence/agents_copy/chat_agents/simple_llm_agent.py (1)
36-44: 🛠️ Refactor suggestionAdd error handling for stream processing.
Similar to the
runmethod, error handling should be added for the streaming implementation.Apply this diff to add error handling:
async def run_stream( self, query: str, history: List[str], node_ids: Optional[List[str]] = None, ) -> AsyncGenerator[ChatAgentResponse, None]: - async for chunk in self.chain.astream({"query": query, "history": history}): - yield chunk + try: + async for chunk in self.chain.astream({"query": query, "history": history}): + yield chunk + except Exception as e: + logger.error(f"Error streaming chain response: {e}") + raisetest.py (1)
106-119: 🛠️ Refactor suggestionStructure test cases properly with assertions.
The current test execution lacks proper structure and assertions. Consider converting this into proper test cases:
-res = await auto_router_agent.run( - query="""What components will be affected if i update PromptService?""", - history=[], -) -print(res.response, res.citations) +async def test_auto_router_agent(): + # Given + query = "What components will be affected if i update PromptService?" + + # When + res = await auto_router_agent.run(query=query, history=[]) + + # Then + assert res.response, "Response should not be empty" + assert res.citations, "Citations should be provided" + +async def test_simple_llm_agent_streaming(): + # Given + query = "who built the taj mahal?" + expected_chunks = 0 + + # When + async for chunk in simple_llm_agent.run_stream(query, history=[]): + # Then + assert chunk.response, "Chunk response should not be empty" + expected_chunks += 1 + + assert expected_chunks > 0, "Should receive at least one chunk" -# res = simple_llm_agent.run_stream("who built the taj mahal?", history=[]) -# async for chunk in res: -# print(chunk.response, chunk.citations) -# print("==============") -asyncio.run(go()) +async def main(): + await test_auto_router_agent() + await test_simple_llm_agent_streaming() + +if __name__ == "__main__": + asyncio.run(main())
🧹 Nitpick comments (18)
app/modules/intelligence/agents_copy/chat_agent.py (1)
6-14: Consider adding a docstring forChatAgentResponse.While the fields have descriptions, adding a brief class-level docstring can further clarify its intended usage.
app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py (1)
8-8: Remove unused import.
code_gen_agentis imported but never used. Removing unused imports helps maintain clean and efficient code.-from .system_agents import blast_radius_agent, code_gen_agent +from .system_agents import blast_radius_agent🧰 Tools
🪛 Ruff (0.8.2)
8-8:
.system_agents.code_gen_agentimported but unusedRemove unused import:
.system_agents.code_gen_agent(F401)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (4)
1-1: Remove unused importsAnyandCoroutine.They are never referenced in the code, so removing them can reduce clutter.
-from typing import Any, AsyncGenerator, Coroutine, List +from typing import AsyncGenerator, List🧰 Tools
🪛 Ruff (0.8.2)
1-1:
typing.Anyimported but unusedRemove unused import
(F401)
1-1:
typing.Coroutineimported but unusedRemove unused import
(F401)
2-2: Remove unused importBaseModel.This import is never used, so it should be removed for cleanliness.
-from pydantic import BaseModel🧰 Tools
🪛 Ruff (0.8.2)
2-2:
pydantic.BaseModelimported but unusedRemove unused import:
pydantic.BaseModel(F401)
3-3: Remove unused importEnum.It's imported but not used, so removing it keeps the code tidy.
-from enum import Enum🧰 Tools
🪛 Ruff (0.8.2)
3-3:
enum.Enumimported but unusedRemove unused import:
enum.Enum(F401)
54-54: Useis Noneinstead of== None.When comparing against
None,is Noneis the preferred Pythonic approach.-if system_prompt == None: +if system_prompt is None:🧰 Tools
🪛 Ruff (0.8.2)
54-54: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
app/modules/intelligence/agents_copy/plan.py (3)
1-2: Consider removing or restructuring this file.
The file header explicitly states, "Delete this file, this is temporary planning". If you only used it as a placeholder, consider removing it or merging its content into the relevant modules before finalizing this PR.
63-88: Complete or remove unimplemented methods inAgentsService.
All methods (list_agents,get_agent, etc.) lack an implementation and only contain placeholders. If this service is part of your final design, implement these or raiseNotImplementedErrorfor clarity.🧰 Tools
🪛 Ruff (0.8.2)
66-66: Undefined name
Session(F821)
98-110: Add implementation details or remove the stub forAgentBuilderService.
Currently,AgentBuilderServiceprovides no actionable logic inbuild_agent. Ensure to either implement agent-building logic or remove/mark it as a TODO.🧰 Tools
🪛 Ruff (0.8.2)
104-104: Undefined name
Session(F821)
app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py (1)
82-124: Be mindful of prompt size and maintainability.
While theclassification_promptis informative, large inline multi-step instructions can be harder to maintain. Consider externalizing to a prompt template file or a constant in a dedicated module if it grows further.app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (3)
60-70: Add robust checks for tool properties
If a tool is missing eithername,description, orargs_schema, the code could produce incomplete descriptions. While there's a basic fallback check forargs_schema, consider proactively validating the presence of critical attributes for improved safety and clarity.
132-139: Add a docstring to improve maintainability
Therunmethod would benefit from a short docstring describing parameters, return type, and behavior. This helps collaborators quickly understand usage and sets expectations for future maintenance.🧰 Tools
🪛 Ruff (0.8.2)
138-138: Undefined name
anext. Consider specifyingrequires-python = ">= 3.10"ortool.ruff.target-version = "py310"in yourpyproject.tomlfile.(F821)
168-169: Improve logging clarity
Currently, the log shows"Error in run method"when the error is raised insiderun_stream. Updating the log message to the correct method name makes debugging easier.Apply this diff to align the log message with the actual method:
- logger.error(f"Error in run method: {str(e)}", exc_info=True) + logger.error(f"Error in run_stream method: {str(e)}", exc_info=True)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (1)
10-10: Class name and file name mismatch
The file is nameddebug_agent.py, yet definesclass CodeGenAgent. Consider renaming either the file tocode_gen_agent.pyor the class toDebugAgentfor clarity and consistency.app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (2)
47-54: Suggest explicit type annotation for tools
Similar to other agents, consider givingtoolsa more specific type annotation instead ofList[Any]. Providing a clear interface or base class for these tool objects can prevent accidental misuse and improve codebase readability.
79-229: Double-check placeholder usage
Yourcode_gen_task_promptreferences placeholders (like{query},{project_id}, etc.) in a triple-quoted string. If they need runtime substitution, convert it to an f-string or another templating strategy. Otherwise, the placeholders remain literal and may confuse users.app/modules/intelligence/agents_copy/chat_agents/simple_llm_agent.py (1)
15-27: Consider validating inputs and clarifying LLM usage.A few suggestions for improvement:
- The constructor should validate that
prompt_templateis not None or empty.- The large LLM is initialized but never used.
- Consider making the output format instructions configurable rather than hardcoding them.
Apply this diff to add input validation and make the format instructions configurable:
def __init__(self, llm_provider: ProviderService, prompt_template: str): + if not prompt_template or not prompt_template.strip(): + raise ValueError("prompt_template cannot be empty") self.llm_large = llm_provider.get_large_llm(AgentType.LANGCHAIN) self.llm_small = llm_provider.get_small_llm(AgentType.LANGCHAIN) + self.format_instructions = "\noutput format: {format_instructions}" self.chain = self._create_chain(prompt_template) def _create_chain(self, prompt_template: str) -> RunnableSequence: parser = PydanticOutputParser(pydantic_object=ChatAgentResponse) prompt_with_parser = ChatPromptTemplate.from_template( - template=prompt_template + "output format: {format_instructions}", + template=prompt_template + self.format_instructions, partial_variables={"format_instructions": parser.get_format_instructions()}, )test.py (1)
87-104: Improve agent configuration and validation.
- Agent descriptions should be moved to constants or configuration files.
- The
curr_agent_idshould be validated against available agent IDs.Apply this diff to add validation:
+AGENT_CONFIGS = [ + AgentWithInfo( + id="code_changes_agent", + name="Code Changes Agent", + description="An agent specialized in generating blast radius of the code changes in your current branch compared to default branch. Use this for functional review of your code changes. Works best with Py, JS, TS", + agent=None, # Will be set during initialization + ), + AgentWithInfo( + id="code_review_agent", + name="Code Review Agent", + description="An agent specialized in code reviews, use this for reviewing codes", + agent=None, # Will be set during initialization + ), +] auto_router_agent = AutoRouterAgent( llm_provider, - agents=[ - AgentWithInfo( - id="code_changes_agent", - name="Code Changes Agent", - description="An agent specialized in generating blast radius of the code changes in your current branch compared to default branch. Use this for functional review of your code changes. Works best with Py, JS, TS", - agent=adaptive_agent, - ), - AgentWithInfo( - id="code_review_agent", - name="Code Review Agent", - description="An agent specialized in code reviews, use this for reviewing codes", - agent=simple_llm_agent, - ), - ], + agents=[ + dataclasses.replace(config, agent=adaptive_agent if config.id == "code_changes_agent" else simple_llm_agent) + for config in AGENT_CONFIGS + ], curr_agent_id="code_changes_agent", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/modules/intelligence/agents_copy/chat_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/custom_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/simple_llm_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py(1 hunks)app/modules/intelligence/agents_copy/plan.py(1 hunks)test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/modules/intelligence/agents_copy/chat_agents/custom_agent.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test.py
56-56: Local variable agent is assigned to but never used
Remove assignment to unused variable agent
(F841)
app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py
8-8: .system_agents.code_gen_agent imported but unused
Remove unused import: .system_agents.code_gen_agent
(F401)
41-41: Undefined name simple_llm_agent
(F821)
app/modules/intelligence/agents_copy/plan.py
52-52: Undefined name Session
(F821)
59-59: Undefined name Session
(F821)
66-66: Undefined name Session
(F821)
94-94: Undefined name LLMConfig
(F821)
95-95: Undefined name AgentType
(F821)
104-104: Undefined name Session
(F821)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
1-1: typing.Any imported but unused
Remove unused import
(F401)
1-1: typing.Coroutine imported but unused
Remove unused import
(F401)
2-2: pydantic.BaseModel imported but unused
Remove unused import: pydantic.BaseModel
(F401)
3-3: enum.Enum imported but unused
Remove unused import: enum.Enum
(F401)
14-14: Redefinition of unused AgentType from line 10
(F811)
54-54: Comparison to None should be cond is None
Replace with cond is None
(E711)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
138-138: Undefined name anext. Consider specifying requires-python = ">= 3.10" or tool.ruff.target-version = "py310" in your pyproject.toml file.
(F821)
🔇 Additional comments (8)
app/modules/intelligence/agents_copy/chat_agent.py (1)
17-38: Interface definition looks good.The abstract methods are clearly defined and typed. This sets a strong foundation for implementing chat agents.
app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py (2)
9-60: Overall agent design looks good.
The asynchronous structure forrunandrun_streamproperly delegates toCrewAIRagAgent. The usage ofAsyncGeneratoris correct for streaming responses.
65-65: Verify the missingChangeDetectionResponseimport.
ChangeDetectionResponse.model_json_schema()is referenced here but never imported or defined in this file. Confirm if it’s declared in another module and ensure proper import statements to prevent runtime errors.app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py (1)
42-68: Classification fallback approach looks robust.
The try-except block gracefully handles malformed classification output, defaulting tocurr_agent_id. This helps keep the agent routing from breaking if the classifier returns unexpected data.app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (1)
71-108: Ensure secure handling of user-provided data
User inputs (query,context) are embedded in the prompt. If these inputs contain sensitive or unverified text, ensure there is sanitization or validation to avoid potential data leaks or injection issues in the prompt.app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (1)
72-72: Placeholder variable will not be substituted
{self.max_iter}in your triple-quoted string does not get expanded automatically unless you use an f-string orstr.format(). Verify whether dynamic substitution is needed or if it's purely illustrative.app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
57-66: Prevent early return in async flow
Therunmethod currently returns only the first response from the streamed results. If multiple responses are intended, capture or yield all of them to avoid data loss.app/modules/intelligence/agents_copy/chat_agents/simple_llm_agent.py (1)
1-14: LGTM! Well-organized imports and clear class definition.The imports are specific and well-organized, and the class name is descriptive and follows naming conventions.
| history: List[str], | ||
| node_ids: Optional[List[str]] = None, | ||
| ) -> AsyncGenerator[ChatAgentResponse, None]: | ||
| return self.rag_agent.run_stream(query, history, node_ids) |
There was a problem hiding this comment.
Fix reference to undefined self.rag_agent.
self.rag_agent is never assigned in the constructor, so this call will fail. Consider using self.agent instead, or properly define self.rag_agent.
-return self.rag_agent.run_stream(query, history, node_ids)
+return self.agent.run_stream(query, history, node_ids)📝 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.
| return self.rag_agent.run_stream(query, history, node_ids) | |
| return self.agent.run_stream(query, history, node_ids) |
| id="code_review_agent", | ||
| name="Code Review Agent", | ||
| description="An agent specialized in code reviews, use this for reviewing codes", | ||
| agent=simple_llm_agent, |
There was a problem hiding this comment.
Undefined name simple_llm_agent.
This reference will cause a runtime error since simple_llm_agent is not imported or defined. Please add the correct import or define the variable before usage.
- name="Code Review Agent",
- agent=simple_llm_agent,
+ name="Code Review Agent",
+ agent=SimpleLLMAgent(llm_provider, prompt_template="Your LLM prompt here"),📝 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.
| agent=simple_llm_agent, | |
| name="Code Review Agent", | |
| agent=SimpleLLMAgent(llm_provider, prompt_template="Your LLM prompt here"), |
🧰 Tools
🪛 Ruff (0.8.2)
41-41: Undefined name simple_llm_agent
(F821)
| AgentType, | ||
| ) | ||
| from app.modules.intelligence.prompts.classification_prompts import ( | ||
| ClassificationPrompts, | ||
| AgentType, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate import of AgentType.
AgentType is imported from two separate modules in lines 10 and 14, causing redefinition. Remove or rename one of these to avoid confusion.
-from app.modules.intelligence.provider.provider_service import (
- ProviderService,
- AgentType,
-)
...
-from app.modules.intelligence.prompts.classification_prompts import (
- ClassificationPrompts,
- AgentType,
-)
+from app.modules.intelligence.provider.provider_service import ProviderService, AgentType
+from app.modules.intelligence.prompts.classification_prompts import ClassificationPrompts📝 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.
| AgentType, | |
| ) | |
| from app.modules.intelligence.prompts.classification_prompts import ( | |
| ClassificationPrompts, | |
| AgentType, | |
| ) | |
| from app.modules.intelligence.provider.provider_service import ProviderService, AgentType | |
| from app.modules.intelligence.prompts.classification_prompts import ClassificationPrompts |
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Redefinition of unused AgentType from line 10
(F811)
| class AgentsPostgresRepo: | ||
| """Repository for agents, get list create update delete etc""" | ||
|
|
||
| def __init__(self, db: Session): | ||
| self.db = db | ||
|
|
There was a problem hiding this comment.
Fix undefined reference to Session.
These constructors reference Session but do not import or define it. Make sure to import Session from the appropriate module (e.g., from sqlalchemy.orm import Session) or otherwise define it:
+ from sqlalchemy.orm import SessionAlso applies to: 56-61, 66-68, 104-106
🧰 Tools
🪛 Ruff (0.8.2)
52-52: Undefined name Session
(F821)
| class AgentContext(BaseModel): | ||
| """Context for building an agent""" | ||
|
|
||
| message: str | ||
| llm_config: LLMConfig | ||
| agent_type: AgentType | ||
|
|
There was a problem hiding this comment.
Fix undefined references to LLMConfig and AgentType.
Both LLMConfig and AgentType are used in the AgentContext class without being imported or defined. Confirm that you have the right imports in place for them:
+ from app.modules.intelligence.llm_config import LLMConfig
+ from app.modules.intelligence.agents_copy.some_module import AgentType📝 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.
| class AgentContext(BaseModel): | |
| """Context for building an agent""" | |
| message: str | |
| llm_config: LLMConfig | |
| agent_type: AgentType | |
| + from app.modules.intelligence.llm_config import LLMConfig | |
| + from app.modules.intelligence.agents_copy.some_module import AgentType | |
| class AgentContext(BaseModel): | |
| """Context for building an agent""" | |
| message: str | |
| llm_config: LLMConfig | |
| agent_type: AgentType |
🧰 Tools
🪛 Ruff (0.8.2)
94-94: Undefined name LLMConfig
(F821)
95-95: Undefined name AgentType
(F821)
| history: List[str], | ||
| node_ids: Optional[List[str]] = None, | ||
| ) -> ChatAgentResponse: | ||
| return await anext(self.run_stream(query, history, node_ids)) |
There was a problem hiding this comment.
anext is incompatible with Python < 3.10
anext was introduced in Python 3.10. If backward compatibility is a requirement, switch to a manual approach or enforce Python 3.10+ in the project configuration.
🧰 Tools
🪛 Ruff (0.8.2)
138-138: Undefined name anext. Consider specifying requires-python = ">= 3.10" or tool.ruff.target-version = "py310" in your pyproject.toml file.
(F821)
| async for response in res: | ||
| return response |
There was a problem hiding this comment.
Premature return in async response loop
Calling return response upon the first iteration halts the loop, discarding subsequent generator outputs. If multiple responses are expected, collect them or yield them all.
Apply this diff for a simple fix:
- async for response in res:
- return response
+ responses = []
+ async for response in res:
+ responses.append(response)
+ if responses:
+ return responses[-1]
+ else:
+ raise Exception("response stream failed!!")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async for response in res: | |
| return response | |
| responses = [] | |
| async for response in res: | |
| responses.append(response) | |
| if responses: | |
| return responses[-1] | |
| else: | |
| raise Exception("response stream failed!!") |
| async def run( | ||
| self, | ||
| query: str, | ||
| history: List[str], | ||
| node_ids: Optional[List[str]] = None, | ||
| ) -> ChatAgentResponse: | ||
| return await self.chain.ainvoke({"query": query, "history": history}) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider unused parameter.
The node_ids parameter is accepted but not used in the implementation. Additionally, error handling should be added for the chain invocation.
Apply this diff to add error handling:
async def run(
self,
query: str,
history: List[str],
node_ids: Optional[List[str]] = None,
) -> ChatAgentResponse:
- return await self.chain.ainvoke({"query": query, "history": history})
+ try:
+ return await self.chain.ainvoke({"query": query, "history": history})
+ except Exception as e:
+ logger.error(f"Error invoking chain: {e}")
+ raiseAlso, if node_ids is not used, consider removing it or adding a TODO comment explaining its future use.
📝 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.
| async def run( | |
| self, | |
| query: str, | |
| history: List[str], | |
| node_ids: Optional[List[str]] = None, | |
| ) -> ChatAgentResponse: | |
| return await self.chain.ainvoke({"query": query, "history": history}) | |
| async def run( | |
| self, | |
| query: str, | |
| history: List[str], | |
| node_ids: Optional[List[str]] = None, | |
| ) -> ChatAgentResponse: | |
| try: | |
| return await self.chain.ainvoke({"query": query, "history": history}) | |
| except Exception as e: | |
| logger.error(f"Error invoking chain: {e}") | |
| raise |
test.py
Outdated
| """ Delete this file, this is temporary testing """ | ||
|
|
||
| from app.modules.intelligence.agents_copy.chat_agents.crewai_rag_agent import ( | ||
| CrewAIRagAgent, | ||
| AgentConfig, | ||
| TaskConfig, | ||
| ) | ||
| from app.modules.intelligence.provider.provider_service import ( | ||
| ProviderService, | ||
| ) | ||
| from app.modules.intelligence.agents_copy.chat_agents.system_agents.blast_radius_agent import ( | ||
| BlastRadiusAgent, | ||
| ) | ||
| from app.modules.intelligence.agents_copy.chat_agents.simple_llm_agent import ( | ||
| SimpleLLMAgent, | ||
| ) | ||
| from app.modules.intelligence.agents_copy.chat_agents.auto_router_agent import ( | ||
| AutoRouterAgent, | ||
| AgentWithInfo, | ||
| ) | ||
| from app.modules.intelligence.agents_copy.chat_agents.adaptive_agent import ( | ||
| AdaptiveAgent, | ||
| AgentType, | ||
| PromptService, | ||
| ) | ||
| from app.modules.intelligence.tools.tool_service import ToolService | ||
| from app.modules.intelligence.tools.kg_based_tools.get_code_from_probable_node_name_tool import ( | ||
| get_code_from_probable_node_name_tool, | ||
| ) | ||
| from app.modules.intelligence.tools.code_query_tools.get_code_file_structure import ( | ||
| get_code_file_structure_tool, | ||
| ) | ||
| from app.modules.intelligence.tools.code_query_tools.get_code_graph_from_node_id_tool import ( | ||
| get_code_graph_from_node_id_tool, | ||
| ) | ||
| from app.core.database import get_db | ||
| from dotenv import load_dotenv | ||
| from app.modules.utils.logger import setup_logger | ||
| import asyncio | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider converting this temporary test into proper test cases.
The file is marked for deletion, but it contains valuable test scenarios. Consider:
- Moving these tests into proper test files under a
testsdirectory. - Organizing tests by agent type.
- Using a test framework like
pytest.
test.py
Outdated
| async def go(): | ||
| print("starting test") | ||
| load_dotenv() | ||
| session = next(get_db()) | ||
| user_id = "0tyA7AEi9KbHYRyLu33qV9BjPgg1" | ||
|
|
||
| llm_provider = ProviderService(session, user_id) | ||
| tools_provider = ToolService(session, user_id) | ||
| prompt_provider = PromptService(session) | ||
| project_id = "0194d052-19b2-7b01-82a0-299da1538355" | ||
|
|
||
| agent = CrewAIRagAgent( | ||
| llm_provider=llm_provider, | ||
| config=AgentConfig( | ||
| role="Code Reviewer Agent", | ||
| goal="Review the code for user and answer queries", | ||
| backstory="You are a helpful code review agent that has access to entire codebase. User want's your help reviewing code", | ||
| tasks=[ | ||
| TaskConfig( | ||
| description="review the code", | ||
| expected_output="code review in markdown format", | ||
| ) | ||
| ], | ||
| ), | ||
| tools=[ | ||
| get_code_file_structure_tool(session), | ||
| get_code_from_probable_node_name_tool(session, user_id), | ||
| get_code_graph_from_node_id_tool(session), | ||
| ], | ||
| project_id=project_id, | ||
| ) | ||
| blast_radius_agent = BlastRadiusAgent(llm_provider, tools_provider, project_id) | ||
|
|
||
| simple_llm_agent = SimpleLLMAgent( | ||
| llm_provider, | ||
| "You are the Code Review Agent, review the code given in the query. Answer the following query: {query}", | ||
| ) | ||
|
|
||
| adaptive_agent = AdaptiveAgent( | ||
| llm_provider, prompt_provider, blast_radius_agent, AgentType.CODE_CHANGES | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused agent and make IDs configurable.
- The
agentvariable is assigned but never used (as flagged by static analysis). - User ID and project ID are hardcoded and should be configurable.
Apply this diff to improve the code:
async def go():
print("starting test")
load_dotenv()
session = next(get_db())
- user_id = "0tyA7AEi9KbHYRyLu33qV9BjPgg1"
+ user_id = os.getenv("TEST_USER_ID")
+ if not user_id:
+ raise ValueError("TEST_USER_ID environment variable is required")
llm_provider = ProviderService(session, user_id)
tools_provider = ToolService(session, user_id)
prompt_provider = PromptService(session)
- project_id = "0194d052-19b2-7b01-82a0-299da1538355"
+ project_id = os.getenv("TEST_PROJECT_ID")
+ if not project_id:
+ raise ValueError("TEST_PROJECT_ID environment variable is required")
- agent = CrewAIRagAgent(
- llm_provider=llm_provider,
- config=AgentConfig(
- role="Code Reviewer Agent",
- goal="Review the code for user and answer queries",
- backstory="You are a helpful code review agent that has access to entire codebase. User want's your help reviewing code",
- tasks=[
- TaskConfig(
- description="review the code",
- expected_output="code review in markdown format",
- )
- ],
- ),
- tools=[
- get_code_file_structure_tool(session),
- get_code_from_probable_node_name_tool(session, user_id),
- get_code_graph_from_node_id_tool(session),
- ],
- project_id=project_id,
- )📝 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.
| async def go(): | |
| print("starting test") | |
| load_dotenv() | |
| session = next(get_db()) | |
| user_id = "0tyA7AEi9KbHYRyLu33qV9BjPgg1" | |
| llm_provider = ProviderService(session, user_id) | |
| tools_provider = ToolService(session, user_id) | |
| prompt_provider = PromptService(session) | |
| project_id = "0194d052-19b2-7b01-82a0-299da1538355" | |
| agent = CrewAIRagAgent( | |
| llm_provider=llm_provider, | |
| config=AgentConfig( | |
| role="Code Reviewer Agent", | |
| goal="Review the code for user and answer queries", | |
| backstory="You are a helpful code review agent that has access to entire codebase. User want's your help reviewing code", | |
| tasks=[ | |
| TaskConfig( | |
| description="review the code", | |
| expected_output="code review in markdown format", | |
| ) | |
| ], | |
| ), | |
| tools=[ | |
| get_code_file_structure_tool(session), | |
| get_code_from_probable_node_name_tool(session, user_id), | |
| get_code_graph_from_node_id_tool(session), | |
| ], | |
| project_id=project_id, | |
| ) | |
| blast_radius_agent = BlastRadiusAgent(llm_provider, tools_provider, project_id) | |
| simple_llm_agent = SimpleLLMAgent( | |
| llm_provider, | |
| "You are the Code Review Agent, review the code given in the query. Answer the following query: {query}", | |
| ) | |
| adaptive_agent = AdaptiveAgent( | |
| llm_provider, prompt_provider, blast_radius_agent, AgentType.CODE_CHANGES | |
| ) | |
| async def go(): | |
| print("starting test") | |
| load_dotenv() | |
| session = next(get_db()) | |
| user_id = os.getenv("TEST_USER_ID") | |
| if not user_id: | |
| raise ValueError("TEST_USER_ID environment variable is required") | |
| llm_provider = ProviderService(session, user_id) | |
| tools_provider = ToolService(session, user_id) | |
| prompt_provider = PromptService(session) | |
| project_id = os.getenv("TEST_PROJECT_ID") | |
| if not project_id: | |
| raise ValueError("TEST_PROJECT_ID environment variable is required") | |
| blast_radius_agent = BlastRadiusAgent(llm_provider, tools_provider, project_id) | |
| simple_llm_agent = SimpleLLMAgent( | |
| llm_provider, | |
| "You are the Code Review Agent, review the code given in the query. Answer the following query: {query}", | |
| ) | |
| adaptive_agent = AdaptiveAgent( | |
| llm_provider, prompt_provider, blast_radius_agent, AgentType.CODE_CHANGES | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
56-56: Local variable agent is assigned to but never used
Remove assignment to unused variable agent
(F841)
| ) | ||
| prompts = {prompt.type: prompt for prompt in llm_prompts} | ||
| system_prompt = prompts.get(PromptType.SYSTEM) | ||
| if system_prompt == None: |
There was a problem hiding this comment.
when would this be true?
| # classify the query into agent needed or not | ||
| classification_response = await self.classifier.run(query, history, node_ids) | ||
| classification = "AGENT_REQUIRED" | ||
| print("Classification response:", classification_response.response) |
There was a problem hiding this comment.
ensure we dont't leave the print statements in final run
| ) | ||
|
|
||
|
|
||
| class SimpleLLMAgent(ChatAgent): |
There was a problem hiding this comment.
name is a misnomer, its a simple llm call, nothing agentic about this, its okay to call it what it is
| max_iter: int = 5 | ||
|
|
||
|
|
||
| class CrewAIRagAgent(ChatAgent): |
There was a problem hiding this comment.
I dont like the name CrewAIRagAgent, unfortunately I don't have suggestions either
There was a problem hiding this comment.
CrewAI and RAG both i dont want to include in the name
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (3)
10-18: Add a docstring for the constructor
Consider adding a docstring to describe the purpose and usage ofLowLevelDesignAgent's constructor for better clarity.
48-55: Add exception handling while fetching code
Consider wrapping therun_multiplecall within a try/except block to handle any unexpected failures or empty responses from the tool.
65-77: Consider externalizing thelld_task_prompt
Storing this prompt string in a dedicated configuration file or constants module can reduce code clutter and improve maintainability.app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py (3)
10-18: Add a docstring for the constructor
Consider including a docstring explaining the responsibilities and dependencies of theUnitTestAgentconstructor.
42-49: Consider error handling for code retrieval
Surround therun_multiplecall with a try/except to gracefully handle exceptions if the node-ID-based retrieval fails or returns unexpected results.
59-97: Encapsulate theqna_task_promptexternally
Consider storing large prompt strings in a separate configuration file to keep the agent class more focused and maintainable.app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (1)
128-159: Stream method currently yields a single result
Sincerun_streamonly yields one result, consider clarifying the method name or enhancing it to provide partial outputs if true streaming is needed.app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (3)
10-18: Consider adding docstrings for clarity.
While the constructor logic is straightforward, providing docstrings for the classIntegrationTestAgentand its__init__method would enhance readability and maintainability.
41-47: Validate return context and potential exceptions.
Your_enriched_contextmethod handles node IDs properly, but it only appends the code graph inctx.additional_context. Consider adding handling for error cases (e.g., ifgraphretrieval fails) or verifying that the appended context remains consistent across multiple runs.
58-120: Ensure instructions align with typical testing workflows.
The extensive prompt is great for guiding a test generation process; however, verify whether each step (like “Reflection and Iteration”) can be adequately handled within the agent’s flow, particularly given limited iteration constraints. If the process is too large, consider modularizing prompts or providing clearer breakpoints for partial user feedback.app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (3)
10-18: Add docstrings for improved clarity.
TheQnAAgentconstructor could benefit from minimal docstrings to describe the purpose of each parameter (llm_provider,tools_provider) and how they integrate with the agent’s Q&A flow.
53-60: Consider more robust error handling.
When callingrun_multiple(...), ensure that any exceptions (e.g., network or parsing issues) are managed gracefully or surfaced to the user. This will make the agent more resilient.
70-152: Validate the complexity of the QnA prompt.
Theqna_task_promptincludes multiple steps and conditions. If these steps become too large or are not strictly required for every query, consider splitting the process into sub-tasks or dynamic prompts to optimize performance and clarity.app/modules/intelligence/agents_copy/chat_agents/llm_chat.py (2)
14-19: Add error handling for LLM initialization.The constructor initializes two LLM instances but doesn't handle potential initialization failures. Consider adding try-catch blocks and proper error handling.
def __init__(self, llm_provider: ProviderService, prompt_template: str): + try: self.llm_large = llm_provider.get_large_llm(AgentType.LANGCHAIN) self.llm_small = llm_provider.get_small_llm(AgentType.LANGCHAIN) self.chain = self._create_chain(prompt_template) + except Exception as e: + logger.error(f"Failed to initialize LLM: {str(e)}") + raise
28-29: Add type hints for the return value of ainvoke.The
ainvokemethod's return value should be explicitly type-hinted for better code maintainability.- async def run(self, ctx: ChatContext) -> ChatAgentResponse: - return await self.chain.ainvoke({"query": ctx.query, "history": ctx.history}) + async def run(self, ctx: ChatContext) -> ChatAgentResponse: + response: ChatAgentResponse = await self.chain.ainvoke( + {"query": ctx.query, "history": ctx.history} + ) + return responseapp/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py (2)
60-71: Enhance error handling in classification.The error handling could be improved by logging more details about the error and by adding type hints for better code maintainability.
Apply this diff to enhance error handling:
try: agent_id, confidence = classification.response.strip("`").split("|") confidence = float(confidence) selected_agent_id = ( agent_id if confidence >= 0.5 and self.agents[agent_id] else self.curr_agent_id ) - except (ValueError, TypeError): - logger.error("Classification format error, falling back to current agent") + except (ValueError, TypeError) as e: + logger.error("Classification format error: %s. Falling back to current agent", str(e)) selected_agent_id = self.curr_agent_id
85-126: Consider splitting the classification prompt into sections.The classification prompt is well-structured but quite long. Consider splitting it into separate constants for better maintainability.
Example refactor:
ANALYSIS_INSTRUCTIONS = """ 1. **Semantic Analysis:** ... """ CONTEXTUAL_WEIGHTING = """ 2. **Contextual Weighting:** ... """ CONFIDENCE_SCORING = """ 4. **Confidence Scoring Guidelines:** ... """ classification_prompt = f""" Given the user query and the current agent ID, select the most appropriate agent by comparing the query's requirements with each agent's specialties. Query: {{query}} Current Agent ID: {{agent_id}} Available agents and their specialties: {{agent_descriptions}} {ANALYSIS_INSTRUCTIONS} {CONTEXTUAL_WEIGHTING} {CONFIDENCE_SCORING} ... """app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py (1)
31-119: Consider moving agent configurations to a separate file.The list of agents with their configurations could be moved to a separate configuration file for better maintainability and easier updates.
Consider creating a new file
agent_configs.pyand moving the agent configurations there:# agent_configs.py from typing import List from .auto_router_agent import AgentWithInfo def get_agent_configs( llm_provider: ProviderService, tools_provider: ToolService, prompt_provider: PromptService ) -> List[AgentWithInfo]: return [ AgentWithInfo( id="codebase_qna_agent", name="Codebase Q&A Agent", description="An agent specialized in answering questions...", agent=AdaptiveAgent(...), ), # ... other agents ]app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
59-65: Improve context enrichment string handling.Using f-strings for large text concatenation could be inefficient. Consider using a list to collect context and join at the end.
Apply this diff to improve string handling:
async def _enriched_context(self, ctx: ChatContext) -> ChatContext: if ctx.node_ids and len(ctx.node_ids) > 0: code_results = await self.tools_provider.get_code_from_probable_node_name_tool.run_multiple( ctx.project_id, ctx.node_ids ) - ctx.additional_context += f"Code results:\n {code_results}" + ctx.additional_context = "\n".join([ctx.additional_context, f"Code results:\n{code_results}"]) return ctx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/modules/intelligence/agents_copy/chat_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/llm_chat.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py(1 hunks)app/modules/intelligence/tools/tool_service.py(3 hunks)test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py
🧰 Additional context used
🪛 GitHub Actions: Pre-commit
app/modules/intelligence/agents_copy/chat_agents/llm_chat.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
test.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/tools/tool_service.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🪛 Ruff (0.8.2)
test.py
93-93: Local variable auto_router_agent is assigned to but never used
Remove assignment to unused variable auto_router_agent
(F841)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
15-15: Redefinition of unused AgentType from line 11
(F811)
55-55: Comparison to None should be cond is None
Replace with cond is None
(E711)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
76-76: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
🔇 Additional comments (21)
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (4)
1-8: Resolve Prettier formatting issues
The pipeline is failing due to Prettier checks. Please runprettier --writeto fix the formatting problems.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
19-46: Verify tool availability in_build_agent
If any of the tool keys are missing inself.tools_provider.tools, aKeyErrorcould occur. Either confirm these keys always exist or handle missing tools gracefully.
56-58: LGTM
Therunmethod is concise and delegates effectively to_build_agent.
59-63: LGTM
Therun_streammethod mirrors the structure ofrun, ensuring consistency in asynchronous flow.app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py (4)
1-8: Resolve Prettier formatting issues
The pipeline is failing due to Prettier checks. Please runprettier --writeto fix the formatting problems.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
19-40: Verify tool references in_build_agent
If the specified keys inself.tools_provider.toolsare absent, aKeyErrormay be raised. Double-check or add error handling for missing tools.
50-52: LGTM
Therunmethod straightforwardly handles asynchronous context enrichment and agent execution.
53-57: LGTM
Therun_streammethod correctly defers to the underlying streaming logic of_build_agent().app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (3)
1-9: Resolve Prettier formatting issues
Pre-commit checks indicate a formatting problem. Please runprettier --writeto address it.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
15-30: LGTM
TheTaskConfigandAgentConfigclasses provide a clean, flexible way to configure your agent and tasks.
32-32: Naming feedback noted (duplicate comment)
A previous reviewer expressed concerns about theCrewAIRagAgentname. If alternative naming aligns better with project conventions, consider adjusting it.app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (1)
1-1: Address the Prettier formatting issue.
The GitHub Actions pipeline indicates that the Prettier formatting check failed on this file. Please run the recommended command (e.g.,prettier --write) or apply equivalent code style fixes to resolve the warning.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (1)
1-1: Fix Prettier formatting issues in this file.
According to the pipeline logs, Prettier flagged this file for formatting problems. Please address the issues by runningprettier --writeor applying the required style changes.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
app/modules/intelligence/agents_copy/chat_agent.py (2)
1-1: Address the Prettier warning for this file.
The pipeline has flagged formatting problems here as well. Run the appropriate formatting commands to keep the file consistent with the project's style guidelines.🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
26-39: Confirm consistent naming and usage across derived agents.
The abstract methodsrunandrun_streamare implemented in multiple child agents. Ensure each agent’s usage is aligned (e.g., consistent parameter naming, concurrency handling, etc.) for a unified developer experience.app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (2)
10-16: Remove duplicate AgentType import.The
AgentTypeis imported from two different modules, causing redefinition.from app.modules.intelligence.provider.provider_service import ( ProviderService, AgentType, ) from app.modules.intelligence.prompts.classification_prompts import ( ClassificationPrompts, - AgentType, )🧰 Tools
🪛 Ruff (0.8.2)
15-15: Redefinition of unused
AgentTypefrom line 11(F811)
73-73: Replace print statements with proper logging.Print statements should be replaced with proper logging for production code.
- print("Classification response:", classification_response.response) + logger.debug("Classification response: %s", classification_response.response)Also applies to: 97-97
app/modules/intelligence/tools/tool_service.py (1)
53-56: LGTM! Clean implementation of new tools.The new tool initialization follows the existing pattern and is well-structured.
test.py (2)
1-1: Convert temporary test into proper test cases.This file is marked for deletion but contains valuable test scenarios.
Consider:
- Moving these tests into proper test files under a
testsdirectory- Organizing tests by agent type
- Using a test framework like
pytest🧰 Tools
🪛 GitHub Actions: Pre-commit
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
53-58: Make IDs configurable.User ID and project ID are hardcoded and should be configurable.
- user_id = "0tyA7AEi9KbHYRyLu33qV9BjPgg1" + user_id = os.getenv("TEST_USER_ID") + if not user_id: + raise ValueError("TEST_USER_ID environment variable is required") llm_provider = ProviderService(session, user_id) tools_provider = ToolService(session, user_id) prompt_provider = PromptService(session) - project_id = "0194d052-19b2-7b01-82a0-299da1538355" + project_id = os.getenv("TEST_PROJECT_ID") + if not project_id: + raise ValueError("TEST_PROJECT_ID environment variable is required")app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py (1)
42-44: Fix potentialKeyErrorincurr_agent_idcheck.The check for
curr_agent_idvalidity is still vulnerable toKeyError.Apply this diff to fix the issue:
-if not self.agents[curr_agent_id]: +if curr_agent_id not in self.agents or not self.agents[curr_agent_id]:
| if isinstance(ctx.node_ids, str): | ||
| node_ids = [ctx.node_ids] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or use node_ids variable
You assign node_ids = [ctx.node_ids] but never use it, which can confuse readers and trigger linting issues. Consider removing the assignment or integrating it into the logic.
- if isinstance(ctx.node_ids, str):
- node_ids = [ctx.node_ids]
+ # Remove or utilize `node_ids` variable, for example:
+ if isinstance(ctx.node_ids, str):
+ ctx.node_ids = [ctx.node_ids]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(ctx.node_ids, str): | |
| node_ids = [ctx.node_ids] | |
| # Remove or utilize `node_ids` variable, for example: | |
| if isinstance(ctx.node_ids, str): | |
| ctx.node_ids = [ctx.node_ids] |
🧰 Tools
🪛 Ruff (0.8.2)
76-76: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
| if system_prompt == None: | ||
| # raise ValueError(f"System Prompt for {self.agent_type} not found!!") | ||
| logger.error(f"System Prompt for {self.agent_type} not found!!") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix comparison to None and improve error handling.
The comparison to None should use is operator, and the error handling could be improved.
- if system_prompt == None:
- # raise ValueError(f"System Prompt for {self.agent_type} not found!!")
- logger.error(f"System Prompt for {self.agent_type} not found!!")
+ if system_prompt is None:
+ error_msg = f"System Prompt for {self.agent_type} not found!"
+ logger.error(error_msg)
+ raise ValueError(error_msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if system_prompt == None: | |
| # raise ValueError(f"System Prompt for {self.agent_type} not found!!") | |
| logger.error(f"System Prompt for {self.agent_type} not found!!") | |
| if system_prompt is None: | |
| error_msg = f"System Prompt for {self.agent_type} not found!" | |
| logger.error(error_msg) | |
| raise ValueError(error_msg) |
🧰 Tools
🪛 Ruff (0.8.2)
55-55: Comparison to None should be cond is None
Replace with cond is None
(E711)
test.py
Outdated
| auto_router_agent = AutoRouterAgent( | ||
| llm_provider, | ||
| agents=[ | ||
| AgentWithInfo( | ||
| id="code_changes_agent", | ||
| name="Code Changes Agent", | ||
| description="An agent specialized in generating blast radius of the code changes in your current branch compared to default branch. Use this for functional review of your code changes. Works best with Py, JS, TS", | ||
| agent=adaptive_agent, | ||
| ), | ||
| AgentWithInfo( | ||
| id="code_review_agent", | ||
| name="Code Review Agent", | ||
| description="An agent specialized in code reviews, use this for reviewing codes", | ||
| agent=simple_llm_agent, | ||
| ), | ||
| ], | ||
| curr_agent_id="code_changes_agent", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused AutoRouterAgent instance.
The auto_router_agent is initialized but never used in the code.
- auto_router_agent = AutoRouterAgent(
- llm_provider,
- agents=[
- AgentWithInfo(
- id="code_changes_agent",
- name="Code Changes Agent",
- description="An agent specialized in generating blast radius of the code changes in your current branch compared to default branch. Use this for functional review of your code changes. Works best with Py, JS, TS",
- agent=adaptive_agent,
- ),
- AgentWithInfo(
- id="code_review_agent",
- name="Code Review Agent",
- description="An agent specialized in code reviews, use this for reviewing codes",
- agent=simple_llm_agent,
- ),
- ],
- curr_agent_id="code_changes_agent",
- )📝 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.
| auto_router_agent = AutoRouterAgent( | |
| llm_provider, | |
| agents=[ | |
| AgentWithInfo( | |
| id="code_changes_agent", | |
| name="Code Changes Agent", | |
| description="An agent specialized in generating blast radius of the code changes in your current branch compared to default branch. Use this for functional review of your code changes. Works best with Py, JS, TS", | |
| agent=adaptive_agent, | |
| ), | |
| AgentWithInfo( | |
| id="code_review_agent", | |
| name="Code Review Agent", | |
| description="An agent specialized in code reviews, use this for reviewing codes", | |
| agent=simple_llm_agent, | |
| ), | |
| ], | |
| curr_agent_id="code_changes_agent", | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
93-93: Local variable auto_router_agent is assigned to but never used
Remove assignment to unused variable auto_router_agent
(F841)
| agent_type=AgentType.CODE_CHANGES, | ||
| ), |
There was a problem hiding this comment.
Fix incorrect agent type for code generation agent.
The code generation agent is using AgentType.CODE_CHANGES instead of what appears to be the correct type for code generation.
Apply this diff to fix the agent type:
- agent_type=AgentType.CODE_CHANGES,
+ agent_type=AgentType.CODE_GENERATION,📝 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.
| agent_type=AgentType.CODE_CHANGES, | |
| ), | |
| agent_type=AgentType.CODE_GENERATION, | |
| ), |
| async def run(self, ctx: ChatContext) -> ChatAgentResponse: | ||
| return await self._build_agent().run(await self._enriched_context(ctx)) | ||
|
|
||
| async def run_stream( | ||
| self, ctx: ChatContext | ||
| ) -> AsyncGenerator[ChatAgentResponse, None]: | ||
| return await self._build_agent().run_stream(await self._enriched_context(ctx)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cache the agent instance to avoid rebuilding.
The agent is rebuilt for every request in both run and run_stream methods. Consider caching the agent instance.
Apply this diff to implement agent caching:
class CodeGenAgent(ChatAgent):
def __init__(
self,
llm_provider: ProviderService,
tools_provider: ToolService,
):
self.llm_provider = llm_provider
self.tools_provider = tools_provider
+ self._agent = None
+
+ @property
+ def agent(self) -> ChatAgent:
+ if self._agent is None:
+ self._agent = self._build_agent()
+ return self._agent
async def run(self, ctx: ChatContext) -> ChatAgentResponse:
- return await self._build_agent().run(await self._enriched_context(ctx))
+ return await self.agent.run(await self._enriched_context(ctx))
async def run_stream(
self, ctx: ChatContext
) -> AsyncGenerator[ChatAgentResponse, None]:
- return await self._build_agent().run_stream(await self._enriched_context(ctx))
+ return await self.agent.run_stream(await self._enriched_context(ctx))📝 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.
| async def run(self, ctx: ChatContext) -> ChatAgentResponse: | |
| return await self._build_agent().run(await self._enriched_context(ctx)) | |
| async def run_stream( | |
| self, ctx: ChatContext | |
| ) -> AsyncGenerator[ChatAgentResponse, None]: | |
| return await self._build_agent().run_stream(await self._enriched_context(ctx)) | |
| class CodeGenAgent(ChatAgent): | |
| def __init__( | |
| self, | |
| llm_provider: ProviderService, | |
| tools_provider: ToolService, | |
| ): | |
| self.llm_provider = llm_provider | |
| self.tools_provider = tools_provider | |
| self._agent = None | |
| @property | |
| def agent(self) -> ChatAgent: | |
| if self._agent is None: | |
| self._agent = self._build_agent() | |
| return self._agent | |
| async def run(self, ctx: ChatContext) -> ChatAgentResponse: | |
| - return await self._build_agent().run(await self._enriched_context(ctx)) | |
| + return await self.agent.run(await self._enriched_context(ctx)) | |
| async def run_stream( | |
| self, ctx: ChatContext | |
| ) -> AsyncGenerator[ChatAgentResponse, None]: | |
| - return await self._build_agent().run_stream(await self._enriched_context(ctx)) | |
| + return await self.agent.run_stream(await self._enriched_context(ctx)) |
nndn
force-pushed
the
refactor/agents
branch
from
994dfd9 to
884785e
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
app/modules/intelligence/agents_copy/agents_service.py (1)
217-223:⚠️ Potential issueComplete the agent ID validation logic.
The
validate_agent_idmethod has commented-out code and a TODO comment. This could lead to incomplete validation of agent IDs.Would you like me to help implement the complete agent ID validation logic? I can generate the code to:
- Check if the agent ID exists in system agents
- Validate custom agents using the
validate_agentfunction- Return a boolean indicating whether the agent ID is valid
🧹 Nitpick comments (20)
app/modules/conversations/conversation/conversation_service_copy.py (4)
5-5: Remove unused import.
typing.Anyis imported but never referenced in the code. Removing this unused import helps keep the codebase clean.- from typing import Any, AsyncGenerator, List + from typing import AsyncGenerator, List🧰 Tools
🪛 Ruff (0.8.2)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
6-6: Remove unused import.
HumanMessageis imported but never referenced in the code. Removing it reduces unnecessary dependencies.- from langchain.schema import HumanMessage🧰 Tools
🪛 Ruff (0.8.2)
6-6:
langchain.schema.HumanMessageimported but unusedRemove unused import:
langchain.schema.HumanMessage(F401)
371-371: Use direct property assignment instead ofsetattr.Hardcoding the attribute name makes
setattrunnecessary. It’s more straightforward to assign the value directly.- setattr(conversation, "human_message_count", human_message_count) + conversation.human_message_count = human_message_count🧰 Tools
🪛 Ruff (0.8.2)
371-371: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
535-536: Preserve original exception context or remove unused variable.If you intend to keep the original traceback, chain the exception with
from e. Otherwise, removeeif not needed.except Exception as e: - raise ConversationServiceError("Failed to get chat history") + raise ConversationServiceError("Failed to get chat history") from e🧰 Tools
🪛 Ruff (0.8.2)
535-535: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
536-536: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
app/modules/intelligence/agents_copy/agents_controller.py (1)
34-36: Chain the original exception or remove unused variable.If preserving traceback is needed, use
raise ... from e; otherwise, there's no need to capturee.except Exception as e: - raise HTTPException( - status_code=500, detail=f"Error listing agents: {str(e)}" - ) + raise HTTPException( + status_code=500, detail=f"Error listing agents: {str(e)}" + ) from e🧰 Tools
🪛 Ruff (0.8.2)
34-36: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
app/modules/intelligence/agents_copy/agents_router.py (2)
28-31: Remove empty constructor.The empty constructor serves no purpose and can be removed.
- def __init__( - self, - ): - pass
36-40: Follow FastAPI best practices for dependency injection.Move the
Dependscalls out of the argument defaults to follow FastAPI best practices.- async def list_available_agents( - db: Session = Depends(get_db), - user=Depends(AuthService.check_auth), - list_system_agents: bool = Query( - default=True, description="Include system agents in the response" - ), - ): + async def list_available_agents( + db: Session, + user: dict, + list_system_agents: bool = Query( + default=True, description="Include system agents in the response" + ), + ): + db = Depends(get_db) + user = Depends(AuthService.check_auth)🧰 Tools
🪛 Ruff (0.8.2)
36-36: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
37-37: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (1)
37-45: Simplify tools array using list comprehension.The tools array can be simplified using list comprehension for better readability.
- tools=[ - self.tools_provider.tools["get_code_from_multiple_node_ids"], - self.tools_provider.tools["get_node_neighbours_from_node_id"], - self.tools_provider.tools["get_code_from_probable_node_name"], - self.tools_provider.tools["ask_knowledge_graph_queries"], - self.tools_provider.tools["get_nodes_from_tags"], - self.tools_provider.tools["webpage_extractor"], - self.tools_provider.tools["github_tool"], - ], + tools=[ + self.tools_provider.tools[tool_name] + for tool_name in [ + "get_code_from_multiple_node_ids", + "get_node_neighbours_from_node_id", + "get_code_from_probable_node_name", + "ask_knowledge_graph_queries", + "get_nodes_from_tags", + "webpage_extractor", + "github_tool", + ] + ],app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (2)
151-153: Improve error handling in the run method.The current error handling re-raises the exception without providing additional context about what failed.
Apply this diff to improve error handling:
except Exception as e: logger.error(f"Error in run method: {str(e)}", exc_info=True) - raise Exception from e + raise Exception(f"CrewAIRagAgent run failed: {str(e)}") from e
155-158: Add error handling to run_stream method.Consider adding try-catch block similar to the run method to handle potential errors gracefully.
async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: - yield await self.run(ctx) # CrewAI doesn't support streaming response + try: + yield await self.run(ctx) # CrewAI doesn't support streaming response + except Exception as e: + logger.error(f"Error in run_stream method: {str(e)}", exc_info=True) + raise Exception from eapp/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (4)
41-47: Add error handling to_enriched_context.The method should handle potential errors when retrieving the code graph.
Apply this diff to add error handling:
def _enriched_context(self, ctx: ChatContext) -> ChatContext: if ctx.node_ids and len(ctx.node_ids) > 0: - graph = self.tools_provider.get_code_graph_from_node_id_tool.run( - ctx.project_id, ctx.node_ids[0] - ) - ctx.additional_context += f"Graph: {graph}" + try: + graph = self.tools_provider.get_code_graph_from_node_id_tool.run( + ctx.project_id, ctx.node_ids[0] + ) + ctx.additional_context += f"Graph: {graph}" + except Exception as e: + logger.warning(f"Failed to retrieve code graph: {str(e)}") return ctx
58-120: Enhance the integration test task prompt.The prompt could be improved by:
- Adding examples of expected test output format.
- Including guidelines for test naming conventions.
- Specifying test framework preferences.
41-47: Add error handling to _enriched_context method.Consider adding try-catch block to handle potential errors from tool execution.
def _enriched_context(self, ctx: ChatContext) -> ChatContext: - if ctx.node_ids and len(ctx.node_ids) > 0: - graph = self.tools_provider.get_code_graph_from_node_id_tool.run( - ctx.project_id, ctx.node_ids[0] - ) - ctx.additional_context += f"Graph: {graph}" - return ctx + try: + if ctx.node_ids and len(ctx.node_ids) > 0: + graph = self.tools_provider.get_code_graph_from_node_id_tool.run( + ctx.project_id, ctx.node_ids[0] + ) + ctx.additional_context += f"Graph: {graph}" + return ctx + except Exception as e: + logger.error(f"Error enriching context: {str(e)}", exc_info=True) + raise
58-120: Add examples of popular testing frameworks.Consider adding examples of commonly used testing frameworks (e.g., pytest, jest) to help guide the test implementation.
app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (5)
37-45: Add debugging-specific tools.Consider adding tools specifically designed for debugging, such as:
- Stack trace analyzer
- Error log parser
- Performance profiler
57-138: Fix prompt numbering and improve structure.The prompt has inconsistent numbering (jumps from step 3 to 5) and could be better structured.
Apply this diff to fix the numbering:
3. Knowledge graph query (if needed): - Transform query for knowledge graph tool - Execute query and analyze results - Additional context retrieval (if needed): + 4. Additional context retrieval (if needed):
10-46: Add logging setup.Consider adding logger initialization for consistent error tracking and debugging.
+from app.modules.utils.logger import setup_logger + +logger = setup_logger(__name__) + class DebugAgent(ChatAgent):
48-54: Add error handling to run methods.Consider adding try-catch blocks to handle potential errors gracefully.
async def run(self, ctx: ChatContext) -> ChatAgentResponse: - return await self.rag_agent.run(ctx) + try: + return await self.rag_agent.run(ctx) + except Exception as e: + logger.error(f"Error in run method: {str(e)}", exc_info=True) + raise async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: - return self.rag_agent.run_stream(ctx) + try: + return self.rag_agent.run_stream(ctx) + except Exception as e: + logger.error(f"Error in run_stream method: {str(e)}", exc_info=True) + raise
97-97: Fix step numbering in code_gen_task_prompt.Step number jumps from 3 to 5, skipping 4. Consider fixing the numbering sequence.
app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
59-65: Optimize context enrichment for better performance.The
_enriched_contextmethod fetches code for each node ID sequentially. Consider fetching code for all node IDs in parallel for better performance.Apply this diff to optimize context enrichment:
async def _enriched_context(self, ctx: ChatContext) -> ChatContext: if ctx.node_ids and len(ctx.node_ids) > 0: - code_results = await self.tools_provider.get_code_from_probable_node_name_tool.run_multiple( - ctx.project_id, ctx.node_ids - ) + tasks = [ + self.tools_provider.get_code_from_probable_node_name_tool.run( + ctx.project_id, node_id + ) + for node_id in ctx.node_ids + ] + code_results = await asyncio.gather(*tasks) ctx.additional_context += f"Code results:\n {code_results}" return ctxDon't forget to add the import:
import asyncio
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/modules/conversations/conversation/conversation_controller.py(1 hunks)app/modules/conversations/conversation/conversation_service_copy.py(1 hunks)app/modules/intelligence/agents_copy/agents_controller.py(1 hunks)app/modules/intelligence/agents_copy/agents_router.py(1 hunks)app/modules/intelligence/agents_copy/agents_service.py(1 hunks)app/modules/intelligence/agents_copy/chat_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/custom_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/llm_chat.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py(1 hunks)app/modules/intelligence/prompts/classification_prompts.py(1 hunks)app/modules/intelligence/tools/tool_service.py(3 hunks)test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/modules/conversations/conversation/conversation_controller.py
🚧 Files skipped from review as they are similar to previous changes (7)
- app/modules/intelligence/agents_copy/chat_agents/custom_agent.py
- app/modules/intelligence/agents_copy/chat_agents/llm_chat.py
- app/modules/intelligence/agents_copy/chat_agents/supervisor_agent.py
- app/modules/intelligence/agents_copy/chat_agents/auto_router_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/intelligence/agents_copy/agents_router.py
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/intelligence/agents_copy/agents_controller.py
34-36: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
76-76: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
app/modules/conversations/conversation/conversation_service_copy.py
5-5: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
6-6: langchain.schema.HumanMessage imported but unused
Remove unused import: langchain.schema.HumanMessage
(F401)
371-371: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
407-407: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
535-535: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
536-536: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/agents_service.py
30-30: app.modules.intelligence.agents.custom_agents.agent_validator.validate_agent imported but unused
Remove unused import: app.modules.intelligence.agents.custom_agents.agent_validator.validate_agent
(F401)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
54-54: Comparison to None should be cond is None
Replace with cond is None
(E711)
test.py
93-93: Local variable auto_router_agent is assigned to but never used
Remove assignment to unused variable auto_router_agent
(F841)
🔇 Additional comments (17)
app/modules/conversations/conversation/conversation_service_copy.py (1)
1-805: Overall well-structured and comprehensive.This new
ConversationServiceimplementation shows robust error handling, clear separation of concerns, and proper usage of async/await for critical operations. Great job!🧰 Tools
🪛 Ruff (0.8.2)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
6-6:
langchain.schema.HumanMessageimported but unusedRemove unused import:
langchain.schema.HumanMessage(F401)
371-371: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
407-407: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
535-535: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
536-536: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
app/modules/intelligence/agents_copy/agents_controller.py (1)
25-33: Method appears clean and straightforward.The
list_available_agentsmethod neatly handles service interaction and error reporting, enhancing maintainability.app/modules/intelligence/agents_copy/chat_agent.py (1)
1-48: Concise and flexible design.The
ChatAgentabstraction, supported byChatContextandChatAgentResponse, cleanly defines the interface for agent implementations, promoting extensibility.app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (2)
10-15: Refactor duplicate import ofAgentType.
AgentTypeis imported from two separate modules, causing redefinition.-from app.modules.intelligence.provider.provider_service import ( - ProviderService, - AgentType, -) ... -from app.modules.intelligence.prompts.classification_prompts import ( - ClassificationPrompts, - AgentType, -) +from app.modules.intelligence.provider.provider_service import ProviderService +from app.modules.intelligence.prompts.classification_prompts import ( + ClassificationPrompts, + AgentType, +)
54-57: Fix comparison to None and improve error handling.The comparison to None should use
isoperator, and the error handling should be consistent.- if system_prompt == None: - # raise ValueError(f"System Prompt for {self.agent_type} not found!!") - logger.error(f"System Prompt for {self.agent_type} not found!!") + if system_prompt is None: + error_msg = f"System Prompt for {self.agent_type} not found!" + logger.error(error_msg) + raise ValueError(error_msg)🧰 Tools
🪛 Ruff (0.8.2)
54-54: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
app/modules/intelligence/tools/tool_service.py (1)
53-56: LGTM!The addition of new tool instances follows the existing patterns and is well-integrated with the service.
test.py (2)
1-2: Convert this temporary test into proper test cases.The file is marked for deletion, but it contains valuable test scenarios. Consider:
- Moving these tests into proper test files under a
testsdirectory.- Organizing tests by agent type.
- Using a test framework like
pytest.
53-58: Remove unused variables and make IDs configurable.
- The
user_idandproject_idare hardcoded and should be configurable.- The
agentandauto_router_agentvariables are assigned but never used.Also applies to: 93-110
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (5)
75-76: Remove or usenode_idsvariable.You assign
node_ids = [ctx.node_ids]but never use it, which can confuse readers and trigger linting issues.🧰 Tools
🪛 Ruff (0.8.2)
76-76: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
37-37: Define a more specific type for the 'tools' parameter.The
toolsparameter is currently typed asList[Any]. Consider defining a dedicated interface or type to make the contract clear and reduce potential runtime errors.
58-67: LGTM! Well-structured tool description generation.The method handles both tools with and without args_schema appropriately, providing a clear description format.
75-76: Remove unused variable assignment.The
node_idsvariable is assigned but never used.- if isinstance(ctx.node_ids, str): - node_ids = [ctx.node_ids] + if isinstance(ctx.node_ids, str): + ctx.node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
76-76: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
128-154: LGTM! Robust implementation with proper error handling.The method includes comprehensive error handling with logging and proper task execution flow.
app/modules/intelligence/agents_copy/agents_service.py (1)
36-40: LGTM!The
AgentInfoPydantic model is well-structured with appropriate fields and types.app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
67-73: Cache the agent instance to avoid rebuilding.The agent is rebuilt for every request in both
runandrun_streammethods.Apply this diff to implement agent caching:
class CodeGenAgent(ChatAgent): def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): self.llm_provider = llm_provider self.tools_provider = tools_provider + self._agent = None + + @property + def agent(self) -> ChatAgent: + if self._agent is None: + self._agent = self._build_agent() + return self._agent async def run(self, ctx: ChatContext) -> ChatAgentResponse: - return await self._build_agent().run(await self._enriched_context(ctx)) + return await self.agent.run(await self._enriched_context(ctx)) async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: - return await self._build_agent().run_stream(await self._enriched_context(ctx)) + return await self.agent.run_stream(await self._enriched_context(ctx))app/modules/intelligence/prompts/classification_prompts.py (1)
491-492: LGTM!The simplified
get_classification_promptmethod correctly handles missing agent types and returns only the classification prompt.app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (1)
49-55: LGTM! Clean implementation of run methods.The methods properly delegate to rag_agent with enriched context.
| self, | ||
| conversation_id: str, | ||
| user_id: str, | ||
| node_ids: List[NodeContext] = [], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using a mutable default argument.
Using [] as a default argument may lead to unexpected behavior. Use None and initialize it inside the function.
- async def regenerate_last_message(
- self, conversation_id: str, user_id: str, node_ids: List[NodeContext] = [], stream: bool = True
- ) -> AsyncGenerator[ChatMessageResponse, None]:
+ async def regenerate_last_message(
+ self, conversation_id: str, user_id: str, node_ids: Optional[List[NodeContext]] = None, stream: bool = True
+ ) -> AsyncGenerator[ChatMessageResponse, None]:
+ if node_ids is None:
+ node_ids = []📝 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.
| node_ids: List[NodeContext] = [], | |
| async def regenerate_last_message( | |
| self, conversation_id: str, user_id: str, node_ids: Optional[List[NodeContext]] = None, stream: bool = True | |
| ) -> AsyncGenerator[ChatMessageResponse, None]: | |
| if node_ids is None: | |
| node_ids = [] | |
| # ... rest of the function implementation |
🧰 Tools
🪛 Ruff (0.8.2)
407-407: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| # classify the query into agent needed or not | ||
| classification_response = await self.classifier.run(ctx) | ||
| classification = "AGENT_REQUIRED" | ||
| print("Classification response:", classification_response.response) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove print statements from production code.
Debug print statements should be replaced with proper logging.
- print("Classification response:", classification_response.response)
+ logger.debug("Classification response: %s", classification_response.response)Also applies to: 96-96
| async def get_agent(self, agent_id: str) -> Any: | ||
| """Get an agent instance by ID""" | ||
| if agent_id in self.system_agents: | ||
| return self.system_agents[agent_id] | ||
| else: | ||
| # For custom agents, we need to validate and get the system prompt | ||
| # if await validate_agent(self.sql_db, self.user_id, agent_id): | ||
| # reasoning_llm = self.provider_service.get_large_llm( | ||
| # agent_type=AgentType.LANGCHAIN | ||
| # ) | ||
| # return CustomAgent( | ||
| # llm=reasoning_llm, | ||
| # db=self.sql_db, | ||
| # agent_id=agent_id, | ||
| # user_id=self.user_id, | ||
| # ) | ||
| # else: | ||
| # raise ValueError(f"Invalid agent ID: {agent_id}") | ||
| pass | ||
|
|
There was a problem hiding this comment.
Implement the custom agent validation logic.
The get_agent method has commented-out code and a bare pass statement. This could lead to unexpected behavior when trying to retrieve custom agents.
Would you like me to help implement the custom agent validation logic? I can generate the code to:
- Validate the custom agent using the
validate_agentfunction - Create and return a new
CustomAgentinstance if validation passes - Raise a
ValueErrorif validation fails
| rag_agent=low_level_design_agent.LowLevelDesignAgent( | ||
| llm_provider, tools_provider | ||
| ), | ||
| agent_type=AgentType.QNA, # TODO: fix this, currently LLD prompt doesn't exist |
There was a problem hiding this comment.
Fix the incorrect agent type for LLD agent.
The LLD_agent is using AgentType.QNA instead of AgentType.LLD.
Apply this diff to fix the agent type:
- agent_type=AgentType.QNA, # TODO: fix this, currently LLD prompt doesn't exist
+ agent_type=AgentType.LLD,📝 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.
| agent_type=AgentType.QNA, # TODO: fix this, currently LLD prompt doesn't exist | |
| agent_type=AgentType.LLD, |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (1)
54-57:⚠️ Potential issueFix comparison to None and restore error handling.
The comparison to None should use the
isoperator, and the error handling should be restored.Apply this diff:
- if system_prompt == None: - # raise ValueError(f"System Prompt for {self.agent_type} not found!!") - logger.error(f"System Prompt for {self.agent_type} not found!!") + if system_prompt is None: + error_msg = f"System Prompt for {self.agent_type} not found!" + logger.error(error_msg) + raise ValueError(error_msg)🧰 Tools
🪛 Ruff (0.8.2)
54-54: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
app/modules/conversations/conversation/conversation_service_copy.py (1)
399-399: 🛠️ Refactor suggestionAvoid using a mutable default argument.
Using
[]as a default fornode_idscan cause unexpected behavior. UseNonefor the default and initialize it inside the function:- async def regenerate_last_message( - self, - conversation_id: str, - user_id: str, - node_ids: List[NodeContext] = [], - stream: bool = True, - ) -> AsyncGenerator[ChatMessageResponse, None]: + async def regenerate_last_message( + self, + conversation_id: str, + user_id: str, + node_ids: Optional[List[NodeContext]] = None, + stream: bool = True, + ) -> AsyncGenerator[ChatMessageResponse, None]: + if node_ids is None: + node_ids = []🧰 Tools
🪛 Ruff (0.8.2)
399-399: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (8)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (3)
2-2: Remove the unusedosimport.This import is never referenced in the file and can be safely removed to keep the code clean.
- import os🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
5-5: Remove the unusedagentopsimport.It is currently commented out and not utilized, so removing it will streamline the code.
- import agentops🧰 Tools
🪛 Ruff (0.8.2)
5-5:
agentopsimported but unusedRemove unused import:
agentops(F401)
35-35: Rename class to avoid “CrewAI” and “RAG” if undesired.There's feedback in a previous review that both “CrewAI” and “RAG” should be removed from the name. Consider renaming this class to better reflect its functionality while aligning with naming preferences.
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (2)
34-42: Improve classification prompt handling.The classification prompt modification is hardcoded as a string concatenation. Consider moving this to the prompt template system for better maintainability and consistency.
Consider this approach:
- classification_prompt = ClassificationPrompts.get_classification_prompt( - agent_type - ) - self.classifier = LLM( - llm_provider, - prompt_template=classification_prompt - + " just return the single classification in response ", - ) + classification_prompt = ClassificationPrompts.get_classification_prompt( + agent_type, + response_format="single" # Add a parameter to control response format + ) + self.classifier = LLM( + llm_provider, + prompt_template=classification_prompt, + )
60-65: Improve default system prompt construction.The default system prompt is constructed using string concatenation, which can be error-prone and harder to maintain.
Consider using a template:
- prompt_template=( - system_prompt.text - if system_prompt - else f"you are a {self.agent_type} agent " - + " who has complete understading of repo. With the given history of chat: {history} \nAnswer the following query with given info: {query}" - ), + prompt_template=( + system_prompt.text if system_prompt + else ( + f"You are a {self.agent_type} agent who has complete understanding " + f"of the repository.\n\n" + f"Chat history: {{history}}\n" + f"Please answer the following query with the given information: {{query}}" + ) + ),Note: Fixed typo in "understading" → "understanding"
app/modules/conversations/conversation/conversation_service_copy.py (3)
5-6: Remove unused imports.Both
AnyandHumanMessageare imported but never used. Removing them helps keep the import list clean and prevents confusion about unused dependencies.Apply this diff to remove them:
-from typing import Any, AsyncGenerator, List +from typing import AsyncGenerator, List -from langchain.schema import HumanMessage🧰 Tools
🪛 Ruff (0.8.2)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
6-6:
langchain.schema.HumanMessageimported but unusedRemove unused import:
langchain.schema.HumanMessage(F401)
363-363: Avoid usingsetattrwith a constant attribute name.Instead of using
setattr(conversation, "human_message_count", human_message_count), directly assignconversation.human_message_count = human_message_count. This is clearer and considered more Pythonic.- setattr(conversation, "human_message_count", human_message_count) + conversation.human_message_count = human_message_count🧰 Tools
🪛 Ruff (0.8.2)
363-363: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
532-533: Chain exceptions or remove the unused variable.Currently,
eis caught but never used, and the exception is re-raised without preserving the traceback. If you need the original traceback, chain the exception withfrom e. Otherwise, remove the unused variable to clean up the code.except Exception as e: - raise ConversationServiceError("Failed to get chat history") + raise ConversationServiceError("Failed to get chat history") from e🧰 Tools
🪛 Ruff (0.8.2)
532-532: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
533-533: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/modules/conversations/conversation/conversation_service_copy.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
2-2: os imported but unused
Remove unused import: os
(F401)
5-5: agentops imported but unused
Remove unused import: agentops
(F401)
79-79: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
app/modules/conversations/conversation/conversation_service_copy.py
5-5: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
6-6: langchain.schema.HumanMessage imported but unused
Remove unused import: langchain.schema.HumanMessage
(F401)
363-363: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
399-399: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
438-438: Undefined name full_response
(F821)
532-532: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
533-533: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
54-54: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (4)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (2)
40-40: Use a more specific type annotation fortools.A past review already suggested this; the current
List[Any]can lead to hidden bugs or mismatched usage. Defining a more precise type or interface would enhance clarity and correctness.
78-79: Remove or utilize thenode_idsvariable.You assign
node_ids = [ctx.node_ids]but never use it thereafter. This is a repeat of a previous review concern.- if isinstance(ctx.node_ids, str): - node_ids = [ctx.node_ids] + if isinstance(ctx.node_ids, str): + ctx.node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
79-79: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (2)
9-15: Remove duplicate import ofAgentType.The
AgentTypeis imported from two different modules, which can lead to confusion and potential issues.Apply this diff to fix the imports:
-from app.modules.intelligence.provider.provider_service import ( - ProviderService, -) -from app.modules.intelligence.prompts.classification_prompts import ( - ClassificationPrompts, - AgentType, -) +from app.modules.intelligence.provider.provider_service import ProviderService +from app.modules.intelligence.prompts.classification_prompts import ( + ClassificationPrompts, + AgentType, +)
72-72: Replace print statement with proper logging.Debug print statements should be replaced with proper logging.
Apply this diff:
- print("Classification response:", classification_response.response) + logger.debug("Classification response: %s", classification_response.response)
| async def run(self, ctx: ChatContext) -> ChatAgentResponse: | ||
| # classify the query into agent needed or not | ||
| classification_response = await self.classifier.run(ctx) | ||
| classification = "AGENT_REQUIRED" | ||
| print("Classification response:", classification_response.response) | ||
| try: | ||
| classification_json = json.loads(classification_response.response) | ||
| if ( | ||
| classification_json | ||
| and classification_json["classification"] == "LLM_SUFFICIENT" | ||
| ): | ||
| classification = "LLM_SUFFICIENT" | ||
| except Exception: | ||
| pass | ||
|
|
||
| if classification == "AGENT_REQUIRED": | ||
| rag_agent_response = await self.rag_agent.run(ctx) | ||
| ctx.query += f" with information: {rag_agent_response.response} and citations: {rag_agent_response.citations}" | ||
|
|
||
| # build llm response | ||
| llm = await self._create_llm() | ||
| return await llm.run(ctx) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated classification logic.
The classification logic is duplicated between run and run_stream methods.
Consider extracting the common logic into a private method:
+ async def _classify_query(self, ctx: ChatContext) -> str:
+ classification_response = await self.classifier.run(ctx)
+ logger.debug("Classification response: %s", classification_response.response)
+
+ try:
+ classification_json = json.loads(classification_response.response)
+ if (
+ classification_json
+ and classification_json["classification"] == "LLM_SUFFICIENT"
+ ):
+ return "LLM_SUFFICIENT"
+ except json.JSONDecodeError as e:
+ logger.warning("Failed to parse classification response: %s", e)
+ except KeyError as e:
+ logger.warning("Missing classification key in response: %s", e)
+
+ return "AGENT_REQUIRED"
+
async def run(self, ctx: ChatContext) -> ChatAgentResponse:
- classification_response = await self.classifier.run(ctx)
- classification = "AGENT_REQUIRED"
- print("Classification response:", classification_response.response)
- try:
- classification_json = json.loads(classification_response.response)
- if (
- classification_json
- and classification_json["classification"] == "LLM_SUFFICIENT"
- ):
- classification = "LLM_SUFFICIENT"
- except Exception:
- pass
+ classification = await self._classify_query(ctx)Apply similar changes to run_stream method.
Also applies to: 91-114
| try: | ||
| classification_json = json.loads(classification_response.response) | ||
| if ( | ||
| classification_json | ||
| and classification_json["classification"] == "LLM_SUFFICIENT" | ||
| ): | ||
| classification = "LLM_SUFFICIENT" | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling.
The bare except clause silently ignores all exceptions, which could hide important issues.
Apply this diff:
try:
classification_json = json.loads(classification_response.response)
if (
classification_json
and classification_json["classification"] == "LLM_SUFFICIENT"
):
classification = "LLM_SUFFICIENT"
- except Exception:
+ except json.JSONDecodeError as e:
+ logger.warning("Failed to parse classification response: %s", e)
+ pass
+ except KeyError as e:
+ logger.warning("Missing classification key in response: %s", e)
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.
| try: | |
| classification_json = json.loads(classification_response.response) | |
| if ( | |
| classification_json | |
| and classification_json["classification"] == "LLM_SUFFICIENT" | |
| ): | |
| classification = "LLM_SUFFICIENT" | |
| except Exception: | |
| pass | |
| try: | |
| classification_json = json.loads(classification_response.response) | |
| if ( | |
| classification_json | |
| and classification_json["classification"] == "LLM_SUFFICIENT" | |
| ): | |
| classification = "LLM_SUFFICIENT" | |
| except json.JSONDecodeError as e: | |
| logger.warning("Failed to parse classification response: %s", e) | |
| pass | |
| except KeyError as e: | |
| logger.warning("Missing classification key in response: %s", e) | |
| pass |
|
|
||
| # Store the complete response as a single message | ||
| self.history_manager.add_message_chunk( | ||
| conversation_id, full_response, MessageType.AI, user_id |
There was a problem hiding this comment.
Fix undefined variable full_response.
The variable full_response is referenced but never defined, causing a runtime error. If you intended to store the accumulated message in a variable named full_response, initialize and update that variable similarly to full_message. For example:
- self.history_manager.add_message_chunk(
- conversation_id, full_response, MessageType.AI, user_id
- )
+ self.history_manager.add_message_chunk(
+ conversation_id, full_message, MessageType.AI, user_id
+ )📝 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.
| conversation_id, full_response, MessageType.AI, user_id | |
| self.history_manager.add_message_chunk( | |
| conversation_id, full_message, MessageType.AI, user_id | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
438-438: Undefined name full_response
(F821)
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
app/modules/conversations/conversation/conversation_service_copy.py (1)
437-439:⚠️ Potential issueFix undefined variable reference.
The variable
full_responseis undefined. This will cause a runtime error.- conversation_id, full_response, MessageType.AI, user_id + conversation_id, full_message, MessageType.AI, user_id🧰 Tools
🪛 Ruff (0.8.2)
438-438: Undefined name
full_response(F821)
🧹 Nitpick comments (22)
app/core/models.py (1)
14-16: Unused Import: Consider RemovingCustomAgent.The updated import now points to the new location in
agents_copy, which aligns with the refactoring goals. However, static analysis (Ruff) indicates thatCustomAgentis imported but not used in this module. If this import is unnecessary, please remove it to keep the code clean and maintainable.🧰 Tools
🪛 Ruff (0.8.2)
15-15:
app.modules.intelligence.agents_copy.custom_agents.custom_agent_model.CustomAgentimported but unusedRemove unused import:
app.modules.intelligence.agents_copy.custom_agents.custom_agent_model.CustomAgent(F401)
app/modules/conversations/conversation/conversation_service_copy.py (3)
1-53: Remove unused imports to maintain clean code.The following imports are not used in the code:
AnyfromtypingHumanMessagefromlangchain.schemaCustomAgentfromapp.modules.intelligence.agents.custom_agents.custom_agent-from typing import Any, AsyncGenerator, List +from typing import AsyncGenerator, List -from langchain.schema import HumanMessage from langchain.prompts import ChatPromptTemplate -from app.modules.intelligence.agents.custom_agents.custom_agent import CustomAgent from app.modules.intelligence.agents.custom_agents.custom_agents_service import ( CustomAgentService, )🧰 Tools
🪛 Ruff (0.8.2)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
6-6:
langchain.schema.HumanMessageimported but unusedRemove unused import:
langchain.schema.HumanMessage(F401)
35-35:
app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgentimported but unusedRemove unused import:
app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgent(F401)
361-364: Simplify attribute assignment.Using
setattrwith a constant attribute name is unnecessary. Direct attribute assignment is cleaner and more readable.- setattr(conversation, "human_message_count", human_message_count) + conversation.human_message_count = human_message_count🧰 Tools
🪛 Ruff (0.8.2)
363-363: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
532-533: Improve error handling.The exception handling can be improved by properly chaining the exception and utilizing the error information.
- except Exception as e: - raise ConversationServiceError("Failed to get chat history") + except Exception as e: + raise ConversationServiceError("Failed to get chat history") from e🧰 Tools
🪛 Ruff (0.8.2)
532-532: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
533-533: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
app/modules/intelligence/agents_copy/agents_service.py (3)
2-2: Remove the unusedAnyimport.The static analysis reports that
Anyis never used. Removing it keeps the codebase clean.-from typing import Any, List +from typing import List🧰 Tools
🪛 Ruff (0.8.2)
2-2:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
163-185: Consider error-handling strategy for custom agents.When fetching custom agents, any exception is logged and the process continues. While this avoids blocking on partial failures, it can mask severe issues. Consider:
- Custom exception handling or re-raising.
- Providing fallback or alternative error messages to users.
- Monitoring repeated failures in logs.
200-207: Return type consistency suggestion.
validate_agent_idreturns string literals"SYSTEM_AGENT"or"CUSTOM_AGENT", orNoneas a sentinel. This is fine, but consider using an Enum or a well-defined type for clarity and maintainability.app/modules/intelligence/agents_copy/custom_agents/custom_agent.py (3)
66-70: Unifyagent_idusage or clarify parameter duplication.Currently,
agent_idis stored inself.agent_idat initialization, yetagent_idis also passed to therunmethod. This could lead to confusion if the two values ever diverge. Consider removing the redundant parameter and using the class attribute consistently, or clarifying why both are needed.
79-82: Improve clarity in error handling.Raising a
ValueErrorhere is correct when the agent is not found. For more contextual debugging, you could supplement or chain any original exception or include more details (e.g., user role, environment, etc.) if applicable.
103-120: Ensure robust error handling paths.The error handling and logging approach is solid. However, certain exceptions (e.g., network timeouts, DB connection errors) might warrant retry logic or specialized handling. Consider whether different exception types deserve more tailored handling or user-friendly messages to improve reliability and diagnostic clarity.
app/modules/intelligence/agents_copy/custom_agents/agent_executor.py (1)
89-108: Validate custom agent outputs before using them.Here, you're appending the "Custom Agent result" into
tool_resultsas aSystemMessage. If the custom agent or tool produces malformed JSON,json.dumps(custom_agent_result)might fail. Consider validating or wrapping these calls with error handling to safeguard against future tool expansions that may produce unstructured outputs.app/modules/intelligence/agents_copy/custom_agents/runtime_agent.py (3)
33-37: Chain exceptions for better debugging.When re-raising within an
exceptblock, consider chaining the exception usingraise ValueError(...) from e. This preserves the context of the original exception and can aid debugging.- raise ValueError("Invalid JSON format for expected_output") + raise ValueError("Invalid JSON format for expected_output") from e🧰 Tools
🪛 Ruff (0.8.2)
37-37: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
189-191: Remove or clarify commented-out code.These lines for updating
project_idin the tools are commented out. If no longer needed, removing them helps keep the code clean. Otherwise, add a clear comment explaining why it’s temporarily commented.
199-199: Rename unused loop variable.
task_configis never referenced within the loop. Renaming it to_communicates that it’s intentionally unused.-for i, task_config in enumerate(self.config.tasks): +for i, _ in enumerate(self.config.tasks): task = await self.create_task(query, node_ids, task_index=i) tasks.append(task)🧰 Tools
🪛 Ruff (0.8.2)
199-199: Loop control variable
task_confignot used within loop bodyRename unused
task_configto_task_config(B007)
app/modules/intelligence/agents_copy/custom_agents/custom_agents_service.py (1)
190-192: Consider converting into an async method for consistency.All other agent service operations are asynchronous. Making
list_agentsasynchronous may maintain uniformity in the service’s interface.app/modules/intelligence/agents_copy/custom_agents/custom_agent_model.py (2)
12-12: Consider indexing theuser_idcolumn.If lookups by
user_idandidoccur frequently, adding an index can improve performance for larger datasets.CREATE INDEX IF NOT EXISTS idx_custom_agents_user_id ON custom_agents (user_id);
17-17: Evaluate storing tasks in a dedicated table.Long or complex tasks might benefit from their own table with one-to-many relationships instead of embedding them as JSON. This can enhance querying, indexing, and data integrity.
app/modules/intelligence/agents_copy/custom_agents/custom_agent_schema.py (3)
30-34: Consider adding field validation for string attributes.Add constraints to ensure non-empty strings and reasonable length limits for the fields.
class AgentBase(BaseModel): - role: str - goal: str - backstory: str - system_prompt: str + role: str = Field(..., min_length=1, max_length=200) + goal: str = Field(..., min_length=1, max_length=500) + backstory: str = Field(..., min_length=1, max_length=1000) + system_prompt: str = Field(..., min_length=1, max_length=2000)
37-46: Extract magic numbers into constants.Consider moving the task limit values into named constants for better maintainability.
+MIN_TASKS = 1 +MAX_TASKS = 5 + class AgentCreate(AgentBase): tasks: List[TaskCreate] @validator("tasks") def validate_tasks(cls, tasks): if not tasks: - raise ValueError("At least one task is required") - if len(tasks) > 5: - raise ValueError("Maximum of 5 tasks allowed") + raise ValueError(f"At least {MIN_TASKS} task is required") + if len(tasks) > MAX_TASKS: + raise ValueError(f"Maximum of {MAX_TASKS} tasks allowed") return tasks
57-68: Consider using an Enum for deployment_status.Using an Enum would provide type safety and clear documentation of valid states.
+from enum import Enum + +class DeploymentStatus(str, Enum): + PENDING = "pending" + DEPLOYED = "deployed" + FAILED = "failed" + class Agent(AgentBase): id: str user_id: str tasks: List[Task] deployment_url: Optional[str] = None created_at: datetime updated_at: datetime - deployment_status: Optional[str] + deployment_status: Optional[DeploymentStatus] = Noneapp/modules/intelligence/agents_copy/custom_agents/custom_agent_controller.py (1)
20-24: Avoid using Depends in constructor parameters.Move the dependency injection into a class method or use a module-level singleton.
class CustomAgentController: - def __init__(self, db: Session = Depends(get_db)): + def __init__(self, db: Session): self.db = db self.service = CustomAgentService(db) + + @classmethod + def from_request(cls, db: Session = Depends(get_db)) -> "CustomAgentController": + return cls(db)🧰 Tools
🪛 Ruff (0.8.2)
21-21: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/intelligence/agents_copy/custom_agents/custom_agent_router.py (1)
32-39: Simplify agent creation by passing the entire request object.Instead of passing individual fields, pass the entire request object to maintain consistency.
return await custom_agent_controller.create_agent( user_id=user_id, - role=request.role, - goal=request.goal, - backstory=request.backstory, - system_prompt=request.system_prompt, - tasks=[task.model_dump(exclude_unset=True) for task in request.tasks], + agent_data=request, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/core/models.py(1 hunks)app/main.py(1 hunks)app/modules/conversations/conversation/conversation_service_copy.py(1 hunks)app/modules/intelligence/agents/custom_agents/custom_agent_model.py(1 hunks)app/modules/intelligence/agents/custom_agents/custom_agents_service.py(1 hunks)app/modules/intelligence/agents_copy/agents_router.py(1 hunks)app/modules/intelligence/agents_copy/agents_service.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/agent_executor.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/agent_validator.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agent.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agent_controller.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agent_model.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agent_router.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agent_schema.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/custom_agents_service.py(1 hunks)app/modules/intelligence/agents_copy/custom_agents/runtime_agent.py(1 hunks)app/modules/users/user_model.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- app/modules/users/user_model.py
- app/modules/intelligence/agents/custom_agents/custom_agents_service.py
- app/main.py
- app/modules/intelligence/agents/custom_agents/custom_agent_model.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/intelligence/agents_copy/custom_agents/custom_agent_controller.py
1-1: typing.List imported but unused
Remove unused import: typing.List
(F401)
21-21: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
70-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
107-110: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/custom_agents/runtime_agent.py
37-37: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
199-199: Loop control variable task_config not used within loop body
Rename unused task_config to _task_config
(B007)
app/modules/intelligence/agents_copy/custom_agents/custom_agents_service.py
65-67: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
138-138: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
141-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
240-240: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
296-298: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
322-322: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
363-363: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
366-366: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
369-369: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
464-466: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/agents_service.py
2-2: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
app/modules/conversations/conversation/conversation_service_copy.py
5-5: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
6-6: langchain.schema.HumanMessage imported but unused
Remove unused import: langchain.schema.HumanMessage
(F401)
35-35: app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgent imported but unused
Remove unused import: app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgent
(F401)
363-363: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
399-399: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
438-438: Undefined name full_response
(F821)
532-532: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
533-533: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/custom_agents/custom_agent_router.py
25-25: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
50-50: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
51-51: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
69-69: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
87-87: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
88-88: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
105-105: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
106-106: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/core/models.py
15-15: app.modules.intelligence.agents_copy.custom_agents.custom_agent_model.CustomAgent imported but unused
Remove unused import: app.modules.intelligence.agents_copy.custom_agents.custom_agent_model.CustomAgent
(F401)
app/modules/intelligence/agents_copy/agents_router.py
36-36: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (7)
app/modules/intelligence/agents_copy/agents_service.py (1)
127-127: Fix incorrect agent type for the LLD agent.As noted in a previous commit review, the LLD agent is being set to
AgentType.QNAinstead ofAgentType.LLD. Consider fixing it if/when a proper LLD prompt or agent type is introduced.- agent_type=AgentType.QNA, # TODO: fix this, currently LLD prompt doesn't exist + agent_type=AgentType.LLD,app/modules/intelligence/agents_copy/agents_router.py (1)
36-37: Ignore static analysis false positive for FastAPI dependencies.Static analysis warns against calling
Depends(...)in function parameters. However, this is the standard and recommended pattern for FastAPI dependency injection. It's safe to use as is.🧰 Tools
🪛 Ruff (0.8.2)
36-36: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
37-37: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/intelligence/agents_copy/custom_agents/agent_executor.py (2)
65-72: Async generator usage is well-structured.The
runmethod properly employs an async generator for streaming partial results from the LLM, which is beneficial for responsiveness. Good job leveraging this pattern for handling incremental outputs.
129-136: Confirm chunk format during LLM streaming.When yielding streamed chunks, ensure
chunkalways has the correct format (e.g.,contentattribute) to avoid potential attribute errors. If there's a case where the chunk might be a different object (e.g., error responses), add a guard or fallback.app/modules/intelligence/agents_copy/custom_agents/custom_agents_service.py (1)
212-219: Verify concurrency handling for DB session lookup.The code retrieves the agent via a synchronous
.query(...)pattern. If multiple async tasks share the same session, ensure the session is safe to use concurrently or create separate sessions.Would you like me to check usage across the codebase for session concurrency issues?
app/modules/intelligence/agents_copy/custom_agents/agent_validator.py (1)
12-23: Straightforward validation approach looks good.The function’s logic is clear, and returning a boolean on success is simple and effective. No concerns here.
app/modules/intelligence/agents_copy/custom_agents/custom_agent_schema.py (1)
7-16: LGTM! Well-structured base class with proper validation.The TaskBase class provides a solid foundation with proper validation of the expected_output field ensuring it's a JSON object.
| async def regenerate_last_message( | ||
| self, | ||
| conversation_id: str, | ||
| user_id: str, | ||
| node_ids: List[NodeContext] = [], | ||
| stream: bool = True, | ||
| ) -> AsyncGenerator[ChatMessageResponse, None]: |
There was a problem hiding this comment.
Fix mutable default argument.
Using a mutable default argument ([]) can lead to unexpected behavior as the same list object will be shared across all function calls.
- node_ids: List[NodeContext] = [],
+ node_ids: Optional[List[NodeContext]] = None,Add this import at the top of the file:
from typing import Optional🧰 Tools
🪛 Ruff (0.8.2)
399-399: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| logger.error( | ||
| f"JSON parsing error: {str(e)}\nResponse text: {response_text}" | ||
| ) | ||
| raise ValueError(f"Failed to parse agent plan: {str(e)}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include original JSON decoding error details.
Adopting raise … from e provides a more descriptive traceback for diagnosing plan parsing failure.
- raise ValueError(f"Failed to parse agent plan: {str(e)}")
+ raise ValueError(f"Failed to parse agent plan: {str(e)}") from e📝 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.
| raise ValueError(f"Failed to parse agent plan: {str(e)}") | |
| raise ValueError(f"Failed to parse agent plan: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
363-363: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| ) | ||
| return {"message": result["response"]} | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage raise … from e for consistency in error-handling.
Preserving the underlying exception ensures complete traceability when debugging issues with agent runtime executions.
- raise HTTPException(status_code=500, detail=str(e))
+ raise HTTPException(status_code=500, detail=str(e)) from e📝 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.
| raise HTTPException(status_code=500, detail=str(e)) | |
| raise HTTPException(status_code=500, detail=str(e)) from e |
🧰 Tools
🪛 Ruff (0.8.2)
240-240: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise HTTPException(status_code=500, detail="Failed to create agent") | ||
| except Exception as e: | ||
| logger.error(f"Error creating agent: {str(e)}") | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context for debugging.
Likewise, chaining the exception ensures the original error details remain accessible.
- raise HTTPException(status_code=500, detail=str(e))
+ raise HTTPException(status_code=500, detail=str(e)) from e📝 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.
| raise HTTPException(status_code=500, detail=str(e)) | |
| raise HTTPException(status_code=500, detail=str(e)) from e |
🧰 Tools
🪛 Ruff (0.8.2)
141-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise ValueError( | ||
| "Invalid JSON format for expected_output in a task." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use raise … from e to preserve the original exception context.
Currently, the code swallows the underlying json.JSONDecodeError. Incorporating raise … from e provides full traceback and highlights precisely what triggered the parse failure.
- except json.JSONDecodeError:
- raise ValueError("Invalid JSON format for expected_output in a task.")
+ except json.JSONDecodeError as e:
+ raise ValueError("Invalid JSON format for expected_output in a task.") from e📝 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.
| raise ValueError( | |
| "Invalid JSON format for expected_output in a task." | |
| ) | |
| - except json.JSONDecodeError: | |
| - raise ValueError("Invalid JSON format for expected_output in a task.") | |
| + except json.JSONDecodeError as e: | |
| + raise ValueError("Invalid JSON format for expected_output in a task.") from e |
🧰 Tools
🪛 Ruff (0.8.2)
65-67: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise HTTPException( | ||
| status_code=500, detail="Failed to fetch available tools" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adopt raise … from e for clarity.
Provides complete detail of the underlying error when a tool retrieval failure occurs.
- raise HTTPException(
- status_code=500, detail="Failed to fetch available tools"
- )
+ raise HTTPException(
+ status_code=500, detail=f"Failed to fetch available tools: {str(e)}"
+ ) from e📝 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.
| raise HTTPException( | |
| status_code=500, detail="Failed to fetch available tools" | |
| ) | |
| raise HTTPException( | |
| status_code=500, detail=f"Failed to fetch available tools: {str(e)}" | |
| ) from e |
🧰 Tools
🪛 Ruff (0.8.2)
464-466: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise ValueError(f"Failed to parse agent plan: {str(e)}") | ||
| except ValueError as e: | ||
| logger.error(f"Validation error: {str(e)}\nResponse text: {response_text}") | ||
| raise ValueError(str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Report the original ValueError cause.
Similarly, chain the ValueError to keep the underlying error context.
- raise ValueError(str(e))
+ raise ValueError(str(e)) from e📝 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.
| raise ValueError(str(e)) | |
| raise ValueError(str(e)) from e |
🧰 Tools
🪛 Ruff (0.8.2)
366-366: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise ValueError(str(e)) | ||
| except Exception as e: | ||
| logger.error(f"Error creating agent from prompt: {str(e)}") | ||
| raise ValueError("Failed to create agent from prompt") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception’s traceback.
By chaining the generic exception, you ensure the reason for failing to create an agent remains evident.
- raise ValueError("Failed to create agent from prompt")
+ raise ValueError("Failed to create agent from prompt") from e📝 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.
| raise ValueError("Failed to create agent from prompt") | |
| raise ValueError("Failed to create agent from prompt") from e |
🧰 Tools
🪛 Ruff (0.8.2)
369-369: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| async def create_agent(self, user_id: str, agent_data: AgentCreate) -> Agent: | ||
| """Create a new custom agent""" | ||
| try: | ||
| return await self.service.create_agent(user_id, agent_data) | ||
| except Exception as e: | ||
| logger.error(f"Error creating custom agent: {str(e)}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Failed to create custom agent: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining for better error tracking.
Properly chain exceptions using raise ... from e to preserve the exception context.
async def create_agent(self, user_id: str, agent_data: AgentCreate) -> Agent:
"""Create a new custom agent"""
try:
return await self.service.create_agent(user_id, agent_data)
except Exception as e:
logger.error(f"Error creating custom agent: {str(e)}")
- raise HTTPException(
+ raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail=f"Failed to create custom agent: {str(e)}",
- )
+ ) from e📝 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.
| async def create_agent(self, user_id: str, agent_data: AgentCreate) -> Agent: | |
| """Create a new custom agent""" | |
| try: | |
| return await self.service.create_agent(user_id, agent_data) | |
| except Exception as e: | |
| logger.error(f"Error creating custom agent: {str(e)}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail=f"Failed to create custom agent: {str(e)}", | |
| ) | |
| async def create_agent(self, user_id: str, agent_data: AgentCreate) -> Agent: | |
| """Create a new custom agent""" | |
| try: | |
| return await self.service.create_agent(user_id, agent_data) | |
| except Exception as e: | |
| logger.error(f"Error creating custom agent: {str(e)}") | |
| raise HTTPException( | |
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | |
| detail=f"Failed to create custom agent: {str(e)}", | |
| ) from e |
🧰 Tools
🪛 Ruff (0.8.2)
31-34: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| async def create_custom_agent( | ||
| request: AgentCreate, | ||
| db: Session = Depends(get_db), | ||
| user=Depends(auth_handler.check_auth), | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor dependency injection pattern across endpoints.
Move Depends calls into the function body for better maintainability and to follow FastAPI best practices.
Example for one endpoint (apply similar pattern to others):
@router.post("/", response_model=Agent)
async def create_custom_agent(
request: AgentCreate,
- db: Session = Depends(get_db),
- user=Depends(auth_handler.check_auth),
+ db: Session,
+ user: dict,
):
+ db = Depends(get_db)
+ user = Depends(auth_handler.check_auth)Also applies to: 48-52, 65-70, 85-89, 103-107
🧰 Tools
🪛 Ruff (0.8.2)
25-25: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
72-78: 🛠️ Refactor suggestionCache the agent instance.
The agent is rebuilt for every request in both
runandrun_streammethods, which is inefficient.
🧹 Nitpick comments (24)
app/modules/intelligence/tools/tool_service.py (1)
61-67: Enhance error handling and type safety in get_tools method.The current implementation silently ignores non-existent tools and uses a generic return type.
Consider these improvements:
-def get_tools(self, tool_names: List[str]) -> List[Any]: +def get_tools(self, tool_names: List[str]) -> List[ToolInfo]: """get tools if exists""" + missing_tools = [name for name in tool_names if name not in self.tools] + if missing_tools: + raise ValueError(f"Tools not found: {', '.join(missing_tools)}") - tools = [] - for tool_name in tool_names: - if self.tools.get(tool_name) is not None: - tools.append(self.tools[tool_name]) - return tools + return [self.tools[name] for name in tool_names]app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (4)
10-18: Consider adding a docstring for the constructor.
Providing a clear summary of what dependencies the constructor expects and how they should be used can improve maintainability and readability.+ """ + Initialize the QnAAgent with a language model provider and a tools provider. + :param llm_provider: ProviderService for the language model or RAG functionalities. + :param tools_provider: ToolService offering various code retrieval and analysis tools. + """
19-51: Optionally consider reusing or caching theCrewAIRagAgentinstance.
Currently,_build_agentcreates a newCrewAIRagAgentwhenever it is called. If the agent’s configuration remains the same across multiple invocations, retaining a single instance may improve performance by avoiding repeated instantiation.
53-66: Add error handling for tool invocations.
If any of the tool calls (likerun_multipleorrun) fail, the method will silently continue. Consider catching exceptions or verifying successful calls to improve reliability and maintenance.if ctx.node_ids and len(ctx.node_ids) > 0: try: code_results = await self.tools_provider.get_code_from_multiple_node_ids_tool.run_multiple( ctx.project_id, ctx.node_ids ) ctx.additional_context += ( f"Code Graph context of the node_ids in query:\n {code_results}" ) except Exception as e: # handle or log the exception pass
75-158: Consider externalizing large prompts.
The multilineqna_task_promptis quite extensive. Storing large prompts or scripts in a dedicated configuration or template file can improve code readability and maintainability.app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (4)
10-18: Add docstring for constructor.
A concise docstring explaining the parameters and intended usage ofLowLevelDesignAgentimproves clarity.+ """ + Initialize the LowLevelDesignAgent. + :param llm_provider: ProviderService instance for language model operations. + :param tools_provider: ToolService instance for any code retrieval or analysis tasks. + """
19-47: Consider using a single instance ofCrewAIRagAgent.
If the configuration does not change across runs, you could save resources by constructing and storing theCrewAIRagAgentonce.
48-60: Handle potential errors when invoking tools.
Similar to the other agent, ifrun_multipleorrunencounters an error, it will currently be silent. Consider adding try/except or similar error-handling logic.
71-83: Storing the prompt externally could enhance maintainability.
This large multiline prompt might be easier to manage if placed in a dedicated file or prompt library.app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (1)
110-112: Consider removing commented-out code if no longer needed.
Leaving large commented-out code blocks can cause confusion. If it’s genuinely needed for reference, add a clarifying comment or move it to version control notes.app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py (10)
1-5: Remove unused imports.The
osandagentopsimports are not used in the code.Apply this diff to remove unused imports:
-import os -import agentops from typing import Any, List, AsyncGenerator🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
5-5:
agentopsimported but unusedRemove unused import:
agentops(F401)
80-81: Remove unused variable assignment.The
node_idsvariable is assigned but never used.Apply this diff to fix the issue:
- if isinstance(ctx.node_ids, str): - node_ids = [ctx.node_ids] + if isinstance(ctx.node_ids, str): + ctx.node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
81-81: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
169-172: Improve streaming implementation.The
run_streammethod doesn't actually stream responses, it just yields a single response fromrun. Consider implementing true streaming if the underlying CrewAI library supports it, or document this limitation.
1-5: Clean up unused imports.The following imports are not used in the code:
os(line 2)agentops(line 5)-import os import json from typing import Any, List, AsyncGenerator - -import agentops from app.modules.intelligence.provider.provider_service import ( ProviderService, AgentType, )🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
5-5:
agentopsimported but unusedRemove unused import:
agentops(F401)
80-81: Remove unused variable assignment.The
node_idsvariable is assigned but never used.if isinstance(ctx.node_ids, str): - node_ids = [ctx.node_ids] + ctx.node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
81-81: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
141-143: Remove commented out code.The commented out
agentopsinitialization code should either be removed or uncommented if needed.- # agentops.init( - # os.getenv("AGENTOPS_API_KEY"), default_tags=["openai-gpt-notebook"] - # )
162-162: Remove commented out code.The commented out
agentops.end_sessioncall should either be removed or uncommented if needed.- # agentops.end_session("success")
1-5: Remove unused imports.The following imports are not used in the code:
os(line 2)agentops(line 5)-import os import json from typing import Any, List, AsyncGenerator -import agentops from app.modules.intelligence.provider.provider_service import (🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
5-5:
agentopsimported but unusedRemove unused import:
agentops(F401)
80-81: Remove unused variable assignment.The
node_idsvariable is assigned but never used.if isinstance(ctx.node_ids, str): - node_ids = [ctx.node_ids] + ctx.node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
81-81: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
165-167: Improve error handling with specific exceptions.The current error handling catches all exceptions and re-raises them. Consider catching specific exceptions and providing more context in the error message.
- except Exception as e: - logger.error(f"Error in run method: {str(e)}", exc_info=True) - raise Exception from e + except (ValueError, TypeError) as e: + logger.error(f"Invalid input or type error in run method: {str(e)}", exc_info=True) + raise + except Exception as e: + logger.error(f"Unexpected error in run method: {str(e)}", exc_info=True) + raise RuntimeError(f"Failed to execute CrewAI tasks: {str(e)}") from eapp/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (3)
33-33: Reference task prompt constant.The
TaskConfigis using an undefined variablecode_gen_task_prompt. Move the prompt definition before its usage.Move the
code_gen_task_promptdefinition before line 33.
65-68: Consider string formatting alternatives.Using f-strings with multiline strings can be hard to read and maintain.
Consider using a template or dedicated formatting method:
- ctx.additional_context += ( - f"Code referred to in the query:\n {code_results}\n" - ) + ctx.additional_context += "\n".join([ + "Code referred to in the query:", + code_results, + "" + ])
16-50: Cache the rag_agent instance.The
rag_agentis initialized in the constructor but could be cached using a property to allow lazy initialization and potential reconfiguration.def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): self.tools_provider = tools_provider - self.rag_agent = CrewAIRagAgent( + self.llm_provider = llm_provider + self._rag_agent = None + + @property + def rag_agent(self) -> CrewAIRagAgent: + if self._rag_agent is None: + self._rag_agent = CrewAIRagAgent( llm_provider, AgentConfig( role="Context curation agent",app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (1)
62-70: Share context enrichment logic.The
_enriched_contextmethod is duplicated acrossDebugAgentandCodeGenAgent. Consider extracting this to a shared base class or utility.Create a new mixin or base class to share this functionality:
class ContextEnrichmentMixin: async def _enriched_context(self, ctx: ChatContext) -> ChatContext: if ctx.node_ids and len(ctx.node_ids) > 0: code_results = await self.tools_provider.get_code_from_multiple_node_ids_tool.run_multiple( ctx.project_id, ctx.node_ids ) ctx.additional_context += "\n".join([ "Code referred to in the query:", code_results, "" ]) return ctx class CodeGenAgent(ChatAgent, ContextEnrichmentMixin): # ... rest of the implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py(1 hunks)app/modules/intelligence/tools/tool_service.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py
👮 Files not reviewed due to content moderation or server errors (3)
- app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/intelligence/agents_copy/chat_agents/crewai_rag_agent.py
2-2: os imported but unused
Remove unused import: os
(F401)
5-5: agentops imported but unused
Remove unused import: agentops
(F401)
81-81: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
54-54: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (14)
app/modules/intelligence/tools/tool_service.py (1)
10-10:❓ Verification inconclusive
Verify the need for both function and class versions of tools.
The codebase now has both function-based tools (e.g.,
get_code_file_structure_tool) and class-based tools (e.g.,GetCodeFileStructureTool). This dual implementation pattern might lead to confusion about which version to use.Let's check if other tools follow this pattern and if there's a migration plan:
Also applies to: 17-17, 30-30
🏁 Script executed:
#!/bin/bash # Check for other tools following this pattern echo "Searching for tool implementations..." rg -l "class \w+Tool\b" app/modules/intelligence/tools/ rg -l "def get_\w+_tool\b" app/modules/intelligence/tools/Length of output: 2036
Action Required: Confirm and Document Dual Tool Implementations
The repository shows a consistent pattern where both class-based and function-based tool implementations coexist (for instance, in the file
app/modules/intelligence/tools/code_query_tools/get_code_file_structure.pyand others). Inapp/modules/intelligence/tools/tool_service.py(line 10), the use ofGetCodeFileStructureToolaligns with this pattern. However, this duality could lead to confusion over which interface to favor.
- Verify if maintaining both versions is intentional.
- If so, please add documentation or comments to clarify the intended use cases.
- Alternatively, if a shift to a single implementation (or a migration plan) is planned, provide guidance accordingly.
app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (3)
1-9: Imports look good.
All import statements appear to be properly referenced and necessary for this file, with no obvious redundancies or missing modules.
67-69: Synchronous agent build in an async method is acceptable.
No issues found here; the logic is concise. Just ensure any heavy or blocking operations in_build_agent()won't degrade performance.
70-73: Consistent approach with stream method.
Therun_streammethod mirrorsrunand maintains consistent context enrichment. Looks good.app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (3)
1-9: Imports look correct.
All references to provider and tool services match their usages below and appear necessary.
62-64: Straightforward async run.
No concerns here, the logic simply defers to the built agent.
65-69: Consistent streaming method.
This mirrors the logic inrun()and keeps the agent’s pattern consistent with other system agents.app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (3)
54-54: Preferis Noneover== None.
This is a well-known Python convention and best practice for None checks.- if system_prompt == None: + if system_prompt is None:🧰 Tools
🪛 Ruff (0.8.2)
54-54: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
72-72: Remove or replace debug print statements.
Print statements in production code can clutter logs and are harder to manage than structured logs withlogger.debug().- print("Classification response:", classification_response.response) + logger.debug("Classification response: %s", classification_response.response)Also applies to: 95-95
80-81: Use narrower exception handlers instead of broadexcept Exception.
Catching all exceptions can obscure important error details. Restricting to more specific exceptions (likejson.JSONDecodeErrororKeyError) can provide better debugging information and stability. This mirrors past review suggestions in other commits.- except Exception: - pass + except json.JSONDecodeError as e: + logger.warning("Failed to parse classification response: %s", e) + except KeyError as e: + logger.warning("Missing classification key in response: %s", e)Also applies to: 103-104
app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (1)
1-152: LGTM! Well-structured implementation with comprehensive task prompt.The implementation is clean and follows good practices:
- Proper type hints and error handling
- Well-structured task prompt with clear instructions
- Good use of CrewAIRagAgent for implementation
- Comprehensive context enrichment
app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (3)
72-78: Cache the agent instance to avoid rebuilding.The agent is rebuilt for every request in both
runandrun_streammethods. Consider caching the agent instance.Apply this diff to implement agent caching:
class CodeGenAgent(ChatAgent): def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): self.llm_provider = llm_provider self.tools_provider = tools_provider + self._agent = None + + @property + def agent(self) -> ChatAgent: + if self._agent is None: + self._agent = self._build_agent() + return self._agent async def run(self, ctx: ChatContext) -> ChatAgentResponse: - return await self._build_agent().run(await self._enriched_context(ctx)) + return await self.agent.run(await self._enriched_context(ctx)) async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: - return await self._build_agent().run_stream(await self._enriched_context(ctx)) + return await self.agent.run_stream(await self._enriched_context(ctx))
81-227: LGTM! Well-structured task prompt with comprehensive instructions.The task prompt is well-organized and covers all necessary aspects:
- Clear sections with emojis for visual separation
- Comprehensive dependency analysis
- Detailed implementation planning
- Clear code generation format
- Thorough verification steps
72-78: Cache the agent instance to avoid rebuilding.The agent is rebuilt for every request in both
runandrun_streammethods. Consider caching the agent instance.
| self.get_code_from_multiple_node_ids_tool = GetCodeFromMultipleNodeIdsTool( | ||
| self.db, self.user_id | ||
| ) | ||
| self.get_code_graph_from_node_id_tool = GetCodeGraphFromNodeIdTool(db) | ||
| self.file_structure_tool = GetCodeFileStructureTool(db) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to avoid tool initialization duplication.
The tools are initialized both as instance variables and in the _initialize_tools method. This creates duplicate instances and could lead to maintenance issues.
Consider this refactor to maintain a single source of truth:
def __init__(self, db: Session, user_id: str):
self.db = db
self.user_id = user_id
self.webpage_extractor_tool = webpage_extractor_tool(db, user_id)
self.github_tool = github_tool(db, user_id)
- self.get_code_from_multiple_node_ids_tool = GetCodeFromMultipleNodeIdsTool(
- self.db, self.user_id
- )
- self.get_code_graph_from_node_id_tool = GetCodeGraphFromNodeIdTool(db)
- self.file_structure_tool = GetCodeFileStructureTool(db)
self.tools = self._initialize_tools()
def _initialize_tools(self) -> Dict[str, Any]:
tools = {
"get_code_from_probable_node_name": get_code_from_probable_node_name_tool(
self.db, self.user_id
),
# ... other tools ...
+ "get_code_from_multiple_node_ids": GetCodeFromMultipleNodeIdsTool(
+ self.db, self.user_id
+ ),
+ "get_code_graph_from_node_id": GetCodeGraphFromNodeIdTool(self.db),
+ "get_code_file_structure": GetCodeFileStructureTool(self.db),
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (4)
31-32: Remove duplicate point about citations in the backstory.Points 4 and 5 in the backstory are redundant as they both mention including citations.
3. Refining and enriching the initial results to provide a detailed and contextually appropriate response. - 4. Maintaining traceability by including relevant citations and references in your output. - 5. Including relevant citations in the response. + 4. Maintaining traceability by including relevant citations and references in your response.
41-50: Document the purpose of each tool in the tools list.While the tools list is comprehensive, it would be helpful to add comments explaining the purpose and expected usage of each tool for better maintainability.
tools=[ + # Retrieves code content for given node IDs self.tools_provider.tools["get_code_from_multiple_node_ids"], + # Gets connected nodes for a given node ID self.tools_provider.tools["get_node_neighbours_from_node_id"], + # Searches and retrieves code based on probable node names self.tools_provider.tools["get_code_from_probable_node_name"], + # Executes semantic queries against the knowledge graph self.tools_provider.tools["ask_knowledge_graph_queries"], + # Retrieves nodes based on associated tags self.tools_provider.tools["get_nodes_from_tags"], + # Gets the project's file structure self.tools_provider.tools["get_code_file_structure"], + # Extracts content from web pages self.tools_provider.tools["webpage_extractor"], + # Interacts with GitHub API self.tools_provider.tools["github_tool"], ],
53-66: Add error handling and improve string formatting.The context enrichment method could be more robust with proper error handling and better string formatting.
async def _enriched_context(self, ctx: ChatContext) -> ChatContext: if ctx.node_ids and len(ctx.node_ids) > 0: - code_results = await self.tools_provider.get_code_from_multiple_node_ids_tool.run_multiple( - ctx.project_id, ctx.node_ids - ) - ctx.additional_context += ( - f"Code Graph context of the node_ids in query:\n {code_results}" - ) + try: + code_results = await self.tools_provider.get_code_from_multiple_node_ids_tool.run_multiple( + ctx.project_id, ctx.node_ids + ) + ctx.additional_context += ( + f"\nCode Graph context of the node_ids in query:\n{code_results}" + ) + except Exception as e: + # Log the error but continue execution + print(f"Error retrieving code results: {e}") - file_structure = self.tools_provider.file_structure_tool.run(ctx.project_id) - ctx.additional_context += f"File Structure of the project:\n {file_structure}" + try: + file_structure = self.tools_provider.file_structure_tool.run(ctx.project_id) + ctx.additional_context += f"\nFile Structure of the project:\n{file_structure}" + except Exception as e: + # Log the error but continue execution + print(f"Error retrieving file structure: {e}") return ctx
76-158: Fix formatting and step numbering in the task prompt.The task prompt has formatting inconsistencies and a missing step number.
- Step numbering is inconsistent (missing step 4)
- Inconsistent indentation in the prompt
- Inconsistent newline usage
1. Analyze project structure: - - Identify key directories, files, and modules - Guide search strategy and provide context - For directories of interest that show "└── ...", use "Get Code File Structure" tool with the directory path to reveal nested files - Only after getting complete file paths, use "Get Code and docstring From Probable Node Name" tool - Locate relevant files or subdirectory path - Directory traversal strategy: - - Start with high-level file structure analysis [...] 3. Knowledge graph query (if needed): - Transform query for knowledge graph tool - Execute query and analyze results - Additional context retrieval (if needed): + 4. Additional context retrieval (if needed): [...] - 5. Use "Get Nodes from Tags" tool as last resort only if absolutely necessary + 5. Use "Get Nodes from Tags" tool: + - Use as last resort only if absolutely necessary 6. Analyze and enrich results:app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (2)
27-47: Add type hints for better code maintainability.Consider adding type hints for the classifier to improve code maintainability and IDE support.
Apply this diff:
def __init__( self, llm_provider: ProviderService, prompt_provider: PromptService, rag_agent: ChatAgent, agent_type: AgentType, ): + self.llm_provider: ProviderService = llm_provider + self.prompt_provider: PromptService = prompt_provider + self.agent_type: AgentType = agent_type + self.rag_agent: ChatAgent = rag_agent + self.classifier: ChatAgent = LLM( - self.llm_provider = llm_provider - self.classifier = LLM( llm_provider, prompt_template=classification_prompt + " just return the single classification in response ", ) - self.prompt_provider = prompt_provider - self.agent_type = agent_type - self.rag_agent = rag_agent
61-66: Fix typo and extract fallback prompt template.The fallback prompt template contains a typo ("understading") and could be extracted to a constant for better maintainability.
Apply this diff:
+ _FALLBACK_PROMPT_TEMPLATE = ( + "You are a {agent_type} agent who has complete understanding of repo. " + "With the given history of chat: {history}\n" + "Answer the following query with given info: {query}" + ) + async def _create_llm(self) -> ChatAgent: # ... return LLM( self.llm_provider, - prompt_template=( - system_prompt.text - if system_prompt - else f"you are a {self.agent_type} agent " - + " who has complete understading of repo. With the given history of chat: {history} \nAnswer the following query with given info: {query}" - ), + prompt_template=system_prompt.text if system_prompt else self._FALLBACK_PROMPT_TEMPLATE.format( + agent_type=self.agent_type + ), )app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py (2)
10-18: Consider callingsuper().__init__in the constructor.
Although it may not be strictly necessary here, callingsuper().__init__()ensures proper initialization of the superclass.class BlastRadiusAgent(ChatAgent): def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): + super().__init__() self.tools_provider = tools_provider self.llm_provider = llm_provider
45-52: Potential agent reuse optimization.
Currently,_build_agent()runs every timerunorrun_streamis invoked. If your usage pattern calls this frequently and always with the same configuration, consider caching the createdCrewAIAgentinstance as an attribute for performance.app/modules/intelligence/agents_copy/chat_agents/crewai_agent.py (4)
2-2: Remove unused import.
The importosis unused. Removing it improves clarity and avoids unnecessary dependencies.- import os🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
5-5: Remove unused import.
The importagentopsis unused. Consider removing it if not needed for future development.- import agentops🧰 Tools
🪛 Ruff (0.8.2)
5-5:
agentopsimported but unusedRemove unused import:
agentops(F401)
81-81: Remove the unused variablenode_ids.
Since there's no further usage ofnode_ids, we can safely remove this line.- node_ids = [ctx.node_ids]🧰 Tools
🪛 Ruff (0.8.2)
81-81: Local variable
node_idsis assigned to but never usedRemove assignment to unused variable
node_ids(F841)
169-172: Streaming behavior limitation.
CrewAIcurrently doesn't support streaming outputs, so you’re simply returning the result ofrun. If streaming becomes a requirement, consider an alternative or custom solution that yields partial outputs over time.app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (1)
11-15: Consider callingsuper().__init__in the constructor.
Just as in other agents, callingsuper().__init__()can ensure consistent initialization ifChatAgentexpands in the future.class DebugAgent(ChatAgent): def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): + super().__init__() self.tools_provider = tools_provider self.rag_agent = CrewAIAgent(app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (3)
10-18: Add type hints to instance variables.Consider adding type hints to instance variables for better code maintainability and IDE support.
def __init__( self, llm_provider: ProviderService, tools_provider: ToolService, ): - self.llm_provider = llm_provider - self.tools_provider = tools_provider + self.llm_provider: ProviderService = llm_provider + self.tools_provider: ToolService = tools_provider
62-70: Improve string handling in context enrichment.Consider using a more structured approach for context enrichment instead of string concatenation.
- ctx.additional_context += ( - f"Code referred to in the query:\n {code_results}\n" - ) + ctx.additional_context += "\n".join([ + "Code referred to in the query:", + code_results, + "" # Empty line for separation + ])
81-227: Consider moving the task prompt to a separate file.The task prompt is quite long and could be better maintained in a dedicated file. This would improve code organization and make it easier to version control the prompt separately.
Consider:
- Moving the prompt to
app/modules/intelligence/agents_copy/chat_agents/system_agents/prompts/code_gen_prompt.py- Using a template engine for better prompt management
- Making the prompt configurable through environment variables or configuration files
app/modules/conversations/conversation/conversation_service_copy.py (5)
5-6: Remove unused imports.Static analysis indicates these imports are never referenced:
-from typing import Any, AsyncGenerator, List +from typing import AsyncGenerator, List -from langchain.schema import HumanMessage🧰 Tools
🪛 Ruff (0.8.2)
5-5:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
6-6:
langchain.schema.HumanMessageimported but unusedRemove unused import:
langchain.schema.HumanMessage(F401)
57-75: Consider adding docstrings for custom exceptions.Adding short docstrings can improve maintainability and readability, clarifying the purpose and usage of these exceptions for your team.
272-344: Add a docstring for store_message.A concise docstring documenting parameters (notably
stream) and the expected return type (an async generator) could help future maintainers.
363-363: Avoid using setattr for constant attribute assignment.Replace:
setattr(conversation, "human_message_count", human_message_count)with a direct assignment:
- setattr(conversation, "human_message_count", human_message_count) + conversation.human_message_count = human_message_count🧰 Tools
🪛 Ruff (0.8.2)
363-363: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
525-526: Preserve original exception detail or omit unused variable.You capture the exception as
ebut never use it. Either re-raise from the original exception to retain context:- except Exception as e: - raise ConversationServiceError("Failed to get chat history") + except Exception as e: + raise ConversationServiceError("Failed to get chat history") from eor remove
as eif you prefer not to propagate the original error message.🧰 Tools
🪛 Ruff (0.8.2)
525-525: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
526-526: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/modules/conversations/conversation/conversation_service_copy.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/crewai_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/llm_chat.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/modules/intelligence/agents_copy/chat_agents/llm_chat.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py
- app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/modules/intelligence/agents_copy/chat_agents/crewai_agent.py
2-2: os imported but unused
Remove unused import: os
(F401)
5-5: agentops imported but unused
Remove unused import: agentops
(F401)
81-81: Local variable node_ids is assigned to but never used
Remove assignment to unused variable node_ids
(F841)
app/modules/conversations/conversation/conversation_service_copy.py
5-5: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
6-6: langchain.schema.HumanMessage imported but unused
Remove unused import: langchain.schema.HumanMessage
(F401)
35-35: app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgent imported but unused
Remove unused import: app.modules.intelligence.agents.custom_agents.custom_agent.CustomAgent
(F401)
363-363: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
399-399: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
525-525: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
526-526: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py
54-54: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (14)
app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (2)
1-18: LGTM! Clean imports and well-structured class definition.The imports are specific and well-organized, and the class follows a clean inheritance pattern from ChatAgent with a focused constructor.
67-74: LGTM! Clean implementation of run methods.The run and run_stream methods are concise, well-structured, and properly reuse the agent building and context enrichment functionality.
app/modules/intelligence/agents_copy/chat_agents/adaptive_agent.py (5)
9-15: Fix duplicate import ofAgentType.
AgentTypeis imported from two separate modules, causing redefinition.Apply this diff to fix the imports:
-from app.modules.intelligence.provider.provider_service import ( - ProviderService, -) -from app.modules.intelligence.prompts.classification_prompts import ( - ClassificationPrompts, - AgentType, -) +from app.modules.intelligence.provider.provider_service import ProviderService +from app.modules.intelligence.prompts.classification_prompts import ( + ClassificationPrompts, + AgentType, +)
23-26: LGTM! Clear and informative class definition.The class definition and docstring effectively communicate the purpose and behavior of the
AdaptiveAgent.
54-54: Useisoperator for None comparison.Replace
==withiswhen comparing withNone.Apply this diff:
- if system_prompt == None: + if system_prompt is None:🧰 Tools
🪛 Ruff (0.8.2)
54-54: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
69-91: Extract duplicated classification logic and improve error handling.The classification logic is duplicated between
runandrun_streammethods, and the error handling could be improved.Extract the common logic into a private method and improve error handling:
+ async def _classify_query(self, ctx: ChatContext) -> str: + classification_response = await self.classifier.run(ctx) + logger.debug("Classification response: %s", classification_response.response) + + try: + classification_json = json.loads(classification_response.response) + if ( + classification_json + and classification_json["classification"] == "LLM_SUFFICIENT" + ): + return "LLM_SUFFICIENT" + except json.JSONDecodeError as e: + logger.warning("Failed to parse classification response: %s", e) + except KeyError as e: + logger.warning("Missing classification key in response: %s", e) + + return "AGENT_REQUIRED" + async def run(self, ctx: ChatContext) -> ChatAgentResponse: - classification_response = await self.classifier.run(ctx) - classification = "AGENT_REQUIRED" - logger.info(f"Classification response: {classification_response.response}") - try: - classification_json = json.loads(classification_response.response) - if ( - classification_json - and classification_json["classification"] == "LLM_SUFFICIENT" - ): - classification = "LLM_SUFFICIENT" - except Exception as e: - logger.error(f"Defaulting to agent because classification failed: {e}") + classification = await self._classify_query(ctx)
92-114: Clean up run_stream implementation.The method needs several improvements:
- Use the extracted classification logic.
- Replace print with logger.
- Remove commented-out code that's no longer needed.
Apply this diff:
async def run_stream( self, ctx: ChatContext ) -> AsyncGenerator[ChatAgentResponse, None]: - classification_response = await self.classifier.run(ctx) - classification = "AGENT_REQUIRED" - print("Classification response:", classification_response.response) - try: - classification_json = json.loads(classification_response.response) - if ( - classification_json - and classification_json["classification"] == "LLM_SUFFICIENT" - ): - classification = "LLM_SUFFICIENT" - except Exception: - pass + classification = await self._classify_query(ctx) if classification == "AGENT_REQUIRED": return await self.rag_agent.run_stream(ctx) - # You can pass the result to llm to stream response, but it's unnecessary and gives a overhead - # rag_agent_response = await self.rag_agent.run(ctx) - # ctx.query += f"\n with tool_response: {rag_agent_response.response} and citations: {rag_agent_response.citations}"app/modules/intelligence/agents_copy/chat_agents/system_agents/blast_radius_agent.py (1)
54-91: Prompt format is well-structured.
Your multi-step instructions for analyzing changes, querying knowledge graphs, and summarizing findings are clearly outlined. This comprehensive prompt should help produce thorough and consistent impact analyses.app/modules/intelligence/agents_copy/chat_agents/system_agents/debug_agent.py (1)
52-60: Potential for test coverage on enriched context logic.
Verifying that_enriched_contextappends code correctly toctx.additional_contextwould help ensure changes to the logic don’t break context handling.app/modules/intelligence/agents_copy/chat_agents/system_agents/code_gen_agent.py (2)
1-8: LGTM! Well-organized imports.The imports are clean, specific, and properly organized with clear separation between external and internal modules.
19-60: Cache the agent instance to avoid rebuilding.The agent is rebuilt for every request in both
runandrun_streammethods. Consider caching the agent instance.app/modules/conversations/conversation/conversation_service_copy.py (3)
78-125: Constructor and helper creation methods look solid.The initialization pattern is clear and well-structured. Dependencies are injected neatly, and error-handling in the helper methods is appropriate.
163-275: Robust conversation creation flow.The conversation creation logic, project lookup, and system message setup appear consistent. Error-handling and logging are suitably in place.
395-401: Fix mutable default argument.This was highlighted in previous reviews and still applies:
- def regenerate_last_message( - self, conversation_id: str, user_id: str, node_ids: List[NodeContext] = [], stream: bool = True - ) -> AsyncGenerator[ChatMessageResponse, None]: + def regenerate_last_message( + self, conversation_id: str, user_id: str, node_ids: Optional[List[NodeContext]] = None, stream: bool = True + ) -> AsyncGenerator[ChatMessageResponse, None]: + if node_ids is None: + node_ids = []🧰 Tools
🪛 Ruff (0.8.2)
399-399: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (1)
19-48: Promote code reuse by introducing a helper to build theCrewAIAgent.
Multiple system agents seem to adopt a similar_build_agentpattern. Extracting this logic into a shared helper function can make future updates simpler and reduce duplication.app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (2)
55-71: Evaluate potential duplication inadditional_context.
Appending code results and file structures each time the agent runs could produce repetitive or excessive context. Consider consolidating or caching results if repeated calls are expected.
82-164: Refine or modularize the extensive Q&A prompt.
Theqna_task_promptis quite comprehensive. Introducing discrete sub-prompts or sections might improve readability and maintainability, especially if you plan to modify or extend certain steps in the future.app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (2)
10-15: Consider documenting the class constructor parameters.Although the constructor is straightforward, adding docstrings for the
llm_providerandtools_providerparameters will improve clarity for future maintainers about their intended roles in this agent's workflow.
53-59: Consider implementing error handling for LLM or tool failures.The agent methods
runandrun_streamdirectly invokerag_agentwithout try-except blocks. If the underlying calls fail or return unexpected responses, it could lead to unhandled exceptions. Consider adding a minimal error handling strategy here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py(1 hunks)app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/modules/intelligence/agents_copy/chat_agents/system_agents/unit_test_agent.py
🔇 Additional comments (7)
app/modules/intelligence/agents_copy/chat_agents/system_agents/low_level_design_agent.py (2)
10-18: Consider callingsuper().__init__()in the constructor.
IfChatAgenthas any initialization logic, omitting thesuper().__init__()call might skip essential setup steps. Verify if the parent class needs this call for internal state initialization.
50-67: Confirm memory usage and performance when appending large code or file structure data.
Appending raw code and file structure data directly toctx.additional_contextmay quickly grow in size, especially with large repositories or numerous node IDs. Consider partial retrieval or summarization if performance becomes an issue.app/modules/intelligence/agents_copy/chat_agents/system_agents/qna_agent.py (1)
10-18: Consider callingsuper().__init__()in the constructor.
Like in other agents, ensure thatChatAgentdoesn’t require explicit initialization logic that might be missed here. If needed, addsuper().__init__()to properly initialize the parent class.app/modules/intelligence/agents_copy/chat_agents/system_agents/integration_test_agent.py (4)
18-31: Use dynamic formatting for the prompt if the{graph}placeholder is expected to be replaced.The
integration_test_task_promptincludes{graph}placeholders, but it’s not dynamically formatted here. If the goal is to incorporate the code graph data into the prompt directly, consider something like:self.rag_agent = CrewAIAgent( llm_provider, AgentConfig( ... - tasks=[TaskConfig(description=integration_test_task_prompt, ...)] + tasks=[TaskConfig(description=integration_test_task_prompt.format(graph="<some dynamic data>"), ...)] ), ... )
44-46: Validate usage of only the first node ID.Currently, only
ctx.node_ids[0]is used. If multiple node IDs are possible or relevant, consider looping through them or clarifying why only the first is necessary.
48-51: Ensurectx.additional_contextis a string.You’re appending a string to
ctx.additional_context. Verify thatctx.additional_contextis initialized as a string in all scenarios to avoid potential type errors.
62-125: Review references to the “{graph}” placeholder within the prompt.The placeholder
{graph}is mentioned, but there’s no code snippet that dynamically injects the actual code graph or context into the final prompt. Ensure that the final prompt text accurately represents real data when building integration tests.
Refactoring efforts for agents
Summary by CodeRabbit
New Features
AdaptiveAgent,AutoRouterAgent,BlastRadiusAgent,CodeGenAgent,DebugAgent,IntegrationTestAgent,LowLevelDesignAgent,QnAAgent,UnitTestAgent, andSupervisorAgentto enhance user interactions.Chores