fix(approvals): persist "always allow" across threads — drop thread_id from persistent approval scope (#4825)

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;
@@ -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,16 @@ pub struct PersistentApprovalScope {
     pub user_id: UserId,
     pub agent_id: Option<ironclaw_host_api::AgentId>,
     pub project_id: Option<ProjectId>,
-    pub thread_id: Option<ThreadId>,
 }
 
 impl PersistentApprovalScope {
-    pub fn from_resource_scope(
-        scope: &ResourceScope,
-    ) -> Result<Self, PersistentApprovalPolicyError> {
-        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,
-        })
+        }
     }
 }
 
@@ -134,13 +120,13 @@ impl PersistentApprovalPolicyKey {
         action: PersistentApprovalAction,
         capability_id: CapabilityId,
         grantee: Principal,
-    ) -> Result<Self, PersistentApprovalPolicyError> {
-        Ok(Self {
-            scope: PersistentApprovalScope::from_resource_scope(scope)?,
+    ) -> Self {
+        Self {
+            scope: PersistentApprovalScope::from_resource_scope(scope),
             action,
             capability_id,
             grantee,
-        })
+        }
     }
 }
 
@@ -243,7 +229,7 @@ impl PersistentApprovalPolicyStore for InMemoryPersistentApprovalPolicyStore {
             input.action,
             input.capability_id,
             input.grantee,
-        )?;
+        );
         let mut policies = self
             .policies
             .write()
@@ -363,7 +349,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;
@@ -480,7 +466,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<Option<(PersistentApprovalPolicy, RecordVersion)>, PersistentApprovalPolicyError>
+    {
+        let Some(versioned) = self.filesystem.get(scope, path).await? else {
             return Ok(None);
         };
         deserialize_versioned_policy(key, versioned)
@@ -597,8 +593,6 @@ fn within_tenant_scope(scope: &PersistentApprovalScope) -> String {
     }
     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()
@@ -626,7 +620,7 @@ 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(),
     }
 }
@@ -660,7 +654,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::*;
@@ -921,25 +915,113 @@ 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.
         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!(
+            derived_a.project_id,
+            Some(ProjectId::new("project-a").unwrap())
+        );
+    }
+
+    #[test]
+    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"),
+            "persistent approval keys must not serialize thread_id; got {json}"
+        );
+
+        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!(
-            PersistentApprovalScope::from_resource_scope(&scope_a).unwrap(),
-            PersistentApprovalScope::from_resource_scope(&scope_b).unwrap()
+            within_tenant_scope(&key.scope),
+            "agents/agent-a/projects/project-a"
         );
     }
 
     #[tokio::test]
-    async fn policy_scope_uses_thread_without_project() {
+    async fn filesystem_project_scoped_policy_matches_in_new_thread_after_reload() {
+        // 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));
+
+        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());
+    }
+
+    #[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"));
 
+        let derived_a = PersistentApprovalScope::from_resource_scope(&scope_a);
+        let derived_b = PersistentApprovalScope::from_resource_scope(&scope_b);
+
+        assert_eq!(derived_a, derived_b);
+    }
+
+    #[test]
+    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)
+        );
+    }
+
+    #[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"))
+        };
+        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 +1069,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 +1096,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 {

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
 }