Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 104 additions & 1 deletion crates/ironclaw_reborn_composition/src/communication_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,21 @@ async fn fetch_communication_context(
actor: Option<TurnActor>,
) -> Option<CommunicationRuntimeContext> {
let actor = actor?;
// Key outbound preferences by the run's *owner*, not the acting principal.
// Product inbound and trusted-trigger runs can carry an explicit thread
// owner (subject/creator) that differs from `actor`; the owner is who the
// stored delivery preference belongs to. Fall back to the actor when no
// explicit owner is set, matching `TurnScope::to_resource_scope`'s owner
// resolution. Without this, shared/channel inbound and trigger runs would
// render the actor's delivery target (or "none set") instead of the
// owner's, producing wrong delivery guidance.
let owner_user_id = scope
.explicit_owner_user_id()
.cloned()
.unwrap_or_else(|| actor.user_id.clone());
let caller = WebUiAuthenticatedCaller::new(
scope.tenant_id.clone(),
actor.user_id.clone(),
owner_user_id,
scope.agent_id.clone(),
scope.project_id.clone(),
);
Comment on lines +92 to 101

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since scope and actor are passed by value and consumed within fetch_communication_context, we can avoid cloning tenant_id, agent_id, project_id, and user_id by matching directly on scope.thread_owner and passing the fields of scope by value to WebUiAuthenticatedCaller::new. This eliminates four unnecessary allocations/clones on a hot path.

Additionally, matching exhaustively on TurnThreadOwner without a wildcard _ arm ensures compile-time safety if new variants are added in the future.

Suggested change
let owner_user_id = scope
.explicit_owner_user_id()
.cloned()
.unwrap_or_else(|| actor.user_id.clone());
let caller = WebUiAuthenticatedCaller::new(
scope.tenant_id.clone(),
actor.user_id.clone(),
owner_user_id,
scope.agent_id.clone(),
scope.project_id.clone(),
);
let owner_user_id = match scope.thread_owner {
ironclaw_turns::scope::TurnThreadOwner::ExplicitUser { owner_user_id } => owner_user_id,
ironclaw_turns::scope::TurnThreadOwner::ActorFallback | ironclaw_turns::scope::TurnThreadOwner::Ownerless => actor.user_id,
};
let caller = WebUiAuthenticatedCaller::new(
scope.tenant_id,
owner_user_id,
scope.agent_id,
scope.project_id,
);
References
  1. In functions that map one enum to another, use an exhaustive match statement instead of a wildcard _ arm. This forces a compile-time error when new variants are added, ensuring all cases are explicitly handled.

Expand Down Expand Up @@ -493,6 +505,97 @@ mod tests {
assert!(result.is_none(), "actor None must return None");
}

// --- Tests: preference lookup is keyed by the run owner, not the actor ---

/// Preferences facade that records the `user_id` of the caller it received,
/// so tests can assert the provider keys the lookup by the run owner rather
/// than the acting principal.
struct CaptureCallerPreferencesFacade {
seen_user_id: Arc<std::sync::Mutex<Option<String>>>,
}

#[async_trait]
impl OutboundPreferencesProductFacade for CaptureCallerPreferencesFacade {
async fn get_outbound_preferences(
&self,
caller: WebUiAuthenticatedCaller,
) -> Result<RebornOutboundPreferencesResponse, RebornServicesError> {
*self.seen_user_id.lock().expect("lock") = Some(caller.user_id.as_str().to_string());
Ok(RebornOutboundPreferencesResponse::default())
}

async fn set_outbound_preferences(
&self,
_caller: WebUiAuthenticatedCaller,
_request: RebornSetOutboundPreferencesRequest,
) -> Result<RebornOutboundPreferencesResponse, RebornServicesError> {
Ok(RebornOutboundPreferencesResponse::default())
}

async fn list_outbound_delivery_targets(
&self,
_caller: WebUiAuthenticatedCaller,
) -> Result<RebornOutboundDeliveryTargetListResponse, RebornServicesError> {
Ok(RebornOutboundDeliveryTargetListResponse {
targets: Vec::new(),
next_cursor: None,
})
}
}

#[tokio::test]
async fn preferences_keyed_by_explicit_owner_not_actor() {
let seen_user_id = Arc::new(std::sync::Mutex::new(None));
let facade = CaptureCallerPreferencesFacade {
seen_user_id: Arc::clone(&seen_user_id),
};
let provider = RuntimeCommunicationContextProvider::new(Arc::new(facade));

// Scope owned by a subject/creator distinct from the acting principal
// (e.g. a trusted trigger or shared/channel inbound run).
let owned_scope = TurnScope::new_with_owner(
TenantId::new("tenant-test").unwrap(),
Some(AgentId::new("agent-test").unwrap()),
Some(ProjectId::new("project-test").unwrap()),
ironclaw_host_api::ThreadId::new("thread-test").unwrap(),
Some(UserId::new("owner-test").unwrap()),
);

provider
.begin_communication_context(owned_scope, Some(actor()))
.resolve(false)
.await
.expect("context");

assert_eq!(
seen_user_id.lock().expect("lock").as_deref(),
Some("owner-test"),
"preference lookup must be keyed by the explicit run owner, not the actor",
);
}

#[tokio::test]
async fn preferences_fall_back_to_actor_without_explicit_owner() {
let seen_user_id = Arc::new(std::sync::Mutex::new(None));
let facade = CaptureCallerPreferencesFacade {
seen_user_id: Arc::clone(&seen_user_id),
};
let provider = RuntimeCommunicationContextProvider::new(Arc::new(facade));

// `scope()` uses `TurnThreadOwner::ActorFallback` (no explicit owner).
provider
.begin_communication_context(scope(), Some(actor()))
.resolve(false)
.await
.expect("context");

assert_eq!(
seen_user_id.lock().expect("lock").as_deref(),
Some("user-test"),
"with no explicit owner the lookup must fall back to the actor",
);
}

// --- Tests: delivery target state branches ---

#[tokio::test]
Expand Down
32 changes: 32 additions & 0 deletions crates/ironclaw_turns/src/run_profile/runtime_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1112,4 +1112,36 @@ mod tests {
text.len()
);
}

// --- CommunicationContextFetch::resolve JoinError degradation ---

#[tokio::test]
async fn fetch_join_error_without_actor_resolves_to_none() {
// A task that panics yields a `JoinError`. With `actor_present = false`
// the slice is not applicable, so resolve must degrade to `None` rather
// than fabricating an `Unknown` communication slice for an actorless run.
let handle = tokio::spawn(async { panic!("simulated communication fetch failure") });
let fetch = CommunicationContextFetch::from_handle(handle, false);
let resolved = fetch.resolve(false).await;
assert!(
resolved.is_none(),
"actorless JoinError must degrade to None, got {resolved:?}"
);
}

#[tokio::test]
async fn fetch_join_error_with_actor_resolves_to_unknown() {
// With `actor_present = true` the same `JoinError` must degrade to a
// `Some(Unknown…)` slice so the actor-present / no-actor distinction is
// preserved on the failure path.
let handle = tokio::spawn(async { panic!("simulated communication fetch failure") });
let fetch = CommunicationContextFetch::from_handle(handle, true);
let resolved = fetch
.resolve(false)
.await
.expect("actor-present JoinError must degrade to Some(Unknown)");
assert_eq!(resolved.connected_channels, ConnectedChannelsState::Unknown);
assert_eq!(resolved.delivery_target, DeliveryTargetState::Unknown);
assert!(!resolved.delivery_tools_visible);
}
}
9 changes: 7 additions & 2 deletions docs/superpowers/plans/2026-06-13-product-context-factory.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,15 @@ pub enum TurnSurfaceType {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct RunOriginAdapter(String);

/// Mirrors `AdapterKind`'s validation bound in `ironclaw_conversations` so that
/// any valid `AdapterKind` always converts without silent narrowing. If
/// `AdapterKind`'s limit changes, update this constant to match.
const MAX_RUN_ORIGIN_ADAPTER_BYTES: usize = 512;

impl RunOriginAdapter {
pub fn new(value: impl Into<String>) -> Result<Self, crate::TurnError> {
let value = value.into();
if value.is_empty() || value.len() > 256 {
if value.is_empty() || value.len() > MAX_RUN_ORIGIN_ADAPTER_BYTES {
return Err(crate::TurnError::InvalidRunOriginAdapter);
}
Ok(Self(value))
Expand Down Expand Up @@ -139,7 +144,7 @@ pub struct ProductTurnContext {
- [ ] **Step 4: Add the error variant** — in `crates/ironclaw_turns/src/status.rs` (find the `TurnError` enum; if errors live elsewhere, grep `enum TurnError`), add:

```rust
#[error("invalid run-origin adapter: must be 1..=256 bytes")]
#[error("invalid run-origin adapter: must be 1..=512 bytes")]
InvalidRunOriginAdapter,
```

Expand Down
Loading