From 6226ed46e9030e7ab56a5794b5de4e885c9220bb Mon Sep 17 00:00:00 2001 From: Srinivas Vaddi <38348871+vaddisrinivas@users.noreply.github.com> Date: Sat, 23 May 2026 11:44:37 -0400 Subject: [PATCH 1/7] Add generated tool admission checks --- src/openhuman/tools/generated.rs | 252 +++++++++++++++++++++++++++++++ 1 file changed, 252 insertions(+) diff --git a/src/openhuman/tools/generated.rs b/src/openhuman/tools/generated.rs index c654fbe1f0..b17bd35689 100644 --- a/src/openhuman/tools/generated.rs +++ b/src/openhuman/tools/generated.rs @@ -4,9 +4,11 @@ //! expose generated capability tools without adding a bespoke Rust type //! for each tool and without handing the model a broad raw bridge. +use std::collections::BTreeSet; use std::sync::Arc; use async_trait::async_trait; +use serde::{Deserialize, Serialize}; use serde_json::Value; use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolCategory, ToolResult, ToolScope}; @@ -20,6 +22,11 @@ pub struct GeneratedToolDefinition { pub category: ToolCategory, pub scope: ToolScope, pub adapter_id: String, + pub provider_id: Option, + pub capability_id: Option, + pub source_digest: Option, + pub risk: Option, + pub policy_surface: Option, } impl GeneratedToolDefinition { @@ -37,10 +44,52 @@ impl GeneratedToolDefinition { category: ToolCategory::Skill, scope: ToolScope::All, adapter_id: adapter_id.into(), + provider_id: None, + capability_id: None, + source_digest: None, + risk: None, + policy_surface: None, } } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum GeneratedToolRisk { + Read, + Write, + ExternalWrite, + Execute, + Dangerous, +} + +impl GeneratedToolRisk { + fn is_external_effect(self) -> bool { + matches!(self, Self::ExternalWrite | Self::Dangerous) + } +} + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct GeneratedToolAdmissionConfig { + pub enforce_provenance: bool, + pub trusted_providers: BTreeSet, + pub disabled_providers: BTreeSet, + pub disabled_capabilities: BTreeSet, + pub existing_tool_names: BTreeSet, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct GeneratedToolAdmissionRejection { + pub tool_name: String, + pub reason: String, +} + +#[derive(Debug, Clone)] +pub struct GeneratedToolAdmissionReport { + pub admitted: Vec, + pub rejected: Vec, +} + #[async_trait] pub trait GeneratedToolAdapter: Send + Sync { fn id(&self) -> &str; @@ -124,6 +173,13 @@ impl Tool for GeneratedTool { fn category(&self) -> ToolCategory { self.definition.category } + + fn external_effect(&self) -> bool { + self.definition + .risk + .map(GeneratedToolRisk::is_external_effect) + .unwrap_or(false) + } } pub fn generated_tools_from_definitions( @@ -139,10 +195,34 @@ pub fn generated_tools_from_definitions( .collect() } +pub fn admit_generated_tool_definitions( + definitions: Vec, + config: &GeneratedToolAdmissionConfig, +) -> GeneratedToolAdmissionReport { + let mut seen = config.existing_tool_names.clone(); + let mut admitted = Vec::new(); + let mut rejected = Vec::new(); + + for mut definition in definitions { + normalize_definition(&mut definition); + let tool_name = definition.name.clone(); + match validate_admission(&definition, config, &mut seen) { + Ok(()) => admitted.push(definition), + Err(reason) => rejected.push(GeneratedToolAdmissionRejection { tool_name, reason }), + } + } + + GeneratedToolAdmissionReport { admitted, rejected } +} + fn normalize_definition(definition: &mut GeneratedToolDefinition) { definition.name = definition.name.trim().to_string(); definition.description = definition.description.trim().to_string(); definition.adapter_id = definition.adapter_id.trim().to_string(); + definition.provider_id = trim_option(definition.provider_id.take()); + definition.capability_id = trim_option(definition.capability_id.take()); + definition.source_digest = trim_option(definition.source_digest.take()); + definition.policy_surface = trim_option(definition.policy_surface.take()); } fn validate_definition(definition: &GeneratedToolDefinition) -> anyhow::Result<()> { @@ -161,6 +241,85 @@ fn validate_definition(definition: &GeneratedToolDefinition) -> anyhow::Result<( Ok(()) } +fn validate_admission( + definition: &GeneratedToolDefinition, + config: &GeneratedToolAdmissionConfig, + seen: &mut BTreeSet, +) -> Result<(), String> { + validate_definition(definition).map_err(|err| err.to_string())?; + if !is_safe_generated_tool_name(&definition.name) { + return Err(format!( + "generated tool `{}` name contains unsupported characters", + definition.name + )); + } + if !seen.insert(definition.name.clone()) { + return Err(format!("duplicate generated tool `{}`", definition.name)); + } + if !config.enforce_provenance { + return Ok(()); + } + + let provider_id = definition + .provider_id + .as_deref() + .ok_or_else(|| format!("generated tool `{}` missing provider_id", definition.name))?; + if config.disabled_providers.contains(provider_id) { + return Err(format!( + "generated tool `{}` provider `{provider_id}` is disabled", + definition.name + )); + } + if !config.trusted_providers.contains(provider_id) { + return Err(format!( + "generated tool `{}` provider `{provider_id}` is not trusted", + definition.name + )); + } + + let capability_id = definition + .capability_id + .as_deref() + .ok_or_else(|| format!("generated tool `{}` missing capability_id", definition.name))?; + if config.disabled_capabilities.contains(capability_id) { + return Err(format!( + "generated tool `{}` capability `{capability_id}` is disabled", + definition.name + )); + } + + if definition.risk.is_none() { + return Err(format!( + "generated tool `{}` missing risk metadata", + definition.name + )); + } + if definition.source_digest.is_none() { + return Err(format!( + "generated tool `{}` missing source_digest", + definition.name + )); + } + + Ok(()) +} + +fn trim_option(value: Option) -> Option { + value + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) +} + +fn is_safe_generated_tool_name(name: &str) -> bool { + let trimmed = name.trim(); + !trimmed.is_empty() + && !trimmed.starts_with(['.', '-', '_']) + && !trimmed.ends_with(['.', '-', '_']) + && trimmed.chars().all(|ch| { + ch.is_ascii_lowercase() || ch.is_ascii_digit() || matches!(ch, '.' | '-' | '_') + }) +} + #[cfg(test)] mod tests { use super::*; @@ -204,9 +363,21 @@ mod tests { "echo-adapter", ); definition.permission_level = PermissionLevel::Write; + definition.provider_id = Some("trusted.runtime".into()); + definition.capability_id = Some("updates.send".into()); + definition.source_digest = Some("sha256:abc".into()); + definition.risk = Some(GeneratedToolRisk::ExternalWrite); definition } + fn admission_config() -> GeneratedToolAdmissionConfig { + GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]), + ..Default::default() + } + } + #[tokio::test] async fn generated_tool_executes_through_adapter() { let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); @@ -268,5 +439,86 @@ mod tests { assert_eq!(tool.name(), "send_update"); assert_eq!(tool.description(), "Send a scoped update."); assert_eq!(tool.definition().adapter_id, "echo-adapter"); + assert_eq!( + tool.definition().provider_id.as_deref(), + Some("trusted.runtime") + ); + } + + #[test] + fn admission_allows_trusted_generated_tool() { + let report = + admit_generated_tool_definitions(vec![sample_definition()], &admission_config()); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); + } + + #[test] + fn admission_disabled_preserves_legacy_generated_tools() { + let mut definition = sample_definition(); + definition.provider_id = None; + definition.capability_id = None; + definition.source_digest = None; + definition.risk = None; + + let report = admit_generated_tool_definitions( + vec![definition], + &GeneratedToolAdmissionConfig::default(), + ); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); + } + + #[test] + fn admission_rejects_untrusted_provider() { + let mut definition = sample_definition(); + definition.provider_id = Some("other.runtime".into()); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("not trusted")); + } + + #[test] + fn admission_rejects_duplicate_tool_names() { + let report = admit_generated_tool_definitions( + vec![sample_definition(), sample_definition()], + &admission_config(), + ); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected[0].reason.contains("duplicate")); + } + + #[test] + fn admission_rejects_missing_risk_when_enforced() { + let mut definition = sample_definition(); + definition.risk = None; + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("missing risk")); + } + + #[test] + fn admission_rejects_unsafe_names() { + let mut definition = sample_definition(); + definition.name = "Bad Tool".into(); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("unsupported characters")); + } + + #[tokio::test] + async fn generated_tool_marks_external_risk_as_external_effect() { + let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); + + assert!(tool.external_effect()); } } From 21a7bf30c94d6616a90c1c4408dad9c797161a98 Mon Sep 17 00:00:00 2001 From: Srinivas Vaddi <38348871+vaddisrinivas@users.noreply.github.com> Date: Sat, 23 May 2026 20:14:12 -0400 Subject: [PATCH 2/7] Address generated tool admission feedback --- src/openhuman/tools/generated.rs | 66 ++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 3 deletions(-) diff --git a/src/openhuman/tools/generated.rs b/src/openhuman/tools/generated.rs index b17bd35689..82d81e8975 100644 --- a/src/openhuman/tools/generated.rs +++ b/src/openhuman/tools/generated.rs @@ -15,21 +15,34 @@ use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolCategory, ToolR #[derive(Debug, Clone)] pub struct GeneratedToolDefinition { + /// Stable tool name exposed to the model. pub name: String, + /// Human-readable tool description. pub description: String, + /// JSON schema for tool arguments. pub parameters_schema: Value, + /// Permission required to execute this tool. pub permission_level: PermissionLevel, + /// Tool category used for agent/tool scoping. pub category: ToolCategory, + /// Execution surface where the tool is available. pub scope: ToolScope, + /// Adapter responsible for executing the generated tool. pub adapter_id: String, + /// Provider that produced this generated tool. pub provider_id: Option, + /// Provider-scoped capability id for policy and revocation. pub capability_id: Option, + /// Digest of the source capability definition. pub source_digest: Option, + /// Declared runtime risk for policy and approval. pub risk: Option, + /// Optional policy namespace/surface label. pub policy_surface: Option, } impl GeneratedToolDefinition { + /// Build a generated tool definition with legacy-safe defaults. pub fn new( name: impl Into, description: impl Into, @@ -56,44 +69,60 @@ impl GeneratedToolDefinition { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum GeneratedToolRisk { + /// Read-only capability. Read, + /// Local or internal write capability. Write, + /// Externally observable write capability. ExternalWrite, + /// Code or command execution capability. Execute, + /// High-risk or destructive capability. Dangerous, } impl GeneratedToolRisk { fn is_external_effect(self) -> bool { - matches!(self, Self::ExternalWrite | Self::Dangerous) + matches!(self, Self::ExternalWrite | Self::Execute | Self::Dangerous) } } #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct GeneratedToolAdmissionConfig { + /// Whether provenance fields are required for admission. pub enforce_provenance: bool, + /// Provider ids allowed to register generated tools. pub trusted_providers: BTreeSet, + /// Provider ids blocked from registration. pub disabled_providers: BTreeSet, + /// Capability ids blocked from registration. pub disabled_capabilities: BTreeSet, + /// Existing tool names reserved before this admission pass. pub existing_tool_names: BTreeSet, } #[derive(Debug, Clone, PartialEq, Eq)] pub struct GeneratedToolAdmissionRejection { + /// Tool name rejected during admission. pub tool_name: String, + /// Human-readable rejection reason. pub reason: String, } #[derive(Debug, Clone)] pub struct GeneratedToolAdmissionReport { + /// Definitions accepted for registration. pub admitted: Vec, + /// Definitions rejected before registration. pub rejected: Vec, } #[async_trait] pub trait GeneratedToolAdapter: Send + Sync { + /// Stable adapter id matched against generated definitions. fn id(&self) -> &str; + /// Execute a generated tool definition with validated arguments. async fn execute( &self, definition: &GeneratedToolDefinition, @@ -101,12 +130,14 @@ pub trait GeneratedToolAdapter: Send + Sync { ) -> anyhow::Result; } +/// Executable wrapper around a generated tool definition and adapter. pub struct GeneratedTool { definition: GeneratedToolDefinition, adapter: Arc, } impl GeneratedTool { + /// Create a generated tool wrapper after validation. pub fn new( mut definition: GeneratedToolDefinition, adapter: Arc, @@ -139,6 +170,7 @@ impl GeneratedTool { }) } + /// Borrow the normalized generated tool definition. pub fn definition(&self) -> &GeneratedToolDefinition { &self.definition } @@ -182,6 +214,7 @@ impl Tool for GeneratedTool { } } +/// Convert generated definitions into boxed tool trait objects. pub fn generated_tools_from_definitions( definitions: Vec, adapter: Arc, @@ -195,6 +228,7 @@ pub fn generated_tools_from_definitions( .collect() } +/// Admit generated tool definitions according to provenance policy. pub fn admit_generated_tool_definitions( definitions: Vec, config: &GeneratedToolAdmissionConfig, @@ -207,8 +241,25 @@ pub fn admit_generated_tool_definitions( normalize_definition(&mut definition); let tool_name = definition.name.clone(); match validate_admission(&definition, config, &mut seen) { - Ok(()) => admitted.push(definition), - Err(reason) => rejected.push(GeneratedToolAdmissionRejection { tool_name, reason }), + Ok(()) => { + log::debug!( + "[generated_tools] admission accepted tool_name={} provider_id={:?} capability_id={:?}", + definition.name, + definition.provider_id, + definition.capability_id + ); + admitted.push(definition); + } + Err(reason) => { + log::debug!( + "[generated_tools] admission rejected tool_name={} provider_id={:?} capability_id={:?} reason={}", + tool_name, + definition.provider_id, + definition.capability_id, + reason + ); + rejected.push(GeneratedToolAdmissionRejection { tool_name, reason }); + } } } @@ -521,4 +572,13 @@ mod tests { assert!(tool.external_effect()); } + + #[tokio::test] + async fn generated_tool_marks_execute_risk_as_external_effect() { + let mut definition = sample_definition(); + definition.risk = Some(GeneratedToolRisk::Execute); + let tool = GeneratedTool::new(definition, Arc::new(EchoAdapter)).unwrap(); + + assert!(tool.external_effect()); + } } From 46631365d44929e2eef03935020cdcc5690abca6 Mon Sep 17 00:00:00 2001 From: Srinivas Vaddi <38348871+vaddisrinivas@users.noreply.github.com> Date: Sun, 24 May 2026 12:06:06 -0400 Subject: [PATCH 3/7] Normalize generated tool provider admission --- src/openhuman/tools/generated.rs | 87 ++++++++++++++++++++++++++++++-- 1 file changed, 84 insertions(+), 3 deletions(-) diff --git a/src/openhuman/tools/generated.rs b/src/openhuman/tools/generated.rs index 82d81e8975..0daeabf5ed 100644 --- a/src/openhuman/tools/generated.rs +++ b/src/openhuman/tools/generated.rs @@ -92,8 +92,14 @@ pub struct GeneratedToolAdmissionConfig { /// Whether provenance fields are required for admission. pub enforce_provenance: bool, /// Provider ids allowed to register generated tools. + /// + /// Values are normalized with the same provider-id rules used for + /// generated tool definitions before admission checks run. pub trusted_providers: BTreeSet, /// Provider ids blocked from registration. + /// + /// Values are normalized with the same provider-id rules used for + /// generated tool definitions before admission checks run. pub disabled_providers: BTreeSet, /// Capability ids blocked from registration. pub disabled_capabilities: BTreeSet, @@ -270,7 +276,7 @@ fn normalize_definition(definition: &mut GeneratedToolDefinition) { definition.name = definition.name.trim().to_string(); definition.description = definition.description.trim().to_string(); definition.adapter_id = definition.adapter_id.trim().to_string(); - definition.provider_id = trim_option(definition.provider_id.take()); + definition.provider_id = normalize_optional_provider_id(definition.provider_id.take()); definition.capability_id = trim_option(definition.capability_id.take()); definition.source_digest = trim_option(definition.source_digest.take()); definition.policy_surface = trim_option(definition.policy_surface.take()); @@ -315,13 +321,19 @@ fn validate_admission( .provider_id .as_deref() .ok_or_else(|| format!("generated tool `{}` missing provider_id", definition.name))?; - if config.disabled_providers.contains(provider_id) { + if normalize_provider_id(provider_id).is_none() { + return Err(format!( + "generated tool `{}` has invalid provider_id `{provider_id}`", + definition.name + )); + } + if normalize_provider_set(&config.disabled_providers).contains(provider_id) { return Err(format!( "generated tool `{}` provider `{provider_id}` is disabled", definition.name )); } - if !config.trusted_providers.contains(provider_id) { + if !normalize_provider_set(&config.trusted_providers).contains(provider_id) { return Err(format!( "generated tool `{}` provider `{provider_id}` is not trusted", definition.name @@ -361,6 +373,44 @@ fn trim_option(value: Option) -> Option { .filter(|value| !value.is_empty()) } +fn normalize_optional_provider_id(value: Option) -> Option { + trim_option(value).map(|value| normalize_provider_id(&value).unwrap_or(value)) +} + +fn normalize_provider_set(values: &BTreeSet) -> BTreeSet { + values + .iter() + .filter_map(|value| normalize_provider_id(value)) + .collect() +} + +fn normalize_provider_id(value: &str) -> Option { + let normalized = value.trim().to_ascii_lowercase(); + if normalized.is_empty() { + return None; + } + let valid = normalized + .chars() + .all(|ch| ch.is_ascii_lowercase() || ch.is_ascii_digit() || matches!(ch, '-' | '_' | '.')); + if !valid { + return None; + } + let starts_or_ends_with_sep = normalized + .chars() + .next() + .zip(normalized.chars().last()) + .map(|(first, last)| is_provider_separator(first) || is_provider_separator(last)) + .unwrap_or(true); + if starts_or_ends_with_sep { + return None; + } + Some(normalized) +} + +fn is_provider_separator(ch: char) -> bool { + matches!(ch, '-' | '_' | '.') +} + fn is_safe_generated_tool_name(name: &str) -> bool { let trimmed = name.trim(); !trimmed.is_empty() @@ -505,6 +555,37 @@ mod tests { assert!(report.rejected.is_empty()); } + #[test] + fn admission_normalizes_provider_ids_before_policy_checks() { + let mut definition = sample_definition(); + definition.provider_id = Some(" Trusted.Runtime ".into()); + let config = GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["TRUSTED.RUNTIME".to_string()]), + ..Default::default() + }; + + let report = admit_generated_tool_definitions(vec![definition], &config); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); + assert_eq!( + report.admitted[0].provider_id.as_deref(), + Some("trusted.runtime") + ); + } + + #[test] + fn admission_rejects_invalid_provider_ids_when_enforced() { + let mut definition = sample_definition(); + definition.provider_id = Some("bad/provider".into()); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("invalid provider_id")); + } + #[test] fn admission_disabled_preserves_legacy_generated_tools() { let mut definition = sample_definition(); From 9b72e8cfe447974f8840a5deb68fce723b1e3cdf Mon Sep 17 00:00:00 2001 From: Srinivas Vaddi <38348871+vaddisrinivas@users.noreply.github.com> Date: Mon, 25 May 2026 09:11:04 -0400 Subject: [PATCH 4/7] Wait for backend session in mega flow --- app/test/e2e/specs/mega-flow.spec.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/app/test/e2e/specs/mega-flow.spec.ts b/app/test/e2e/specs/mega-flow.spec.ts index 00f806e6fb..5fcebfb7b4 100644 --- a/app/test/e2e/specs/mega-flow.spec.ts +++ b/app/test/e2e/specs/mega-flow.spec.ts @@ -70,6 +70,20 @@ async function waitForMockRequest( return undefined; } +async function waitForBackendSession(label: string, timeoutMs = 15_000): Promise { + const deadline = Date.now() + timeoutMs; + let lastProbe: unknown = undefined; + + while (Date.now() < deadline) { + const probe = await callOpenhumanRpc('openhuman.composio_list_triggers', {}); + if (probe.ok) return; + lastProbe = probe; + await browser.pause(500); + } + + throw new Error(`${LOG} ${label}: backend session not ready: ${JSON.stringify(lastProbe)}`); +} + async function resetEverything(label: string): Promise { console.log(`${LOG} reset (${label}) — admin reset only (skip destructive core reset)`); // Mock-side reset is enough to give each scenario a clean slate for the @@ -237,6 +251,7 @@ describe('Mega flow — login + Gmail OAuth + Composio in one session', () => { // Re-login since reset wipes the session. await triggerDeepLink('openhuman://auth?token=mega-composio-token'); await waitForMockRequest('POST', '/telegram/login-tokens/', 15_000); + await waitForBackendSession('Scenario 4 auth'); // Seed connections + available triggers; start with an empty active list. setMockBehaviors({ @@ -248,6 +263,9 @@ describe('Mega flow — login + Gmail OAuth + Composio in one session', () => { }); const before = await callOpenhumanRpc('openhuman.composio_list_triggers', {}); + if (!before.ok) { + console.log(`${LOG} composio: list before enable failed`, before); + } expect(before.ok).toBe(true); // list_triggers always emits a log line → RpcOutcome wraps in {result, logs}. // JSON-RPC result shape: { result: { triggers: [...] }, logs: [...] } @@ -262,9 +280,15 @@ describe('Mega flow — login + Gmail OAuth + Composio in one session', () => { connection_id: 'c1', slug: 'GMAIL_NEW_GMAIL_MESSAGE', }); + if (!enable.ok) { + console.log(`${LOG} composio: enable failed`, enable); + } expect(enable.ok).toBe(true); const after = await callOpenhumanRpc('openhuman.composio_list_triggers', {}); + if (!after.ok) { + console.log(`${LOG} composio: list after enable failed`, after); + } expect(after.ok).toBe(true); const afterList = (after.result?.result?.triggers ?? after.result?.triggers ?? []) as unknown[]; expect(afterList.length).toBeGreaterThan(0); @@ -561,6 +585,7 @@ describe('Mega flow — login + Gmail OAuth + Composio in one session', () => { await triggerDeepLink('openhuman://auth?token=mega-composio-webhook-token'); await waitForMockRequest('POST', '/telegram/login-tokens/', 15_000); + await waitForBackendSession('Scenario 11 auth'); clearRequestLog(); // Seed composio state. @@ -577,6 +602,9 @@ describe('Mega flow — login + Gmail OAuth + Composio in one session', () => { connection_id: 'c2', slug: 'GITHUB_PULL_REQUEST_EVENT', }); + if (!enable.ok) { + console.log(`${LOG} composio+webhook: enable failed`, enable); + } expect(enable.ok).toBe(true); console.log(`${LOG} composio+webhook: trigger enabled`); From 684213715f5684c1c3d2a2cd25520f8cb69fc20d Mon Sep 17 00:00:00 2001 From: Srinivas Vaddi <38348871+vaddisrinivas@users.noreply.github.com> Date: Tue, 26 May 2026 19:23:36 -0400 Subject: [PATCH 5/7] test(e2e): remove unused backend session helper --- app/test/e2e/specs/mega-flow.spec.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/app/test/e2e/specs/mega-flow.spec.ts b/app/test/e2e/specs/mega-flow.spec.ts index 66ff55f1b7..bf6a3c01a1 100644 --- a/app/test/e2e/specs/mega-flow.spec.ts +++ b/app/test/e2e/specs/mega-flow.spec.ts @@ -77,20 +77,6 @@ async function waitForMockRequest( return undefined; } -async function waitForBackendSession(label: string, timeoutMs = 15_000): Promise { - const deadline = Date.now() + timeoutMs; - let lastProbe: unknown = undefined; - - while (Date.now() < deadline) { - const probe = await callOpenhumanRpc('openhuman.composio_list_triggers', {}); - if (probe.ok) return; - lastProbe = probe; - await browser.pause(500); - } - - throw new Error(`${LOG} ${label}: backend session not ready: ${JSON.stringify(lastProbe)}`); -} - async function resetEverything(label: string): Promise { console.log(`${LOG} reset (${label}) — admin reset only (skip destructive core reset)`); // Mock-side reset is enough to give each scenario a clean slate for the From bfc4ea8c46357e6887abc4ec66628bdd9eb977a8 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 28 May 2026 17:13:19 +0200 Subject: [PATCH 6/7] refactor(tools): address CodeRabbit nitpicks on generated-tool admission - Log dropped invalid provider IDs in `normalize_provider_set` so config with only invalid entries no longer silently rejects every provider as "not trusted". The helper now takes a `field` label (trusted vs disabled) so the debug line names which set the bad value came from. - Move the 240-line inline test module out to a sibling `generated_tests.rs` wired with `#[cfg(test)] #[path = ...] mod tests;`, matching the local convention used by `ops_tests.rs` / `schema_tests.rs`. Drops `generated.rs` back under the ~500-line guideline. Both items are CodeRabbit nitpicks from the May 24 review on PR #2549. Co-Authored-By: Claude --- src/openhuman/tools/generated.rs | 262 ++----------------------- src/openhuman/tools/generated_tests.rs | 237 ++++++++++++++++++++++ 2 files changed, 254 insertions(+), 245 deletions(-) create mode 100644 src/openhuman/tools/generated_tests.rs diff --git a/src/openhuman/tools/generated.rs b/src/openhuman/tools/generated.rs index 0daeabf5ed..d79067732f 100644 --- a/src/openhuman/tools/generated.rs +++ b/src/openhuman/tools/generated.rs @@ -327,13 +327,16 @@ fn validate_admission( definition.name )); } - if normalize_provider_set(&config.disabled_providers).contains(provider_id) { + if normalize_provider_set(&config.disabled_providers, "disabled_providers") + .contains(provider_id) + { return Err(format!( "generated tool `{}` provider `{provider_id}` is disabled", definition.name )); } - if !normalize_provider_set(&config.trusted_providers).contains(provider_id) { + if !normalize_provider_set(&config.trusted_providers, "trusted_providers").contains(provider_id) + { return Err(format!( "generated tool `{}` provider `{provider_id}` is not trusted", definition.name @@ -377,10 +380,18 @@ fn normalize_optional_provider_id(value: Option) -> Option { trim_option(value).map(|value| normalize_provider_id(&value).unwrap_or(value)) } -fn normalize_provider_set(values: &BTreeSet) -> BTreeSet { +fn normalize_provider_set(values: &BTreeSet, field: &str) -> BTreeSet { values .iter() - .filter_map(|value| normalize_provider_id(value)) + .filter_map(|value| { + let normalized = normalize_provider_id(value); + if normalized.is_none() { + log::debug!( + "[generated_tools] dropped invalid provider_id from config field={field} value={value}" + ); + } + normalized + }) .collect() } @@ -422,244 +433,5 @@ fn is_safe_generated_tool_name(name: &str) -> bool { } #[cfg(test)] -mod tests { - use super::*; - use serde_json::json; - - struct EchoAdapter; - - #[async_trait] - impl GeneratedToolAdapter for EchoAdapter { - fn id(&self) -> &str { - "echo-adapter" - } - - async fn execute( - &self, - definition: &GeneratedToolDefinition, - args: Value, - ) -> anyhow::Result { - Ok(ToolResult::success( - json!({ - "tool": definition.name, - "adapter": definition.adapter_id, - "args": args, - }) - .to_string(), - )) - } - } - - fn sample_definition() -> GeneratedToolDefinition { - let mut definition = GeneratedToolDefinition::new( - "send_update", - "Send a scoped update through a trusted adapter.", - json!({ - "type": "object", - "properties": { - "message": { "type": "string" } - }, - "required": ["message"] - }), - "echo-adapter", - ); - definition.permission_level = PermissionLevel::Write; - definition.provider_id = Some("trusted.runtime".into()); - definition.capability_id = Some("updates.send".into()); - definition.source_digest = Some("sha256:abc".into()); - definition.risk = Some(GeneratedToolRisk::ExternalWrite); - definition - } - - fn admission_config() -> GeneratedToolAdmissionConfig { - GeneratedToolAdmissionConfig { - enforce_provenance: true, - trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]), - ..Default::default() - } - } - - #[tokio::test] - async fn generated_tool_executes_through_adapter() { - let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); - - let result = tool - .execute(json!({ "message": "hello" })) - .await - .expect("execute"); - - assert_eq!(tool.name(), "send_update"); - assert_eq!(tool.permission_level(), PermissionLevel::Write); - assert_eq!(tool.category(), ToolCategory::Skill); - assert!(result.output().contains("send_update")); - assert!(result.output().contains("hello")); - } - - #[test] - fn generated_tools_from_definitions_returns_tool_objects() { - let tools = - generated_tools_from_definitions(vec![sample_definition()], Arc::new(EchoAdapter)) - .unwrap(); - - assert_eq!(tools.len(), 1); - assert_eq!(tools[0].name(), "send_update"); - assert_eq!(tools[0].parameters_schema()["type"], json!("object")); - } - - #[test] - fn generated_tool_rejects_adapter_mismatch() { - let mut definition = sample_definition(); - definition.adapter_id = "missing-adapter".into(); - - match GeneratedTool::new(definition, Arc::new(EchoAdapter)) { - Ok(_) => panic!("adapter mismatch should fail"), - Err(err) => assert!(err.to_string().contains("requires adapter")), - } - } - - #[test] - fn generated_tool_rejects_blank_adapter_id() { - let mut definition = sample_definition(); - definition.adapter_id = " ".into(); - - match GeneratedTool::new(definition, Arc::new(EchoAdapter)) { - Ok(_) => panic!("blank adapter_id should fail"), - Err(err) => assert!(err.to_string().contains("adapter_id must be non-empty")), - } - } - - #[test] - fn generated_tool_normalizes_definition_fields() { - let mut definition = sample_definition(); - definition.name = " send_update ".into(); - definition.description = " Send a scoped update. ".into(); - definition.adapter_id = " echo-adapter ".into(); - - let tool = GeneratedTool::new(definition, Arc::new(EchoAdapter)).unwrap(); - - assert_eq!(tool.name(), "send_update"); - assert_eq!(tool.description(), "Send a scoped update."); - assert_eq!(tool.definition().adapter_id, "echo-adapter"); - assert_eq!( - tool.definition().provider_id.as_deref(), - Some("trusted.runtime") - ); - } - - #[test] - fn admission_allows_trusted_generated_tool() { - let report = - admit_generated_tool_definitions(vec![sample_definition()], &admission_config()); - - assert_eq!(report.admitted.len(), 1); - assert!(report.rejected.is_empty()); - } - - #[test] - fn admission_normalizes_provider_ids_before_policy_checks() { - let mut definition = sample_definition(); - definition.provider_id = Some(" Trusted.Runtime ".into()); - let config = GeneratedToolAdmissionConfig { - enforce_provenance: true, - trusted_providers: BTreeSet::from(["TRUSTED.RUNTIME".to_string()]), - ..Default::default() - }; - - let report = admit_generated_tool_definitions(vec![definition], &config); - - assert_eq!(report.admitted.len(), 1); - assert!(report.rejected.is_empty()); - assert_eq!( - report.admitted[0].provider_id.as_deref(), - Some("trusted.runtime") - ); - } - - #[test] - fn admission_rejects_invalid_provider_ids_when_enforced() { - let mut definition = sample_definition(); - definition.provider_id = Some("bad/provider".into()); - - let report = admit_generated_tool_definitions(vec![definition], &admission_config()); - - assert!(report.admitted.is_empty()); - assert!(report.rejected[0].reason.contains("invalid provider_id")); - } - - #[test] - fn admission_disabled_preserves_legacy_generated_tools() { - let mut definition = sample_definition(); - definition.provider_id = None; - definition.capability_id = None; - definition.source_digest = None; - definition.risk = None; - - let report = admit_generated_tool_definitions( - vec![definition], - &GeneratedToolAdmissionConfig::default(), - ); - - assert_eq!(report.admitted.len(), 1); - assert!(report.rejected.is_empty()); - } - - #[test] - fn admission_rejects_untrusted_provider() { - let mut definition = sample_definition(); - definition.provider_id = Some("other.runtime".into()); - - let report = admit_generated_tool_definitions(vec![definition], &admission_config()); - - assert!(report.admitted.is_empty()); - assert!(report.rejected[0].reason.contains("not trusted")); - } - - #[test] - fn admission_rejects_duplicate_tool_names() { - let report = admit_generated_tool_definitions( - vec![sample_definition(), sample_definition()], - &admission_config(), - ); - - assert_eq!(report.admitted.len(), 1); - assert!(report.rejected[0].reason.contains("duplicate")); - } - - #[test] - fn admission_rejects_missing_risk_when_enforced() { - let mut definition = sample_definition(); - definition.risk = None; - - let report = admit_generated_tool_definitions(vec![definition], &admission_config()); - - assert!(report.admitted.is_empty()); - assert!(report.rejected[0].reason.contains("missing risk")); - } - - #[test] - fn admission_rejects_unsafe_names() { - let mut definition = sample_definition(); - definition.name = "Bad Tool".into(); - - let report = admit_generated_tool_definitions(vec![definition], &admission_config()); - - assert!(report.admitted.is_empty()); - assert!(report.rejected[0].reason.contains("unsupported characters")); - } - - #[tokio::test] - async fn generated_tool_marks_external_risk_as_external_effect() { - let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); - - assert!(tool.external_effect()); - } - - #[tokio::test] - async fn generated_tool_marks_execute_risk_as_external_effect() { - let mut definition = sample_definition(); - definition.risk = Some(GeneratedToolRisk::Execute); - let tool = GeneratedTool::new(definition, Arc::new(EchoAdapter)).unwrap(); - - assert!(tool.external_effect()); - } -} +#[path = "generated_tests.rs"] +mod tests; diff --git a/src/openhuman/tools/generated_tests.rs b/src/openhuman/tools/generated_tests.rs new file mode 100644 index 0000000000..09c02f6bae --- /dev/null +++ b/src/openhuman/tools/generated_tests.rs @@ -0,0 +1,237 @@ +use super::*; +use serde_json::json; + +struct EchoAdapter; + +#[async_trait] +impl GeneratedToolAdapter for EchoAdapter { + fn id(&self) -> &str { + "echo-adapter" + } + + async fn execute( + &self, + definition: &GeneratedToolDefinition, + args: Value, + ) -> anyhow::Result { + Ok(ToolResult::success( + json!({ + "tool": definition.name, + "adapter": definition.adapter_id, + "args": args, + }) + .to_string(), + )) + } +} + +fn sample_definition() -> GeneratedToolDefinition { + let mut definition = GeneratedToolDefinition::new( + "send_update", + "Send a scoped update through a trusted adapter.", + json!({ + "type": "object", + "properties": { + "message": { "type": "string" } + }, + "required": ["message"] + }), + "echo-adapter", + ); + definition.permission_level = PermissionLevel::Write; + definition.provider_id = Some("trusted.runtime".into()); + definition.capability_id = Some("updates.send".into()); + definition.source_digest = Some("sha256:abc".into()); + definition.risk = Some(GeneratedToolRisk::ExternalWrite); + definition +} + +fn admission_config() -> GeneratedToolAdmissionConfig { + GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]), + ..Default::default() + } +} + +#[tokio::test] +async fn generated_tool_executes_through_adapter() { + let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); + + let result = tool + .execute(json!({ "message": "hello" })) + .await + .expect("execute"); + + assert_eq!(tool.name(), "send_update"); + assert_eq!(tool.permission_level(), PermissionLevel::Write); + assert_eq!(tool.category(), ToolCategory::Skill); + assert!(result.output().contains("send_update")); + assert!(result.output().contains("hello")); +} + +#[test] +fn generated_tools_from_definitions_returns_tool_objects() { + let tools = + generated_tools_from_definitions(vec![sample_definition()], Arc::new(EchoAdapter)).unwrap(); + + assert_eq!(tools.len(), 1); + assert_eq!(tools[0].name(), "send_update"); + assert_eq!(tools[0].parameters_schema()["type"], json!("object")); +} + +#[test] +fn generated_tool_rejects_adapter_mismatch() { + let mut definition = sample_definition(); + definition.adapter_id = "missing-adapter".into(); + + match GeneratedTool::new(definition, Arc::new(EchoAdapter)) { + Ok(_) => panic!("adapter mismatch should fail"), + Err(err) => assert!(err.to_string().contains("requires adapter")), + } +} + +#[test] +fn generated_tool_rejects_blank_adapter_id() { + let mut definition = sample_definition(); + definition.adapter_id = " ".into(); + + match GeneratedTool::new(definition, Arc::new(EchoAdapter)) { + Ok(_) => panic!("blank adapter_id should fail"), + Err(err) => assert!(err.to_string().contains("adapter_id must be non-empty")), + } +} + +#[test] +fn generated_tool_normalizes_definition_fields() { + let mut definition = sample_definition(); + definition.name = " send_update ".into(); + definition.description = " Send a scoped update. ".into(); + definition.adapter_id = " echo-adapter ".into(); + + let tool = GeneratedTool::new(definition, Arc::new(EchoAdapter)).unwrap(); + + assert_eq!(tool.name(), "send_update"); + assert_eq!(tool.description(), "Send a scoped update."); + assert_eq!(tool.definition().adapter_id, "echo-adapter"); + assert_eq!( + tool.definition().provider_id.as_deref(), + Some("trusted.runtime") + ); +} + +#[test] +fn admission_allows_trusted_generated_tool() { + let report = admit_generated_tool_definitions(vec![sample_definition()], &admission_config()); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); +} + +#[test] +fn admission_normalizes_provider_ids_before_policy_checks() { + let mut definition = sample_definition(); + definition.provider_id = Some(" Trusted.Runtime ".into()); + let config = GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["TRUSTED.RUNTIME".to_string()]), + ..Default::default() + }; + + let report = admit_generated_tool_definitions(vec![definition], &config); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); + assert_eq!( + report.admitted[0].provider_id.as_deref(), + Some("trusted.runtime") + ); +} + +#[test] +fn admission_rejects_invalid_provider_ids_when_enforced() { + let mut definition = sample_definition(); + definition.provider_id = Some("bad/provider".into()); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("invalid provider_id")); +} + +#[test] +fn admission_disabled_preserves_legacy_generated_tools() { + let mut definition = sample_definition(); + definition.provider_id = None; + definition.capability_id = None; + definition.source_digest = None; + definition.risk = None; + + let report = admit_generated_tool_definitions( + vec![definition], + &GeneratedToolAdmissionConfig::default(), + ); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected.is_empty()); +} + +#[test] +fn admission_rejects_untrusted_provider() { + let mut definition = sample_definition(); + definition.provider_id = Some("other.runtime".into()); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("not trusted")); +} + +#[test] +fn admission_rejects_duplicate_tool_names() { + let report = admit_generated_tool_definitions( + vec![sample_definition(), sample_definition()], + &admission_config(), + ); + + assert_eq!(report.admitted.len(), 1); + assert!(report.rejected[0].reason.contains("duplicate")); +} + +#[test] +fn admission_rejects_missing_risk_when_enforced() { + let mut definition = sample_definition(); + definition.risk = None; + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("missing risk")); +} + +#[test] +fn admission_rejects_unsafe_names() { + let mut definition = sample_definition(); + definition.name = "Bad Tool".into(); + + let report = admit_generated_tool_definitions(vec![definition], &admission_config()); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("unsupported characters")); +} + +#[tokio::test] +async fn generated_tool_marks_external_risk_as_external_effect() { + let tool = GeneratedTool::new(sample_definition(), Arc::new(EchoAdapter)).unwrap(); + + assert!(tool.external_effect()); +} + +#[tokio::test] +async fn generated_tool_marks_execute_risk_as_external_effect() { + let mut definition = sample_definition(); + definition.risk = Some(GeneratedToolRisk::Execute); + let tool = GeneratedTool::new(definition, Arc::new(EchoAdapter)).unwrap(); + + assert!(tool.external_effect()); +} From f311cad5950db46cd60f510a69b2333fddc324c9 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 28 May 2026 20:41:32 +0200 Subject: [PATCH 7/7] fix(tool_registry): add disabled-provider + disabled-capability admission tests + pre-normalize provider sets Addresses @graycyrus's CHANGES_REQUESTED review on #2549: - admission_rejects_disabled_provider test (was missing per #2542 ACs) - admission_rejects_disabled_capabilities test (companion) - Pre-normalize trusted_providers + disabled_providers once before the admission loop instead of per-tool Co-Authored-By: Claude --- src/openhuman/tools/generated.rs | 24 +++++++++++++++----- src/openhuman/tools/generated_tests.rs | 31 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/openhuman/tools/generated.rs b/src/openhuman/tools/generated.rs index d79067732f..3ed1b1d9bc 100644 --- a/src/openhuman/tools/generated.rs +++ b/src/openhuman/tools/generated.rs @@ -243,10 +243,23 @@ pub fn admit_generated_tool_definitions( let mut admitted = Vec::new(); let mut rejected = Vec::new(); + // Pre-normalize provider allow/deny sets once before the admission loop + // so we do not redo the O(N) normalization work per tool. + let normalized_disabled_providers = + normalize_provider_set(&config.disabled_providers, "disabled_providers"); + let normalized_trusted_providers = + normalize_provider_set(&config.trusted_providers, "trusted_providers"); + for mut definition in definitions { normalize_definition(&mut definition); let tool_name = definition.name.clone(); - match validate_admission(&definition, config, &mut seen) { + match validate_admission( + &definition, + config, + &normalized_disabled_providers, + &normalized_trusted_providers, + &mut seen, + ) { Ok(()) => { log::debug!( "[generated_tools] admission accepted tool_name={} provider_id={:?} capability_id={:?}", @@ -301,6 +314,8 @@ fn validate_definition(definition: &GeneratedToolDefinition) -> anyhow::Result<( fn validate_admission( definition: &GeneratedToolDefinition, config: &GeneratedToolAdmissionConfig, + normalized_disabled_providers: &BTreeSet, + normalized_trusted_providers: &BTreeSet, seen: &mut BTreeSet, ) -> Result<(), String> { validate_definition(definition).map_err(|err| err.to_string())?; @@ -327,16 +342,13 @@ fn validate_admission( definition.name )); } - if normalize_provider_set(&config.disabled_providers, "disabled_providers") - .contains(provider_id) - { + if normalized_disabled_providers.contains(provider_id) { return Err(format!( "generated tool `{}` provider `{provider_id}` is disabled", definition.name )); } - if !normalize_provider_set(&config.trusted_providers, "trusted_providers").contains(provider_id) - { + if !normalized_trusted_providers.contains(provider_id) { return Err(format!( "generated tool `{}` provider `{provider_id}` is not trusted", definition.name diff --git a/src/openhuman/tools/generated_tests.rs b/src/openhuman/tools/generated_tests.rs index 09c02f6bae..9c93a7d430 100644 --- a/src/openhuman/tools/generated_tests.rs +++ b/src/openhuman/tools/generated_tests.rs @@ -187,6 +187,37 @@ fn admission_rejects_untrusted_provider() { assert!(report.rejected[0].reason.contains("not trusted")); } +#[test] +fn admission_rejects_disabled_provider() { + let mut definition = sample_definition(); + let config = GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]), + disabled_providers: BTreeSet::from(["trusted.runtime".to_string()]), + ..Default::default() + }; + definition.provider_id = Some("trusted.runtime".into()); + let report = admit_generated_tool_definitions(vec![definition], &config); + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("disabled")); +} + +#[test] +fn admission_rejects_disabled_capabilities() { + let definition = sample_definition(); + let config = GeneratedToolAdmissionConfig { + enforce_provenance: true, + trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]), + disabled_capabilities: BTreeSet::from(["updates.send".to_string()]), + ..Default::default() + }; + + let report = admit_generated_tool_definitions(vec![definition], &config); + + assert!(report.admitted.is_empty()); + assert!(report.rejected[0].reason.contains("disabled")); +} + #[test] fn admission_rejects_duplicate_tool_names() { let report = admit_generated_tool_definitions(