refactor(composio): extract derive_toolkit_slug helper#3035
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:
📝 WalkthroughWalkthroughCentralizes toolkit-slug derivation into a private ChangesToolkit Slug Extraction
EnvVarGuard safety docs
AgentProgress test updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/inference_local_admin_raw_coverage_e2e.rs (1)
52-64: ⚡ Quick winConsider enforcing the lock precondition at compile time.
The safety comments document that callers must hold
ENV_VAR_LOCK, but this precondition isn't enforced. A future test could callEnvVarGuard::setor::unsetwithout acquiring the lock first, violating the safety contract for theunsafeenv mutations.🔒 Proposed fix to enforce the lock at compile time
impl EnvVarGuard { - fn set(key: &'static str, value: impl AsRef<std::ffi::OsStr>) -> Self { + fn set(key: &'static str, value: impl AsRef<std::ffi::OsStr>, _lock: &tokio::sync::MutexGuard<()>) -> Self { let previous = std::env::var_os(key); // SAFETY: tests that mutate environment variables hold ENV_VAR_LOCK. unsafe { std::env::set_var(key, value) }; Self { key, previous } } - fn unset(key: &'static str) -> Self { + fn unset(key: &'static str, _lock: &tokio::sync::MutexGuard<()>) -> Self { let previous = std::env::var_os(key); // SAFETY: tests that mutate environment variables hold ENV_VAR_LOCK. unsafe { std::env::remove_var(key) }; Self { key, previous } }Then update call sites to pass the lock reference:
let _env_lock = ENV_VAR_LOCK.lock().await; // ... - let _path = EnvVarGuard::set("PATH", scripts.path()); + let _path = EnvVarGuard::set("PATH", scripts.path(), &_env_lock);🤖 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 `@tests/inference_local_admin_raw_coverage_e2e.rs` around lines 52 - 64, EnvVarGuard::set and EnvVarGuard::unset perform unsafe environment mutations but only document that callers must hold ENV_VAR_LOCK; enforce this at compile time by requiring a reference/token representing the acquired lock (e.g. an &EnvVarLock or EnvVarLockGuard parameter) in both function signatures instead of calling them with no proof, update the implementations to use that parameter and remove the explanatory SAFETY comment, and update all call sites that currently call EnvVarGuard::set/unset to pass the lock guard (the ENV_VAR_LOCK acquisition site should now produce the guard value that callers forward).
🤖 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 `@tests/inference_local_admin_raw_coverage_e2e.rs`:
- Around line 52-64: EnvVarGuard::set and EnvVarGuard::unset perform unsafe
environment mutations but only document that callers must hold ENV_VAR_LOCK;
enforce this at compile time by requiring a reference/token representing the
acquired lock (e.g. an &EnvVarLock or EnvVarLockGuard parameter) in both
function signatures instead of calling them with no proof, update the
implementations to use that parameter and remove the explanatory SAFETY comment,
and update all call sites that currently call EnvVarGuard::set/unset to pass the
lock guard (the ENV_VAR_LOCK acquisition site should now produce the guard value
that callers forward).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28a3d9b4-0689-4529-9b65-d667563fb050
📒 Files selected for processing (1)
tests/inference_local_admin_raw_coverage_e2e.rs
…oolkit-slug # Conflicts: # tests/inference_local_admin_raw_coverage_e2e.rs
…oolkit-slug # Conflicts: # src/openhuman/threads/turn_state/mirror_tests.rs # tests/memory_core_threads_raw_coverage_e2e.rs # tests/memory_threads_raw_coverage_e2e.rs
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
The core refactor is clean: derive_toolkit_slug lifts the duplicated split('_').next().unwrap_or("integration").to_ascii_lowercase() expression verbatim, the doc-comment correctly explains the unreachable "integration" fallback, and the three unit tests (multi-segment, single-segment, empty-string parity) lock the behavior down. Nice touch covering the empty-string Some("") edge that the unwrap_or does not catch.
I checked the riskier bundled changes and they hold up:
generate_id()switching from a UUID string to an opaquer+a-p encoding is safe -- rule ids are treated as opaque strings everywhere (only ever prefixed withrule/as a storage key, never parsed back as a UUID), so no consumer breaks. Still 128 bits of entropy, and it's covered by a test.- The
meet_agentfile.flush()is a correct durability fix -- tokio'sFiledoesn't guarantee a flush on drop. - The
thread_stack_sizebump and thearia-pressedaddition are additive and low-risk.
One thing worth fixing for the record, not blocking: the description frames this as a "pure refactor with zero behavior change," but the PR actually bundles several real production changes that aren't called out in the Changes list -- the server runtime stack-size bump (src/core/cli.rs), the rule-id generation format (memory_tools/types.rs), and the meet-agent flush (meet_agent/store.rs). They're each defensible, but they aren't behavior-neutral and they're unrelated to the toolkit-slug refactor. Please update the description so the production changes are visible to reviewers and the changelog, and ideally split unrelated production fixes into their own PRs next time -- it keeps the diff reviewable and the history bisectable.
Everything is correct and CI is fully green, so approving. Thanks for also stabilizing the flaky env-mutating coverage tests with the shared env_lock -- that's a real improvement.
Change summary:
| Area | Change | Assessment |
|---|---|---|
| composio/error_mapping | extract derive_toolkit_slug + tests |
clean, the stated refactor |
| core/cli.rs | 8 MiB worker thread stack | safe, undisclosed in description |
| memory_tools/types.rs | opaque rule-id format | safe, ids are opaque |
| meet_agent/store.rs | flush after write | correct durability fix |
| frontend (ToolsPanel) | aria-pressed + tests |
good accessibility win |
| tests (~15 files) | env serialization, fixtures, pagination | stabilization, fine |
| let rt = tokio::runtime::Builder::new_multi_thread() | ||
| .enable_all() | ||
| // Match the desktop host runtime stack: sub-agent prompt/tool | ||
| // orchestration can exceed Tokio's default 2 MiB worker stack. |
There was a problem hiding this comment.
[minor] This raises the server runtime's per-worker stack to 8 MiB, which is a real production change (and a sensible one) -- but it isn't mentioned in the PR's Changes list, which describes the PR as a zero-behavior-change refactor. Please call it out in the description so it's visible to reviewers and the changelog.
2 participants
Summary
Follow-up to the CodeRabbit nitpick on PR #3009 (Composio trigger-permission copy). The toolkit-slug extraction logic was duplicated across two user-facing message builders in
src/openhuman/composio/error_mapping.rs:This is a pure refactor with zero behavior change: it extracts that expression into a shared
derive_toolkit_slug(tool: &str) -> Stringhelper.Changes
derive_toolkit_slugreplicating the original logic verbatim (split('_').next().unwrap_or("integration").to_ascii_lowercase()), with a doc-comment noting the"integration"fallback is unreachable for&strinput but kept for parity.format_insufficient_scope_messageformat_trigger_permission_messageupstream/mainto clear the dirty merge state and pick up the current local-admin env-lock coverage fix.display_name: Nonefield, fixing Rust Core Coverage compilation after the upstream sync.object-errormodel-listing flake undercargo llvm-covparallel test execution.cargo llvm-covparallel test execution.toolkit_from_slugbehavior forMICROSOFT_TEAMS_*->microsoft_teams.Tests
Added unit tests in
error_mapping_tests.rs:GMAIL_NEW_GMAIL_MESSAGE->gmailSLACK->slack""->""- behavior-parity guard verifying"".split('_').next()yieldsSome("")(so theunwrap_orfallback does NOT apply), exactly as beforeValidation:
GGML_NATIVE=OFF cargo test -p openhuman --test inference_local_admin_raw_coverage_e2e -- --test-threads=4- 4 passedGGML_NATIVE=OFF cargo test -p openhuman composio::error_mapping- 12 passedGGML_NATIVE=OFF cargo test -p openhuman --lib subagent_lifecycle_records_and_clears_active- 1 passedGGML_NATIVE=OFF cargo test -p openhuman --test memory_core_threads_raw_coverage_e2e --test memory_threads_raw_coverage_e2e --no-run- compiled both test targetsGGML_NATIVE=OFF cargo test -p openhuman --test inference_provider_admin_round22_raw_coverage_e2e -- --test-threads=4- 5 passedGGML_NATIVE=OFF cargo test -p openhuman --test inference_voice_http_round23_raw_coverage_e2e -- --test-threads=4- 3 passedGGML_NATIVE=OFF cargo test -p openhuman --test memory_raw_coverage_e2e memory_sources_validation_and_sync_classification_edges -- --exact- 1 passedGGML_NATIVE=OFF cargo test -p openhuman --test memory_threads_raw_coverage_e2e memory_sync_composio_catalog_scope_and_state_helpers_cover_edge_cases -- --exact- 1 passedGGML_NATIVE=OFF cargo test -p openhuman --test memory_tree_sync_raw_coverage_e2e composio_providers_sync_state_and_bus_surfaces_cover_read_write_edges -- --exact- 1 passedGGML_NATIVE=OFF cargo test -p openhuman --lib toolkit_from_slug_handles_known_multi_segment_toolkits- 1 passedcargo fmt --manifest-path Cargo.toml --all --check- passedgit diff --check- passedpnpm compile- passed afterpnpm install --frozen-lockfilerefreshed local deps from the synced lockfileNot run locally:
cargo llvm-cov -p openhuman --test inference_voice_http_round23_raw_coverage_e2e --lcov --output-path /tmp/openhuman-inference-voice-round23.lcov- localcargo-llvm-covis not installed; CI Rust Core Coverage remains the source of truth.Notes
e49e41637d5bfcaef385660b46f8edcf13a64f0d.Summary by CodeRabbit
Refactor
Tests