diff --git a/crates/ironclaw_host_runtime/src/credential_presence.rs b/crates/ironclaw_host_runtime/src/credential_presence.rs new file mode 100644 index 00000000000..310675b6672 --- /dev/null +++ b/crates/ironclaw_host_runtime/src/credential_presence.rs @@ -0,0 +1,354 @@ +//! Production [`CapabilityCredentialPresence`] composing the pre-flight +//! secret store and the product-auth account resolver behind a short-TTL +//! cache. +//! +//! Extracted from `production.rs` (issue #5416, Phase 2 review Fix D) to keep +//! that file from growing past its already-large size, and to co-locate the +//! presence check next to [`crate::credential_presence_cache`], which it is +//! tightly coupled to. + +use async_trait::async_trait; +use ironclaw_host_api::{CapabilityDescriptor, CredentialStageError, ResourceScope}; +use ironclaw_secrets::SecretStore; + +use crate::{ + credential_presence_cache::{ + CredentialOwnerScope, CredentialPresenceCache, CredentialPresenceKey, CredentialSetupKind, + }, + obligations::{ + RuntimeCredentialAccountRequest, RuntimeCredentialAccountResolver, secret_present, + }, + production::capability_credential_requirements, + surface::{CapabilityCredentialPresence, CredentialPresenceStatus}, +}; + +/// Mirrors [`crate::production::DefaultHostRuntime::credential_preflight_check`]'s +/// fail-open discipline: a backend error / indeterminate outcome must never +/// be reported as "missing" (that would burn a false-positive sign-in prompt +/// on the model-visible surface), so only conclusive results are cached and +/// only a conclusive `false` downgrades the surface. +pub(crate) struct ProductionCredentialPresence<'a> { + pub(crate) secret_store: Option<&'a dyn SecretStore>, + pub(crate) resolver: Option<&'a dyn RuntimeCredentialAccountResolver>, + pub(crate) cache: &'a CredentialPresenceCache, +} + +#[async_trait] +impl CapabilityCredentialPresence for ProductionCredentialPresence<'_> { + async fn required_credentials_present( + &self, + scope: &ResourceScope, + descriptor: &CapabilityDescriptor, + ) -> CredentialPresenceStatus { + let (required_secrets, credential_requirements) = + capability_credential_requirements(descriptor); + if required_secrets.is_empty() && credential_requirements.is_empty() { + return CredentialPresenceStatus::Present; + } + + let owner_scope = CredentialOwnerScope::from_scope(scope); + let mut any_missing = false; + let mut any_indeterminate = false; + + if !required_secrets.is_empty() { + match self.secret_store { + Some(store) => { + for handle in &required_secrets { + let key = + CredentialPresenceKey::Secret(owner_scope.clone(), handle.clone()); + if let Some(present) = self.cache.get(&key) { + any_missing |= !present; + continue; + } + match secret_present(store, scope, handle).await { + Ok(present) => { + self.cache.insert(key, present); + any_missing |= !present; + } + Err(error) => { + tracing::debug!( + secret_handle = handle.as_str(), + error = %error, + "credential presence: secret store metadata query failed; treating as indeterminate" + ); + any_indeterminate = true; // silent-ok: backend error must not report a missing credential; caller stays Available and the dispatch-time obligation check remains the backstop + } + } + } + } + None => { + any_indeterminate = true; // silent-ok: no secret store wired for this graph; dispatch-time obligation check remains the enforcing backstop + } + } + } + + for requirement in &credential_requirements { + // Scopes are sorted AND deduplicated before the key is built + // (mirrors `stable_auth_gate_id`'s scope-sort) so key equality + // does not depend on manifest declaration order or duplicate + // scope entries, and — critically — so a requirement's presence + // answer is never aliased onto a different requirement that + // shares provider/extension but requests different scopes + // (#5416 Phase 2 Fix B). The setup kind is part of the key too: + // `ManualToken` and `OAuth` select accounts differently even for + // an otherwise-identical requirement (see `CredentialSetupKind`). + let mut scopes = requirement.provider_scopes.clone(); + scopes.sort(); + scopes.dedup(); + let key = CredentialPresenceKey::ProductAuth( + owner_scope.clone(), + requirement.provider.clone(), + requirement.requester_extension.clone(), + CredentialSetupKind::from_setup(&requirement.setup), + scopes, + ); + if let Some(present) = self.cache.get(&key) { + any_missing |= !present; + continue; + } + let Some(resolver) = self.resolver else { + any_indeterminate = true; // silent-ok: no account resolver wired; a missing resolver is a wiring gap, not evidence the user lacks a credential + continue; + }; + // `account_configured` is side-effect-free (no token refresh, no + // network staging) — safe to call on every capability-surface + // render, unlike `resolve_access_secret` which performs an OAuth + // refresh round-trip (#5416 Phase 2 Fix A). + match resolver + .account_configured(RuntimeCredentialAccountRequest { + scope, + provider: &requirement.provider, + setup: &requirement.setup, + provider_scopes: &requirement.provider_scopes, + requester_extension: &requirement.requester_extension, + }) + .await + { + Ok(present) => { + self.cache.insert(key, present); + any_missing |= !present; + } + Err(CredentialStageError::Backend) => { + any_indeterminate = true; // silent-ok: internal staging failure is not attributable to the user's credentials + } + Err(CredentialStageError::AuthRequired) => { + // `account_configured`'s contract folds "no configured + // account" into `Ok(false)`; this arm only exists for + // match exhaustiveness. Treat defensively as + // indeterminate (fail open) rather than caching a + // possibly-wrong "missing" answer if a future impl + // violates the contract. + any_indeterminate = true; // silent-ok: contract-violation guard, not evidence of a missing user credential + } + } + } + + if any_missing { + CredentialPresenceStatus::Missing + } else if any_indeterminate { + CredentialPresenceStatus::Indeterminate + } else { + CredentialPresenceStatus::Present + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::obligations::RuntimeCredentialAccessSecret; + use ironclaw_host_api::{ + CapabilityId, EffectKind, ExtensionId, InvocationId, NetworkTargetPattern, PermissionMode, + RuntimeCredentialAccountProviderId, RuntimeCredentialAccountSetup, + RuntimeCredentialRequirement, RuntimeCredentialRequirementSource, RuntimeCredentialTarget, + RuntimeKind, SecretHandle, TrustClass, UserId, + }; + + /// A capability descriptor requiring a single product-auth credential + /// scoped to `scope`, owned by the `gmail` extension against provider + /// `google` — mirrors the real gmail manifest shape (multiple + /// capabilities under the same provider/extension with different + /// `provider_scopes`) used to reproduce the #5416 Phase 2 Fix B cache + /// aliasing bug. + fn descriptor_with_product_auth_scope(id: &str, scope: &str) -> CapabilityDescriptor { + descriptor_with_product_auth_scopes(id, &[scope]) + } + + /// Same as [`descriptor_with_product_auth_scope`] but accepts the full + /// `provider_scopes` list verbatim (including literal duplicates), so + /// tests can pin the cache key's canonicalization behavior. + fn descriptor_with_product_auth_scopes(id: &str, scopes: &[&str]) -> CapabilityDescriptor { + CapabilityDescriptor { + id: CapabilityId::new(id).unwrap(), + provider: ExtensionId::new("gmail").unwrap(), + runtime: RuntimeKind::Wasm, + trust_ceiling: TrustClass::UserTrusted, + description: "test capability".to_string(), + parameters_schema: serde_json::json!({}), + effects: vec![EffectKind::DispatchCapability], + default_permission: PermissionMode::Allow, + runtime_credentials: vec![RuntimeCredentialRequirement { + handle: SecretHandle::new("google_oauth_token").unwrap(), + source: RuntimeCredentialRequirementSource::ProductAuthAccount { + provider: RuntimeCredentialAccountProviderId::new("google").unwrap(), + setup: RuntimeCredentialAccountSetup::ManualToken, + }, + provider_scopes: scopes.iter().map(|scope| scope.to_string()).collect(), + audience: NetworkTargetPattern { + scheme: None, + host_pattern: "gmail.googleapis.com".to_string(), + port: None, + }, + target: RuntimeCredentialTarget::Header { + name: "Authorization".to_string(), + prefix: None, + }, + required: true, + }], + resource_profile: None, + } + } + + fn test_resource_scope() -> ResourceScope { + ResourceScope::local_default(UserId::new("user").unwrap(), InvocationId::new()).unwrap() + } + + /// Resolver whose presence answer depends on the requested provider + /// scopes — used to prove `ProductionCredentialPresence` does not alias + /// one scope's cached answer onto a different scope under the same + /// `(provider, requester_extension)`. + #[derive(Debug)] + struct ScopeGatedAccountResolver { + present_scope: String, + } + + #[async_trait] + impl RuntimeCredentialAccountResolver for ScopeGatedAccountResolver { + async fn resolve_access_secret( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + if request.provider_scopes.contains(&self.present_scope) { + Ok(RuntimeCredentialAccessSecret { + scope: request.scope.clone(), + handle: SecretHandle::new("google_oauth_token").unwrap(), + }) + } else { + Err(CredentialStageError::AuthRequired) + } + } + + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(request.provider_scopes.contains(&self.present_scope)) + } + } + + /// Resolver that always reports the account as configured, counting how + /// many times `account_configured` is actually invoked — used to prove a + /// second lookup hit the cache instead of the backend. + #[derive(Debug, Default)] + struct CountingAccountResolver { + calls: std::sync::atomic::AtomicUsize, + } + + #[async_trait] + impl RuntimeCredentialAccountResolver for CountingAccountResolver { + async fn resolve_access_secret( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(RuntimeCredentialAccessSecret { + scope: request.scope.clone(), + handle: SecretHandle::new("google_oauth_token").unwrap(), + }) + } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Ok(true) + } + } + + /// #5416 Phase 2 Fix B (BLOCKER) regression: two capabilities that share + /// `(provider, requester_extension)` but require different + /// `provider_scopes` (the real gmail manifest shape — gmail.readonly / + /// gmail.send / gmail.modify all under `provider="google"`, + /// `requester_extension="gmail"`) must get independently correct presence + /// answers within the same render. Before the fix, the cache key omitted + /// scopes, so checking the readonly capability first would cache `true` + /// under a key the send capability's lookup also hits — reporting the + /// send capability wrongly `Available` (or vice versa, depending on + /// check order). + #[tokio::test] + async fn required_credentials_present_does_not_alias_across_different_scopes() { + let cache = CredentialPresenceCache::new(); + let resolver = ScopeGatedAccountResolver { + present_scope: "gmail.readonly".to_string(), + }; + let presence = ProductionCredentialPresence { + secret_store: None, + resolver: Some(&resolver), + cache: &cache, + }; + let scope = test_resource_scope(); + let readonly = descriptor_with_product_auth_scope("gmail.readonly_cap", "gmail.readonly"); + let send = descriptor_with_product_auth_scope("gmail.send_cap", "gmail.send"); + + let readonly_present = presence + .required_credentials_present(&scope, &readonly) + .await; + let send_present = presence.required_credentials_present(&scope, &send).await; + + assert_eq!(readonly_present, CredentialPresenceStatus::Present); + assert_eq!( + send_present, + CredentialPresenceStatus::Missing, + "the send-scope capability must not inherit the readonly capability's \ + cached presence answer" + ); + } + + /// PR #5528 review regression: two requirements for the SAME scope, one + /// declared once and one declared with a literal duplicate, must resolve + /// through the identical cache key. Before the fix, `scopes.sort()` ran + /// without a following `.dedup()`, so `["gmail.readonly"]` and + /// `["gmail.readonly", "gmail.readonly"]` hashed to different keys — + /// wasting a cache slot and a resolver round trip on a manifest-declared + /// duplicate that carries no additional meaning. + #[tokio::test] + async fn required_credentials_present_key_is_canonical_across_duplicate_scope_declarations() { + let cache = CredentialPresenceCache::new(); + let resolver = CountingAccountResolver::default(); + let presence = ProductionCredentialPresence { + secret_store: None, + resolver: Some(&resolver), + cache: &cache, + }; + let scope = test_resource_scope(); + let single = descriptor_with_product_auth_scope("gmail.single_cap", "gmail.readonly"); + let duplicated = descriptor_with_product_auth_scopes( + "gmail.duplicated_cap", + &["gmail.readonly", "gmail.readonly"], + ); + + let single_present = presence.required_credentials_present(&scope, &single).await; + let duplicated_present = presence + .required_credentials_present(&scope, &duplicated) + .await; + + assert_eq!(single_present, CredentialPresenceStatus::Present); + assert_eq!(duplicated_present, CredentialPresenceStatus::Present); + assert_eq!( + resolver.calls.load(std::sync::atomic::Ordering::SeqCst), + 1, + "a manifest-declared duplicate scope must not miss the cache entry the \ + canonical (deduplicated) scope list already populated" + ); + } +} diff --git a/crates/ironclaw_host_runtime/src/credential_presence_cache.rs b/crates/ironclaw_host_runtime/src/credential_presence_cache.rs new file mode 100644 index 00000000000..19669c27719 --- /dev/null +++ b/crates/ironclaw_host_runtime/src/credential_presence_cache.rs @@ -0,0 +1,484 @@ +//! Short-TTL cache for capability-credential presence checks. +//! +//! [`CredentialPresenceCache`] avoids re-querying the secret store / product-auth +//! account resolver on every `visible_capabilities` render. The host rebuilds +//! the capability surface at roughly every LLM step within a turn, but +//! credential presence (is an account connected / is a secret stored) changes +//! far less often than that — so the naive approach would issue N redundant +//! presence lookups per step for no benefit. +//! +//! Only CONCLUSIVE presence results are cached: `Ok(true)`/`Ok(false)` from the +//! secret store's `metadata` lookup, or `Ok(true)`/`Ok(false)` from the +//! side-effect-free `account_configured` product-auth check. Any backend error +//! — from the secret store, or `Err(CredentialStageError::Backend)` from +//! `account_configured` — is indeterminate and is never cached, so a transient +//! blip cannot wedge a stale wrong answer for the whole TTL window; the next +//! lookup simply re-probes live. +//! +//! ## Cache key: owner scope, not full [`ResourceScope`] +//! +//! The key intentionally narrows `ResourceScope` to its owner-identity axes +//! (tenant/user/agent/project) via [`CredentialOwnerScope`], dropping +//! `invocation_id`/`thread_id`/`mission_id`. Credential presence is an +//! account-level property, not an invocation-scoped one. `ResourceScope` +//! carries a fresh `invocation_id` on every request (see +//! `ExecutionContext`/`ResourceScope` in `ironclaw_host_api`), so keying the +//! cache on the full scope would mean every lookup gets a fresh cache key and +//! the cache would never hit — defeating its purpose entirely. + +use std::{ + collections::HashMap, + sync::Mutex, + time::{Duration, Instant}, +}; + +use ironclaw_host_api::{ + AgentId, ExtensionId, ProjectId, ResourceScope, RuntimeCredentialAccountProviderId, + RuntimeCredentialAccountSetup, SecretHandle, TenantId, UserId, +}; + +/// Default TTL for cached credential-presence answers. Short enough that a +/// user connecting/disconnecting an account is reflected within one heartbeat +/// of surface renders; long enough to collapse the N-per-step lookups a +/// single turn's LLM steps would otherwise repeat against the same backend. +pub(crate) const DEFAULT_CREDENTIAL_PRESENCE_CACHE_TTL: Duration = Duration::from_secs(10); + +/// Owner-identity projection of [`ResourceScope`] used as the scope component +/// of a cache key. Deliberately excludes `invocation_id`/`thread_id`/ +/// `mission_id` — see the module doc for why. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct CredentialOwnerScope { + tenant_id: TenantId, + user_id: UserId, + agent_id: Option, + project_id: Option, +} + +impl CredentialOwnerScope { + pub(crate) fn from_scope(scope: &ResourceScope) -> Self { + Self { + tenant_id: scope.tenant_id.clone(), + user_id: scope.user_id.clone(), + agent_id: scope.agent_id.clone(), + project_id: scope.project_id.clone(), + } + } +} + +/// Cache-key-only projection of [`RuntimeCredentialAccountSetup`]'s +/// selection-relevant axis. +/// +/// Account selection (`account_has_provider_scopes` in +/// `ironclaw_reborn_composition::product_auth_runtime_credentials`) branches +/// only on the setup *variant* — `ManualToken` skips the stored-scopes gate +/// entirely, `OAuth` requires the account to already carry the requested +/// `provider_scopes`. Two requirements identical on every other axis but with +/// different setup variants therefore select accounts differently and must +/// not share a cache entry. +/// +/// Deliberately does NOT encode the `OAuth { scopes }` payload: that field is +/// the manifest-declared OAuth *consent* scope list (what to request from the +/// provider when creating an authorization flow), never consulted during +/// account selection. Encoding it here would double-encode the already-keyed +/// `provider_scopes` requirement axis for no correctness benefit. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub(crate) enum CredentialSetupKind { + ManualToken, + OAuth, +} + +impl CredentialSetupKind { + pub(crate) fn from_setup(setup: &RuntimeCredentialAccountSetup) -> Self { + match setup { + RuntimeCredentialAccountSetup::ManualToken => Self::ManualToken, + RuntimeCredentialAccountSetup::OAuth { .. } => Self::OAuth, + } + } +} + +/// Cache key for one credential presence answer. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) enum CredentialPresenceKey { + /// Generic secret-handle credential, keyed by owner scope + handle. + Secret(CredentialOwnerScope, SecretHandle), + /// Product-auth account credential, keyed by owner scope + provider + + /// requesting extension + setup kind + requested provider scopes (the + /// account identity axes — see `RuntimeCredentialAuthRequirement`). + /// + /// The scopes component is REQUIRED, not cosmetic: presence is + /// scope-dependent (`account_has_provider_scopes` filters configured + /// accounts by requested scopes), and a single provider/extension pair + /// commonly backs multiple capabilities with different required scopes + /// (e.g. gmail.readonly / gmail.send / gmail.modify all under + /// `provider="google"`, `requester_extension="gmail"`). Omitting scopes + /// from the key would let the first-checked capability's answer alias + /// onto every other capability sharing the same provider/extension in + /// the same render — reintroducing the #5416 false-"connected" bug (or a + /// spurious sign-in prompt) depending on check order. Callers must build + /// this variant with scopes sorted (mirrors `stable_auth_gate_id`'s + /// scope-sort) so key equality does not depend on manifest declaration + /// order. + /// + /// The setup-kind component is likewise required: the same + /// provider/extension/scopes triple can be requested under `ManualToken` + /// or `OAuth` setup, and `account_has_provider_scopes` selects + /// differently for each (see [`CredentialSetupKind`]). + ProductAuth( + CredentialOwnerScope, + RuntimeCredentialAccountProviderId, + ExtensionId, + CredentialSetupKind, + Vec, + ), +} + +struct CacheEntry { + present: bool, + inserted_at: Instant, +} + +/// Short-TTL, presence-only cache. Never authoritative — a miss or an expired +/// entry simply means the caller re-probes the live backend. +pub(crate) struct CredentialPresenceCache { + entries: Mutex>, + ttl: Duration, +} + +impl CredentialPresenceCache { + pub(crate) fn new() -> Self { + Self::with_ttl(DEFAULT_CREDENTIAL_PRESENCE_CACHE_TTL) + } + + pub(crate) fn with_ttl(ttl: Duration) -> Self { + Self { + entries: Mutex::new(HashMap::new()), + ttl, + } + } + + /// Returns the cached presence if a conclusive entry exists and has not + /// expired; otherwise `None` (cache miss — including "expired", which is + /// treated identically to "never cached"). + /// + /// Lock poisoning (a prior holder panicked mid-mutation) is recovered via + /// `into_inner` rather than propagated: this cache is never an authority — + /// the worst case of trusting a possibly-torn map is a spurious cache miss + /// (falls through to a live re-probe), never a wrong presence answer, since + /// entries are only ever inserted after a conclusive backend result. + pub(crate) fn get(&self, key: &CredentialPresenceKey) -> Option { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + match entries.get(key) { + Some(entry) if entry.inserted_at.elapsed() < self.ttl => Some(entry.present), + Some(_) => { + entries.remove(key); + None + } + None => None, + } + } + + /// Records a conclusive presence result. Callers must never insert an + /// indeterminate/backend-error outcome. + /// + /// Opportunistically sweeps every expired entry first (not just the key + /// being inserted). Without this, a key that stops being queried after + /// expiry (e.g. a capability that scrolls out of a manifest, or a scope + /// combination checked once and never again) lingers in the map for the + /// process lifetime — unbounded growth on a long-running multi-user host. + /// `get` only ever reclaims the single key it was asked about, so it + /// cannot bound the map on its own. + pub(crate) fn insert(&self, key: CredentialPresenceKey, present: bool) { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let ttl = self.ttl; + entries.retain(|_, entry| entry.inserted_at.elapsed() < ttl); + entries.insert( + key, + CacheEntry { + present, + inserted_at: Instant::now(), + }, + ); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn owner_scope(tenant: &str, user: &str) -> CredentialOwnerScope { + CredentialOwnerScope { + tenant_id: TenantId::new(tenant).unwrap(), + user_id: UserId::new(user).unwrap(), + agent_id: None, + project_id: None, + } + } + + fn secret_key(tenant: &str, user: &str, handle: &str) -> CredentialPresenceKey { + CredentialPresenceKey::Secret( + owner_scope(tenant, user), + SecretHandle::new(handle).unwrap(), + ) + } + + fn product_auth_key( + tenant: &str, + user: &str, + provider: &str, + extension: &str, + scopes: &[&str], + ) -> CredentialPresenceKey { + product_auth_key_with_setup( + tenant, + user, + provider, + extension, + scopes, + CredentialSetupKind::ManualToken, + ) + } + + fn product_auth_key_with_setup( + tenant: &str, + user: &str, + provider: &str, + extension: &str, + scopes: &[&str], + setup: CredentialSetupKind, + ) -> CredentialPresenceKey { + let mut scopes: Vec = scopes.iter().map(|scope| scope.to_string()).collect(); + scopes.sort(); + CredentialPresenceKey::ProductAuth( + owner_scope(tenant, user), + RuntimeCredentialAccountProviderId::new(provider).unwrap(), + ExtensionId::new(extension).unwrap(), + setup, + scopes, + ) + } + + #[test] + fn hit_within_ttl_returns_cached_presence() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let key = secret_key("tenant", "user", "handle"); + + cache.insert(key.clone(), true); + + assert_eq!(cache.get(&key), Some(true)); + } + + #[test] + fn miss_after_expiry_returns_none() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_millis(10)); + let key = secret_key("tenant", "user", "handle"); + + cache.insert(key.clone(), false); + std::thread::sleep(Duration::from_millis(40)); + + assert_eq!(cache.get(&key), None); + } + + #[test] + fn never_cached_indeterminate_result_is_a_miss() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let key = secret_key("tenant", "user", "handle"); + + // Simulates a backend-error/indeterminate outcome: the caller never + // calls `insert` for it. The cache must report a miss, not a stale or + // fabricated answer. + assert_eq!(cache.get(&key), None); + } + + #[test] + fn distinct_owner_scopes_do_not_collide() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let key_a = secret_key("tenant-a", "user", "handle"); + let key_b = secret_key("tenant-b", "user", "handle"); + + cache.insert(key_a.clone(), true); + + assert_eq!(cache.get(&key_a), Some(true)); + assert_eq!(cache.get(&key_b), None); + } + + #[test] + fn product_auth_key_is_distinct_from_secret_key_with_overlapping_scope() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let scope = owner_scope("tenant", "user"); + let secret_key = + CredentialPresenceKey::Secret(scope.clone(), SecretHandle::new("h").unwrap()); + let product_auth_key = CredentialPresenceKey::ProductAuth( + scope, + RuntimeCredentialAccountProviderId::new("google").unwrap(), + ExtensionId::new("gmail").unwrap(), + CredentialSetupKind::ManualToken, + Vec::new(), + ); + + cache.insert(secret_key.clone(), true); + + assert_eq!(cache.get(&secret_key), Some(true)); + assert_eq!(cache.get(&product_auth_key), None); + } + + /// #5416 Phase 2 Fix B (BLOCKER) regression: two product-auth + /// requirements that share `(owner_scope, provider, requester_extension)` + /// but request different provider scopes (e.g. gmail.readonly vs. + /// gmail.send under the same `provider="google"`, + /// `requester_extension="gmail"`) must not alias onto the same cache + /// entry. Before the fix, `CredentialPresenceKey::ProductAuth` omitted + /// scopes entirely, so caching the readonly capability's answer would be + /// wrongly reused for the send capability's lookup in the same render. + #[test] + fn product_auth_keys_with_different_scopes_do_not_collide() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let readonly_key = + product_auth_key("tenant", "user", "google", "gmail", &["gmail.readonly"]); + let send_key = product_auth_key("tenant", "user", "google", "gmail", &["gmail.send"]); + + cache.insert(readonly_key.clone(), true); + + assert_eq!( + cache.get(&readonly_key), + Some(true), + "the scope this entry was inserted for must retain its own answer" + ); + assert_eq!( + cache.get(&send_key), + None, + "a different scope combination under the same provider/extension must not \ + alias onto the readonly answer" + ); + } + + /// PR #5528 review regression: two product-auth requirements identical on + /// every axis except `setup` (`ManualToken` vs `OAuth`) must not collide. + /// `account_has_provider_scopes` branches on the setup variant — + /// `ManualToken` ignores stored account scopes, `OAuth` requires the + /// account to already carry them — so caching one setup's answer under a + /// key the other setup's lookup also hits would alias a wrong presence + /// result across the two selection semantics. + #[test] + fn product_auth_keys_with_different_setup_do_not_collide() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_secs(60)); + let manual_token_key = product_auth_key_with_setup( + "tenant", + "user", + "google", + "gmail", + &["gmail.readonly"], + CredentialSetupKind::ManualToken, + ); + let oauth_key = product_auth_key_with_setup( + "tenant", + "user", + "google", + "gmail", + &["gmail.readonly"], + CredentialSetupKind::OAuth, + ); + + cache.insert(manual_token_key.clone(), true); + + assert_eq!( + cache.get(&manual_token_key), + Some(true), + "the setup this entry was inserted for must retain its own answer" + ); + assert_eq!( + cache.get(&oauth_key), + None, + "a different setup under the same owner/provider/extension/scopes must not \ + alias onto the manual-token answer" + ); + } + + /// `CredentialOwnerScope::from_scope` must project two `ResourceScope` + /// values that share tenant/user/agent/project but differ in + /// invocation/thread/mission id onto the SAME owner scope — those axes are + /// transient per-request identifiers (see the module doc), not part of + /// credential ownership, and keeping them out of the projection is what + /// makes the cache hit across the fresh `invocation_id` every request + /// carries. + #[test] + fn owner_scope_projection_ignores_invocation_thread_and_mission_id() { + let tenant_id = TenantId::new("tenant").unwrap(); + let user_id = UserId::new("user").unwrap(); + let agent_id = AgentId::new("agent").unwrap(); + let project_id = ProjectId::new("project").unwrap(); + + let scope_a = ResourceScope { + tenant_id: tenant_id.clone(), + user_id: user_id.clone(), + agent_id: Some(agent_id.clone()), + project_id: Some(project_id.clone()), + mission_id: Some(ironclaw_host_api::MissionId::new("mission-a").unwrap()), + thread_id: Some(ironclaw_host_api::ThreadId::new("thread-a").unwrap()), + invocation_id: ironclaw_host_api::InvocationId::new(), + }; + let scope_b = ResourceScope { + tenant_id, + user_id, + agent_id: Some(agent_id), + project_id: Some(project_id), + mission_id: Some(ironclaw_host_api::MissionId::new("mission-b").unwrap()), + thread_id: Some(ironclaw_host_api::ThreadId::new("thread-b").unwrap()), + invocation_id: ironclaw_host_api::InvocationId::new(), + }; + + assert_ne!( + scope_a.mission_id, scope_b.mission_id, + "test fixture must actually vary mission_id" + ); + assert_ne!( + scope_a.thread_id, scope_b.thread_id, + "test fixture must actually vary thread_id" + ); + assert_ne!( + scope_a.invocation_id, scope_b.invocation_id, + "test fixture must actually vary invocation_id" + ); + assert_eq!( + CredentialOwnerScope::from_scope(&scope_a), + CredentialOwnerScope::from_scope(&scope_b), + "owner scope must be identical across differing invocation/thread/mission ids" + ); + } + + /// #5416 Phase 2 Fix C (MEDIUM) regression: `insert` must sweep every + /// expired entry, not just the key being inserted — otherwise a key that + /// stops being queried after expiry lingers in the map for the process + /// lifetime. Confirmed by inspecting the underlying map length directly + /// (a `get`-only check would pass even without the sweep, since `get` + /// already treats an expired entry as a miss without evicting anything + /// but the queried key). + #[test] + fn insert_sweeps_expired_entries_for_unrelated_keys() { + let cache = CredentialPresenceCache::with_ttl(Duration::from_millis(10)); + let stale_key = secret_key("tenant", "user", "stale-handle"); + cache.insert(stale_key.clone(), true); + std::thread::sleep(Duration::from_millis(40)); + + let fresh_key = secret_key("tenant", "user", "fresh-handle"); + cache.insert(fresh_key.clone(), false); + + let entries = cache + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + assert_eq!( + entries.len(), + 1, + "insert must opportunistically sweep expired entries, not just answer \ + misses lazily on re-query" + ); + assert!(entries.contains_key(&fresh_key)); + assert!(!entries.contains_key(&stale_key)); + } +} diff --git a/crates/ironclaw_host_runtime/src/lib.rs b/crates/ironclaw_host_runtime/src/lib.rs index ff46368c769..99742950123 100644 --- a/crates/ironclaw_host_runtime/src/lib.rs +++ b/crates/ironclaw_host_runtime/src/lib.rs @@ -37,6 +37,8 @@ use std::{collections::BTreeMap, env, fmt}; use thiserror::Error; mod capability_catalog; +mod credential_presence; +mod credential_presence_cache; mod document_output; mod egress; mod extension_contracts; diff --git a/crates/ironclaw_host_runtime/src/obligations.rs b/crates/ironclaw_host_runtime/src/obligations.rs index 3ca174c8a54..786b404ff74 100644 --- a/crates/ironclaw_host_runtime/src/obligations.rs +++ b/crates/ironclaw_host_runtime/src/obligations.rs @@ -70,6 +70,17 @@ pub trait RuntimeCredentialAccountResolver: Send + Sync + fmt::Debug { &self, request: RuntimeCredentialAccountRequest<'_>, ) -> Result; + + /// Side-effect-free presence check: is a configured account available for + /// this requirement WITHOUT staging or refreshing any secret material? Safe + /// to call repeatedly on every capability-surface render. `Ok(true)` = + /// configured account present; `Ok(false)` = no configured account (user must + /// sign in); `Err(Backend)` = indeterminate backend failure (caller fails + /// open). Must NOT perform token refresh, network staging, or lease consumption. + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result; } /// Runtime secret material staged after `InjectSecretOnce` lease consumption. diff --git a/crates/ironclaw_host_runtime/src/production.rs b/crates/ironclaw_host_runtime/src/production.rs index 9e49b2b8f7b..831615829be 100644 --- a/crates/ironclaw_host_runtime/src/production.rs +++ b/crates/ironclaw_host_runtime/src/production.rs @@ -104,7 +104,11 @@ use crate::{ RuntimeCapabilityCompleted, RuntimeCapabilityFailure, RuntimeCapabilityOutcome, RuntimeCapabilityRequest, RuntimeCapabilityResumeRequest, RuntimeFailureKind, RuntimeGateId, RuntimeStatusRequest, RuntimeWorkId, RuntimeWorkSummary, VisibleCapabilityRequest, - VisibleCapabilitySurface, obligations::secret_present, plan_capability, + VisibleCapabilitySurface, + credential_presence::ProductionCredentialPresence, + credential_presence_cache::CredentialPresenceCache, + obligations::{RuntimeCredentialAccountResolver, secret_present}, + plan_capability, surface::CapabilityCatalog, }; @@ -142,6 +146,22 @@ pub struct DefaultHostRuntime { // arch-exempt: optional_arc, credential pre-flight is disabled in minimal/test // host-runtime graphs that do not wire a secret store, plan #4539 (Fix B) credential_preflight_store: Option>, + /// Optional product-auth account resolver used for capability-surface + /// credential-presence checks (`visible_capabilities` `NeedsAuth` + /// downgrade — issue #5416, Phase 2). + /// + /// Distinct from the dispatch-time resolver wired into + /// `BuiltinObligationHandler`: that one stages the actual credential + /// material for one capability invocation; this one only answers "is an + /// account configured" for surface rendering. Production composition + /// wires both from the same underlying resolver. + // arch-exempt: optional_arc, credential presence is disabled in minimal/test + // host-runtime graphs that do not wire a resolver, plan #5416 (Phase 2) + credential_account_resolver: Option>, + /// Short-TTL cache backing the credential-presence checks above. Always + /// constructed (cheap, in-memory); it is inert unless a presence check + /// actually runs. + credential_presence_cache: Arc, surface_version: CapabilitySurfaceVersion, runtime_policy: EffectiveRuntimePolicy, } @@ -208,6 +228,8 @@ impl DefaultHostRuntime { runtime_health: None, obligation_handler: None, credential_preflight_store: None, + credential_account_resolver: None, + credential_presence_cache: Arc::new(CredentialPresenceCache::new()), surface_version, runtime_policy, } @@ -385,6 +407,22 @@ impl DefaultHostRuntime { self } + /// Attaches the product-auth account resolver used for capability-surface + /// credential-presence checks (`visible_capabilities` `NeedsAuth` + /// downgrade). + /// + /// Production code must use `HostRuntimeServices::build_host_runtime()` + /// which wires the resolver automatically. This setter is `pub(crate)` for + /// the same reason as [`Self::with_credential_preflight_store`]: prevent a + /// second public seam for this wiring. + pub(crate) fn with_credential_account_resolver( + mut self, + resolver: Arc, + ) -> Self { + self.credential_account_resolver = Some(resolver); + self + } + /// Spawns an already-authorized process request through the configured /// process manager. pub async fn spawn_process( @@ -1044,6 +1082,18 @@ impl HostRuntime for DefaultHostRuntime { Some(filesystem) => catalog.with_filesystem(filesystem), None => catalog, }; + let presence = ProductionCredentialPresence { + secret_store: self.credential_preflight_store.as_deref(), + resolver: self.credential_account_resolver.as_deref(), + cache: &self.credential_presence_cache, + }; + let catalog = if self.credential_preflight_store.is_some() + || self.credential_account_resolver.is_some() + { + catalog.with_credential_presence(&presence) + } else { + catalog + }; catalog.visible_capabilities(request).await } diff --git a/crates/ironclaw_host_runtime/src/services.rs b/crates/ironclaw_host_runtime/src/services.rs index 8396b3f3dca..393b23e7d8d 100644 --- a/crates/ironclaw_host_runtime/src/services.rs +++ b/crates/ironclaw_host_runtime/src/services.rs @@ -598,6 +598,13 @@ where if let Some(secret_store) = &self.secret_store { runtime = runtime.with_credential_preflight_store(Arc::clone(secret_store)); } + // Wire the same product-auth account resolver used by the obligation + // handler (below) into the capability-surface credential-presence + // check, so `visible_capabilities` and dispatch-time staging agree on + // "is this account connected" (issue #5416, Phase 2). + if let Some(resolver) = &self.runtime_credential_account_resolver { + runtime = runtime.with_credential_account_resolver(Arc::clone(resolver)); + } runtime.with_obligation_handler(Arc::new(self.builtin_obligation_handler())) } diff --git a/crates/ironclaw_host_runtime/src/surface.rs b/crates/ironclaw_host_runtime/src/surface.rs index dfbe8e75034..823faa13be0 100644 --- a/crates/ironclaw_host_runtime/src/surface.rs +++ b/crates/ironclaw_host_runtime/src/surface.rs @@ -1,9 +1,11 @@ +use async_trait::async_trait; +use futures_util::{StreamExt, stream}; use ironclaw_authorization::TrustAwareCapabilityDispatchAuthorizer; use ironclaw_extensions::{CapabilityVisibility, ExtensionPackage, ExtensionRegistry}; use ironclaw_filesystem::RootFilesystem; use ironclaw_host_api::{ - CapabilityDescriptor, CapabilityGrant, Decision, EffectKind, ResourceEstimate, RuntimeKind, - canonical_json_v1, runtime_policy::EffectiveRuntimePolicy, sha256_digest_token, + CapabilityDescriptor, CapabilityGrant, Decision, EffectKind, ResourceEstimate, ResourceScope, + RuntimeKind, canonical_json_v1, runtime_policy::EffectiveRuntimePolicy, sha256_digest_token, }; use ironclaw_trust::TrustDecision; use serde_json::{Value, json}; @@ -15,6 +17,14 @@ use crate::{ plan_capability, }; +/// Fan-out width for the per-capability credential-presence downgrade pass in +/// `visible_capabilities`. Mirrors +/// `ironclaw_reborn_composition::extension_availability::EXTENSION_READINESS_CONCURRENCY` +/// (the extension-search path's identical bounded fan-out over credential +/// readiness checks) — kept as a separate constant because the two live in +/// different crates. +const CREDENTIAL_PRESENCE_DOWNGRADE_CONCURRENCY: usize = 8; + const ALL_RUNTIME_KINDS: &[RuntimeKind] = &[ RuntimeKind::Wasm, RuntimeKind::Mcp, @@ -98,6 +108,48 @@ pub enum VisibleCapabilityAccess { /// Capability may be shown as askable, but actual use must still block on /// the approval/lease path. RequiresApproval, + /// Installed & authorized, but a required credential is missing — the + /// model should trigger sign-in; dispatch still re-checks at execution + /// time. + NeedsAuth, +} + +/// Outcome of a capability-credential presence check. +/// +/// A three-state enum instead of `Option` per `.claude/rules/types.md` +/// ("match-on-string-literals means the type should be an enum" — the same +/// reasoning applies to a boolean-plus-sentinel encoding two independent +/// facts, presence and conclusiveness, into one `Option`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum CredentialPresenceStatus { + /// All required credentials are present (or none are required). + Present, + /// At least one required credential is missing → downgrade to + /// [`VisibleCapabilityAccess::NeedsAuth`]. + Missing, + /// Indeterminate (backend blip, unwired dependency, or a defensive + /// contract-violation guard). The caller keeps `Available` — fail-open, + /// matching `credential_preflight_check` — rather than falsely prompting + /// sign-in for a transient failure that has nothing to do with the user's + /// credentials. + Indeterminate, +} + +/// Presence check for capability-required credentials, used to downgrade an +/// otherwise-`Available` capability to [`VisibleCapabilityAccess::NeedsAuth`] +/// on the visible surface. +/// +/// This is a dependency-inverted port: `surface.rs` stays agnostic to secret +/// stores / product-auth account resolvers. See `production.rs` for the +/// concrete implementation composing a `SecretStore` and a +/// `RuntimeCredentialAccountResolver` behind a short-TTL cache. +#[async_trait] +pub(crate) trait CapabilityCredentialPresence: Send + Sync { + async fn required_credentials_present( + &self, + scope: &ResourceScope, + descriptor: &CapabilityDescriptor, + ) -> CredentialPresenceStatus; } /// Capability metadata safe to render on a model/tool surface. @@ -121,6 +173,7 @@ pub(crate) struct CapabilityCatalog<'a> { base_version: &'a CapabilitySurfaceVersion, runtime_policy: &'a EffectiveRuntimePolicy, filesystem: Option<&'a dyn RootFilesystem>, + credential_presence: Option<&'a dyn CapabilityCredentialPresence>, } impl<'a> CapabilityCatalog<'a> { @@ -136,6 +189,7 @@ impl<'a> CapabilityCatalog<'a> { base_version, runtime_policy, filesystem: None, + credential_presence: None, } } @@ -144,6 +198,14 @@ impl<'a> CapabilityCatalog<'a> { self } + pub(crate) fn with_credential_presence( + mut self, + presence: &'a dyn CapabilityCredentialPresence, + ) -> Self { + self.credential_presence = Some(presence); + self + } + pub(crate) async fn visible_capabilities( &self, request: VisibleCapabilityRequest, @@ -197,12 +259,57 @@ impl<'a> CapabilityCatalog<'a> { }); } + // The fingerprint is computed from the authorization-only `access` + // states (Available/RequiresApproval), BEFORE the credential-presence + // downgrade pass below. This keeps the surface version fingerprint + // credential-independent: a capability whose credential later goes + // missing (or comes back) must not churn `version`, since credential + // state is re-derived per render, not part of the authorized + // capability set. Mirrors the #4789 principle (a signal that flips + // independently of the authorized set must not be fingerprinted) — + // see `access_token`, which never encodes `NeedsAuth`. let version = surface_version( self.base_version, &request, self.runtime_policy, &capabilities, )?; + + if let Some(presence) = self.credential_presence { + let scope = &request.context.resource_scope; + let available_indices: Vec = capabilities + .iter() + .enumerate() + .filter(|(_, capability)| capability.access == VisibleCapabilityAccess::Available) + .map(|(index, _)| index) + .collect(); + // Presence checks are read-only backend round trips (secret-store + // metadata reads / product-auth `account_configured` calls) — on a + // cold cache, checking each Available capability serially pays N + // sequential round trips per render. Resolve with bounded + // concurrency instead, mirroring + // `ironclaw_reborn_composition::extension_availability::EXTENSION_READINESS_CONCURRENCY` + // (the search path's identical fan-out over the same kind of + // credential-gate lookup). `buffered` (not `buffer_unordered`) + // preserves the input order, so `available_indices` zips back onto + // `results` positionally without threading the index through the + // future itself, keeping result application deterministic. + let results: Vec = + stream::iter(available_indices.iter().copied()) + .map(|index| { + presence + .required_credentials_present(scope, &capabilities[index].descriptor) + }) + .buffered(CREDENTIAL_PRESENCE_DOWNGRADE_CONCURRENCY) + .collect() + .await; + for (index, status) in available_indices.into_iter().zip(results) { + if status == CredentialPresenceStatus::Missing { + capabilities[index].access = VisibleCapabilityAccess::NeedsAuth; + } + } + } + Ok(VisibleCapabilitySurface { version, capabilities, @@ -462,6 +569,13 @@ fn access_token(access: VisibleCapabilityAccess) -> &'static str { match access { VisibleCapabilityAccess::Available => "available", VisibleCapabilityAccess::RequiresApproval => "requires_approval", + // Deliberately unreachable in practice: `access_token` is only called + // from `surface_version`/`capability_version_key`, and the credential + // presence downgrade to `NeedsAuth` runs strictly AFTER + // `surface_version` is computed (see `visible_capabilities`). A token + // still needs to exist so the match stays exhaustive if a future + // caller ever fingerprints a post-downgrade capability list. + VisibleCapabilityAccess::NeedsAuth => "needs_auth", } } @@ -478,13 +592,17 @@ fn host_api_error(error: ironclaw_host_api::HostApiError) -> HostRuntimeError { mod tests { use super::*; use ironclaw_authorization::GrantAuthorizer; + use ironclaw_extensions::{ExtensionManifest, ManifestSource}; use ironclaw_host_api::{ - CapabilityId, ExtensionId, PermissionMode, TrustClass, + Action, ApprovalRequest, ApprovalRequestId, CapabilityId, CapabilitySet, ExecutionContext, + ExtensionId, HostPortCatalog, MountView, Obligations, PermissionMode, Principal, + TrustClass, UserId, VirtualPath, runtime_policy::{ ApprovalPolicy, AuditMode, DeploymentMode, EffectiveRuntimePolicy, FilesystemBackendKind, NetworkMode, ProcessBackendKind, RuntimeProfile, SecretMode, }, }; + use std::collections::HashMap; fn test_runtime_policy() -> EffectiveRuntimePolicy { EffectiveRuntimePolicy { @@ -532,4 +650,348 @@ mod tests { "unexpected error: {error:?}" ); } + + // ─── NeedsAuth credential-presence downgrade (issue #5416, Phase 2) ────── + + /// Fixed-response fake [`CapabilityCredentialPresence`], keyed by + /// capability id string. A capability id not present in the map defaults + /// to [`CredentialPresenceStatus::Present`] (not gated) so tests only need + /// to register the ids they care about. + struct FakeCredentialPresence { + responses: HashMap, + } + + impl FakeCredentialPresence { + fn new>( + responses: impl IntoIterator, + ) -> Self { + Self { + responses: responses + .into_iter() + .map(|(id, presence)| (id.into(), presence)) + .collect(), + } + } + } + + #[async_trait] + impl CapabilityCredentialPresence for FakeCredentialPresence { + async fn required_credentials_present( + &self, + _scope: &ResourceScope, + descriptor: &CapabilityDescriptor, + ) -> CredentialPresenceStatus { + self.responses + .get(descriptor.id.as_str()) + .copied() + .unwrap_or(CredentialPresenceStatus::Present) + } + } + + /// Authorizer returning a fixed `Decision` per capability id: `Allow` by + /// default, `RequireApproval` for ids named in `require_approval_for`. + struct FixedAccessAuthorizer { + require_approval_for: Vec, + } + + #[async_trait] + impl TrustAwareCapabilityDispatchAuthorizer for FixedAccessAuthorizer { + async fn authorize_dispatch_with_trust( + &self, + context: &ExecutionContext, + descriptor: &CapabilityDescriptor, + estimate: &ResourceEstimate, + _trust_decision: &ironclaw_trust::TrustDecision, + ) -> Decision { + if self.require_approval_for.contains(&descriptor.id) { + Decision::RequireApproval { + request: ApprovalRequest { + id: ApprovalRequestId::new(), + correlation_id: context.correlation_id, + requested_by: Principal::Extension(context.extension_id.clone()), + action: Box::new(Action::Dispatch { + capability: descriptor.id.clone(), + estimated_resources: estimate.clone(), + }), + invocation_fingerprint: None, + reason: "approval required".to_string(), + reusable_scope: None, + }, + } + } else { + Decision::Allow { + obligations: Obligations::default(), + } + } + } + } + + fn registry_with_capabilities(ids: &[&str], provider: &str) -> ExtensionRegistry { + let mut manifest = format!( + r#"schema_version = "reborn.extension_manifest.v2" +id = "{provider}" +name = "Test Provider" +version = "0.1.0" +description = "Test extension" +trust = "third_party" + +[runtime] +kind = "wasm" +module = "test.wasm" +"# + ); + for id in ids { + manifest.push_str(&format!( + r#" +[[capabilities]] +id = "{id}" +description = "Test capability" +effects = ["dispatch_capability"] +default_permission = "allow" +visibility = "model" +input_schema_ref = "schemas/test/input.v1.json" +output_schema_ref = "schemas/test/output.v1.json" +prompt_doc_ref = "prompts/test/say.md" +"# + )); + } + let manifest = ExtensionManifest::parse( + &manifest, + ManifestSource::InstalledLocal, + &HostPortCatalog::empty(), + ) + .expect("manifest must parse"); + let root = VirtualPath::new(format!("/system/extensions/{provider}")).unwrap(); + let package = ExtensionPackage::from_manifest(manifest, root).expect("package must build"); + let mut registry = ExtensionRegistry::new(); + registry.insert(package).unwrap(); + registry + } + + fn test_execution_context() -> ExecutionContext { + ExecutionContext::local_default( + UserId::new("user").unwrap(), + ExtensionId::new("caller").unwrap(), + RuntimeKind::FirstParty, + TrustClass::UserTrusted, + CapabilitySet::default(), + MountView::default(), + ) + .unwrap() + } + + fn test_visible_request(context: ExecutionContext, provider: &str) -> VisibleCapabilityRequest { + let mut provider_trust = std::collections::BTreeMap::new(); + provider_trust.insert( + ExtensionId::new(provider).unwrap(), + ironclaw_trust::TrustDecision { + effective_trust: ironclaw_trust::EffectiveTrustClass::user_trusted(), + authority_ceiling: ironclaw_trust::AuthorityCeiling { + allowed_effects: vec![EffectKind::DispatchCapability], + max_resource_ceiling: None, + }, + provenance: ironclaw_trust::TrustProvenance::Default, + evaluated_at: chrono::Utc::now(), + }, + ); + VisibleCapabilityRequest::new(context, crate::SurfaceKind::new("agent_loop").unwrap()) + .with_policy(CapabilitySurfacePolicy::allow_all()) + .with_provider_trust(provider_trust) + } + + #[tokio::test] + async fn available_capability_downgrades_to_needs_auth_when_credential_missing() { + let registry = registry_with_capabilities(&["test-ext.available"], "test-ext"); + let authorizer = FixedAccessAuthorizer { + require_approval_for: Vec::new(), + }; + let runtime_policy = test_runtime_policy(); + let surface_version = CapabilitySurfaceVersion::new("surface-v1").unwrap(); + let presence = FakeCredentialPresence::new([( + "test-ext.available", + CredentialPresenceStatus::Missing, + )]); + let catalog = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&presence); + let request = test_visible_request(test_execution_context(), "test-ext"); + + let surface = catalog.visible_capabilities(request).await.unwrap(); + + assert_eq!(surface.capabilities.len(), 1); + assert_eq!( + surface.capabilities[0].access, + VisibleCapabilityAccess::NeedsAuth + ); + } + + #[tokio::test] + async fn available_capability_stays_available_when_presence_present_or_indeterminate() { + let registry = + registry_with_capabilities(&["test-ext.present", "test-ext.indeterminate"], "test-ext"); + let authorizer = FixedAccessAuthorizer { + require_approval_for: Vec::new(), + }; + let runtime_policy = test_runtime_policy(); + let surface_version = CapabilitySurfaceVersion::new("surface-v1").unwrap(); + let presence = FakeCredentialPresence::new([ + ("test-ext.present", CredentialPresenceStatus::Present), + ( + "test-ext.indeterminate", + CredentialPresenceStatus::Indeterminate, + ), + ]); + let catalog = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&presence); + let request = test_visible_request(test_execution_context(), "test-ext"); + + let surface = catalog.visible_capabilities(request).await.unwrap(); + + assert_eq!(surface.capabilities.len(), 2); + assert!( + surface + .capabilities + .iter() + .all(|capability| capability.access == VisibleCapabilityAccess::Available), + "presence Present/Indeterminate must not downgrade Available: {:?}", + surface.capabilities + ); + } + + #[tokio::test] + async fn requires_approval_capability_is_never_downgraded_to_needs_auth() { + let registry = registry_with_capabilities(&["test-ext.approval"], "test-ext"); + let authorizer = FixedAccessAuthorizer { + require_approval_for: vec![CapabilityId::new("test-ext.approval").unwrap()], + }; + let runtime_policy = test_runtime_policy(); + let surface_version = CapabilitySurfaceVersion::new("surface-v1").unwrap(); + let presence = + FakeCredentialPresence::new([("test-ext.approval", CredentialPresenceStatus::Missing)]); + let catalog = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&presence); + let request = test_visible_request(test_execution_context(), "test-ext"); + + let surface = catalog.visible_capabilities(request).await.unwrap(); + + assert_eq!(surface.capabilities.len(), 1); + assert_eq!( + surface.capabilities[0].access, + VisibleCapabilityAccess::RequiresApproval, + "a capability requiring approval must never be downgraded to NeedsAuth" + ); + } + + /// M3 load-bearing regression: the surface fingerprint must not change + /// when only credential presence flips. A naive implementation that fed + /// `NeedsAuth` into `surface_version` (or otherwise let credential state + /// leak into the fingerprint) would fail this test. + #[tokio::test] + async fn surface_version_is_identical_regardless_of_credential_presence() { + let registry = registry_with_capabilities(&["test-ext.cred"], "test-ext"); + let authorizer = FixedAccessAuthorizer { + require_approval_for: Vec::new(), + }; + let runtime_policy = test_runtime_policy(); + let surface_version = CapabilitySurfaceVersion::new("surface-v1").unwrap(); + let context = test_execution_context(); + + let present = + FakeCredentialPresence::new([("test-ext.cred", CredentialPresenceStatus::Present)]); + let missing = + FakeCredentialPresence::new([("test-ext.cred", CredentialPresenceStatus::Missing)]); + + let catalog_present = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&present); + let catalog_missing = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&missing); + + let surface_present = catalog_present + .visible_capabilities(test_visible_request(context.clone(), "test-ext")) + .await + .unwrap(); + let surface_missing = catalog_missing + .visible_capabilities(test_visible_request(context, "test-ext")) + .await + .unwrap(); + + assert_eq!( + surface_present.capabilities[0].access, + VisibleCapabilityAccess::Available + ); + assert_eq!( + surface_missing.capabilities[0].access, + VisibleCapabilityAccess::NeedsAuth + ); + assert_eq!( + surface_present.version, surface_missing.version, + "credential-derived NeedsAuth downgrade must not change the surface \ + fingerprint (see #4789)" + ); + } + + /// PR #5528 review regression: the credential-presence downgrade pass runs + /// with bounded concurrency (`CREDENTIAL_PRESENCE_DOWNGRADE_CONCURRENCY`) + /// instead of one presence check at a time. This must not scramble which + /// capability gets which answer. Uses more capabilities than the + /// concurrency width so at least two `buffered` batches are exercised, and + /// interleaves missing/present so a naive unordered fan-out (or a + /// mismatched index zip) would misapply at least one result. + #[tokio::test] + async fn concurrent_downgrade_pass_applies_each_result_to_the_right_capability() { + const CAPABILITY_COUNT: usize = 20; + let ids: Vec = (0..CAPABILITY_COUNT) + .map(|index| format!("test-ext.cap-{index}")) + .collect(); + let id_refs: Vec<&str> = ids.iter().map(String::as_str).collect(); + let registry = registry_with_capabilities(&id_refs, "test-ext"); + let authorizer = FixedAccessAuthorizer { + require_approval_for: Vec::new(), + }; + let runtime_policy = test_runtime_policy(); + let surface_version = CapabilitySurfaceVersion::new("surface-v1").unwrap(); + // Odd-indexed capabilities are missing their credential; even-indexed + // stay present. Deliberately not a uniform answer so a scrambled + // ordering shows up as a specific-capability mismatch, not a + // count-only discrepancy. + let presence = FakeCredentialPresence::new(ids.iter().enumerate().map(|(index, id)| { + let status = if index % 2 == 1 { + CredentialPresenceStatus::Missing + } else { + CredentialPresenceStatus::Present + }; + (id.as_str(), status) + })); + let catalog = + CapabilityCatalog::new(®istry, &authorizer, &surface_version, &runtime_policy) + .with_credential_presence(&presence); + let request = test_visible_request(test_execution_context(), "test-ext"); + + let surface = catalog.visible_capabilities(request).await.unwrap(); + + assert_eq!(surface.capabilities.len(), CAPABILITY_COUNT); + for capability in &surface.capabilities { + let index: usize = capability + .descriptor + .id + .as_str() + .strip_prefix("test-ext.cap-") + .and_then(|suffix| suffix.parse().ok()) + .expect("capability id must carry its index suffix"); + let expected = if index % 2 == 1 { + VisibleCapabilityAccess::NeedsAuth + } else { + VisibleCapabilityAccess::Available + }; + assert_eq!( + capability.access, expected, + "capability {} got the wrong access after the concurrent downgrade pass", + capability.descriptor.id + ); + } + } } diff --git a/crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs b/crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs index 1c30118585b..7e26c3bcf23 100644 --- a/crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs +++ b/crates/ironclaw_host_runtime/tests/builtin_obligation_handler_contract.rs @@ -1267,6 +1267,13 @@ impl RuntimeCredentialAccountResolver for AlwaysAuthRequiredResolver { ) -> Result { Err(ironclaw_host_api::CredentialStageError::AuthRequired) } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(false) + } } #[derive(Debug)] @@ -1283,6 +1290,13 @@ impl RuntimeCredentialAccountResolver for FixedHandleResolver { handle: self.0.clone(), }) } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(true) + } } #[derive(Debug)] @@ -1302,6 +1316,13 @@ impl RuntimeCredentialAccountResolver for SourceScopedHandleResolver { handle: self.handle.clone(), }) } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(true) + } } #[tokio::test] diff --git a/crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs b/crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs index 4ed12bf0827..6dffd1b4594 100644 --- a/crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs +++ b/crates/ironclaw_host_runtime/tests/github_wasm_runtime_contract.rs @@ -1781,6 +1781,19 @@ impl RuntimeCredentialAccountResolver for FixedRuntimeCredentialAccountResolver handle, }) } + + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + assert_eq!(request.provider.as_str(), "github"); + assert_eq!(request.requester_extension.as_str(), "github"); + match &self.result { + Ok(_) => Ok(true), + Err(CredentialStageError::AuthRequired) => Ok(false), + Err(CredentialStageError::Backend) => Err(CredentialStageError::Backend), + } + } } #[derive(Debug)] @@ -1809,6 +1822,23 @@ impl RuntimeCredentialAccountResolver for FixedGoogleRuntimeCredentialAccountRes handle, }) } + + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + assert_eq!(request.provider.as_str(), "google"); + assert_eq!( + request.requester_extension, + &self.expected_requester_extension + ); + assert_eq!(request.provider_scopes, self.expected_scopes.as_slice()); + match &self.result { + Ok(_) => Ok(true), + Err(CredentialStageError::AuthRequired) => Ok(false), + Err(CredentialStageError::Backend) => Err(CredentialStageError::Backend), + } + } } fn registry_with_github_package() -> ExtensionRegistry { diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_visible_capability_credential_presence_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_visible_capability_credential_presence_contract.rs new file mode 100644 index 00000000000..14db5047eb9 --- /dev/null +++ b/crates/ironclaw_host_runtime/tests/host_runtime_visible_capability_credential_presence_contract.rs @@ -0,0 +1,645 @@ +//! Caller-level regression tests for `visible_capabilities` credential-presence +//! downgrade to `VisibleCapabilityAccess::NeedsAuth` (issue #5416, Phase 2). +//! +//! `crates/ironclaw_host_runtime/src/surface.rs`'s unit tests pin the +//! downgrade/fingerprint behavior against a fake `CapabilityCredentialPresence`. +//! These tests drive the real production composition instead — a +//! `HostRuntimeServices`-built `DefaultHostRuntime` backed by a real +//! `SecretStore` and a real `RuntimeCredentialAccountResolver` — so the wiring +//! in `services.rs`/`production.rs` (not just the trait contract) is covered. +//! Per `.claude/rules/testing.md` ("Test Through the Caller, Not Just the +//! Helper"): `ProductionCredentialPresence` is a transform with more than one +//! input (secret store presence, resolver outcome) gating a model-visible +//! signal, with `HostRuntimeServices::build_host_runtime` as the wrapper +//! between it and the surface — a unit test on the helper alone would not +//! catch a wiring regression in that wrapper. +//! +//! Helpers/manifests are duplicated from `host_runtime_credential_preflight_contract.rs` +//! (same convention documented there: Rust integration test binaries cannot +//! share helpers across files without a support module, and the duplication is +//! small and intentional). + +mod support; + +use std::sync::Arc; + +use async_trait::async_trait; +use chrono::Utc; +use ironclaw_authorization::GrantAuthorizer; +use ironclaw_extensions::{ExtensionManifest, ExtensionPackage, ExtensionRegistry, ManifestSource}; +use ironclaw_filesystem::LocalFilesystem; +use ironclaw_host_api::*; +use ironclaw_host_runtime::{ + CapabilitySurfacePolicy, CapabilitySurfaceVersion, HostRuntime, HostRuntimeServices, + RuntimeCredentialAccessSecret, RuntimeCredentialAccountRequest, + RuntimeCredentialAccountResolver, SurfaceKind, VisibleCapabilityAccess, + VisibleCapabilityRequest, +}; +use ironclaw_processes::ProcessServices; +use ironclaw_resources::InMemoryResourceGovernor; +use ironclaw_secrets::{ + InMemorySecretStore, SecretLease, SecretLeaseId, SecretMaterial, SecretMetadata, SecretStore, + SecretStoreError, +}; +use ironclaw_trust::{ + AdminConfig, AdminEntry, AuthorityCeiling, EffectiveTrustClass, HostTrustAssignment, + HostTrustPolicy, TrustDecision, TrustProvenance, +}; +use support::legacy_capability_fixture_to_v2; +use tempfile::TempDir; + +// ─── Manifests (duplicated from host_runtime_credential_preflight_contract.rs) ── + +const SCRIPT_WITH_SECRET_HANDLE_MANIFEST: &str = r#" +id = "script" +name = "Script With Secret Handle" +version = "0.1.0" +description = "Script extension that requires a secret-handle credential" +trust = "untrusted" + +[runtime] +kind = "script" +runner = "sandboxed_process" +command = "echo" +args = [] + +[[capabilities]] +id = "script.echo" +description = "Echo through Script" +effects = ["dispatch_capability", "use_secret"] +default_permission = "allow" +parameters_schema = { type = "object" } + +[[capabilities.runtime_credentials]] +handle = "script_api_token" +source = { type = "secret_handle" } +audience = { scheme = "https", host_pattern = "api.example.com" } +target = { type = "header", name = "x-api-key" } +required = true +"#; + +const SCRIPT_WITH_PRODUCT_AUTH_MANIFEST: &str = r#" +id = "script" +name = "Script With Product Auth" +version = "0.1.0" +description = "Script extension that requires a product-auth account credential" +trust = "untrusted" + +[runtime] +kind = "script" +runner = "sandboxed_process" +command = "echo" +args = [] + +[[capabilities]] +id = "script.echo" +description = "Echo through Script" +effects = ["dispatch_capability", "use_secret"] +default_permission = "allow" +parameters_schema = { type = "object" } + +[[capabilities.runtime_credentials]] +handle = "google_oauth_token" +source = { type = "product_auth_account", provider = "google", setup = { kind = "oauth", scopes = ["https://www.googleapis.com/auth/gmail.readonly"] } } +audience = { scheme = "https", host_pattern = "gmail.googleapis.com" } +target = { type = "header", name = "Authorization" } +required = true +"#; + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +/// Builds a registry from a v1-style test manifest, plus the backing +/// `LocalFilesystem` with real schema/prompt files written under a tempdir — +/// `visible_capabilities` (unlike `invoke_capability`, which the sibling +/// `host_runtime_credential_preflight_contract.rs` file drives) resolves each +/// descriptor's `input_schema_ref` against the wired surface filesystem, so +/// the referenced files must actually exist on disk. The returned `TempDir` +/// must be kept alive for the duration of the test (dropping it deletes the +/// files). +fn registry_with_manifest(manifest: &str) -> (TempDir, LocalFilesystem, ExtensionRegistry) { + let manifest_text = legacy_capability_fixture_to_v2(manifest); + let manifest = ExtensionManifest::parse( + &manifest_text, + ManifestSource::InstalledLocal, + &HostPortCatalog::empty(), + ) + .expect("manifest must parse"); + + let storage = tempfile::tempdir().unwrap(); + let extension_dir = storage.path().join(manifest.id.as_str()); + std::fs::create_dir_all(extension_dir.join("schemas/test")).unwrap(); + std::fs::create_dir_all(extension_dir.join("prompts")).unwrap(); + std::fs::write( + extension_dir.join("schemas/test/input.v1.json"), + r#"{"type":"object"}"#, + ) + .unwrap(); + std::fs::write( + extension_dir.join("schemas/test/output.v1.json"), + r#"{"type":"object"}"#, + ) + .unwrap(); + std::fs::write(extension_dir.join("prompts/test.md"), "Test prompt").unwrap(); + + let mut fs = LocalFilesystem::new(); + fs.mount_local( + VirtualPath::new("/system/extensions").unwrap(), + HostPath::from_path_buf(storage.path().to_path_buf()), + ) + .unwrap(); + + let root = VirtualPath::new(format!("/system/extensions/{}", manifest.id.as_str())).unwrap(); + let package = ExtensionPackage::from_manifest(manifest, root).expect("package must build"); + let mut registry = ExtensionRegistry::new(); + registry.insert(package).unwrap(); + (storage, fs, registry) +} + +fn script_capability_id() -> CapabilityId { + CapabilityId::new("script.echo").unwrap() +} + +fn local_manifest_trust_policy( + extension_id: &str, + allowed_effects: Vec, +) -> HostTrustPolicy { + HostTrustPolicy::new(vec![Box::new(AdminConfig::with_entries(vec![ + AdminEntry::for_local_manifest( + PackageId::new(extension_id).unwrap(), + format!("/system/extensions/{extension_id}/manifest.toml"), + None, + HostTrustAssignment::user_trusted(), + allowed_effects, + None, + ), + ]))]) + .unwrap() +} + +/// Execution context carrying a grant for `script.echo` with the given +/// effects, so `GrantAuthorizer` (the real authorizer, not a fake) returns +/// `Allow` and the credential-presence downgrade is the only thing gating +/// `NeedsAuth` vs `Available`. +/// +/// `secrets` mirrors `GrantConstraints.secrets` — the caller's AUTHORITY to +/// use a `SecretHandle`-source credential slot. This is orthogonal to whether +/// the secret's material is actually stored (`SecretStore` presence): a grant +/// can authorize the slot while the store is empty (never completed a +/// paste-token flow) — exactly the "authorized but credential missing" +/// scenario Phase 2 covers. `ironclaw_authorization::obligations_for_grant` +/// denies with `PolicyDenied` (not `NeedsAuth`) if a required `SecretHandle` +/// credential's handle is absent from `secrets`, so callers exercising a +/// `SecretHandle`-source capability must include it here. +fn execution_context_with_script_grant( + effects: Vec, + secrets: Vec, +) -> ExecutionContext { + let grants = CapabilitySet { + grants: vec![CapabilityGrant { + id: CapabilityGrantId::new(), + capability: script_capability_id(), + grantee: Principal::Extension(ExtensionId::new("caller").unwrap()), + issued_by: Principal::HostRuntime, + constraints: GrantConstraints { + allowed_effects: effects, + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets, + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + }], + }; + ExecutionContext::local_default( + UserId::new("user").unwrap(), + ExtensionId::new("caller").unwrap(), + RuntimeKind::Script, + TrustClass::UserTrusted, + grants, + MountView::default(), + ) + .unwrap() +} + +fn visible_request(context: ExecutionContext) -> VisibleCapabilityRequest { + let mut provider_trust = std::collections::BTreeMap::new(); + provider_trust.insert( + ExtensionId::new("script").unwrap(), + TrustDecision { + effective_trust: EffectiveTrustClass::user_trusted(), + authority_ceiling: AuthorityCeiling { + allowed_effects: vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + max_resource_ceiling: None, + }, + provenance: TrustProvenance::Default, + evaluated_at: Utc::now(), + }, + ); + VisibleCapabilityRequest::new(context, SurfaceKind::new("agent_loop").unwrap()) + .with_policy(CapabilitySurfacePolicy::allow_all()) + .with_provider_trust(provider_trust) +} + +#[derive(Debug)] +struct FixedRuntimeCredentialAccountResolver { + result: Result, +} + +#[async_trait] +impl RuntimeCredentialAccountResolver for FixedRuntimeCredentialAccountResolver { + async fn resolve_access_secret( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + self.result + .clone() + .map(|handle| RuntimeCredentialAccessSecret { + scope: request.scope.clone(), + handle, + }) + } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + match &self.result { + Ok(_) => Ok(true), + Err(CredentialStageError::AuthRequired) => Ok(false), + Err(CredentialStageError::Backend) => Err(CredentialStageError::Backend), + } + } +} + +/// Regression guard for issue #5416 Phase 2 Fix A: the capability-surface +/// presence check must call the side-effect-free `account_configured`, never +/// the refresh-performing `resolve_access_secret`. `resolve_access_secret` +/// panics on any call here so a regression to the old wiring fails loudly +/// (a real "silently refreshes tokens on every render" bug would otherwise be +/// invisible from the surface's `NeedsAuth`/`Available` output alone). +#[derive(Debug, Default)] +struct PanicsOnResolveAccountResolver { + resolve_access_secret_calls: std::sync::atomic::AtomicUsize, +} + +#[async_trait] +impl RuntimeCredentialAccountResolver for PanicsOnResolveAccountResolver { + async fn resolve_access_secret( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + self.resolve_access_secret_calls + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + panic!( + "resolve_access_secret must never be called from the capability-surface \ + presence path — it performs a token refresh (a side effect); the presence \ + path must use account_configured instead" + ); + } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(false) + } +} + +/// `SecretStore` wrapper whose `metadata` call fails while `fail.load()` is +/// true and otherwise delegates to a real `InMemorySecretStore`. Used to prove +/// the fail-open + never-cache contract from the caller (not just the +/// `ProductionCredentialPresence` unit level): a `metadata` backend error must +/// leave the capability `Available` AND must not populate the presence cache, +/// so a subsequent render against a healthy store gets the fresh (not stale) +/// answer. +#[derive(Debug, Default)] +struct ToggleableFailureSecretStore { + inner: InMemorySecretStore, + fail: std::sync::atomic::AtomicBool, +} + +impl ToggleableFailureSecretStore { + fn set_failing(&self, failing: bool) { + self.fail + .store(failing, std::sync::atomic::Ordering::SeqCst); + } +} + +#[async_trait] +impl SecretStore for ToggleableFailureSecretStore { + async fn put( + &self, + scope: ResourceScope, + handle: SecretHandle, + material: SecretMaterial, + expires_at: Option, + ) -> Result { + self.inner.put(scope, handle, material, expires_at).await + } + + async fn metadata( + &self, + scope: &ResourceScope, + handle: &SecretHandle, + ) -> Result, SecretStoreError> { + if self.fail.load(std::sync::atomic::Ordering::SeqCst) { + return Err(SecretStoreError::StoreUnavailable { + reason: "simulated backend outage".to_string(), + }); + } + self.inner.metadata(scope, handle).await + } + + async fn metadata_for_scope( + &self, + scope: &ResourceScope, + ) -> Result, SecretStoreError> { + self.inner.metadata_for_scope(scope).await + } + + async fn delete( + &self, + scope: &ResourceScope, + handle: &SecretHandle, + ) -> Result { + self.inner.delete(scope, handle).await + } + + async fn lease_once( + &self, + scope: &ResourceScope, + handle: &SecretHandle, + ) -> Result { + self.inner.lease_once(scope, handle).await + } + + async fn consume( + &self, + scope: &ResourceScope, + lease_id: SecretLeaseId, + ) -> Result { + self.inner.consume(scope, lease_id).await + } + + async fn revoke( + &self, + scope: &ResourceScope, + lease_id: SecretLeaseId, + ) -> Result { + self.inner.revoke(scope, lease_id).await + } + + async fn leases_for_scope( + &self, + scope: &ResourceScope, + ) -> Result, SecretStoreError> { + self.inner.leases_for_scope(scope).await + } +} + +fn only_capability( + surface: &ironclaw_host_runtime::VisibleCapabilitySurface, +) -> VisibleCapabilityAccess { + assert_eq!( + surface.capabilities.len(), + 1, + "expected exactly one visible capability, got {:?}", + surface.capabilities + ); + surface.capabilities[0].access +} + +// ─── Tests ────────────────────────────────────────────────────────────────── + +/// A capability requiring a `SecretHandle` credential with no matching secret +/// in the store must surface as `NeedsAuth`, not `Available` — driven through +/// `HostRuntimeServices::build_host_runtime` (the real production wiring), +/// not just the `CapabilityCredentialPresence` trait in isolation. +#[tokio::test] +async fn generic_secret_capability_needs_auth_when_secret_absent() { + let secret_store = Arc::new(InMemorySecretStore::new()); + // Deliberately do NOT seed "script_api_token" — the store is empty. + + let (_storage, fs, registry) = registry_with_manifest(SCRIPT_WITH_SECRET_HANDLE_MANIFEST); + let services = HostRuntimeServices::new( + Arc::new(registry), + Arc::new(fs), + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(GrantAuthorizer::new()), + ProcessServices::in_memory(), + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy( + "script", + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + ))) + .with_secret_store(Arc::clone(&secret_store)); + let runtime = services.host_runtime_for_local_testing(); + + let context = execution_context_with_script_grant( + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + vec![SecretHandle::new("script_api_token").unwrap()], + ); + let surface = runtime + .visible_capabilities(visible_request(context)) + .await + .unwrap(); + + assert_eq!( + only_capability(&surface), + VisibleCapabilityAccess::NeedsAuth + ); +} + +/// A capability requiring a product-auth account credential whose resolver +/// reports `AuthRequired` (account missing/unconfigured/expired/revoked) must +/// surface as `NeedsAuth`. +#[tokio::test] +async fn product_auth_capability_needs_auth_when_resolver_reports_auth_required() { + let resolver = Arc::new(FixedRuntimeCredentialAccountResolver { + result: Err(CredentialStageError::AuthRequired), + }); + + let (_storage, fs, registry) = registry_with_manifest(SCRIPT_WITH_PRODUCT_AUTH_MANIFEST); + let services = HostRuntimeServices::new( + Arc::new(registry), + Arc::new(fs), + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(GrantAuthorizer::new()), + ProcessServices::in_memory(), + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy( + "script", + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + ))) + .with_runtime_credential_account_resolver(resolver); + let runtime = services.host_runtime_for_local_testing(); + + let context = execution_context_with_script_grant( + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + Vec::new(), + ); + let surface = runtime + .visible_capabilities(visible_request(context)) + .await + .unwrap(); + + assert_eq!( + only_capability(&surface), + VisibleCapabilityAccess::NeedsAuth + ); +} + +/// A resolver `Backend` error (internal staging failure, not attributable to +/// the user's credentials) must fail open — the capability stays `Available` +/// rather than falsely prompting sign-in for a transient backend blip. +#[tokio::test] +async fn product_auth_capability_stays_available_when_resolver_backend_error() { + let resolver = Arc::new(FixedRuntimeCredentialAccountResolver { + result: Err(CredentialStageError::Backend), + }); + + let (_storage, fs, registry) = registry_with_manifest(SCRIPT_WITH_PRODUCT_AUTH_MANIFEST); + let services = HostRuntimeServices::new( + Arc::new(registry), + Arc::new(fs), + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(GrantAuthorizer::new()), + ProcessServices::in_memory(), + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy( + "script", + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + ))) + .with_runtime_credential_account_resolver(resolver); + let runtime = services.host_runtime_for_local_testing(); + + let context = execution_context_with_script_grant( + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + Vec::new(), + ); + let surface = runtime + .visible_capabilities(visible_request(context)) + .await + .unwrap(); + + assert_eq!( + only_capability(&surface), + VisibleCapabilityAccess::Available + ); +} + +/// Issue #5416 Phase 2 Fix A (BLOCKER): the capability-surface presence check +/// must be side-effect-free. `visible_capabilities` runs on every LLM step — +/// if it called `resolve_access_secret` (which performs an OAuth token +/// refresh) instead of the presence-only `account_configured`, every render +/// would burn a network round-trip. `PanicsOnResolveAccountResolver` panics +/// if the old (wrong) method is ever invoked, so this test fails loudly +/// instead of silently passing while re-introducing the side effect. +#[tokio::test] +async fn product_auth_capability_presence_check_never_calls_resolve_access_secret() { + let resolver = Arc::new(PanicsOnResolveAccountResolver::default()); + + let (_storage, fs, registry) = registry_with_manifest(SCRIPT_WITH_PRODUCT_AUTH_MANIFEST); + let services = HostRuntimeServices::new( + Arc::new(registry), + Arc::new(fs), + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(GrantAuthorizer::new()), + ProcessServices::in_memory(), + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy( + "script", + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + ))) + .with_runtime_credential_account_resolver(Arc::clone(&resolver)); + let runtime = services.host_runtime_for_local_testing(); + + let context = execution_context_with_script_grant( + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + Vec::new(), + ); + let surface = runtime + .visible_capabilities(visible_request(context)) + .await + .unwrap(); + + assert_eq!( + only_capability(&surface), + VisibleCapabilityAccess::NeedsAuth, + "account_configured returned Ok(false) so the capability must downgrade" + ); + assert_eq!( + resolver + .resolve_access_secret_calls + .load(std::sync::atomic::Ordering::SeqCst), + 0, + "capability-surface presence check must never invoke resolve_access_secret" + ); +} + +/// PR #5528 review regression (fail-open coverage gap): a `SecretStore::metadata` +/// backend error must fail open (capability stays `Available`, matching +/// `generic_secret_capability_needs_auth_when_secret_absent`'s Ok(None) case +/// staying missing) AND must not populate the credential-presence cache. +/// Proven from the caller (`HostRuntimeServices::build_host_runtime`'s real +/// wiring, not just the `ProductionCredentialPresence` unit) by rendering +/// twice against the SAME runtime instance (so the cache persists across +/// renders): first while the store fails (must stay `Available`), then after +/// the store recovers (must get the FRESH answer — `NeedsAuth`, since the +/// secret was never actually stored). If the first render had wrongly cached +/// anything, the second render would still report the render-1 outcome +/// instead of re-probing live. +#[tokio::test] +async fn generic_secret_capability_stays_available_and_uncached_when_metadata_errors() { + let secret_store = Arc::new(ToggleableFailureSecretStore::default()); + secret_store.set_failing(true); + + let (_storage, fs, registry) = registry_with_manifest(SCRIPT_WITH_SECRET_HANDLE_MANIFEST); + let services = HostRuntimeServices::new( + Arc::new(registry), + Arc::new(fs), + Arc::new(InMemoryResourceGovernor::new()), + Arc::new(GrantAuthorizer::new()), + ProcessServices::in_memory(), + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy( + "script", + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + ))) + .with_secret_store(Arc::clone(&secret_store)); + let runtime = services.host_runtime_for_local_testing(); + + let context = execution_context_with_script_grant( + vec![EffectKind::DispatchCapability, EffectKind::UseSecret], + vec![SecretHandle::new("script_api_token").unwrap()], + ); + + let surface_while_failing = runtime + .visible_capabilities(visible_request(context.clone())) + .await + .unwrap(); + assert_eq!( + only_capability(&surface_while_failing), + VisibleCapabilityAccess::Available, + "a metadata backend error must fail open, not report NeedsAuth" + ); + + secret_store.set_failing(false); + let surface_after_recovery = runtime + .visible_capabilities(visible_request(context)) + .await + .unwrap(); + assert_eq!( + only_capability(&surface_after_recovery), + VisibleCapabilityAccess::NeedsAuth, + "the indeterminate render must not have cached anything — once the store \ + recovers, the capability must get the fresh (never-stored) answer, not a \ + stale value carried over from the failing render" + ); +} diff --git a/crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs b/crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs index af0bd230975..5c09c1b28e2 100644 --- a/crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs +++ b/crates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rs @@ -4499,6 +4499,13 @@ impl RuntimeCredentialAccountResolver for SourceScopedCredentialAccountResolver handle: self.handle.clone(), }) } + + async fn account_configured( + &self, + _request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + Ok(true) + } } #[async_trait::async_trait] diff --git a/crates/ironclaw_loop_support/src/capability_port.rs b/crates/ironclaw_loop_support/src/capability_port.rs index 45645207b71..b9ea57fdc55 100644 --- a/crates/ironclaw_loop_support/src/capability_port.rs +++ b/crates/ironclaw_loop_support/src/capability_port.rs @@ -14,7 +14,7 @@ use ironclaw_host_runtime::{ CapabilityFailureDisposition, HostRuntime, HostRuntimeError, IdempotencyKey, RuntimeBlockedReason, RuntimeCapabilityAuthResumeRequest, RuntimeCapabilityFailure, RuntimeCapabilityOutcome, RuntimeCapabilityRequest, RuntimeCapabilityResumeRequest, - RuntimeFailureKind, + RuntimeFailureKind, VisibleCapabilityAccess, }; use ironclaw_process_sandbox::{SandboxProcessPlan, ValidatedSandboxProcessPlan}; use ironclaw_turns::{ @@ -1591,6 +1591,12 @@ impl LoopCapabilityPort for HostRuntimeLoopCapabilityPort { snapshot .provider_names .insert(provider_tool_name.clone(), capability_id.clone()); + // Fold the connection-state signal (#5416 Phase 3) into ONE + // description binding shared by both the provider tool-def + // snapshot and the descriptor view below, so they can never + // drift from each other. + let description = + describe_with_access(capability.descriptor.description, capability.access); snapshot.capabilities.insert( capability_id.clone(), SurfaceCapabilitySnapshot::Runtime(Box::new( @@ -1598,7 +1604,7 @@ impl LoopCapabilityPort for HostRuntimeLoopCapabilityPort { provider: capability.descriptor.provider.clone(), runtime: capability.descriptor.runtime, estimate: capability.estimated_resources.clone(), - safe_description: capability.descriptor.description.clone(), + safe_description: description.clone(), parameters_schema: capability.descriptor.parameters_schema.clone(), effects: capability.descriptor.effects.clone(), provider_tool_name, @@ -1610,7 +1616,7 @@ impl LoopCapabilityPort for HostRuntimeLoopCapabilityPort { provider: Some(capability.descriptor.provider), runtime: capability.descriptor.runtime, safe_name: capability.descriptor.id.as_str().to_string(), - safe_description: capability.descriptor.description, + safe_description: description, concurrency_hint: concurrency_hint_from_effects(&capability.descriptor.effects), parameters_schema: capability.descriptor.parameters_schema, }) @@ -2523,6 +2529,41 @@ fn loop_surface_version( }) } +/// Fixed, host-authored sentence appended to a capability's model-visible +/// description when its `VisibleCapabilityAccess` is `NeedsAuth` (issue +/// #5416, Phase 3). Kept as a `const` (rather than inlined per call site) so +/// wording tweaks land in one place and tests assert against this symbol, not +/// a duplicated literal. +/// +/// Never interpolate descriptor/user data into this text — see +/// `.claude/rules/agent-loop-capabilities.md` (Invariant 2: safe/model-visible +/// host strings must stay fixed text, not string-built from untrusted input). +pub const CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER: &str = " [Not connected: a required account \ + or credential is missing. Do not claim this tool is ready — ask the user to connect it \ + first; invoking it will require sign-in.]"; + +/// Folds a `VisibleCapabilityAccess` connection-state signal into the +/// model-visible capability description, at the ONE seam +/// (`HostRuntimeLoopCapabilityPort::visible_capabilities`) where a host +/// `VisibleCapability` is mapped onto the loop surface. Both the provider +/// tool-definition snapshot and the `CapabilityDescriptorView` must read the +/// same folded string, so callers should compute it once and reuse it for +/// both — never re-derive it independently, or the two can drift. +/// +/// `RequiresApproval` is deliberately a no-op here: approval gating is +/// already enforced at invocation time (dispatch authorization / lease +/// checks), so no model-visible description change is needed. Out of scope +/// for #5416 Phase 3. +fn describe_with_access(description: String, access: VisibleCapabilityAccess) -> String { + match access { + VisibleCapabilityAccess::Available => description, + VisibleCapabilityAccess::RequiresApproval => description, + VisibleCapabilityAccess::NeedsAuth => { + format!("{description}{CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER}") + } + } +} + fn runtime_resume_replay<'a>( input: Option<&'a Value>, estimate: Option<&'a ResourceEstimate>, @@ -7966,6 +8007,20 @@ mod tests { } } + /// Like [`visible_capability`], but with the given [`VisibleCapabilityAccess`] + /// instead of always `Available` — for #5416 Phase 3 connection-state fold + /// coverage. + fn visible_capability_with_access( + id: CapabilityId, + provider: ExtensionId, + access: VisibleCapabilityAccess, + ) -> VisibleCapability { + VisibleCapability { + access, + ..visible_capability(id, provider) + } + } + fn dummy_runtime() -> Arc { Arc::new(NoopHostRuntime) } @@ -8629,4 +8684,118 @@ mod tests { resolved, ) } + + // ── #5416 Phase 3: connection-state description fold ─────────────────── + + #[test] + fn describe_with_access_available_is_unchanged() { + let description = describe_with_access( + "demo capability".to_string(), + VisibleCapabilityAccess::Available, + ); + assert_eq!(description, "demo capability"); + assert!(!description.contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER)); + } + + #[test] + fn describe_with_access_requires_approval_is_unchanged() { + // Deliberate no-op: approval gating is already enforced at invocation + // time, not on the model-visible description. See the doc comment on + // `describe_with_access`. + let description = describe_with_access( + "demo capability".to_string(), + VisibleCapabilityAccess::RequiresApproval, + ); + assert_eq!(description, "demo capability"); + } + + #[test] + fn describe_with_access_needs_auth_appends_fixed_marker() { + let description = describe_with_access( + "demo capability".to_string(), + VisibleCapabilityAccess::NeedsAuth, + ); + assert_eq!( + description, + format!("demo capability{CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER}") + ); + } + + #[tokio::test] + async fn visible_capabilities_folds_needs_auth_marker_into_description_and_tool_definition() { + let available_id = CapabilityId::new("demo.available").expect("valid capability id"); + let needs_auth_id = CapabilityId::new("demo.needs-auth").expect("valid capability id"); + let provider_id = ExtensionId::new("demo").expect("valid provider id"); + let context = execution_context("thread-capability-needs-auth-marker"); + let run_context = loop_run_context(&context).await; + let runtime = Arc::new(RecordingHostRuntime::new(vec![ + visible_capability(available_id.clone(), provider_id.clone()), + visible_capability_with_access( + needs_auth_id.clone(), + provider_id, + VisibleCapabilityAccess::NeedsAuth, + ), + ])); + let port = HostRuntimeLoopCapabilityPortFactory::new( + runtime, + visible_request(context), + dummy_input_resolver(), + dummy_result_writer(), + dummy_milestone_sink(), + ) + .port_for_run_context(run_context); + + let surface = port + .visible_capabilities(VisibleCapabilityRequest {}) + .await + .expect("visible capabilities load"); + + let available_view = surface + .descriptors + .iter() + .find(|descriptor| descriptor.capability_id == available_id) + .expect("available capability is on the surface"); + assert!( + !available_view + .safe_description + .contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER), + "Available capability must not carry the not-connected marker" + ); + + let needs_auth_view = surface + .descriptors + .iter() + .find(|descriptor| descriptor.capability_id == needs_auth_id) + .expect("needs-auth capability is on the surface"); + assert!( + needs_auth_view + .safe_description + .contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER), + "NeedsAuth capability descriptor view must carry the not-connected marker" + ); + + let definitions = port + .tool_definitions() + .expect("tool definitions build from the cached surface snapshot"); + let available_definition = definitions + .iter() + .find(|definition| definition.capability_id == available_id) + .expect("available capability has a tool definition"); + assert!( + !available_definition + .description + .contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER), + "Available capability's model-visible tool definition must not carry the marker" + ); + let needs_auth_definition = definitions + .iter() + .find(|definition| definition.capability_id == needs_auth_id) + .expect("needs-auth capability has a tool definition"); + assert!( + needs_auth_definition + .description + .contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER), + "NeedsAuth capability's model-visible tool definition must carry the not-connected marker" + ); + } } diff --git a/crates/ironclaw_loop_support/src/lib.rs b/crates/ironclaw_loop_support/src/lib.rs index b70d0b3aeab..33d08e304b1 100644 --- a/crates/ironclaw_loop_support/src/lib.rs +++ b/crates/ironclaw_loop_support/src/lib.rs @@ -54,8 +54,8 @@ pub use capability_allow_set::{ CapabilityAllowSet, CapabilityResolveError, CapabilitySurfaceProfileResolver, }; pub use capability_port::{ - CapabilityResultWrite, CapabilityTrajectoryObserver, CapabilityWriteResult, - DecoratingLoopCapabilityPortFactory, HostRuntimeLoopCapabilityPort, + CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER, CapabilityResultWrite, CapabilityTrajectoryObserver, + CapabilityWriteResult, DecoratingLoopCapabilityPortFactory, HostRuntimeLoopCapabilityPort, HostRuntimeLoopCapabilityPortFactory, LoopCapabilityInputResolver, LoopCapabilityPortDecorator, LoopCapabilityPortFactory, LoopCapabilityResultWriter, concurrency_hint_from_effects, loop_driver_execution_extension_id, diff --git a/crates/ironclaw_product_workflow/src/lib.rs b/crates/ironclaw_product_workflow/src/lib.rs index fbc4793c7e1..a1aaecf2287 100644 --- a/crates/ironclaw_product_workflow/src/lib.rs +++ b/crates/ironclaw_product_workflow/src/lib.rs @@ -109,14 +109,15 @@ pub use inbound_turn::{ }; pub use ledger::{IdempotencyDecision, IdempotencyLedger}; pub use lifecycle::{ - LifecycleBlockerRef, LifecycleCommandKind, LifecycleExtensionCredentialRequirement, - LifecycleExtensionCredentialSetup, LifecycleExtensionOnboarding, LifecycleExtensionRuntimeKind, - LifecycleExtensionSource, LifecycleExtensionSummary, LifecycleExtensionSurfaceKind, - LifecycleInstalledExtensionSummary, LifecyclePackageId, LifecyclePackageKind, - LifecyclePackageRef, LifecyclePhase, LifecycleProductAction, LifecycleProductContext, - LifecycleProductFacade, LifecycleProductPayload, LifecycleProductResponse, - LifecycleProductSurfaceContext, LifecycleReadinessBlocker, LifecycleSearchExtensionSummary, - LifecycleSkillSource, LifecycleSkillSummary, UnsupportedLifecycleProductFacade, + ExtensionAvailability, LifecycleBlockerRef, LifecycleCommandKind, + LifecycleExtensionCredentialRequirement, LifecycleExtensionCredentialSetup, + LifecycleExtensionOnboarding, LifecycleExtensionRuntimeKind, LifecycleExtensionSource, + LifecycleExtensionSummary, LifecycleExtensionSurfaceKind, LifecycleInstalledExtensionSummary, + LifecyclePackageId, LifecyclePackageKind, LifecyclePackageRef, LifecyclePhase, + LifecycleProductAction, LifecycleProductContext, LifecycleProductFacade, + LifecycleProductPayload, LifecycleProductResponse, LifecycleProductSurfaceContext, + LifecycleReadinessBlocker, LifecycleSearchExtensionSummary, LifecycleSkillSource, + LifecycleSkillSummary, UnsupportedLifecycleProductFacade, }; // Product hosts use this outbound orchestration seam to wire outbound policy // decisions to adapter rendering without reaching into module internals. diff --git a/crates/ironclaw_product_workflow/src/lifecycle.rs b/crates/ironclaw_product_workflow/src/lifecycle.rs index 39f8199d011..3c67428d569 100644 --- a/crates/ironclaw_product_workflow/src/lifecycle.rs +++ b/crates/ironclaw_product_workflow/src/lifecycle.rs @@ -147,6 +147,41 @@ pub enum LifecyclePhase { UnsupportedOrLegacy, } +/// Collapsed model-facing connection state for an extension search result +/// (#5416 Phase 0). +/// +/// The model's only questions for "connect X" are "is it installed?" and "is +/// the credential present?" — this is the single projected answer, replacing +/// the raw `LifecyclePhase` (which stays internal; it drives the +/// install/activate tool flow, not the model's connection judgment) and the +/// separate credential-readiness axis. See +/// `docs/plans/2026-07-01-reborn-google-connection-state-5416.md` §4.1. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ExtensionAvailability { + /// Usable now — connect without asking the user for anything. + Available, + /// Installed, but a required credential is missing — run sign-in. + NeedsAuth, + /// No installation record — discover / install it first. + NotInstalled, + /// Could not determine (transient backend) — don't claim either way. + Unknown, +} + +impl ExtensionAvailability { + /// Wire/UI rendering label. Never `format!("{:?}", ..)` — see + /// `.claude/rules/types.md` "Wire-stable enums". + pub fn as_str(self) -> &'static str { + match self { + Self::Available => "available", + Self::NeedsAuth => "needs_auth", + Self::NotInstalled => "not_installed", + Self::Unknown => "unknown", + } + } +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub enum LifecycleReadinessBlocker { @@ -359,7 +394,7 @@ pub struct LifecycleSearchExtensionSummary { #[serde(flatten)] pub summary: LifecycleExtensionSummary, #[serde(default, skip_serializing_if = "Option::is_none")] - pub installation_phase: Option, + pub availability: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs b/crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs index 9620b035868..585420fcc7b 100644 --- a/crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs +++ b/crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs @@ -37,6 +37,22 @@ pub(super) fn for_installed_with_credential_status( if readiness == ExtensionCredentialReadiness::MissingRequired { return credential_onboarding(&extension.summary); } + if readiness == ExtensionCredentialReadiness::Unknown + && matches!( + extension.phase, + LifecyclePhase::Active | LifecyclePhase::Activating | LifecyclePhase::Configured + ) + { + // #5416 Defect B: an `Unknown` credential readiness on an extension that + // was plausibly connected (Active/Activating/Configured) must stay + // actionable via reconnect/reverify, not fall through to generic install + // onboarding that would tell an already-connected user to start setup + // from scratch. For a not-yet-connected phase (Installed/Failed/…), + // `Unknown` falls through to the normal setup onboarding below — the + // user genuinely still needs initial setup, and "reverify" copy would be + // misleading for something never connected. + return reverify_onboarding(&extension.summary); + } if readiness == ExtensionCredentialReadiness::Configured && matches!( extension.phase, @@ -53,6 +69,30 @@ pub(super) fn for_installed_with_credential_status( for_installed(extension) } +/// Reconnect/reverify affordance for a credential readiness that could not be +/// determined. Distinct from `credential_onboarding` (genuinely missing +/// credential): this extension may already be connected, so the copy asks the +/// user to reverify rather than walking through setup from scratch. +fn reverify_onboarding(summary: &LifecycleExtensionSummary) -> ExtensionOnboarding { + let instructions = format!( + "{} connection status could not be confirmed right now. Reverify or reconnect its credentials to restore access.", + summary.name + ); + ExtensionOnboarding { + state: Some(RebornExtensionOnboardingState::ReverifyRequired), + onboarding: Some(RebornExtensionOnboardingPayload { + credential_instructions: Some(instructions.clone()), + setup_url: setup_url(summary), + credential_next_step: Some(format!( + "Reconnect {} to confirm its credentials are still valid.", + summary.name + )), + }), + instructions: Some(instructions), + awaiting_token: None, + } +} + pub(super) fn from_lifecycle(lifecycle: &LifecycleProductResponse) -> ExtensionOnboarding { let Some(LifecycleProductPayload::ExtensionList { extensions, .. }) = &lifecycle.payload else { return ExtensionOnboarding::empty(); @@ -425,6 +465,48 @@ mod tests { ); } + #[test] + fn unknown_credential_readiness_projects_reverify_state_not_generic_onboarding() { + // #5416 Defect B: an `Active` extension whose credential backend blipped + // (`Unknown`) must stay actionable — a reconnect/reverify affordance, + // not generic "not connected" install onboarding with no way to fix it. + let extension = installed_extension( + "gmail", + "Gmail", + LifecyclePhase::Active, + vec![oauth_requirement("gmail_account", "google")], + LifecycleExtensionRuntimeKind::FirstParty, + Some(LifecycleExtensionOnboarding { + instructions: "Gmail needs Google OAuth authorization before mail tools can run." + .to_string(), + credential_instructions: Some( + "Authorize the Google account that IronClaw should use for Gmail.".to_string(), + ), + setup_url: None, + credential_next_step: Some( + "After authorization completes, activate Gmail to publish its tools." + .to_string(), + ), + }), + ); + + let onboarding = + for_installed_with_credential_status(&extension, ExtensionCredentialReadiness::Unknown); + + assert_eq!( + onboarding.state, + Some(RebornExtensionOnboardingState::ReverifyRequired) + ); + assert!( + onboarding + .instructions + .as_deref() + .is_some_and(|instructions| instructions.contains("could not be confirmed")), + "expected a reverify-specific message, got {:?}", + onboarding.instructions + ); + } + #[test] fn lifecycle_projection_uses_matching_package_ref() { let lifecycle = LifecycleProductResponse { diff --git a/crates/ironclaw_product_workflow/src/reborn_services/extensions.rs b/crates/ironclaw_product_workflow/src/reborn_services/extensions.rs index e9cdf0d3cf3..8fe3d154c68 100644 --- a/crates/ironclaw_product_workflow/src/reborn_services/extensions.rs +++ b/crates/ironclaw_product_workflow/src/reborn_services/extensions.rs @@ -264,8 +264,13 @@ fn extension_info( let authenticated = match readiness { ExtensionCredentialReadiness::NotRequired => lifecycle_authenticated, ExtensionCredentialReadiness::Configured => true, - ExtensionCredentialReadiness::MissingRequired => false, - ExtensionCredentialReadiness::Unknown => lifecycle_authenticated, + // #5416 Defect B: fail closed. An unresolved credential backend must + // not read as "connected" just because the lifecycle phase looks + // active — that's exactly the fail-open contradiction the settings + // list must not repeat. + ExtensionCredentialReadiness::MissingRequired | ExtensionCredentialReadiness::Unknown => { + false + } }; let onboarding = extension_onboarding::for_installed_with_credential_status(&installed, readiness); @@ -279,11 +284,16 @@ fn extension_info( authenticated, active: phase == LifecyclePhase::Active, tools: summary.visible_capability_ids, - needs_setup: readiness == ExtensionCredentialReadiness::MissingRequired - || matches!( - phase, - LifecyclePhase::Installed | LifecyclePhase::Configured | LifecyclePhase::Failed - ), + // `Unknown` also stays actionable (paired with `authenticated: false` + // above) rather than landing on `authenticated: false, needs_setup: + // false` — a dead-end state with no way to fix it. + needs_setup: matches!( + readiness, + ExtensionCredentialReadiness::MissingRequired | ExtensionCredentialReadiness::Unknown + ) || matches!( + phase, + LifecyclePhase::Installed | LifecyclePhase::Configured | LifecyclePhase::Failed + ), has_auth, activation_status: Some(phase_status(phase).to_string()), activation_error: None, @@ -458,7 +468,13 @@ mod tests { } #[tokio::test] - async fn list_preserves_lifecycle_state_when_credential_status_is_retryably_unavailable() { + async fn list_fails_closed_and_stays_actionable_when_credential_status_is_retryably_unavailable() + { + // #5416 Defect B: a retryable credential-status outage on an `Active` + // extension must project `Unknown` readiness fail-CLOSED (not the old + // fail-open `lifecycle_authenticated` fallback), while remaining + // actionable — a reconnect/reverify affordance, not a dead-end + // "not connected, no way to fix it" state. let facade = ListingFacade { extension: LifecycleInstalledExtensionSummary { summary: summary_with_onboarding(), @@ -472,13 +488,20 @@ mod tests { .expect("list extensions"); let extension = response.extensions.first().expect("one extension"); - assert!(extension.active); + assert!(extension.active, "lifecycle activation remains visible"); assert!( - extension.authenticated, - "retryable status outages should not be projected as missing credentials" + !extension.authenticated, + "an unresolved credential backend must not read as connected" + ); + assert!( + extension.needs_setup, + "Unknown readiness must stay actionable, not a dead end" + ); + assert_eq!( + extension.onboarding_state, + Some(RebornExtensionOnboardingState::ReverifyRequired), + "Unknown readiness should surface a reconnect/reverify affordance, not generic install onboarding" ); - assert!(!extension.needs_setup); - assert!(extension.onboarding_state.is_none()); } #[tokio::test] @@ -1008,7 +1031,7 @@ mod tests { ) -> LifecycleSearchExtensionSummary { LifecycleSearchExtensionSummary { summary, - installation_phase: None, + availability: None, } } } diff --git a/crates/ironclaw_product_workflow/src/reborn_services/types.rs b/crates/ironclaw_product_workflow/src/reborn_services/types.rs index e97cd2a6226..8781e050620 100644 --- a/crates/ironclaw_product_workflow/src/reborn_services/types.rs +++ b/crates/ironclaw_product_workflow/src/reborn_services/types.rs @@ -1171,6 +1171,12 @@ pub enum RebornExtensionOnboardingState { SetupRequired, Installed, Failed, + /// Credential readiness could not be determined (transient backend + /// failure) — distinct from `AuthRequired` (a genuinely missing + /// credential): the extension may already be connected, so onboarding + /// should offer to reconnect/reverify rather than generic setup copy. + /// #5416 Defect B. + ReverifyRequired, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/ironclaw_product_workflow/tests/reborn_services_contract.rs b/crates/ironclaw_product_workflow/tests/reborn_services_contract.rs index b84919ead00..e3d6fc145b0 100644 --- a/crates/ironclaw_product_workflow/tests/reborn_services_contract.rs +++ b/crates/ironclaw_product_workflow/tests/reborn_services_contract.rs @@ -4878,6 +4878,17 @@ async fn list_extensions_projects_onboarding_payload_through_reborn_services() { let extension = response.extensions.first().expect("one extension"); assert_eq!(extension.tools, vec!["github.read", "github.write"]); + // No `ExtensionCredentialSetupService` is wired above, so credential + // readiness for this required requirement resolves to `Unknown` (#5416 + // Defect B) — not `MissingRequired`. The extension's phase here is + // `Installed` (never yet connected), which `for_installed_with_credential_status` + // deliberately excludes from the reconnect/reverify affordance (that copy + // would be misleading for something that was never connected) — so + // `Unknown` falls through to the normal setup onboarding, same as before. + // The reverify-specific projection is covered by + // `extension_onboarding::tests::unknown_credential_readiness_projects_reverify_state_not_generic_onboarding` + // and `extensions::tests::list_fails_closed_and_stays_actionable_when_credential_status_is_retryably_unavailable`, + // both of which use an `Active` phase. assert_eq!( extension.onboarding_state, Some(RebornExtensionOnboardingState::SetupRequired) diff --git a/crates/ironclaw_reborn_composition/src/extension_availability.rs b/crates/ironclaw_reborn_composition/src/extension_availability.rs new file mode 100644 index 00000000000..932af8c64ec --- /dev/null +++ b/crates/ironclaw_reborn_composition/src/extension_availability.rs @@ -0,0 +1,144 @@ +//! Collapsed model-facing connection-state projection for the extension +//! search path (#5416 Phase 0). +//! +//! A Google (or any credentialed) extension has two independent axes — +//! lifecycle (installed/enabled) and credential presence — and the search +//! surface used to answer "is it ready?" from the lifecycle axis alone. This +//! module is the single place that projects "is it installed?" plus "is the +//! credential present?" into the one value the model actually needs: +//! [`ExtensionAvailability`]. See +//! `docs/plans/2026-07-01-reborn-google-connection-state-5416.md` §4.1. + +use ironclaw_extensions::ExtensionInstallation; +use ironclaw_host_api::CredentialStageError; +use ironclaw_product_workflow::{ + ExtensionAvailability, LifecycleSearchExtensionSummary, ProductWorkflowError, +}; + +use crate::available_extensions::AvailableExtensionPackage; +use crate::extension_activation_credentials::RuntimeExtensionActivationCredentialGate; +use crate::extension_credential_requirements::package_runtime_credential_auth_requirements; + +/// Fan-out width for per-extension credential-readiness checks in the search +/// loop. Mirrors `reborn_services/extensions.rs::EXTENSION_READINESS_CONCURRENCY` +/// (the list path's identical pattern) — kept as a separate constant because +/// the two live in different crates. +pub(crate) const EXTENSION_READINESS_CONCURRENCY: usize = 8; + +/// Pure projection: `(is the extension installed, credential gate outcome) -> +/// ExtensionAvailability`. +/// +/// `credentials_satisfied` is `None` when the extension has no required +/// credentials to gate on (nothing to check); otherwise it carries the +/// credential gate's `Ok(all_required_present)` / `Err(stage)` outcome. +pub(crate) fn project_availability( + installed: bool, + credentials_satisfied: Option>, +) -> ExtensionAvailability { + if !installed { + return ExtensionAvailability::NotInstalled; + } + match credentials_satisfied { + None | Some(Ok(true)) => ExtensionAvailability::Available, + Some(Ok(false)) => ExtensionAvailability::NeedsAuth, + Some(Err(CredentialStageError::Backend)) => ExtensionAvailability::Unknown, + // Dead in practice: `missing_requirements` folds `AuthRequired` into + // `Ok(false)` upstream (product_auth_runtime_credentials.rs). Handled + // for exhaustiveness with the same fail-mode as a known-missing + // credential rather than silently panicking on a future caller. + Some(Err(CredentialStageError::AuthRequired)) => ExtensionAvailability::NeedsAuth, + } +} + +/// `ready` predicate: does at least one summary project to `Available`? +pub(crate) fn any_available(extensions: &[LifecycleSearchExtensionSummary]) -> bool { + extensions + .iter() + .any(|extension| extension.availability == Some(ExtensionAvailability::Available)) +} + +/// Resolves the full `(installation present?, gate result)` fact for one +/// extension into `ExtensionAvailability`, consulting the credential gate only +/// when the package actually declares required credentials. +pub(crate) async fn resolve_extension_availability( + extension: &AvailableExtensionPackage, + installation: Option<&ExtensionInstallation>, + credential_gate: Option<&RuntimeExtensionActivationCredentialGate>, +) -> Result { + if installation.is_none() { + return Ok(project_availability(false, None)); + } + let requirements = package_runtime_credential_auth_requirements(&extension.package); + if requirements.is_empty() { + return Ok(project_availability(true, None)); + } + let Some(credential_gate) = credential_gate else { + return Ok(project_availability(true, Some(Ok(false)))); + }; + let outcome = credential_gate + .missing_requirements(requirements) + .await + .map(|missing| missing.is_empty()); + Ok(project_availability(true, Some(outcome))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn not_installed_is_not_installed_regardless_of_credentials() { + assert_eq!( + project_availability(false, None), + ExtensionAvailability::NotInstalled + ); + assert_eq!( + project_availability(false, Some(Ok(true))), + ExtensionAvailability::NotInstalled + ); + assert_eq!( + project_availability(false, Some(Err(CredentialStageError::Backend))), + ExtensionAvailability::NotInstalled + ); + } + + #[test] + fn installed_with_no_required_credentials_is_available() { + assert_eq!( + project_availability(true, None), + ExtensionAvailability::Available + ); + } + + #[test] + fn installed_with_satisfied_credentials_is_available() { + assert_eq!( + project_availability(true, Some(Ok(true))), + ExtensionAvailability::Available + ); + } + + #[test] + fn installed_with_missing_credentials_needs_auth() { + assert_eq!( + project_availability(true, Some(Ok(false))), + ExtensionAvailability::NeedsAuth + ); + } + + #[test] + fn installed_with_backend_error_is_unknown() { + assert_eq!( + project_availability(true, Some(Err(CredentialStageError::Backend))), + ExtensionAvailability::Unknown + ); + } + + #[test] + fn installed_with_auth_required_error_needs_auth() { + assert_eq!( + project_availability(true, Some(Err(CredentialStageError::AuthRequired))), + ExtensionAvailability::NeedsAuth + ); + } +} diff --git a/crates/ironclaw_reborn_composition/src/extension_lifecycle.rs b/crates/ironclaw_reborn_composition/src/extension_lifecycle.rs index 0fc0d4e7f14..74822f035ab 100644 --- a/crates/ironclaw_reborn_composition/src/extension_lifecycle.rs +++ b/crates/ironclaw_reborn_composition/src/extension_lifecycle.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeSet, sync::Arc}; +use futures::{StreamExt, TryStreamExt, stream}; use ironclaw_extensions::{ CapabilityVisibility, ExtensionActivationState, ExtensionError, ExtensionInstallation, ExtensionInstallationError, ExtensionInstallationId, ExtensionInstallationStore, @@ -13,9 +14,9 @@ use ironclaw_host_api::{ sha256_digest_token, }; use ironclaw_product_workflow::{ - LifecycleExtensionSummary, LifecycleInstalledExtensionSummary, LifecyclePackageKind, - LifecyclePackageRef, LifecyclePhase, LifecycleProductPayload, LifecycleProductResponse, - LifecycleSearchExtensionSummary, ProductWorkflowError, + ExtensionAvailability, LifecycleExtensionSummary, LifecycleInstalledExtensionSummary, + LifecyclePackageKind, LifecyclePackageRef, LifecyclePhase, LifecycleProductPayload, + LifecycleProductResponse, LifecycleSearchExtensionSummary, ProductWorkflowError, }; use tokio::sync::Mutex; @@ -31,6 +32,9 @@ use crate::extension_activation_credentials::{ ExtensionActivationCredentialGate, RuntimeExtensionActivationCredentialGate, UnavailableExtensionActivationCredentialGate, }; +use crate::extension_availability::{ + EXTENSION_READINESS_CONCURRENCY, any_available, resolve_extension_availability, +}; use crate::extension_credential_requirements::package_runtime_credential_auth_requirements; use crate::lifecycle::response_with_payload; use crate::mcp_discovery::{ @@ -190,11 +194,23 @@ impl RebornLocalExtensionManagementPort { query: &str, credential_gate: Option<&RuntimeExtensionActivationCredentialGate>, ) -> Result { - let extensions = self.catalog.search(query); - let mut summaries = Vec::new(); - for extension in extensions { - summaries.push(self.search_summary(extension, credential_gate).await?); - } + // Every result's credential readiness is now checked (not just + // `Installed`-phase ones, see #5416 A1), which widens this into an + // O(extensions × requirements) await chain. Fan out with the same + // bounded concurrency the list path uses + // (`reborn_services/extensions.rs::lifecycle_extension_infos`). + // + // Indexed by position (rather than `.map()` over the borrowed items + // directly) to sidestep a higher-ranked-lifetime inference limitation: + // a closure taking a bare `&AvailableExtensionPackage` and returning + // this method's opaque `impl Future` gets inferred as `for<'r> Fn(&'r + // ..)`, which the single-lifetime future type cannot satisfy. + let extensions: Vec<&AvailableExtensionPackage> = self.catalog.search(query).collect(); + let summaries: Vec = stream::iter(0..extensions.len()) + .map(|index| self.search_summary(extensions[index], credential_gate)) + .buffered(EXTENSION_READINESS_CONCURRENCY) + .try_collect() + .await?; let count = summaries.len(); let mut response = response_with_payload( None, @@ -321,17 +337,14 @@ impl RebornLocalExtensionManagementPort { credential_gate: Option<&RuntimeExtensionActivationCredentialGate>, ) -> Result { let mut summary = extension.summary(); - suppress_search_credential_onboarding(&mut summary); - let Some(installation) = self.search_installation(&extension.package.id).await? else { - return Ok(LifecycleSearchExtensionSummary { - summary, - installation_phase: None, - }); - }; - let phase = search_installation_phase(extension, &installation, credential_gate).await?; + let installation = self.search_installation(&extension.package.id).await?; + let availability = + resolve_extension_availability(extension, installation.as_ref(), credential_gate) + .await?; + apply_search_availability_onboarding(&mut summary, availability); Ok(LifecycleSearchExtensionSummary { summary, - installation_phase: Some(phase), + availability: Some(availability), }) } @@ -1190,73 +1203,26 @@ fn phase_for_activation_state(state: ExtensionActivationState) -> LifecyclePhase } } -async fn search_installation_phase( - extension: &AvailableExtensionPackage, - installation: &ExtensionInstallation, - credential_gate: Option<&RuntimeExtensionActivationCredentialGate>, -) -> Result { - let phase = phase_for_activation_state(installation.activation_state()); - if phase != LifecyclePhase::Installed { - return Ok(phase); - } - if search_credentials_configured(extension, credential_gate).await? { - return Ok(LifecyclePhase::Configured); - } - Ok(phase) -} - -async fn search_credentials_configured( - extension: &AvailableExtensionPackage, - credential_gate: Option<&RuntimeExtensionActivationCredentialGate>, -) -> Result { - let requirements = package_runtime_credential_auth_requirements(&extension.package); - if requirements.is_empty() { - return Ok(false); +/// Availability-driven onboarding handling for a search result (#5416 §4.3): +/// `available` clears the requirements/onboarding a searcher no longer needs +/// to act on; `needs_auth` / `unknown` keep them (the model needs the +/// actionable how-to-connect detail); `not_installed` is left unchanged (raw +/// discovery copy). +fn apply_search_availability_onboarding( + summary: &mut LifecycleExtensionSummary, + availability: ExtensionAvailability, +) { + if availability == ExtensionAvailability::Available { + summary.credential_requirements.clear(); + summary.onboarding = None; } - let Some(credential_gate) = credential_gate else { - return Ok(false); - }; - Ok(credential_gate - .missing_requirements(requirements) - .await - .map_err(map_search_credential_stage_error)? - .is_empty()) -} - -fn suppress_search_credential_onboarding(summary: &mut LifecycleExtensionSummary) { - summary.credential_requirements.clear(); - summary.onboarding = None; } fn extension_search_has_ready_result(payload: Option<&LifecycleProductPayload>) -> bool { let Some(LifecycleProductPayload::ExtensionSearch { extensions, .. }) = payload else { return false; }; - extensions.iter().any(|extension| { - matches!( - extension.installation_phase, - Some(LifecyclePhase::Configured | LifecyclePhase::Active) - ) && extension.summary.credential_requirements.is_empty() - && extension.summary.onboarding.is_none() - }) -} - -fn map_search_credential_stage_error( - error: ironclaw_host_api::CredentialStageError, -) -> ProductWorkflowError { - match error { - ironclaw_host_api::CredentialStageError::AuthRequired => { - ProductWorkflowError::InvalidBindingRequest { - reason: "extension requires product auth credentials before search can project configured state".to_string(), - } - } - ironclaw_host_api::CredentialStageError::Backend => { - ProductWorkflowError::Transient { - reason: "extension product auth credential state is temporarily unavailable" - .to_string(), - } - } - } + any_available(extensions) } fn map_extension_error(error: ExtensionError) -> ProductWorkflowError { @@ -2356,7 +2322,7 @@ mod tests { .iter() .find(|extension| extension.summary.package_ref.id.as_str() == "github") .expect("github search result"); - assert_eq!(github.installation_phase, Some(LifecyclePhase::Configured)); + assert_eq!(github.availability, Some(ExtensionAvailability::Available)); assert!( github.summary.credential_requirements.is_empty(), "configured inactive GitHub search results must not expose satisfied PAT requirements" @@ -2415,7 +2381,7 @@ mod tests { .iter() .find(|extension| extension.summary.package_ref.id.as_str() == "github") .expect("github search result"); - assert_eq!(github.installation_phase, Some(LifecyclePhase::Active)); + assert_eq!(github.availability, Some(ExtensionAvailability::Available)); assert!( github.summary.credential_requirements.is_empty(), "active GitHub search results must not expose satisfied PAT requirements" @@ -2452,7 +2418,10 @@ mod tests { } #[tokio::test] - async fn extension_lifecycle_search_reports_credential_backend_failure_as_transient() { + async fn extension_lifecycle_search_reports_credential_backend_failure_as_unknown() { + // #5416 §4.4 "Backend degrade": a credential backend blip must not + // fail the whole search — it degrades that one result to `unknown` + // (fail-closed for the "ready" claim) while search still succeeds. let (_dir, _storage_root, facade, _active_registry, _installation_store) = github_extension_lifecycle_fixture(); let facade = facade.with_runtime_credential_accounts(Arc::new( @@ -2470,7 +2439,7 @@ mod tests { ) .await .expect("install extension"); - let error = facade + let search = facade .execute( lifecycle_surface_context(), LifecycleProductAction::ExtensionSearch { @@ -2478,9 +2447,27 @@ mod tests { }, ) .await - .expect_err("search reports credential backend failure"); + .expect("search succeeds despite the credential backend blip"); - assert!(matches!(error, ProductWorkflowError::Transient { .. })); + assert!( + search.message.is_none(), + "an unknown-availability result must not claim ready, got {:?}", + search.message + ); + let Some(LifecycleProductPayload::ExtensionSearch { extensions, .. }) = + search.payload.as_ref() + else { + panic!("expected extension search payload"); + }; + let github = extensions + .iter() + .find(|extension| extension.summary.package_ref.id.as_str() == "github") + .expect("github search result"); + assert_eq!(github.availability, Some(ExtensionAvailability::Unknown)); + assert!( + !github.summary.credential_requirements.is_empty(), + "unknown-availability search results must retain actionable credential requirements" + ); } #[tokio::test] diff --git a/crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs b/crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs index 5e6a5713cfd..3e24c99a646 100644 --- a/crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs +++ b/crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs @@ -501,7 +501,12 @@ mod tests { } #[tokio::test] - async fn local_dev_extension_search_hides_onboarding_after_credentialed_activation() { + async fn local_dev_extension_search_availability_gates_onboarding_and_ready_message() { + // #5416 Phase 0: the collapsed `availability` field — not the raw + // lifecycle phase — decides both the ready message and whether + // credential_requirements/onboarding are cleared. `not_installed` and + // `needs_auth` keep the actionable how-to-connect detail; only + // `available` clears it. let dir = tempfile::tempdir().expect("tempdir"); let services = build_reborn_services(RebornBuildInput::local_dev( "extension-tools-active-search-owner", @@ -524,14 +529,16 @@ mod tests { .iter() .find(|extension| extension["package_ref"]["id"] == "github") .expect("github search result"); - assert_eq!(available_github.get("installation_phase"), None); + assert_eq!(available_github["availability"], "not_installed"); assert!( - available_github.get("credential_requirements").is_none(), - "available GitHub model-visible search results must not expose PAT requirements before activation" + available_github + .get("credential_requirements") + .is_some_and(|requirements| requirements.as_array().is_some_and(|r| !r.is_empty())), + "not-installed GitHub discovery results should show how to connect once installed" ); assert!( - available_github.get("onboarding").is_none(), - "available GitHub model-visible search results must not expose PAT setup onboarding before activation" + available_github.get("onboarding").is_some(), + "not-installed GitHub discovery results should show setup onboarding" ); invoke_json( @@ -556,14 +563,23 @@ mod tests { .iter() .find(|extension| extension["package_ref"]["id"] == "github") .expect("github search result"); - assert_eq!(installed_github["installation_phase"], "installed"); + assert_eq!(installed_github["availability"], "needs_auth"); assert!( - installed_github.get("credential_requirements").is_none(), - "installed inactive GitHub model-visible search results must not expose stale PAT requirements before activation" + !installed_search["message"] + .as_str() + .unwrap_or_default() + .contains("already configured or active"), + "an installed-but-uncredentialed GitHub must not be reported ready" + ); + assert!( + installed_github + .get("credential_requirements") + .is_some_and(|requirements| requirements.as_array().is_some_and(|r| !r.is_empty())), + "installed inactive GitHub with a missing credential should expose actionable PAT requirements" ); assert!( - installed_github.get("onboarding").is_none(), - "installed inactive GitHub model-visible search results must not expose stale PAT setup onboarding before activation" + installed_github.get("onboarding").is_some(), + "installed inactive GitHub with a missing credential should expose setup onboarding" ); let activate_context = execution_context([EXTENSION_ACTIVATE_CAPABILITY_ID]); @@ -589,7 +605,7 @@ mod tests { .iter() .find(|extension| extension["package_ref"]["id"] == "github") .expect("github search result"); - assert_eq!(github["installation_phase"], "configured"); + assert_eq!(github["availability"], "available"); assert!( github.get("credential_requirements").is_none(), "configured GitHub model-visible search results must not expose satisfied PAT requirements" @@ -628,7 +644,7 @@ mod tests { .iter() .find(|extension| extension["package_ref"]["id"] == "github") .expect("github search result"); - assert_eq!(github["installation_phase"], "active"); + assert_eq!(github["availability"], "available"); assert!( github.get("credential_requirements").is_none(), "active GitHub model-visible search results must not expose satisfied PAT requirements" diff --git a/crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs b/crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs index 554c25a1278..e689033e630 100644 --- a/crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs +++ b/crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs @@ -338,7 +338,7 @@ mod tests { credential_requirements: Vec::new(), onboarding: None, }, - installation_phase: None, + availability: None, }], }), }; diff --git a/crates/ironclaw_reborn_composition/src/lib.rs b/crates/ironclaw_reborn_composition/src/lib.rs index f28afb927ea..2dca6d3e57f 100644 --- a/crates/ironclaw_reborn_composition/src/lib.rs +++ b/crates/ironclaw_reborn_composition/src/lib.rs @@ -38,6 +38,7 @@ mod credential_refresh_worker; mod default_system_prompt; mod error; mod extension_activation_credentials; +mod extension_availability; mod extension_credential_requirements; mod extension_installation_store; mod extension_lifecycle; diff --git a/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs b/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs index 091a1f36c38..b10c429535b 100644 --- a/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs +++ b/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs @@ -570,6 +570,46 @@ impl RuntimeCredentialAccountResolver for ProductAuthRuntimeCredentialResolver { handle, }) } + + /// Side-effect-free presence check for the capability-surface downgrade + /// path (issue #5416, Phase 2, Fix A). Deliberately uses + /// `select_unique_configured_runtime_account` ONLY — no refresh + /// round-trip — so this is safe to call on every `visible_capabilities` + /// render. `resolve_access_secret` above stays the dispatch-time path + /// that stages/refreshes the actual secret material. + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + let selection_request = runtime_credential_account_selection_request( + request.scope, + request.provider, + request.setup.clone(), + request.provider_scopes, + request.requester_extension, + )?; + match self + .accounts + .select_unique_configured_runtime_account(selection_request) + .await + { + Ok(account) if account.status != CredentialAccountStatus::Configured => Ok(false), + // A `Configured` account with no `access_secret` indicates data + // corruption, not a re-auth prompt — mirrors `resolve_access_secret`'s + // classification above (see the comment on that branch). The durable + // product-auth store preserves the Configured ↔ access_secret=Some + // invariant, so this branch can only fire on corrupt state; return + // `Backend` (indeterminate, not cached, capability stays `Available`) + // rather than `Ok(false)` (which the surface reads as `NeedsAuth` and + // re-auth cannot fix). + Ok(account) if account.access_secret.is_none() => Err(CredentialStageError::Backend), + Ok(_) => Ok(true), + Err(error) => match map_account_error(error) { + CredentialStageError::AuthRequired => Ok(false), + CredentialStageError::Backend => Err(CredentialStageError::Backend), + }, + } + } } fn runtime_credential_account_selection_request( diff --git a/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs b/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs index 0e75f04e668..c9fab78ec5b 100644 --- a/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs +++ b/crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs @@ -1582,6 +1582,74 @@ async fn resolver_maps_configured_account_without_access_secret_to_backend() { assert_eq!(error, CredentialStageError::Backend); } +/// PR #5528 review regression: `account_configured` (the side-effect-free +/// capability-surface presence check) must mirror `resolve_access_secret`'s +/// data-corruption classification above. Before the fix it collapsed a +/// `Configured` account with no `access_secret` into `Ok(false)` — the same +/// bucket as "no account configured yet" — which the capability-surface +/// downgrade path (`surface.rs`) reads as `NeedsAuth` and prompts the user to +/// re-authenticate. Re-auth cannot fix data corruption (the durable store +/// preserves Configured ↔ access_secret=Some, so this state means something +/// went wrong server-side, not that the user needs to sign in again); the +/// correct outcome is `Err(Backend)`, which the presence path treats as +/// indeterminate (fail-open, not cached, capability stays `Available`). +#[tokio::test] +async fn account_configured_maps_configured_account_without_access_secret_to_backend() { + let accounts = Arc::new(InMemoryAuthProductServices::new()); + let scope = + ResourceScope::local_default(UserId::new("alice").unwrap(), InvocationId::new()).unwrap(); + let auth_scope = AuthProductScope::new(scope.clone(), AuthSurface::Api); + ConfiguredAccount::new(auth_scope, "github") + // Configured but missing secret — data corruption + .access_secret(None) + .create(&accounts) + .await; + let resolver = resolver_with_accounts(accounts); + + let error = resolver + .account_configured(RuntimeCredentialAccountRequest { + scope: &scope, + provider: &RuntimeCredentialAccountProviderId::new("github").unwrap(), + setup: &RuntimeCredentialAccountSetup::ManualToken, + provider_scopes: &[], + requester_extension: &ExtensionId::new("github").unwrap(), + }) + .await + .unwrap_err(); + + assert_eq!(error, CredentialStageError::Backend); +} + +/// Control for the regression above: an account that is genuinely not +/// `Configured` (no data corruption, just no account yet) must still resolve +/// to `Ok(false)`, not `Err`. +#[tokio::test] +async fn account_configured_maps_unconfigured_account_status_to_ok_false() { + let accounts = Arc::new(InMemoryAuthProductServices::new()); + let scope = + ResourceScope::local_default(UserId::new("alice").unwrap(), InvocationId::new()).unwrap(); + let auth_scope = AuthProductScope::new(scope.clone(), AuthSurface::Api); + ConfiguredAccount::new(auth_scope, "github") + .status(CredentialAccountStatus::PendingSetup) + .access_secret(None) + .create(&accounts) + .await; + let resolver = resolver_with_accounts(accounts); + + let present = resolver + .account_configured(RuntimeCredentialAccountRequest { + scope: &scope, + provider: &RuntimeCredentialAccountProviderId::new("github").unwrap(), + setup: &RuntimeCredentialAccountSetup::ManualToken, + provider_scopes: &[], + requester_extension: &ExtensionId::new("github").unwrap(), + }) + .await + .unwrap(); + + assert!(!present); +} + #[tokio::test] async fn activation_preflight_maps_configured_account_without_access_secret_to_backend() { let accounts = Arc::new(InMemoryAuthProductServices::new()); diff --git a/docs/plans/2026-07-01-reborn-google-connection-state-5416.md b/docs/plans/2026-07-01-reborn-google-connection-state-5416.md new file mode 100644 index 00000000000..aa0f0c5a217 --- /dev/null +++ b/docs/plans/2026-07-01-reborn-google-connection-state-5416.md @@ -0,0 +1,376 @@ +# Reborn Google connection-state contradiction (#5416) + +**Status:** plan (pre-implementation) +**Issue:** nearai/ironclaw#5416 — `[QA] Incorrect Google connection state causes contradictory authentication flow` (label `bug_bash_P2`) +**Scope:** Reborn stack only. Engine v1/v2 out of scope. + +## 1. Symptom + +Fresh user, no Google connected. User: "connect to Gmail". Agent replies "Gmail is +already connected". User questions it; agent flips to "only installed, not +activated" and then activates it. Two contradictory answers, eroded trust. + +## 2. Root cause + +A Google extension has **two independent axes**: + +- **lifecycle** — is the extension installed / enabled (`LifecyclePhase`) +- **credential** — does a valid Google OAuth token exist for this caller + +The model-facing **extension search** path asserts "connected / ready" from the +lifecycle axis alone. An extension can be `Active` (enabled) while the OAuth +token is missing or expired — activation gates on credentials, but the lifecycle +record is durable and outlives the token (expiry/revocation). That is exactly +the "fresh session, no Google connected" state in the report. + +Concrete defects (all in `ironclaw_reborn_composition/src/extension_lifecycle.rs` +unless noted): + +- **A1 — phase overload / missing cred check on `Active`.** + `search_installation_phase()` (~:1193) checks credentials **only** on the + `Installed` branch (mapping `Installed → Configured` when creds present). An + already-`Active` extension returns `Active` early with **no** credential check. +- **A2 — destructive blanket suppression.** + `suppress_search_credential_onboarding()` (~:1226) unconditionally clears + `credential_requirements` and `onboarding` on every search summary, erasing the + information the "ready?" predicate then inspects. +- **A3 — predicate degenerates to phase-only.** + `extension_search_has_ready_result()` (~:1231) checks `phase ∈ {Configured, + Active}` AND requirements empty AND onboarding none — but A2 always makes the + latter two true, so it collapses to phase-only. It gates the model-visible + message: *"Search found installed extension results that are already configured + or active. Treat those results as ready for this connection request; do not ask + the user for credentials unless a later tool call reports auth_required."* + (~:207). That string is the literal driver of "already connected". +- **B — wire `authenticated` bit fails open.** + `reborn_services/extensions.rs::extension_info()` (:254) derives `authenticated` + from `ExtensionCredentialReadiness`; the `Unknown` arm (:268) falls back to + `lifecycle_authenticated` (phase), so the settings-UI list claims "connected" + whenever the credential service is unwired/unavailable. + +## 3. Why this keeps happening (the reason a minimal patch is wrong) + +This code is on its **third** iteration of the same oscillating bug: + +- **#4996** `[codex] Fix stale extension search onboarding` — added + `search_setup_is_complete(phase)`. +- **#5037** `suppress stale extension search credential prompts` — replaced it with + the blanket `suppress_search_credential_onboarding()`. + +#4996/#5037 fixed the **opposite** symptom — the agent *nagging* for credentials +when already set up. The blanket-suppress overshot and now the agent *claims +connected* when it isn't. The flow oscillates between "nag-when-ready" and +"claim-ready-when-not" because **no single value answers "are the credentials +actually present?"** — the search path infers it from `LifecyclePhase` plus a +destructive mutation. A minimal "check creds on the `Active` branch too" patch is +patch #4 and sets up patch #5. + +### Structural defect behind all three: credential presence is answered twice + +"Is the credential present?" is resolved by **two** trait fronts over the **same** +backend (`RebornProductAuthServices`), with **divergent** result shapes and +fail-modes: + +| Surface | Port | Result | Fail-mode | +|---|---|---|---| +| search / activate | `RuntimeExtensionActivationCredentialGate` → `RuntimeCredentialAccountSelectionService` | `missing_requirements: Vec` | gate `None`/Backend err → whole search errors or `false` | +| extension list | `ExtensionCredentialSetupService` → `ProductAuthExtensionCredentialSetup` | `Option` → `ExtensionCredentialReadiness` | `Unknown` → fail **open** | + +Two answers to one question, guaranteed to drift — and they have (one fails +closed, one fails open). + +## 4. Design (behavior-preserving reframe, not patch #4) + +Make credential readiness an **explicit axis** the search path computes once, and +delete the phase-overload + blanket suppression so the "ready" decision reads +truth. Keep the two ports for now (converging them is a flagged follow-up), but +give the search path the **same 3-state semantics** the list path already uses. + +### 4.1 One model-facing state — collapse the two axes into `availability` + +The model does not need the lifecycle phase or the raw credential-readiness enum. +Its only questions for "connect X" are **"is it installed?"** and **"is the +credential present?"**. Those two internal facts project to exactly one +model-legible field. Exposing `Installed`/`Configured`/`Active` phase jargon *and* +a separate readiness axis is both confusing to the model and the very +two-signals-for-one-question shape §3 blames. + +**New wire enum** `ExtensionAvailability` (in `lifecycle.rs`, next to +`LifecyclePhase`; `#[serde(rename_all = "snake_case")]`, wire-stable per `types.md` +— helper methods for rendering, no `format!("{:?}")`): + +| Model value | Meaning (what the model should do) | Projected from | +|---|---|---| +| `available` | usable — connect without asking the user for anything | installed **and** credentials satisfied (or none required) | +| `needs_auth` | installed, a required credential is missing → run sign-in | installed **and** `MissingRequired` | +| `not_installed` | discover / install it first | no installation record | +| `unknown` | could not determine (transient backend) → don't claim either way | credential backend blip | + +Crucially, **`Active` vs `Installed` vs `Disabled` does not change the model +value** — the projection is `installed? × credential-readiness`, nothing else. The +lifecycle phase stays entirely internal (it drives the install→activate *tool +flow*, not the model's connection judgment; `activate` is idempotent and +re-enables a `Disabled` extension). This is the real collapse the reported bug was +missing. + +Internally the credential fact still comes from the existing search gate +(`RuntimeExtensionActivationCredentialGate.missing_requirements`); the projection: + +| Installation | Gate result | `availability` | +|---|---|---| +| none | — | `not_installed` | +| present | no required credentials | `available` | +| present | `Ok(missing.is_empty())` | `available` | +| present | `Ok(non-empty)` | `needs_auth` | +| present | `Err(Backend)` | `unknown` | + +`missing_requirements()` only ever returns `Ok(vec)` or `Err(Backend)`: +`CredentialStageError::AuthRequired` is folded to `Ok(false)` upstream +(`product_auth_runtime_credentials.rs:164-174`), so there is no `AuthRequired` arm +to build (the existing `map_search_credential_stage_error` `AuthRequired` arm is +already dead code). + +**`ExtensionCredentialReadiness` (the list path's 4-state) is untouched** — it stays +internal to `reborn_services` for the settings DTO's `authenticated`/`needs_setup` +bits (Defect B, §4.5). No cross-crate move; the model surface uses the collapsed +`ExtensionAvailability`, the settings surface keeps its richer internal axis. The +one thing both share is the credential *source* (`RebornProductAuthServices`); +convergence of the two ports remains the deferred follow-up (§7). + +### 4.2 Pure lifecycle phase, internal only + +`search_installation_phase()` returns `phase_for_activation_state()` **only** — drop +the `Installed → Configured` credential smuggling. That synthetic `Configured` +existed solely to carry "creds present" into the single field the predicate read; +with `availability` the phase no longer needs to lie, and it is no longer on the +model surface at all (§4.3). + +### 4.3 Replace `installation_phase` on the model surface with `availability` + +`LifecycleSearchExtensionSummary` (`lifecycle.rs:358`) today carries +`installation_phase: Option`. Verified consumers: only the +same-file `search()` predicate, `extension_lifecycle_command.rs`, the registry +producer in `reborn_services/extensions.rs`, and this file's tests — **no +webui_v2 / gateway / frontend reader**. Replace it: + +```rust +#[serde(default, skip_serializing_if = "Option::is_none")] +pub availability: Option, +``` + +The model reads one first-class token (`availability: "needs_auth"`) instead of a +lifecycle phase it must interpret. Both producers of the summary +(`extension_lifecycle.rs::search_summary` and the registry path in +`reborn_services/extensions.rs`) set it. + +Replace `suppress_search_credential_onboarding` (delete it) with +availability-driven handling in `search_summary`: + +- `available` → clear `credential_requirements` + `onboarding` (satisfied / not + needed; the honest version of #5037's intent). +- `needs_auth` / `unknown` → **keep** `credential_requirements` + `onboarding` so + the model has the actionable how-to-connect detail (provider, OAuth scopes, + setup URL). +- `not_installed` → unchanged (no installation, discovery result). + +### 4.4 Message off `availability`, not payload re-inspection + +`search()` computes `availability` per extension and decides the message directly: + +``` +ready = any result has availability == Available +``` + +`extension_search_has_ready_result` becomes a pure function of the collected +`availability` values (delete the payload-field re-inspection). The "treat as ready +/ do not ask for credentials" message fires only when at least one result is +`available` — which, by construction, cannot happen with a missing required token. + +**Concurrency (round-2 finding B).** Today the sequential per-extension credential +check in `search()` (`extension_lifecycle.rs:194` — `for extension { push(search_ +summary(..).await) }`) only fires for `Installed`-phase extensions (A1 skips +`Active`). Projecting `availability` for **every** result widens a live +O(extensions × requirements) sequential await chain (each `missing_requirements` +hits account selection per requirement). The list path already solved this one file +away: `lifecycle_extension_infos` (`reborn_services/extensions.rs:192-213`) fans out +with `stream::iter(..).buffered(EXTENSION_READINESS_CONCURRENCY)` (const = 8, +ceiling test at `:515`; `futures = "0.3"` is already a composition dep, +`Cargo.toml:111`). Adopt the same pattern for the search loop. Lives in the +extracted module (§4.6). + +**Backend degrade.** A `Backend` blip currently **fails the entire search** +(`map_search_credential_stage_error` → `Transient`). New: it becomes `unknown` +(search still returns; that extension shown not-`available`). Fail-closed for the +"ready" claim, search stays up. + +**Behavior-flip to lock with a test (round-1 finding #7):** the old +`search_credentials_configured` short-circuited `requirements.is_empty() → false` +*before* consulting the gate, so a **credential-less** extension never reached +"ready" via search. Under the projection it becomes `available` for an installed +credential-less extension. That is the intended, more-correct + +**Concurrency (round-2 finding B).** Today the sequential per-extension credential +check in `search()` (`extension_lifecycle.rs:194` — `for extension { push(search_ +summary(..).await) }`) only fires for `Installed`-phase extensions (A1 skips +`Active`). Closing A1 means computing readiness for **every** result, widening a +live O(extensions × requirements) sequential await chain (each +`missing_requirements` hits account selection per requirement). The list path +already solved this one file away: `lifecycle_extension_infos` +(`reborn_services/extensions.rs:192-213`) fans out with +`stream::iter(..).buffered(EXTENSION_READINESS_CONCURRENCY)` (const = 8, with a +ceiling test at `:515`). Adopt the **same** canonical pattern for the search loop — +don't leave a widened sequential fan-out. This lives naturally in the extracted +module (§4.6). + +**Behavior-flip to lock with a test (round-1 finding #7):** the old +`search_credentials_configured` short-circuited `requirements.is_empty() → false` +*before* consulting the gate, so a **credential-less** extension never reached +"ready" via search. Under the projection it becomes `available` for an installed +credential-less extension. That is the intended, more-correct behavior (an +extension needing no credentials is legitimately "don't ask for credentials"), but +it is a silent change — pin it explicitly in §6. + +### 4.5 Defect B — fail closed, without creating a new dead-end state + +`reborn_services/extensions.rs::extension_info()` (:254). Flipping only the +`authenticated` `Unknown` arm to `false` is **incomplete** and relocates the +contradiction (round-1 finding #3): `needs_setup` (:282) is +`readiness == MissingRequired || phase ∈ {Installed, Configured, Failed}`. For +`phase ∈ {Active, Activating}` with `readiness == Unknown`, you'd get +`authenticated: false, needs_setup: false`, and `for_installed_with_credential_status` +(`extension_onboarding.rs:33-54`) has no `Unknown` branch so it falls through to +generic onboarding — the UI/model would see "not connected" with **no** way to fix +it. Same class of contradiction as #5416, just on the settings surface. + +Fix all three coherently for `Unknown`: + +- `authenticated` → `false` (fail closed). +- `needs_setup` → also `true` when `readiness == Unknown` (so the state is + actionable), or introduce a distinct "needs reverify" signal — prefer reusing + `needs_setup` unless a separate signal earns its keep. +- `extension_onboarding::for_installed_with_credential_status` → add an `Unknown` + branch that yields a reconnect/reverify affordance rather than generic install + onboarding. + +Self-corrects on the next successful poll (the retryable path already exists). + +### 4.6 Decomposition (file health) + +`extension_lifecycle.rs` is 4,326 lines total, but `#[cfg(test)] mod tests` starts +at ~:1348 — **production code is ~1,300 lines**, already under the 1,500 "aim +smaller" bar (round-1 finding #6). So the file-size framing is weaker than it +looks; the extraction is still worth doing on its own merits (a cohesive +~100–150-line concern), just not justified by "4× the smell line". + +Extract the **search-readiness mapping + `ready` predicate** into a focused module +(neutral name, e.g. `extension_credential_readiness.rs`, since it's the shared +credential axis, not search-only). **Do not move `phase_for_activation_state`** — +it's shared by `list_installed` (:241) and `installed_summaries` (:312); moving it +into a search-named module makes the list path import from "search". Keep it in the +parent file. + +The real long-term decomposition is that ~15 operations +(install/activate/remove/search/list/restore/rollback/…) share one impl block. The +tracking issue should scope **"split by operation"**, not a vague "further +decomposition". + +## 5. Net effect + +- **Deletes:** `suppress_search_credential_onboarding` (destructive mutation), + the `Installed → Configured` phase overload, `search_credentials_configured`'s + bool-collapse, the payload re-inspection in the predicate, and the + search-fails-on-Backend-error path. +- **Adds:** one collapsed model-facing `ExtensionAvailability` wire enum + (`available`/`needs_auth`/`not_installed`/`unknown`), replacing `installation_phase` + on the search DTO; a focused module for the projection + predicate. +- **Leaves internal:** `ExtensionCredentialReadiness` (list path only) and + `LifecyclePhase` (drives the tool flow, not the model surface) — no cross-crate + move, no phase jargon exposed to the model. +- Fewer moving parts than the "A+B minimal" patch; the model sees **one explicit + state** instead of two raw axes; and the oscillation is structurally impossible + (the projection reads credential truth). + +## 6. Tests (test-first, through the caller) + +Per `.claude/rules/testing.md` "test through the caller": + +1. **Search — Active + no token (the bug).** Drive the `builtin.extension_search` + handler (`ExtensionLifecycleToolHandler::dispatch`) with gmail `Enabled` and no + OAuth account. Assert: response `message` does **not** contain the "treat as + ready / do not ask" string; the gmail summary retains `credential_requirements` + + `onboarding`; `availability == NeedsAuth`. +2. **Search — Active + token present (no regression).** Same handler, credential + account configured. Assert: message present; requirements + onboarding cleared; + `availability == Available`. +3. **Search — Installed (inactive) + token present.** Assert: `available`, message + present, requirements cleared. (Confirms lifecycle Active-vs-inactive does not + change the model state.) +4. **Search — backend blip → Unknown.** Credential service returns a retryable + error. Assert: search **succeeds** (no error), `availability == Unknown`, + requirements retained, no "ready" message. +5. **Search — credential-less extension (behavior-flip lock, finding #7).** A + bundled extension with **no** credential requirements, `Installed`. Assert + `availability == Available` and message present — pins the intended change from + the old `requirements.is_empty() → false`. +6. **Defect B — list fails closed AND stays actionable.** Drive `extension_info` + via the facade (`RebornServices`) with readiness `Unknown` on an `Active` + extension; assert `authenticated == false`, `needs_setup == true`, and + onboarding yields a reconnect/reverify affordance (not generic install + onboarding). + +Extend existing search tests (`extension_lifecycle.rs` tests ~:1938) rather than +standing up parallel suites; add the new module's unit tests for the projection +function (each `(installation, gate result)` → `availability` row). + +**Must-change existing assertion (round-2 finding).** `extension_lifecycle.rs:2359` +currently asserts `github.installation_phase == Some(Configured)` for an +Installed+credentialed extension found via search — the exact `Installed→Configured` +smuggling §4.2 deletes, and `installation_phase` itself is being removed from the +model DTO. Under the new design that becomes `availability == Some(Available)` +(requirements still empty, onboarding still none). Update in place. + +## 7. Non-goals / follow-ups + +- **Defect C — model capability surface `NeedsAuth`.** `VisibleCapabilityAccess` + (`host_runtime/surface.rs`) has only `Available`/`RequiresApproval`; the catalog + has no credential-presence handle. Adding a credential-aware access state is a + cross-crate architectural change (the engine-v2 analogue is even marked + "intentional"). Deferred — only reachable once gmail is activated, and this plan + fixes the search/list messaging that drives the reported contradiction. Track + separately. +- **Converge the two credential ports.** `RuntimeCredentialAccountSelectionService` + + `ExtensionCredentialSetupService` should become one credential-readiness port + so there is exactly one answer to "is the credential present?". Strategic; file a + tracking issue. This plan aligns their *semantics* (3-state, fail-closed) without + merging the traits. + +## 8. Touch points + +- `crates/ironclaw_product_workflow/src/lifecycle.rs` — new `ExtensionAvailability` + wire enum next to `LifecyclePhase` (+ serde/snake_case); replace + `installation_phase` with `availability: Option` on + `LifecycleSearchExtensionSummary`. +- `crates/ironclaw_product_workflow/src/lib.rs` — add `ExtensionAvailability` to the + `pub use lifecycle::{...}` block (:111). +- `crates/ironclaw_reborn_composition/src/extension_lifecycle.rs` — search path + (delete overload/suppression; project `availability`; move projection+predicate + cluster out; keep `phase_for_activation_state` here). +- `crates/ironclaw_reborn_composition/src/extension_availability.rs` — **new** + (`(installation, gate result) → ExtensionAvailability` projection + `ready` + predicate + `buffered(8)` fan-out). +- `crates/ironclaw_reborn_composition/src/extension_lifecycle_command.rs` — registry + summary producer sets `availability` (`not_installed` for registry-only rows). +- `crates/ironclaw_reborn_composition/src/extension_lifecycle_capabilities.rs` — + caller-level test entry (`ExtensionLifecycleToolHandler`). +- `crates/ironclaw_product_workflow/src/reborn_services/extensions.rs` — Defect B + (`Unknown` → `authenticated=false` + `needs_setup=true`); registry search producer + sets `availability`. +- `crates/ironclaw_product_workflow/src/reborn_services/extension_onboarding.rs` — + `Unknown` reconnect/reverify branch in `for_installed_with_credential_status`. + +> **Note:** this §8 is the *interim* fix. The user's north-star directive (one +> unified tool-lifecycle pipeline) supersedes the layering here — see the north-star +> plan (separate doc) and §7. The interim fix is designed to be a strict subset of +> the north star (the `ExtensionAvailability` projection becomes the pipeline's +> single connection-state output), not throwaway. +``` diff --git a/docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md b/docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md new file mode 100644 index 00000000000..614e61b483d --- /dev/null +++ b/docs/plans/2026-07-01-reborn-tool-lifecycle-unified-pipeline-northstar.md @@ -0,0 +1,301 @@ +# North star: one unified tool-lifecycle pipeline (Reborn) + +**Status:** north-star design + phased plan (pre-implementation) +**Motivating issue:** nearai/ironclaw#5416 (false "Gmail already connected") — a *symptom* of the sprawl below. +**Interim fix:** `docs/plans/2026-07-01-reborn-google-connection-state-5416.md` — designed as a strict subset (Phase 0). +**Scope:** Reborn stack only. Retiring the legacy v1/v2 extension stack is explicitly out of scope. + +## 1. The vision + +There is **one** question the whole tool lifecycle keeps re-answering in different +places: *"what is the connection state of this capability/extension for this +caller — can the model use it right now, and if not, what unblocks it?"* Today that +question is answered by **8 disjoint state representations** and **3 separate +capability-listing passes**, reconciled by hand at each surface. #5416 is one of the +contradictions that fall out. + +North star: **one canonical connection-state, computed once, consumed by every +surface** — search, settings list, the model tool surface, and the pre-dispatch +check. Exactly the shape the authorizer already ships (see §4). + +## 2. The sprawl (why this is worth doing) + +Eight state types for one concept, none sharing a variant set (investigator map): + +| Type | Variants | Crate | Consumed by | +|---|---|---|---| +| `LifecyclePhase` | 12 | `product_workflow` `lifecycle.rs:135` | search/install/activate, list, CLI | +| `ExtensionActivationState` | 3 | `extensions` `installations.rs:172` | durable store; 1-way → `LifecyclePhase` | +| `ExtensionCredentialReadiness` | 4 | `product_workflow` `extension_credentials.rs:16` (`pub(super)`) | settings list only | +| `VisibleCapabilityAccess` | 2 | `host_runtime` `surface.rs:94` | model tool surface | +| `BlockedReason::Auth` | 1/5 | `turns` `status.rs:140` | turn status / gate resume | +| `FirstPartyCapabilityError::AuthRequired` | 1/2 | `host_runtime` `first_party.rs:131` | dispatch-time failure | +| `Decision` (Allow/RequireApproval/Deny) | 3 | `authorization` | **both** surface + dispatch (already unified) | +| `CapabilityStatus` (legacy) | 8 | `src/bridge/…` (non-Reborn) | v1/v2 only — out of scope | + +Plus three hand-maintained stringifiers of `LifecyclePhase` (`phase_status`, +`phase_label`, the settings `authenticated`/`needs_setup` cross-product) and **three +capability-listing passes**: `CapabilityCatalog::visible_capabilities` +(`host_runtime/surface.rs:147`, → `VisibleCapability`), composition's +`active_model_visible_capabilities` (`extension_lifecycle.rs:254`, → +`ActiveExtensionCapability`), and the settings `lifecycle_extension_infos` +(`reborn_services/extensions.rs:192`, → `RebornExtensionInfo`). + +Every new surface re-reconciles the axes by hand → the oscillation class (#4996 → +#5037 → #5416, three patches on one predicate). + +## 3. Feasibility verdict + +**Achievable, incrementally, no hard blocker.** Three independent investigations: + +- **Credential convergence is ~80% already done.** Both credential "ports" + (`RuntimeExtensionActivationCredentialGate` and + `ProductAuthExtensionCredentialSetup::credential_status`) already terminate at + **one** method — `RuntimeCredentialAccountSelectionService::select_unique_configured_runtime_account` + over one `CredentialAccountRecordSource`. The only duplication is two result-mapping + match arms differing solely by `AuthSurface::Api` vs `Web` + (`product_auth_runtime_credentials.rs:152-175` vs `webui_extension_credentials.rs:39-64`). +- **The surface can be made credential-aware through an existing seam.** + `CapabilityCatalog` has no credential handle today, but `descriptor.runtime_credentials` + is already in scope in the loop (`surface.rs:181`), `DefaultHostRuntime` already + holds `credential_preflight_store: Option>` on the same struct + that builds the catalog (`production.rs:1032`), and `secret_present()` + (`obligations.rs:2061`) is *documented* as the single source of truth for + presence — already shared by two dispatch-time call sites. Product-auth (Gmail + OAuth) presence comes in via the existing dependency-inverted + `RuntimeCredentialAccountResolver` trait (`obligations.rs:60`; host_runtime defines + the port, composition implements over `ironclaw_auth`). +- **The authorizer already proves the whole pattern.** `Decision` + (`authorization`) is computed once by `authorize_dispatch_with_trust` and consumed + by **both** `visible_capabilities` (surface) and actual dispatch. Credential-presence + should follow the identical shape. + +### Hard constraints (not blockers, but they shape the design) + +- **Crate boundaries fix where the pipeline lives.** `product_workflow` cannot see + `host_runtime`'s types and vice-versa; only `ironclaw_reborn_composition` depends + on all of `product_workflow`, `host_runtime`, `extensions`, `auth`, + `authorization`. **The canonical state and its assembly can only live in + composition** (or a new crate above both) — never pushed down into + `product_workflow` or `host_runtime`. Cross-layer needs use **dependency-inverted + ports** (host_runtime defines a trait, composition implements it) — the pattern + `RuntimeCredentialAccountResolver` already uses. +- **Two credential lifetimes must stay separate.** Durable "is an account + configured" (presence, `SecretHandle`) ≠ ephemeral dispatch-time secret material + (`RuntimeSecretInjectionStore`, TTL-bound, keyed by in-flight capability). The + unified readiness port answers only the first. Never fold in the second. +- **ProductAuthAccount ≠ generic secret.** `capability_credential_requirements` + deliberately excludes product-auth-account requirements from the `secret_present` + pre-flight (`production.rs:1993`) to avoid false positives — those are resolved by + `resolve_access_secret` at dispatch. The surface fold must call the **resolver** + for product-auth creds, not `secret_present`. +- **The model tool schema has no structured state field.** Even today's + `Available`/`RequiresApproval` is dropped before the LLM schema + (`surface_snapshot.rs:14` has no `access` field). Surfacing connection-state to the + model means either prose in `description` (cheap, stringly) or a new structured + field on `ProviderToolDefinition`/`ToolDefinition` threaded through every provider + adapter (clean, wider). +- **Identifier sprawl.** `ExtensionId` / `ExtensionInstallationId` (1:1 today, split + for an unbuilt multi-install model) / `LifecyclePackageRef` (a third string shape, + lossy round-trip). Unification should *not* build the 1:many model — just stop + re-deriving. + +## 4. The canonical model + +**`CapabilityConnectionState`** — the single source of truth. The **enum + its pure +`project(...)` function** live in a thin, near-zero-dependency leaf crate (new +`ironclaw_capability_state`, or `ironclaw_host_api` if that is the established +shared-vocabulary home) — **not** in composition. Rationale (thermo M4): the enum +and projection depend only on the *shapes* of `Decision`, `ExtensionActivationState`, +and `CredentialReadiness`, none of composition's heavy substrate; and composition is +already the largest crate in the tree (155k lines; `runtime.rs` 10.4k, five files +>2.5k — several already past the `architecture.md` >3k decomposition bar). Dropping a +new cross-cutting concept into it compounds the sprawl this effort fights. +**Composition owns only the *assembly/wiring*** (fetching the three inputs and calling +`project`), never the type. + +Projected from three inputs, mirroring how `Decision` is projected from +trust+grants+policy: + +``` +project(authz: Decision, lifecycle: ExtensionActivationState, cred: CredentialReadiness) + -> CapabilityConnectionState +``` + +Model-facing collapse (what the LLM / UI reads — the #5416 interim `ExtensionAvailability` +is exactly this, at the search surface): + +| State | Meaning | Projection | +|---|---|---| +| `available` | usable now / connect with no user action | authorized ∧ installed ∧ credential present (or none required) | +| `needs_auth` | a required credential is missing → sign-in | authorized ∧ installed ∧ credential missing | +| `needs_approval` | usable but gated on approval | `Decision::RequireApproval` | +| `not_installed` | discover / install first | no installation | +| `unknown` | indeterminate (transient) → claim nothing | credential backend blip | + +Lifecycle phase (Active/Installed/Disabled) and the raw readiness enum stay +**internal** — they feed the projection, they are not surfaced. One state out, not +three axes. + +## 5. Architecture + +The four surfaces are **not symmetric consumers** — they sit in three different +crates, so each reaches the canonical state differently (thermo B1). Type lives in a +leaf crate (§4); assembly in composition; the two surfaces composition can't reach +directly (host_runtime, product_workflow) each get a **dependency-inverted port**. + +``` + leaf crate (ironclaw_capability_state): enum CapabilityConnectionState + fn project(..) + + ironclaw_reborn_composition (assembly only) + ┌──────────────────────────────────────────────────────────────┐ + │ project(Decision, ExtensionActivationState, CredentialReadiness) │ + │ ▲ ▲ ▲ │ + │ CredentialReadiness durable store authorizer Decision │ + │ (§P1: one fn over the single selector; already unified) │ + └───┬───────────────────────────┬──────────────────────┬─────────┘ + in-crate │ port ▼ (host_runtime) port ▼ (product_workflow) + ┌──────┴───────┐ RuntimeCredentialAccountResolver CapabilityConnectionStateProvider + search tool pre-dispatch + credential-presence handle on (NEW, §P4a — product_workflow + (extension_ check CapabilityCatalog (§P2) defines trait, composition impls, + lifecycle) (composition) → model tool surface injected into RebornServices) + → settings list (extension_info) +``` + +- **Type home:** thin leaf crate (§4), not composition. +- **Assembly home:** `ironclaw_reborn_composition` — the only crate that can *see* all + three inputs (verified: `host_runtime` Cargo.toml has no `product_workflow`/`auth` + dep; `product_workflow`'s composition dep is `[dev-dependencies]` only). This + mutual-blindness premise is the load-bearing constraint and it holds. +- **Three dependency-inverted ports** (host_runtime never depends on + `auth`/`product_workflow`; product_workflow never depends on `host_runtime`/ + composition): + 1. `RuntimeCredentialAccountResolver` (exists) — product-auth presence into host_runtime. + 2. credential-presence handle on `CapabilityCatalog` (NEW, §P2) — surface fold. + 3. `CapabilityConnectionStateProvider` (NEW, §P4a) — **the missing port** the first + draft omitted: product_workflow defines the trait, composition supplies the impl, + injected into `RebornServices`, so the settings list consumes the *same* canonical + projection instead of its hand cross-product. Without this, "consumed by every + surface" is false and the settings list keeps an independent `authenticated`/ + `needs_setup` forever. +- **The one credential answer:** a single composition-owned `credential_readiness()` + wrapping the one selector, consumed by search + activate + list + surface fold. + +## 6. Phased delivery (each phase independently shippable) + +**Phase 0 — interim #5416 fix (already planned, strict subset).** Collapse the +*search* surface to the model-facing `ExtensionAvailability`; fail the settings +`authenticated` bit closed on `Unknown`. Ships the canonical model's *output* shape at +one surface and fixes the reported bug. Doc: the companion 5416 plan. + +**Phase 1 — one credential-readiness function (NOT behavior-preserving; needs its own +test).** Extract `credential_readiness(selector, scope, surface, requirement) -> +CredentialReadiness` in composition; route both existing shims through it. **Correction +(thermo M1):** the two paths do *not* "differ only by `AuthSurface`". The +search/activate path flags an account that is `Configured` but missing its +`access_secret` as `Backend`/anomaly (`product_auth_runtime_credentials.rs:164-174`); +the settings path treats **any** `Ok(Some(_))` as `Configured` with no `access_secret` +check (`webui_extension_credentials.rs:35-64` → `extension_credentials.rs:126`). That's +the same fail-open/fail-closed class §3 of the 5416 doc diagnoses. Unifying therefore +**changes settings-list behavior**: an orphaned-secret account stops reading as +"connected". Scope Phase 1 as "extract one fn **and** reconcile the access-secret +divergence behind a `MissingSecretHandle` (or `Unknown`) distinction, with a dedicated +regression test on the settings path" — do not estimate it as a mechanical extract. + +**Phase 2 — credential-aware capability surface (closes #5416 Defect C).** Thread a +credential-presence handle into `CapabilityCatalog` via `with_credential_presence(...)` +(mirrors `with_filesystem`); inside `visible_capabilities`, after the authorizer +`Decision`, consult `capability_credential_requirements(descriptor)` + +`secret_present` (generic) / `RuntimeCredentialAccountResolver` (product-auth) and +downgrade to a new `VisibleCapabilityAccess::NeedsAuth`. Verified feasible: +`descriptor.runtime_credentials` is already in the loop, `DefaultHostRuntime` already +holds `credential_preflight_store` on the same struct that builds the catalog +(`production.rs:1032`). `capability_credential_requirements` moves from `pub(crate)` in +`production.rs` to a shared home. **Acceptance criteria (not follow-ups):** +- **Caching (thermo M2).** The surface is rebuilt on *every LLM step* + (`HostRuntimeLoopCapabilityPort::visible_capabilities` → + `DefaultHostRuntime::visible_capabilities`, `production.rs:1032`), not once per turn. + A naive fold adds N credential lookups (DB/keychain/account-resolver) per build. + Ship a short-TTL presence cache keyed by `(scope, SecretHandle)` shared across + surface builds within a run, or explicitly justify the added per-step latency. +- **Fingerprint stability (thermo M3).** `access` already feeds `surface_version` + (`surface.rs:304,393`). `NeedsAuth` is credential-derived → flips on OAuth + expiry/revocation — a **time-sensitive** signal, the same class that caused the + #4789 stale-surface incident (`expires_at` kept out of the hash). Decide explicitly + whether `NeedsAuth` participates in the version hash; default to **excluding** the + credential-derived component from the fingerprint and re-deriving it per render, as + #4789's follow-up established. + +> **Phase 2 landed (2026-07-02, thermo-approved).** Implementation notes that bind +> Phase 3: (a) product-auth presence uses the new side-effect-free +> `RuntimeCredentialAccountResolver::account_configured` (select-only) — the +> original assumption that `resolve_access_secret` was a pure presence check was +> wrong; it refreshes OAuth tokens and must never run on surface render. (b) The +> presence cache key is `(owner scope, provider, requester_extension, sorted +> provider_scopes)` — scopes are load-bearing (gmail readonly/send/modify differ). +> (c) Confirmed drop point for Phase 3: `ironclaw_loop_support/src/capability_port.rs` +> maps `VisibleCapability` → `CapabilityDescriptorView` +> (`ironclaw_turns/src/run_profile/host.rs:1389`), which carries no `access` +> field — the single seam where the prose-fold (or a structured field) attaches. + +**Phase 3 — carry state to the model schema.** Stop dropping capability state at +`surface_snapshot.rs:14` (even today's `Available`/`RequiresApproval` never reaches the +LLM). **Default: fold `CapabilityConnectionState` into the tool `description`** — one +seam, no schema change. The structured-field alternative +(`ProviderToolDefinition`/`ToolDefinition` + a per-provider rendering decision) touches +~8 backend adapters (`codex_chatgpt`, `gemini_oauth`, `bedrock`, `rig_adapter`, +`nearai_chat`, `openai_codex_provider`, `reasoning`, `placeholder_stripping`) — real +cost, opt-in only if the prose channel proves insufficient. + +**Phase 4 — collapse the redundant representations (strategic).** Split by reachability: +- **4a — the missing product_workflow port (prerequisite).** Add + `CapabilityConnectionStateProvider` (thermo B1): product_workflow trait, composition + impl, injected into `RebornServices`. *Only after this* can the settings + `authenticated`/`needs_setup` hand cross-product be replaced by the canonical + projection. Without 4a, the settings surface is unreachable from the canonical state. +- **4b — deletions, opportunistic.** Retire the two extra stringifiers; fold + `active_model_visible_capabilities` into the one catalog pass; unify the identifier + round-trips (stop re-deriving `ExtensionInstallationId`/`LifecyclePackageRef` from + `ExtensionId` — do **not** build the unbuilt 1:many model). Tracking issue; no rush. + +**Where "good enough" is.** #5416 is fully fixed by **Phase 0 + Phase 2** (search and +tool surface both credential-honest). Phase 1 is the correctness/dedup investment that +makes 0/2 rest on one credential answer. Phase 3 improves model legibility. Phase 4 is +pure debt paydown — valuable, but not required for correctness. Stop and reassess after +Phase 2. + +## 7. Non-goals + +- Retiring the legacy v1/v2 stack (`src/extensions/`, `CapabilityStatus`, + `InstalledExtension`) — disjoint, separately owned. +- Building the multi-install-per-extension model implied by the + `ExtensionId`/`ExtensionInstallationId` split. +- Folding dispatch-time secret staging (`RuntimeSecretInjectionStore`) into the + readiness port — different lifetime, must stay separate. + +## 8. Interim-vs-north-star contract + +The Phase 0 interim fix is a **subset, not throwaway**: its `ExtensionAvailability` +(`available`/`needs_auth`/`not_installed`/`unknown`) maps cleanly onto +`CapabilityConnectionState` (same states minus `needs_approval`, which search has no +axis for). Later phases *extend* it, they don't discard it: Phase 1 swaps its inline +gate mapping for the shared credential-readiness fn, Phase 2 extends the same state to +the tool surface, Phase 3 carries it to the schema. + +**One honest caveat (thermo m4):** Phase 1 introduces the `MissingSecretHandle`/`Unknown` +reconciliation (§P1), which changes the *inputs* to Phase 0's projection — so Phase 0's +projection fn and its test #5 get a **one-line follow-up edit** when Phase 1 lands. Not +"thrown away", but not zero-touch either. + +## 9. Delivery order & decision points + +1. **Phase 0** — ship now (companion 5416 doc); independently valuable. +2. **Decide the crate-home question (§4/M4) before Phase 2** lands new state into + composition — cheap now, expensive once P2/P4 code exists there. +3. **Phase 1** — rescoped to reconcile the access-secret divergence (M1) with its own + settings-path regression test. Not "small". +4. **Phase 2** — with the caching strategy (M2) and fingerprint-stability decision (M3) + as *acceptance criteria*, not deferred follow-ups. `#5416` fully closed here. +5. **Phase 4a** (the missing product_workflow port, B1) — prerequisite before promising + any "fold the settings cross-product". +6. **Phase 3 + Phase 4b** — opportunistically. Reassess necessity after Phase 2. diff --git a/tests/reborn_group_extensions/main.rs b/tests/reborn_group_extensions/main.rs index 5fd0c8233cc..f0dea7de74d 100644 --- a/tests/reborn_group_extensions/main.rs +++ b/tests/reborn_group_extensions/main.rs @@ -24,6 +24,7 @@ mod support; mod scenario_activate_then_active_cross_thread; mod scenario_install_then_visible_cross_thread; mod scenario_remove_then_absent_cross_thread; +mod scenario_search_ready_message_requires_credential; use reborn_support::group::{RebornIntegrationGroup, ScenarioReport}; @@ -53,14 +54,29 @@ async fn extensions_group_e2e() { ); // Scenario 3: install in thread A → activate in thread B → search in thread C - // confirms the extension reports `installation_phase:active` over the shared - // store. Closes the `extension_activate` int-tier gap. Independent of - // Scenarios 1 & 2: it uses "web-access" (the only credential-free bundled - // extension), untouched by "github"/"notion", so it is self-contained. + // confirms the extension reports `availability:available` over the shared + // store (plus a direct durable-store read confirming activation specifically, + // see the scenario's SHAPE NOTE). Closes the `extension_activate` int-tier + // gap. Independent of Scenarios 1 & 2: it uses "web-access" (the only + // credential-free bundled extension), untouched by "github"/"notion", so it + // is self-contained. report.record( "activate_then_active_cross_thread", scenario_activate_then_active_cross_thread::run(&g).await, ); + // Scenario 4 (regression, bug #5416): extensions seeded `Enabled` with NO + // credential account must NOT be reported by `builtin.extension_search` as + // "already configured or active" / safe to skip asking for credentials — + // across credential types: gmail (Google OAuth), github (GitHub OAuth), + // notion (Notion OAuth / MCP). A credential-free extension (web-access) + // must STILL be reported ready (control against over-suppression). Runs + // after Scenarios 1-3 and reuses their shared-store state (github installed, + // notion removed, web-access Enabled). + report.record( + "search_ready_message_requires_credential", + scenario_search_ready_message_requires_credential::run(&g).await, + ); + report.assert_all_passed(); } diff --git a/tests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rs b/tests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rs index f06300fe2aa..5ec03ae4883 100644 --- a/tests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rs +++ b/tests/reborn_group_extensions/scenario_activate_then_active_cross_thread.rs @@ -15,20 +15,32 @@ //! observes a real lifecycle transition over the shared store rather than an //! already-installed/activated no-op. //! -//! Key behaviours asserted (from `extension_lifecycle.rs::commit_activation` and -//! `search_installation_phase`): a successful activate yields `"activated":true` -//! plus a `visible_capability_ids` array of the now-published capability ids; and -//! `extension_search` renders an ACTIVE extension as `"installation_phase":"active"` -//! versus `"installed"` for an installed-but-not-yet-activated one. +//! Key behaviours asserted (from `extension_lifecycle.rs::commit_activation`): a +//! successful activate yields `"activated":true` plus a `visible_capability_ids` +//! array of the now-published capability ids; and `extension_search` renders a +//! ready, credential-free extension as `"availability":"available"`. //! //! Because all three conversations use different conversation IDs but the same -//! `Arc`, asserting that thread C sees -//! `installation_phase:active` proves cross-thread ACTIVATION persistence: an -//! activate in thread B is durably visible to thread C, and the extension's -//! capability surface (e.g. `web-access.search`) has come online. +//! `Arc`, asserting that thread C's search sees +//! `availability:available` proves cross-thread install+activate persistence: +//! an activate in thread B is durably visible to thread C. +//! +//! SHAPE NOTE (post-#5416 Phase 0): `availability` is the collapsed +//! `installed? × credential-readiness` projection — for a credential-free +//! extension like "web-access", `Installed` and `Active` BOTH project to +//! `available` (by design: activation vs mere installation is a lifecycle +//! detail the model does not need, see the 5416 plan §4.1). So +//! `extension_search`'s `availability` field can no longer discriminate +//! "installed but not yet activated" from "active" the way the old +//! `installation_phase` field did. To still prove that THREAD B's +//! **activation** specifically (not just thread A's install) is durably +//! visible to thread C, this scenario also reads the shared durable +//! installation store directly (the same E-EXTSTORE seam the credential-gap +//! regression test uses) and asserts `ExtensionActivationState::Enabled`. use super::reborn_support::group::{HarnessResult, RebornIntegrationGroup}; use super::reborn_support::reply::RebornScriptedReply; +use ironclaw_extensions::{ExtensionActivationState, ExtensionInstallationId}; use serde_json::json; pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { @@ -92,8 +104,8 @@ pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { // ── Thread C: viewer (DIFFERENT conversation, SAME shared store) ───────── // A third distinct conversation_id over the Arc-cloned store. Searching for - // "web-access" must now report it as ACTIVE — observing Thread B's activation - // across threads. + // "web-access" must now report it ready — observing Thread A's install and + // Thread B's activation across threads. let viewer = g .thread("ext-activate-phase-viewer") .script([ @@ -111,33 +123,17 @@ pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { viewer .assert_tool_invoked("builtin.extension_search") .await?; - // Cross-thread activation persistence: the search result carries - // `installation_phase:"active"` only because Thread B's activation enabled the - // installation in the shared store. Assert the VALUE so a still-`installed` - // phase cannot satisfy this. + // Cross-thread install+activate persistence: the search result carries + // `availability:"available"` because both Thread A's install and Thread B's + // activation reached the shared store (web-access needs no credentials, so + // `available` fires as soon as it's installed — see the module SHAPE NOTE). viewer - .assert_tool_result_contains(r#""installation_phase":"active""#) + .assert_tool_result_contains(r#""availability":"available""#) .await?; - // Discriminating guard: an extension that was installed but NOT activated - // renders `installation_phase:"installed"`. Its ABSENCE here proves the phase - // genuinely advanced past install — a no-op activate (leaving the extension - // inactive) would surface `"installed"` and trip this guard. - if viewer - .assert_tool_result_contains(r#""installation_phase":"installed""#) - .await - .is_ok() - { - return Err( - "web-access still shows installation_phase:installed after a cross-thread activate; \ - builtin.extension_activate did not advance the lifecycle through the shared store" - .into(), - ); - } - // Non-vacuity guard: "web-access" must still appear in the catalog search // result, proving the search actually ran and returned a catalog entry. The - // presence of `installation_phase:active` is therefore meaningful — not a + // presence of `availability:available` is therefore meaningful — not a // symptom of an empty or errored result. if viewer .assert_tool_result_contains("\"web-access\"") @@ -146,10 +142,34 @@ pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { { return Err( "non-vacuity guard failed: web-access catalog entry must appear in search results \ - after activation; the active-phase assertion would otherwise be vacuous" + after activation; the availability assertion would otherwise be vacuous" .into(), ); } + // `availability` cannot discriminate "installed" from "active" for a + // credential-free extension (see module SHAPE NOTE), so read the shared + // durable installation store directly to prove Thread B's ACTIVATION + // specifically — not just Thread A's install — is durably visible: the + // store must report `ExtensionActivationState::Enabled`, not `Installed`. + let installation_id = ExtensionInstallationId::new("web-access")?; + let store = g + .capability_harness() + .ok_or("extension_lifecycle group missing HostRuntime capability harness")? + .extension_installation_store_for_test() + .ok_or("harness missing extension installation store (E-EXTSTORE seam)")?; + let installation = store + .get_installation(&installation_id) + .await? + .ok_or("web-access installation missing from the shared store after activation")?; + if installation.activation_state() != ExtensionActivationState::Enabled { + return Err(format!( + "web-access installation state is {:?}, not Enabled, after a cross-thread activate; \ + builtin.extension_activate did not durably advance the lifecycle through the shared store", + installation.activation_state() + ) + .into()); + } + Ok(()) } diff --git a/tests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rs b/tests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rs index a85b6ba33e9..1087ffd2613 100644 --- a/tests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rs +++ b/tests/reborn_group_extensions/scenario_install_then_visible_cross_thread.rs @@ -3,10 +3,12 @@ //! //! Thread A calls `builtin.extension_install` for "github". Thread B calls //! `builtin.extension_search` for "github" and asserts the result carries -//! `installation_phase: "installed"` — a field that only appears in search -//! results for an already-installed extension. Because the two threads use -//! different conversation IDs but the same `Arc`, -//! this proves cross-thread extension persistence. +//! `availability: "needs_auth"` — GitHub requires a credential and none is +//! configured here, so an installed-but-uncredentialed extension projects +//! `needs_auth` (not `not_installed`, which is what an unknown/never-installed +//! extension would show). Because the two threads use different conversation +//! IDs but the same `Arc`, this proves +//! cross-thread extension persistence. use super::reborn_support::group::{HarnessResult, RebornIntegrationGroup}; use super::reborn_support::reply::RebornScriptedReply; @@ -54,13 +56,13 @@ pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { viewer .assert_tool_invoked("builtin.extension_search") .await?; - // The search result carries `installation_phase: "installed"` for the github - // package only when it is already installed (before installation the field is - // absent entirely). Assert the VALUE, not just the key — a `pending`/`failed` - // phase must not satisfy this — so the check proves thread B observes thread - // A's *successful* installation over the shared store. + // The search result carries `availability: "needs_auth"` for the github + // package only once it is installed (before installation the field reads + // `not_installed`). Assert the VALUE, not just the key, so the check proves + // thread B observes thread A's *successful* installation over the shared + // store. viewer - .assert_tool_result_contains(r#""installation_phase":"installed""#) + .assert_tool_result_contains(r#""availability":"needs_auth""#) .await?; // Committed negative guard (non-vacuity): a marker for a never-installed, diff --git a/tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs b/tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs index cc9bad08267..9150ac852bc 100644 --- a/tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs +++ b/tests/reborn_group_extensions/scenario_remove_then_absent_cross_thread.rs @@ -9,11 +9,13 @@ //! Thread A-install calls `builtin.extension_install` for "notion". Thread //! A-remove (a distinct conversation) calls `builtin.extension_remove` for the //! same package. Thread B calls `builtin.extension_search` for "notion" and -//! asserts the result does NOT carry `installation_phase: "installed"` — the -//! field is absent in search results for extensions that are not installed. -//! Because all three conversations use different conversation IDs but the same -//! `Arc`, this proves cross-thread extension -//! removal persistence: a remove in thread A is durably visible to thread B. +//! asserts the result carries `availability: "not_installed"` (not +//! `needs_auth`/`available`) — `not_installed` is the collapsed projection's +//! explicit value for "no installation record", replacing the old +//! field-presence-as-signal shape. Because all three conversations use +//! different conversation IDs but the same `Arc`, +//! this proves cross-thread extension removal persistence: a remove in thread A +//! is durably visible to thread B. //! //! The `builtin.extension_remove` arg shape is `{"extension_id": ""}`, //! identical to `builtin.extension_install` and `builtin.extension_activate` @@ -23,13 +25,13 @@ //! (confirmed from the unit test in `extension_lifecycle_capabilities.rs` that //! calls `assert_eq!(remove["payload"]["removed"], true)`). //! -//! The search output contains `installation_phase: "installed"` only when the -//! extension is in the installed lifecycle state — the field is entirely absent -//! when the extension is not installed (confirmed from the unit test that calls -//! `assert_eq!(available_github.get("installation_phase"), None)` before install -//! and `assert_eq!(installed_github["installation_phase"], "installed")` after). -//! After removal the extension reverts to the pre-install state, so -//! `installation_phase` disappears again. +//! Post-#5416-Phase-0, `availability` is populated for every search result, +//! including registry-only (not-installed) ones — `not_installed` for "notion" +//! after removal proves the removal reverted the installation record, not that +//! the whole field vanished (confirmed from the unit test that calls +//! `assert_eq!(available_github["availability"], "not_installed")` before +//! install and `assert_eq!(installed_github["availability"], "needs_auth")` +//! after). use super::reborn_support::group::{HarnessResult, RebornIntegrationGroup}; use super::reborn_support::reply::RebornScriptedReply; @@ -107,26 +109,18 @@ pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { .assert_tool_invoked("builtin.extension_search") .await?; - // Absence assertion: after removal `installation_phase` reverts to absent. - // `assert_tool_result_contains` returns `Ok` when the needle IS present and - // `Err` when it is absent — invert to assert absence. - if viewer - .assert_tool_result_contains(r#""installation_phase":"installed""#) - .await - .is_ok() - { - return Err( - "removed extension still shows installation_phase:installed in cross-thread search; \ - builtin.extension_remove did not propagate through the shared store" - .into(), - ); - } + // Positive assertion: after removal, "notion" reverts to `not_installed` + // (the collapsed projection's explicit "no installation record" value) — + // not `needs_auth`/`available`, which would mean the removal did not + // propagate through the shared store. + viewer + .assert_tool_result_contains(r#""availability":"not_installed""#) + .await?; // Non-vacuity guard: "notion" must still appear in the catalog search result // (as an available-but-not-installed bundled extension), proving the search - // actually ran and returned catalog entries. The absence of - // `installation_phase` is therefore meaningful — not a symptom of an empty - // or errored result. + // actually ran and returned catalog entries. The `not_installed` value is + // therefore meaningful — not a symptom of an empty or errored result. if viewer .assert_tool_result_contains("\"notion\"") .await diff --git a/tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs b/tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs new file mode 100644 index 00000000000..68249af8ca9 --- /dev/null +++ b/tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs @@ -0,0 +1,215 @@ +//! Regression coverage for bug #5416: `builtin.extension_search` must NOT render +//! the model-visible "already configured or active … do not ask the user for +//! credentials" message for an extension that is `Enabled` in lifecycle state +//! but has NO credential account configured — and must STILL render it for an +//! extension that is legitimately ready (no credential required). +//! +//! Root cause (as originally diagnosed, in `extension_lifecycle.rs::search_installation_phase`, +//! since REMOVED by the #5416 fix — see the SHAPE NOTE below): once an +//! installation's `activation_state()` mapped to `LifecyclePhase::Active`, the +//! function returned that phase immediately — `search_credentials_configured` +//! (which consulted the `RuntimeExtensionActivationCredentialGate`) was only ever +//! called for the `LifecyclePhase::Installed` case. An `Enabled` installation +//! therefore never had its credential state checked, so +//! `extension_search_has_ready_result` (which looked at `installation_phase` + +//! the always-cleared `credential_requirements` field — see +//! `suppress_search_credential_onboarding`) reported it as ready regardless of +//! whether a credential account existed. +//! +//! The fix replaced that lifecycle-phase-gated check with the collapsed +//! `resolve_extension_availability` / `ExtensionAvailability` projection (see +//! `ironclaw_reborn_composition::extension_availability`), which now checks +//! credential presence uniformly regardless of activation state — so this +//! scenario drives the current `builtin.extension_search` path, not the +//! removed `search_installation_phase`/`LifecyclePhase::Active` branch. +//! +//! `builtin.extension_activate` cannot reach `Enabled` without a configured +//! credential account for a credentialed extension (it raises `AuthRequired` +//! first). So each buggy case installs through the real `builtin.extension_install` +//! tool call (install needs no credentials) and then flips JUST the activation +//! state to `Enabled` directly on the shared installation store via +//! `set_activation_state` (E-EXTSTORE seam) — bypassing the credential gate, +//! with NO credential account created anywhere. +//! +//! ## Coverage — not just Google OAuth +//! +//! | Extension | Credential kind | Expected ready message | +//! |--------------|------------------------|------------------------| +//! | `gmail` | Google OAuth | ABSENT (needs auth) | +//! | `github` | GitHub OAuth (≠ google)| ABSENT (needs auth) | +//! | `notion` | Notion OAuth + **MCP** | ABSENT (needs auth) | +//! | `web-access` | none required | PRESENT (control) | +//! +//! The three credentialed cases pin the bug (message must be absent). The +//! `web-access` control pins that the fix does NOT over-suppress a genuinely +//! ready, credential-free extension. +//! +//! Shared-store preconditions from earlier scenarios in this group: +//! `github` installed (Scenario 1), `notion` removed (Scenario 2), `web-access` +//! Enabled (Scenario 3), `gmail` untouched. This scenario seeds/installs +//! accordingly and does not disturb their recorded assertions. +//! +//! SHAPE NOTE (Phase 0 of the #5416 fix, LANDED): the fix replaced the +//! model-facing `installation_phase` field on the search summary with a +//! collapsed `availability` field. The `assert_active_in_result` guards below +//! now assert `"availability":"needs_auth"` to prove the seeded `Enabled` +//! state actually reached the search result as a credentialed-but-unauthenticated +//! extension — without that guard a failed seed would leave the extension +//! `Installed`, which is NOT the buggy Active branch, and the message-absence +//! assertion would pass vacuously. + +use super::reborn_support::builder::RebornIntegrationHarness; +use super::reborn_support::group::{HarnessResult, RebornIntegrationGroup}; +use super::reborn_support::reply::RebornScriptedReply; +use ironclaw_extensions::{ExtensionActivationState, ExtensionInstallationId}; +use serde_json::json; + +const READY_MESSAGE_MARKER: &str = "already configured or active"; + +pub async fn run(g: &RebornIntegrationGroup) -> HarnessResult<()> { + let mut bugs: Vec = Vec::new(); + + // ── gmail — Google OAuth, fresh install then seed Enabled ─────────────── + install_extension(g, "gmail", "ready-cred-gmail-install").await?; + seed_enabled(g, "gmail").await?; + let gmail = search(g, "gmail", "ready-cred-gmail-view").await?; + gmail.assert_tool_result_contains("\"gmail\"").await?; + assert_active_in_result(&gmail).await?; // SHAPE-COUPLED (see module note) + if ready_message_present(&gmail).await { + bugs.push("gmail (Google OAuth)".into()); + } + + // ── github — GitHub OAuth (different provider). Scenario 1 already + // installed it; seed Enabled directly on the shared store. ──────────── + seed_enabled(g, "github").await?; + let github = search(g, "github", "ready-cred-github-view").await?; + github.assert_tool_result_contains("\"github\"").await?; + assert_active_in_result(&github).await?; // SHAPE-COUPLED + if ready_message_present(&github).await { + bugs.push("github (GitHub OAuth)".into()); + } + + // ── notion — Notion OAuth + MCP. Scenario 2 removed it; re-install. ───── + install_extension(g, "notion", "ready-cred-notion-install").await?; + seed_enabled(g, "notion").await?; + let notion = search(g, "notion", "ready-cred-notion-view").await?; + notion.assert_tool_result_contains("\"notion\"").await?; + assert_active_in_result(¬ion).await?; // SHAPE-COUPLED + if ready_message_present(¬ion).await { + bugs.push("notion (Notion OAuth / MCP)".into()); + } + + // ── web-access CONTROL — no credential required, already Enabled by + // Scenario 3. The ready message SHOULD be present; the fix must not + // over-suppress a genuinely ready, credential-free extension. ───────── + let web = search(g, "web-access", "ready-cred-web-view").await?; + web.assert_tool_result_contains("\"web-access\"").await?; + if !ready_message_present(&web).await { + bugs.push( + "CONTROL web-access: ready message wrongly ABSENT (over-suppression of a \ + credential-free ready extension)" + .into(), + ); + } + + if !bugs.is_empty() { + return Err(format!( + "bug #5416: `builtin.extension_search` mis-reported credential readiness for: {bugs:?}. \ + Credentialed Enabled-but-unauthenticated extensions must NOT be reported \"already \ + configured or active / do not ask the user for credentials\", and a credential-free \ + ready extension must still be. `resolve_extension_availability` must consult the \ + credential gate for every installed extension regardless of activation state, not \ + only for a specific lifecycle phase (the historical bug — see the module doc — gated \ + that check on the now-removed `LifecyclePhase::Installed` vs `LifecyclePhase::Active` \ + branch in the deleted `search_installation_phase`)." + ) + .into()); + } + Ok(()) +} + +/// Install an extension through the real `builtin.extension_install` tool call +/// (needs no credentials; writes the manifest + `Installed` installation row). +async fn install_extension( + g: &RebornIntegrationGroup, + extension_id: &str, + conversation: &str, +) -> HarnessResult<()> { + let installer = g + .thread(conversation) + .script([ + RebornScriptedReply::tool_call( + "builtin.extension_install", + json!({ "extension_id": extension_id }), + ), + RebornScriptedReply::text("installed"), + ]) + .build() + .await?; + installer + .submit_turn(&format!("install {extension_id}")) + .await?; + installer + .assert_tool_result_contains("\"installed\":true") + .await?; + Ok(()) +} + +/// Flip an already-installed extension straight to `Enabled` on the shared +/// installation store (E-EXTSTORE seam) — the only way to construct the buggy +/// Active-but-uncredentialed state, since `builtin.extension_activate` gates on +/// credentials for a credentialed extension. No credential account is created. +async fn seed_enabled(g: &RebornIntegrationGroup, extension_id: &str) -> HarnessResult<()> { + let store = g + .capability_harness() + .ok_or("extension_lifecycle group missing HostRuntime capability harness")? + .extension_installation_store_for_test() + .ok_or("harness missing extension installation store (E-EXTSTORE seam)")?; + let installation_id = ExtensionInstallationId::new(extension_id)?; + store + .set_activation_state(&installation_id, ExtensionActivationState::Enabled) + .await?; + Ok(()) +} + +/// Run `builtin.extension_search` for `query` on a fresh thread over the shared +/// store and return the viewer harness for result assertions. +async fn search( + g: &RebornIntegrationGroup, + query: &str, + conversation: &str, +) -> HarnessResult { + let viewer = g + .thread(conversation) + .script([ + RebornScriptedReply::tool_call("builtin.extension_search", json!({ "query": query })), + RebornScriptedReply::text("searched"), + ]) + .build() + .await?; + viewer.submit_turn(&format!("search {query}")).await?; + viewer + .assert_tool_invoked("builtin.extension_search") + .await?; + Ok(viewer) +} + +/// Whether the model-visible "already configured or active … do not ask for +/// credentials" ready message appears in the search tool result. +async fn ready_message_present(viewer: &RebornIntegrationHarness) -> bool { + viewer + .assert_tool_result_contains(READY_MESSAGE_MARKER) + .await + .is_ok() +} + +/// Non-vacuity guard that the seeded `Enabled` state actually reached the +/// search result (the buggy branch only fires for an Active installation). +/// +/// Post-Phase-0: an `Enabled`-but-uncredentialed extension projects +/// `availability: needs_auth` — see module-level SHAPE NOTE. +async fn assert_active_in_result(viewer: &RebornIntegrationHarness) -> HarnessResult<()> { + viewer + .assert_tool_result_contains(r#""availability":"needs_auth""#) + .await +} diff --git a/tests/reborn_integration_auth_gate.rs b/tests/reborn_integration_auth_gate.rs index 6ae74a066fb..0291d5a9c1a 100644 --- a/tests/reborn_integration_auth_gate.rs +++ b/tests/reborn_integration_auth_gate.rs @@ -6,6 +6,39 @@ //! returns `AuthRequired`) → `CapabilityObligationError::AuthRequired` → the //! agent loop blocks the run at `BlockedAuth` with a `gate:auth-` ref. Nothing //! is faked except the model at the vendor-SDK seam. +//! +//! ## #5416 Phase 3 tie-in +//! +//! This is also the load-bearing e2e coverage for the Phase 3 connection-state +//! description fold (`ironclaw_loop_support::capability_port::describe_with_access`). +//! `FixedRuntimeCredentialAccountResolver::account_configured` returning +//! `Ok(false)` for the `github` provider is exactly the Phase 2 signal that +//! downgrades `VisibleCapabilityAccess` to `NeedsAuth` for every `github.*` +//! capability on this harness's surface — so the real production chain +//! (`HostRuntimeLoopCapabilityPort::visible_capabilities` → +//! `LlmProviderModelGateway` → the real `ironclaw_llm` decorator chain) should +//! ship a model-visible tool description carrying the not-connected marker for +//! `github.create_issue`. +//! +//! This test was chosen over extending +//! `tests/reborn_group_extensions/scenario_search_ready_message_requires_credential.rs` +//! (the scenario named in the original Phase 3 plan): that scenario's group +//! (`RebornIntegrationGroup::extension_lifecycle`) seeds a generic +//! `CredentialOwnership::UserReusable` "google"/"github"/"notion" account at +//! harness construction (`seed_extension_lifecycle_credentials`) that a +//! `UserReusable` account satisfies for ANY requester extension of the same +//! provider — so `account_configured` resolves `true` for `gmail.*` capabilities +//! there regardless of the scenario's `Enabled`-but-uncredentialed extension +//! activation-state manipulation. That scenario's `"availability":"needs_auth"` +//! JSON field comes from a different, still-unconverged signal +//! (`ironclaw_product_workflow`'s per-installation credential-binding check — +//! see `docs/plans/2026-07-01-reborn-google-connection-state-5416.md`), not from +//! `VisibleCapabilityAccess`/`CapabilityCredentialPresence`. Extending it would +//! not actually exercise a `NeedsAuth` capability on the model's tool surface. +//! This `live_auth_gate` harness is the one that genuinely drives the +//! `CapabilityCredentialPresence` → `VisibleCapabilityAccess::NeedsAuth` path our +//! fold depends on, through the same production wiring +//! (`HostRuntimeServices::build_host_runtime`) real Reborn instances use. #[allow(dead_code)] #[path = "support/reborn/mod.rs"] @@ -13,6 +46,7 @@ mod reborn_support; #[allow(dead_code)] mod support; +use ironclaw_loop_support::CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER; use reborn_support::group::RebornIntegrationGroup; use reborn_support::reply::RebornScriptedReply; @@ -38,4 +72,33 @@ async fn github_capability_with_auth_required_credential_raises_blocked_auth_gat // `submit_turn_until_auth_blocked` already validates the `gate:auth-` prefix // and returns `Err` otherwise, so the `.expect` above is the real failure // point — no redundant assert needed here. + + // #5416 Phase 3: the real decorator chain must have shipped the + // `github.create_issue` tool definition to the model with the fixed + // not-connected marker folded into its description — proving the + // NeedsAuth→description fold reaches the actual model-visible tool list, + // not just the host-side descriptor view. + let captured = harness.captured_tool_definitions(); + let first_call_tools = captured + .first() + .expect("the scripted turn made at least one tool-bearing model call"); + // Exact match, not a substring: the harness's github surface also + // registers `github.create_issue_comment` + // (`tests/support/reborn/extension_surface.rs`), whose provider tool + // name (`provider_tool_name_base` in + // `ironclaw_loop_support::capability_port` maps `.` -> `__`) is + // `github__create_issue_comment` — a `.contains("create_issue")` match + // would silently accept either tool depending on iteration order. + let create_issue_tool = first_call_tools + .iter() + .find(|tool| tool.name == "github__create_issue") + .expect("github.create_issue is on the model-visible tool list"); + assert!( + create_issue_tool + .description + .contains(CAPABILITY_NEEDS_AUTH_DESCRIPTION_MARKER), + "github.create_issue's model-visible description must carry the \ + not-connected marker when its credential account is unconfigured; got: {:?}", + create_issue_tool.description + ); } diff --git a/tests/support/reborn/builder.rs b/tests/support/reborn/builder.rs index 3e12ad064c9..568e5b22bba 100644 --- a/tests/support/reborn/builder.rs +++ b/tests/support/reborn/builder.rs @@ -56,6 +56,8 @@ use super::process::ScriptedProcessResult; use super::reply::RebornScriptedReply; use super::session_thread::RebornThreadHarness; use super::test_adapter::RebornTestIngress; +use crate::support::trace_llm::TraceLlm; +use ironclaw_llm::ToolDefinition; type HarnessResult = Result>; @@ -324,6 +326,12 @@ pub struct RebornIntegrationHarness { pub(crate) baseline_result_count: usize, /// Recorded-process-command count at harness construction. See `baseline_invocation_count`. pub(crate) baseline_process_count: usize, + /// The scripted raw-provider seam, retained (in addition to the + /// type-erased `Arc` fed to the real decorator chain) so + /// tests can inspect what the real chain actually shipped to the model — + /// e.g. `captured_tool_definitions()` for asserting on model-visible tool + /// descriptions (#5416 Phase 3). + pub(crate) trace_llm: Arc, } impl RebornIntegrationHarness { @@ -536,6 +544,18 @@ impl RebornIntegrationHarness { all[self.baseline_egress_count..].to_vec() } + /// Tool definitions the real `ironclaw_llm` decorator chain shipped to the + /// scripted model for each call so far, in call order (index `i` matches + /// the `i`-th model turn). Reads straight through to `TraceLlm`, the + /// vendor-SDK-seam fake — everything above it (dispatcher, capability + /// surface mapping, `CompletionRequest`/tool-def assembly) ran for real. + /// Not baseline-sliced: unlike the capability recorder, `TraceLlm` is + /// per-thread (built fresh in `RebornThreadBuilder::build()`), so there is + /// no prior-thread history to exclude. + pub fn captured_tool_definitions(&self) -> Vec> { + self.trace_llm.captured_tool_definitions() + } + /// Assert that a `builtin.shell` command was recorded by the inert process /// port and that the recorded command string contains `substr`. This proves /// the shell tool call was dispatched through the process port without diff --git a/tests/support/reborn/group.rs b/tests/support/reborn/group.rs index 9d9f9a96817..fa403763700 100644 --- a/tests/support/reborn/group.rs +++ b/tests/support/reborn/group.rs @@ -767,7 +767,12 @@ impl<'g> RebornThreadBuilder<'g> { // --- per-thread scripted gateway, registered before any submit --------- // Session path is per-conversation so group threads do not clobber each // other's LLM session cache under the same `turn_root`. - let raw: Arc = Arc::new(scripted_trace_llm(self.replies)); + // Retain the concrete `TraceLlm` handle (before it is type-erased into + // `Arc` below) so the harness can expose + // `captured_tool_definitions()` for assertions on the model-visible + // tool descriptions the real decorator chain actually shipped. + let trace_llm = Arc::new(scripted_trace_llm(self.replies)); + let raw: Arc = trace_llm.clone(); let session = create_session_manager(SessionConfig { session_path: shared .turn_root @@ -844,6 +849,7 @@ impl<'g> RebornThreadBuilder<'g> { baseline_egress_count, baseline_result_count, baseline_process_count, + trace_llm, }) } } diff --git a/tests/support/reborn/harness.rs b/tests/support/reborn/harness.rs index a64bd7b929d..6af82bf3d5d 100644 --- a/tests/support/reborn/harness.rs +++ b/tests/support/reborn/harness.rs @@ -35,7 +35,7 @@ use ironclaw_auth::{ CredentialOwnership, NewCredentialAccount, ProviderScope, }; use ironclaw_authorization::{GrantAuthorizer, TrustAwareCapabilityDispatchAuthorizer}; -use ironclaw_extensions::ExtensionRegistry; +use ironclaw_extensions::{ExtensionInstallationStore, ExtensionRegistry}; use ironclaw_filesystem::{ BackendCapabilities, BackendId, BackendKind, CompositeRootFilesystem, ContentKind, InMemoryBackend, IndexPolicy, LocalFilesystem, MountDescriptor, RootFilesystem, @@ -1591,6 +1591,15 @@ pub(crate) struct HostRuntimeCapabilityHarness { /// `capability_ids` (i.e. only `project_tools()` surfaces it), so other /// local-dev groups are unaffected. Tests read projects back via `project_service`. project_service: Option>, + /// Extension installation store backing the local-dev extension-lifecycle + /// port (E-EXTSTORE seam). `Some` only for `new_with_options`-built + /// local-dev harnesses (which flow through `RebornServices`); `None` for + /// the lower-level constructors and the Echo backend. Mirrors + /// `profile_filesystem`/`project_service`'s role: lets a test seed an + /// `ExtensionInstallation` directly (e.g. `Enabled` with no credential + /// binding) without driving it through `install`/`activate`, which gates + /// on credentials. Read back via `extension_installation_store_for_test`. + installation_store: Option>, } struct HostRuntimeHarnessOptions { @@ -1804,6 +1813,7 @@ impl HostRuntimeCapabilityHarness { process_port: None, profile_filesystem: None, project_service: None, + installation_store: None, }) } @@ -2136,10 +2146,12 @@ impl HostRuntimeCapabilityHarness { } let approval_parts = services.local_dev_approval_test_parts(); let auto_approve_settings = services.local_dev_auto_approve_settings_for_test(); - // Capture the profile filesystem + project service before - // `services.host_runtime` is moved out below (E-PROFILE / E-PROJ seams). + // Capture the profile filesystem + project service + extension + // installation store before `services.host_runtime` is moved out below + // (E-PROFILE / E-PROJ / E-EXTSTORE seams). let profile_filesystem = services.local_dev_profile_filesystem_for_test(); let project_service = services.local_dev_project_service_for_test(); + let installation_store = services.extension_installation_store_for_test(); let pending_approval_scopes = Arc::new(Mutex::new(HashMap::new())); let runtime = services .host_runtime @@ -2173,6 +2185,7 @@ impl HostRuntimeCapabilityHarness { process_port: None, profile_filesystem, project_service, + installation_store, }) } @@ -2319,6 +2332,7 @@ impl HostRuntimeCapabilityHarness { process_port: None, profile_filesystem: None, project_service: None, + installation_store: None, }) } @@ -2387,6 +2401,7 @@ impl HostRuntimeCapabilityHarness { process_port: None, profile_filesystem: None, project_service: None, + installation_store: None, }) } @@ -2460,6 +2475,7 @@ impl HostRuntimeCapabilityHarness { process_port: None, profile_filesystem: None, project_service: None, + installation_store: None, }) } @@ -2661,6 +2677,23 @@ impl HostRuntimeCapabilityHarness { self.project_service.clone() } + /// E-EXTSTORE: the extension installation store backing this harness's + /// local-dev extension-lifecycle port, for direct seed/read-back in tests. + /// Mirrors `profile_filesystem_for_test`/`project_service_for_test`'s role. + /// `Some` only for `new_with_options`-built local-dev harnesses (e.g. + /// `extension_lifecycle_tools()`). Lets a test write an + /// `ExtensionInstallation` directly (e.g. `Enabled` with an empty + /// `credential_bindings` list) so `builtin.extension_search` observes an + /// activation state that `install`/`activate` cannot reach on their own — + /// `activate` gates on credentials and returns `AuthRequired` instead of + /// ever reaching `Enabled` when the extension requires credentials that + /// are not configured. + pub(crate) fn extension_installation_store_for_test( + &self, + ) -> Option> { + self.installation_store.clone() + } + /// E-PROJ: wrap `port` with the local-dev synthetic capabilities this harness /// surfaces, in one linear step (keeps the capability-specific knowledge out /// of `create_capability_port`'s main assembly chain). @@ -3396,6 +3429,19 @@ impl RuntimeCredentialAccountResolver for FixedRuntimeCredentialAccountResolver handle, }) } + + async fn account_configured( + &self, + request: RuntimeCredentialAccountRequest<'_>, + ) -> Result { + assert_eq!(request.provider.as_str(), "github"); + assert_eq!(request.requester_extension.as_str(), "github"); + match &self.result { + Ok(_) => Ok(true), + Err(CredentialStageError::AuthRequired) => Ok(false), + Err(CredentialStageError::Backend) => Err(CredentialStageError::Backend), + } + } } struct StaticSecretStore {