From 0399bf4d7ed599da23dc00c1847b52ff7a859dc3 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Thu, 11 Jun 2026 21:55:14 +0300 Subject: [PATCH 1/6] fix(reborn): steer routine delivery through outbound targets --- .../src/first_party_tools/schemas.rs | 2 +- .../first_party_tools/trigger_management.rs | 4 ++- .../tests/tool_surface_contract.rs | 25 ++++++++++++++++ .../assets/prompts/default-system.md | 5 ++++ .../runtime/local_dev/outbound_delivery.rs | 2 +- .../src/runtime/local_dev/tests.rs | 29 +++++++++++++++---- .../runtime/tests/default_system_prompt.rs | 12 ++++++++ docs/reborn/contracts/triggers.md | 9 ++++-- 8 files changed, 78 insertions(+), 10 deletions(-) diff --git a/crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs b/crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs index 16f666bba55..ce22180185b 100644 --- a/crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs +++ b/crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs @@ -376,7 +376,7 @@ pub(crate) fn resolve_builtin_input_schema_ref(reference: &str) -> Option }, "prompt": { "type": "string", - "description": "Prompt submitted when the trigger fires. Runtime validation caps UTF-8 content at 32768 bytes." + "description": "Prompt submitted when the trigger fires. Runtime validation caps UTF-8 content at 32768 bytes. Do not embed delivery routing here; when the user asks to send routine or trigger results through an outbound product/channel, first select the target through the visible outbound delivery target capabilities, then create the trigger." }, "cron": { "type": "string", "description": "Five-, six-, or seven-field cron expression; fire cadence must be at least one minute" }, "timezone": { "type": "string", "description": "IANA timezone name for cron evaluation (e.g. 'America/New_York', 'Europe/London', 'UTC'). The cron expression is evaluated in this timezone; fire times are stored and compared in UTC. If the user's timezone is already known from the conversation or their settings, use it without asking; if unknown, ask the user before creating the trigger. Never silently assume UTC — a trigger that fires at the wrong local time is worse than no trigger." } diff --git a/crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs b/crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs index 3a52b2808ea..c01ca1179aa 100644 --- a/crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs +++ b/crates/ironclaw_host_runtime/src/first_party_tools/trigger_management.rs @@ -32,11 +32,13 @@ pub const TRIGGER_CREATE_CAPABILITY_ID: &str = "builtin.trigger_create"; pub const TRIGGER_LIST_CAPABILITY_ID: &str = "builtin.trigger_list"; pub const TRIGGER_REMOVE_CAPABILITY_ID: &str = "builtin.trigger_remove"; +const TRIGGER_CREATE_DESCRIPTION: &str = "Create a caller-scoped scheduled trigger. If the user asks for routine or trigger results to be sent through an outbound product or channel, use the visible outbound delivery target capabilities to select that delivery target before creating the trigger; delivery routing is not encoded in this input."; + pub(super) fn manifests() -> Result, ExtensionError> { Ok(vec![ first_party_capability_manifest( TRIGGER_CREATE_CAPABILITY_ID, - "Create a caller-scoped scheduled trigger", + TRIGGER_CREATE_DESCRIPTION, vec![EffectKind::DispatchCapability, EffectKind::ExternalWrite], PermissionMode::Ask, resource_profile(), diff --git a/crates/ironclaw_host_runtime/tests/tool_surface_contract.rs b/crates/ironclaw_host_runtime/tests/tool_surface_contract.rs index 3103900c68d..9461e1a2ac9 100644 --- a/crates/ironclaw_host_runtime/tests/tool_surface_contract.rs +++ b/crates/ironclaw_host_runtime/tests/tool_surface_contract.rs @@ -543,6 +543,31 @@ async fn visible_surface_resolves_builtin_first_party_input_schema_refs() { assert_schema_has_property(&surface, "builtin.skill_install", "url"); assert_schema_has_property(&surface, "builtin.skill_install", "name"); + let trigger_create = surface + .capabilities + .iter() + .find(|capability| capability.descriptor.id == capability_id("builtin.trigger_create")) + .expect("builtin.trigger_create should be visible"); + assert!( + trigger_create + .descriptor + .description + .contains("outbound delivery target capabilities"), + "trigger_create description should point the model at delivery target selection" + ); + let trigger_prompt_description = trigger_create + .descriptor + .parameters_schema + .get("properties") + .and_then(|properties| properties.get("prompt")) + .and_then(|property| property.get("description")) + .and_then(serde_json::Value::as_str) + .expect("trigger prompt description should be present"); + assert!( + trigger_prompt_description.contains("first select the target"), + "trigger_create prompt schema should steer delivery requests before trigger creation" + ); + let http_schema = &surface .capabilities .iter() diff --git a/crates/ironclaw_reborn_composition/assets/prompts/default-system.md b/crates/ironclaw_reborn_composition/assets/prompts/default-system.md index 7d464a9fb91..8df69be1c32 100644 --- a/crates/ironclaw_reborn_composition/assets/prompts/default-system.md +++ b/crates/ironclaw_reborn_composition/assets/prompts/default-system.md @@ -10,6 +10,11 @@ You are IronClaw Agent, a secure autonomous assistant. When a tool result is partial, truncated, failed, or otherwise shows the requested work is unfinished, adapt and continue autonomously. Ask the user only when progress requires external information, approval, or a product decision. +## Delivery Targets + +- When visible outbound delivery target tools exist and the user asks to send final replies, routine results, or trigger results through a product or channel such as Slack, call `builtin__outbound_delivery_targets_list` first, then call `builtin__outbound_delivery_target_set` with a returned `target_id` before creating the routine or trigger. +- Do not say a delivery product is unavailable, and do not ask the user to reconnect it, until you have listed available outbound delivery targets and found none. + ## Safety - You have no independent goals. Do not pursue self-preservation, replication, resource acquisition, or power-seeking beyond the user's request. diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs index 286c059e6f4..8d5745ceda4 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs @@ -36,7 +36,7 @@ pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID: &str = "builtin.outbound_delivery_target_set"; const OUTBOUND_DELIVERY_TARGET_SET_PROVIDER_TOOL_NAME: &str = "builtin__outbound_delivery_target_set"; -const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply outbound delivery target, such as a Slack DM or Slack channel. Approval may be required before the preference is changed."; +const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply outbound delivery target, such as a Slack DM or Slack channel, to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger. Approval may be required before the preference is changed."; pub(super) fn outbound_delivery_capabilities( facade: Arc, diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs index bd1e5d37b64..5a25e97047c 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs @@ -1305,14 +1305,33 @@ mod tests { .collect::>(); assert!(descriptor_ids.contains(&OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID)); assert!(descriptor_ids.contains(&OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID)); - let tool_definition_names = port - .tool_definitions() - .expect("tool definitions") - .into_iter() - .map(|definition| definition.name) + let tool_definitions = port.tool_definitions().expect("tool definitions"); + let tool_definition_names = tool_definitions + .iter() + .map(|definition| definition.name.clone()) .collect::>(); assert!(tool_definition_names.contains(&"builtin__outbound_delivery_targets_list".into())); assert!(tool_definition_names.contains(&"builtin__outbound_delivery_target_set".into())); + let list_tool = tool_definitions + .iter() + .find(|definition| definition.name == "builtin__outbound_delivery_targets_list") + .expect("list tool definition should exist"); + assert!( + list_tool + .description + .contains("before builtin.trigger_create"), + "list tool description should steer delivery requests before trigger creation" + ); + let set_tool = tool_definitions + .iter() + .find(|definition| definition.name == "builtin__outbound_delivery_target_set") + .expect("set tool definition should exist"); + assert!( + set_tool + .description + .contains("before creating the routine or trigger"), + "set tool description should steer delivery requests before trigger creation" + ); let malformed_list = port .register_provider_tool_call(provider_tool_call_with_name( diff --git a/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs b/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs index 0bc9c7a765b..d733eb3ec92 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs @@ -73,6 +73,18 @@ async fn local_dev_runtime_injects_default_system_prompt_into_model_request() { }), "local-dev runtime should send the editable default system prompt to the model gateway" ); + assert!( + recorded_requests[0].messages.iter().any(|message| { + message.role == HostManagedModelMessageRole::System + && message + .content + .contains("builtin__outbound_delivery_targets_list") + && message + .content + .contains("before creating the routine or trigger") + }), + "local-dev runtime should tell the model how to select outbound delivery targets before triggers" + ); assert!( recorded_requests[0].messages.iter().any(|message| { message.role == HostManagedModelMessageRole::User && message.content == "ping" diff --git a/docs/reborn/contracts/triggers.md b/docs/reborn/contracts/triggers.md index 8a28d35bd3e..c82be597253 100644 --- a/docs/reborn/contracts/triggers.md +++ b/docs/reborn/contracts/triggers.md @@ -420,8 +420,13 @@ Capability follow-ups before launch: Trigger delivery target selection is outside trigger identity and goes through the outbound delivery track. Product-facing outbound preference APIs and explicit provider-backed target tooling own discovery and approval-gated -selection; durable selection remains product-owned and trigger records still do -not embed delivery targets. +selection. Local-dev Reborn exposes model-visible outbound target +discovery/selection capabilities that write the caller's final-reply +preference. When a user asks to send routine or trigger results through a +delivery product/channel, model-visible trigger surfaces must steer the model to +discover and select an outbound delivery target before calling +`trigger_create`; durable selection remains product-owned and trigger records +still do not embed delivery targets. - Trigger ingress identity must not include delivery targets. - Trigger record identity must not include delivery targets. From 67303e9132ddde364495b858d5cd530fa067499c Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Sun, 14 Jun 2026 00:07:14 +0300 Subject: [PATCH 2/6] fix(reborn): address outbound delivery review feedback (#4780) --- .../assets/prompts/default-system.md | 5 - .../src/outbound_preferences.rs | 101 +++++- .../src/runtime.rs | 97 ++++-- .../runtime/local_dev/outbound_delivery.rs | 2 +- .../src/runtime/local_dev/tests.rs | 2 +- .../runtime/tests/default_system_prompt.rs | 12 - .../src/runtime/tests/outbound_delivery.rs | 293 ++++++++++++++++++ .../src/slack_connectable_channel.rs | 10 +- .../src/slack_host_beta.rs | 24 +- .../ironclaw_reborn_composition/src/webui.rs | 7 +- 10 files changed, 503 insertions(+), 50 deletions(-) create mode 100644 crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs diff --git a/crates/ironclaw_reborn_composition/assets/prompts/default-system.md b/crates/ironclaw_reborn_composition/assets/prompts/default-system.md index 8df69be1c32..7d464a9fb91 100644 --- a/crates/ironclaw_reborn_composition/assets/prompts/default-system.md +++ b/crates/ironclaw_reborn_composition/assets/prompts/default-system.md @@ -10,11 +10,6 @@ You are IronClaw Agent, a secure autonomous assistant. When a tool result is partial, truncated, failed, or otherwise shows the requested work is unfinished, adapt and continue autonomously. Ask the user only when progress requires external information, approval, or a product decision. -## Delivery Targets - -- When visible outbound delivery target tools exist and the user asks to send final replies, routine results, or trigger results through a product or channel such as Slack, call `builtin__outbound_delivery_targets_list` first, then call `builtin__outbound_delivery_target_set` with a returned `target_id` before creating the routine or trigger. -- Do not say a delivery product is unavailable, and do not ask the user to reconnect it, until you have listed available outbound delivery targets and found none. - ## Safety - You have no independent goals. Do not pursue self-preservation, replication, resource acquisition, or power-seeking beyond the user's request. diff --git a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs index cfb9a7bee6d..ee636e58063 100644 --- a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs +++ b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs @@ -1,4 +1,7 @@ -use std::sync::Arc; +use std::{ + collections::BTreeMap, + sync::{Arc, RwLock}, +}; use async_trait::async_trait; use chrono::Utc; @@ -81,6 +84,91 @@ impl OutboundDeliveryTargetRegistry { } } +pub(crate) struct MutableOutboundDeliveryTargetRegistry { + providers: RwLock>>, +} + +impl std::fmt::Debug for MutableOutboundDeliveryTargetRegistry { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let provider_count = self + .providers + .read() + .map(|providers| providers.len()) + .unwrap_or_default(); + formatter + .debug_struct("MutableOutboundDeliveryTargetRegistry") + .field("providers", &provider_count) + .finish() + } +} + +impl Default for MutableOutboundDeliveryTargetRegistry { + fn default() -> Self { + Self { + providers: RwLock::new(BTreeMap::new()), + } + } +} + +impl MutableOutboundDeliveryTargetRegistry { + #[cfg_attr(not(feature = "slack-v2-host-beta"), allow(dead_code))] + pub(crate) fn register_provider( + &self, + provider_key: impl Into, + provider: Arc, + ) -> bool { + let Ok(mut providers) = self.providers.write() else { + tracing::warn!( + target = "ironclaw::reborn::outbound_preferences", + "outbound target registry lock failed while registering provider" + ); + return false; + }; + providers.insert(provider_key.into(), provider).is_none() + } + + fn providers( + &self, + ) -> Result>, RebornServicesError> { + self.providers + .read() + .map(|providers| providers.values().cloned().collect()) + .map_err(|_| outbound_target_registry_error()) + } +} + +#[async_trait] +impl OutboundDeliveryTargetProvider for MutableOutboundDeliveryTargetRegistry { + async fn list_outbound_delivery_targets( + &self, + caller: &WebUiAuthenticatedCaller, + ) -> Result, RebornServicesError> { + OutboundDeliveryTargetRegistry::new(self.providers()?) + .list_outbound_delivery_targets(caller) + .await + } + + async fn resolve_outbound_delivery_target( + &self, + caller: &WebUiAuthenticatedCaller, + target_id: &RebornOutboundDeliveryTargetId, + ) -> Result, RebornServicesError> { + OutboundDeliveryTargetRegistry::new(self.providers()?) + .resolve_outbound_delivery_target(caller, target_id) + .await + } + + async fn resolve_reply_target_binding( + &self, + caller: &WebUiAuthenticatedCaller, + target: &ReplyTargetBindingRef, + ) -> Result, RebornServicesError> { + OutboundDeliveryTargetRegistry::new(self.providers()?) + .resolve_reply_target_binding(caller, target) + .await + } +} + #[async_trait] impl OutboundDeliveryTargetProvider for OutboundDeliveryTargetRegistry { async fn list_outbound_delivery_targets( @@ -327,6 +415,17 @@ fn outbound_target_not_found() -> RebornServicesError { } } +fn outbound_target_registry_error() -> RebornServicesError { + RebornServicesError { + code: RebornServicesErrorCode::Internal, + kind: RebornServicesErrorKind::Internal, + status_code: 500, + retryable: false, + field: None, + validation_code: None, + } +} + fn map_outbound_repository_error(error: OutboundError) -> RebornServicesError { match error { OutboundError::InvalidRequest { .. } diff --git a/crates/ironclaw_reborn_composition/src/runtime.rs b/crates/ironclaw_reborn_composition/src/runtime.rs index 309c6bbac97..6c207286095 100644 --- a/crates/ironclaw_reborn_composition/src/runtime.rs +++ b/crates/ironclaw_reborn_composition/src/runtime.rs @@ -55,7 +55,7 @@ use ironclaw_product_workflow::{ ApprovalBlockedTurnRun, ApprovalInteractionScope, ApprovalInteractionService, ApprovalResolverPort, ApprovalTurnRunLocator, AuthInteractionService, DefaultApprovalInteractionService, DefaultAuthInteractionService, - RunStateApprovalInteractionReadModel, + OutboundPreferencesProductFacade, RunStateApprovalInteractionReadModel, }; use ironclaw_reborn::loop_exit_applier::{ ApprovalGateEvidenceStore, ThreadCheckpointLoopExitEvidencePort, @@ -94,6 +94,10 @@ use ironclaw_turns::{ use crate::default_system_prompt::DefaultSystemPromptIdentitySource; use crate::factory::{LocalDevRootFilesystem, LocalDevTurnStateStore, builtin_extension_registry}; use crate::local_dev_capability_policy::local_dev_capability_policy; +use crate::outbound_preferences::{ + MutableOutboundDeliveryTargetRegistry, OutboundDeliveryTargetProvider, + RebornOutboundPreferencesFacade, +}; use crate::projection::{RebornProjectionServices, build_reborn_projection_services}; use crate::runtime_input::{ PollSettings, RebornRuntimeIdentity, RebornRuntimeInput, TriggerPollerAuthorizerConfig, @@ -236,6 +240,9 @@ mod auth_interaction_tests; #[path = "runtime/tests/default_system_prompt.rs"] mod default_system_prompt_tests; mod local_dev; +#[cfg(test)] +#[path = "runtime/tests/outbound_delivery.rs"] +mod outbound_delivery_tests; mod production; mod skills; @@ -386,6 +393,7 @@ pub struct RebornRuntime { #[cfg(any(test, feature = "test-support"))] trigger_conversation_pairing: Option>, + outbound_delivery_target_registry: Option>, budget_event_projection: Option, poll_settings: PollSettings, actor_user_id: UserId, @@ -894,6 +902,7 @@ impl RebornRuntime { )] pub(crate) fn clear_local_runtime_for_test(&mut self) { self.services.local_runtime = None; + self.outbound_delivery_target_registry = None; } /// Operator boot config, when the runtime was assembled with one. The @@ -1070,6 +1079,33 @@ impl RebornRuntime { self.auth_interaction_service.clone() } + pub(crate) fn outbound_delivery_target_provider( + &self, + ) -> Option> { + self.outbound_delivery_target_registry + .as_ref() + .map(|registry| { + let registry = Arc::clone(registry); + let provider: Arc = registry; + provider + }) + } + + #[cfg_attr(not(feature = "slack-v2-host-beta"), allow(dead_code))] + pub(crate) fn register_outbound_delivery_target_provider( + &self, + provider_key: impl Into, + provider: Arc, + ) -> bool { + let Some(registry) = self.outbound_delivery_target_registry.as_ref() else { + tracing::debug!( + "register_outbound_delivery_target_provider: local runtime registry unavailable" + ); + return false; + }; + registry.register_provider(provider_key, provider) + } + #[cfg(feature = "slack-v2-host-beta")] pub(crate) fn auth_challenge_provider(&self) -> Option> { self.services @@ -2349,6 +2385,21 @@ pub async fn build_reborn_runtime( ) .map_err(|error| RebornRuntimeError::SkillExecution(error.to_string()))?; } + let outbound_delivery_target_registry = + local_runtime.map(|_| Arc::new(MutableOutboundDeliveryTargetRegistry::default())); + let outbound_preferences_facade: Option> = + match (local_runtime, &outbound_delivery_target_registry) { + (Some(local_runtime), Some(registry)) => { + let registry = Arc::clone(registry); + let provider: Arc = registry; + Some(Arc::new(RebornOutboundPreferencesFacade::new( + Arc::clone(&local_runtime.outbound_preferences), + provider, + )) + as Arc) + } + _ => None, + }; let milestone_sink = projection_services.with_live_progress_milestone_sink_for_publisher( durable_milestone_sink, live_projection_publisher, @@ -2378,7 +2429,7 @@ pub async fn build_reborn_runtime( model_gateway, milestone_sink.clone(), skill_activation_source.clone(), - None, + outbound_preferences_facade.clone(), trajectory_observer, ) .ok_or(RebornRuntimeError::HostRuntimeUnavailable)?; @@ -2486,28 +2537,27 @@ pub async fn build_reborn_runtime( let communication_context_provider: Option< Arc, - > = local_runtime.map(|local_runtime| { - let mut lifecycle_facade = crate::lifecycle::RebornLocalLifecycleFacade::new(Arc::clone( - &local_runtime.skill_management, - )); - if let Some(extension_management) = &local_runtime.extension_management { - lifecycle_facade = - lifecycle_facade.with_extension_management(Arc::clone(extension_management)); - } - Arc::new( - crate::communication_context::RuntimeCommunicationContextProvider::new(Arc::new( - crate::outbound_preferences::RebornOutboundPreferencesFacade::new( - Arc::clone(&local_runtime.outbound_preferences), - Arc::new( - crate::outbound_preferences::OutboundDeliveryTargetRegistry::new( - Vec::new(), - ), - ), - ), - )) + > = match (local_runtime, outbound_preferences_facade.clone()) { + (Some(local_runtime), Some(outbound_preferences_facade)) => { + let mut lifecycle_facade = crate::lifecycle::RebornLocalLifecycleFacade::new( + Arc::clone(&local_runtime.skill_management), + ); + if let Some(extension_management) = &local_runtime.extension_management { + lifecycle_facade = + lifecycle_facade.with_extension_management(Arc::clone(extension_management)); + } + Some(Arc::new( + crate::communication_context::RuntimeCommunicationContextProvider::new( + outbound_preferences_facade, + ) .with_lifecycle_facade(Arc::new(lifecycle_facade)), - ) as Arc - }); + ) + as Arc< + dyn ironclaw_turns::run_profile::CommunicationContextProvider, + >) + } + _ => None, + }; let planned_runtime_parts = DefaultPlannedRuntimeParts { turn_state: Arc::clone(&turn_state_store), @@ -2790,6 +2840,7 @@ pub async fn build_reborn_runtime( post_submit_hook_slot: runtime_post_submit_hook_slot, #[cfg(any(test, feature = "test-support"))] trigger_conversation_pairing: trigger_conversation_pairing_value, + outbound_delivery_target_registry, budget_event_projection, poll_settings: poll, actor_user_id, diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs index 8d5745ceda4..e123058c692 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs @@ -30,7 +30,7 @@ pub(crate) const OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID: &str = "builtin.outbound_delivery_targets_list"; const OUTBOUND_DELIVERY_TARGETS_LIST_PROVIDER_TOOL_NAME: &str = "builtin__outbound_delivery_targets_list"; -const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. Use before saying a delivery product is unavailable or asking the user to reconnect it."; +const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin__trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID: &str = "builtin.outbound_delivery_target_set"; diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs index 5a25e97047c..b8d01f0f601 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs @@ -1319,7 +1319,7 @@ mod tests { assert!( list_tool .description - .contains("before builtin.trigger_create"), + .contains("before builtin__trigger_create"), "list tool description should steer delivery requests before trigger creation" ); let set_tool = tool_definitions diff --git a/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs b/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs index d733eb3ec92..0bc9c7a765b 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/tests/default_system_prompt.rs @@ -73,18 +73,6 @@ async fn local_dev_runtime_injects_default_system_prompt_into_model_request() { }), "local-dev runtime should send the editable default system prompt to the model gateway" ); - assert!( - recorded_requests[0].messages.iter().any(|message| { - message.role == HostManagedModelMessageRole::System - && message - .content - .contains("builtin__outbound_delivery_targets_list") - && message - .content - .contains("before creating the routine or trigger") - }), - "local-dev runtime should tell the model how to select outbound delivery targets before triggers" - ); assert!( recorded_requests[0].messages.iter().any(|message| { message.role == HostManagedModelMessageRole::User && message.content == "ping" diff --git a/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs b/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs new file mode 100644 index 00000000000..fc6eb92e62b --- /dev/null +++ b/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs @@ -0,0 +1,293 @@ +use std::sync::{Arc, Mutex as StdMutex}; +use std::time::Duration; + +use async_trait::async_trait; +use ironclaw_host_api::CapabilityId; +use ironclaw_loop_support::{ + HostManagedModelError, HostManagedModelErrorKind, HostManagedModelGateway, + HostManagedModelMessageRole, HostManagedModelRequest, HostManagedModelResponse, +}; +use ironclaw_product_workflow::{ + RebornOutboundDeliveryTargetCapabilities, RebornOutboundDeliveryTargetId, + RebornOutboundDeliveryTargetSummary, RebornServicesError, WebUiAuthenticatedCaller, +}; +use ironclaw_threads::{LoadContextMessagesRequest, MessageKind, ThreadHistoryRequest}; +use ironclaw_turns::{ + ReplyTargetBindingRef, TurnStatus, + run_profile::{LoopCapabilityPort, ProviderToolCall}, +}; + +use crate::RebornCompositionProfile; +use crate::input::RebornBuildInput; +use crate::outbound_preferences::{OutboundDeliveryTargetEntry, OutboundDeliveryTargetProvider}; +use crate::runtime_input::{PollSettings, RebornRuntimeIdentity, RebornRuntimeInput}; + +use super::build_reborn_runtime; + +const RUNTIME_SEND_TIMEOUT: Duration = Duration::from_secs(10); + +#[derive(Debug, Default)] +struct OutboundDeliveryTriggerGateway { + calls: StdMutex, + requests: StdMutex>, +} + +#[derive(Clone)] +struct StaticOutboundDeliveryTargetProvider { + entry: OutboundDeliveryTargetEntry, +} + +#[async_trait] +impl OutboundDeliveryTargetProvider for StaticOutboundDeliveryTargetProvider { + async fn list_outbound_delivery_targets( + &self, + _caller: &WebUiAuthenticatedCaller, + ) -> Result, RebornServicesError> { + Ok(vec![self.entry.clone()]) + } +} + +#[async_trait] +impl HostManagedModelGateway for OutboundDeliveryTriggerGateway { + async fn stream_model( + &self, + request: HostManagedModelRequest, + ) -> Result { + self.requests + .lock() + .expect("outbound trigger gateway requests lock poisoned") + .push(request); + Err(HostManagedModelError::safe( + HostManagedModelErrorKind::InvalidRequest, + "expected capability-aware model path", + )) + } + + async fn stream_model_with_capabilities( + &self, + request: HostManagedModelRequest, + capabilities: Arc, + ) -> Result { + let call_index = { + let mut calls = self + .calls + .lock() + .expect("outbound trigger gateway call lock poisoned"); + let call_index = *calls; + *calls += 1; + call_index + }; + self.requests + .lock() + .expect("outbound trigger gateway requests lock poisoned") + .push(request.clone()); + + if call_index >= 3 { + let tool_result_count = request + .messages + .iter() + .filter(|message| message.role == HostManagedModelMessageRole::ToolResult) + .count(); + assert_eq!( + tool_result_count, 3, + "final model request should observe list, set, and trigger_create results" + ); + return Ok(HostManagedModelResponse::assistant_reply( + "trigger delivery target selected", + )); + } + + let tool_definitions = capabilities + .tool_definitions() + .map_err(model_capability_error)?; + let call = match call_index { + 0 => provider_tool_call( + &tool_definitions, + "builtin.outbound_delivery_targets_list", + "call-list-outbound-delivery-targets", + serde_json::json!({"channel": "slack"}), + ), + 1 => provider_tool_call( + &tool_definitions, + "builtin.outbound_delivery_target_set", + "call-set-outbound-delivery-target", + serde_json::json!({"target_id": "slack:test-dm"}), + ), + 2 => provider_tool_call( + &tool_definitions, + "builtin.trigger_create", + "call-trigger-create", + serde_json::json!({ + "name": "Slack status digest", + "prompt": "Send the status digest to Slack.", + "cron": "0 9 * * *", + "timezone": "UTC" + }), + ), + _ => unreachable!("handled above"), + }; + let candidate = capabilities + .register_provider_tool_call(call) + .await + .map_err(model_capability_error)?; + Ok(HostManagedModelResponse::capability_calls( + vec![candidate], + "", + )) + } +} + +fn provider_tool_call( + tool_definitions: &[ironclaw_turns::run_profile::ProviderToolDefinition], + capability_id: &str, + call_id: &str, + arguments: serde_json::Value, +) -> ProviderToolCall { + let capability_id = CapabilityId::new(capability_id).expect("capability id"); + let tool = tool_definitions + .iter() + .find(|definition| definition.capability_id == capability_id) + .unwrap_or_else(|| panic!("{capability_id} provider tool definition should exist")); + ProviderToolCall { + provider_id: "test-provider".to_string(), + provider_model_id: "test-model".to_string(), + turn_id: Some("provider-turn-1".to_string()), + id: call_id.to_string(), + name: tool.name.clone(), + arguments, + response_reasoning: None, + reasoning: None, + signature: None, + } +} + +fn model_capability_error(error: impl std::fmt::Display) -> HostManagedModelError { + let safe_summary = error.to_string(); + HostManagedModelError::safe(HostManagedModelErrorKind::Unavailable, safe_summary) +} + +#[tokio::test] +async fn local_dev_runtime_selects_outbound_delivery_target_before_trigger_create() { + let root = tempfile::tempdir().expect("tempdir"); + let host_home = root.path().join("host-home"); + std::fs::create_dir_all(&host_home).expect("host home"); + let gateway = Arc::new(OutboundDeliveryTriggerGateway::default()); + let gateway_for_runtime: Arc = gateway.clone(); + let input = RebornRuntimeInput::from_services( + RebornBuildInput::local_dev_with_profile( + RebornCompositionProfile::LocalDevYolo, + "runtime-outbound-trigger-owner", + root.path().join("local-dev"), + ) + .with_runtime_policy( + crate::local_dev_yolo_runtime_policy(true).expect("local-yolo policy resolves"), + ) + .with_local_dev_confirmed_host_home_root(host_home), + ) + .with_identity(RebornRuntimeIdentity { + tenant_id: "runtime-outbound-trigger-tenant".to_string(), + agent_id: "runtime-outbound-trigger-agent".to_string(), + source_binding_id: "runtime-outbound-trigger-source".to_string(), + reply_target_binding_id: "runtime-outbound-trigger-reply".to_string(), + }) + .with_poll_settings(PollSettings { + interval: Duration::from_millis(10), + max_total: Duration::from_secs(3), + }) + .with_model_gateway_override(gateway_for_runtime); + + let runtime = build_reborn_runtime(input).await.expect("runtime builds"); + let slack_target_id = RebornOutboundDeliveryTargetId::new("slack:test-dm").expect("target id"); + let registered = runtime.register_outbound_delivery_target_provider( + "slack:test", + Arc::new(StaticOutboundDeliveryTargetProvider { + entry: OutboundDeliveryTargetEntry { + summary: RebornOutboundDeliveryTargetSummary::new( + slack_target_id, + "slack", + "Slack DM", + Some("Personal Slack direct message".to_string()), + ) + .expect("target summary"), + capabilities: RebornOutboundDeliveryTargetCapabilities { + final_replies: true, + gate_prompts: false, + auth_prompts: false, + }, + reply_target_binding_ref: ReplyTargetBindingRef::new("reply:test:slack-dm") + .expect("reply target"), + }, + }), + ); + assert!(registered, "test Slack target provider should register"); + + let conversation = runtime.new_conversation().await.expect("conversation"); + let reply = tokio::time::timeout( + RUNTIME_SEND_TIMEOUT, + runtime.send_user_message( + &conversation, + "Create a daily trigger and send the result to my Slack DM.", + ), + ) + .await + .expect("runtime send should finish") + .expect("runtime send should succeed"); + + assert_eq!(reply.status, TurnStatus::Completed); + assert_eq!( + reply.text.as_deref(), + Some("trigger delivery target selected") + ); + let history = runtime + .thread_service + .list_thread_history(ThreadHistoryRequest { + scope: runtime.thread_scope.clone(), + thread_id: conversation.0.clone(), + }) + .await + .expect("thread history"); + let tool_result_ids = history + .messages + .iter() + .filter(|message| message.kind == MessageKind::ToolResultReference) + .map(|message| message.message_id) + .collect::>(); + assert_eq!( + tool_result_ids.len(), + 3, + "runtime should persist list, set, and trigger_create tool results" + ); + let context = runtime + .thread_service + .load_context_messages(LoadContextMessagesRequest { + scope: runtime.thread_scope.clone(), + thread_id: conversation.0.clone(), + message_ids: tool_result_ids, + }) + .await + .expect("tool result context"); + let invoked_capability_ids = context + .messages + .iter() + .map(|message| { + message + .tool_result_provider_call + .as_ref() + .expect("provider replay metadata") + .capability_id + .as_str() + .to_string() + }) + .collect::>(); + assert_eq!( + invoked_capability_ids, + vec![ + "builtin.outbound_delivery_targets_list", + "builtin.outbound_delivery_target_set", + "builtin.trigger_create", + ], + "Slack trigger delivery should list targets, select one, then create the trigger" + ); + + runtime.shutdown().await.expect("runtime shutdown"); +} diff --git a/crates/ironclaw_reborn_composition/src/slack_connectable_channel.rs b/crates/ironclaw_reborn_composition/src/slack_connectable_channel.rs index faa7b921d02..d9152396a51 100644 --- a/crates/ironclaw_reborn_composition/src/slack_connectable_channel.rs +++ b/crates/ironclaw_reborn_composition/src/slack_connectable_channel.rs @@ -39,14 +39,16 @@ pub fn build_webui_services_with_slack_host_beta_mounts( SlackConnectableChannelVisibility::PersonalPairingAndAdminChannelManagement } }; - let outbound_delivery_target_providers = slack_mounts - .map(|mounts| vec![Arc::clone(&mounts.outbound_delivery_target_provider)]) - .unwrap_or_default(); + if slack_mounts.is_some() && runtime.outbound_delivery_target_provider().is_none() { + return Err(RebornBuildError::InvalidConfig { + reason: "outbound delivery target providers require local runtime services".to_string(), + }); + } build_webui_services_with_connectable_channels( runtime, event_stream, slack_connectable_channels(visibility), - outbound_delivery_target_providers, + Vec::new(), ) } diff --git a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs index bb30b358a37..7f12f3bae2f 100644 --- a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs +++ b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs @@ -249,11 +249,11 @@ pub enum SlackHostBetaBuildError { InvalidConfig { field: &'static str, reason: String }, } +#[non_exhaustive] pub struct SlackHostBetaMounts { pub events: PublicRouteMount, pub personal_binding_pairing: SlackPersonalBindingPairingRouteConfig, pub channel_routes: SlackChannelRouteAdminRouteConfig, - pub(crate) outbound_delivery_target_provider: Arc, } pub fn build_slack_events_route_mount( @@ -462,11 +462,20 @@ pub fn build_slack_host_beta_mounts( channel_route_store, Arc::clone(&personal_dm_target_store), )); + let registered = runtime.register_outbound_delivery_target_provider( + SLACK_V2_ADAPTER_ID, + Arc::clone(&outbound_delivery_target_provider), + ); + if !registered { + tracing::debug!( + target = "ironclaw::reborn::slack_host_beta", + "Slack outbound delivery target provider replaced an existing registration" + ); + } Ok(SlackHostBetaMounts { events, personal_binding_pairing: SlackPersonalBindingPairingRouteConfig::new(pairing), channel_routes, - outbound_delivery_target_provider, }) } @@ -1816,6 +1825,17 @@ mod tests { assert_eq!(target.target.channel.as_str(), "slack"); assert_eq!(target.target.display_name.as_str(), "Slack channel C0HOST"); assert!(target.capabilities.final_replies); + let runtime_targets = runtime + .outbound_delivery_target_provider() + .expect("Slack mounts should register runtime outbound target provider") + .list_outbound_delivery_targets(&shared_subject) + .await + .expect("runtime target list"); + assert_eq!(runtime_targets.len(), 1); + assert_eq!( + runtime_targets[0].summary.target_id.as_str(), + target.target.target_id.as_str() + ); let selected = bundle .api diff --git a/crates/ironclaw_reborn_composition/src/webui.rs b/crates/ironclaw_reborn_composition/src/webui.rs index fb8d71baa3f..9a2151a2426 100644 --- a/crates/ironclaw_reborn_composition/src/webui.rs +++ b/crates/ironclaw_reborn_composition/src/webui.rs @@ -76,9 +76,14 @@ pub(crate) fn build_webui_services_with_connectable_channels( runtime: &RebornRuntime, event_stream: Option>, connectable_channels: Option>, - outbound_delivery_target_providers: Vec>, + mut outbound_delivery_target_providers: Vec>, ) -> Result { let services = runtime.services(); + if services.local_runtime.is_some() + && let Some(provider) = runtime.outbound_delivery_target_provider() + { + outbound_delivery_target_providers.push(provider); + } let mut api = ProductRebornServices::new( runtime.webui_thread_service(), From 19267dca58f7c3743e0e6c29e448ae6291a125bb Mon Sep 17 00:00:00 2001 From: Firat Sertgoz Date: Mon, 15 Jun 2026 20:13:36 +0300 Subject: [PATCH 3/6] fix(reborn): fail loud on outbound target registration (#4780) --- .../src/outbound_preferences.rs | 48 ++++++++++++++----- .../src/runtime.rs | 18 ++++--- .../src/runtime/tests/outbound_delivery.rs | 10 +++- .../src/slack_host_beta.rs | 32 +++++++++---- 4 files changed, 77 insertions(+), 31 deletions(-) diff --git a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs index ee636e58063..9561ea33c3a 100644 --- a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs +++ b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs @@ -88,13 +88,25 @@ pub(crate) struct MutableOutboundDeliveryTargetRegistry { providers: RwLock>>, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum OutboundDeliveryTargetRegistrationOutcome { + Registered, + Replaced, +} + impl std::fmt::Debug for MutableOutboundDeliveryTargetRegistry { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let provider_count = self - .providers - .read() - .map(|providers| providers.len()) - .unwrap_or_default(); + let provider_count = match self.providers.read() { + Ok(providers) => providers.len(), + Err(error) => { + tracing::debug!( + target = "ironclaw::reborn::outbound_preferences", + error = ?error, + "outbound target registry read lock failed during debug formatting" + ); + 0 + } + }; formatter .debug_struct("MutableOutboundDeliveryTargetRegistry") .field("providers", &provider_count) @@ -116,15 +128,20 @@ impl MutableOutboundDeliveryTargetRegistry { &self, provider_key: impl Into, provider: Arc, - ) -> bool { - let Ok(mut providers) = self.providers.write() else { - tracing::warn!( + ) -> Result { + let mut providers = self.providers.write().map_err(|error| { + tracing::debug!( target = "ironclaw::reborn::outbound_preferences", - "outbound target registry lock failed while registering provider" + error = ?error, + "outbound target registry write lock failed" ); - return false; + outbound_target_registry_error() + })?; + let outcome = match providers.insert(provider_key.into(), provider) { + Some(_) => OutboundDeliveryTargetRegistrationOutcome::Replaced, + None => OutboundDeliveryTargetRegistrationOutcome::Registered, }; - providers.insert(provider_key.into(), provider).is_none() + Ok(outcome) } fn providers( @@ -133,7 +150,14 @@ impl MutableOutboundDeliveryTargetRegistry { self.providers .read() .map(|providers| providers.values().cloned().collect()) - .map_err(|_| outbound_target_registry_error()) + .map_err(|error| { + tracing::debug!( + target = "ironclaw::reborn::outbound_preferences", + error = ?error, + "outbound target registry read lock failed" + ); + outbound_target_registry_error() + }) } } diff --git a/crates/ironclaw_reborn_composition/src/runtime.rs b/crates/ironclaw_reborn_composition/src/runtime.rs index 6c207286095..fa3a9b3fc40 100644 --- a/crates/ironclaw_reborn_composition/src/runtime.rs +++ b/crates/ironclaw_reborn_composition/src/runtime.rs @@ -96,7 +96,7 @@ use crate::factory::{LocalDevRootFilesystem, LocalDevTurnStateStore, builtin_ext use crate::local_dev_capability_policy::local_dev_capability_policy; use crate::outbound_preferences::{ MutableOutboundDeliveryTargetRegistry, OutboundDeliveryTargetProvider, - RebornOutboundPreferencesFacade, + OutboundDeliveryTargetRegistrationOutcome, RebornOutboundPreferencesFacade, }; use crate::projection::{RebornProjectionServices, build_reborn_projection_services}; use crate::runtime_input::{ @@ -1096,14 +1096,18 @@ impl RebornRuntime { &self, provider_key: impl Into, provider: Arc, - ) -> bool { + ) -> Result { let Some(registry) = self.outbound_delivery_target_registry.as_ref() else { - tracing::debug!( - "register_outbound_delivery_target_provider: local runtime registry unavailable" - ); - return false; + return Err(RebornRuntimeError::InvalidArgument { + reason: "outbound delivery target registry unavailable for this runtime" + .to_string(), + }); }; - registry.register_provider(provider_key, provider) + registry + .register_provider(provider_key, provider) + .map_err(|error| RebornRuntimeError::InvalidArgument { + reason: format!("outbound delivery target provider registration failed: {error}"), + }) } #[cfg(feature = "slack-v2-host-beta")] diff --git a/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs b/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs index fc6eb92e62b..a6cca1ea2de 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/tests/outbound_delivery.rs @@ -19,7 +19,10 @@ use ironclaw_turns::{ use crate::RebornCompositionProfile; use crate::input::RebornBuildInput; -use crate::outbound_preferences::{OutboundDeliveryTargetEntry, OutboundDeliveryTargetProvider}; +use crate::outbound_preferences::{ + OutboundDeliveryTargetEntry, OutboundDeliveryTargetProvider, + OutboundDeliveryTargetRegistrationOutcome, +}; use crate::runtime_input::{PollSettings, RebornRuntimeIdentity, RebornRuntimeInput}; use super::build_reborn_runtime; @@ -219,7 +222,10 @@ async fn local_dev_runtime_selects_outbound_delivery_target_before_trigger_creat }, }), ); - assert!(registered, "test Slack target provider should register"); + assert_eq!( + registered.expect("test Slack target provider should register"), + OutboundDeliveryTargetRegistrationOutcome::Registered + ); let conversation = runtime.new_conversation().await.expect("conversation"); let reply = tokio::time::timeout( diff --git a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs index 7f12f3bae2f..bf9d7bf17e9 100644 --- a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs +++ b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs @@ -37,7 +37,9 @@ use secrecy::{ExposeSecret, SecretString}; use thiserror::Error; use crate::RebornRuntime; -use crate::outbound_preferences::OutboundDeliveryTargetProvider; +use crate::outbound_preferences::{ + OutboundDeliveryTargetProvider, OutboundDeliveryTargetRegistrationOutcome, +}; use crate::slack_actor_identity::SlackUserIdentityActorResolver; use crate::slack_channel_routes::{ SlackChannelRouteAdminRouteConfig, SlackChannelRouteStore, SlackChannelRouteSubjectResolver, @@ -241,6 +243,8 @@ pub enum SlackHostBetaBuildError { RuntimeHttpEgressUnavailable, #[error("Slack host-beta requires durable host state")] DurableHostStateUnavailable, + #[error("Slack host-beta outbound delivery target registration failed: {reason}")] + OutboundDeliveryTargetRegistration { reason: String }, #[error( "Slack host-beta personal binding requires [slack].api_app_id for tenant app-scoped pairing" )] @@ -462,15 +466,23 @@ pub fn build_slack_host_beta_mounts( channel_route_store, Arc::clone(&personal_dm_target_store), )); - let registered = runtime.register_outbound_delivery_target_provider( - SLACK_V2_ADAPTER_ID, - Arc::clone(&outbound_delivery_target_provider), - ); - if !registered { - tracing::debug!( - target = "ironclaw::reborn::slack_host_beta", - "Slack outbound delivery target provider replaced an existing registration" - ); + match runtime + .register_outbound_delivery_target_provider( + SLACK_V2_ADAPTER_ID, + Arc::clone(&outbound_delivery_target_provider), + ) + .map_err( + |error| SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { + reason: error.to_string(), + }, + )? { + OutboundDeliveryTargetRegistrationOutcome::Registered => {} + OutboundDeliveryTargetRegistrationOutcome::Replaced => { + tracing::debug!( + target = "ironclaw::reborn::slack_host_beta", + "Slack outbound delivery target provider replaced an existing registration" + ); + } } Ok(SlackHostBetaMounts { events, From fe4395298e16647a035b66a9e6f09ddb6f8d8091 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Tue, 16 Jun 2026 00:55:10 +0300 Subject: [PATCH 4/6] fix(slack): harden outbound delivery rendering --- .../src/slack_delivery.rs | 34 +- .../ironclaw_slack_v2_adapter/src/adapter.rs | 290 +++--------- .../ironclaw_slack_v2_adapter/src/delivery.rs | 207 ++++++++ crates/ironclaw_slack_v2_adapter/src/lib.rs | 4 + .../ironclaw_slack_v2_adapter/src/mrkdwn.rs | 441 ++++++++++++++++++ .../ironclaw_slack_v2_adapter/src/render.rs | 196 ++------ 6 files changed, 791 insertions(+), 381 deletions(-) create mode 100644 crates/ironclaw_slack_v2_adapter/src/delivery.rs create mode 100644 crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs diff --git a/crates/ironclaw_reborn_composition/src/slack_delivery.rs b/crates/ironclaw_reborn_composition/src/slack_delivery.rs index f2e29954789..b36941119a5 100644 --- a/crates/ironclaw_reborn_composition/src/slack_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/slack_delivery.rs @@ -54,6 +54,7 @@ use crate::auth_prompt::auth_prompt_view_for_blocked_auth; use crate::slack_outbound_targets::slack_conversation_id_from_reply_target_binding_ref; const MAX_SLACK_RUN_POLL_INTERVAL: Duration = Duration::from_secs(5); +const DEFAULT_TRIGGERED_RUN_DELIVERY_MAX_WAIT: Duration = Duration::from_secs(30 * 60); const SLACK_RUN_POLL_JITTER_BUCKETS: u32 = 5; const SLACK_API_HOST: &str = "slack.com"; const SLACK_BOT_TOKEN_HANDLE: &str = "slack_bot_token"; @@ -1570,7 +1571,10 @@ impl TriggeredRunDeliveryDriver { ) -> Self { Self::with_settings( services, - SlackFinalReplyDeliverySettings::default(), + SlackFinalReplyDeliverySettings { + max_wait: DEFAULT_TRIGGERED_RUN_DELIVERY_MAX_WAIT, + ..SlackFinalReplyDeliverySettings::default() + }, delivery_store, route_store, fallback_agent_id, @@ -3056,6 +3060,34 @@ mod tests { ); } + #[test] + fn triggered_driver_default_wait_budget_is_longer_than_live_slack_reply_wait() { + let install = "test-install"; + let scope = personal_turn_scope(); + let coordinator = Arc::new(ScriptedTurnCoordinator::with_single_status( + TurnStatus::Completed, + )); + let thread_service = Arc::new(InMemorySessionThreadService::default()); + let outbound = Arc::new(InMemoryOutboundStateStore::default()); + let egress = Arc::new(FakeProtocolHttpEgress::new(vec!["slack.com".to_string()])); + let delivery_store = Arc::new(InMemoryTriggeredRunDeliveryStore::default()); + let route_store = Arc::new(InMemoryDeliveredGateRouteStore::default()); + let services = make_services(coordinator, thread_service, egress, outbound, install); + + let driver = TriggeredRunDeliveryDriver::new( + services, + delivery_store, + route_store, + scope.agent_id.clone().expect("test scope has agent"), + ); + + assert_eq!( + driver.settings.max_wait, + DEFAULT_TRIGGERED_RUN_DELIVERY_MAX_WAIT + ); + assert!(driver.settings.max_wait > SlackFinalReplyDeliverySettings::default().max_wait); + } + // --- BlockedAuth / timeout driver tests ------------------------------------ /// BlockedAuth state: driver sends an auth-prompt notification (no http/https URL), diff --git a/crates/ironclaw_slack_v2_adapter/src/adapter.rs b/crates/ironclaw_slack_v2_adapter/src/adapter.rs index dddef7e2649..dd3d1e17386 100644 --- a/crates/ironclaw_slack_v2_adapter/src/adapter.rs +++ b/crates/ironclaw_slack_v2_adapter/src/adapter.rs @@ -8,23 +8,20 @@ use ironclaw_product_adapters::{ ParsedProductInbound, ProductAdapter, ProductAdapterCapabilities, ProductAdapterError, ProductAdapterId, ProductCapabilityFlag, ProductOutboundEnvelope, ProductOutboundPayload, ProductOutboundTarget, ProductRenderOutcome, ProductSurfaceKind, ProtocolAuthEvidence, - ProtocolHttpEgress, ProtocolHttpEgressError, + ProtocolHttpEgress, }; use ironclaw_turns::TurnRunId; -use serde::Deserialize; +use crate::delivery::send_slack_post_message; use crate::payload::{SLACK_API_HOST, SlackPayloadParseError, parse_slack_event}; -use crate::render::{SlackRenderError, render_auth_prompt, render_final_reply, render_gate_prompt}; +use crate::render::{ + SlackRenderError, render_auth_prompt, render_final_reply_messages, render_gate_prompt, +}; /// Timeout for recording a delivery status to the sink. /// Guards against a hung sink blocking the delivery hot path indefinitely. const SINK_RECORD_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); -/// Maximum accepted byte length for a Slack `chat.postMessage` response body. -/// Protects against WAF/proxy responses (e.g. large HTML error pages on 200 OK) -/// causing full-allocation and O(n) deserialization on the delivery hot path. -const MAX_SLACK_RESPONSE_BYTES: usize = 64 * 1024; // 64 KB - #[derive(Debug, Clone)] pub struct SlackV2AdapterConfig { pub adapter_id: ProductAdapterId, @@ -154,13 +151,13 @@ impl ProductAdapter for SlackV2Adapter { let target_binding = envelope.target.reply_target_binding_ref.clone(); let run_id = payload_run_id(&envelope.payload); - let request = match render_supported_payload( + let requests = match render_supported_payload( &envelope.target, &envelope.payload, self.config.egress_credential_handle.clone(), ) { - Ok(Some(request)) => request, - Ok(None) => { + Ok(RenderedSlackOutbound::Messages(requests)) => requests, + Ok(RenderedSlackOutbound::Deferred) => { record_status( delivery_sink, DeliveryStatus::Deferred { @@ -190,108 +187,12 @@ impl ProductAdapter for SlackV2Adapter { } }; - let response = match egress.send(request).await { - Ok(response) => response, - Err(egress_err) => { - let failure = SlackDeliveryFailureKind::from_egress_error(&egress_err); - let reason = RedactedString::new(egress_err.to_string()); - let status = match failure { - SlackDeliveryFailureKind::Retryable => DeliveryStatus::FailedRetryable { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - SlackDeliveryFailureKind::Unauthorized => DeliveryStatus::FailedUnauthorized { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - SlackDeliveryFailureKind::Permanent => DeliveryStatus::FailedPermanent { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - }; - record_status(delivery_sink, status).await; - return Err(failure.to_adapter_error(reason)); - } - }; - - if !(200..300).contains(&response.status()) { - let reason = RedactedString::new(format!( - "slack web api returned status {}", - response.status() - )); - let failure = SlackDeliveryFailureKind::from_http_status(response.status()); - let status = match failure { - SlackDeliveryFailureKind::Retryable => DeliveryStatus::FailedRetryable { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - SlackDeliveryFailureKind::Unauthorized => DeliveryStatus::FailedUnauthorized { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - SlackDeliveryFailureKind::Permanent => DeliveryStatus::FailedPermanent { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - }; - record_status(delivery_sink, status).await; - return Err(failure.to_adapter_error(reason)); - } - - if let Err(slack_err) = slack_post_message_result(response.body()) { - let reason = slack_err.reason; // already RedactedString — wrapped at construction - match slack_err.kind { - SlackDeliveryFailureKind::Unauthorized => { - record_status( - delivery_sink, - DeliveryStatus::FailedUnauthorized { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - ) - .await; - return Err(ProductAdapterError::EgressDenied { reason }); - } - SlackDeliveryFailureKind::Retryable => { - record_status( - delivery_sink, - DeliveryStatus::FailedRetryable { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - ) - .await; - return Err(ProductAdapterError::EgressTransient { reason }); - } - SlackDeliveryFailureKind::Permanent => { - record_status( - delivery_sink, - DeliveryStatus::FailedPermanent { - attempt_id, - target: target_binding.clone(), - run_id, - reason: reason.clone(), - }, - ) - .await; - return Err(ProductAdapterError::EgressDenied { reason }); - } + for request in requests { + if let Err(error) = + send_slack_post_message(egress, request, attempt_id, &target_binding, run_id).await + { + record_status(delivery_sink, error.status).await; + return Err(error.adapter_error); } } @@ -312,26 +213,34 @@ fn render_supported_payload( target: &ProductOutboundTarget, payload: &ProductOutboundPayload, credential_handle: EgressCredentialHandle, -) -> Result, SlackRenderError> { +) -> Result { match payload { ProductOutboundPayload::FinalReply(view) => { - render_final_reply(target, view, credential_handle).map(Some) + render_final_reply_messages(target, view, credential_handle) + .map(RenderedSlackOutbound::Messages) } ProductOutboundPayload::GatePrompt(view) => { - render_gate_prompt(target, view, credential_handle).map(Some) + render_gate_prompt(target, view, credential_handle) + .map(|request| RenderedSlackOutbound::Messages(vec![request])) } ProductOutboundPayload::AuthPrompt(view) => { - render_auth_prompt(target, view, credential_handle).map(Some) + render_auth_prompt(target, view, credential_handle) + .map(|request| RenderedSlackOutbound::Messages(vec![request])) } ProductOutboundPayload::Progress(_) | ProductOutboundPayload::CapabilityActivity(_) | ProductOutboundPayload::CapabilityDisplayPreview(_) | ProductOutboundPayload::ProjectionSnapshot { .. } | ProductOutboundPayload::ProjectionUpdate { .. } - | ProductOutboundPayload::KeepAlive => Ok(None), + | ProductOutboundPayload::KeepAlive => Ok(RenderedSlackOutbound::Deferred), } } +enum RenderedSlackOutbound { + Messages(Vec), + Deferred, +} + async fn record_status(sink: &dyn OutboundDeliverySink, status: DeliveryStatus) { // silent-ok: sink timeout guard — hung sink must not block the delivery hot path. let _ = tokio::time::timeout(SINK_RECORD_TIMEOUT, sink.record(status)).await; @@ -366,111 +275,6 @@ fn map_render_error(err: SlackRenderError) -> ProductAdapterError { } } -fn slack_post_message_result(body: &[u8]) -> Result<(), SlackPostMessageFailure> { - if body.len() > MAX_SLACK_RESPONSE_BYTES { - return Err(SlackPostMessageFailure::permanent( - "response body too large", - )); - } - let parsed: SlackPostMessageResponse = serde_json::from_slice(body).map_err(|err| { - // A truncated/empty body from a proxy/LB timeout is a transient infra - // condition; treat as retryable rather than permanently abandoning. - SlackPostMessageFailure { - reason: RedactedString::new(format!( - "Slack chat.postMessage response was not valid JSON: {err}" - )), - kind: SlackDeliveryFailureKind::Retryable, - } - })?; - if parsed.ok { - Ok(()) - } else { - let error = parsed.error.unwrap_or_else(|| "unknown_error".to_string()); - Err(SlackPostMessageFailure { - reason: RedactedString::new(format!("Slack rejected chat.postMessage ({})", error)), - kind: slack_error_kind(&error), - }) - } -} - -#[derive(Debug, Deserialize)] -struct SlackPostMessageResponse { - ok: bool, - error: Option, -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum SlackDeliveryFailureKind { - Unauthorized, - Retryable, - Permanent, -} - -impl SlackDeliveryFailureKind { - fn from_egress_error(err: &ProtocolHttpEgressError) -> Self { - match err { - ProtocolHttpEgressError::Timeout - | ProtocolHttpEgressError::Network(_) - | ProtocolHttpEgressError::LeakDetected => Self::Retryable, - ProtocolHttpEgressError::UnknownCredentialHandle { .. } - | ProtocolHttpEgressError::UnauthorizedCredentialHandle { .. } => Self::Unauthorized, - ProtocolHttpEgressError::UndeclaredHost { .. } - | ProtocolHttpEgressError::PolicyDenied { .. } => Self::Permanent, - } - } - - fn from_http_status(status: u16) -> Self { - if status >= 500 || status == 429 { - Self::Retryable - } else if status == 401 || status == 403 { - Self::Unauthorized - } else { - Self::Permanent - } - } - - fn to_adapter_error(self, reason: RedactedString) -> ProductAdapterError { - match self { - Self::Retryable => ProductAdapterError::EgressTransient { reason }, - Self::Unauthorized | Self::Permanent => ProductAdapterError::EgressDenied { reason }, - } - } -} - -#[derive(Debug, Clone, PartialEq, Eq)] -struct SlackPostMessageFailure { - reason: RedactedString, - kind: SlackDeliveryFailureKind, -} - -impl SlackPostMessageFailure { - fn permanent(reason: impl Into) -> Self { - Self { - reason: RedactedString::new(reason.into()), - kind: SlackDeliveryFailureKind::Permanent, - } - } -} - -fn slack_error_kind(error: &str) -> SlackDeliveryFailureKind { - match error { - "not_authed" - | "invalid_auth" - | "account_inactive" - | "token_revoked" - | "missing_scope" - | "no_permission" - | "is_bot" - | "not_allowed_token_type" => SlackDeliveryFailureKind::Unauthorized, - "fatal_error" - | "internal_error" - | "service_unavailable" - | "request_timeout" - | "ratelimited" => SlackDeliveryFailureKind::Retryable, - _ => SlackDeliveryFailureKind::Permanent, - } -} - #[cfg(test)] mod tests { use super::*; @@ -630,6 +434,46 @@ mod tests { )); } + #[tokio::test] + async fn large_final_reply_sends_multiple_slack_messages_and_records_one_delivery() { + let adapter = SlackV2Adapter::new(config()); + let egress = FakeProtocolHttpEgress::new(vec![SLACK_API_HOST.to_string()]); + egress.allow_credential_handle("slack_bot_token"); + let sink = FakeOutboundDeliverySink::new(); + let run_id = TurnRunId::new(); + let payload = ProductOutboundPayload::FinalReply(FinalReplyView { + turn_run_id: run_id, + text: "a".repeat(35_050), + generated_at: Utc::now(), + }); + + let outcome = adapter + .render_outbound(envelope(payload), &egress, &sink) + .await + .expect("render outbound"); + + assert_eq!(outcome, ProductRenderOutcome::DeliveryRecorded); + let calls = egress.calls(); + assert_eq!(calls.len(), 2); + for (index, call) in calls.iter().enumerate() { + assert_eq!(call.host, SLACK_API_HOST); + assert_eq!(call.path, "/api/chat.postMessage"); + let body: serde_json::Value = serde_json::from_slice(&call.body).expect("body json"); + assert_eq!(body["channel"], "C123"); + assert_eq!(body["thread_ts"], "1710000000.000001"); + assert!( + body["text"] + .as_str() + .expect("text") + .starts_with(&format!("Part {}/2\n", index + 1)) + ); + } + assert!(matches!( + sink.statuses().as_slice(), + [DeliveryStatus::Delivered { run_id: Some(delivered), .. }] if delivered == &run_id + )); + } + #[tokio::test] async fn render_outbound_rejects_mismatched_envelope_ids_without_egress() { let adapter = SlackV2Adapter::new(config()); diff --git a/crates/ironclaw_slack_v2_adapter/src/delivery.rs b/crates/ironclaw_slack_v2_adapter/src/delivery.rs new file mode 100644 index 00000000000..1adb457af83 --- /dev/null +++ b/crates/ironclaw_slack_v2_adapter/src/delivery.rs @@ -0,0 +1,207 @@ +//! Slack Web API delivery and status classification. + +use ironclaw_product_adapters::redaction::RedactedString; +use ironclaw_product_adapters::{ + DeliveryAttemptId, DeliveryStatus, EgressRequest, ProductAdapterError, ProtocolHttpEgress, + ProtocolHttpEgressError, +}; +use ironclaw_turns::{ReplyTargetBindingRef, TurnRunId}; +use serde::Deserialize; + +/// Maximum accepted byte length for a Slack `chat.postMessage` response body. +/// Protects against WAF/proxy responses (e.g. large HTML error pages on 200 OK) +/// causing full-allocation and O(n) deserialization on the delivery hot path. +const MAX_SLACK_RESPONSE_BYTES: usize = 64 * 1024; // 64 KB + +pub(crate) struct SlackPostMessageDeliveryError { + pub(crate) status: DeliveryStatus, + pub(crate) adapter_error: ProductAdapterError, +} + +pub(crate) async fn send_slack_post_message( + egress: &dyn ProtocolHttpEgress, + request: EgressRequest, + attempt_id: DeliveryAttemptId, + target_binding: &ReplyTargetBindingRef, + run_id: Option, +) -> Result<(), SlackPostMessageDeliveryError> { + let response = match egress.send(request).await { + Ok(response) => response, + Err(egress_err) => { + let failure = SlackDeliveryFailureKind::from_egress_error(&egress_err); + let reason = RedactedString::new(egress_err.to_string()); + return Err(slack_delivery_error( + failure, + attempt_id, + target_binding, + run_id, + reason, + )); + } + }; + + if !(200..300).contains(&response.status()) { + let reason = RedactedString::new(format!( + "slack web api returned status {}", + response.status() + )); + let failure = SlackDeliveryFailureKind::from_http_status(response.status()); + return Err(slack_delivery_error( + failure, + attempt_id, + target_binding, + run_id, + reason, + )); + } + + if let Err(slack_err) = slack_post_message_result(response.body()) { + return Err(slack_delivery_error( + slack_err.kind, + attempt_id, + target_binding, + run_id, + slack_err.reason, + )); + } + + Ok(()) +} + +fn slack_delivery_error( + failure: SlackDeliveryFailureKind, + attempt_id: DeliveryAttemptId, + target_binding: &ReplyTargetBindingRef, + run_id: Option, + reason: RedactedString, +) -> SlackPostMessageDeliveryError { + let status = match failure { + SlackDeliveryFailureKind::Retryable => DeliveryStatus::FailedRetryable { + attempt_id, + target: target_binding.clone(), + run_id, + reason: reason.clone(), + }, + SlackDeliveryFailureKind::Unauthorized => DeliveryStatus::FailedUnauthorized { + attempt_id, + target: target_binding.clone(), + run_id, + reason: reason.clone(), + }, + SlackDeliveryFailureKind::Permanent => DeliveryStatus::FailedPermanent { + attempt_id, + target: target_binding.clone(), + run_id, + reason: reason.clone(), + }, + }; + SlackPostMessageDeliveryError { + status, + adapter_error: failure.to_adapter_error(reason), + } +} + +fn slack_post_message_result(body: &[u8]) -> Result<(), SlackPostMessageFailure> { + if body.len() > MAX_SLACK_RESPONSE_BYTES { + return Err(SlackPostMessageFailure::permanent( + "response body too large", + )); + } + let parsed: SlackPostMessageResponse = serde_json::from_slice(body).map_err(|err| { + // A truncated/empty body from a proxy/LB timeout is a transient infra + // condition; treat as retryable rather than permanently abandoning. + SlackPostMessageFailure { + reason: RedactedString::new(format!( + "Slack chat.postMessage response was not valid JSON: {err}" + )), + kind: SlackDeliveryFailureKind::Retryable, + } + })?; + if parsed.ok { + Ok(()) + } else { + let error = parsed.error.unwrap_or_else(|| "unknown_error".to_string()); + Err(SlackPostMessageFailure { + reason: RedactedString::new(format!("Slack rejected chat.postMessage ({})", error)), + kind: slack_error_kind(&error), + }) + } +} + +#[derive(Debug, Deserialize)] +struct SlackPostMessageResponse { + ok: bool, + error: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum SlackDeliveryFailureKind { + Unauthorized, + Retryable, + Permanent, +} + +impl SlackDeliveryFailureKind { + fn from_egress_error(err: &ProtocolHttpEgressError) -> Self { + match err { + ProtocolHttpEgressError::Timeout + | ProtocolHttpEgressError::Network(_) + | ProtocolHttpEgressError::LeakDetected => Self::Retryable, + ProtocolHttpEgressError::UnknownCredentialHandle { .. } + | ProtocolHttpEgressError::UnauthorizedCredentialHandle { .. } => Self::Unauthorized, + ProtocolHttpEgressError::UndeclaredHost { .. } + | ProtocolHttpEgressError::PolicyDenied { .. } => Self::Permanent, + } + } + + fn from_http_status(status: u16) -> Self { + if status >= 500 || status == 429 { + Self::Retryable + } else if status == 401 || status == 403 { + Self::Unauthorized + } else { + Self::Permanent + } + } + + fn to_adapter_error(self, reason: RedactedString) -> ProductAdapterError { + match self { + Self::Retryable => ProductAdapterError::EgressTransient { reason }, + Self::Unauthorized | Self::Permanent => ProductAdapterError::EgressDenied { reason }, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct SlackPostMessageFailure { + reason: RedactedString, + kind: SlackDeliveryFailureKind, +} + +impl SlackPostMessageFailure { + fn permanent(reason: impl Into) -> Self { + Self { + reason: RedactedString::new(reason.into()), + kind: SlackDeliveryFailureKind::Permanent, + } + } +} + +fn slack_error_kind(error: &str) -> SlackDeliveryFailureKind { + match error { + "not_authed" + | "invalid_auth" + | "account_inactive" + | "token_revoked" + | "missing_scope" + | "no_permission" + | "is_bot" + | "not_allowed_token_type" => SlackDeliveryFailureKind::Unauthorized, + "fatal_error" + | "internal_error" + | "service_unavailable" + | "request_timeout" + | "ratelimited" => SlackDeliveryFailureKind::Retryable, + _ => SlackDeliveryFailureKind::Permanent, + } +} diff --git a/crates/ironclaw_slack_v2_adapter/src/lib.rs b/crates/ironclaw_slack_v2_adapter/src/lib.rs index 45ce4632302..c4e1529508c 100644 --- a/crates/ironclaw_slack_v2_adapter/src/lib.rs +++ b/crates/ironclaw_slack_v2_adapter/src/lib.rs @@ -6,12 +6,16 @@ //! signing secrets or bot tokens. //! //! * [`adapter`] — ProductAdapter implementation and egress/auth metadata. +//! * [`delivery`] — Slack Web API response classification and status mapping. +//! * [`mrkdwn`] — Slack mrkdwn rendering and message chunking. //! * [`payload`] — Slack Events API payload normalization. //! * [`render`] — `FinalReplyView` -> `chat.postMessage` request shaping. #![forbid(unsafe_code)] mod adapter; +mod delivery; +mod mrkdwn; mod payload; mod render; diff --git a/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs b/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs new file mode 100644 index 00000000000..284fb6400bf --- /dev/null +++ b/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs @@ -0,0 +1,441 @@ +//! Slack mrkdwn rendering helpers. + +/// Slack truncates very large `chat.postMessage` payloads. Keep chunks below +/// the documented hard ceiling and leave room for the part header we add when +/// a final reply spans multiple messages. +const SLACK_TEXT_SOFT_LIMIT_CHARS: usize = 35_000; +const SLACK_TEXT_CHUNK_BODY_CHARS: usize = 34_900; + +pub(crate) fn render_slack_mrkdwn(markdown: &str) -> String { + let mut rendered = String::with_capacity(markdown.len()); + let lines = markdown.lines().collect::>(); + let mut index = 0; + while index < lines.len() { + if let Some((headers, row_start)) = markdown_table_header_at(&lines, index) { + let mut row_index = row_start; + let mut rows = Vec::new(); + while row_index < lines.len() { + let Some(cells) = split_pipe_cells(lines[row_index]) else { + break; + }; + if is_markdown_table_separator_cells(&cells) { + row_index += 1; + continue; + } + rows.push(normalize_table_row_cells(headers.len(), cells)); + row_index += 1; + } + if !rows.is_empty() { + for (row_offset, row) in rows.iter().enumerate() { + rendered.push_str(&render_slack_table_record(&headers, row)); + if row_offset + 1 < rows.len() || row_index < lines.len() { + rendered.push('\n'); + } + } + index = row_index; + continue; + } + } + if is_markdown_table_separator(lines[index]) { + index += 1; + continue; + } + let line = lines[index]; + let converted = if is_markdown_table_row(line) { + render_table_row(line) + } else { + render_slack_mrkdwn_line(line) + }; + rendered.push_str(&converted); + if index + 1 < lines.len() { + rendered.push('\n'); + } + index += 1; + } + rendered +} + +pub(crate) fn slack_text_chunks(text: &str) -> Vec { + if text.chars().count() <= SLACK_TEXT_SOFT_LIMIT_CHARS { + return vec![text.to_string()]; + } + + let mut chunks = Vec::new(); + let mut current = String::new(); + let mut current_chars = 0usize; + + for segment in text.split_inclusive('\n') { + push_slack_text_segment( + segment, + &mut current, + &mut current_chars, + &mut chunks, + SLACK_TEXT_CHUNK_BODY_CHARS, + ); + } + if !current.is_empty() || chunks.is_empty() { + chunks.push(current); + } + + let total = chunks.len(); + chunks + .into_iter() + .enumerate() + .map(|(index, chunk)| format!("Part {}/{}\n{}", index + 1, total, chunk)) + .collect() +} + +fn push_slack_text_segment( + segment: &str, + current: &mut String, + current_chars: &mut usize, + chunks: &mut Vec, + limit: usize, +) { + let segment_chars = segment.chars().count(); + if segment_chars > limit { + flush_slack_text_chunk(current, current_chars, chunks); + for ch in segment.chars() { + current.push(ch); + *current_chars += 1; + if *current_chars >= limit { + flush_slack_text_chunk(current, current_chars, chunks); + } + } + return; + } + + if *current_chars > 0 && *current_chars + segment_chars > limit { + flush_slack_text_chunk(current, current_chars, chunks); + } + current.push_str(segment); + *current_chars += segment_chars; +} + +fn flush_slack_text_chunk( + current: &mut String, + current_chars: &mut usize, + chunks: &mut Vec, +) { + if current.is_empty() { + return; + } + chunks.push(std::mem::take(current)); + *current_chars = 0; +} + +fn markdown_table_header_at(lines: &[&str], index: usize) -> Option<(Vec, usize)> { + let cells = split_pipe_cells(lines[index])?; + let next_is_separator = lines + .get(index + 1) + .and_then(|line| split_pipe_cells(line)) + .is_some_and(|cells| is_markdown_table_separator_cells(&cells)); + if !(is_issue_table_header(&cells) || is_markdown_table_row(lines[index]) && next_is_separator) + { + return None; + } + let row_start = if next_is_separator { + index + 2 + } else { + index + 1 + }; + Some((cells, row_start)) +} + +fn split_pipe_cells(line: &str) -> Option> { + let trimmed = line.trim(); + if !trimmed.contains('|') { + return None; + } + let cells = trimmed + .trim_matches('|') + .split('|') + .map(|cell| cell.trim().to_string()) + .collect::>(); + (cells.len() >= 2).then_some(cells) +} + +fn normalize_table_row_cells(header_len: usize, mut cells: Vec) -> Vec { + if cells.len() > header_len && header_len > 0 { + let overflow = cells.split_off(header_len - 1); + cells.push(overflow.join(" | ")); + } + while cells.len() < header_len { + cells.push(String::new()); + } + cells +} + +fn is_issue_table_header(cells: &[String]) -> bool { + let normalized = cells + .iter() + .map(|cell| normalized_table_header(cell)) + .collect::>(); + normalized + .first() + .is_some_and(|cell| cell == "#" || cell == "issue") + && normalized.iter().any(|cell| cell == "title") +} + +fn normalized_table_header(cell: &str) -> String { + cell.trim() + .trim_matches('*') + .trim_matches('`') + .trim() + .to_ascii_lowercase() +} + +fn is_markdown_table_separator_cells(cells: &[String]) -> bool { + cells.iter().all(|cell| { + let cell = cell.trim(); + !cell.is_empty() && cell.chars().all(|ch| matches!(ch, '-' | ':' | ' ')) + }) +} + +fn render_slack_table_record(headers: &[String], row: &[String]) -> String { + if is_issue_table_header(headers) { + return render_issue_table_record(headers, row); + } + + let mut output = String::new(); + for (index, (header, cell)) in headers.iter().zip(row.iter()).enumerate() { + let cell = render_slack_mrkdwn_line(cell).trim().to_string(); + if cell.is_empty() || cell == "-" { + continue; + } + let header = render_slack_mrkdwn_line(header).trim().to_string(); + if output.is_empty() { + output.push_str("• *"); + output.push_str(&header); + output.push_str(":* "); + output.push_str(&cell); + } else { + output.push('\n'); + output.push_str(" *"); + output.push_str(&header); + output.push_str(":* "); + output.push_str(&cell); + } + if index == 0 && headers.len() == 1 { + break; + } + } + if output.is_empty() { + return "•".to_string(); + } + output +} + +fn render_issue_table_record(headers: &[String], row: &[String]) -> String { + let issue = row.first().map_or("", String::as_str); + let title = cell_for_header(headers, row, "title").unwrap_or(""); + let mut output = String::from("• "); + output.push_str(&render_issue_reference(issue)); + let title = render_slack_mrkdwn_line(title).trim().to_string(); + if !title.is_empty() && title != "-" { + output.push(' '); + output.push_str(&title); + } + + for (header, cell) in headers.iter().zip(row.iter()).skip(1) { + let normalized = normalized_table_header(header); + if normalized == "title" { + continue; + } + let cell = render_slack_mrkdwn_line(cell).trim().to_string(); + if cell.is_empty() || cell == "-" { + continue; + } + output.push('\n'); + output.push_str(" *"); + output.push_str(issue_detail_label(&normalized, header)); + output.push_str(":* "); + output.push_str(&cell); + } + output +} + +fn cell_for_header<'a>(headers: &[String], row: &'a [String], name: &str) -> Option<&'a str> { + headers + .iter() + .position(|header| normalized_table_header(header) == name) + .and_then(|index| row.get(index)) + .map(String::as_str) +} + +fn issue_detail_label<'a>(normalized: &str, original: &'a str) -> &'a str { + match normalized { + "status / summary" | "status/summary" | "summary" => "Summary", + "assignees" => "Assignees", + "labels" => "Labels", + "updated" => "Updated", + _ => original.trim(), + } +} + +fn render_issue_reference(issue: &str) -> String { + let rendered = render_slack_mrkdwn_line(issue).trim().to_string(); + if let Some((url, label)) = parse_slack_link(&rendered) + && label.chars().all(|ch| ch.is_ascii_digit()) + { + return format!("<{url}|#{label}>"); + } + if rendered.chars().all(|ch| ch.is_ascii_digit()) { + return format!("#{rendered}"); + } + rendered +} + +fn parse_slack_link(value: &str) -> Option<(&str, &str)> { + let inner = value.strip_prefix('<')?.strip_suffix('>')?; + inner.split_once('|') +} + +fn render_slack_mrkdwn_line(line: &str) -> String { + let line = strip_heading_marker(line); + let line = convert_markdown_links(line); + convert_markdown_bold(&line) +} + +fn strip_heading_marker(line: &str) -> &str { + let trimmed = line.trim_start(); + if !trimmed.starts_with('#') { + return line; + } + let hash_count = trimmed.chars().take_while(|ch| *ch == '#').count(); + if !(1..=6).contains(&hash_count) { + return line; + } + let rest = &trimmed[hash_count..]; + let Some(rest) = rest.strip_prefix(' ') else { + return line; + }; + rest +} + +fn convert_markdown_links(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + let bytes = line.as_bytes(); + let mut index = 0; + while index < line.len() { + if bytes[index] == b'[' + && let Some(label_end_rel) = line[index + 1..].find(']') + { + let label_end = index + 1 + label_end_rel; + if line[label_end..].starts_with("](") + && let Some(url_end_rel) = line[label_end + 2..].find(')') + { + let label = &line[index + 1..label_end]; + let url_end = label_end + 2 + url_end_rel; + let url = &line[label_end + 2..url_end]; + if is_safe_slack_link_url(url) { + out.push('<'); + out.push_str(url); + out.push('|'); + out.push_str(label); + out.push('>'); + index = url_end + 1; + continue; + } + } + } + let Some(ch) = line[index..].chars().next() else { + break; + }; + out.push(ch); + index += ch.len_utf8(); + } + out +} + +fn is_safe_slack_link_url(url: &str) -> bool { + url.starts_with("http://") || url.starts_with("https://") +} + +fn convert_markdown_bold(line: &str) -> String { + let mut out = String::with_capacity(line.len()); + for (index, part) in line.split("**").enumerate() { + if index > 0 { + out.push('*'); + } + out.push_str(part); + } + out +} + +fn is_markdown_table_row(line: &str) -> bool { + let trimmed = line.trim(); + trimmed.starts_with('|') && trimmed.ends_with('|') && trimmed.matches('|').count() >= 2 +} + +fn is_markdown_table_separator(line: &str) -> bool { + if !is_markdown_table_row(line) { + return false; + } + line.trim().trim_matches('|').split('|').all(|cell| { + let cell = cell.trim(); + !cell.is_empty() && cell.chars().all(|ch| matches!(ch, '-' | ':' | ' ')) + }) +} + +fn render_table_row(line: &str) -> String { + line.trim() + .trim_matches('|') + .split('|') + .map(|cell| render_slack_mrkdwn_line(cell.trim())) + .collect::>() + .join(" | ") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn common_markdown_renders_as_slack_mrkdwn() { + let text = render_slack_mrkdwn( + "Here are your top Notion docs:\n\n### Top Priority Docs\n\n1. **NEAR AI Engineering Weekly Updates** ([link](https://www.notion.com/p/abc))\n - \"Multi tenancy for migrating from Railway => top priority\"\n\n| Doc | Highlight |\n|---|---|\n| **Priority Agents** | Priority Agents |", + ); + + assert!(text.contains("Top Priority Docs")); + assert!(!text.contains("###")); + assert!(text.contains("*NEAR AI Engineering Weekly Updates*")); + assert!(text.contains("")); + assert!(!text.contains("|---|---|")); + assert!(text.contains("• *Doc:* *Priority Agents*")); + assert!(text.contains("*Highlight:* Priority Agents")); + } + + #[test] + fn issue_table_renders_as_slack_bullets_instead_of_raw_pipes() { + let text = render_slack_mrkdwn( + "GitHub check result\n\n# | Title | Labels | Assignees | Updated | Status / summary\n[4657](https://github.com/nearai/ironclaw/issues/4657) | **Unify reusable Google OAuth credentials** | enhancement, reborn | serrrfirat | 2026-06-10 | Investigate cross-extension OAuth credential reuse.\n4625 | Slack channel-routed personal and team agents | - | serrrfirat | 2026-06-09 | Mostly implemented.", + ); + + assert!(!text.contains("# | Title | Labels")); + assert!(text.contains( + "• *Unify reusable Google OAuth credentials*" + )); + assert!(text.contains(" *Labels:* enhancement, reborn")); + assert!(text.contains(" *Assignees:* serrrfirat")); + assert!(text.contains(" *Updated:* 2026-06-10")); + assert!(text.contains(" *Summary:* Investigate cross-extension OAuth credential reuse.")); + assert!(text.contains("• #4625 Slack channel-routed personal and team agents")); + } + + #[test] + fn long_text_renders_numbered_chunks_under_soft_limit() { + let chunks = slack_text_chunks(&"a".repeat(SLACK_TEXT_SOFT_LIMIT_CHARS + 10)); + + assert_eq!(chunks.len(), 2); + for (index, chunk) in chunks.iter().enumerate() { + assert!( + chunk.chars().count() <= SLACK_TEXT_SOFT_LIMIT_CHARS, + "chunk {} exceeded soft Slack text limit", + index + 1 + ); + } + assert!(chunks[0].starts_with("Part 1/2\n")); + assert!(chunks[1].starts_with("Part 2/2\n")); + } +} diff --git a/crates/ironclaw_slack_v2_adapter/src/render.rs b/crates/ironclaw_slack_v2_adapter/src/render.rs index 63f4cf600a5..13d105b45f2 100644 --- a/crates/ironclaw_slack_v2_adapter/src/render.rs +++ b/crates/ironclaw_slack_v2_adapter/src/render.rs @@ -12,6 +12,7 @@ use ironclaw_product_adapters::{ use serde::Serialize; use thiserror::Error; +use crate::mrkdwn::{render_slack_mrkdwn, slack_text_chunks}; use crate::payload::SLACK_API_HOST; #[derive(Debug, Clone, PartialEq, Eq, Error)] @@ -59,6 +60,15 @@ pub fn render_final_reply( ) } +pub(crate) fn render_final_reply_messages( + target: &ProductOutboundTarget, + view: &FinalReplyView, + credential_handle: EgressCredentialHandle, +) -> Result, SlackRenderError> { + let text = render_slack_mrkdwn(&view.text); + render_text_messages(target, slack_text_chunks(&text), true, credential_handle) +} + pub fn render_gate_prompt( target: &ProductOutboundTarget, view: &GatePromptView, @@ -128,142 +138,37 @@ fn render_text_message( mrkdwn: bool, credential_handle: EgressCredentialHandle, ) -> Result { - let reply = slack_reply_target(target)?; - let body = ChatPostMessageRequest { - channel: reply.channel, - text, - mrkdwn, - thread_ts: reply.thread_ts, - }; - let body_bytes = serde_json::to_vec(&body).map_err(|err| SlackRenderError::Serialization { - reason: err.to_string(), - })?; - - Ok(build_egress_request( - "/api/chat.postMessage", - body_bytes, - credential_handle, - )) -} - -fn render_slack_mrkdwn(markdown: &str) -> String { - let mut rendered = String::with_capacity(markdown.len()); - let lines = markdown.lines().collect::>(); - let mut index = 0; - while index < lines.len() { - if is_markdown_table_separator(lines[index]) { - index += 1; - continue; - } - let line = lines[index]; - let converted = if is_markdown_table_row(line) { - render_table_row(line) - } else { - render_slack_mrkdwn_line(line) - }; - rendered.push_str(&converted); - if index + 1 < lines.len() { - rendered.push('\n'); - } - index += 1; - } - rendered -} - -fn render_slack_mrkdwn_line(line: &str) -> String { - let line = strip_heading_marker(line); - let line = convert_markdown_links(line); - convert_markdown_bold(&line) -} - -fn strip_heading_marker(line: &str) -> &str { - let trimmed = line.trim_start(); - if !trimmed.starts_with('#') { - return line; - } - let hash_count = trimmed.chars().take_while(|ch| *ch == '#').count(); - if !(1..=6).contains(&hash_count) { - return line; - } - let rest = &trimmed[hash_count..]; - let Some(rest) = rest.strip_prefix(' ') else { - return line; - }; - rest -} - -fn convert_markdown_links(line: &str) -> String { - let mut out = String::with_capacity(line.len()); - let bytes = line.as_bytes(); - let mut index = 0; - while index < line.len() { - if bytes[index] == b'[' - && let Some(label_end_rel) = line[index + 1..].find(']') - { - let label_end = index + 1 + label_end_rel; - if line[label_end..].starts_with("](") - && let Some(url_end_rel) = line[label_end + 2..].find(')') - { - let label = &line[index + 1..label_end]; - let url_end = label_end + 2 + url_end_rel; - let url = &line[label_end + 2..url_end]; - if is_safe_slack_link_url(url) { - out.push('<'); - out.push_str(url); - out.push('|'); - out.push_str(label); - out.push('>'); - index = url_end + 1; - continue; - } - } - } - let Some(ch) = line[index..].chars().next() else { - break; - }; - out.push(ch); - index += ch.len_utf8(); - } - out -} - -fn is_safe_slack_link_url(url: &str) -> bool { - url.starts_with("http://") || url.starts_with("https://") -} - -fn convert_markdown_bold(line: &str) -> String { - let mut out = String::with_capacity(line.len()); - for (index, part) in line.split("**").enumerate() { - if index > 0 { - out.push('*'); - } - out.push_str(part); - } - out -} - -fn is_markdown_table_row(line: &str) -> bool { - let trimmed = line.trim(); - trimmed.starts_with('|') && trimmed.ends_with('|') && trimmed.matches('|').count() >= 2 + let mut requests = render_text_messages(target, vec![text], mrkdwn, credential_handle)?; + Ok(requests.remove(0)) } -fn is_markdown_table_separator(line: &str) -> bool { - if !is_markdown_table_row(line) { - return false; - } - line.trim().trim_matches('|').split('|').all(|cell| { - let cell = cell.trim(); - !cell.is_empty() && cell.chars().all(|ch| matches!(ch, '-' | ':' | ' ')) - }) -} - -fn render_table_row(line: &str) -> String { - line.trim() - .trim_matches('|') - .split('|') - .map(|cell| render_slack_mrkdwn_line(cell.trim())) - .collect::>() - .join(" | ") +fn render_text_messages( + target: &ProductOutboundTarget, + texts: Vec, + mrkdwn: bool, + credential_handle: EgressCredentialHandle, +) -> Result, SlackRenderError> { + let reply = slack_reply_target(target)?; + texts + .into_iter() + .map(|text| { + let body = ChatPostMessageRequest { + channel: reply.channel.clone(), + text, + mrkdwn, + thread_ts: reply.thread_ts.clone(), + }; + let body_bytes = + serde_json::to_vec(&body).map_err(|err| SlackRenderError::Serialization { + reason: err.to_string(), + })?; + Ok(build_egress_request( + "/api/chat.postMessage", + body_bytes, + credential_handle.clone(), + )) + }) + .collect() } #[derive(Debug, Serialize)] @@ -349,29 +254,6 @@ mod tests { assert_eq!(body["thread_ts"], "1710000000.000001"); } - #[test] - fn final_reply_renders_common_markdown_as_slack_mrkdwn() { - let view = FinalReplyView { - turn_run_id: TurnRunId::new(), - text: "Here are your top Notion docs:\n\n### Top Priority Docs\n\n1. **NEAR AI Engineering Weekly Updates** ([link](https://www.notion.com/p/abc))\n - \"Multi tenancy for migrating from Railway => top priority\"\n\n| Doc | Highlight |\n|---|---|\n| **Priority Agents** | Priority Agents |" - .to_string(), - generated_at: Utc::now(), - }; - - let request = render_final_reply(&target("C123", None), &view, handle()).expect("render"); - let body: serde_json::Value = serde_json::from_slice(request.body()).expect("body json"); - - assert_eq!(body["mrkdwn"], true); - let text = body["text"].as_str().expect("text"); - assert!(text.contains("Top Priority Docs")); - assert!(!text.contains("###")); - assert!(text.contains("*NEAR AI Engineering Weekly Updates*")); - assert!(text.contains("")); - assert!(!text.contains("|---|---|")); - assert!(text.contains("Doc | Highlight")); - assert!(text.contains("*Priority Agents* | Priority Agents")); - } - #[test] fn dm_final_reply_omits_thread_when_absent() { let view = FinalReplyView { From e2e33afd3dae21aafb18456d877643068798b247 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Tue, 16 Jun 2026 01:23:46 +0300 Subject: [PATCH 5/6] fix(slack): address outbound delivery review feedback --- crates/ironclaw_reborn_composition/src/lib.rs | 1 + .../outbound_delivery_capability_surface.rs | 197 +++++++++++++++ .../src/runtime/local_dev.rs | 4 +- .../runtime/local_dev/outbound_delivery.rs | 225 ++++++------------ .../src/slack_host_beta.rs | 29 ++- .../ironclaw_slack_v2_adapter/src/adapter.rs | 62 +++++ .../ironclaw_slack_v2_adapter/src/delivery.rs | 2 +- .../ironclaw_slack_v2_adapter/src/mrkdwn.rs | 89 +++++-- 8 files changed, 421 insertions(+), 188 deletions(-) create mode 100644 crates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rs diff --git a/crates/ironclaw_reborn_composition/src/lib.rs b/crates/ironclaw_reborn_composition/src/lib.rs index cd26ee0dcfc..5edb5febb86 100644 --- a/crates/ironclaw_reborn_composition/src/lib.rs +++ b/crates/ironclaw_reborn_composition/src/lib.rs @@ -74,6 +74,7 @@ mod oauth_provider_client; #[cfg(feature = "openai-compat-beta")] mod openai_compat_serve; mod operator_logs; +mod outbound_delivery_capability_surface; mod outbound_preferences; mod product_auth_durable; mod product_auth_providers; diff --git a/crates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rs b/crates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rs new file mode 100644 index 00000000000..f767103f7b6 --- /dev/null +++ b/crates/ironclaw_reborn_composition/src/outbound_delivery_capability_surface.rs @@ -0,0 +1,197 @@ +use ironclaw_product_workflow::{ + OutboundPreferencesProductFacade, RebornOutboundDeliveryTargetId, + RebornOutboundDeliveryTargetListResponse, RebornOutboundPreferencesResponse, + RebornServicesError, RebornSetOutboundPreferencesRequest, WebUiAuthenticatedCaller, +}; +use thiserror::Error; + +pub(crate) const OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID: &str = + "builtin.outbound_delivery_targets_list"; +pub(crate) const OUTBOUND_DELIVERY_TARGETS_LIST_PROVIDER_TOOL_NAME: &str = + "builtin__outbound_delivery_targets_list"; +pub(crate) const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin__trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; + +pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID: &str = + "builtin.outbound_delivery_target_set"; +pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_PROVIDER_TOOL_NAME: &str = + "builtin__outbound_delivery_target_set"; +pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply outbound delivery target, such as a Slack DM or Slack channel, to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger. Approval may be required before the preference is changed."; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct OutboundDeliveryTargetsListInput { + channel: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct OutboundDeliveryTargetSetInput { + target_id: RebornOutboundDeliveryTargetId, +} + +impl OutboundDeliveryTargetSetInput { + pub(crate) fn target_id(&self) -> &RebornOutboundDeliveryTargetId { + &self.target_id + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Error)] +#[error("{reason}")] +pub(crate) struct OutboundDeliveryCapabilityInputError { + reason: String, +} + +impl OutboundDeliveryCapabilityInputError { + fn new(reason: impl Into) -> Self { + Self { + reason: reason.into(), + } + } +} + +pub(crate) async fn list_outbound_delivery_targets_for_model( + facade: &dyn OutboundPreferencesProductFacade, + caller: WebUiAuthenticatedCaller, + input: OutboundDeliveryTargetsListInput, +) -> Result { + let mut response = facade.list_outbound_delivery_targets(caller).await?; + if let Some(channel_filter) = input.channel { + response.targets.retain(|option| { + option + .target + .channel + .as_str() + .eq_ignore_ascii_case(channel_filter.as_str()) + }); + } + Ok(response) +} + +pub(crate) async fn set_outbound_delivery_target_for_model( + facade: &dyn OutboundPreferencesProductFacade, + caller: WebUiAuthenticatedCaller, + input: OutboundDeliveryTargetSetInput, +) -> Result { + facade + .set_outbound_preferences( + caller, + RebornSetOutboundPreferencesRequest { + final_reply_target_id: Some(input.target_id), + }, + ) + .await +} + +pub(crate) fn outbound_delivery_targets_list_input_schema() -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "channel": { + "type": "string", + "description": "Optional product/channel filter such as slack." + } + }, + "additionalProperties": false + }) +} + +pub(crate) fn outbound_delivery_target_set_input_schema() -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "target_id": { + "type": "string", + "description": "Target id returned by builtin__outbound_delivery_targets_list." + } + }, + "required": ["target_id"], + "additionalProperties": false + }) +} + +pub(crate) fn parse_outbound_delivery_targets_list_input( + input: &serde_json::Value, +) -> Result { + let input = input_object(input, "outbound delivery target list", &["channel"])?; + let channel = match input.get("channel") { + None => None, + Some(value) => Some( + value + .as_str() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) + .ok_or_else(|| { + OutboundDeliveryCapabilityInputError::new( + "outbound delivery target list channel must be a non-empty string", + ) + })?, + ), + }; + Ok(OutboundDeliveryTargetsListInput { channel }) +} + +pub(crate) fn parse_outbound_delivery_target_set_input( + input: &serde_json::Value, +) -> Result { + let input = input_object(input, "outbound delivery target set", &["target_id"])?; + let value = input + .get("target_id") + .and_then(serde_json::Value::as_str) + .ok_or_else(|| { + OutboundDeliveryCapabilityInputError::new( + "outbound delivery target set target_id must be a string", + ) + })?; + let target_id = RebornOutboundDeliveryTargetId::new(value).map_err(|reason| { + OutboundDeliveryCapabilityInputError::new(format!( + "outbound delivery target set target_id is invalid: {reason}" + )) + })?; + Ok(OutboundDeliveryTargetSetInput { target_id }) +} + +fn input_object<'a>( + input: &'a serde_json::Value, + capability_name: &'static str, + allowed_fields: &[&str], +) -> Result<&'a serde_json::Map, OutboundDeliveryCapabilityInputError> { + let object = input.as_object().ok_or_else(|| { + OutboundDeliveryCapabilityInputError::new(format!( + "{capability_name} input must be an object" + )) + })?; + if let Some(field) = object + .keys() + .find(|field| !allowed_fields.contains(&field.as_str())) + { + return Err(OutboundDeliveryCapabilityInputError::new(format!( + "{capability_name} input contains unsupported field `{field}`" + ))); + } + Ok(object) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn list_input_rejects_unknown_fields() { + let err = parse_outbound_delivery_targets_list_input(&serde_json::json!({ + "channel": "slack", + "extra": true + })) + .expect_err("unknown field should fail"); + + assert!(err.to_string().contains("unsupported field `extra`")); + } + + #[test] + fn set_input_validates_target_id_shape() { + let err = parse_outbound_delivery_target_set_input(&serde_json::json!({ + "target_id": "bad\nid" + })) + .expect_err("invalid target id should fail"); + + assert!(err.to_string().contains("target_id is invalid")); + } +} diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev.rs index ee29b8be5bc..3708dbd2929 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev.rs @@ -55,11 +55,11 @@ mod skill_activation; mod surface_disclosure; mod synthetic_capability; -use extension_surface::{LocalDevExtensionSurface, LocalDevExtensionSurfaceSource}; #[cfg(test)] -pub(crate) use outbound_delivery::{ +pub(crate) use crate::outbound_delivery_capability_surface::{ OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID, OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID, }; +use extension_surface::{LocalDevExtensionSurface, LocalDevExtensionSurfaceSource}; use refreshing_capability_port::{ RefreshingLocalDevCapabilityPortConfig, create_refreshing_local_dev_capability_port, }; diff --git a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs index e123058c692..1b5757e5e11 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/local_dev/outbound_delivery.rs @@ -9,7 +9,7 @@ use ironclaw_host_api::{ use ironclaw_loop_support::{CapabilityResultWrite, loop_driver_execution_extension_id}; use ironclaw_product_workflow::{ OutboundPreferencesProductFacade, RebornOutboundDeliveryTargetId, RebornServicesError, - RebornServicesErrorCode, RebornSetOutboundPreferencesRequest, WebUiAuthenticatedCaller, + RebornServicesErrorCode, WebUiAuthenticatedCaller, }; use ironclaw_run_state::{ApprovalRequestStore, ApprovalStatus, RunStateError}; use ironclaw_turns::{ @@ -21,23 +21,20 @@ use ironclaw_turns::{ }, }; +use crate::outbound_delivery_capability_surface::{ + OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID, OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION, + OUTBOUND_DELIVERY_TARGET_SET_PROVIDER_TOOL_NAME, OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID, + OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION, OUTBOUND_DELIVERY_TARGETS_LIST_PROVIDER_TOOL_NAME, + OutboundDeliveryCapabilityInputError, list_outbound_delivery_targets_for_model, + outbound_delivery_target_set_input_schema, outbound_delivery_targets_list_input_schema, + parse_outbound_delivery_target_set_input, parse_outbound_delivery_targets_list_input, + set_outbound_delivery_target_for_model, +}; use crate::runtime::local_dev::synthetic_capability::{ LocalDevSyntheticCapability, LocalDevSyntheticCapabilityDescriptor, LocalDevSyntheticCapabilityHandler, LocalDevSyntheticCapabilityInvocation, }; -pub(crate) const OUTBOUND_DELIVERY_TARGETS_LIST_CAPABILITY_ID: &str = - "builtin.outbound_delivery_targets_list"; -const OUTBOUND_DELIVERY_TARGETS_LIST_PROVIDER_TOOL_NAME: &str = - "builtin__outbound_delivery_targets_list"; -const OUTBOUND_DELIVERY_TARGETS_LIST_DESCRIPTION: &str = "List available outbound delivery targets for final replies and routine/trigger results, such as Slack DMs or Slack channels. When the user asks to send routine or trigger results through Slack or another product/channel, call this before builtin__trigger_create and before saying a delivery product is unavailable or asking the user to reconnect it."; - -pub(crate) const OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID: &str = - "builtin.outbound_delivery_target_set"; -const OUTBOUND_DELIVERY_TARGET_SET_PROVIDER_TOOL_NAME: &str = - "builtin__outbound_delivery_target_set"; -const OUTBOUND_DELIVERY_TARGET_SET_DESCRIPTION: &str = "Set the current user's final-reply outbound delivery target, such as a Slack DM or Slack channel, to an id returned by builtin__outbound_delivery_targets_list. Use after the user asks to send replies or routine/trigger results through that product or channel, and before creating the routine or trigger. Approval may be required before the preference is changed."; - pub(super) fn outbound_delivery_capabilities( facade: Arc, fallback_user_id: UserId, @@ -89,29 +86,22 @@ impl LocalDevSyntheticCapabilityHandler for OutboundDeliveryTargetsListHandler { &self, arguments: &serde_json::Value, ) -> Result<(), AgentLoopHostError> { - parse_optional_channel(arguments).map(|_| ()) + parse_outbound_delivery_targets_list_input(arguments) + .map(|_| ()) + .map_err(input_error) } async fn invoke( &self, invocation: LocalDevSyntheticCapabilityInvocation, ) -> Result { - let channel_filter = parse_optional_channel(&invocation.input)?; + let input = + parse_outbound_delivery_targets_list_input(&invocation.input).map_err(input_error)?; let caller = caller_for_run(&invocation, &self.fallback_user_id); - let mut response = self - .facade - .list_outbound_delivery_targets(caller) - .await - .map_err(|error| outbound_delivery_host_error("list_targets", error))?; - if let Some(channel_filter) = channel_filter { - response.targets.retain(|option| { - option - .target - .channel - .as_str() - .eq_ignore_ascii_case(channel_filter.as_str()) - }); - } + let response = + list_outbound_delivery_targets_for_model(self.facade.as_ref(), caller, input) + .await + .map_err(|error| outbound_delivery_host_error("list_targets", error))?; let count = response.targets.len(); let output = serde_json::to_value(response).map_err(|error| { AgentLoopHostError::new( @@ -147,7 +137,9 @@ impl LocalDevSyntheticCapabilityHandler for OutboundDeliveryTargetSetHandler { &self, arguments: &serde_json::Value, ) -> Result<(), AgentLoopHostError> { - parse_target_id(arguments).map(|_| ()) + parse_outbound_delivery_target_set_input(arguments) + .map(|_| ()) + .map_err(input_error) } async fn invoke( @@ -162,14 +154,18 @@ impl LocalDevSyntheticCapabilityHandler for OutboundDeliveryTargetSetHandler { } let input = invocation_replay_input(&invocation).clone(); - let target_id = parse_target_id(&input)?; + let target_input = parse_outbound_delivery_target_set_input(&input).map_err(input_error)?; let approved_lease = if self.requires_approval { match invocation.request.approval_resume.clone() { Some(resume) => Some( self.verify_approved_resume(&invocation, &resume, &input) .await?, ), - None => return self.request_approval(&invocation, &input, &target_id).await, + None => { + return self + .request_approval(&invocation, &input, target_input.target_id()) + .await; + } } } else { if invocation.request.approval_resume.is_some() { @@ -181,7 +177,7 @@ impl LocalDevSyntheticCapabilityHandler for OutboundDeliveryTargetSetHandler { None }; - let target_summary = target_id.as_str().to_string(); + let target_summary = target_input.target_id().as_str().to_string(); let caller = caller_for_run(&invocation, &self.fallback_user_id); if let Some(approved_lease) = approved_lease { self.capability_leases @@ -189,16 +185,10 @@ impl LocalDevSyntheticCapabilityHandler for OutboundDeliveryTargetSetHandler { .await .map_err(|error| approval_lease_error("consume_approval_lease", error))?; } - let response = self - .facade - .set_outbound_preferences( - caller, - RebornSetOutboundPreferencesRequest { - final_reply_target_id: Some(target_id), - }, - ) - .await - .map_err(|error| outbound_delivery_host_error("set_target", error))?; + let response = + set_outbound_delivery_target_for_model(self.facade.as_ref(), caller, target_input) + .await + .map_err(|error| outbound_delivery_host_error("set_target", error))?; let output = serde_json::to_value(response).map_err(|error| { AgentLoopHostError::new( AgentLoopHostErrorKind::Internal, @@ -442,95 +432,6 @@ fn effective_user_id(run_context: &LoopRunContext, fallback_user_id: &UserId) -> .unwrap_or_else(|| fallback_user_id.clone()) } -fn outbound_delivery_targets_list_input_schema() -> serde_json::Value { - serde_json::json!({ - "type": "object", - "properties": { - "channel": { - "type": "string", - "description": "Optional product/channel filter such as slack." - } - }, - "additionalProperties": false - }) -} - -fn outbound_delivery_target_set_input_schema() -> serde_json::Value { - serde_json::json!({ - "type": "object", - "properties": { - "target_id": { - "type": "string", - "description": "Target id returned by builtin__outbound_delivery_targets_list." - } - }, - "required": ["target_id"], - "additionalProperties": false - }) -} - -fn parse_optional_channel(input: &serde_json::Value) -> Result, AgentLoopHostError> { - let input = input_object(input, "outbound delivery target list", &["channel"])?; - match input.get("channel") { - None => Ok(None), - Some(value) => value - .as_str() - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(|value| Some(value.to_string())) - .ok_or_else(|| { - AgentLoopHostError::new( - AgentLoopHostErrorKind::InvalidInvocation, - "outbound delivery target list channel must be a non-empty string", - ) - }), - } -} - -fn parse_target_id( - input: &serde_json::Value, -) -> Result { - let input = input_object(input, "outbound delivery target set", &["target_id"])?; - let value = input - .get("target_id") - .and_then(serde_json::Value::as_str) - .ok_or_else(|| { - AgentLoopHostError::new( - AgentLoopHostErrorKind::InvalidInvocation, - "outbound delivery target set target_id must be a string", - ) - })?; - RebornOutboundDeliveryTargetId::new(value).map_err(|reason| { - AgentLoopHostError::new( - AgentLoopHostErrorKind::InvalidInvocation, - format!("outbound delivery target set target_id is invalid: {reason}"), - ) - }) -} - -fn input_object<'a>( - input: &'a serde_json::Value, - capability_name: &'static str, - allowed_fields: &[&str], -) -> Result<&'a serde_json::Map, AgentLoopHostError> { - let object = input.as_object().ok_or_else(|| { - AgentLoopHostError::new( - AgentLoopHostErrorKind::InvalidInvocation, - format!("{capability_name} input must be an object"), - ) - })?; - if let Some(field) = object - .keys() - .find(|field| !allowed_fields.contains(&field.as_str())) - { - return Err(AgentLoopHostError::new( - AgentLoopHostErrorKind::InvalidInvocation, - format!("{capability_name} input contains unsupported field `{field}`"), - )); - } - Ok(object) -} - fn outbound_delivery_target_set_capability_id() -> Result { CapabilityId::new(OUTBOUND_DELIVERY_TARGET_SET_CAPABILITY_ID).map_err(|error| { AgentLoopHostError::new( @@ -589,6 +490,10 @@ fn invocation_id_from_resume_token( }) } +fn input_error(error: OutboundDeliveryCapabilityInputError) -> AgentLoopHostError { + AgentLoopHostError::new(AgentLoopHostErrorKind::InvalidInvocation, error.to_string()) +} + fn outbound_delivery_host_error( operation: &'static str, error: RebornServicesError, @@ -654,51 +559,57 @@ mod tests { use super::*; #[test] - fn parse_optional_channel_rejects_empty_channel() { - let error = parse_optional_channel(&serde_json::json!({"channel": " "})) - .expect_err("empty channel should fail"); + fn parse_outbound_delivery_targets_list_input_rejects_empty_channel() { + let error = + parse_outbound_delivery_targets_list_input(&serde_json::json!({"channel": " "})) + .expect_err("empty channel should fail"); - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + assert!(error.to_string().contains("must be a non-empty string")); } #[test] - fn parse_optional_channel_rejects_non_object_input() { - let error = parse_optional_channel(&serde_json::Value::Null) + fn parse_outbound_delivery_targets_list_input_rejects_non_object_input() { + let error = parse_outbound_delivery_targets_list_input(&serde_json::Value::Null) .expect_err("non-object input should fail"); - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + assert!(error.to_string().contains("input must be an object")); } #[test] - fn parse_optional_channel_rejects_unknown_fields() { - let error = parse_optional_channel(&serde_json::json!({"unexpected": "value"})) - .expect_err("unknown fields should fail"); + fn parse_outbound_delivery_targets_list_input_rejects_unknown_fields() { + let error = + parse_outbound_delivery_targets_list_input(&serde_json::json!({"unexpected": "value"})) + .expect_err("unknown fields should fail"); - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + assert!(error.to_string().contains("unsupported field `unexpected`")); } #[test] - fn parse_target_id_requires_target_id() { - let error = - parse_target_id(&serde_json::json!({})).expect_err("missing target id should fail"); + fn parse_outbound_delivery_target_set_input_requires_target_id() { + let error = parse_outbound_delivery_target_set_input(&serde_json::json!({})) + .expect_err("missing target id should fail"); - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + assert!(error.to_string().contains("target_id must be a string")); } #[test] - fn parse_target_id_rejects_malformed_target_id() { - let error = parse_target_id(&serde_json::json!({"target_id": "bad\nid"})) - .expect_err("malformed target id should fail"); + fn parse_outbound_delivery_target_set_input_rejects_malformed_target_id() { + let error = parse_outbound_delivery_target_set_input(&serde_json::json!({ + "target_id": "bad\nid" + })) + .expect_err("malformed target id should fail"); - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + assert!(error.to_string().contains("target_id is invalid")); } #[test] - fn parse_target_id_rejects_unknown_fields() { - let error = - parse_target_id(&serde_json::json!({"target_id": "slack:test", "unexpected": "value"})) - .expect_err("unknown fields should fail"); - - assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation); + fn parse_outbound_delivery_target_set_input_rejects_unknown_fields() { + let error = parse_outbound_delivery_target_set_input(&serde_json::json!({ + "target_id": "slack:test", + "unexpected": "value" + })) + .expect_err("unknown fields should fail"); + + assert!(error.to_string().contains("unsupported field `unexpected`")); } } diff --git a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs index bf9d7bf17e9..a588e43b0dc 100644 --- a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs +++ b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs @@ -478,10 +478,9 @@ pub fn build_slack_host_beta_mounts( )? { OutboundDeliveryTargetRegistrationOutcome::Registered => {} OutboundDeliveryTargetRegistrationOutcome::Replaced => { - tracing::debug!( - target = "ironclaw::reborn::slack_host_beta", - "Slack outbound delivery target provider replaced an existing registration" - ); + return Err(SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { + reason: "Slack outbound delivery target provider is already registered; replacement would diverge from the first-writer trigger delivery hook".to_string(), + }); } } Ok(SlackHostBetaMounts { @@ -1891,6 +1890,28 @@ mod tests { runtime.shutdown().await.expect("runtime shuts down"); } + #[tokio::test] + async fn build_slack_host_beta_mounts_rejects_outbound_target_provider_replacement() { + let (runtime, _root) = runtime().await; + let _mounts = build_slack_host_beta_mounts(&runtime, config()).expect("first mount builds"); + + let error = match build_slack_host_beta_mounts(&runtime, config()) { + Ok(_) => panic!("second Slack mount must not replace outbound provider"), + Err(error) => error, + }; + + assert!( + matches!( + error, + SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { ref reason } + if reason.contains("already registered") + ), + "unexpected replacement error: {error:?}" + ); + + runtime.shutdown().await.expect("runtime shuts down"); + } + #[tokio::test] async fn slack_host_beta_stored_and_static_routes_appear_without_duplicates() { let (runtime, _root) = runtime().await; diff --git a/crates/ironclaw_slack_v2_adapter/src/adapter.rs b/crates/ironclaw_slack_v2_adapter/src/adapter.rs index dd3d1e17386..29c40512d67 100644 --- a/crates/ironclaw_slack_v2_adapter/src/adapter.rs +++ b/crates/ironclaw_slack_v2_adapter/src/adapter.rs @@ -187,13 +187,33 @@ impl ProductAdapter for SlackV2Adapter { } }; + let mut delivered_any_part = false; for request in requests { if let Err(error) = send_slack_post_message(egress, request, attempt_id, &target_binding, run_id).await { + if delivered_any_part + && matches!(&error.status, DeliveryStatus::FailedRetryable { .. }) + { + let reason = RedactedString::new( + "partial Slack multipart delivery; suppressing retry to avoid duplicate parts", + ); + record_status( + delivery_sink, + DeliveryStatus::FailedPermanent { + attempt_id, + target: target_binding.clone(), + run_id, + reason: reason.clone(), + }, + ) + .await; + return Err(ProductAdapterError::EgressDenied { reason }); + } record_status(delivery_sink, error.status).await; return Err(error.adapter_error); } + delivered_any_part = true; } record_status( @@ -474,6 +494,47 @@ mod tests { )); } + #[tokio::test] + async fn multipart_final_reply_suppresses_retry_after_partial_delivery() { + use ironclaw_product_adapters::ProtocolHttpEgressError; + + let adapter = SlackV2Adapter::new(config()); + let egress = FakeProtocolHttpEgress::new(vec![SLACK_API_HOST.to_string()]); + egress.allow_credential_handle("slack_bot_token"); + egress.program_response( + SLACK_API_HOST, + Ok(ironclaw_product_adapters::EgressResponse::new( + 200, + br#"{"ok":true}"#.to_vec(), + )), + ); + egress.program_response(SLACK_API_HOST, Err(ProtocolHttpEgressError::Timeout)); + let sink = FakeOutboundDeliverySink::new(); + + let err = adapter + .render_outbound( + envelope(final_reply_payload(&"a".repeat(35_050))), + &egress, + &sink, + ) + .await + .expect_err("second multipart send should fail"); + + assert!( + matches!(err, ProductAdapterError::EgressDenied { .. }), + "partial multipart failure must not be retryable, got {err:?}" + ); + assert_eq!(egress.calls().len(), 2); + assert!( + matches!( + sink.statuses().as_slice(), + [DeliveryStatus::FailedPermanent { .. }] + ), + "partial multipart failure must record permanent status, got {:?}", + sink.statuses() + ); + } + #[tokio::test] async fn render_outbound_rejects_mismatched_envelope_ids_without_egress() { let adapter = SlackV2Adapter::new(config()); @@ -559,6 +620,7 @@ mod tests { #[tokio::test] async fn render_outbound_records_status_for_slack_http_failures() { for (status, expected) in [ + (408, "retryable"), (429, "retryable"), (500, "retryable"), (401, "unauthorized"), diff --git a/crates/ironclaw_slack_v2_adapter/src/delivery.rs b/crates/ironclaw_slack_v2_adapter/src/delivery.rs index 1adb457af83..8fcd7785d6f 100644 --- a/crates/ironclaw_slack_v2_adapter/src/delivery.rs +++ b/crates/ironclaw_slack_v2_adapter/src/delivery.rs @@ -155,7 +155,7 @@ impl SlackDeliveryFailureKind { } fn from_http_status(status: u16) -> Self { - if status >= 500 || status == 429 { + if status >= 500 || status == 429 || status == 408 { Self::Retryable } else if status == 401 || status == 403 { Self::Unauthorized diff --git a/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs b/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs index 284fb6400bf..aca26f4f176 100644 --- a/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs +++ b/crates/ironclaw_slack_v2_adapter/src/mrkdwn.rs @@ -302,11 +302,23 @@ fn strip_heading_marker(line: &str) -> &str { if !trimmed.starts_with('#') { return line; } - let hash_count = trimmed.chars().take_while(|ch| *ch == '#').count(); + let mut hash_count = 0usize; + let mut rest_start = None; + for (index, ch) in trimmed.char_indices() { + if ch == '#' { + hash_count += 1; + continue; + } + rest_start = Some(index); + break; + } if !(1..=6).contains(&hash_count) { return line; } - let rest = &trimmed[hash_count..]; + let Some(rest_start) = rest_start else { + return line; + }; + let rest = &trimmed[rest_start..]; let Some(rest) = rest.strip_prefix(' ') else { return line; }; @@ -315,39 +327,59 @@ fn strip_heading_marker(line: &str) -> &str { fn convert_markdown_links(line: &str) -> String { let mut out = String::with_capacity(line.len()); - let bytes = line.as_bytes(); let mut index = 0; while index < line.len() { - if bytes[index] == b'[' - && let Some(label_end_rel) = line[index + 1..].find(']') - { - let label_end = index + 1 + label_end_rel; - if line[label_end..].starts_with("](") - && let Some(url_end_rel) = line[label_end + 2..].find(')') - { - let label = &line[index + 1..label_end]; - let url_end = label_end + 2 + url_end_rel; - let url = &line[label_end + 2..url_end]; - if is_safe_slack_link_url(url) { - out.push('<'); - out.push_str(url); - out.push('|'); - out.push_str(label); - out.push('>'); - index = url_end + 1; - continue; - } - } + if !line.is_char_boundary(index) { + break; } - let Some(ch) = line[index..].chars().next() else { + let Some((_, ch)) = line[index..].char_indices().next() else { break; }; + if ch == '[' + && let Some((next_index, label, url)) = markdown_link_at(line, index) + && is_safe_slack_link_url(url) + { + out.push('<'); + out.push_str(url); + out.push('|'); + out.push_str(label); + out.push('>'); + index = next_index; + continue; + } out.push(ch); index += ch.len_utf8(); } out } +fn markdown_link_at(line: &str, start: usize) -> Option<(usize, &str, &str)> { + if !line.is_char_boundary(start) { + return None; + } + if line[start..].char_indices().next()?.1 != '[' { + return None; + } + let label_start = start + '['.len_utf8(); + let label_end = line[label_start..] + .char_indices() + .find_map(|(index, ch)| (ch == ']').then_some(label_start + index))?; + let after_label = &line[label_end..]; + if !after_label.starts_with("](") { + return None; + } + let url_start = label_end + "](".len(); + let url_end = line[url_start..] + .char_indices() + .find_map(|(index, ch)| (ch == ')').then_some(url_start + index))?; + let next_index = url_end + ')'.len_utf8(); + Some(( + next_index, + &line[label_start..label_end], + &line[url_start..url_end], + )) +} + fn is_safe_slack_link_url(url: &str) -> bool { url.starts_with("http://") || url.starts_with("https://") } @@ -406,6 +438,15 @@ mod tests { assert!(text.contains("*Highlight:* Priority Agents")); } + #[test] + fn unicode_markdown_renders_without_byte_boundary_slicing() { + let text = render_slack_mrkdwn("### Résumé\nПривет [世界](https://example.com/路径)"); + + assert!(text.contains("Résumé")); + assert!(!text.contains("###")); + assert!(text.contains("Привет ")); + } + #[test] fn issue_table_renders_as_slack_bullets_instead_of_raw_pipes() { let text = render_slack_mrkdwn( From 2877fac81483a8392f721e97fe85b25db8880a51 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Tue, 16 Jun 2026 02:11:52 +0300 Subject: [PATCH 6/6] fix(slack): allow idempotent host remounts --- .../src/outbound_preferences.rs | 18 +++ .../src/runtime.rs | 25 +++ .../src/slack_host_beta.rs | 152 +++++++++++++++++- 3 files changed, 188 insertions(+), 7 deletions(-) diff --git a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs index 9561ea33c3a..573c3c8c958 100644 --- a/crates/ironclaw_reborn_composition/src/outbound_preferences.rs +++ b/crates/ironclaw_reborn_composition/src/outbound_preferences.rs @@ -144,6 +144,24 @@ impl MutableOutboundDeliveryTargetRegistry { Ok(outcome) } + #[cfg_attr(not(feature = "slack-v2-host-beta"), allow(dead_code))] + pub(crate) fn contains_provider_key( + &self, + provider_key: &str, + ) -> Result { + self.providers + .read() + .map(|providers| providers.contains_key(provider_key)) + .map_err(|error| { + tracing::debug!( + target = "ironclaw::reborn::outbound_preferences", + error = ?error, + "outbound target registry read lock failed" + ); + outbound_target_registry_error() + }) + } + fn providers( &self, ) -> Result>, RebornServicesError> { diff --git a/crates/ironclaw_reborn_composition/src/runtime.rs b/crates/ironclaw_reborn_composition/src/runtime.rs index fa3a9b3fc40..6e1f8ff0818 100644 --- a/crates/ironclaw_reborn_composition/src/runtime.rs +++ b/crates/ironclaw_reborn_composition/src/runtime.rs @@ -1110,6 +1110,24 @@ impl RebornRuntime { }) } + #[cfg_attr(not(feature = "slack-v2-host-beta"), allow(dead_code))] + pub(crate) fn outbound_delivery_target_provider_key_registered( + &self, + provider_key: &str, + ) -> Result { + let Some(registry) = self.outbound_delivery_target_registry.as_ref() else { + return Err(RebornRuntimeError::InvalidArgument { + reason: "outbound delivery target registry unavailable for this runtime" + .to_string(), + }); + }; + registry + .contains_provider_key(provider_key) + .map_err(|error| RebornRuntimeError::InvalidArgument { + reason: format!("outbound delivery target provider lookup failed: {error}"), + }) + } + #[cfg(feature = "slack-v2-host-beta")] pub(crate) fn auth_challenge_provider(&self) -> Option> { self.services @@ -1145,6 +1163,13 @@ impl RebornRuntime { } } + #[cfg(feature = "slack-v2-host-beta")] + pub(crate) fn trigger_post_submit_hook_is_set(&self) -> bool { + self.post_submit_hook_slot + .as_ref() + .is_some_and(|slot| slot.get().is_some()) + } + #[cfg(test)] fn webui_approval_audit_sink(&self) -> Arc { self.approval_audit_sink.clone() diff --git a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs index a588e43b0dc..490c515b649 100644 --- a/crates/ironclaw_reborn_composition/src/slack_host_beta.rs +++ b/crates/ironclaw_reborn_composition/src/slack_host_beta.rs @@ -34,6 +34,7 @@ use ironclaw_wasm_product_adapters::{ WebhookAuth, }; use secrecy::{ExposeSecret, SecretString}; +use sha2::{Digest, Sha256}; use thiserror::Error; use crate::RebornRuntime; @@ -78,6 +79,7 @@ const SLACK_WEBHOOK_WORKFLOW_TIMEOUT: Duration = Duration::from_secs(2); const SLACK_MAX_IN_FLIGHT_WEBHOOKS: usize = 64; const SLACK_IDEMPOTENCY_LEDGER_SETTLED_LIMIT: usize = 10_000; const SLACK_IDEMPOTENCY_LEDGER_PRUNE_INTERVAL: usize = 1_000; +const SLACK_OUTBOUND_PROVIDER_KEY_PREFIX: &str = "slack-v2-host-beta"; struct NoopSlackDeliverySink; @@ -237,6 +239,103 @@ impl std::fmt::Debug for SlackHostBetaConfig { } } +fn slack_outbound_delivery_target_provider_key(config: &SlackHostBetaConfig) -> String { + let mut hasher = Sha256::new(); + hash_slack_mount_field(&mut hasher, config.tenant_id.as_str()); + hash_slack_mount_field(&mut hasher, config.agent_id.as_str()); + hash_slack_mount_field( + &mut hasher, + config.project_id.as_ref().map_or("", ProjectId::as_str), + ); + hash_slack_mount_field(&mut hasher, config.installation_id.as_str()); + hash_slack_mount_field(&mut hasher, config.team_id.as_str()); + hash_slack_installation_selector(&mut hasher, &config.installation_selector); + hash_slack_mount_field( + &mut hasher, + config.slack_actor.as_ref().map_or("", ExternalActorRef::id), + ); + hash_slack_mount_field(&mut hasher, config.user_id.as_str()); + hash_slack_mount_field( + &mut hasher, + config + .shared_subject_user_id + .as_ref() + .map_or("", UserId::as_str), + ); + for route in &config.channel_routes { + hash_slack_mount_field(&mut hasher, &route.channel_id); + hash_slack_mount_field(&mut hasher, route.subject_user_id.as_str()); + } + hash_slack_mount_field(&mut hasher, config.signing_secret.expose_secret()); + hash_slack_mount_field(&mut hasher, config.bot_token.expose_secret()); + + let digest = hasher.finalize(); + let mut suffix = String::with_capacity(64); + for byte in digest { + use std::fmt::Write as _; + let _ = write!(&mut suffix, "{byte:02x}"); + } + format!("{SLACK_OUTBOUND_PROVIDER_KEY_PREFIX}:{suffix}") +} + +fn hash_slack_installation_selector(hasher: &mut Sha256, selector: &SlackInstallationSelector) { + match selector { + SlackInstallationSelector::Team { team_id } => { + hash_slack_mount_field(hasher, "team"); + hash_slack_mount_field(hasher, team_id.as_str()); + } + SlackInstallationSelector::AppTeam { + api_app_id, + team_id, + } => { + hash_slack_mount_field(hasher, "app_team"); + hash_slack_mount_field(hasher, api_app_id.as_str()); + hash_slack_mount_field(hasher, team_id.as_str()); + } + SlackInstallationSelector::EnterpriseTeam { + enterprise_id, + team_id, + } => { + hash_slack_mount_field(hasher, "enterprise_team"); + hash_slack_mount_field(hasher, enterprise_id.as_str()); + hash_slack_mount_field(hasher, team_id.as_str()); + } + SlackInstallationSelector::InstallUser { + team_id, + install_user_id, + } => { + hash_slack_mount_field(hasher, "install_user"); + hash_slack_mount_field(hasher, team_id.as_str()); + hash_slack_mount_field(hasher, install_user_id.as_str()); + } + SlackInstallationSelector::EnterpriseInstallUser { + enterprise_id, + team_id, + install_user_id, + } => { + hash_slack_mount_field(hasher, "enterprise_install_user"); + hash_slack_mount_field(hasher, enterprise_id.as_str()); + hash_slack_mount_field(hasher, team_id.as_str()); + hash_slack_mount_field(hasher, install_user_id.as_str()); + } + SlackInstallationSelector::AppInstallUser { + api_app_id, + team_id, + install_user_id, + } => { + hash_slack_mount_field(hasher, "app_install_user"); + hash_slack_mount_field(hasher, api_app_id.as_str()); + hash_slack_mount_field(hasher, team_id.as_str()); + hash_slack_mount_field(hasher, install_user_id.as_str()); + } + } +} + +fn hash_slack_mount_field(hasher: &mut Sha256, value: &str) { + hasher.update(value.len().to_le_bytes()); + hasher.update(value.as_bytes()); +} + #[derive(Debug, Error)] pub enum SlackHostBetaBuildError { #[error("Slack host-beta requires local runtime HTTP egress")] @@ -422,6 +521,15 @@ pub fn build_slack_host_beta_mounts( ) .with_allowed_subject_user_ids(allowed_route_subjects); + let outbound_delivery_provider_key = slack_outbound_delivery_target_provider_key(&config); + let outbound_delivery_provider_already_registered = runtime + .outbound_delivery_target_provider_key_registered(&outbound_delivery_provider_key) + .map_err( + |error| SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { + reason: error.to_string(), + }, + )?; + // Wire the triggered-run delivery hook. The delivery store comes from the // composition-owned outbound store, shared with preferences so the same // backing tree is used for all outbound roles. `set_trigger_post_submit_hook` @@ -432,7 +540,15 @@ pub fn build_slack_host_beta_mounts( Arc::clone(&local_runtime.triggered_run_delivery); match build_triggered_run_delivery_hook(runtime, &config, delivery_store) { Ok(hook) => { - runtime.set_trigger_post_submit_hook(hook); + let hook_set = runtime.set_trigger_post_submit_hook(hook); + if !hook_set + && runtime.trigger_post_submit_hook_is_set() + && !outbound_delivery_provider_already_registered + { + return Err(SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { + reason: "Slack triggered delivery hook is already wired for a different Slack host config".to_string(), + }); + } } Err(err) => { tracing::warn!( @@ -466,9 +582,16 @@ pub fn build_slack_host_beta_mounts( channel_route_store, Arc::clone(&personal_dm_target_store), )); + if outbound_delivery_provider_already_registered { + return Ok(SlackHostBetaMounts { + events, + personal_binding_pairing: SlackPersonalBindingPairingRouteConfig::new(pairing), + channel_routes, + }); + } match runtime .register_outbound_delivery_target_provider( - SLACK_V2_ADAPTER_ID, + outbound_delivery_provider_key, Arc::clone(&outbound_delivery_target_provider), ) .map_err( @@ -479,7 +602,7 @@ pub fn build_slack_host_beta_mounts( OutboundDeliveryTargetRegistrationOutcome::Registered => {} OutboundDeliveryTargetRegistrationOutcome::Replaced => { return Err(SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { - reason: "Slack outbound delivery target provider is already registered; replacement would diverge from the first-writer trigger delivery hook".to_string(), + reason: "Slack outbound delivery target provider was concurrently registered; replacement would diverge from the first-writer trigger delivery hook".to_string(), }); } } @@ -1891,12 +2014,27 @@ mod tests { } #[tokio::test] - async fn build_slack_host_beta_mounts_rejects_outbound_target_provider_replacement() { + async fn build_slack_host_beta_mounts_allows_same_config_rebuild_without_replacement() { let (runtime, _root) = runtime().await; let _mounts = build_slack_host_beta_mounts(&runtime, config()).expect("first mount builds"); - let error = match build_slack_host_beta_mounts(&runtime, config()) { - Ok(_) => panic!("second Slack mount must not replace outbound provider"), + build_slack_host_beta_mounts(&runtime, config()).expect("same-config rebuild builds"); + + runtime.shutdown().await.expect("runtime shuts down"); + } + + #[tokio::test] + async fn build_slack_host_beta_mounts_rejects_different_config_after_trigger_hook_wired() { + let (runtime, _root) = runtime_with_trigger_poller().await; + let _mounts = build_slack_host_beta_mounts(&runtime, config()).expect("first mount builds"); + let mut different_config = config(); + different_config.channel_routes = vec![SlackHostBetaChannelRoute::new( + "C1HOST", + UserId::new(SHARED_SUBJECT).expect("shared subject"), + )]; + + let error = match build_slack_host_beta_mounts(&runtime, different_config) { + Ok(_) => panic!("different Slack mount must not replace outbound provider"), Err(error) => error, }; @@ -1904,7 +2042,7 @@ mod tests { matches!( error, SlackHostBetaBuildError::OutboundDeliveryTargetRegistration { ref reason } - if reason.contains("already registered") + if reason.contains("different Slack host config") ), "unexpected replacement error: {error:?}" );