fix(agent): reuse cached Composio toolkit actions#4358
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesScope-Aware Action Execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: 3887957068
ℹ️ 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".
coderabbitai
Bot
added
agent
coderabbitai
Bot
removed
the
rust-core
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/agent/harness/test_support_tests.rs (1)
30-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WorkspaceEnvGuardis duplicated here and insubagent_runner/ops_tests.rswith divergentunsafeusage.This copy wraps
set_var/remove_varinunsafe, while the copy insrc/openhuman/agent/harness/subagent_runner/ops_tests.rs(Lines 211-225) does not. Only one can be correct for the crate edition: under edition 2024 these calls areunsafe(the other copy won't compile), and under 2021 thisunsafeblock isunused_unsafe. Consider extracting a single shared guard intotest_supportto keep them in sync. See the companion comment on the other file for the compile-risk detail.🤖 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 `@src/openhuman/agent/harness/test_support_tests.rs` around lines 30 - 53, `WorkspaceEnvGuard` is duplicated with inconsistent `std::env::set_var`/`remove_var` handling, so unify the implementation to avoid edition-dependent compile issues. Move the guard into a single shared test-support location and have both `test_support_tests` and `subagent_runner::ops_tests` use that shared `WorkspaceEnvGuard` instead of maintaining separate copies; ensure the chosen implementation matches the crate edition’s safety requirements and removes the divergent `unsafe` usage.
🤖 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.
Nitpick comments:
In `@src/openhuman/agent/harness/test_support_tests.rs`:
- Around line 30-53: `WorkspaceEnvGuard` is duplicated with inconsistent
`std::env::set_var`/`remove_var` handling, so unify the implementation to avoid
edition-dependent compile issues. Move the guard into a single shared
test-support location and have both `test_support_tests` and
`subagent_runner::ops_tests` use that shared `WorkspaceEnvGuard` instead of
maintaining separate copies; ensure the chosen implementation matches the crate
edition’s safety requirements and removes the divergent `unsafe` usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f02a027-3542-4329-b300-9afe4cf0ddee
📒 Files selected for processing (5)
src/openhuman/agent/harness/subagent_runner/ops_tests.rssrc/openhuman/agent/harness/test_support_tests.rssrc/openhuman/composio/action_tool.rssrc/openhuman/composio/mod.rssrc/openhuman/composio/ops_tests.rs
✅ Files skipped from review due to trivial changes (1)
- src/openhuman/composio/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/composio/action_tool.rs
…4317-composio-cached-actions # Conflicts: # src/openhuman/agent/harness/test_support_tests.rs
coderabbitai
Bot
removed
agent
Review — not an approvalThe core change is sound: reuse a non-empty I especially like that Two things to address before this is ready:
Minor: |
|
Maintainer review changes: merged current
No behavioral changes to the PR's own logic. Letting PR CI verify. |
|
Updated the PR description to cover both review items.
|
# Conflicts: # src/openhuman/composio/action_tool.rs
coderabbitai
Bot
added
agent
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)
.github/workflows/ci-lite.yml (1)
902-914: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent code injection via template expansion.
GitHub context variables and step outputs that contain branch names are interpolated directly into bash scripts. This exposes the workflow to code injection if a branch name is maliciously crafted. Pass these values securely via step
envvariables instead.
.github/workflows/ci-lite.yml#L902-L914: Mapgithub.event_name,github.base_ref, andgithub.event.beforeintoenvvariables, and use standard shell variables (e.g.,"${BASE_REF}") in the script..github/workflows/ci-lite.yml#L939-L952: Mapsteps.coverage-compare.outputs.refinto anenvvariable and use it in thediff-covercommand instead of${{ ... }}.🔒️ Proposed fixes
For
coverage-compare(Lines 902-914):- name: Resolve coverage compare ref if: needs.changes.outputs.coverage == 'true' id: coverage-compare + env: + EVENT_NAME: ${{ github.event_name }} + BASE_REF: ${{ github.base_ref }} + EVENT_BEFORE: ${{ github.event.before }} run: | set -euo pipefail - if [ "${{ github.event_name }}" = "pull_request" ]; then - git fetch origin "${{ github.base_ref }}" --depth=200 - echo "ref=origin/${{ github.base_ref }}" >> "$GITHUB_OUTPUT" - elif [ -n "${{ github.event.before }}" ] && [ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then - echo "ref=${{ github.event.before }}" >> "$GITHUB_OUTPUT" + if [ "${EVENT_NAME}" = "pull_request" ]; then + git fetch origin "${BASE_REF}" --depth=200 + echo "ref=origin/${BASE_REF}" >> "$GITHUB_OUTPUT" + elif [ -n "${EVENT_BEFORE}" ] && [ "${EVENT_BEFORE}" != "0000000000000000000000000000000000000000" ]; then + echo "ref=${EVENT_BEFORE}" >> "$GITHUB_OUTPUT" else echo "ref=HEAD^" >> "$GITHUB_OUTPUT" fiFor the coverage enforcement step (Lines 939-952):
- name: Enforce >= 80% coverage on changed lines if: needs.changes.outputs.coverage == 'true' + env: + COMPARE_REF: ${{ steps.coverage-compare.outputs.ref }} run: | set -euo pipefail mapfile -t LCOV_FILES < <(find lcov-artifacts -type f -name '*.info' | sort) if [ "${`#LCOV_FILES`[@]}" -eq 0 ]; then echo "::error::No lcov files found; coverage gate cannot run" exit 1 fi diff-cover "${LCOV_FILES[@]}" \ - --compare-branch="${{ steps.coverage-compare.outputs.ref }}" \ + --compare-branch="${COMPARE_REF}" \ --fail-under=80 \ --html-report diff-coverage.html \ --markdown-report diff-coverage.md🤖 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 @.github/workflows/ci-lite.yml around lines 902 - 914, Prevent shell injection in both coverage workflow steps: in the coverage-compare step (.github/workflows/ci-lite.yml lines 902-914), map github.event_name, github.base_ref, and github.event.before to env variables and reference those quoted shell variables in the script; in the coverage enforcement step (.github/workflows/ci-lite.yml lines 939-952), map steps.coverage-compare.outputs.ref to an env variable and pass the quoted shell variable to diff-cover instead of directly interpolating the GitHub expression.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci-lite.yml:
- Around line 902-914: Prevent shell injection in both coverage workflow steps:
in the coverage-compare step (.github/workflows/ci-lite.yml lines 902-914), map
github.event_name, github.base_ref, and github.event.before to env variables and
reference those quoted shell variables in the script; in the coverage
enforcement step (.github/workflows/ci-lite.yml lines 939-952), map
steps.coverage-compare.outputs.ref to an env variable and pass the quoted shell
variable to diff-cover instead of directly interpolating the GitHub expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8be86fd7-4ce7-448e-a1ab-68e6f166b254
📒 Files selected for processing (2)
.github/workflows/ci-lite.yml.github/workflows/pr-quality.yml
4 participants
Summary
ConnectedIntegration.toolscatalogue forintegrations_agenttoolkit delegation when action schemas are already cached.list_toolsonly when the cached toolkit catalogue is empty./toolsrequest.action_tool.rs, so stale cached actions cannot run after a scope is narrowed.Problem
list_toolscalls adding avoidable latency to integration delegation.run_typed_modealready finds the connected toolkit from the parent snapshot, but backend mode still refetched toolkit actions before using that cached catalogue.Solution
cached_integration.toolsas the primary action source for toolkit overrides.integrations_agent, and asserts the fake backend/toolscall count does not increase.Submission Checklist
Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4317-composio-cached-actions388795706869c18872bb60fc2f2a736265ac59d7Validation Run
pnpm --filter openhuman-app format:check(no frontend changes)pnpm typecheck(no TypeScript changes)GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib integrations_agent_reuses_cached_toolkit_actions_without_refetching_list_tools -- --nocaptureGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib agent::harness::subagent_runner::ops::tests::cargo fmt --manifest-path Cargo.toml --checkgit diff --checkGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
integrations_agentnow reuses cached Composio action schemas when available instead of refetchinglist_toolson every delegation.Parity Contract
list_tools; direct mode and unavailable-client fallbacks continue to use the cached catalogue./toolsrequest.Duplicate / Superseded PR Handling
Summary by CodeRabbit