fix: run Composio meta tools in a per-conversation tool-router session#1004
fix: run Composio meta tools in a per-conversation tool-router session#1004milovate wants to merge 2 commits into
Conversation
|
@codex review |
📝 WalkthroughWalkthroughThis PR adds meta-tool routing to Composio integration. It introduces a session-id cache key and three utilities: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@cursor review |
|
Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d84ab5b05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| convo = Conversation.objects.get( | ||
| id=api_hashids.decode(conversation_id)[0] |
There was a problem hiding this comment.
This lookup trusts the conversation_id copied from gui.session_state["variables"], but in the bots API path request overrides are merged after the system variables (daras_ai_v2/bots.py:514-516), so a client can send a different variables.conversation_id than the already-validated request conversation. When a COMPOSIO_* tool runs in that scenario, this code will read/reuse/save the Composio session on that unrelated Conversation (or fail if the id is invalid), mixing tool-router workbench/auth state across conversations. Use the validated active conversation ID or verify the decoded conversation matches it before persisting/reusing the session.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/composio_tools.py`:
- Around line 302-316: Wrap the caught exception and re-raise the UserError with
the original exception chained so tracebacks are preserved: change the except
block that catches (IndexError, Conversation.DoesNotExist) around
Conversation.objects.get(...) / api_hashids.decode(conversation_id) to capture
the exception (e) and use "raise UserError(f'Conversation with
id={conversation_id} not found') from e" instead of raising without chaining.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94fb6000-4e6e-4873-8e3f-d9c642faee87
📒 Files selected for processing (4)
bots/migrations/0121_conversation_composio_session_id.pybots/models/convo_msg.pydaras_ai_v2/base.pyfunctions/composio_tools.py
| if conversation_id: | ||
| try: | ||
| convo = Conversation.objects.get( | ||
| id=api_hashids.decode(conversation_id)[0] | ||
| ) | ||
| except (IndexError, Conversation.DoesNotExist): | ||
| raise UserError(f"Conversation with id={conversation_id} not found") | ||
| if convo.composio_session_id: | ||
| return convo.composio_session_id | ||
| session = composio.create( | ||
| user_id=user_id, manage_connections=manage_connections | ||
| ) | ||
| convo.composio_session_id = session.session_id | ||
| convo.save(update_fields=["composio_session_id"]) | ||
| return session.session_id |
There was a problem hiding this comment.
Chain the exception for better error context.
When re-raising as UserError, chain the original exception to preserve the traceback for debugging.
Proposed fix
try:
convo = Conversation.objects.get(
id=api_hashids.decode(conversation_id)[0]
)
except (IndexError, Conversation.DoesNotExist):
- raise UserError(f"Conversation with id={conversation_id} not found")
+ raise UserError(f"Conversation with id={conversation_id} not found") from NoneUsing from None explicitly suppresses the chained exception when the original is not meaningful to callers (IndexError from decode is an implementation detail).
📝 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 conversation_id: | |
| try: | |
| convo = Conversation.objects.get( | |
| id=api_hashids.decode(conversation_id)[0] | |
| ) | |
| except (IndexError, Conversation.DoesNotExist): | |
| raise UserError(f"Conversation with id={conversation_id} not found") | |
| if convo.composio_session_id: | |
| return convo.composio_session_id | |
| session = composio.create( | |
| user_id=user_id, manage_connections=manage_connections | |
| ) | |
| convo.composio_session_id = session.session_id | |
| convo.save(update_fields=["composio_session_id"]) | |
| return session.session_id | |
| if conversation_id: | |
| try: | |
| convo = Conversation.objects.get( | |
| id=api_hashids.decode(conversation_id)[0] | |
| ) | |
| except (IndexError, Conversation.DoesNotExist): | |
| raise UserError(f"Conversation with id={conversation_id} not found") from None | |
| if convo.composio_session_id: | |
| return convo.composio_session_id | |
| session = composio.create( | |
| user_id=user_id, manage_connections=manage_connections | |
| ) | |
| convo.composio_session_id = session.session_id | |
| convo.save(update_fields=["composio_session_id"]) | |
| return session.session_id |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 308-308: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/composio_tools.py` around lines 302 - 316, Wrap the caught
exception and re-raise the UserError with the original exception chained so
tracebacks are preserved: change the except block that catches (IndexError,
Conversation.DoesNotExist) around Conversation.objects.get(...) /
api_hashids.decode(conversation_id) to capture the exception (e) and use "raise
UserError(f'Conversation with id={conversation_id} not found') from e" instead
of raising without chaining.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/inbuilt_tools.py (1)
329-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize reset with session creation.
Lines 329-343 update
Conversation.composio_session_id, but the create path infunctions/composio_tools.pyprotects the same row withtransaction.atomic()+select_for_update(). Without taking that same lock here, a reset racing with a meta-tool call can clear a freshly created router session id aftercomposio.create(...)returns, orphaning the external session and violating the per-conversation lifecycle this PR is introducing.Proposed fix
+from django.db import transaction from django.core.exceptions import ValidationError from django.utils import timezone ... def call(self) -> dict: from bots.models import Conversation from routers.bots_api import api_hashids try: - convo = Conversation.objects.get( - id=api_hashids.decode(self.conversation_id)[0] - ) + with transaction.atomic(): + convo = Conversation.objects.select_for_update().get( + id=api_hashids.decode(self.conversation_id)[0] + ) + convo.reset_at = timezone.now() + convo.composio_session_id = "" + convo.save(update_fields=["reset_at", "composio_session_id"]) except (IndexError, Conversation.DoesNotExist): return { "success": False, "error": ( f"Conversation not found for conversation_id={self.conversation_id}. " "Please call this tool from an active deployment." ), } - convo.reset_at = timezone.now() - convo.composio_session_id = "" - convo.save(update_fields=["reset_at", "composio_session_id"]) return {"success": True}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/inbuilt_tools.py` around lines 329 - 343, The reset path in inbuilt_tools.py must acquire the same DB lock as composio.create to avoid races: wrap the Conversation lookup and update in a transaction.atomic() block and fetch the row with select_for_update() (i.e., replace Conversation.objects.get(...) with Conversation.objects.select_for_update().get(...)) before setting convo.reset_at and convo.composio_session_id and saving; keep the same IndexError/Conversation.DoesNotExist handling around the locked get so you still return the same error when absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@functions/inbuilt_tools.py`:
- Around line 329-343: The reset path in inbuilt_tools.py must acquire the same
DB lock as composio.create to avoid races: wrap the Conversation lookup and
update in a transaction.atomic() block and fetch the row with
select_for_update() (i.e., replace Conversation.objects.get(...) with
Conversation.objects.select_for_update().get(...)) before setting convo.reset_at
and convo.composio_session_id and saving; keep the same
IndexError/Conversation.DoesNotExist handling around the locked get so you still
return the same error when absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de4f64e4-b1ef-44d2-98b3-fe843fca8641
📒 Files selected for processing (2)
functions/composio_tools.pyfunctions/inbuilt_tools.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/composio_tools.py`:
- Around line 118-121: Don’t fall back to a stale function["slug"] when the row
has a URL that no longer points at a tool: call
get_external_tool_slug_from_url(url) and only use function.get("slug") when the
URL is missing/empty; if the URL exists but yields no slug, treat the row as
having no composio slug. Update the composio_tool_slug assignment logic (the
composio_tool_slug variable, get_external_tool_slug_from_url, and the
is_composio_meta_tool check) so you only consult the stored slug when there is
no URL to resolve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28e873e0-6dc6-4061-a519-c8c6c2ac72c7
📒 Files selected for processing (2)
functions/base_llm_tool.pyfunctions/composio_tools.py
| composio_tool_slug = get_external_tool_slug_from_url( | ||
| function.get("url") or "" | ||
| ) or function.get("slug") | ||
| if not composio_tool_slug or not is_composio_meta_tool(composio_tool_slug): |
There was a problem hiding this comment.
Avoid classifying rows from stale slug state.
get_external_tool_slug_from_url(...) or function.get("slug") can treat a non-tool row as a Composio meta tool if the dict still carries an older slug. The editor in functions/base_llm_tool.py refreshes fn["slug"] only when the current URL resolves to a tool, and its non-tool branch does not clear that field, so this warning can fire after the row stops pointing at /tools/.
Suggested change
- composio_tool_slug = get_external_tool_slug_from_url(
- function.get("url") or ""
- ) or function.get("slug")
+ url = function.get("url") or ""
+ composio_tool_slug = get_external_tool_slug_from_url(url)
+ if not composio_tool_slug and not url:
+ composio_tool_slug = function.get("slug")📝 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_tool_slug = get_external_tool_slug_from_url( | |
| function.get("url") or "" | |
| ) or function.get("slug") | |
| if not composio_tool_slug or not is_composio_meta_tool(composio_tool_slug): | |
| url = function.get("url") or "" | |
| composio_tool_slug = get_external_tool_slug_from_url(url) | |
| if not composio_tool_slug and not url: | |
| composio_tool_slug = function.get("slug") | |
| if not composio_tool_slug or not is_composio_meta_tool(composio_tool_slug): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/composio_tools.py` around lines 118 - 121, Don’t fall back to a
stale function["slug"] when the row has a URL that no longer points at a tool:
call get_external_tool_slug_from_url(url) and only use function.get("slug") when
the URL is missing/empty; if the URL exists but yields no slug, treat the row as
having no composio slug. Update the composio_tool_slug assignment logic (the
composio_tool_slug variable, get_external_tool_slug_from_url, and the
is_composio_meta_tool check) so you only consult the stored slug when there is
no URL to resolve.
|
having a conversation_id means that this only works for deployments? I think the COMPOSIO_SEARCH_TOOLS will return a session_id so we can just use that in-process inside the celery task |
Route COMPOSIO_* tools through composio.create/use + session.execute so meta-tool chains work within a single run, while app tools keep the existing tools.execute path. Warn in the builder when Composio meta tools use mixed scopes. Co-authored-by: Cursor <cursoragent@cursor.com>
milovate
force-pushed
the
fix/composio-manage-connections
branch
from
13d2148 to
81cc2c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
functions/composio_tools.py (1)
113-116:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid stale-slug fallback when URL is present but no longer resolves to a tool.
get_external_tool_slug_from_url(url) or function.get("slug")at Lines 113-116 can classify a non-tool row as meta ifslugis stale. Only fall back to storedslugwhen URL is empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/composio_tools.py` around lines 113 - 116, The code currently falls back to function.get("slug") even when a URL is present but no longer resolves; change the logic so you only use get_external_tool_slug_from_url(url) when url is non-empty and only fall back to the stored slug when the URL is empty. Concretely: retrieve url = function.get("url") or "" ; if url is non-empty call get_external_tool_slug_from_url(url) and assign composio_tool_slug from that result; otherwise assign composio_tool_slug = function.get("slug"); then call is_composio_meta_tool(composio_tool_slug) as before. This prevents classifying rows as meta based on a stale stored slug when the URL exists but doesn't resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/composio_tools.py`:
- Around line 138-142: The cached COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY is shared
across all callers causing sessions to be reused when user_id changes; update
the caching to partition by caller identity (e.g., include user_id or a caller
scope in the key) before reading/writing gui.session_state so each user_id gets
its own stored session_id; in the function that calls composio.create and
interacts with gui.session_state (the block that checks
COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY, calls composio.create(user_id=...,
manage_connections=...), and assigns session.session_id) build a composite key
(like f"{COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY}:{user_id}" or a per-scope map) to
read/assign the correct session_id and fall back to creating a new session when
no matching keyed entry exists.
---
Duplicate comments:
In `@functions/composio_tools.py`:
- Around line 113-116: The code currently falls back to function.get("slug")
even when a URL is present but no longer resolves; change the logic so you only
use get_external_tool_slug_from_url(url) when url is non-empty and only fall
back to the stored slug when the URL is empty. Concretely: retrieve url =
function.get("url") or "" ; if url is non-empty call
get_external_tool_slug_from_url(url) and assign composio_tool_slug from that
result; otherwise assign composio_tool_slug = function.get("slug"); then call
is_composio_meta_tool(composio_tool_slug) as before. This prevents classifying
rows as meta based on a stale stored slug when the URL exists but doesn't
resolve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43ae4080-2102-4aba-ae89-2ba163c5e980
📒 Files selected for processing (2)
functions/base_llm_tool.pyfunctions/composio_tools.py
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/base_llm_tool.py
| if session_id := gui.session_state.get(COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY): | ||
| return session_id | ||
| session = composio.create(user_id=user_id, manage_connections=manage_connections) | ||
| gui.session_state[COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY] = session.session_id | ||
| return session.session_id |
There was a problem hiding this comment.
Session cache is not partitioned by caller identity/scope.
At Line 138, the cached COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY is reused for all later calls in the same GUI session, even if user_id changes. That can route meta-tool execution through the wrong tool-router session and mix identities across runs/conversations.
Suggested fix
-COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY = "__composio_tool_router_session_id__"
+COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY = "__composio_tool_router_session_id__"
def get_or_create_composio_tool_router_session_id(
composio: Composio,
*,
user_id: str,
redirect_url: str,
) -> str:
manage_connections = {"enable": True, "callback_url": redirect_url}
-
- if session_id := gui.session_state.get(COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY):
- return session_id
+ key = f"{COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY}:{user_id}"
+ if session_id := gui.session_state.get(key):
+ return session_id
session = composio.create(user_id=user_id, manage_connections=manage_connections)
- gui.session_state[COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY] = session.session_id
+ gui.session_state[key] = session.session_id
return session.session_id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions/composio_tools.py` around lines 138 - 142, The cached
COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY is shared across all callers causing
sessions to be reused when user_id changes; update the caching to partition by
caller identity (e.g., include user_id or a caller scope in the key) before
reading/writing gui.session_state so each user_id gets its own stored
session_id; in the function that calls composio.create and interacts with
gui.session_state (the block that checks COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY,
calls composio.create(user_id=..., manage_connections=...), and assigns
session.session_id) build a composite key (like
f"{COMPOSIO_TOOL_ROUTER_SESSION_ID_KEY}:{user_id}" or a per-scope map) to
read/assign the correct session_id and fall back to creating a new session when
no matching keyed entry exists.
2 participants
COMPOSIO_* tools require a Composio tool-router session, so persist composio_session_id on Conversation and route meta-tool calls through composio.use while keeping app tools on tools.execute.
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.