From cd2174345b3cc1ad7e1be721818101e6ca1b2059 Mon Sep 17 00:00:00 2001 From: Emil Bogomolov Date: Fri, 12 Jun 2026 16:00:50 -0700 Subject: [PATCH 1/8] make 'always allow' approvals persist (tested on google suite)' --- Cargo.lock | 1 + crates/ironclaw_approvals/src/policy.rs | 109 +++--- .../ironclaw_host_runtime/src/production.rs | 20 +- ...t_runtime_persistent_approvals_contract.rs | 42 ++- crates/ironclaw_product_workflow/Cargo.toml | 1 + .../src/approval_interaction/service.rs | 12 +- .../tests/approval_interaction_contract.rs | 334 +++++++++++++++++- docs/reborn/contracts/approvals.md | 7 +- 8 files changed, 440 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0332cda14a3..15920affa33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4821,6 +4821,7 @@ dependencies = [ "ironclaw_conversations", "ironclaw_event_projections", "ironclaw_events", + "ironclaw_filesystem", "ironclaw_host_api", "ironclaw_host_runtime", "ironclaw_loop_support", diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index fc263178bf8..69632253e57 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -40,8 +40,6 @@ pub fn persistent_approval_grant_issuer() -> Principal { #[derive(Debug, Error)] pub enum PersistentApprovalPolicyError { - #[error("persistent approval scope must include project_id or thread_id")] - UnsupportedScope, #[error("unknown persistent approval policy")] UnknownPolicy, #[error("persistent approval policy changed concurrently")] @@ -95,28 +93,28 @@ pub struct PersistentApprovalScope { pub user_id: UserId, pub agent_id: Option, pub project_id: Option, + /// Always `None`. The field is retained (rather than removed) purely to + /// preserve the policy digest: the digest is `serde_json::to_vec(key)`, and + /// pre-existing project-scoped policies serialized `"thread_id": null`. + /// Keeping the field means those digests are unchanged so old policies still + /// match. `thread_id` never participates in the persistent approval scope — + /// an "always allow" granted in one thread applies to all of that user's + /// threads with the same agent (and project, when present). pub thread_id: Option, } impl PersistentApprovalScope { - pub fn from_resource_scope( - scope: &ResourceScope, - ) -> Result { - if scope.project_id.is_none() && scope.thread_id.is_none() { - return Err(PersistentApprovalPolicyError::UnsupportedScope); - } - let thread_id = if scope.project_id.is_some() { - None - } else { - scope.thread_id.clone() - }; - Ok(Self { + pub fn from_resource_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(), - thread_id, - }) + // Intentionally always `None`: see the field doc-comment. The scope + // is (tenant_id, user_id, agent_id?, project_id?) and is thread- and + // channel-agnostic. + thread_id: None, + } } } @@ -134,13 +132,13 @@ impl PersistentApprovalPolicyKey { action: PersistentApprovalAction, capability_id: CapabilityId, grantee: Principal, - ) -> Result { - Ok(Self { - scope: PersistentApprovalScope::from_resource_scope(scope)?, + ) -> Self { + Self { + scope: PersistentApprovalScope::from_resource_scope(scope), action, capability_id, grantee, - }) + } } } @@ -243,7 +241,7 @@ impl PersistentApprovalPolicyStore for InMemoryPersistentApprovalPolicyStore { input.action, input.capability_id, input.grantee, - )?; + ); let mut policies = self .policies .write() @@ -363,7 +361,7 @@ where input.action, input.capability_id, input.grantee, - )?; + ); let path = self.cached_policy_path(&key)?; let lock = self.mutation_lock(&key); let _guard = lock.lock().await; @@ -595,10 +593,10 @@ fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { if let Some(agent_id) = &scope.agent_id { segments.push(format!("agents/{agent_id}")); } + // `thread_id` is always `None` (see `PersistentApprovalScope`), so the scope + // path only ever branches on `project_id`. if let Some(project_id) = &scope.project_id { segments.push(format!("projects/{project_id}")); - } else if let Some(thread_id) = &scope.thread_id { - segments.push(format!("threads/{thread_id}")); } if segments.is_empty() { "scope".to_string() @@ -923,23 +921,65 @@ mod tests { #[tokio::test] async fn policy_scope_prefers_project_over_thread() { + // Project is still part of the scope; differing thread ids under the same + // project produce identical keys (project carried, thread_id always None). let scope_a = scope(Some("project-a"), Some("thread-a")); let scope_b = scope(Some("project-a"), Some("thread-b")); + let derived_a = PersistentApprovalScope::from_resource_scope(&scope_a); + let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b); + + assert_eq!(derived_a, derived_b); assert_eq!( - PersistentApprovalScope::from_resource_scope(&scope_a).unwrap(), - PersistentApprovalScope::from_resource_scope(&scope_b).unwrap() + derived_a.project_id, + Some(ProjectId::new("project-a").unwrap()) ); + assert_eq!(derived_a.thread_id, None); } #[tokio::test] - async fn policy_scope_uses_thread_without_project() { + async fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); let scope_b = scope(None, Some("thread-b")); + let derived_a = PersistentApprovalScope::from_resource_scope(&scope_a); + let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b); + + assert_eq!(derived_a, derived_b); + assert_eq!(derived_a.thread_id, None); + } + + #[tokio::test] + async fn policy_scope_without_project_isolates_users() { + let scope_a = ResourceScope { + user_id: UserId::new("alice").unwrap(), + ..scope(None, Some("thread-a")) + }; + let scope_b = ResourceScope { + user_id: UserId::new("bob").unwrap(), + ..scope(None, Some("thread-a")) + }; + + assert_ne!( + PersistentApprovalScope::from_resource_scope(&scope_a), + PersistentApprovalScope::from_resource_scope(&scope_b) + ); + } + + #[tokio::test] + async fn policy_scope_without_project_isolates_agents() { + let scope_a = ResourceScope { + agent_id: Some(AgentId::new("agent-x").unwrap()), + ..scope(None, Some("thread-a")) + }; + let scope_b = ResourceScope { + agent_id: Some(AgentId::new("agent-y").unwrap()), + ..scope(None, Some("thread-a")) + }; + assert_ne!( - PersistentApprovalScope::from_resource_scope(&scope_a).unwrap(), - PersistentApprovalScope::from_resource_scope(&scope_b).unwrap() + PersistentApprovalScope::from_resource_scope(&scope_a), + PersistentApprovalScope::from_resource_scope(&scope_b) ); } @@ -987,16 +1027,6 @@ mod tests { assert_eq!(grant.issued_by, persistent_approval_grant_issuer()); } - #[tokio::test] - async fn from_resource_scope_errs_without_project_or_thread() { - let scope = scope(None, None); - - assert!(matches!( - PersistentApprovalScope::from_resource_scope(&scope), - Err(PersistentApprovalPolicyError::UnsupportedScope) - )); - } - fn input(scope: ResourceScope) -> PersistentApprovalPolicyInput { PersistentApprovalPolicyInput { scope, @@ -1024,7 +1054,6 @@ mod tests { CapabilityId::new("fixture.echo").unwrap(), Principal::User(UserId::new("alice").unwrap()), ) - .unwrap() } fn scope(project_id: Option<&str>, thread_id: Option<&str>) -> ResourceScope { diff --git a/crates/ironclaw_host_runtime/src/production.rs b/crates/ironclaw_host_runtime/src/production.rs index 93747d8fb22..ae31048d7a3 100644 --- a/crates/ironclaw_host_runtime/src/production.rs +++ b/crates/ironclaw_host_runtime/src/production.rs @@ -986,17 +986,7 @@ impl DefaultHostRuntime { ); return; } - let scope = match PersistentApprovalScope::from_resource_scope(&context.resource_scope) { - Ok(scope) => scope, - Err(error) => { - tracing::debug!( - capability_id = %capability_id, - error = %error, - "persistent approval lookup skipped for unsupported scope" - ); - return; - } - }; + let scope = PersistentApprovalScope::from_resource_scope(&context.resource_scope); let lookup_results = join_all(persistent_approval_grantees(context).into_iter().map( |grantee| { let policies = Arc::clone(policies); @@ -1557,9 +1547,11 @@ fn persistent_approval_grantees(context: &ironclaw_host_api::ExecutionContext) - if let Some(mission_id) = &context.mission_id { grantees.push(Principal::Mission(mission_id.clone())); } - if let Some(thread_id) = &context.thread_id { - grantees.push(Principal::Thread(thread_id.clone())); - } + // No `Principal::Thread` grantee: persistent approval policies are never + // written under a thread grantee (the grantee always comes from + // `ApprovalRequest.requested_by`, which is `Principal::User` or + // `Principal::Extension`), so looking one up could never match. Persistent + // approvals are deliberately thread-agnostic (see #4825). grantees } diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index ce7f1df0758..1a99add0123 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -477,20 +477,44 @@ async fn default_runtime_skips_expired_persistent_policy() { } #[tokio::test] -async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope() { +async fn default_runtime_uses_persistent_policy_for_no_project_no_thread_scope() { + // A WebChat-style context (no project, no thread) now yields a valid + // (tenant, user, agent) persistent approval scope: the lookup proceeds and a + // seeded "always allow" policy authorizes dispatch without a gate. let registry = Arc::new(registry_with_echo_capability()); let dispatcher = Arc::new(RecordingDispatcher::default()); let authorizer: Arc = Arc::new(GrantAuthorizer); let run_state = Arc::new(InMemoryRunStateStore::new()); let approval_requests = Arc::new(InMemoryApprovalRequestStore::new()); - let policies: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let policies = Arc::new(InMemoryPersistentApprovalPolicyStore::new()); let mut context = execution_context_without_grants(); context.project_id = None; context.thread_id = None; context.resource_scope.project_id = None; context.resource_scope.thread_id = None; + policies + .allow(PersistentApprovalPolicyInput { + scope: context.resource_scope.clone(), + action: PersistentApprovalAction::Dispatch, + capability_id: capability_id(), + grantee: Principal::Extension(context.extension_id.clone()), + approved_by: Principal::User(context.user_id.clone()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: Some(1), + }, + source_approval_request_id: None, + }) + .await + .expect("seed persistent policy"); + let policy_store: Arc = policies; + let runtime = DefaultHostRuntime::new( registry, dispatcher.clone(), @@ -501,7 +525,7 @@ async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope( .with_trust_policy(Arc::new(local_manifest_trust_policy())) .with_run_state(run_state) .with_approval_requests(approval_requests) - .with_persistent_approval_policies(policies); + .with_persistent_approval_policies(policy_store); let outcome = runtime .invoke_capability(RuntimeCapabilityRequest::new( @@ -515,13 +539,13 @@ async fn default_runtime_falls_back_gracefully_for_unsupported_persistent_scope( .unwrap(); match outcome { - ironclaw_host_runtime::RuntimeCapabilityOutcome::Failed(failure) => { - assert_eq!(failure.capability_id, capability_id()); - assert_eq!(failure.kind, RuntimeFailureKind::Authorization); + ironclaw_host_runtime::RuntimeCapabilityOutcome::Completed(completed) => { + assert_eq!(completed.capability_id, capability_id()); + assert_eq!(completed.output, json!({"ok": true})); } - other => panic!("expected authorization failure, got {:?}", other), + other => panic!("expected Completed outcome, got {:?}", other), } - assert!(!dispatcher.has_request()); + assert!(dispatcher.has_request()); } #[tokio::test] diff --git a/crates/ironclaw_product_workflow/Cargo.toml b/crates/ironclaw_product_workflow/Cargo.toml index 163093ede2b..fb1e01deef8 100644 --- a/crates/ironclaw_product_workflow/Cargo.toml +++ b/crates/ironclaw_product_workflow/Cargo.toml @@ -44,6 +44,7 @@ url = "2" uuid = { version = "1", features = ["v4", "v5", "serde"] } [dev-dependencies] +ironclaw_filesystem = { path = "../ironclaw_filesystem" } ironclaw_host_runtime = { path = "../ironclaw_host_runtime" } ironclaw_loop_support = { path = "../ironclaw_loop_support" } ironclaw_event_projections = { path = "../ironclaw_event_projections" } diff --git a/crates/ironclaw_product_workflow/src/approval_interaction/service.rs b/crates/ironclaw_product_workflow/src/approval_interaction/service.rs index 127e66e92e2..003ef9d957e 100644 --- a/crates/ironclaw_product_workflow/src/approval_interaction/service.rs +++ b/crates/ironclaw_product_workflow/src/approval_interaction/service.rs @@ -268,17 +268,7 @@ impl DefaultApprovalInteractionService { input.action, input.capability_id.clone(), input.grantee.clone(), - ) - .map_err(|error| { - tracing::warn!( - error = %error, - approval_request_id = %gate.request().id, - "persistent approval policy preparation failed" - ); - ProductWorkflowError::Transient { - reason: "persistent approval policy unavailable".to_string(), - } - })?; + ); Ok(PreparedAllowPolicy { input, key }) } diff --git a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs index 4489e50a2bc..bd8c30a81be 100644 --- a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs +++ b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs @@ -542,6 +542,75 @@ fn resource_scope(actor: &TurnActor) -> ResourceScope { } } +/// A no-project resource scope (WebChat shape) with explicit user/agent/thread. +/// Threads carry `project_id = None`, which is the case the persistent approval +/// scope fix targets: the scope key must be thread-agnostic. +fn no_project_scope(user: &str, agent: Option<&str>, thread: &str) -> ResourceScope { + ResourceScope { + tenant_id: TenantId::new("tenant-alpha").expect("tenant"), + user_id: UserId::new(user).expect("user"), + agent_id: agent.map(|id| ironclaw_host_api::AgentId::new(id).expect("agent")), + project_id: None, + mission_id: None, + thread_id: Some(ThreadId::new(thread).expect("thread")), + invocation_id: InvocationId::new(), + } +} + +/// In-memory-backed scoped filesystem matching the approvals store mount layout. +fn scoped_fs( + tenant: &str, + user: &str, +) -> Arc> { + use ironclaw_host_api::{MountAlias, MountGrant, MountPermissions, MountView, VirtualPath}; + let backend = Arc::new(ironclaw_filesystem::InMemoryBackend::new()); + let mounts = MountView::new(vec![MountGrant::new( + MountAlias::new("/approvals").expect("alias"), + VirtualPath::new(format!("/engine/tenants/{tenant}/users/{user}/approvals")) + .expect("virtual path"), + MountPermissions::read_write_list_delete(), + )]) + .expect("mount view"); + Arc::new(ironclaw_filesystem::ScopedFilesystem::with_fixed_view( + backend, mounts, + )) +} + +/// Builds a service fixture whose pending gate carries the supplied resource +/// scope and approval request, so tests can drive the real `resolve` path with +/// a chosen persisted scope and grantee. +fn service_fixture_with_scope( + request: ApprovalRequest, + gate_scope: ResourceScope, +) -> ( + DefaultApprovalInteractionService, + Arc, + Arc, + TurnRunId, + GateRef, +) { + let actor = actor(gate_scope.user_id.as_str()); + let gate_ref = approval_gate_ref(request.id).expect("gate ref"); + let run_id = TurnRunId::new(); + let gate = ApprovalGateRecord::with_status( + gate_scope, + run_id, + gate_ref.clone(), + request, + ApprovalStatus::Pending, + ) + .expect("approval gate"); + let resolver = Arc::new(RecordingApprovalResolver::default()); + let coordinator = Arc::new(FakeTurnCoordinator::blocked(actor, gate_ref.clone())); + let service = DefaultApprovalInteractionService::new( + Arc::new(FakeReadModel::with_gate(gate)), + Arc::new(FixedLeaseTermsProvider), + resolver.clone(), + coordinator.clone(), + ); + (service, resolver, coordinator, run_id, gate_ref) +} + fn approval_request(reason: &str) -> ApprovalRequest { ApprovalRequest { id: ApprovalRequestId::new(), @@ -557,6 +626,14 @@ fn approval_request(reason: &str) -> ApprovalRequest { } } +/// Dispatch approval request for `demo.echo` with an explicit grantee +/// (`requested_by`). Used by isolation tests that vary the policy grantee. +fn approval_request_by(reason: &str, requested_by: Principal) -> ApprovalRequest { + let mut request = approval_request(reason); + request.requested_by = requested_by; + request +} + fn spawn_approval_request(reason: &str) -> ApprovalRequest { let mut request = approval_request(reason); request.action = Box::new(Action::SpawnCapability { @@ -686,8 +763,7 @@ async fn always_allow_resolves_gate_and_persists_reusable_policy() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let (service, resolver, coordinator, run_id, gate_ref) = service_fixture_for_request(request); let policies = Arc::new(InMemoryPersistentApprovalPolicyStore::new()); let policy_store: Arc = policies.clone(); @@ -725,6 +801,248 @@ async fn always_allow_resolves_gate_and_persists_reusable_policy() { assert!(policy.active_grant().is_some()); } +/// Drives the real `resolve(AlwaysAllow)` path: builds a service whose pending +/// gate carries `gate_scope` and `request`, wires `store`, and resolves with a +/// turn scope/actor derived from `gate_scope` (so the read-model gate lookup +/// matches). Asserts the resolution approved and resumed. The persisted policy +/// scope is `gate_scope` and the grantee is `request.requested_by`. +async fn drive_always_allow( + store: Arc, + request: ApprovalRequest, + gate_scope: ResourceScope, + idempotency: &str, +) { + let request_actor = TurnActor::new(gate_scope.user_id.clone()); + let request_scope = TurnScope::new( + gate_scope.tenant_id.clone(), + gate_scope.agent_id.clone(), + gate_scope.project_id.clone(), + gate_scope + .thread_id + .clone() + .expect("gate scope must carry a thread id"), + ); + let (service, resolver, coordinator, run_id, gate_ref) = + service_fixture_with_scope(request, gate_scope); + let service = service.with_persistent_policy_store(store); + + let response = service + .resolve(ResolveApprovalInteractionRequest { + scope: request_scope, + actor: request_actor, + run_id_hint: Some(run_id), + gate_ref, + decision: ApprovalInteractionDecision::AlwaysAllow, + idempotency_key: IdempotencyKey::new(idempotency).expect("idempotency"), + }) + .await + .expect("always allow"); + + assert!(matches!( + response, + ResolveApprovalInteractionResponse::Approved(_) + )); + assert_eq!(resolver.approval_count(), 1); + assert_eq!(coordinator.resumption_count(), 1); +} + +/// Acceptance criterion 1: an "always allow" granted while resolving a gate in +/// thread 1 (no project) is reused for the same capability in thread 2 without a +/// gate. Covered against both InMemory and Filesystem stores because the +/// filesystem scope path is part of the fix. +#[tokio::test] +async fn always_allow_grants_reuse_in_new_thread_without_project() { + let in_memory: Arc = + Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let filesystem: Arc = Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ); + + for (store, idempotency) in [ + (in_memory, "reuse-in-memory"), + (filesystem, "reuse-filesystem"), + ] { + let request = approval_request("send the email"); + let capability = match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + }; + // Grant in thread 1. + let thread_one = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, thread_one, idempotency).await; + + // Look up from thread 2 (same user/agent, no project): different thread, + // same persistent scope key, so the grant is active. + let thread_two = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &thread_two, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let policy = store + .lookup(&key) + .await + .expect("persistent policy lookup") + .expect("persistent policy active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + +/// Acceptance criterion 4 (isolation): an "always allow" granted by user A in +/// thread 1 must NOT authorize user B in thread 2 under the same tenant/agent. +#[tokio::test] +async fn always_allow_does_not_grant_other_user_in_new_thread() { + let in_memory: Arc = + Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let filesystem: Arc = Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ); + + for (store, idempotency) in [ + (in_memory, "user-iso-in-memory"), + (filesystem, "user-iso-filesystem"), + ] { + let request = approval_request_by( + "send the email", + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let capability = match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + }; + let user_a = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, user_a, idempotency).await; + + let user_b = no_project_scope("user-beta", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &user_b, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-beta").expect("user")), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another user must not inherit the grant" + ); + } +} + +/// Acceptance criterion 4 (isolation): an "always allow" granted under agent X +/// must NOT authorize agent Y under the same tenant/user. +#[tokio::test] +async fn always_allow_does_not_grant_other_agent_in_new_thread() { + let in_memory: Arc = + Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let filesystem: Arc = Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ); + + for (store, idempotency) in [ + (in_memory, "agent-iso-in-memory"), + (filesystem, "agent-iso-filesystem"), + ] { + let request = approval_request("send the email"); + let capability = match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + }; + let agent_x = no_project_scope("user-alpha", Some("agent-x"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, agent_x, idempotency).await; + + let agent_y = no_project_scope("user-alpha", Some("agent-y"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &agent_y, + PersistentApprovalAction::Dispatch, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another agent must not inherit the grant" + ); + } +} + +/// Acceptance criterion 4 (isolation): the policy key includes the grantee, so an +/// approval for extension X must not match a lookup for extension Y requesting +/// the same capability under the same scope. +#[tokio::test] +async fn always_allow_does_not_grant_other_extension_grantee() { + let in_memory: Arc = + Arc::new(InMemoryPersistentApprovalPolicyStore::new()); + let filesystem: Arc = Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ); + + for (store, idempotency) in [ + (in_memory, "ext-iso-in-memory"), + (filesystem, "ext-iso-filesystem"), + ] { + let extension_x = ironclaw_host_api::ExtensionId::new("extension-x").expect("extension"); + let request = + approval_request_by("send the email", Principal::Extension(extension_x.clone())); + let capability = match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + }; + let gate_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_always_allow(Arc::clone(&store), request, gate_scope, idempotency).await; + + // Same scope, same capability, but a different extension grantee. + let lookup_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let extension_y = ironclaw_host_api::ExtensionId::new("extension-y").expect("extension"); + let key = PersistentApprovalPolicyKey::new( + &lookup_scope, + PersistentApprovalAction::Dispatch, + capability.clone(), + Principal::Extension(extension_y), + ); + assert!( + store + .lookup(&key) + .await + .expect("persistent policy lookup") + .is_none(), + "another extension grantee must not match" + ); + + // Sanity: the granting extension X DOES match (thread-agnostic reuse). + let key_x = PersistentApprovalPolicyKey::new( + &lookup_scope, + PersistentApprovalAction::Dispatch, + capability, + Principal::Extension(extension_x), + ); + let policy = store + .lookup(&key_x) + .await + .expect("persistent policy lookup") + .expect("granting extension active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + #[tokio::test] async fn always_allow_without_policy_store_rejects_before_approval_side_effects() { let (service, resolver, coordinator, run_id, gate_ref) = service_fixture("send the email"); @@ -793,8 +1111,7 @@ async fn always_allow_disallowed_by_policy_rejects_without_persisting_or_approvi PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let actor = actor("user-alpha"); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); @@ -865,8 +1182,7 @@ async fn always_allow_does_not_persist_policy_when_resolution_fails() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); let gate = ApprovalGateRecord::with_status( @@ -932,8 +1248,7 @@ async fn always_allow_resolution_failure_preserves_existing_policy() { PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let gate_ref = approval_gate_ref(request.id).expect("gate ref"); let run_id = TurnRunId::new(); let gate = ApprovalGateRecord::with_status( @@ -1147,8 +1462,7 @@ async fn already_approved_always_allow_replay_rejects_without_persisting_policy( PersistentApprovalAction::Dispatch, capability, Principal::User(UserId::new("user-alpha").expect("user")), - ) - .expect("persistent policy key"); + ); let (service, resolver, coordinator, run_id, gate_ref) = service_fixture_for_request_status(request, ApprovalStatus::Approved); coordinator.set_status(TurnStatus::Queued); diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index c063937c94e..618d64881bb 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -256,8 +256,11 @@ This slice intentionally keeps approval resolution narrow: - no approval support for actions other than dispatch and one-shot spawn yet - persistent approval policies cover dispatch and `Action::SpawnCapability` approval interaction decisions at the current Reborn sandbox scope - (`tenant_id`, `user_id`, optional `agent_id`, and `project_id` when present, - otherwise `thread_id`) + (`tenant_id`, `user_id`, optional `agent_id`, and optional `project_id`); + `thread_id` never participates in the scope, so an "always allow" granted in + one thread applies to all of that user's threads with the same agent (and + project, when present) and is channel-agnostic — a WebUI grant is honored for + a Slack message resolving to the same `(user, agent)` - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval From 39cff9a73667073cfa1fc0d3b42e365fbc79a317 Mon Sep 17 00:00:00 2001 From: Emil Bogomolov Date: Fri, 12 Jun 2026 16:40:26 -0700 Subject: [PATCH 2/8] test(approvals): lock criterion-5 backward-compat for project-scoped policies (#4825) --- crates/ironclaw_approvals/src/policy.rs | 56 +++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index 69632253e57..3de05ca64b2 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -937,6 +937,62 @@ mod tests { assert_eq!(derived_a.thread_id, None); } + #[test] + fn project_scoped_policy_key_serialization_stays_digest_stable() { + // Criterion 5 (#4825): the persistent-approval digest is + // `serde_json::to_vec(key)` and the on-disk path derives from it. + // Pre-existing project-scoped policies serialized the scope with + // `"thread_id": null` (an old project-scoped scope already blanked + // thread_id). The field is intentionally retained (always `None`) so the + // serialization — and thus the digest and storage path — is byte-identical + // after the upgrade. If this breaks, pre-existing project-scoped + // "always allow" policies orphan. + let key = key_for(&scope(Some("project-a"), Some("thread-ignored"))); + let json = serde_json::to_string(&key).expect("serialize policy key"); + assert!( + json.contains("\"thread_id\":null"), + "project-scoped key must serialize thread_id as null to preserve the digest; got {json}" + ); + + // Digest and tenant-scope path are independent of the originating thread, + // so a policy written before the upgrade is still located afterwards. + let digest_thread_one = + policy_digest(&key_for(&scope(Some("project-a"), Some("thread-1")))).expect("digest"); + let digest_thread_two = + policy_digest(&key_for(&scope(Some("project-a"), Some("thread-2")))).expect("digest"); + assert_eq!(digest_thread_one, digest_thread_two); + assert_eq!( + within_tenant_scope(&key.scope), + "agents/agent-a/projects/project-a" + ); + } + + #[tokio::test] + async fn filesystem_project_scoped_policy_matches_in_new_thread_after_reload() { + // Criterion 5 (#4825): a project-scoped "always allow" persisted before + // thread_id was dropped from the scope must still match afterwards. A + // fresh store instance (simulating a restart/upgrade) looks the policy up + // from a DIFFERENT thread under the same project and finds it. + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); + let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); + + let granted = store + .allow(input(scope(Some("project-a"), Some("thread-1")))) + .await + .expect("allow project-scoped policy"); + + let new_thread_key = key_for(&scope(Some("project-a"), Some("thread-2"))); + let reloaded = FilesystemPersistentApprovalPolicyStore::new(scoped) + .lookup(&new_thread_key) + .await + .expect("lookup") + .expect("pre-existing project-scoped policy still matches in a new thread"); + + assert_eq!(reloaded, granted); + assert!(reloaded.active_grant().is_some()); + } + #[tokio::test] async fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); From 39677aca35cf04fe9eb1176df2f7b8d4365f2830 Mon Sep 17 00:00:00 2001 From: Emil Bogomolov Date: Fri, 12 Jun 2026 16:53:20 -0700 Subject: [PATCH 3/8] reduce slop --- .../tests/approval_interaction_contract.rs | 113 +++++++----------- 1 file changed, 40 insertions(+), 73 deletions(-) diff --git a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs index bd8c30a81be..708b6130eb5 100644 --- a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs +++ b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs @@ -634,6 +634,34 @@ fn approval_request_by(reason: &str, requested_by: Principal) -> ApprovalRequest request } +/// The two persistent-approval store backends every caller-level test exercises. +/// Filesystem scope paths are part of the fix, so both must pass. `prefix` +/// distinguishes the per-backend idempotency keys. +fn caller_level_store_pair(prefix: &str) -> [(Arc, String); 2] { + [ + ( + Arc::new(InMemoryPersistentApprovalPolicyStore::new()), + format!("{prefix}-in-memory"), + ), + ( + Arc::new( + ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( + "tenant-alpha", + "user-alpha", + )), + ), + format!("{prefix}-filesystem"), + ), + ] +} + +fn dispatch_capability(request: &ApprovalRequest) -> CapabilityId { + match request.action.as_ref() { + Action::Dispatch { capability, .. } => capability.clone(), + _ => panic!("test request should be dispatch"), + } +} + fn spawn_approval_request(reason: &str) -> ApprovalRequest { let mut request = approval_request(reason); request.action = Box::new(Action::SpawnCapability { @@ -852,27 +880,11 @@ async fn drive_always_allow( /// filesystem scope path is part of the fix. #[tokio::test] async fn always_allow_grants_reuse_in_new_thread_without_project() { - let in_memory: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); - let filesystem: Arc = Arc::new( - ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( - "tenant-alpha", - "user-alpha", - )), - ); - - for (store, idempotency) in [ - (in_memory, "reuse-in-memory"), - (filesystem, "reuse-filesystem"), - ] { + for (store, idempotency) in caller_level_store_pair("reuse") { let request = approval_request("send the email"); - let capability = match request.action.as_ref() { - Action::Dispatch { capability, .. } => capability.clone(), - _ => panic!("test request should be dispatch"), - }; - // Grant in thread 1. + let capability = dispatch_capability(&request); let thread_one = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); - drive_always_allow(Arc::clone(&store), request, thread_one, idempotency).await; + drive_always_allow(Arc::clone(&store), request, thread_one, &idempotency).await; // Look up from thread 2 (same user/agent, no project): different thread, // same persistent scope key, so the grant is active. @@ -896,29 +908,14 @@ async fn always_allow_grants_reuse_in_new_thread_without_project() { /// thread 1 must NOT authorize user B in thread 2 under the same tenant/agent. #[tokio::test] async fn always_allow_does_not_grant_other_user_in_new_thread() { - let in_memory: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); - let filesystem: Arc = Arc::new( - ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( - "tenant-alpha", - "user-alpha", - )), - ); - - for (store, idempotency) in [ - (in_memory, "user-iso-in-memory"), - (filesystem, "user-iso-filesystem"), - ] { + for (store, idempotency) in caller_level_store_pair("user-iso") { let request = approval_request_by( "send the email", Principal::User(UserId::new("user-alpha").expect("user")), ); - let capability = match request.action.as_ref() { - Action::Dispatch { capability, .. } => capability.clone(), - _ => panic!("test request should be dispatch"), - }; + let capability = dispatch_capability(&request); let user_a = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); - drive_always_allow(Arc::clone(&store), request, user_a, idempotency).await; + drive_always_allow(Arc::clone(&store), request, user_a, &idempotency).await; let user_b = no_project_scope("user-beta", Some("agent-a"), "thread-2"); let key = PersistentApprovalPolicyKey::new( @@ -942,26 +939,11 @@ async fn always_allow_does_not_grant_other_user_in_new_thread() { /// must NOT authorize agent Y under the same tenant/user. #[tokio::test] async fn always_allow_does_not_grant_other_agent_in_new_thread() { - let in_memory: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); - let filesystem: Arc = Arc::new( - ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( - "tenant-alpha", - "user-alpha", - )), - ); - - for (store, idempotency) in [ - (in_memory, "agent-iso-in-memory"), - (filesystem, "agent-iso-filesystem"), - ] { + for (store, idempotency) in caller_level_store_pair("agent-iso") { let request = approval_request("send the email"); - let capability = match request.action.as_ref() { - Action::Dispatch { capability, .. } => capability.clone(), - _ => panic!("test request should be dispatch"), - }; + let capability = dispatch_capability(&request); let agent_x = no_project_scope("user-alpha", Some("agent-x"), "thread-1"); - drive_always_allow(Arc::clone(&store), request, agent_x, idempotency).await; + drive_always_allow(Arc::clone(&store), request, agent_x, &idempotency).await; let agent_y = no_project_scope("user-alpha", Some("agent-y"), "thread-2"); let key = PersistentApprovalPolicyKey::new( @@ -986,28 +968,13 @@ async fn always_allow_does_not_grant_other_agent_in_new_thread() { /// the same capability under the same scope. #[tokio::test] async fn always_allow_does_not_grant_other_extension_grantee() { - let in_memory: Arc = - Arc::new(InMemoryPersistentApprovalPolicyStore::new()); - let filesystem: Arc = Arc::new( - ironclaw_approvals::FilesystemPersistentApprovalPolicyStore::new(scoped_fs( - "tenant-alpha", - "user-alpha", - )), - ); - - for (store, idempotency) in [ - (in_memory, "ext-iso-in-memory"), - (filesystem, "ext-iso-filesystem"), - ] { + for (store, idempotency) in caller_level_store_pair("ext-iso") { let extension_x = ironclaw_host_api::ExtensionId::new("extension-x").expect("extension"); let request = approval_request_by("send the email", Principal::Extension(extension_x.clone())); - let capability = match request.action.as_ref() { - Action::Dispatch { capability, .. } => capability.clone(), - _ => panic!("test request should be dispatch"), - }; + let capability = dispatch_capability(&request); let gate_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); - drive_always_allow(Arc::clone(&store), request, gate_scope, idempotency).await; + drive_always_allow(Arc::clone(&store), request, gate_scope, &idempotency).await; // Same scope, same capability, but a different extension grantee. let lookup_scope = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); From 577a1e696804b46195100bd2d288142c405093c9 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 14 Jun 2026 22:07:59 -0700 Subject: [PATCH 4/8] fix(approvals): address approval scope review --- crates/ironclaw_approvals/src/policy.rs | 16 ++++++++-------- ...host_runtime_persistent_approvals_contract.rs | 4 ++-- docs/reborn/contracts/approvals.md | 8 +++++++- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index 3de05ca64b2..c6e7b5160f0 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -919,8 +919,8 @@ mod tests { ); } - #[tokio::test] - async fn policy_scope_prefers_project_over_thread() { + #[test] + fn policy_scope_prefers_project_over_thread() { // Project is still part of the scope; differing thread ids under the same // project produce identical keys (project carried, thread_id always None). let scope_a = scope(Some("project-a"), Some("thread-a")); @@ -993,8 +993,8 @@ mod tests { assert!(reloaded.active_grant().is_some()); } - #[tokio::test] - async fn policy_scope_without_project_is_thread_agnostic() { + #[test] + fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); let scope_b = scope(None, Some("thread-b")); @@ -1005,8 +1005,8 @@ mod tests { assert_eq!(derived_a.thread_id, None); } - #[tokio::test] - async fn policy_scope_without_project_isolates_users() { + #[test] + fn policy_scope_without_project_isolates_users() { let scope_a = ResourceScope { user_id: UserId::new("alice").unwrap(), ..scope(None, Some("thread-a")) @@ -1022,8 +1022,8 @@ mod tests { ); } - #[tokio::test] - async fn policy_scope_without_project_isolates_agents() { + #[test] + fn policy_scope_without_project_isolates_agents() { let scope_a = ResourceScope { agent_id: Some(AgentId::new("agent-x").unwrap()), ..scope(None, Some("thread-a")) diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index 1a99add0123..4844f8b9545 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -478,7 +478,7 @@ async fn default_runtime_skips_expired_persistent_policy() { #[tokio::test] async fn default_runtime_uses_persistent_policy_for_no_project_no_thread_scope() { - // A WebChat-style context (no project, no thread) now yields a valid + // A fully unscoped context (no project, no thread) now yields a valid // (tenant, user, agent) persistent approval scope: the lookup proceeds and a // seeded "always allow" policy authorizes dispatch without a gate. let registry = Arc::new(registry_with_echo_capability()); @@ -507,7 +507,7 @@ async fn default_runtime_uses_persistent_policy_for_no_project_no_thread_scope() secrets: Vec::new(), resource_ceiling: None, expires_at: None, - max_invocations: Some(1), + max_invocations: None, }, source_approval_request_id: None, }) diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index 618d64881bb..349eefb5cb9 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -260,7 +260,13 @@ This slice intentionally keeps approval resolution narrow: `thread_id` never participates in the scope, so an "always allow" granted in one thread applies to all of that user's threads with the same agent (and project, when present) and is channel-agnostic — a WebUI grant is honored for - a Slack message resolving to the same `(user, agent)` + a Slack message resolving to the same `(user, agent)`. Migration note: + legacy no-project policies persisted with thread-scoped keys/paths before + this change are not discovered by the new thread-agnostic lookup and remain + orphaned until cleanup tooling runs; recommended remediation is a revoke + sweep to delete legacy thread-scoped records or a migration/rekey that drops + `thread_id` into the new tenant/user/agent/project scope where possible, + run after deploy during rollout - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval From e4d145dc5c13190ff597f99b608dcd6ae7c3307a Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 14 Jun 2026 22:45:15 -0700 Subject: [PATCH 5/8] fix(approvals): preserve legacy approval lookup --- crates/ironclaw_approvals/src/policy.rs | 155 +++++++++++++++++- .../ironclaw_host_runtime/src/production.rs | 4 +- ...t_runtime_persistent_approvals_contract.rs | 146 ++++++++++++++++- .../tests/approval_interaction_contract.rs | 71 ++++++++ docs/reborn/contracts/approvals.md | 9 +- 5 files changed, 374 insertions(+), 11 deletions(-) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index c6e7b5160f0..a572d891322 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -202,6 +202,17 @@ pub trait PersistentApprovalPolicyStore: Send + Sync { key: &PersistentApprovalPolicyKey, ) -> Result, PersistentApprovalPolicyError>; + /// Scope-aware lookup for compatibility with legacy filesystem policies + /// that were keyed by a concrete thread id before persistent approvals + /// became threadless. + async fn lookup_with_scope( + &self, + _scope: &ResourceScope, + key: &PersistentApprovalPolicyKey, + ) -> Result, PersistentApprovalPolicyError> { + self.lookup(key).await + } + async fn revoke( &self, key: &PersistentApprovalPolicyKey, @@ -407,6 +418,31 @@ where .map(|(policy, _version)| policy)) } + async fn lookup_with_scope( + &self, + scope: &ResourceScope, + key: &PersistentApprovalPolicyKey, + ) -> Result, PersistentApprovalPolicyError> { + if let Some(policy) = self + .lookup_versioned(key) + .await? + .map(|(policy, _version)| policy) + { + return Ok(Some(policy)); + } + + let Some(legacy_key) = legacy_thread_scoped_key(scope, key) else { + return Ok(None); + }; + let legacy_path = legacy_policy_path(&legacy_key)?; + let legacy_scope = resource_scope_for_policy_key(&legacy_key); + + Ok(self + .lookup_versioned_at(&legacy_key, &legacy_scope, &legacy_path) + .await? + .map(|(policy, _version)| policy)) + } + async fn revoke( &self, key: &PersistentApprovalPolicyKey, @@ -478,7 +514,17 @@ where { let path = self.cached_policy_path(key)?; let scope = resource_scope_for_policy_key(key); - let Some(versioned) = self.filesystem.get(&scope, &path).await? else { + self.lookup_versioned_at(key, &scope, &path).await + } + + async fn lookup_versioned_at( + &self, + key: &PersistentApprovalPolicyKey, + scope: &ResourceScope, + path: &ScopedPath, + ) -> Result, PersistentApprovalPolicyError> + { + let Some(versioned) = self.filesystem.get(scope, path).await? else { return Ok(None); }; deserialize_versioned_policy(key, versioned) @@ -588,6 +634,19 @@ fn policy_path( .map_err(invalid_path) } +fn legacy_policy_path( + key: &PersistentApprovalPolicyKey, +) -> Result { + ScopedPath::new(format!( + "{}/{}/{}/{}.json", + POLICY_PREFIX, + legacy_within_tenant_scope(&key.scope), + key.action.as_path_segment(), + policy_digest(key)? + )) + .map_err(invalid_path) +} + fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { let mut segments = Vec::new(); if let Some(agent_id) = &scope.agent_id { @@ -605,6 +664,23 @@ fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { } } +fn legacy_within_tenant_scope(scope: &PersistentApprovalScope) -> String { + let mut segments = Vec::new(); + if let Some(agent_id) = &scope.agent_id { + segments.push(format!("agents/{agent_id}")); + } + if let Some(project_id) = &scope.project_id { + segments.push(format!("projects/{project_id}")); + } else if let Some(thread_id) = &scope.thread_id { + segments.push(format!("threads/{thread_id}")); + } + if segments.is_empty() { + "scope".to_string() + } else { + segments.join("/") + } +} + fn policy_digest( key: &PersistentApprovalPolicyKey, ) -> Result { @@ -629,6 +705,25 @@ fn resource_scope_for_policy_key(key: &PersistentApprovalPolicyKey) -> ResourceS } } +fn legacy_thread_scoped_key( + scope: &ResourceScope, + key: &PersistentApprovalPolicyKey, +) -> Option { + let thread_id = scope.thread_id.clone()?; + if key.scope.project_id.is_some() { + return None; + } + + let mut legacy_scope = key.scope.clone(); + legacy_scope.thread_id = Some(thread_id); + Some(PersistentApprovalPolicyKey { + scope: legacy_scope, + action: key.action, + capability_id: key.capability_id.clone(), + grantee: key.grantee.clone(), + }) +} + fn serialize(value: &T) -> Result, PersistentApprovalPolicyError> where T: Serialize, @@ -982,9 +1077,10 @@ mod tests { .await .expect("allow project-scoped policy"); + let lookup_scope = scope(Some("project-a"), Some("thread-2")); let new_thread_key = key_for(&scope(Some("project-a"), Some("thread-2"))); let reloaded = FilesystemPersistentApprovalPolicyStore::new(scoped) - .lookup(&new_thread_key) + .lookup_with_scope(&lookup_scope, &new_thread_key) .await .expect("lookup") .expect("pre-existing project-scoped policy still matches in a new thread"); @@ -993,6 +1089,61 @@ mod tests { assert!(reloaded.active_grant().is_some()); } + #[tokio::test] + async fn filesystem_no_project_legacy_thread_scoped_policy_matches_with_scope_lookup() { + // Criterion 5 (#4825): no-project persistent approvals that were stored + // under a concrete thread id before the scope became threadless still + // need to be found after the upgrade. The new scope-aware lookup should + // try the canonical key first, then fall back to the legacy thread-keyed + // filesystem record when the caller still has a thread id. + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); + let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); + let legacy_scope = scope(None, Some("thread-legacy")); + let canonical_key = key_for(&legacy_scope); + let legacy_key = PersistentApprovalPolicyKey { + scope: PersistentApprovalScope { + thread_id: legacy_scope.thread_id.clone(), + ..PersistentApprovalScope::from_resource_scope(&legacy_scope) + }, + action: canonical_key.action, + capability_id: canonical_key.capability_id.clone(), + grantee: canonical_key.grantee.clone(), + }; + let policy = PersistentApprovalPolicy { + key: legacy_key.clone(), + grant_id: CapabilityGrantId::new(), + approved_by: Principal::User(UserId::new("alice").unwrap()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: Some(ApprovalRequestId::new()), + created_at: Utc::now(), + updated_at: Utc::now(), + revoked_at: None, + }; + let legacy_path = legacy_policy_path(&legacy_key).expect("legacy path"); + store + .write_policy_raw(&legacy_scope, &legacy_path, &policy, CasExpectation::Absent) + .await + .expect("write legacy policy"); + + let reloaded = store + .lookup_with_scope(&legacy_scope, &canonical_key) + .await + .expect("lookup legacy policy") + .expect("legacy thread-scoped policy still reachable"); + + assert_eq!(reloaded, policy); + assert!(reloaded.active_grant().is_some()); + } + #[test] fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); diff --git a/crates/ironclaw_host_runtime/src/production.rs b/crates/ironclaw_host_runtime/src/production.rs index ae31048d7a3..d3ca5c431fa 100644 --- a/crates/ironclaw_host_runtime/src/production.rs +++ b/crates/ironclaw_host_runtime/src/production.rs @@ -987,16 +987,18 @@ impl DefaultHostRuntime { return; } let scope = PersistentApprovalScope::from_resource_scope(&context.resource_scope); + let lookup_scope = context.resource_scope.clone(); let lookup_results = join_all(persistent_approval_grantees(context).into_iter().map( |grantee| { let policies = Arc::clone(policies); + let lookup_scope = lookup_scope.clone(); let key = PersistentApprovalPolicyKey { scope: scope.clone(), action, capability_id: capability_id.clone(), grantee, }; - async move { policies.lookup(&key).await } + async move { policies.lookup_with_scope(&lookup_scope, &key).await } }, )) .await; diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index 4844f8b9545..ca22df36ad9 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -7,12 +7,13 @@ use std::sync::{Arc, Mutex}; use async_trait::async_trait; use chrono::Utc; use ironclaw_approvals::{ - InMemoryPersistentApprovalPolicyStore, PersistentApprovalAction, PersistentApprovalPolicy, - PersistentApprovalPolicyError, PersistentApprovalPolicyInput, PersistentApprovalPolicyKey, - PersistentApprovalPolicyStore, + FilesystemPersistentApprovalPolicyStore, InMemoryPersistentApprovalPolicyStore, + PersistentApprovalAction, PersistentApprovalPolicy, PersistentApprovalPolicyError, + PersistentApprovalPolicyInput, PersistentApprovalPolicyKey, PersistentApprovalPolicyStore, }; use ironclaw_authorization::{GrantAuthorizer, TrustAwareCapabilityDispatchAuthorizer}; use ironclaw_extensions::{ExtensionManifest, ExtensionPackage, ExtensionRegistry, ManifestSource}; +use ironclaw_filesystem::{CasExpectation, ContentType, Entry, InMemoryBackend, ScopedFilesystem}; use ironclaw_host_api::*; use ironclaw_host_runtime::{ CapabilitySurfaceVersion, DefaultHostRuntime, HostRuntime, RuntimeCapabilityRequest, @@ -156,6 +157,103 @@ async fn default_runtime_uses_user_grantee_persistent_policy_as_dispatch_authori assert!(dispatcher.has_request()); } +#[tokio::test] +async fn default_runtime_uses_legacy_thread_scoped_policy_as_dispatch_authority() { + let registry = Arc::new(registry_with_echo_capability()); + let dispatcher = Arc::new(RecordingDispatcher::default()); + let authorizer: Arc = Arc::new(GrantAuthorizer); + let run_state = Arc::new(InMemoryRunStateStore::new()); + let approval_requests = Arc::new(InMemoryApprovalRequestStore::new()); + let scoped = scoped_approval_fs(); + let policies = Arc::new(FilesystemPersistentApprovalPolicyStore::new(Arc::clone( + &scoped, + ))); + let mut context = execution_context_without_grants(); + let legacy_thread = ThreadId::new("thread-legacy").unwrap(); + context.project_id = None; + context.thread_id = Some(legacy_thread.clone()); + context.resource_scope.project_id = None; + context.resource_scope.thread_id = Some(legacy_thread); + + let canonical_key = PersistentApprovalPolicyKey::new( + &context.resource_scope, + PersistentApprovalAction::Dispatch, + capability_id(), + Principal::Extension(context.extension_id.clone()), + ); + let legacy_key = PersistentApprovalPolicyKey { + scope: { + let mut legacy_scope = canonical_key.scope.clone(); + legacy_scope.thread_id = context.resource_scope.thread_id.clone(); + legacy_scope + }, + action: canonical_key.action, + capability_id: canonical_key.capability_id.clone(), + grantee: canonical_key.grantee.clone(), + }; + let policy = PersistentApprovalPolicy { + key: legacy_key.clone(), + grant_id: CapabilityGrantId::new(), + approved_by: Principal::User(context.user_id.clone()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: None, + created_at: Utc::now(), + updated_at: Utc::now(), + revoked_at: None, + }; + scoped + .put( + &context.resource_scope, + &legacy_policy_path_for_test(&legacy_key), + Entry::bytes(serde_json::to_vec_pretty(&policy).expect("serialize policy")) + .with_content_type(ContentType::json()), + CasExpectation::Absent, + ) + .await + .expect("seed legacy policy"); + let policy_store: Arc = policies; + + let runtime = DefaultHostRuntime::new( + registry, + dispatcher.clone(), + authorizer, + CapabilitySurfaceVersion::new("surface-v1").unwrap(), + local_test_runtime_policy(), + ) + .with_trust_policy(Arc::new(local_manifest_trust_policy())) + .with_run_state(run_state) + .with_approval_requests(approval_requests) + .with_persistent_approval_policies(policy_store); + + let outcome = runtime + .invoke_capability(RuntimeCapabilityRequest::new( + context, + capability_id(), + ResourceEstimate::default(), + json!({"message": "hello"}), + trust_decision_with_dispatch_authority(), + )) + .await + .unwrap(); + + match outcome { + ironclaw_host_runtime::RuntimeCapabilityOutcome::Completed(completed) => { + assert_eq!(completed.capability_id, capability_id()); + assert_eq!(completed.output, json!({"ok": true})); + } + other => panic!("expected Completed outcome, got {:?}", other), + } + assert!(dispatcher.has_request()); +} + #[tokio::test] async fn default_runtime_does_not_replay_tenant_grantee_persistent_policy() { let registry = Arc::new(registry_with_echo_capability()); @@ -784,6 +882,48 @@ fn execution_context_without_grants() -> ExecutionContext { .unwrap() } +fn scoped_approval_fs() -> Arc> { + let mounts = MountView::new(vec![MountGrant::new( + MountAlias::new("/approvals").unwrap(), + VirtualPath::new("/approvals").unwrap(), + MountPermissions { + read: true, + write: true, + delete: false, + list: true, + execute: false, + }, + )]) + .expect("approval mount"); + Arc::new(ScopedFilesystem::with_fixed_view( + Arc::new(InMemoryBackend::new()), + mounts, + )) +} + +fn legacy_policy_path_for_test(key: &PersistentApprovalPolicyKey) -> ScopedPath { + let mut segments = Vec::new(); + if let Some(agent_id) = &key.scope.agent_id { + segments.push(format!("agents/{agent_id}")); + } + if let Some(project_id) = &key.scope.project_id { + segments.push(format!("projects/{project_id}")); + } else if let Some(thread_id) = &key.scope.thread_id { + segments.push(format!("threads/{thread_id}")); + } + let scope_path = if segments.is_empty() { + "scope".to_string() + } else { + segments.join("/") + }; + let digest = sha256_digest_token(&serde_json::to_vec(key).expect("serialize key")); + let digest = digest.strip_prefix("sha256:").unwrap_or(digest.as_str()); + ScopedPath::new(format!( + "/approvals/persistent/{scope_path}/dispatch/{digest}.json" + )) + .expect("legacy policy path") +} + fn local_manifest_trust_policy() -> HostTrustPolicy { local_manifest_trust_policy_with_effects(vec![EffectKind::DispatchCapability]) } diff --git a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs index 708b6130eb5..4268cf9077e 100644 --- a/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs +++ b/crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs @@ -874,6 +874,47 @@ async fn drive_always_allow( assert_eq!(coordinator.resumption_count(), 1); } +async fn drive_spawn_always_allow( + store: Arc, + request: ApprovalRequest, + gate_scope: ResourceScope, + idempotency: &str, +) { + let request_actor = TurnActor::new(gate_scope.user_id.clone()); + let request_scope = TurnScope::new( + gate_scope.tenant_id.clone(), + gate_scope.agent_id.clone(), + gate_scope.project_id.clone(), + gate_scope + .thread_id + .clone() + .expect("gate scope must carry a thread id"), + ); + let (service, resolver, coordinator, run_id, gate_ref) = + service_fixture_with_scope(request, gate_scope); + let service = service.with_persistent_policy_store(store); + + let response = service + .resolve(ResolveApprovalInteractionRequest { + scope: request_scope, + actor: request_actor, + run_id_hint: Some(run_id), + gate_ref, + decision: ApprovalInteractionDecision::AlwaysAllow, + idempotency_key: IdempotencyKey::new(idempotency).expect("idempotency"), + }) + .await + .expect("always allow"); + + assert!(matches!( + response, + ResolveApprovalInteractionResponse::Approved(_) + )); + assert_eq!(resolver.approval_count(), 0); + assert_eq!(resolver.spawn_approval_count(), 1); + assert_eq!(coordinator.resumption_count(), 1); +} + /// Acceptance criterion 1: an "always allow" granted while resolving a gate in /// thread 1 (no project) is reused for the same capability in thread 2 without a /// gate. Covered against both InMemory and Filesystem stores because the @@ -904,6 +945,36 @@ async fn always_allow_grants_reuse_in_new_thread_without_project() { } } +/// Acceptance criterion 2: a spawn-capability "always allow" is persisted as a +/// reusable policy and can be matched again from a later thread with the same +/// user/agent/project scope. +#[tokio::test] +async fn always_allow_spawn_grants_reuse_in_new_thread_without_project() { + for (store, idempotency) in caller_level_store_pair("spawn-reuse") { + let request = spawn_approval_request("start the worker"); + let capability = match request.action.as_ref() { + Action::SpawnCapability { capability, .. } => capability.clone(), + _ => panic!("test request should be spawn"), + }; + let thread_one = no_project_scope("user-alpha", Some("agent-a"), "thread-1"); + drive_spawn_always_allow(Arc::clone(&store), request, thread_one, &idempotency).await; + + let thread_two = no_project_scope("user-alpha", Some("agent-a"), "thread-2"); + let key = PersistentApprovalPolicyKey::new( + &thread_two, + PersistentApprovalAction::SpawnCapability, + capability, + Principal::User(UserId::new("user-alpha").expect("user")), + ); + let policy = store + .lookup(&key) + .await + .expect("persistent policy lookup") + .expect("persistent policy active in new thread"); + assert!(policy.active_grant().is_some()); + } +} + /// Acceptance criterion 4 (isolation): an "always allow" granted by user A in /// thread 1 must NOT authorize user B in thread 2 under the same tenant/agent. #[tokio::test] diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index 349eefb5cb9..ac6fffdcc04 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -262,11 +262,10 @@ This slice intentionally keeps approval resolution narrow: project, when present) and is channel-agnostic — a WebUI grant is honored for a Slack message resolving to the same `(user, agent)`. Migration note: legacy no-project policies persisted with thread-scoped keys/paths before - this change are not discovered by the new thread-agnostic lookup and remain - orphaned until cleanup tooling runs; recommended remediation is a revoke - sweep to delete legacy thread-scoped records or a migration/rekey that drops - `thread_id` into the new tenant/user/agent/project scope where possible, - run after deploy during rollout + this change are read through a compatibility fallback when the caller still + has the original thread id; recommended cleanup remains a revoke sweep to + delete legacy thread-scoped records or a migration/rekey that drops + `thread_id` into the new tenant/user/agent/project scope where possible - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval From 0d5b62c9ecce09589054f15a3807988bef300115 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 14 Jun 2026 23:15:47 -0700 Subject: [PATCH 6/8] fix(approvals): find legacy grants across threads --- crates/ironclaw_approvals/src/policy.rs | 200 +++++++++++++++++- ...t_runtime_persistent_approvals_contract.rs | 15 +- docs/reborn/contracts/approvals.md | 9 +- 3 files changed, 212 insertions(+), 12 deletions(-) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index a572d891322..8918f002716 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -20,6 +20,9 @@ use thiserror::Error; const POLICY_PREFIX: &str = "/approvals/persistent"; const POLICY_PATH_CACHE_MAX_ENTRIES: usize = 1024; const POLICY_CAS_RETRY_ATTEMPTS: usize = 3; +const LEGACY_POLICY_SCAN_MAX_THREADS: usize = 1024; +const LEGACY_POLICY_SCAN_MAX_ACTION_DIRS: usize = 8; +const LEGACY_POLICY_SCAN_MAX_POLICIES_PER_ACTION: usize = 1024; const PERSISTENT_APPROVAL_GRANT_ISSUER: &str = "persistent-approval"; /// Returns whether an extension manifest permission mode may be upgraded by an @@ -437,10 +440,24 @@ where let legacy_path = legacy_policy_path(&legacy_key)?; let legacy_scope = resource_scope_for_policy_key(&legacy_key); - Ok(self + if let Some(policy) = self .lookup_versioned_at(&legacy_key, &legacy_scope, &legacy_path) .await? - .map(|(policy, _version)| policy)) + .map(|(policy, _version)| policy) + { + return Ok(Some(policy)); + } + + if scope.project_id.is_none() + && scope.thread_id.is_some() + && let Some((policy, _version)) = self + .lookup_legacy_no_project_thread_scoped(scope, key) + .await? + { + return Ok(Some(policy)); + } + + Ok(None) } async fn revoke( @@ -530,6 +547,82 @@ where deserialize_versioned_policy(key, versioned) } + async fn lookup_legacy_no_project_thread_scoped( + &self, + scope: &ResourceScope, + key: &PersistentApprovalPolicyKey, + ) -> Result, PersistentApprovalPolicyError> + { + let Some(root) = legacy_thread_root_path(&key.scope)? else { + return Ok(None); + }; + + let thread_dirs = match self + .filesystem + .list_dir_bounded(scope, &root, LEGACY_POLICY_SCAN_MAX_THREADS) + .await + { + Ok(entries) => entries, + Err(FilesystemError::NotFound { .. }) => return Ok(None), + Err(error) => return Err(error.into()), + }; + + for thread_dir in thread_dirs + .into_iter() + .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::Directory) + { + let action_root = scoped_child_path(&root, &thread_dir.name)?; + let action_dirs = match self + .filesystem + .list_dir_bounded(scope, &action_root, LEGACY_POLICY_SCAN_MAX_ACTION_DIRS) + .await + { + Ok(entries) => entries, + Err(FilesystemError::NotFound { .. }) => continue, + Err(error) => return Err(error.into()), + }; + + for action_dir in action_dirs + .into_iter() + .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::Directory) + { + if action_dir.name != key.action.as_path_segment() { + continue; + } + + let policy_dir = scoped_child_path(&action_root, &action_dir.name)?; + let policy_entries = match self + .filesystem + .list_dir_bounded( + scope, + &policy_dir, + LEGACY_POLICY_SCAN_MAX_POLICIES_PER_ACTION, + ) + .await + { + Ok(entries) => entries, + Err(FilesystemError::NotFound { .. }) => continue, + Err(error) => return Err(error.into()), + }; + + for policy_entry in policy_entries + .into_iter() + .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::File) + { + let policy_path = scoped_child_path(&policy_dir, &policy_entry.name)?; + let Some(versioned) = self.filesystem.get(scope, &policy_path).await? else { + continue; + }; + if let Some(policy) = deserialize_legacy_versioned_policy(key, versioned)? { + return Ok(Some(policy)); + } + } + } + } + + Ok(None) + } + fn mutation_lock(&self, key: &PersistentApprovalPolicyKey) -> Arc> { let mut locks = self .mutation_locks @@ -621,6 +714,18 @@ fn deserialize_versioned_policy( } } +fn deserialize_legacy_versioned_policy( + key: &PersistentApprovalPolicyKey, + versioned: VersionedEntry, +) -> Result, PersistentApprovalPolicyError> { + let policy = deserialize::(&versioned.entry.body)?; + if legacy_policy_matches_lookup(&policy.key, key) { + Ok(Some((policy, versioned.version))) + } else { + Ok(None) + } +} + fn policy_path( key: &PersistentApprovalPolicyKey, ) -> Result { @@ -724,6 +829,42 @@ fn legacy_thread_scoped_key( }) } +fn legacy_thread_root_path( + scope: &PersistentApprovalScope, +) -> Result, PersistentApprovalPolicyError> { + if scope.project_id.is_some() { + return Ok(None); + } + + let mut path = String::from(POLICY_PREFIX); + if let Some(agent_id) = &scope.agent_id { + path.push_str(&format!("/agents/{agent_id}")); + } + path.push_str("/threads"); + Ok(Some(ScopedPath::new(path).map_err(invalid_path)?)) +} + +fn scoped_child_path( + parent: &ScopedPath, + child: &str, +) -> Result { + ScopedPath::new(format!("{parent}/{child}")).map_err(invalid_path) +} + +fn legacy_policy_matches_lookup( + stored: &PersistentApprovalPolicyKey, + expected: &PersistentApprovalPolicyKey, +) -> bool { + stored.action == expected.action + && stored.capability_id == expected.capability_id + && stored.grantee == expected.grantee + && stored.scope.tenant_id == expected.scope.tenant_id + && stored.scope.user_id == expected.scope.user_id + && stored.scope.agent_id == expected.scope.agent_id + && stored.scope.project_id == expected.scope.project_id + && stored.scope.thread_id.is_some() +} + fn serialize(value: &T) -> Result, PersistentApprovalPolicyError> where T: Serialize, @@ -1144,6 +1285,61 @@ mod tests { assert!(reloaded.active_grant().is_some()); } + #[tokio::test] + async fn filesystem_no_project_legacy_thread_scoped_policy_survives_thread_change() { + // Regression for the unresolved PR review comment on #4835: + // the current thread may differ from the one that originally stored + // the legacy no-project approval, but lookup_with_scope should still + // recover the record by scanning the thread-scoped legacy layout. + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); + let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); + let legacy_scope = scope(None, Some("thread-original")); + let lookup_scope = scope(None, Some("thread-current")); + let canonical_key = key_for(&lookup_scope); + let legacy_key = PersistentApprovalPolicyKey { + scope: PersistentApprovalScope { + thread_id: legacy_scope.thread_id.clone(), + ..PersistentApprovalScope::from_resource_scope(&legacy_scope) + }, + action: canonical_key.action, + capability_id: canonical_key.capability_id.clone(), + grantee: canonical_key.grantee.clone(), + }; + let policy = PersistentApprovalPolicy { + key: legacy_key.clone(), + grant_id: CapabilityGrantId::new(), + approved_by: Principal::User(UserId::new("alice").unwrap()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: Some(ApprovalRequestId::new()), + created_at: Utc::now(), + updated_at: Utc::now(), + revoked_at: None, + }; + let legacy_path = legacy_policy_path(&legacy_key).expect("legacy path"); + store + .write_policy_raw(&legacy_scope, &legacy_path, &policy, CasExpectation::Absent) + .await + .expect("write legacy policy"); + + let reloaded = store + .lookup_with_scope(&lookup_scope, &canonical_key) + .await + .expect("lookup legacy policy") + .expect("legacy thread-scoped policy still reachable after thread change"); + + assert_eq!(reloaded, policy); + assert!(reloaded.active_grant().is_some()); + } + #[test] fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index ca22df36ad9..f0bca661c03 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -158,7 +158,7 @@ async fn default_runtime_uses_user_grantee_persistent_policy_as_dispatch_authori } #[tokio::test] -async fn default_runtime_uses_legacy_thread_scoped_policy_as_dispatch_authority() { +async fn default_runtime_uses_legacy_thread_scoped_policy_after_thread_change() { let registry = Arc::new(registry_with_echo_capability()); let dispatcher = Arc::new(RecordingDispatcher::default()); let authorizer: Arc = Arc::new(GrantAuthorizer); @@ -169,11 +169,14 @@ async fn default_runtime_uses_legacy_thread_scoped_policy_as_dispatch_authority( &scoped, ))); let mut context = execution_context_without_grants(); - let legacy_thread = ThreadId::new("thread-legacy").unwrap(); + let legacy_thread = ThreadId::new("thread-original").unwrap(); + let current_thread = ThreadId::new("thread-current").unwrap(); context.project_id = None; - context.thread_id = Some(legacy_thread.clone()); + context.thread_id = Some(current_thread.clone()); context.resource_scope.project_id = None; - context.resource_scope.thread_id = Some(legacy_thread); + context.resource_scope.thread_id = Some(current_thread); + let mut legacy_resource_scope = context.resource_scope.clone(); + legacy_resource_scope.thread_id = Some(legacy_thread); let canonical_key = PersistentApprovalPolicyKey::new( &context.resource_scope, @@ -184,7 +187,7 @@ async fn default_runtime_uses_legacy_thread_scoped_policy_as_dispatch_authority( let legacy_key = PersistentApprovalPolicyKey { scope: { let mut legacy_scope = canonical_key.scope.clone(); - legacy_scope.thread_id = context.resource_scope.thread_id.clone(); + legacy_scope.thread_id = legacy_resource_scope.thread_id.clone(); legacy_scope }, action: canonical_key.action, @@ -211,7 +214,7 @@ async fn default_runtime_uses_legacy_thread_scoped_policy_as_dispatch_authority( }; scoped .put( - &context.resource_scope, + &legacy_resource_scope, &legacy_policy_path_for_test(&legacy_key), Entry::bytes(serde_json::to_vec_pretty(&policy).expect("serialize policy")) .with_content_type(ContentType::json()), diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index ac6fffdcc04..9c6b0d475be 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -262,10 +262,11 @@ This slice intentionally keeps approval resolution narrow: project, when present) and is channel-agnostic — a WebUI grant is honored for a Slack message resolving to the same `(user, agent)`. Migration note: legacy no-project policies persisted with thread-scoped keys/paths before - this change are read through a compatibility fallback when the caller still - has the original thread id; recommended cleanup remains a revoke sweep to - delete legacy thread-scoped records or a migration/rekey that drops - `thread_id` into the new tenant/user/agent/project scope where possible + this change are read through a bounded compatibility fallback that can match + old records from another thread under the same `(tenant, user, agent)`; + recommended cleanup remains a revoke sweep to delete legacy thread-scoped + records or a migration/rekey that drops `thread_id` into the new + tenant/user/agent/project scope where possible - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval From d4ed1efd269c003bc849888af9d1a5c9d4c7be90 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 15 Jun 2026 09:00:10 -0700 Subject: [PATCH 7/8] fix(approvals): drop legacy approval scope compatibility --- crates/ironclaw_approvals/src/policy.rs | 383 ++---------------------- 1 file changed, 18 insertions(+), 365 deletions(-) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index 8918f002716..d98200b1014 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -12,7 +12,7 @@ use ironclaw_filesystem::{ use ironclaw_host_api::{ Action, ApprovalRequestId, CapabilityGrant, CapabilityGrantId, CapabilityId, GrantConstraints, HostApiError, PermissionMode, Principal, ProjectId, ResourceScope, ScopedPath, SystemServiceId, - TenantId, ThreadId, UserId, sha256_digest_token, + TenantId, UserId, sha256_digest_token, }; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -20,9 +20,6 @@ use thiserror::Error; const POLICY_PREFIX: &str = "/approvals/persistent"; const POLICY_PATH_CACHE_MAX_ENTRIES: usize = 1024; const POLICY_CAS_RETRY_ATTEMPTS: usize = 3; -const LEGACY_POLICY_SCAN_MAX_THREADS: usize = 1024; -const LEGACY_POLICY_SCAN_MAX_ACTION_DIRS: usize = 8; -const LEGACY_POLICY_SCAN_MAX_POLICIES_PER_ACTION: usize = 1024; const PERSISTENT_APPROVAL_GRANT_ISSUER: &str = "persistent-approval"; /// Returns whether an extension manifest permission mode may be upgraded by an @@ -96,14 +93,6 @@ pub struct PersistentApprovalScope { pub user_id: UserId, pub agent_id: Option, pub project_id: Option, - /// Always `None`. The field is retained (rather than removed) purely to - /// preserve the policy digest: the digest is `serde_json::to_vec(key)`, and - /// pre-existing project-scoped policies serialized `"thread_id": null`. - /// Keeping the field means those digests are unchanged so old policies still - /// match. `thread_id` never participates in the persistent approval scope — - /// an "always allow" granted in one thread applies to all of that user's - /// threads with the same agent (and project, when present). - pub thread_id: Option, } impl PersistentApprovalScope { @@ -113,10 +102,6 @@ impl PersistentApprovalScope { user_id: scope.user_id.clone(), agent_id: scope.agent_id.clone(), project_id: scope.project_id.clone(), - // Intentionally always `None`: see the field doc-comment. The scope - // is (tenant_id, user_id, agent_id?, project_id?) and is thread- and - // channel-agnostic. - thread_id: None, } } } @@ -205,9 +190,11 @@ pub trait PersistentApprovalPolicyStore: Send + Sync { key: &PersistentApprovalPolicyKey, ) -> Result, PersistentApprovalPolicyError>; - /// Scope-aware lookup for compatibility with legacy filesystem policies - /// that were keyed by a concrete thread id before persistent approvals - /// became threadless. + /// Scope-aware lookup entry point used by authorization callers. + /// + /// Persistent approvals are keyed by tenant, user, optional agent, and + /// optional project only; the runtime scope is accepted so callers do not + /// need a separate path, but thread ids do not participate. async fn lookup_with_scope( &self, _scope: &ResourceScope, @@ -421,45 +408,6 @@ where .map(|(policy, _version)| policy)) } - async fn lookup_with_scope( - &self, - scope: &ResourceScope, - key: &PersistentApprovalPolicyKey, - ) -> Result, PersistentApprovalPolicyError> { - if let Some(policy) = self - .lookup_versioned(key) - .await? - .map(|(policy, _version)| policy) - { - return Ok(Some(policy)); - } - - let Some(legacy_key) = legacy_thread_scoped_key(scope, key) else { - return Ok(None); - }; - let legacy_path = legacy_policy_path(&legacy_key)?; - let legacy_scope = resource_scope_for_policy_key(&legacy_key); - - if let Some(policy) = self - .lookup_versioned_at(&legacy_key, &legacy_scope, &legacy_path) - .await? - .map(|(policy, _version)| policy) - { - return Ok(Some(policy)); - } - - if scope.project_id.is_none() - && scope.thread_id.is_some() - && let Some((policy, _version)) = self - .lookup_legacy_no_project_thread_scoped(scope, key) - .await? - { - return Ok(Some(policy)); - } - - Ok(None) - } - async fn revoke( &self, key: &PersistentApprovalPolicyKey, @@ -547,82 +495,6 @@ where deserialize_versioned_policy(key, versioned) } - async fn lookup_legacy_no_project_thread_scoped( - &self, - scope: &ResourceScope, - key: &PersistentApprovalPolicyKey, - ) -> Result, PersistentApprovalPolicyError> - { - let Some(root) = legacy_thread_root_path(&key.scope)? else { - return Ok(None); - }; - - let thread_dirs = match self - .filesystem - .list_dir_bounded(scope, &root, LEGACY_POLICY_SCAN_MAX_THREADS) - .await - { - Ok(entries) => entries, - Err(FilesystemError::NotFound { .. }) => return Ok(None), - Err(error) => return Err(error.into()), - }; - - for thread_dir in thread_dirs - .into_iter() - .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::Directory) - { - let action_root = scoped_child_path(&root, &thread_dir.name)?; - let action_dirs = match self - .filesystem - .list_dir_bounded(scope, &action_root, LEGACY_POLICY_SCAN_MAX_ACTION_DIRS) - .await - { - Ok(entries) => entries, - Err(FilesystemError::NotFound { .. }) => continue, - Err(error) => return Err(error.into()), - }; - - for action_dir in action_dirs - .into_iter() - .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::Directory) - { - if action_dir.name != key.action.as_path_segment() { - continue; - } - - let policy_dir = scoped_child_path(&action_root, &action_dir.name)?; - let policy_entries = match self - .filesystem - .list_dir_bounded( - scope, - &policy_dir, - LEGACY_POLICY_SCAN_MAX_POLICIES_PER_ACTION, - ) - .await - { - Ok(entries) => entries, - Err(FilesystemError::NotFound { .. }) => continue, - Err(error) => return Err(error.into()), - }; - - for policy_entry in policy_entries - .into_iter() - .filter(|entry| entry.file_type == ironclaw_filesystem::FileType::File) - { - let policy_path = scoped_child_path(&policy_dir, &policy_entry.name)?; - let Some(versioned) = self.filesystem.get(scope, &policy_path).await? else { - continue; - }; - if let Some(policy) = deserialize_legacy_versioned_policy(key, versioned)? { - return Ok(Some(policy)); - } - } - } - } - - Ok(None) - } - fn mutation_lock(&self, key: &PersistentApprovalPolicyKey) -> Arc> { let mut locks = self .mutation_locks @@ -714,18 +586,6 @@ fn deserialize_versioned_policy( } } -fn deserialize_legacy_versioned_policy( - key: &PersistentApprovalPolicyKey, - versioned: VersionedEntry, -) -> Result, PersistentApprovalPolicyError> { - let policy = deserialize::(&versioned.entry.body)?; - if legacy_policy_matches_lookup(&policy.key, key) { - Ok(Some((policy, versioned.version))) - } else { - Ok(None) - } -} - fn policy_path( key: &PersistentApprovalPolicyKey, ) -> Result { @@ -739,26 +599,11 @@ fn policy_path( .map_err(invalid_path) } -fn legacy_policy_path( - key: &PersistentApprovalPolicyKey, -) -> Result { - ScopedPath::new(format!( - "{}/{}/{}/{}.json", - POLICY_PREFIX, - legacy_within_tenant_scope(&key.scope), - key.action.as_path_segment(), - policy_digest(key)? - )) - .map_err(invalid_path) -} - fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { let mut segments = Vec::new(); if let Some(agent_id) = &scope.agent_id { segments.push(format!("agents/{agent_id}")); } - // `thread_id` is always `None` (see `PersistentApprovalScope`), so the scope - // path only ever branches on `project_id`. if let Some(project_id) = &scope.project_id { segments.push(format!("projects/{project_id}")); } @@ -769,23 +614,6 @@ fn within_tenant_scope(scope: &PersistentApprovalScope) -> String { } } -fn legacy_within_tenant_scope(scope: &PersistentApprovalScope) -> String { - let mut segments = Vec::new(); - if let Some(agent_id) = &scope.agent_id { - segments.push(format!("agents/{agent_id}")); - } - if let Some(project_id) = &scope.project_id { - segments.push(format!("projects/{project_id}")); - } else if let Some(thread_id) = &scope.thread_id { - segments.push(format!("threads/{thread_id}")); - } - if segments.is_empty() { - "scope".to_string() - } else { - segments.join("/") - } -} - fn policy_digest( key: &PersistentApprovalPolicyKey, ) -> Result { @@ -805,66 +633,11 @@ fn resource_scope_for_policy_key(key: &PersistentApprovalPolicyKey) -> ResourceS agent_id: key.scope.agent_id.clone(), project_id: key.scope.project_id.clone(), mission_id: None, - thread_id: key.scope.thread_id.clone(), + thread_id: None, invocation_id: ironclaw_host_api::InvocationId::new(), } } -fn legacy_thread_scoped_key( - scope: &ResourceScope, - key: &PersistentApprovalPolicyKey, -) -> Option { - let thread_id = scope.thread_id.clone()?; - if key.scope.project_id.is_some() { - return None; - } - - let mut legacy_scope = key.scope.clone(); - legacy_scope.thread_id = Some(thread_id); - Some(PersistentApprovalPolicyKey { - scope: legacy_scope, - action: key.action, - capability_id: key.capability_id.clone(), - grantee: key.grantee.clone(), - }) -} - -fn legacy_thread_root_path( - scope: &PersistentApprovalScope, -) -> Result, PersistentApprovalPolicyError> { - if scope.project_id.is_some() { - return Ok(None); - } - - let mut path = String::from(POLICY_PREFIX); - if let Some(agent_id) = &scope.agent_id { - path.push_str(&format!("/agents/{agent_id}")); - } - path.push_str("/threads"); - Ok(Some(ScopedPath::new(path).map_err(invalid_path)?)) -} - -fn scoped_child_path( - parent: &ScopedPath, - child: &str, -) -> Result { - ScopedPath::new(format!("{parent}/{child}")).map_err(invalid_path) -} - -fn legacy_policy_matches_lookup( - stored: &PersistentApprovalPolicyKey, - expected: &PersistentApprovalPolicyKey, -) -> bool { - stored.action == expected.action - && stored.capability_id == expected.capability_id - && stored.grantee == expected.grantee - && stored.scope.tenant_id == expected.scope.tenant_id - && stored.scope.user_id == expected.scope.user_id - && stored.scope.agent_id == expected.scope.agent_id - && stored.scope.project_id == expected.scope.project_id - && stored.scope.thread_id.is_some() -} - fn serialize(value: &T) -> Result, PersistentApprovalPolicyError> where T: Serialize, @@ -894,7 +667,7 @@ mod tests { use ironclaw_filesystem::{InMemoryBackend, LocalFilesystem, ScopedFilesystem}; use ironclaw_host_api::{ AgentId, EffectKind, GrantConstraints, HostPath, InvocationId, MountAlias, MountGrant, - MountPermissions, MountView, NetworkPolicy, ProjectId, VirtualPath, + MountPermissions, MountView, NetworkPolicy, ProjectId, ThreadId, VirtualPath, }; use super::*; @@ -1158,7 +931,7 @@ mod tests { #[test] fn policy_scope_prefers_project_over_thread() { // Project is still part of the scope; differing thread ids under the same - // project produce identical keys (project carried, thread_id always None). + // project produce identical keys. let scope_a = scope(Some("project-a"), Some("thread-a")); let scope_b = scope(Some("project-a"), Some("thread-b")); @@ -1170,28 +943,20 @@ mod tests { derived_a.project_id, Some(ProjectId::new("project-a").unwrap()) ); - assert_eq!(derived_a.thread_id, None); } #[test] - fn project_scoped_policy_key_serialization_stays_digest_stable() { - // Criterion 5 (#4825): the persistent-approval digest is - // `serde_json::to_vec(key)` and the on-disk path derives from it. - // Pre-existing project-scoped policies serialized the scope with - // `"thread_id": null` (an old project-scoped scope already blanked - // thread_id). The field is intentionally retained (always `None`) so the - // serialization — and thus the digest and storage path — is byte-identical - // after the upgrade. If this breaks, pre-existing project-scoped - // "always allow" policies orphan. + fn project_scoped_policy_key_serialization_is_threadless() { + // Criterion 5 (#4825): persistent approvals intentionally ignore + // thread_id. There is no backward-compatibility read path for old local + // test records, so the key serialization should not retain thread_id. let key = key_for(&scope(Some("project-a"), Some("thread-ignored"))); let json = serde_json::to_string(&key).expect("serialize policy key"); assert!( - json.contains("\"thread_id\":null"), - "project-scoped key must serialize thread_id as null to preserve the digest; got {json}" + !json.contains("thread_id"), + "persistent approval keys must not serialize thread_id; got {json}" ); - // Digest and tenant-scope path are independent of the originating thread, - // so a policy written before the upgrade is still located afterwards. let digest_thread_one = policy_digest(&key_for(&scope(Some("project-a"), Some("thread-1")))).expect("digest"); let digest_thread_two = @@ -1205,10 +970,9 @@ mod tests { #[tokio::test] async fn filesystem_project_scoped_policy_matches_in_new_thread_after_reload() { - // Criterion 5 (#4825): a project-scoped "always allow" persisted before - // thread_id was dropped from the scope must still match afterwards. A - // fresh store instance (simulating a restart/upgrade) looks the policy up - // from a DIFFERENT thread under the same project and finds it. + // Criterion 5 (#4825): a project-scoped "always allow" applies across + // threads in the same project. A fresh store instance looks the policy + // up from a different thread and finds it through the canonical path. let backend = Arc::new(InMemoryBackend::new()); let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); @@ -1230,116 +994,6 @@ mod tests { assert!(reloaded.active_grant().is_some()); } - #[tokio::test] - async fn filesystem_no_project_legacy_thread_scoped_policy_matches_with_scope_lookup() { - // Criterion 5 (#4825): no-project persistent approvals that were stored - // under a concrete thread id before the scope became threadless still - // need to be found after the upgrade. The new scope-aware lookup should - // try the canonical key first, then fall back to the legacy thread-keyed - // filesystem record when the caller still has a thread id. - let backend = Arc::new(InMemoryBackend::new()); - let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); - let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); - let legacy_scope = scope(None, Some("thread-legacy")); - let canonical_key = key_for(&legacy_scope); - let legacy_key = PersistentApprovalPolicyKey { - scope: PersistentApprovalScope { - thread_id: legacy_scope.thread_id.clone(), - ..PersistentApprovalScope::from_resource_scope(&legacy_scope) - }, - action: canonical_key.action, - capability_id: canonical_key.capability_id.clone(), - grantee: canonical_key.grantee.clone(), - }; - let policy = PersistentApprovalPolicy { - key: legacy_key.clone(), - grant_id: CapabilityGrantId::new(), - approved_by: Principal::User(UserId::new("alice").unwrap()), - constraints: GrantConstraints { - allowed_effects: vec![EffectKind::DispatchCapability], - mounts: MountView::default(), - network: NetworkPolicy::default(), - secrets: Vec::new(), - resource_ceiling: None, - expires_at: None, - max_invocations: None, - }, - source_approval_request_id: Some(ApprovalRequestId::new()), - created_at: Utc::now(), - updated_at: Utc::now(), - revoked_at: None, - }; - let legacy_path = legacy_policy_path(&legacy_key).expect("legacy path"); - store - .write_policy_raw(&legacy_scope, &legacy_path, &policy, CasExpectation::Absent) - .await - .expect("write legacy policy"); - - let reloaded = store - .lookup_with_scope(&legacy_scope, &canonical_key) - .await - .expect("lookup legacy policy") - .expect("legacy thread-scoped policy still reachable"); - - assert_eq!(reloaded, policy); - assert!(reloaded.active_grant().is_some()); - } - - #[tokio::test] - async fn filesystem_no_project_legacy_thread_scoped_policy_survives_thread_change() { - // Regression for the unresolved PR review comment on #4835: - // the current thread may differ from the one that originally stored - // the legacy no-project approval, but lookup_with_scope should still - // recover the record by scanning the thread-scoped legacy layout. - let backend = Arc::new(InMemoryBackend::new()); - let scoped = scoped_fs(Arc::clone(&backend), "tenant-a", "alice"); - let store = FilesystemPersistentApprovalPolicyStore::new(Arc::clone(&scoped)); - let legacy_scope = scope(None, Some("thread-original")); - let lookup_scope = scope(None, Some("thread-current")); - let canonical_key = key_for(&lookup_scope); - let legacy_key = PersistentApprovalPolicyKey { - scope: PersistentApprovalScope { - thread_id: legacy_scope.thread_id.clone(), - ..PersistentApprovalScope::from_resource_scope(&legacy_scope) - }, - action: canonical_key.action, - capability_id: canonical_key.capability_id.clone(), - grantee: canonical_key.grantee.clone(), - }; - let policy = PersistentApprovalPolicy { - key: legacy_key.clone(), - grant_id: CapabilityGrantId::new(), - approved_by: Principal::User(UserId::new("alice").unwrap()), - constraints: GrantConstraints { - allowed_effects: vec![EffectKind::DispatchCapability], - mounts: MountView::default(), - network: NetworkPolicy::default(), - secrets: Vec::new(), - resource_ceiling: None, - expires_at: None, - max_invocations: None, - }, - source_approval_request_id: Some(ApprovalRequestId::new()), - created_at: Utc::now(), - updated_at: Utc::now(), - revoked_at: None, - }; - let legacy_path = legacy_policy_path(&legacy_key).expect("legacy path"); - store - .write_policy_raw(&legacy_scope, &legacy_path, &policy, CasExpectation::Absent) - .await - .expect("write legacy policy"); - - let reloaded = store - .lookup_with_scope(&lookup_scope, &canonical_key) - .await - .expect("lookup legacy policy") - .expect("legacy thread-scoped policy still reachable after thread change"); - - assert_eq!(reloaded, policy); - assert!(reloaded.active_grant().is_some()); - } - #[test] fn policy_scope_without_project_is_thread_agnostic() { let scope_a = scope(None, Some("thread-a")); @@ -1349,7 +1003,6 @@ mod tests { let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b); assert_eq!(derived_a, derived_b); - assert_eq!(derived_a.thread_id, None); } #[test] From 8cf82dca739c303c237e7b8c08a2b72d60c84914 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 15 Jun 2026 09:14:22 -0700 Subject: [PATCH 8/8] fix(approvals): simplify threadless policy lookup --- crates/ironclaw_approvals/src/policy.rs | 16 +-- .../ironclaw_host_runtime/src/production.rs | 4 +- ...t_runtime_persistent_approvals_contract.rs | 97 +++++-------------- docs/reborn/contracts/approvals.md | 11 +-- 4 files changed, 31 insertions(+), 97 deletions(-) diff --git a/crates/ironclaw_approvals/src/policy.rs b/crates/ironclaw_approvals/src/policy.rs index d98200b1014..7f8e6308db6 100644 --- a/crates/ironclaw_approvals/src/policy.rs +++ b/crates/ironclaw_approvals/src/policy.rs @@ -190,19 +190,6 @@ pub trait PersistentApprovalPolicyStore: Send + Sync { key: &PersistentApprovalPolicyKey, ) -> Result, PersistentApprovalPolicyError>; - /// Scope-aware lookup entry point used by authorization callers. - /// - /// Persistent approvals are keyed by tenant, user, optional agent, and - /// optional project only; the runtime scope is accepted so callers do not - /// need a separate path, but thread ids do not participate. - async fn lookup_with_scope( - &self, - _scope: &ResourceScope, - key: &PersistentApprovalPolicyKey, - ) -> Result, PersistentApprovalPolicyError> { - self.lookup(key).await - } - async fn revoke( &self, key: &PersistentApprovalPolicyKey, @@ -982,10 +969,9 @@ mod tests { .await .expect("allow project-scoped policy"); - let lookup_scope = scope(Some("project-a"), Some("thread-2")); let new_thread_key = key_for(&scope(Some("project-a"), Some("thread-2"))); let reloaded = FilesystemPersistentApprovalPolicyStore::new(scoped) - .lookup_with_scope(&lookup_scope, &new_thread_key) + .lookup(&new_thread_key) .await .expect("lookup") .expect("pre-existing project-scoped policy still matches in a new thread"); diff --git a/crates/ironclaw_host_runtime/src/production.rs b/crates/ironclaw_host_runtime/src/production.rs index d3ca5c431fa..ae31048d7a3 100644 --- a/crates/ironclaw_host_runtime/src/production.rs +++ b/crates/ironclaw_host_runtime/src/production.rs @@ -987,18 +987,16 @@ impl DefaultHostRuntime { return; } let scope = PersistentApprovalScope::from_resource_scope(&context.resource_scope); - let lookup_scope = context.resource_scope.clone(); let lookup_results = join_all(persistent_approval_grantees(context).into_iter().map( |grantee| { let policies = Arc::clone(policies); - let lookup_scope = lookup_scope.clone(); let key = PersistentApprovalPolicyKey { scope: scope.clone(), action, capability_id: capability_id.clone(), grantee, }; - async move { policies.lookup_with_scope(&lookup_scope, &key).await } + async move { policies.lookup(&key).await } }, )) .await; diff --git a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs index f0bca661c03..68823918e21 100644 --- a/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs +++ b/crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs @@ -13,7 +13,7 @@ use ironclaw_approvals::{ }; use ironclaw_authorization::{GrantAuthorizer, TrustAwareCapabilityDispatchAuthorizer}; use ironclaw_extensions::{ExtensionManifest, ExtensionPackage, ExtensionRegistry, ManifestSource}; -use ironclaw_filesystem::{CasExpectation, ContentType, Entry, InMemoryBackend, ScopedFilesystem}; +use ironclaw_filesystem::{InMemoryBackend, ScopedFilesystem}; use ironclaw_host_api::*; use ironclaw_host_runtime::{ CapabilitySurfaceVersion, DefaultHostRuntime, HostRuntime, RuntimeCapabilityRequest, @@ -158,7 +158,7 @@ async fn default_runtime_uses_user_grantee_persistent_policy_as_dispatch_authori } #[tokio::test] -async fn default_runtime_uses_legacy_thread_scoped_policy_after_thread_change() { +async fn default_runtime_uses_threadless_filesystem_policy_after_thread_change() { let registry = Arc::new(registry_with_echo_capability()); let dispatcher = Arc::new(RecordingDispatcher::default()); let authorizer: Arc = Arc::new(GrantAuthorizer); @@ -169,59 +169,35 @@ async fn default_runtime_uses_legacy_thread_scoped_policy_after_thread_change() &scoped, ))); let mut context = execution_context_without_grants(); - let legacy_thread = ThreadId::new("thread-original").unwrap(); + let original_thread = ThreadId::new("thread-original").unwrap(); let current_thread = ThreadId::new("thread-current").unwrap(); context.project_id = None; context.thread_id = Some(current_thread.clone()); context.resource_scope.project_id = None; context.resource_scope.thread_id = Some(current_thread); - let mut legacy_resource_scope = context.resource_scope.clone(); - legacy_resource_scope.thread_id = Some(legacy_thread); - - let canonical_key = PersistentApprovalPolicyKey::new( - &context.resource_scope, - PersistentApprovalAction::Dispatch, - capability_id(), - Principal::Extension(context.extension_id.clone()), - ); - let legacy_key = PersistentApprovalPolicyKey { - scope: { - let mut legacy_scope = canonical_key.scope.clone(); - legacy_scope.thread_id = legacy_resource_scope.thread_id.clone(); - legacy_scope - }, - action: canonical_key.action, - capability_id: canonical_key.capability_id.clone(), - grantee: canonical_key.grantee.clone(), - }; - let policy = PersistentApprovalPolicy { - key: legacy_key.clone(), - grant_id: CapabilityGrantId::new(), - approved_by: Principal::User(context.user_id.clone()), - constraints: GrantConstraints { - allowed_effects: vec![EffectKind::DispatchCapability], - mounts: MountView::default(), - network: NetworkPolicy::default(), - secrets: Vec::new(), - resource_ceiling: None, - expires_at: None, - max_invocations: None, - }, - source_approval_request_id: None, - created_at: Utc::now(), - updated_at: Utc::now(), - revoked_at: None, - }; - scoped - .put( - &legacy_resource_scope, - &legacy_policy_path_for_test(&legacy_key), - Entry::bytes(serde_json::to_vec_pretty(&policy).expect("serialize policy")) - .with_content_type(ContentType::json()), - CasExpectation::Absent, - ) + + let mut original_scope = context.resource_scope.clone(); + original_scope.thread_id = Some(original_thread); + policies + .allow(PersistentApprovalPolicyInput { + scope: original_scope, + action: PersistentApprovalAction::Dispatch, + capability_id: capability_id(), + grantee: Principal::Extension(context.extension_id.clone()), + approved_by: Principal::User(context.user_id.clone()), + constraints: GrantConstraints { + allowed_effects: vec![EffectKind::DispatchCapability], + mounts: MountView::default(), + network: NetworkPolicy::default(), + secrets: Vec::new(), + resource_ceiling: None, + expires_at: None, + max_invocations: None, + }, + source_approval_request_id: None, + }) .await - .expect("seed legacy policy"); + .expect("seed persistent policy"); let policy_store: Arc = policies; let runtime = DefaultHostRuntime::new( @@ -904,29 +880,6 @@ fn scoped_approval_fs() -> Arc> { )) } -fn legacy_policy_path_for_test(key: &PersistentApprovalPolicyKey) -> ScopedPath { - let mut segments = Vec::new(); - if let Some(agent_id) = &key.scope.agent_id { - segments.push(format!("agents/{agent_id}")); - } - if let Some(project_id) = &key.scope.project_id { - segments.push(format!("projects/{project_id}")); - } else if let Some(thread_id) = &key.scope.thread_id { - segments.push(format!("threads/{thread_id}")); - } - let scope_path = if segments.is_empty() { - "scope".to_string() - } else { - segments.join("/") - }; - let digest = sha256_digest_token(&serde_json::to_vec(key).expect("serialize key")); - let digest = digest.strip_prefix("sha256:").unwrap_or(digest.as_str()); - ScopedPath::new(format!( - "/approvals/persistent/{scope_path}/dispatch/{digest}.json" - )) - .expect("legacy policy path") -} - fn local_manifest_trust_policy() -> HostTrustPolicy { local_manifest_trust_policy_with_effects(vec![EffectKind::DispatchCapability]) } diff --git a/docs/reborn/contracts/approvals.md b/docs/reborn/contracts/approvals.md index 9c6b0d475be..fcb981d6ded 100644 --- a/docs/reborn/contracts/approvals.md +++ b/docs/reborn/contracts/approvals.md @@ -260,13 +260,10 @@ This slice intentionally keeps approval resolution narrow: `thread_id` never participates in the scope, so an "always allow" granted in one thread applies to all of that user's threads with the same agent (and project, when present) and is channel-agnostic — a WebUI grant is honored for - a Slack message resolving to the same `(user, agent)`. Migration note: - legacy no-project policies persisted with thread-scoped keys/paths before - this change are read through a bounded compatibility fallback that can match - old records from another thread under the same `(tenant, user, agent)`; - recommended cleanup remains a revoke sweep to delete legacy thread-scoped - records or a migration/rekey that drops `thread_id` into the new - tenant/user/agent/project scope where possible + a Slack message resolving to the same `(user, agent)`. Existing local + thread-scoped approval-policy files from pre-DB testing are intentionally not + migrated or read through a compatibility fallback; wipe local approval state + when moving to this scope shape - persistent approval is fail-closed by manifest policy: the current default only allows durable reuse for capabilities whose manifest `default_permission` is `allow`; `ask` and `deny` remain one-shot approval