Feat/sales assistant #134
Feat/sales assistant #134
Conversation
|
Warning Rate limit exceeded@omgate234 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as SalesAssistantAgent
participant Env as Environment (HubSpot Token)
participant VDB as VideoDBTool
participant LM as LanguageModel
participant CT as ComposioTool
Agent->>Env: Retrieve access token
alt Token missing
Agent->>Agent: Return error response
else Token present
Agent->>VDB: Extract transcript for video_id
alt Transcript missing
Agent->>VDB: Attempt indexing spoken words
Agent->>VDB: Retry transcript extraction
end
Agent->>LM: Generate sales summary prompt with transcript & user prompt
alt LM fails
Agent->>Agent: Log error and return error response
else LM succeeds
Agent->>CT: Create new deal in HubSpot with access token
alt Composio call fails
Agent->>Agent: Log error and return error response
else Composio call succeeds
Agent->>LM: Generate follow-up summary prompt
Agent->>Agent: Return success response
end
end
end
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (5)
backend/director/agents/sales_assistant.py (3)
1-20: Consider verifying environment variables early.
You retrieve environment variables (e.g.,HUBSPOT_ACCESS_TOKEN) further down in the code. For clarity and maintainability, you might consider centralizing environment variable validation at the very start of the file or during initialization to fail fast if crucial configuration is missing.
41-88: Extract long prompt text for maintainability.
The multilineSALES_ASSISTANT_PROMPTis quite large. Storing it in a separate template file or a dedicated helper function can enhance readability and maintainability.
113-235: Improve error handling and remove commented-out dummy code.
- The
runmethod effectively handles exceptions but consider separating transcript re-indexing from the main logic for better clarity.- Line 164 references a
DUMMY_TRANSCRIPTvariable that is currently commented out. Remove if no longer needed to keep the codebase clean.backend/director/tools/composio_tool.py (2)
13-39: Validateauth_datafields robustly.
You check for"name"and"token"withinauth_data. Consider additional guards or helpful error messages if either field is missing or empty, improving debugging during integration.
40-45: Distinguish distinct tool collections with meaningful logs.
Currently, you switch betweenappsandactionswithout logging which path is taken. Adding a small log line can aid troubleshooting if the wrong tools set is loaded.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/director/agents/sales_assistant.py(1 hunks)backend/director/handler.py(2 hunks)backend/director/tools/composio_tool.py(2 hunks)
🔇 Additional comments (4)
backend/director/agents/sales_assistant.py (2)
21-38: Validate optional vs. required fields inSALES_ASSISTANT_PARAMETER.
While "video_id" and "collection_id" are correctly marked as required, ensure that "prompt" is optional in both your code logic and user-facing documentation. Currently, there's no default behavior if the user omits "prompt."
90-111: Check for potential prompt injection or edge cases.
The_generate_promptmethod directly embeds user-suppliedtranscriptandpromptinto the final prompt. Consider sanitizing or limiting the maximum length of these fields to avoid potential injection or excessive tokens usage.backend/director/tools/composio_tool.py (1)
6-12: Well-definedToolsTypeenum.
The newToolsTypeenum cleanly distinguishes between apps and actions. This is a good approach to keep the usage explicit and type-safe.backend/director/handler.py (1)
28-70: Seamless integration of theSalesAssistantAgent.
Adding theSalesAssistantAgentimport and registration in theagentslist follows the existing pattern. This ensures it’s available for use in the chat flow with minimal friction.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
backend/director/agents/sales_assistant.py (3)
21-38: Consider adding format validation for IDs.The parameter schema could be more restrictive by adding format validation for
video_idandcollection_id."video_id": { "type": "string", + "pattern": "^[a-zA-Z0-9-_]+$", "description": "The ID of the sales call video", }, "collection_id": { "type": "string", + "pattern": "^[a-zA-Z0-9-_]+$", "description": "The ID of the collection which the video belongs to" },
91-98: Consider making environment variable name configurable.The Hubspot token environment variable name should be configurable to support different deployment environments.
+ HUBSPOT_TOKEN_ENV_VAR = "HUBSPOT_ACCESS_TOKEN" def __init__(self, session: Session, **kwargs): self.agent_name = "sales_assistant" self.description = "This agent will transcribe sales calls, automatically create deal summaries & update CRM software like Salesforce & Hubspot" self.parameters = SALES_ASSISTANT_PARAMETER self.llm = get_default_llm() + self.hubspot_token = os.getenv(self.HUBSPOT_TOKEN_ENV_VAR) super().__init__(session=session, **kwargs)
113-119: Add missing type hints.Type hints are missing for *args and **kwargs parameters.
def run(self, video_id:str, collection_id:str, prompt="", - *args, - **kwargs + *args: Any, + **kwargs: Any ) -> AgentResponse:Don't forget to add the import:
+from typing import Any
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/agents/sales_assistant.py(1 hunks)
🔇 Additional comments (1)
backend/director/agents/sales_assistant.py (1)
1-19: LGTM! Well-organized imports and proper logging setup.The imports are logically organized and include all necessary dependencies. The logger setup follows best practices.
| return AgentResponse( | ||
| status=AgentStatus.SUCCESS, | ||
| message=f"Agent {self.name} completed successfully.", | ||
| data={}, | ||
| ) |
There was a problem hiding this comment.
Fix incorrect variable usage in success message.
The success message uses self.name which doesn't exist, should be self.agent_name.
return AgentResponse(
status=AgentStatus.SUCCESS,
- message=f"Agent {self.name} completed successfully.",
+ message=f"Agent {self.agent_name} completed successfully.",
data={},
)📝 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 AgentResponse( | |
| status=AgentStatus.SUCCESS, | |
| message=f"Agent {self.name} completed successfully.", | |
| data={}, | |
| ) | |
| return AgentResponse( | |
| status=AgentStatus.SUCCESS, | |
| message=f"Agent {self.agent_name} completed successfully.", | |
| data={}, | |
| ) |
| composio_response = composio_tool( | ||
| task=composio_prompt, | ||
| auth_data={ | ||
| "name": "HUBSPOT", | ||
| "token": HUBSPOT_ACCESS_TOKEN | ||
| }, | ||
| tools_type=ToolsType.actions | ||
| ) |
There was a problem hiding this comment.
Add error handling for composio_tool response.
The composio_tool response should be validated to ensure it was successful.
composio_response = composio_tool(
task=composio_prompt,
auth_data={
"name": "HUBSPOT",
"token": HUBSPOT_ACCESS_TOKEN
},
tools_type=ToolsType.actions
)
+ if not composio_response.get("success"):
+ raise Exception(f"Composio failed: {composio_response.get('error', 'Unknown error')}")📝 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.
| composio_response = composio_tool( | |
| task=composio_prompt, | |
| auth_data={ | |
| "name": "HUBSPOT", | |
| "token": HUBSPOT_ACCESS_TOKEN | |
| }, | |
| tools_type=ToolsType.actions | |
| ) | |
| composio_response = composio_tool( | |
| task=composio_prompt, | |
| auth_data={ | |
| "name": "HUBSPOT", | |
| "token": HUBSPOT_ACCESS_TOKEN | |
| }, | |
| tools_type=ToolsType.actions | |
| ) | |
| if not composio_response.get("success"): | |
| raise Exception(f"Composio failed: {composio_response.get('error', 'Unknown error')}") |
| HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN") | ||
|
|
||
| if not HUBSPOT_ACCESS_TOKEN: | ||
| return AgentResponse( | ||
| status=AgentStatus.ERROR, | ||
| message="Hubspot token not present" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve security of token handling.
The Hubspot access token should be handled more securely:
- Validate token format
- Avoid logging the token value
- HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN")
+ HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN", "").strip()
+ if not HUBSPOT_ACCESS_TOKEN or not HUBSPOT_ACCESS_TOKEN.startswith("pat-"):
+ logger.error("Invalid or missing Hubspot token")
return AgentResponse(
status=AgentStatus.ERROR,
- message="Hubspot token not present"
+ message="Invalid or missing Hubspot token configuration"
)📝 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.
| HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN") | |
| if not HUBSPOT_ACCESS_TOKEN: | |
| return AgentResponse( | |
| status=AgentStatus.ERROR, | |
| message="Hubspot token not present" | |
| ) | |
| HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN", "").strip() | |
| if not HUBSPOT_ACCESS_TOKEN or not HUBSPOT_ACCESS_TOKEN.startswith("pat-"): | |
| logger.error("Invalid or missing Hubspot token") | |
| return AgentResponse( | |
| status=AgentStatus.ERROR, | |
| message="Invalid or missing Hubspot token configuration" | |
| ) |
| SALES_ASSISTANT_PROMPT = """ | ||
| Under "transcript", transcript of a sales call video is present. | ||
| Under "user prompt", the user has given additional context or information given | ||
| Generate a sales summary from it and generate answers from the transcript for the following properties | ||
|
|
||
| Following are the properties for which you need to find answers for and the Field type definition or the only possible answers to answer are given below | ||
|
|
||
| Each field has a predefined format or set of possible values and can also have **default** property which needs to be used if a field is missing in transcript and user prompt: | ||
|
|
||
| #### **Fields & Expected Answers** | ||
| Field: dealname | ||
| description: (The company or individuals name with whom we are making a deal with) | ||
| type: text (which says the name of person we are dealing with or the company) | ||
|
|
||
| Field: dealstage | ||
| Possible Answers: appointmentscheduled, qualifiedtobuy, presentationscheduled, decisionmakerboughtin, contractsent, closedwon, closedlost | ||
| default: appointmentscheduled | ||
|
|
||
| Field: budget | ||
| type: Multi line text (Around 150 words description) | ||
| description: | ||
| The multi line text answer for this field must consist of a detailed analysis of the budget situation of the company. | ||
| If numbers are mentioned, do include those details aswell. | ||
| If the deal is overpriced, underpriced or considered fair, should also be added if mentioned | ||
|
|
||
|
|
||
| Field: authority | ||
| type: Multi line text (Around 150 words description) | ||
| description: | ||
| The multi line text answer for this field must consist of a detailed analysis of the authority the client possesses for the conclusion of the deal. | ||
| If decision making powers are mentioned, do include those details. | ||
| If the client mention that they are the final signing authority, or any other details signifying their level of power in the deal. mention them | ||
|
|
||
|
|
||
| Field: need | ||
| type: Multi line text (Around 150 words description) | ||
| description: | ||
| The multi line text answer for this field must consist of a detailed analysis of how much the client wants the product. | ||
| Need can be found from the level or urgency, the depth or importance of problem they want to get solved or the amount of hurry they have | ||
|
|
||
|
|
||
| Field: timeline | ||
| type: Multi line text (Around 150 words description) | ||
| description: | ||
| The multi line text answer for this field must consist of a detailed analysis of how the timeline of the project looks like | ||
| Mention when they need the product, when they want to test the product etc. Important details about the timelines must be added here. | ||
|
|
||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix prompt template formatting and add dealstage validation.
The prompt template has two issues:
-
Inconsistent indentation could cause issues with string formatting
-
Missing validation for dealstage values
-
Fix the indentation:
-SALES_ASSISTANT_PROMPT = """
- Under "transcript", transcript of a sales call video is present.
+SALES_ASSISTANT_PROMPT = """Under "transcript", transcript of a sales call video is present.- Add validation for dealstage:
Field: dealstage
- Possible Answers: appointmentscheduled, qualifiedtobuy, presentationscheduled, decisionmakerboughtin, contractsent, closedwon, closedlost
+ Possible Answers: ONLY ONE of the following values is allowed:
+ - appointmentscheduled
+ - qualifiedtobuy
+ - presentationscheduled
+ - decisionmakerboughtin
+ - contractsent
+ - closedwon
+ - closedlost
default: appointmentscheduled📝 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.
| SALES_ASSISTANT_PROMPT = """ | |
| Under "transcript", transcript of a sales call video is present. | |
| Under "user prompt", the user has given additional context or information given | |
| Generate a sales summary from it and generate answers from the transcript for the following properties | |
| Following are the properties for which you need to find answers for and the Field type definition or the only possible answers to answer are given below | |
| Each field has a predefined format or set of possible values and can also have **default** property which needs to be used if a field is missing in transcript and user prompt: | |
| #### **Fields & Expected Answers** | |
| Field: dealname | |
| description: (The company or individuals name with whom we are making a deal with) | |
| type: text (which says the name of person we are dealing with or the company) | |
| Field: dealstage | |
| Possible Answers: appointmentscheduled, qualifiedtobuy, presentationscheduled, decisionmakerboughtin, contractsent, closedwon, closedlost | |
| default: appointmentscheduled | |
| Field: budget | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of the budget situation of the company. | |
| If numbers are mentioned, do include those details aswell. | |
| If the deal is overpriced, underpriced or considered fair, should also be added if mentioned | |
| Field: authority | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of the authority the client possesses for the conclusion of the deal. | |
| If decision making powers are mentioned, do include those details. | |
| If the client mention that they are the final signing authority, or any other details signifying their level of power in the deal. mention them | |
| Field: need | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of how much the client wants the product. | |
| Need can be found from the level or urgency, the depth or importance of problem they want to get solved or the amount of hurry they have | |
| Field: timeline | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of how the timeline of the project looks like | |
| Mention when they need the product, when they want to test the product etc. Important details about the timelines must be added here. | |
| """ | |
| SALES_ASSISTANT_PROMPT = """Under "transcript", transcript of a sales call video is present. | |
| Under "user prompt", the user has given additional context or information given | |
| Generate a sales summary from it and generate answers from the transcript for the following properties | |
| Following are the properties for which you need to find answers for and the Field type definition or the only possible answers to answer are given below | |
| Each field has a predefined format or set of possible values and can also have **default** property which needs to be used if a field is missing in transcript and user prompt: | |
| #### **Fields & Expected Answers** | |
| Field: dealname | |
| description: (The company or individuals name with whom we are making a deal with) | |
| type: text (which says the name of person we are dealing with or the company) | |
| Field: dealstage | |
| Possible Answers: ONLY ONE of the following values is allowed: | |
| - appointmentscheduled | |
| - qualifiedtobuy | |
| - presentationscheduled | |
| - decisionmakerboughtin | |
| - contractsent | |
| - closedwon | |
| - closedlost | |
| default: appointmentscheduled | |
| Field: budget | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of the budget situation of the company. | |
| If numbers are mentioned, do include those details aswell. | |
| If the deal is overpriced, underpriced or considered fair, should also be added if mentioned | |
| Field: authority | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of the authority the client possesses for the conclusion of the deal. | |
| If decision making powers are mentioned, do include those details. | |
| If the client mention that they are the final signing authority, or any other details signifying their level of power in the deal. mention them | |
| Field: need | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of how much the client wants the product. | |
| Need can be found from the level or urgency, the depth or importance of problem they want to get solved or the amount of hurry they have | |
| Field: timeline | |
| type: Multi line text (Around 150 words description) | |
| description: | |
| The multi line text answer for this field must consist of a detailed analysis of how the timeline of the project looks like | |
| Mention when they need the product, when they want to test the product etc. Important details about the timelines must be added here. | |
| """ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/director/agents/sales_assistant.py (2)
100-112: Improve prompt generation using f-strings.The string concatenation in
_generate_promptcould be improved for better readability and maintainability.def _generate_prompt(self, transcript:str, prompt:str): - final_prompt = SALES_ASSISTANT_PROMPT - - final_prompt += f""" - "transcript": - {transcript} - - "user prompt": - {prompt} - """ - - return final_prompt + return f"""{SALES_ASSISTANT_PROMPT} + "transcript": + {transcript} + + "user prompt": + {prompt} + """
204-212: Improve LLM prompt formatting and clarity.The LLM prompt string has several issues:
- Missing periods in sentences
- Unclear instructions
- Poor string formatting
- llm_prompt = ( - f"User has asked to run a task: {composio_prompt} in Composio. \n" - "Dont mention the action name directly as is" - "Comment on the fact whether the composio call was sucessful or not" - "Make this message short and crisp" - f"{json.dumps(composio_response)}" - "If there are any errors or if it was not successful, do tell about that as well" - "If the response is successful, Show the details given to create a new deal in a markdown table format" - ) + llm_prompt = f""" + User has asked to run a task in Composio: + {composio_prompt} + + Instructions: + 1. Do not mention the action name directly + 2. Provide a brief status of the Composio call (success/failure) + 3. Keep the response concise + 4. If successful, display deal details in a markdown table + 5. If unsuccessful, explain the error + + Response: + {json.dumps(composio_response)} + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/agents/sales_assistant.py(1 hunks)
🔇 Additional comments (4)
backend/director/agents/sales_assistant.py (4)
41-88: Fix prompt template formatting and add dealstage validation.The prompt template has formatting issues that could affect string interpolation:
- Fix the indentation:
-SALES_ASSISTANT_PROMPT = """ - Under "transcript", transcript of a sales call video is present. +SALES_ASSISTANT_PROMPT = """Under "transcript", transcript of a sales call video is present.
- Add validation for dealstage:
Field: dealstage - Possible Answers: appointmentscheduled, qualifiedtobuy, presentationscheduled, decisionmakerboughtin, contractsent, closedwon, closedlost + Possible Answers: ONLY ONE of the following values is allowed: + - appointmentscheduled + - qualifiedtobuy + - presentationscheduled + - decisionmakerboughtin + - contractsent + - closedwon + - closedlost default: appointmentscheduled
132-138: Improve security of token handling.The Hubspot access token handling needs improvement:
- HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN") + HUBSPOT_ACCESS_TOKEN = os.getenv("HUBSPOT_ACCESS_TOKEN", "").strip() + if not HUBSPOT_ACCESS_TOKEN or not HUBSPOT_ACCESS_TOKEN.startswith("pat-"): + logger.error("Invalid or missing Hubspot token") return AgentResponse( status=AgentStatus.ERROR, - message="Hubspot token not present" + message="Invalid or missing Hubspot token configuration" )
195-202: Add error handling for composio_tool response.The composio_tool response should be validated:
composio_response = composio_tool( task=composio_prompt, auth_data={ "name": "HUBSPOT", "token": HUBSPOT_ACCESS_TOKEN }, tools_type=ToolsType.actions ) + if not composio_response.get("success"): + raise Exception(f"Composio failed: {composio_response.get('error', 'Unknown error')}")
229-233: Fix incorrect variable usage in success message.The success message uses an undefined variable:
return AgentResponse( status=AgentStatus.SUCCESS, - message=f"Agent {self.name} completed successfully.", + message=f"Agent {self.agent_name} completed successfully.", data={final_message: llm_response.content}, )
| return AgentResponse( | ||
| status=AgentStatus.SUCCESS, | ||
| message=f"Agent {self.name} completed successfully.", | ||
| data={final_message: llm_response.content}, |
There was a problem hiding this comment.
Fix incorrect data structure in response.
The data field in the success response uses a ContextMessage object as a dictionary key, which could cause serialization issues.
- data={final_message: llm_response.content},
+ data={"message": llm_response.content},📝 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.
| data={final_message: llm_response.content}, | |
| data={"message": llm_response.content}, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/director/agents/sales_assistant.py (2)
21-38: Enhance parameter schema validation.Consider adding more validation constraints to improve robustness:
"prompt": { "type": "string", - "description": "Additional information/query given by the user to make the appropriate action to take place" + "description": "Additional context or specific requirements for deal creation in Hubspot CRM", + "default": "", + "examples": ["Focus on pricing discussion", "Extract timeline commitments"] }
140-162: Add retry mechanism for transcript extraction.The current implementation attempts transcript extraction only once after indexing. Consider adding a retry mechanism with backoff.
+ from tenacity import retry, stop_after_attempt, wait_exponential + + @retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=4, max=10)) + def get_transcript_with_retry(tool, vid_id): + try: + return tool.get_transcript(vid_id) + except Exception as e: + logger.warning(f"Attempt to get transcript failed: {e}") + tool.index_spoken_words(vid_id) + return tool.get_transcript(vid_id) + try: - transcript_text = videodb_tool.get_transcript(video_id) - except Exception: - logger.error("Transcript not found. Indexing spoken words..") - self.output_message.actions.append("Indexing spoken words..") - self.output_message.push_update() - videodb_tool.index_spoken_words(video_id) - transcript_text = videodb_tool.get_transcript(video_id) + transcript_text = get_transcript_with_retry(videodb_tool, video_id) + except Exception as e: + logger.error(f"Failed to get transcript after retries: {e}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/agents/sales_assistant.py(1 hunks)
🔇 Additional comments (5)
backend/director/agents/sales_assistant.py (5)
1-20: LGTM! Well-organized imports.The imports are logically grouped and include all necessary dependencies for the agent's functionality.
41-88: Fix prompt template formatting and add dealstage validation.This issue was previously identified. Please refer to the earlier review comment about fixing indentation and adding dealstage validation.
91-98: LGTM! Clean class initialization.The initialization is well-structured with clear agent identification and parameter setup.
182-202: Add validation for Composio response.This issue was previously identified. Please refer to the earlier review comment about adding error handling for the composio_tool response.
229-233: Fix incorrect variable and data structure usage.These issues were previously identified. Please refer to the earlier review comments about:
- Using
self.nameinstead ofself.agent_name- Using ContextMessage object as dictionary key
|
|
||
| def _generate_prompt(self, transcript:str, prompt:str): | ||
| final_prompt = SALES_ASSISTANT_PROMPT | ||
|
|
||
| final_prompt += f""" | ||
| "transcript": | ||
| {transcript} | ||
|
|
||
| "user prompt": | ||
| {prompt} | ||
| """ | ||
|
|
||
| return final_prompt | ||
|
|
There was a problem hiding this comment.
Improve prompt generation security.
The prompt generation method could be vulnerable to injection attacks.
def _generate_prompt(self, transcript:str, prompt:str):
final_prompt = SALES_ASSISTANT_PROMPT
+ # Sanitize inputs to prevent prompt injection
+ transcript = json.dumps(transcript)
+ prompt = json.dumps(prompt)
final_prompt += f"""
"transcript":
{transcript}
"user prompt":
{prompt}
"""
return final_prompt📝 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.
| def _generate_prompt(self, transcript:str, prompt:str): | |
| final_prompt = SALES_ASSISTANT_PROMPT | |
| final_prompt += f""" | |
| "transcript": | |
| {transcript} | |
| "user prompt": | |
| {prompt} | |
| """ | |
| return final_prompt | |
| def _generate_prompt(self, transcript:str, prompt:str): | |
| final_prompt = SALES_ASSISTANT_PROMPT | |
| # Sanitize inputs to prevent prompt injection | |
| transcript = json.dumps(transcript) | |
| prompt = json.dumps(prompt) | |
| final_prompt += f""" | |
| "transcript": | |
| {transcript} | |
| "user prompt": | |
| {prompt} | |
| """ | |
| return final_prompt |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/director/agents/sales_assistant.py (1)
21-38: Add pattern validation for ID fields.Consider adding pattern validation to ensure video_id and collection_id follow the expected format.
"video_id": { "type": "string", + "pattern": "^[a-zA-Z0-9-_]+$", "description": "The ID of the sales call video", }, "collection_id": { "type": "string", + "pattern": "^[a-zA-Z0-9-_]+$", "description": "The ID of the collection which the video belongs to" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/agents/sales_assistant.py(1 hunks)
🔇 Additional comments (6)
backend/director/agents/sales_assistant.py (6)
1-20: LGTM! Well-organized imports.The imports are properly organized and all imported modules are utilized in the implementation.
41-93: Fix prompt template formatting.The prompt template has inconsistent indentation that could cause formatting issues.
137-143: Improve security of token handling.The Hubspot access token needs more secure handling.
105-116: Improve prompt generation security.The prompt generation method needs protection against injection attacks.
200-207: Add error handling for composio_tool response.The composio_tool response should be validated.
234-238: Fix incorrect variable usage in success message.The success message uses
self.namewhich doesn't exist.
| try: | ||
| transcript_text = videodb_tool.get_transcript(video_id) | ||
| except Exception: | ||
| logger.error("Transcript not found. Indexing spoken words..") | ||
| self.output_message.actions.append("Indexing spoken words..") | ||
| self.output_message.push_update() | ||
| videodb_tool.index_spoken_words(video_id) | ||
| transcript_text = videodb_tool.get_transcript(video_id) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for external API calls.
The API calls to VideoDB and LLM services should include timeout handling to prevent hanging operations.
try:
- transcript_text = videodb_tool.get_transcript(video_id)
+ transcript_text = videodb_tool.get_transcript(video_id, timeout=30)
except Exception:
logger.error("Transcript not found. Indexing spoken words..")
self.output_message.actions.append("Indexing spoken words..")
self.output_message.push_update()
- videodb_tool.index_spoken_words(video_id)
- transcript_text = videodb_tool.get_transcript(video_id)
+ videodb_tool.index_spoken_words(video_id, timeout=60)
+ transcript_text = videodb_tool.get_transcript(video_id, timeout=30)
# ... later in the code ...
- llm_response = self.llm.chat_completions([sales_assist_llm_message.to_llm_msg()])
+ llm_response = self.llm.chat_completions([sales_assist_llm_message.to_llm_msg()], timeout=30)Also applies to: 175-185
| sales_assist_llm_prompt = self._generate_prompt(transcript=transcript_text, prompt=prompt) | ||
| sales_assist_llm_message = ContextMessage( | ||
| content=sales_assist_llm_prompt, role=RoleTypes.user | ||
| ) | ||
| llm_response = self.llm.chat_completions([sales_assist_llm_message.to_llm_msg()]) | ||
|
|
||
| if not llm_response.status: | ||
| logger.error(f"LLM failed with {llm_response}") | ||
| text_content.status = MsgStatus.error | ||
| text_content.status_message = "Failed to generate the response." | ||
| self.output_message.publish() | ||
| return AgentResponse( | ||
| status=AgentStatus.ERROR, | ||
| message="Sales assistant failed due to LLM error.", | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement rate limiting for LLM calls.
Add rate limiting to prevent excessive LLM API usage and potential costs.
+ def __init__(self, session: Session, **kwargs):
+ super().__init__(session=session, **kwargs)
+ self.agent_name = "sales_assistant"
+ self.description = "This agent will transcribe sales calls, automatically create deal summaries & update CRM software like Salesforce & Hubspot"
+ self.parameters = SALES_ASSISTANT_PARAMETER
+ self.llm = get_default_llm()
+ self._rate_limiter = RateLimiter(max_calls=10, time_window=60) # 10 calls per minute
def run(self, video_id:str, collection_id:str, prompt="", *args, **kwargs) -> AgentResponse:
# ... existing code ...
- llm_response = self.llm.chat_completions([sales_assist_llm_message.to_llm_msg()])
+ with self._rate_limiter:
+ llm_response = self.llm.chat_completions([sales_assist_llm_message.to_llm_msg()])
# ... later in the code ...
- llm_response = self.llm.chat_completions([final_message.to_llm_msg()])
+ with self._rate_limiter:
+ llm_response = self.llm.chat_completions([final_message.to_llm_msg()])Also applies to: 219-222
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/director/agents/sales_assistant.py (2)
21-38: Enhance parameter schema validation.Consider adding these improvements to the parameter schema:
- Add pattern validation for
video_idandcollection_id- Add
minLengthandmaxLengthconstraints for thepromptfield- Add
promptto required fields if it's mandatorySALES_ASSISTANT_PARAMETER = { "type": "object", "properties": { "video_id": { "type": "string", "description": "The ID of the sales call video", + "pattern": "^[a-zA-Z0-9-_]+$", + "minLength": 1 }, "collection_id": { "type": "string", "description": "The ID of the collection which the video belongs to" + "pattern": "^[a-zA-Z0-9-_]+$", + "minLength": 1 }, "prompt": { "type": "string", "description": "Additional information/query given by the user to make the appropriate action to take place" + "maxLength": 1000 } }, - "required": ["video_id", "collection_id"] + "required": ["video_id", "collection_id", "prompt"] }
209-217: Improve LLM prompt construction.The LLM prompt construction has several issues:
- Missing period after "directly as is"
- Inconsistent capitalization in "sucessful"
- Raw JSON dump could expose sensitive data
llm_prompt = ( f"User has asked to run a task: {composio_prompt} in Composio. \n" - "Dont mention the action name directly as is" + "Don't mention the action name directly as is." "Comment on the fact whether the composio call was sucessful or not" "Make this message short and crisp" - f"{json.dumps(composio_response)}" + f"{json.dumps(composio_response, default=str, indent=2)}" "If there are any errors or if it was not successful, do tell about that as well" "If the response is successful, Show the details given to create a new deal in a markdown table format" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/director/agents/sales_assistant.py(1 hunks)
🔇 Additional comments (7)
backend/director/agents/sales_assistant.py (7)
41-93: Fix prompt template formatting and add dealstage validation.The prompt template has two issues:
- Inconsistent indentation could cause issues with string formatting
- Missing validation for dealstage values
105-116: Improve prompt generation security.The prompt generation method could be vulnerable to injection attacks.
159-167: Add timeout handling for external API calls.The API calls to VideoDB should include timeout handling to prevent hanging operations.
171-185: Implement rate limiting for LLM calls.Add rate limiting to prevent excessive LLM API usage and potential costs.
137-143: Improve security of token handling.The Hubspot access token should be handled more securely.
200-207: Add error handling for composio_tool response.The composio_tool response should be validated to ensure it was successful.
234-238: Fix incorrect variable usage in success message.The success message uses
self.namewhich doesn't exist, should beself.agent_name.
Action Flow:
Summary by CodeRabbit