[refactor]: migrate to cog-based command architecture #76
Conversation
WalkthroughThis update introduces conversation summarization and memory management to the DevRelAgent workflow. The agent now checkpoints state, conditionally summarizes conversations, and manages session memory with new nodes and state fields. Several node functions were refactored to return dictionaries instead of mutating state. Discord bot commands for thread reset/help were moved to a new cog, and new summarization prompts and logic were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscordBot
participant AgentCoordinator
participant DevRelAgent
participant InMemorySaver
participant LLM
User->>DiscordBot: Sends message
DiscordBot->>AgentCoordinator: Forwards message (with memory_thread_id)
AgentCoordinator->>DevRelAgent: run(initial_state, thread_id)
DevRelAgent->>InMemorySaver: Retrieve/checkpoint state
DevRelAgent->>LLM: Generate response (uses summary/context)
DevRelAgent->>DevRelAgent: Decide if summarization needed
alt Summarization needed
DevRelAgent->>LLM: Summarize conversation
DevRelAgent->>InMemorySaver: Update state with summary/topics
end
DevRelAgent->>InMemorySaver: Checkpoint updated state
DevRelAgent->>AgentCoordinator: Return response, state
AgentCoordinator->>DiscordBot: Send response to user
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @smokeyScraper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces significant architectural changes to the agent system, primarily focusing on implementing state persistence and conversation memory using LangGraph's checkpointer feature. It adds a summarization mechanism to maintain context over extended interactions and refactors the Discord bot's command handling to use cogs, improving code structure and enabling stateful conversations.
Highlights
- State Persistence: Integrated LangGraph's
InMemorySaverto persist agent state across messages within a thread/session, enabling multi-turn conversations with memory. - Conversation Summarization: Added new nodes and logic to the DevRel agent workflow to periodically summarize conversations based on interaction count or timeout, storing the summary and key topics in the agent state for long-term context.
- Structured State Updates: Refactored existing agent nodes (
gather_context,handle_faq,handle_onboarding,handle_technical_support,handle_web_search,generate_response) to return state updates as dictionaries, aligning with LangGraph's recommended pattern for state modification and persistence. - Discord Cog Migration: Migrated Discord commands (
!reset,!help_devrel) from the main bot class to a dedicated cog (DevRelCommands) for better modularity and organization. - Memory Management: Implemented functionality to clear thread memory, triggered manually via the
!resetcommand or automatically upon session timeout, including a placeholder for storing the final summary to a database.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes migrate the command architecture to a cog-based system, incorporating memory persistence and summarization capabilities. The DevRel agent is enhanced with conversation summarization and memory management, improving its ability to handle long-term interactions. The Discord bot is updated to use cogs for command handling and includes a reset command to clear user threads and memory.
|
@chandansgowda, this seems good to go. could you please review and merge? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (4)
backend/app/agents/devrel/nodes/handle_web_search_node.py (1)
21-33: 🛠️ Refactor suggestionAdd defensive error handling around the external search call
search_tool.search()is an external I/O-bound operation and may raise network or quota related exceptions. A single unhandled failure will bubble up and break the LangGraph run.- search_results = await search_tool.search(search_query) + try: + search_results = await search_tool.search(search_query) + except Exception as exc: + logger.error(f"Web-search failure for query '{search_query}': {exc}") + return { + "errors": [str(exc)], + "current_task": "web_search_failed" + }backend/app/agents/devrel/nodes/handle_onboarding_node.py (1)
6-17:⚠️ Potential issueReturn-type annotation is now wrong
The function advertises
-> AgentStatebut returns a plaindict.
This will:
- Mislead static type-checkers / IDEs.
- Break any downstream code expecting an
AgentStateinstance.-async def handle_onboarding_node(state: AgentState) -> AgentState: +async def handle_onboarding_node(state: AgentState) -> dict:backend/app/agents/devrel/nodes/handle_technical_support_node.py (1)
6-17:⚠️ Potential issueReturn-type annotation mismatch (same as onboarding node)
-async def handle_technical_support_node(state: AgentState) -> AgentState: +async def handle_technical_support_node(state: AgentState) -> dict:Without this fix, mypy/pyright will complain and callers casting to
AgentStatewill crash.backend/app/agents/devrel/nodes/gather_context_node.py (1)
8-36:⚠️ Potential issueUpdate signature & docstring to reflect immutable-state pattern
The node now returns a delta
dict, not a mutatedAgentState.-async def gather_context_node(state: AgentState) -> AgentState: +async def gather_context_node(state: AgentState) -> dict:Please also adjust the docstring and any unit tests accordingly.
🧹 Nitpick comments (15)
backend/app/agents/shared/state.py (1)
12-17:addon lists risks silent duplication
operator.addconcatenates the full list, potentially duplicating messages during repeated merges.
If LangGraph supports a custom dedupe combiner, consider using it, or slice the result to a max length to avoid unbounded memory growth.backend/main.py (1)
10-11: Import order nitProject imports (
bots.*) normally come after stdlib & 3rd-party groups. Not blocking, just mentioning for consistency.backend/app/agents/devrel/nodes/handle_faq_node.py (1)
6-6: Keep type hints aligned with new return shapeYou still import
AgentState, but only for the first arg. Consider declaring the return type withTypedDictordict[str, Any]for clarity:from typing import Any, Dict async def handle_faq_node(state: AgentState, faq_tool) -> Dict[str, Any]:backend/app/agents/devrel/nodes/handle_web_search_node.py (1)
34-42: Avoid hard-coding the tool identifier
"tavily_search"is embedded twice, tying the node to one concrete implementation. Either read the name fromsearch_tool(if exposed) or inject it via the caller to keep the node reusable.backend/app/agents/devrel/nodes/gather_context_node.py (1)
16-22:original_messagemay be empty – consider graceful fallbackIf
original_messageis missing, the addednew_messagewill have an emptycontentfield, polluting the message history.original = state.context.get("original_message") if original: new_message = { ... } messages = [new_message] else: messages = []backend/bots/discord/discord_cogs.py (2)
4-4: Prefer project-root-relative import to avoid module resolution issuesDepending on how
backend/is added toPYTHONPATH,bots.discord.discord_botmay fail to resolve when the app is executed from outside the project root. Using the full package path (backend.bots.discord.discord_bot) or an absolute import anchored at the top-level package prevents such surprises.-from bots.discord.discord_bot import DiscordBot +from backend.bots.discord.discord_bot import DiscordBot
25-40: Minor UX enhancement – mark help embed as ephemeralUsers in busy channels may appreciate the help message being ephemeral or DM-ed to them instead of cluttering the thread. Consider
await ctx.send(embed=embed, ephemeral=True)if you are on interactions or switch to slash-command +default_permission=Falsefor cleaner UX.backend/app/agents/devrel/nodes/generate_response_node.py (2)
84-99: Return typing hints & key ordering for downstream clarityDown-stream nodes/consumers expect
final_responsefirst; add aTypedDict/dataclass for stronger guarantees and deterministic ordering.
17-25: Guard against missing keys in web-search results
result.get('title')etc. may returnNone, leading to"None"in user-facing text. Fallback to empty string improves readability.backend/app/core/orchestration/agent_coordinator.py (2)
4-4: Remove unuseddatetimeimportStatic analysis flags it and it adds dead weight.
-from datetime import datetime🧰 Tools
🪛 Ruff (0.11.9)
4-4:
datetime.datetimeimported but unusedRemove unused import:
datetime.datetime(F401)
55-62: Missing awaitable error propagationIf
self.devrel_agent.runthrows, the surroundingexceptblock swallows the traceback and only sends a generic error message. Logexc_info=Trueto aid debugging.backend/app/agents/shared/base_agent.py (1)
56-76: Consider back-pressure mechanism in streaming
astreammay yield large bursts; optionally throttle or buffer to prevent flooding upstream consumers.backend/app/agents/devrel/nodes/summarization_node.py (3)
28-32: Clarify additive update patternReturning
"interaction_count": 1relies on the LangGraphaddaggregator to increment the stored count. Add a brief comment to avoid future confusion for maintainers unfamiliar with this pattern.
33-38: Hard-coded timeout/threshold
THREAD_TIMEOUT_HOURSandSUMMARIZATION_THRESHOLDare constants here. Moving them tosettings(or env vars) enables tuning without code changes.
104-109: Error path swallows original exceptionThe except block logs but only returns a generic error string. Re-raise or include
traceback.format_exc()to aid debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
backend/app/agents/devrel/agent.py(6 hunks)backend/app/agents/devrel/nodes/gather_context_node.py(2 hunks)backend/app/agents/devrel/nodes/generate_response_node.py(1 hunks)backend/app/agents/devrel/nodes/handle_faq_node.py(2 hunks)backend/app/agents/devrel/nodes/handle_onboarding_node.py(1 hunks)backend/app/agents/devrel/nodes/handle_technical_support_node.py(1 hunks)backend/app/agents/devrel/nodes/handle_web_search_node.py(2 hunks)backend/app/agents/devrel/nodes/summarization_node.py(1 hunks)backend/app/agents/devrel/prompts/base_prompt.py(1 hunks)backend/app/agents/devrel/prompts/summarization_prompt.py(1 hunks)backend/app/agents/shared/base_agent.py(2 hunks)backend/app/agents/shared/state.py(2 hunks)backend/app/core/orchestration/agent_coordinator.py(5 hunks)backend/bots/discord/discord_bot.py(2 hunks)backend/bots/discord/discord_cogs.py(1 hunks)backend/main.py(2 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
backend/main.py (1)
backend/bots/discord/discord_cogs.py (1)
DevRelCommands(6-40)
backend/app/agents/devrel/nodes/handle_faq_node.py (1)
backend/app/agents/shared/state.py (1)
AgentState(18-73)
backend/app/agents/devrel/nodes/generate_response_node.py (1)
backend/app/agents/shared/state.py (1)
AgentState(18-73)
backend/app/core/orchestration/agent_coordinator.py (5)
backend/app/agents/devrel/agent.py (2)
get_thread_state(128-136)clear_thread_memory(138-170)backend/app/agents/shared/state.py (1)
AgentState(18-73)backend/app/core/orchestration/queue_manager.py (2)
AsyncQueueManager(14-135)register_handler(67-70)backend/app/agents/devrel/nodes/summarization_node.py (1)
store_summary_to_database(132-136)backend/app/agents/shared/base_agent.py (1)
run(22-54)
backend/app/agents/devrel/agent.py (2)
backend/app/agents/devrel/nodes/summarization_node.py (3)
check_summarization_needed(14-46)summarize_conversation_node(48-109)store_summary_to_database(132-136)backend/app/agents/shared/state.py (1)
AgentState(18-73)
backend/app/agents/devrel/nodes/handle_web_search_node.py (1)
backend/app/agents/shared/state.py (1)
AgentState(18-73)
backend/app/agents/shared/base_agent.py (1)
backend/app/agents/shared/state.py (1)
AgentState(18-73)
🪛 Ruff (0.11.9)
backend/app/core/orchestration/agent_coordinator.py
4-4: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
🪛 Pylint (3.3.7)
backend/app/agents/devrel/agent.py
[refactor] 148-163: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (9)
pyproject.toml (1)
25-25: Confirmweaviate-clientupper-bound is intentionalPinning
<5.0.0prevents automatic upgrades to the next major. Check the project’s upgrade policy—weaviate-clientv5 may ship breaking changes you actually want to adopt later.
If the cap is just precautionary, consider adding a comment in the TOML or CI alert to revisit the bound periodically.Would you like a quick script that checks PyPI for the latest major and warns if your cap is lagging?
backend/app/agents/shared/state.py (1)
41-42:interaction_countadditive merge can overflowThe additive combiner is great, but consider a ceiling or cast to
intafter merge to guard against malicious or erroneous large values.backend/main.py (1)
33-33: Cog registration happens before login—goodRegistering the cog early keeps command discovery synchronous and avoids race conditions. No issues spotted.
backend/app/agents/devrel/prompts/summarization_prompt.py (1)
1-18: Prompt template looks solidClear instructions, token-friendly length cap, and placeholders follow the existing style. 👍
backend/bots/discord/discord_bot.py (2)
46-50: 👍 Cleaner command dispatchUsing
get_context+invokerespects command cooldowns, checks and cogs automatically. Nice upgrade over the manual prefix check.
87-88:memory_thread_idkey looks sensible – verify consumersThe new field ties messages to long-term memory. Ensure every downstream component that deserialises queue items tolerates this extra key; otherwise add a default
**kwargssink.backend/app/agents/devrel/agent.py (3)
73-87: Edge ordering looks correct – good use of conditional routing
128-137: ConfirmCompiledGraph.get_statebehaviour
self.graph.get_state(config)is called inside anasyncmethod but the call itself is synchronous. Verify that:
get_stateis non-blocking (no I/O);- Returned object exposes
valuesas expected.If either assumption is false, wrap with
awaitor move to a thread-pool.
34-34: 🛠️ Refactor suggestionSingleton vs. per-instance memory store
InMemorySaver()is instantiated perDevRelAgent. If multiple agent instances are created (e.g., per-request workers), conversation state will fragment across processes, defeating “persistent” memory. Consider injecting a shared store (RedisSaver, DynamoDBSaver, etc.) or passing a singleton instance via DI.⛔ Skipped due to learnings
Learnt from: smokeyScraper PR: AOSSIE-Org/Devr.AI#75 File: backend/app/agents/devrel/agent.py:34-35 Timestamp: 2025-06-13T21:56:19.160Z Learning: In the Devr.AI backend, the DevRelAgent follows a singleton pattern where only one instance exists for the entire application lifetime, using InMemorySaver with thread-based conversation management to persist user conversations across sessions.
| session_start_time: datetime = Field(default_factory=datetime.now) | ||
| last_interaction_time: datetime = Field(default_factory=datetime.now) | ||
| interaction_count: Annotated[int, add] = Field(default=0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use timezone-aware timestamps
datetime.now() returns naive datetimes. Mixing naive and aware values later (e.g., when comparing against utcnow()) will raise errors.
-from datetime import datetime
+from datetime import datetime, timezone
...
-session_start_time: datetime = Field(default_factory=datetime.now)
-last_interaction_time: datetime = Field(default_factory=datetime.now)
+session_start_time: datetime = Field(default_factory=lambda: datetime.now(tz=timezone.utc))
+last_interaction_time: datetime = Field(default_factory=lambda: datetime.now(tz=timezone.utc))📝 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.
| session_start_time: datetime = Field(default_factory=datetime.now) | |
| last_interaction_time: datetime = Field(default_factory=datetime.now) | |
| interaction_count: Annotated[int, add] = Field(default=0) | |
| --- a/backend/app/agents/shared/state.py | |
| +++ b/backend/app/agents/shared/state.py | |
| @@ -1,5 +1,5 @@ | |
| -from datetime import datetime | |
| +from datetime import datetime, timezone | |
| from pydantic import Field | |
| from typing import Annotated | |
| from .helpers import add | |
| @@ -37,8 +37,9 @@ class AgentState(BaseModel): | |
| """ | |
| - session_start_time: datetime = Field(default_factory=datetime.now) | |
| - last_interaction_time: datetime = Field(default_factory=datetime.now) | |
| + session_start_time: datetime = Field( | |
| + default_factory=lambda: datetime.now(tz=timezone.utc) | |
| + ) | |
| + last_interaction_time: datetime = Field( | |
| + default_factory=lambda: datetime.now(tz=timezone.utc) | |
| + ) | |
| interaction_count: Annotated[int, add] = Field(default=0) |
🤖 Prompt for AI Agents
In backend/app/agents/shared/state.py around lines 39 to 41, the use of
datetime.now() creates naive datetime objects which can cause errors when mixed
with timezone-aware datetimes. Replace datetime.now() with a timezone-aware
alternative such as datetime.now(timezone.utc) or use datetime.utcnow() with
proper timezone info to ensure all timestamps are timezone-aware and consistent.
| def replace_summary(existing: Optional[str], new: Optional[str]) -> Optional[str]: | ||
| """Replace summary""" | ||
| if new is not None: | ||
| return new | ||
| return existing | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow None inputs in helpers
replace_summary already accepts Optional[str], but replace_topics below doesn’t allow None. When state merges from scratch the incoming value may be None, causing a TypeError.
-def replace_topics(existing: List[str], new: List[str]) -> List[str]:
+from typing import Optional
+
+def replace_topics(existing: Optional[List[str]], new: Optional[List[str]]) -> List[str]:
"""Replace topics"""
- if new:
+ if new:
return new
- return existing
+ return existing or []📝 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 replace_summary(existing: Optional[str], new: Optional[str]) -> Optional[str]: | |
| """Replace summary""" | |
| if new is not None: | |
| return new | |
| return existing | |
| from typing import Optional | |
| def replace_topics(existing: Optional[List[str]], new: Optional[List[str]]) -> List[str]: | |
| """Replace topics""" | |
| if new: | |
| return new | |
| return existing or [] |
🤖 Prompt for AI Agents
In backend/app/agents/shared/state.py around lines 6 to 11, the function
replace_topics should be updated to accept Optional[str] inputs like
replace_summary does. Modify replace_topics to handle None values gracefully by
allowing its parameters to be Optional[str] and adjusting the logic to avoid
TypeErrors when merging state with None inputs.
| return { | ||
| "task_result": { | ||
| "type": "faq", | ||
| "response": faq_response, | ||
| "source": "faq_database" | ||
| }, | ||
| "current_task": "faq_handled" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling around external FAQ call
An exception inside faq_tool.get_response will bubble up and break the graph. Wrap it to surface a controlled failure:
- faq_response = await faq_tool.get_response(latest_message)
+ try:
+ faq_response = await faq_tool.get_response(latest_message)
+ except Exception as e:
+ logger.exception("FAQ tool failed")
+ return {
+ "errors": [str(e)],
+ "current_task": "faq_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.
| return { | |
| "task_result": { | |
| "type": "faq", | |
| "response": faq_response, | |
| "source": "faq_database" | |
| }, | |
| "current_task": "faq_handled" | |
| } | |
| try: | |
| faq_response = await faq_tool.get_response(latest_message) | |
| except Exception as e: | |
| logger.exception("FAQ tool failed") | |
| return { | |
| "errors": [str(e)], | |
| "current_task": "faq_failed" | |
| } | |
| return { | |
| "task_result": { | |
| "type": "faq", | |
| "response": faq_response, | |
| "source": "faq_database" | |
| }, | |
| "current_task": "faq_handled" | |
| } |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/nodes/handle_faq_node.py around lines 19 to 26, the
call to faq_tool.get_response lacks error handling, so any exception will
propagate and break the graph. Wrap the call to faq_tool.get_response in a
try-except block, catch exceptions, and return a controlled failure response
indicating the error instead of letting the exception bubble up.
| @commands.command(name="reset") | ||
| async def reset_thread(self, ctx: commands.Context): | ||
| """Reset your DevRel thread and memory.""" | ||
| user_id = str(ctx.author.id) | ||
| cleanup = { | ||
| "type": "clear_thread_memory", | ||
| "memory_thread_id": user_id, | ||
| "user_id": user_id, | ||
| "cleanup_reason": "manual_reset" | ||
| } | ||
| await self.queue.enqueue(cleanup, QueuePriority.HIGH) | ||
| self.bot.active_threads.pop(user_id, None) | ||
| await ctx.send("Your DevRel thread & memory have been reset! Send another message to start fresh.") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle enqueue/cleanup failures to prevent user confusion
If enqueue raises (e.g. queue manager down) the thread won’t reset but the user still receives the success message. Wrap the enqueue/active_threads.pop pair in a try/except and inform the user on failure.
🤖 Prompt for AI Agents
In backend/bots/discord/discord_cogs.py around lines 11 to 24, the reset_thread
command does not handle exceptions from the enqueue call or the
active_threads.pop operation, which can cause the user to receive a success
message even if the reset fails. Wrap the enqueue and active_threads.pop calls
in a try/except block, and if an exception occurs, catch it and send an error
message to the user indicating the reset failed, ensuring accurate user
feedback.
| recent_messages_count = min(10, len(state.messages)) | ||
| conversation_history_str = "\n".join([ | ||
| f"{msg.get('type', 'unknown')}: {msg.get('content', '')}" | ||
| for msg in state.conversation_history[-5:] | ||
| f"{msg.get('role', 'user')}: {msg.get('content', '')}" | ||
| for msg in state.messages[-recent_messages_count:] | ||
| ]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fallback to legacy "type" field to avoid empty roles after migration
Older AgentState.messages objects still use the "type" key. Restricting to "role" risks emitting "user" for every historic message, degrading context.
- f"{msg.get('role', 'user')}: {msg.get('content', '')}"
+ f"{msg.get('role', msg.get('type', 'user'))}: {msg.get('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.
| recent_messages_count = min(10, len(state.messages)) | |
| conversation_history_str = "\n".join([ | |
| f"{msg.get('type', 'unknown')}: {msg.get('content', '')}" | |
| for msg in state.conversation_history[-5:] | |
| f"{msg.get('role', 'user')}: {msg.get('content', '')}" | |
| for msg in state.messages[-recent_messages_count:] | |
| ]) | |
| recent_messages_count = min(10, len(state.messages)) | |
| conversation_history_str = "\n".join([ | |
| f"{msg.get('role', msg.get('type', 'user'))}: {msg.get('content', '')}" | |
| for msg in state.messages[-recent_messages_count:] | |
| ]) |
🤖 Prompt for AI Agents
In backend/app/agents/devrel/nodes/generate_response_node.py around lines 39 to
43, the code currently uses only the "role" key to determine message roles,
which causes fallback to "user" for older messages that use the "type" key.
Update the code to first check for the "role" key and if missing, fallback to
the "type" key to correctly preserve roles from legacy messages and avoid
degrading context.
| "USER'S CURRENT MESSAGE:\n" | ||
| "\"{latest_message}\"\n\n" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Escape user input to avoid prompt-format breakage
A user message containing " or {} can corrupt the prompt formatting. Use repr(latest_message) or a safe Jinja-style escape.
🤖 Prompt for AI Agents
In backend/app/agents/devrel/prompts/base_prompt.py around lines 11 to 13, the
user input latest_message is inserted directly into the prompt string, which can
break formatting if it contains quotes or braces. To fix this, escape
latest_message by using repr(latest_message) or apply a safe Jinja-style escape
before inserting it into the prompt to prevent prompt-format corruption.
| async def run(self, initial_state: AgentState, thread_id: str) -> AgentState: | ||
| """Execute the agent workflow with memory persistence""" | ||
| try: | ||
| logger.info("Starting %s for session %s", self.agent_name, initial_state.session_id) | ||
| result = await self.graph.ainvoke(initial_state.model_dump()) | ||
| logger.info(f"Starting {self.agent_name} for session {initial_state.session_id}") | ||
| config = {"configurable": {"thread_id": thread_id}} | ||
| existing_state = self.graph.get_state(config) | ||
|
|
||
| if existing_state and existing_state.values: | ||
| existing_agent_state = AgentState(**existing_state.values) | ||
| logger.info(f"RECOVERED existing state for thread {thread_id}:") | ||
| logger.info(f" - Messages: {len(existing_agent_state.messages)}") | ||
| logger.info(f" - Summary: '{existing_agent_state.conversation_summary or 'None'}'") | ||
| logger.info(f" - Summary length: {len(existing_agent_state.conversation_summary or '')} chars") | ||
| logger.info(f" - Topics: {existing_agent_state.key_topics}") | ||
| logger.info(f" - Interaction count: {existing_agent_state.interaction_count}") | ||
| logger.info(f" - Session start: {existing_agent_state.session_start_time}") | ||
| else: | ||
| logger.info(f"No existing state found for thread {thread_id} - starting fresh") | ||
|
|
||
| logger.info(f"Running {self.agent_name} with memory for thread {thread_id}") | ||
|
|
||
| result = await self.graph.ainvoke(initial_state.model_dump(), config) | ||
| return AgentState(**result) |
There was a problem hiding this comment.
Gracefully handle empty thread_id
When thread_id is empty the checkpoint key collides for all sessions, causing state bleeding. Assert non-empty or derive a deterministic fallback.
- config = {"configurable": {"thread_id": thread_id}}
+ if not thread_id:
+ raise ValueError("thread_id must be provided for memory persistence")
+ config = {"configurable": {"thread_id": thread_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 run(self, initial_state: AgentState, thread_id: str) -> AgentState: | |
| """Execute the agent workflow with memory persistence""" | |
| try: | |
| logger.info("Starting %s for session %s", self.agent_name, initial_state.session_id) | |
| result = await self.graph.ainvoke(initial_state.model_dump()) | |
| logger.info(f"Starting {self.agent_name} for session {initial_state.session_id}") | |
| config = {"configurable": {"thread_id": thread_id}} | |
| existing_state = self.graph.get_state(config) | |
| if existing_state and existing_state.values: | |
| existing_agent_state = AgentState(**existing_state.values) | |
| logger.info(f"RECOVERED existing state for thread {thread_id}:") | |
| logger.info(f" - Messages: {len(existing_agent_state.messages)}") | |
| logger.info(f" - Summary: '{existing_agent_state.conversation_summary or 'None'}'") | |
| logger.info(f" - Summary length: {len(existing_agent_state.conversation_summary or '')} chars") | |
| logger.info(f" - Topics: {existing_agent_state.key_topics}") | |
| logger.info(f" - Interaction count: {existing_agent_state.interaction_count}") | |
| logger.info(f" - Session start: {existing_agent_state.session_start_time}") | |
| else: | |
| logger.info(f"No existing state found for thread {thread_id} - starting fresh") | |
| logger.info(f"Running {self.agent_name} with memory for thread {thread_id}") | |
| result = await self.graph.ainvoke(initial_state.model_dump(), config) | |
| return AgentState(**result) | |
| async def run(self, initial_state: AgentState, thread_id: str) -> AgentState: | |
| """Execute the agent workflow with memory persistence""" | |
| try: | |
| logger.info(f"Starting {self.agent_name} for session {initial_state.session_id}") | |
| if not thread_id: | |
| raise ValueError("thread_id must be provided for memory persistence") | |
| config = {"configurable": {"thread_id": thread_id}} | |
| existing_state = self.graph.get_state(config) | |
| if existing_state and existing_state.values: | |
| existing_agent_state = AgentState(**existing_state.values) | |
| logger.info(f"RECOVERED existing state for thread {thread_id}:") | |
| logger.info(f" - Messages: {len(existing_agent_state.messages)}") | |
| logger.info(f" - Summary: '{existing_agent_state.conversation_summary or 'None'}'") | |
| logger.info(f" - Summary length: {len(existing_agent_state.conversation_summary or '')} chars") | |
| logger.info(f" - Topics: {existing_agent_state.key_topics}") | |
| logger.info(f" - Interaction count: {existing_agent_state.interaction_count}") | |
| logger.info(f" - Session start: {existing_agent_state.session_start_time}") | |
| else: | |
| logger.info(f"No existing state found for thread {thread_id} - starting fresh") | |
| logger.info(f"Running {self.agent_name} with memory for thread {thread_id}") | |
| result = await self.graph.ainvoke(initial_state.model_dump(), config) | |
| return AgentState(**result) |
🤖 Prompt for AI Agents
In backend/app/agents/shared/base_agent.py around lines 22 to 44, the run method
does not handle empty thread_id values, which causes checkpoint key collisions
and state bleeding across sessions. Add a check at the start of the method to
assert that thread_id is not empty or provide a deterministic fallback value if
it is empty. This ensures unique keys for each session and prevents state
overlap.
There was a problem hiding this comment.
the user_id itself maps as thread_id for any user so not a problem. will always be passed.
There was a problem hiding this comment.
@smokeyScraper, understood! Thank you for the clarification. Since user_id directly maps to thread_id in your architecture, you're guaranteed to always have a non-empty value, which eliminates the potential collision issue. Your design ensures each user gets a unique thread identifier automatically.
✏️ Learnings added
Learnt from: smokeyScraper
PR: AOSSIE-Org/Devr.AI#76
File: backend/app/agents/shared/base_agent.py:22-44
Timestamp: 2025-06-14T05:57:43.849Z
Learning: In the Devr.AI codebase, user_id directly maps as thread_id for memory persistence in the agent system, ensuring thread_id is always non-empty and unique per user.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/core/orchestration/agent_coordinator.py (1)
99-103:⚠️ Potential issueDuplicate summary persistence — store once
The final summary is written explicitly (
store_summary_to_database) and then
clear_thread_memory(force_clear=True)is invoked, which (per earlier discussion)
already persists the same summary. This double-writes the record and wastes I/O.- # Store final summary to database - await store_summary_to_database(state) - - # Clear from InMemorySaver - await self.devrel_agent.clear_thread_memory(memory_thread_id, force_clear=True) + # Let clear_thread_memory handle summary persistence + await self.devrel_agent.clear_thread_memory(memory_thread_id, force_clear=True)
🧹 Nitpick comments (1)
backend/app/core/orchestration/agent_coordinator.py (1)
4-4: Remove unuseddatetimeimport
datetimeisn’t referenced anywhere in this module and is flagged by Ruff (F401).
Eliminate it to keep the import list lean.-from datetime import datetime🧰 Tools
🪛 Ruff (0.11.9)
4-4:
datetime.datetimeimported but unusedRemove unused import:
datetime.datetime(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/core/orchestration/agent_coordinator.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/core/orchestration/agent_coordinator.py
4-4: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
🔇 Additional comments (1)
backend/app/core/orchestration/agent_coordinator.py (1)
38-40: Guard against emptymemory_thread_idFalling back to
""when bothmemory_thread_idanduser_idare absent can lead to
different users sharing the same blank thread key, corrupting their conversation
state. Consider:
- Failing fast when neither identifier is present, or
- Generating a deterministic key (e.g.,
f"anonymous:{session_id}").Please verify that upstream producers always provide one of the two fields.
| logger.info(f"Running DevRel agent for session {session_id} with memory thread {memory_thread_id}") | ||
| result_state = await self.devrel_agent.run(initial_state, memory_thread_id) | ||
|
|
||
| # Check if thread timeout was reached during processing | ||
| if result_state.memory_timeout_reached: | ||
| await self._handle_memory_timeout(memory_thread_id, result_state) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
memory_timeout_reached check risks AttributeError
result_state.memory_timeout_reached assumes the attribute exists on every
returned AgentState. If an older agent version returns a state lacking that
field, this will raise. Wrap with getattr(result_state, "memory_timeout_reached", False)
or ensure the contract is enforced across all agents.
🤖 Prompt for AI Agents
In backend/app/core/orchestration/agent_coordinator.py around lines 56 to 62,
the code accesses result_state.memory_timeout_reached directly, which may cause
an AttributeError if the attribute is missing in older AgentState versions. To
fix this, replace the direct attribute access with getattr(result_state,
"memory_timeout_reached", False) to safely check the attribute's presence and
default to False if it does not exist.
2 participants
Interactions Attached
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
weaviate-clientdependency for future integrations.