feat(composio): improve toolkit sync and connection handling#507
Merged
senamakel merged 15 commits intotinyhumansai:mainfrom Apr 11, 2026
Merged
feat(composio): improve toolkit sync and connection handling#507senamakel merged 15 commits intotinyhumansai:mainfrom
senamakel merged 15 commits intotinyhumansai:mainfrom
Conversation
…ucture - Added a new input parameter `build_target` to specify the environment (production or staging) for the release process. - Made `release_type` input optional with a default value of `patch`. - Refactored job names and dependencies to reflect the new build target logic, including conditional steps for production and staging environments. - Introduced a `resolve` step to determine build outputs based on the selected environment, enhancing the workflow's flexibility and clarity. - Updated the `create-release` job to depend on the new `prepare-build` job, ensuring proper execution flow based on the build target.
…and testing - Added `KNOWN_COMPOSIO_TOOLKITS` constant to facilitate access to available toolkits. - Implemented unit tests for `useComposioIntegrations` to ensure correct behavior during toolkit and connection fetching, including error handling scenarios. - Updated `Skills` page to utilize the new `KNOWN_COMPOSIO_TOOLKITS` for improved toolkit display logic. - Refactored hooks to handle connection errors gracefully and maintain toolkit visibility. - Enhanced backend integration by updating Composio client configuration to streamline toolkit management.
- Deleted the `channel_delivery_instructions` function, which provided response guidelines for Telegram messages. This change simplifies the message processing logic in the `process_channel_message` function by eliminating unnecessary instructions, enhancing clarity and maintainability.
… toggles - Updated the `build_composio_client` function to remove unnecessary configuration checks, as Composio is always enabled when the user is signed in. - Revised the `resolve_client` function to clarify error handling related to user authentication. - Streamlined the `IntegrationsConfig` structure by removing toggles for Composio and related backend settings, ensuring a consistent configuration approach across integrations. - Adjusted tests to reflect the removal of integration toggles and focus on core API key usage.
…handling - Eliminated the `disabled` state from the `useComposioIntegrations` hook, as Composio is always enabled when the user is authenticated. - Updated error handling to surface backend connection issues directly, replacing previous checks for a disabled state. - Revised tests to reflect the new error handling logic, ensuring clarity in toolkit fetch error reporting.
…figuration - Revised the `build_client` function to prioritize app-session JWT for user authentication, enhancing clarity in the fallback mechanism to `config.api_key`. - Improved error messages in `resolve_client` and `build_client` to provide clearer guidance on authentication issues related to session tokens. - Streamlined comments and documentation to reflect the updated authentication flow, ensuring consistency across integration components.
- Introduced a new `unwrapCliEnvelope` function to handle the response format from the Rust side, allowing for easier access to the flat shapes defined in `./types`. - Updated `listToolkits`, `listConnections`, `listTools`, `authorize`, `deleteConnection`, and `execute` functions to utilize the new unwrapping logic, improving response handling consistency across the Composio API. - Enhanced error handling by ensuring that responses without logs pass through unchanged, maintaining backward compatibility.
… tool integration - Revised the `when_to_use` description in `agent.toml` to clarify the role of the Skills Agent as a service integration specialist, emphasizing its capability to execute both Composio and QuickJS skill tools. - Expanded the `prompt.md` documentation to detail available tool surfaces and typical Composio flow, improving clarity on how to interact with external services. - Implemented category overrides for Composio tools in `tools.rs` to ensure they are recognized as part of the Skill category, allowing proper access through the skills sub-agent. - Added tests to verify that Composio tools are correctly filtered and accessible by the skills sub-agent, ensuring robust integration and functionality.
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRestructures release workflow with production/staging branching, centralizes integrations client/config around root Config, reclassifies Composio tools as Skill, simplifies frontend Composio hook/API and Skills UI (removes third‑party skill UI), hardens embedding panics, and adds/updates related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GH_Actions as "GitHub Actions (workflow)"
participant Prepare as "prepare-build job"
participant Git as "Git (checkout/tag/push)"
participant Release as "Release steps / Artifact storage"
User->>GH_Actions: trigger workflow (build_target)
GH_Actions->>Prepare: run prepare-build
Prepare-->>GH_Actions: outputs {version, tag, sha, build_ref, release_enabled, base_url}
alt build_target == production
GH_Actions->>Git: create tag & push
GH_Actions->>Release: create-release & upload production artifacts
else build_target == staging
GH_Actions->>Release: package/upload staging artifacts (no tag/release)
end
GH_Actions->>Git: checkout using build_ref for build jobs
GH_Actions->>Release: cleanup (only if release_enabled == 'true')
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/memory/embeddings.rs (1)
242-267:⚠️ Potential issue | 🟠 MajorLog the remaining failure branches before returning.
src/openhuman/memory/store/unified/documents.rscurrently doesembed_one(...).await.ok(), so theErrpaths on Line 252, Line 255, and Line 267 can now drop document embeddings silently. Please emit a structured log on those branches with stable fields likemodel,batch_len, and the error/panic message so indexing failures stay observable.As per coding guidelines "Add substantial, development-oriented logs on new/changed flows; include logs at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths".Suggested fix
FastembedState::Ready(model) => { + let batch_len = items.len(); // Also guard the actual embed call — fastembed / ort // can panic on certain inputs or runtime errors, and // we want to surface those as regular errors too. let embed_result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { model.embed(items, None) })); match embed_result { Ok(Ok(vectors)) => Ok(vectors), - Ok(Err(e)) => Err(anyhow::anyhow!("fastembed embed failed: {e}")), + Ok(Err(e)) => { + tracing::error!( + target: "memory.embeddings", + model = %provider, + batch_len, + error = %e, + "[embeddings] fastembed embed failed" + ); + Err(anyhow::anyhow!("fastembed embed failed: {e}")) + } Err(panic_payload) => { let panic_msg = extract_panic_message(&panic_payload); - Err(anyhow::anyhow!("fastembed embed panicked: {panic_msg}")) + tracing::error!( + target: "memory.embeddings", + model = %provider, + batch_len, + panic = %panic_msg, + "[embeddings] fastembed embed panicked" + ); + Err(anyhow::anyhow!("fastembed embed panicked: {panic_msg}")) } } } FastembedState::Failed(message) => Err(anyhow::anyhow!(message.clone())), FastembedState::Uninitialized => { Err(anyhow::anyhow!("fastembed provider did not initialize")) } } }) .await; - join_result.map_err(|e| anyhow::anyhow!("fastembed task join failed: {e}"))? + join_result.map_err(|e| { + tracing::error!( + target: "memory.embeddings", + model = %self.model, + error = %e, + "[embeddings] fastembed blocking task join failed" + ); + anyhow::anyhow!("fastembed task join failed: {e}") + })?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/memory/embeddings.rs` around lines 242 - 267, The code currently swallows embed errors/panics; update the failure branches inside the FastembedState handling and the task join error to emit structured logs before returning Err so embed_one(...).await.ok() callers see failures in logs. Specifically, in the match arm handling Ok(Err(e)) (fastembed embed failed), the Err(panic_payload) branch (fastembed embed panicked via extract_panic_message), the FastembedState::Failed(message) arm, FastembedState::Uninitialized arm, and the join_result.map_err branch, call the project logger (use the same logger used elsewhere in this module) to log a structured record containing stable fields model (the model identifier available in FastembedState::Ready or message/context for Failed), batch_len (items.len()), and the error/panic message; then return the same Err as before. Also ensure embed_one's callers that use .ok() will at least have these logs to surface indexing failures..github/workflows/release.yml (1)
349-357:⚠️ Potential issue | 🟠 MajorStaging builds inherit production updater endpoint from
vars.UPDATER_ENDPOINT.The "Define Tauri configuration overrides" step passes
UPDATER_ENDPOINT: ${{ vars.UPDATER_ENDPOINT }}toprepareTauriConfig.jsregardless ofbuild_target. Ifvars.UPDATER_ENDPOINTis set to a production URL, staging builds will be configured to check that production endpoint for updates, even thoughBASE_URLcorrectly switches to staging. The fallback inprepareTauriConfig.jsonly mitigates this ifUPDATER_ENDPOINTis empty or unset.To fix, either:
- Make
UPDATER_ENDPOINTconditional onbuild_target(similar to howBASE_URLis handled inprepare-build), or- Disable
WITH_UPDATERfor staging builds, or- Ensure
vars.UPDATER_ENDPOINTis never populated (rely on the BASE_URL fallback).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 349 - 357, The workflow step with id config-overrides is passing UPDATER_ENDPOINT unconditionally which causes staging builds to inherit a production updater URL; update the step so UPDATER_ENDPOINT is set conditionally based on build_target (or unset for staging) or set WITH_UPDATER to "false" for non-production builds before calling prepareTauriConfig.js; reference the env names UPDATER_ENDPOINT, WITH_UPDATER and BASE_URL and the build_target decision logic used in prepare-build, or alternatively ensure prepareTauriConfig.js receives no UPDATER_ENDPOINT so its internal fallback is used.
🧹 Nitpick comments (3)
app/src/pages/__tests__/Skills.composio-catalog.test.tsx (1)
20-29: Remove extraneousdisabledfield from mock.The
useComposioIntegrationshook interface (inapp/src/lib/composio/hooks.ts:8-19) does not include adisabledfield — it only returnstoolkits,connectionByToolkit,loading,error, andrefresh. The extra field doesn't break the test but could mislead future developers into thinking the hook exposes this property.🔧 Proposed fix
vi.mock('../../lib/composio/hooks', () => ({ useComposioIntegrations: () => ({ toolkits: [], connectionByToolkit: new Map(), refresh: vi.fn(), loading: false, error: null, - disabled: false, }), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/__tests__/Skills.composio-catalog.test.tsx` around lines 20 - 29, The mock for the hook useComposioIntegrations in the test currently returns an extra disabled property; remove the extraneous disabled field from the mocked return object so the mock matches the actual hook shape (toolkits, connectionByToolkit, refresh, loading, error) and update the vi.mock block that defines useComposioIntegrations accordingly.src/openhuman/agent/agents/skills_agent/prompt.md (2)
27-27: Inconsistent spelling: "authorise" vs. "authorize".The document uses British spelling "re-authorise" here but American spelling in the function name
composio_authorize(line 10, 18). For consistency in AI-consumed prompts, consider standardizing to one variant throughout.📝 Suggested fix for consistency
-- **Handle OAuth expiry** — if an action fails with an auth error, surface the need to re-authorise rather than looping. +- **Handle OAuth expiry** — if an action fails with an auth error, surface the need to re-authorize rather than looping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agents/skills_agent/prompt.md` at line 27, The prompt text uses British spelling "re-authorise" while the function names use American spelling `composio_authorize`; standardize the spelling across the prompt and code references by replacing "re-authorise" with "re-authorize" (or vice versa if you prefer British) so the prompt matches the function name `composio_authorize` and any other occurrences, ensuring consistent terminology in `prompt.md` and references to `composio_authorize`.
19-19: Consider clarifying the toolkit scope phrasing.The phrase "optionally scoped to one or two toolkits" is oddly specific. Consider rephrasing to "optionally filtered by toolkit" or "optionally scoped to specific toolkits" to avoid implying an arbitrary limit.
📝 Suggested clarification
-3. Once connected, call `composio_list_tools` (optionally scoped to one or two toolkits) to discover the action slug and its JSON schema. +3. Once connected, call `composio_list_tools` (optionally filtered by toolkit) to discover the action slug and its JSON schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/agents/skills_agent/prompt.md` at line 19, Update the phrasing in prompt.md where it instructs calling composio_list_tools: replace "optionally scoped to one or two toolkits" with a clearer, non-arbitrary phrase such as "optionally filtered by toolkit" or "optionally scoped to specific toolkits" so the guidance about toolkit scoping for composio_list_tools is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 796-800: The cleanup job's if condition only runs when
needs.create-release.result == 'success', so tags pushed by prepare-build remain
on origin if create-release failed; update the if expression in the job that
lists needs: [prepare-build, create-release, build-desktop, build-docker] to
include create-release failures (e.g. change needs.create-release.result ==
'success' to (needs.create-release.result == 'success' ||
needs.create-release.result == 'failure')), and apply the same change to the
other identical if block later (the block spanning the other 804-815 region) so
cleanup runs when create-release fails as well.
In `@app/src/lib/composio/hooks.ts`:
- Around line 89-100: On successful polling in the interval handler (the promise
chain that calls listConnections and then setConnections), clear the stale
connection error so the UI doesn't keep showing a recovered failure;
specifically, after setConnections(resp.connections ?? []) (and guarded by
mountedRef.current), call the same state updater used for the connection error
(e.g., setError(null) or setConnectionError(null)) to reset the error state so a
prior listConnections() failure is cleared on success.
In `@src/openhuman/integrations/mod.rs`:
- Around line 58-78: The tests calling build_client should not depend on on-disk
auth state; modify the three tests
(build_client_returns_none_when_no_auth_token, build_client_uses_core_api_key,
build_client_rejects_whitespace_only_api_key) to isolate the auth-store by
either directing the JWT/session lookup to an empty temporary location before
calling build_client (e.g., set the auth/profile path env or create a temp dir
and configure the auth store to it) or stub out
crate::api::jwt::get_session_token(config) to return None for the duration of
the test, ensuring build_client only reads the supplied Config.api_key/api_url
and making the assertions deterministic.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 349-357: The workflow step with id config-overrides is passing
UPDATER_ENDPOINT unconditionally which causes staging builds to inherit a
production updater URL; update the step so UPDATER_ENDPOINT is set conditionally
based on build_target (or unset for staging) or set WITH_UPDATER to "false" for
non-production builds before calling prepareTauriConfig.js; reference the env
names UPDATER_ENDPOINT, WITH_UPDATER and BASE_URL and the build_target decision
logic used in prepare-build, or alternatively ensure prepareTauriConfig.js
receives no UPDATER_ENDPOINT so its internal fallback is used.
In `@src/openhuman/memory/embeddings.rs`:
- Around line 242-267: The code currently swallows embed errors/panics; update
the failure branches inside the FastembedState handling and the task join error
to emit structured logs before returning Err so embed_one(...).await.ok()
callers see failures in logs. Specifically, in the match arm handling Ok(Err(e))
(fastembed embed failed), the Err(panic_payload) branch (fastembed embed
panicked via extract_panic_message), the FastembedState::Failed(message) arm,
FastembedState::Uninitialized arm, and the join_result.map_err branch, call the
project logger (use the same logger used elsewhere in this module) to log a
structured record containing stable fields model (the model identifier available
in FastembedState::Ready or message/context for Failed), batch_len
(items.len()), and the error/panic message; then return the same Err as before.
Also ensure embed_one's callers that use .ok() will at least have these logs to
surface indexing failures.
---
Nitpick comments:
In `@app/src/pages/__tests__/Skills.composio-catalog.test.tsx`:
- Around line 20-29: The mock for the hook useComposioIntegrations in the test
currently returns an extra disabled property; remove the extraneous disabled
field from the mocked return object so the mock matches the actual hook shape
(toolkits, connectionByToolkit, refresh, loading, error) and update the vi.mock
block that defines useComposioIntegrations accordingly.
In `@src/openhuman/agent/agents/skills_agent/prompt.md`:
- Line 27: The prompt text uses British spelling "re-authorise" while the
function names use American spelling `composio_authorize`; standardize the
spelling across the prompt and code references by replacing "re-authorise" with
"re-authorize" (or vice versa if you prefer British) so the prompt matches the
function name `composio_authorize` and any other occurrences, ensuring
consistent terminology in `prompt.md` and references to `composio_authorize`.
- Line 19: Update the phrasing in prompt.md where it instructs calling
composio_list_tools: replace "optionally scoped to one or two toolkits" with a
clearer, non-arbitrary phrase such as "optionally filtered by toolkit" or
"optionally scoped to specific toolkits" so the guidance about toolkit scoping
for composio_list_tools is unambiguous.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a79c44b-8a74-4fe2-8517-f6631c471dec
📒 Files selected for processing (20)
.github/workflows/release.ymlapp/src/components/composio/toolkitMeta.tsapp/src/lib/composio/composioApi.tsapp/src/lib/composio/hooks.test.tsapp/src/lib/composio/hooks.tsapp/src/pages/Skills.tsxapp/src/pages/__tests__/Skills.composio-catalog.test.tsxsrc/openhuman/agent/agents/skills_agent/agent.tomlsrc/openhuman/agent/agents/skills_agent/prompt.mdsrc/openhuman/agent/harness/subagent_runner.rssrc/openhuman/channels/runtime/dispatch.rssrc/openhuman/composio/client.rssrc/openhuman/composio/ops.rssrc/openhuman/composio/tools.rssrc/openhuman/config/schema/tools.rssrc/openhuman/integrations/client.rssrc/openhuman/integrations/mod.rssrc/openhuman/memory/embeddings.rssrc/openhuman/tools/impl/network/composio.rssrc/openhuman/tools/ops.rs
💤 Files with no reviewable changes (1)
- src/openhuman/channels/runtime/dispatch.rs
Comment on lines
+796
to
800
| needs: [prepare-build, create-release, build-desktop, build-docker] | ||
| if: >- | ||
| always() | ||
| && needs.prepare-build.outputs.release_enabled == 'true' | ||
| && needs.create-release.result == 'success' |
Contributor
There was a problem hiding this comment.
Include create-release failures in cleanup.
prepare-build has already pushed the tag before this job is evaluated, but this cleanup path only runs when create-release succeeded and a later build failed. If draft release creation fails, both build jobs are skipped and the pushed tag is left on origin, so the next retry immediately trips Ensure tag does not already exist.
Suggested fix
- if: >-
- always()
- && needs.prepare-build.outputs.release_enabled == 'true'
- && needs.create-release.result == 'success'
- && (needs.build-desktop.result == 'failure' || needs.build-desktop.result == 'cancelled'
- || needs.build-docker.result == 'failure' || needs.build-docker.result == 'cancelled')
+ if: >-
+ always()
+ && needs.prepare-build.outputs.release_enabled == 'true'
+ && (
+ needs.create-release.result == 'failure' || needs.create-release.result == 'cancelled'
+ || needs.build-desktop.result == 'failure' || needs.build-desktop.result == 'cancelled'
+ || needs.build-docker.result == 'failure' || needs.build-docker.result == 'cancelled'
+ )- if (!Number.isFinite(releaseId) || releaseId <= 0) {
- core.setFailed('Invalid or missing release_id; cannot delete release.');
- return;
- }
+ if (!Number.isFinite(releaseId) || releaseId <= 0) {
+ core.info('No draft release to delete.');
+ return;
+ }Also applies to: 804-815
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 796 - 800, The cleanup job's if
condition only runs when needs.create-release.result == 'success', so tags
pushed by prepare-build remain on origin if create-release failed; update the if
expression in the job that lists needs: [prepare-build, create-release,
build-desktop, build-docker] to include create-release failures (e.g. change
needs.create-release.result == 'success' to (needs.create-release.result ==
'success' || needs.create-release.result == 'failure')), and apply the same
change to the other identical if block later (the block spanning the other
804-815 region) so cleanup runs when create-release fails as well.
Comment on lines
89
to
100
| const id = window.setInterval(() => { | ||
| if (disabled) return; | ||
| void listConnections() | ||
| .then(resp => { | ||
| if (!mountedRef.current) return; | ||
| setConnections(resp.connections ?? []); | ||
| }) | ||
| .catch(() => { | ||
| /* swallow — non-fatal for poll cadence */ | ||
| .catch(err => { | ||
| console.warn( | ||
| '[composio] polling connections failed:', | ||
| err instanceof Error ? err.message : String(err) | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Clear the connection error when polling recovers.
If the initial refresh() hits a listConnections() failure, error is set. A later successful poll updates connections, but this path never clears that old error, so the UI can keep showing a Composio failure after the backend has recovered. Tracking toolkit/connection errors separately, or clearing the connection-side error on successful polls, would avoid the stale banner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/composio/hooks.ts` around lines 89 - 100, On successful polling
in the interval handler (the promise chain that calls listConnections and then
setConnections), clear the stale connection error so the UI doesn't keep showing
a recovered failure; specifically, after setConnections(resp.connections ?? [])
(and guarded by mountedRef.current), call the same state updater used for the
connection error (e.g., setError(null) or setConnectionError(null)) to reset the
error state so a prior listConnections() failure is cleared on success.
Comment on lines
+58
to
78
| fn build_client_returns_none_when_no_auth_token() { | ||
| let mut config = crate::openhuman::config::Config::default(); | ||
| config.api_key = None; | ||
| assert!(build_client(&config).is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_client_returns_none_when_url_missing() { | ||
| let config = crate::openhuman::config::IntegrationsConfig { | ||
| enabled: true, | ||
| backend_url: None, | ||
| auth_token: Some("tok".into()), | ||
| ..Default::default() | ||
| }; | ||
| assert!(build_client(&config).is_none()); | ||
| fn build_client_uses_core_api_key() { | ||
| // No per-integration config exists any more — the client is | ||
| // built solely from the core `config.api_key` / `config.api_url`. | ||
| let mut config = crate::openhuman::config::Config::default(); | ||
| config.api_key = Some("root-token".into()); | ||
| config.api_url = Some("https://api.example.test".into()); | ||
| assert!(build_client(&config).is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn build_client_rejects_whitespace_only_values() { | ||
| let config = crate::openhuman::config::IntegrationsConfig { | ||
| enabled: true, | ||
| backend_url: Some(" ".into()), | ||
| auth_token: Some("tok".into()), | ||
| ..Default::default() | ||
| }; | ||
| fn build_client_rejects_whitespace_only_api_key() { | ||
| let mut config = crate::openhuman::config::Config::default(); | ||
| config.api_key = Some(" ".into()); | ||
| assert!(build_client(&config).is_none()); |
Contributor
There was a problem hiding this comment.
Isolate auth-store state in these build_client tests.
build_client() now checks crate::api::jwt::get_session_token(config) before config.api_key, so these cases can start returning Some(_) whenever the test machine already has an app-session profile on disk. As written, the assertions depend on ambient local state instead of only the config under test. Point the auth store/profile to an empty temp location, or stub the session-token lookup, so these tests stay deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/integrations/mod.rs` around lines 58 - 78, The tests calling
build_client should not depend on on-disk auth state; modify the three tests
(build_client_returns_none_when_no_auth_token, build_client_uses_core_api_key,
build_client_rejects_whitespace_only_api_key) to isolate the auth-store by
either directing the JWT/session lookup to an empty temporary location before
calling build_client (e.g., set the auth/profile path env or create a temp dir
and configure the auth store to it) or stub out
crate::api::jwt::get_session_token(config) to return None for the duration of
the test, ensuring build_client only reads the supplied Config.api_key/api_url
and making the assertions deterministic.
- Refactored tests for the Skills page to integrate Gmail and Notion as composio tools, enhancing the testing framework. - Removed mock data and streamlined the test setup to reflect the current state of available skills. - Updated assertions to verify the rendering of connected and disconnected states for Gmail and Notion integrations, respectively. - Improved clarity and maintainability of test cases by consolidating mock implementations and removing redundant code.
…rtions - Modified the Skills component to assign categories dynamically based on toolkit metadata, improving organization of displayed tools. - Updated test cases to reflect the new categorization, ensuring accurate rendering of tools under their respective categories instead of a generic 'Other' group. - Enhanced assertions in tests to verify the presence of specific tools and their categories, improving test coverage and reliability.
- Simplified the item creation logic in the Skills component by removing unnecessary line breaks, enhancing code readability without altering functionality. - This change contributes to cleaner code structure and maintainability in the Skills page.
…ved clarity - Updated the integration tests for Gmail and Notion on the Skills page to utilize the `within` function for more precise querying of elements within their respective sections. - Improved test assertions to ensure that the connected and disconnected states are accurately verified, enhancing the reliability of the tests. - Streamlined the test setup to better reflect the current structure of the Skills component, contributing to overall test maintainability.
Contributor
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)
app/src/pages/Skills.tsx (1)
195-199:⚠️ Potential issue | 🟠 MajorDon’t collapse Composio fetch failures into a “Connect” state.
useComposioIntegrations()exposes anerror, but the page ignores it and derives every Composio card state only fromconnectionByToolkit. That makes a failed/stale connection load indistinguishable from a genuinely disconnected toolkit, so users can be shown a misleadingConnectCTA after a backend failure. Please surface the hook error as a distinct page/card state instead of treating it as "not connected".Also applies to: 466-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Skills.tsx` around lines 195 - 199, The Composio UI currently derives toolkit state solely from connectionByToolkit and treats any missing connection as "Connect"; instead, update the components using useComposioIntegrations() (references: useComposioIntegrations, composioToolkits, composioConnectionByToolkit, refreshComposio) to read the hook's exposed error and render a separate error/stale state for the page and each toolkit card when error is present rather than falling back to "not connected"; specifically, branch on hook.error before checking connectionByToolkit, surface the error message/cta (e.g., "Retry" using refreshComposio) in the card and page-level UI, and apply the same change to the other usage block around lines 466-491 so backend failures are not treated as a disconnected toolkit.
🧹 Nitpick comments (1)
app/src/pages/Skills.tsx (1)
195-199: Add debug logs around the Composio catalog fallback path.This code now reconciles live toolkits, known catalog entries, and connection state, but there’s no
[skills][composio]logging for the hook result or the fallback branch. A dev-only log for toolkit count, connection count, error presence, and whenKNOWN_COMPOSIO_TOOLKITSis filling gaps would make this flow much easier to diagnose.As per coding guidelines "Add substantial, development-oriented logs while implementing features or fixes so issues are easy to trace end-to-end; log critical checkpoints including entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths."
Also applies to: 263-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Skills.tsx` around lines 195 - 199, Add dev-only [skills][composio] debug logs around the useComposioIntegrations() result and the fallback that uses KNOWN_COMPOSIO_TOOLKITS: log toolkit count (composioToolkits.length), connection count (Object.keys(composioConnectionByToolkit).length), any error presence from the hook, and a clear message when you are filling gaps from KNOWN_COMPOSIO_TOOLKITS; place logs at the hook result/decision point and inside the fallback branch that merges known toolkits, and ensure they are gated by a development flag so they don’t run in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/pages/Skills.tsx`:
- Around line 195-199: The Composio UI currently derives toolkit state solely
from connectionByToolkit and treats any missing connection as "Connect";
instead, update the components using useComposioIntegrations() (references:
useComposioIntegrations, composioToolkits, composioConnectionByToolkit,
refreshComposio) to read the hook's exposed error and render a separate
error/stale state for the page and each toolkit card when error is present
rather than falling back to "not connected"; specifically, branch on hook.error
before checking connectionByToolkit, surface the error message/cta (e.g.,
"Retry" using refreshComposio) in the card and page-level UI, and apply the same
change to the other usage block around lines 466-491 so backend failures are not
treated as a disconnected toolkit.
---
Nitpick comments:
In `@app/src/pages/Skills.tsx`:
- Around line 195-199: Add dev-only [skills][composio] debug logs around the
useComposioIntegrations() result and the fallback that uses
KNOWN_COMPOSIO_TOOLKITS: log toolkit count (composioToolkits.length), connection
count (Object.keys(composioConnectionByToolkit).length), any error presence from
the hook, and a clear message when you are filling gaps from
KNOWN_COMPOSIO_TOOLKITS; place logs at the hook result/decision point and inside
the fallback branch that merges known toolkits, and ensure they are gated by a
development flag so they don’t run in production.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a9c964f-8dbb-40cb-b8b0-00537fdd0602
📒 Files selected for processing (4)
app/src/pages/Skills.tsxapp/src/pages/__tests__/Skills.composio-catalog.test.tsxapp/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsxapp/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/tests/Skills.composio-catalog.test.tsx
- Added error handling for Composio integrations in the Skills component, displaying a user-friendly message when integration status is stale or an error occurs. - Implemented logging in development mode to provide insights into the state of Composio toolkits and connections, aiding in debugging. - Updated the item rendering logic to reflect the error state, ensuring users can retry fetching integrations when an error is detected. - Enhanced tests to verify the display of error messages and the functionality of the retry mechanism, improving overall test coverage and reliability.
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Solution
Submission Checklist
app/) and/orcargo test(core) for logic you add or changeapp/test/e2e, mock backend,tests/json_rpc_e2e.rsas appropriate)//////!(Rust), JSDoc or brief file/module headers (TS) on public APIs and non-obvious modulesPre-push checks run locally:
format:check,lint,tsc --noEmit, andcargo check --manifest-path app/src-tauri/Cargo.toml.No unit, Rust integration, or app E2E suites were run in this pass.
Impact
Related
Summary by CodeRabbit
New Features
Improvements
Changes
Tests / Docs