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

[zetyquickly]

Fixes #4825.

Summary

thread_id no longer participates in the persistent approval scope. The scope key is now (tenant_id, user_id, agent_id?, project_id?), 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.

Root cause: PersistentApprovalScope::from_resource_scope kept thread_id in the key whenever project_id was absent. WebChat threads carry project_id = None, so every new thread missed the lookup and re-prompted.

Changes

• PersistentApprovalScope now contains only tenant/user plus optional agent/project; it does not serialize or store thread_id.
• lookup_with_scope now uses the canonical threadless key only.
• Removed legacy thread-scoped compatibility lookup/scanning. There is no production persistent DB/storage yet, and local test approval state can be wiped instead of migrated.
• Removed the UnsupportedScope error variant and now-dead handling at the call sites.
• Removed the threads/<thread_id> persistent-policy path branch and the Principal::Thread grantee lookup arm.
• Updated docs/reborn/contracts/approvals.md durable approval scope semantics.

Acceptance criteria

#	Criterion	Status
1	WebChat: "always allow" in thread 1 -> same capability in thread 2 dispatches with no gate	Done: manually verified in WebUI + caller/unit coverage
2	Approval granted in WebUI honored for a Slack message resolving to the same (user, agent)	Mechanism in place; live verification blocked on Slack connection/routing follow-ups
3	Triggered run with creator = approving user dispatches without a gate	Mechanism in place; live verification blocked by missing user-facing trigger authoring path
4	Different user / agent / requesting extension still gets its own gate	Done: InMemory + Filesystem coverage
5	Old local thread-scoped approval records	Intentionally not supported; wipe local approval state
6	Targeted tests and clippy	Done

Verified locally

End-to-end in Reborn WebUI v2 (ironclaw-reborn serve, local-dev profile) with Google Workspace connected, capability google-drive.list_files:

• Baseline (nearai/main): after choosing "always allow", a new chat re-prompted for approval.
• This branch: after "always allow", a new chat ran google-drive.list_files with no gate.

local-dev-yolo bypasses approval gates entirely, so the gate is only observable under local-dev or stricter profiles.

Tests

• cargo test -p ironclaw_approvals
• cargo clippy -p ironclaw_approvals --tests

Earlier branch validation also covered the product-workflow and host-runtime persistent-approval caller paths.

Summary by CodeRabbit

• New Features

  • Persistent “always allow” approvals now match in a scope-aware, thread-agnostic way for the same tenant/user/agent, with optional project scoping.

• Bug Fixes

  • Durable approval policy matching is more reliable after thread changes and across reloads.
  • “No project, no thread” contexts correctly honor an allow policy and perform dispatch/spawn.

• Documentation

  • Updated durable approval scope semantics to explicitly exclude thread_id, including a migration note for legacy thread-scoped records.

• Tests

  • Expanded contract and filesystem-backed coverage for thread-agnostic behavior and isolation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7c7704da-4809-41da-8abe-2d7faac26ac2

📥 Commits

Reviewing files that changed from the base of the PR and between d4ed1ef and 8cf82dc.

📒 Files selected for processing (4)

• crates/ironclaw_approvals/src/policy.rs
• crates/ironclaw_host_runtime/src/production.rs
• crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs
• docs/reborn/contracts/approvals.md

---

📝 Walkthrough

Walkthrough

Drops thread_id from persistent approval scope, making "always allow" grants reusable across all threads for the same user+agent pair. Scope contract becomes infallible. Policy stores implement lookup via scope; runtime and workflow consume the change. Tests validate digest stability and isolation.

Changes

Persistent Approval Scope Redesign

Layer / File(s)	Summary
Scope contract and key infallibility crates/ironclaw_approvals/src/policy.rs	Remove UnsupportedScope error variant and ThreadId import. Make PersistentApprovalScope::from_resource_scope and PersistentApprovalPolicyKey::new infallible by always setting thread_id: None while retaining the field for digest stability with existing project-scoped policies.
Store implementations—infallible key construction crates/ironclaw_approvals/src/policy.rs	Update in-memory and filesystem store allow methods to call infallible key constructor (remove ? error propagation).
Filesystem path scoping and resource scope derivation crates/ironclaw_approvals/src/policy.rs	Simplify canonical within_tenant_scope to branch only on optional agent_id and project_id, removing the now-unreachable per-thread path segment. Update resource_scope_for_policy_key to always set thread_id: None when deriving filesystem paths from policy keys.
Filesystem lookup refactoring crates/ironclaw_approvals/src/policy.rs	Introduce lookup_versioned_at helper that performs scoped/versioned get using explicit scope and path, preserving deserialization-with-key-mismatch validation. Delegate lookup_versioned to it.
Policy unit tests crates/ironclaw_approvals/src/policy.rs	Assert infallible scope construction. Validate digest and path stability for project-scoped keys across different thread IDs. Add isolation tests verifying different users/agents produce distinct scopes. Remove error-case test for UnsupportedScope. Update test helper key_for for infallible constructor.
Runtime consumption crates/ironclaw_host_runtime/src/production.rs	Remove error-handling match for unsupported scopes in apply_persistent_approval_policy; call infallible from_resource_scope directly. Delete Principal::Thread grantee from persistent_approval_grantees (unreachable lookup).
Host runtime contract tests crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs	Add imports and filesystem infrastructure. New test default_runtime_uses_threadless_filesystem_policy_after_thread_change validates filesystem policy lookup across thread mutations. Replace unsupported-scope test with default_runtime_uses_persistent_policy_for_no_project_no_thread_scope asserting unscoped (user+agent only) policy succeeds. Add scoped_approval_fs() helper.
Product workflow service and infrastructure crates/ironclaw_product_workflow/Cargo.toml, src/approval_interaction/service.rs, tests/approval_interaction_contract.rs	Add ironclaw_filesystem dev dependency. Simplify prepare_allow_policy by removing .map_err() (key construction infallible). Introduce test helpers: no_project_scope, scoped_fs, service_fixture_with_scope, approval_request_by, caller_level_store_pair, dispatch_capability.
Product workflow acceptance tests crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs	Add drive_always_allow and drive_spawn_always_allow helpers. Introduce acceptance tests validating cross-thread grant reuse (same tenant/agent, no project) and isolation (different user, agent, or extension grantee do not inherit). Adjust punctuation at key construction call sites.
Contract documentation docs/reborn/contracts/approvals.md	Redefine scope as (tenant_id, user_id, optional agent_id, optional project_id), explicitly excluding thread_id. Clarify grants are thread-agnostic and channel-agnostic for matching (user, agent) pairs. Add migration guidance: legacy thread-scoped orphaned records require revoke sweep or migration/rekey.

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• [Reborn] Add durable approval-policy port before AlwaysAllow #3891: This PR implements the durable approval-policy port design proposed in issue #3891, specifically by introducing lookup_with_scope and redefining the approval scope as (tenant_id, user_id, optional agent_id, optional project_id), which directly fulfills the required shape and cross-thread reuse criteria.

Poem

Thread shadows fade to None,
Approvals now outlive the conversation,
One grant spans many channels—
user + agent = durable soul. ✨

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title follows Conventional Commits style (type(scope): summary), clearly indicates the fix removes thread_id from persistent approval scope, and includes the issue number.
Description check	✅ Passed	Description covers summary, change type (bug fix), linked issue, validation steps (tests run), and reborn trust-boundary checklist items relevant to scope changes.
Linked Issues check	✅ Passed	Changes directly address #4825: thread_id removed from persistent approval scope derivation, UnsupportedScope error eliminated, from_resource_scope made infallible, scope key now (tenant_id, user_id, agent_id?, project_id?), and both unit and caller-level tests verify cross-thread reuse and isolation.
Out of Scope Changes check	✅ Passed	All changes are scoped to approval persistence: policy.rs scope derivation, host-runtime caller refactor, product-workflow error handling, test coverage expansion, and documentation updates. No drift into unrelated subsystems.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request makes persistent approval policies thread-agnostic by removing thread_id from participating in the active scope evaluation, while retaining the field as None to preserve serialization and digest compatibility for existing project-scoped policies. This simplifies the scope resolution logic and removes the UnsupportedScope error. The feedback points out several newly added test cases in policy.rs that are marked as asynchronous #[tokio::test] but do not perform any async operations, suggesting they be simplified to synchronous #[test] functions to avoid unnecessary runtime overhead and potential compiler warnings.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Copilot]

Pull request overview

This PR fixes persistent “always allow” approvals re-prompting across new threads by removing thread_id from the persistent approval scope key. Persistent approvals now key off (tenant_id, user_id, agent_id?, project_id?), making grants thread-agnostic and effectively channel-agnostic when the same (user, agent, grantee) applies.

Changes:

• Made PersistentApprovalScope::from_resource_scope infallible and always set thread_id: None (field retained to preserve serialized-key digest stability for existing project-scoped policies).
• Removed now-dead “unsupported scope” handling and thread-grantee lookup paths in host runtime / product workflow.
• Added/updated contract and regression tests (including filesystem-backed store coverage) and updated approvals contract docs.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/reborn/contracts/approvals.md	Updates contract text to reflect thread-agnostic persistent approval scope.
crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs	Adds caller-level regression/isolation tests and filesystem-backed store fixture coverage.
crates/ironclaw_product_workflow/src/approval_interaction/service.rs	Removes dead error mapping around persistent policy key derivation (now infallible).
crates/ironclaw_product_workflow/Cargo.toml	Adds ironclaw_filesystem as a dev-dependency for new tests.
crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs	Updates runtime contract test to validate lookup proceeds with no project/thread and can authorize dispatch.
crates/ironclaw_host_runtime/src/production.rs	Makes persistent approval scope derivation unconditional and removes Principal::Thread from lookup grantees.
crates/ironclaw_approvals/src/policy.rs	Drops UnsupportedScope, makes scope derivation thread-agnostic, updates filesystem path derivation and adds migration-stability tests.
Cargo.lock	Lockfile update for new dev-dependency resolution.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/reborn/contracts/approvals.md`:
- Around line 259-263: The docs must call out migration behavior when removing
thread_id from persistent scope: add a paragraph to
docs/reborn/contracts/approvals.md stating that existing persisted policies
keyed with thread_id (created before the change in
crates/ironclaw_approvals/src/policy.rs) will no longer be discovered by the new
lookups and are effectively orphaned; state expected remediation options (run a
revoke-sweep to delete legacy thread-scoped records, or a migration that rekeys
old records by dropping thread_id into the new (tenant_id, user_id, agent_id,
project_id) scope where possible), and include an example migration intent and
recommended timing/rollout note so operators know to run the sweep/migration
after deploy.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a6860aee-0cdb-4171-86ab-56a667ea0b38

📥 Commits

Reviewing files that changed from the base of the PR and between 2a4e017 and 660a721.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

📒 Files selected for processing (7)

• crates/ironclaw_approvals/src/policy.rs
• crates/ironclaw_host_runtime/src/production.rs
• crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs
• crates/ironclaw_product_workflow/Cargo.toml
• crates/ironclaw_product_workflow/src/approval_interaction/service.rs
• crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs
• docs/reborn/contracts/approvals.md

[henrypark133]

Code Review (multi-agent)

Intent: Fix persistent approvals so always-allow grants are reused across threads for the same user, agent, and project scope.

Stats: 1 finding (from 4 raw, 1 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.

Filtered during aggregation: 2 live E2E test-gap findings for Slack/trigger coverage because the PR body explicitly documents those paths as live-verification blocked while covering the mechanism at caller/unit level.

Bugs

1. Medium Existing no-project always-allow policies are orphaned (crates/ironclaw_approvals/src/policy.rs:107-117, confidence 75) — anchor: crates/ironclaw_approvals/src/policy.rs:107
   Existing no-project approvals were persisted with a concrete thread_id in both the serialized policy digest and filesystem path. This change always derives thread_id: None, so after upgrade lookup only checks the new thread-agnostic key/path and misses already persisted WebChat/no-project policies even in the original thread.
   Also flagged by: approach/Medium.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[henrypark133]

Code Review (multi-agent)

Intent: Make persistent always-allow approval grants thread-agnostic while preserving existing project-scoped policy digests.

Stats: 5 findings (from 5 raw, 5 after dedup/filter) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1

Bugs

1. High Legacy thread-scoped approvals become unreachable after the scope change (crates/ironclaw_approvals/src/policy.rs:107-118, confidence 92) — anchor: crates/ironclaw_approvals/src/policy.rs:107
   Before this PR, no-project scopes persisted under threads/<thread_id> and included that concrete thread_id in the key digest. The new canonical key always uses thread_id: null and the scope path, with no legacy lookup/rekey path, so existing WebChat always-allow decisions stop matching after deploy until users approve again.

Tests

1. Medium No caller-level test covers AlwaysAllow on SpawnCapability (crates/ironclaw_product_workflow/src/approval_interaction/service.rs:163-196, confidence 91) — anchor: crates/ironclaw_product_workflow/src/approval_interaction/service.rs:163
   The changed approve_gate path persists always-allow policies for both dispatch and spawn approvals, but the product-workflow tests only exercise SpawnCapability through approve-once. A regression in the persistent spawn path could slip through while dispatch coverage remains green.
2. Medium Channel-agnostic reuse still lacks an end-to-end Slack/WebUI proof (crates/ironclaw_host_runtime/src/production.rs:990-1068, confidence 68) — anchor: crates/ironclaw_host_runtime/src/production.rs:990
   The PR promises a WebUI grant can be honored for another channel resolving to the same user and agent. The current tests prove the lower-level mechanism, but not the actual WebUI-to-Slack consumption path; if that live path remains blocked, this should be tracked explicitly as follow-up verification rather than implied covered.

Maintainability

1. Medium Keep digest compatibility separate from the live scope model (crates/ironclaw_approvals/src/policy.rs:96-116, confidence 63) — anchor: crates/ironclaw_approvals/src/policy.rs:96
PersistentApprovalScope now exposes a permanently-None thread_id field so the digest serialization remains stable. That preserves one compatibility detail, but it also leaves dead state in the live policy model and makes callers remember which fields are semantic versus serialization-only.
2. Low PreparedAllowPolicy only carries a write input plus log context (crates/ironclaw_product_workflow/src/approval_interaction/service.rs:231-286, confidence 78) — anchor: crates/ironclaw_product_workflow/src/approval_interaction/service.rs:79 (no separate inline comment — same validated diff line as the stronger spawn coverage finding)
   PreparedAllowPolicy mostly ferries PersistentApprovalPolicyInput and a derived key into persist_allow_policy. Since the key is only used for warning fields, this wrapper adds another hop without isolating much behavior.

[henrypark133]

Code Review (multi-agent)

Intent: Persist "always allow" approvals across threads by removing thread_id from the persistent approval scope while preserving policy digest compatibility and isolation boundaries.

Stats: 1 finding (from 3 raw, 1 after validation/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.

Bugs

1. Medium Legacy no-project approvals still miss after a thread change (crates/ironclaw_approvals/src/policy.rs:434-443, confidence 84) — anchor: crates/ironclaw_approvals/src/policy.rs:434

   The legacy fallback reconstructs the legacy key from the caller's current scope.thread_id, so an old no-project policy is only found when the new invocation happens to use the same original thread. Existing no-project "always allow" approvals remain thread-bound after upgrade, which undercuts the compatibility goal for users who open a different thread.

Related maintainability/approach reviewers also flagged this area as a permanent dual-read compatibility branch on the shared store API; the concrete blocker is that the branch still cannot find legacy no-project records across threads without a rekey/backfill/index.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs (1)

925-925: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Hardcoded action path segment—fragile if test scope expands.

The helper hardcodes "dispatch" instead of calling key.action.as_path_segment(). If tests later exercise SpawnCapability actions, this helper will generate the wrong path.

♻️ Strengthen the helper

     let digest = digest.strip_prefix("sha256:").unwrap_or(digest.as_str());
     ScopedPath::new(format!(
-        "/approvals/persistent/{scope_path}/dispatch/{digest}.json"
+        "/approvals/persistent/{scope_path}/{}/{digest}.json",
+        key.action.as_path_segment()
     ))
     .expect("legacy policy path")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs`
at line 925, The path construction at line 925 hardcodes "dispatch" as the
action path segment, which will break if tests expand to cover other action
types. Replace the hardcoded "dispatch" string with a dynamic call to
key.action.as_path_segment() to generate the correct path segment based on the
actual action type being tested.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_approvals/src/policy.rs`:
- Around line 451-458: The condition on lines 451-453 in the if statement is too
restrictive by requiring scope.thread_id.is_some() before attempting to look up
legacy no-project policies. Remove the && scope.thread_id.is_some() check from
the condition, keeping only the check for scope.project_id.is_none(), so that
the lookup_legacy_no_project_thread_scoped call can be executed for threadless
callers and make legacy no-project policies accessible regardless of whether
thread_id is present or None. This aligns with the thread-agnostic compatibility
behavior documented in the module spec.

---

Outside diff comments:
In
`@crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs`:
- Line 925: The path construction at line 925 hardcodes "dispatch" as the action
path segment, which will break if tests expand to cover other action types.
Replace the hardcoded "dispatch" string with a dynamic call to
key.action.as_path_segment() to generate the correct path segment based on the
actual action type being tested.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1f5bbcaf-a2b5-40a3-b0b7-86b0f302addc

📥 Commits

Reviewing files that changed from the base of the PR and between e4d145d and 0d5b62c.

📒 Files selected for processing (3)

• crates/ironclaw_approvals/src/policy.rs
• crates/ironclaw_host_runtime/tests/host_runtime_persistent_approvals_contract.rs
• docs/reborn/contracts/approvals.md

[henrypark133]

Code Review (multi-agent)

Intent: Make persistent "always allow" approvals reuse across threads by removing thread_id from the approval scope while preserving policy compatibility.

Stats: 4 findings (from 8 raw reviewer findings, 4 after dedup/filter) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2

Bugs

1. High Threadless current scopes never enter the legacy fallback (crates/ironclaw_approvals/src/policy.rs:437-460, confidence 95) — anchor: crates/ironclaw_approvals/src/policy.rs:437
lookup_with_scope returns before the broader legacy no-project scan when the current scope has no thread_id, so pre-upgrade no-project approvals stored under legacy thread keys disappear for no-thread contexts.

Performance / Security / Maintainability

1. High Legacy approval fallback walks the whole tree on every miss (crates/ironclaw_approvals/src/policy.rs:550-623, confidence 87) — anchor: crates/ironclaw_approvals/src/policy.rs:550
   The compatibility fallback can scan legacy thread/action/policy directories and deserialize candidates on every canonical miss; DefaultHostRuntime may do this once per candidate grantee. Also flagged by security, maintainability, and bugs for availability/design/truncation risk.

Tests

1. Medium Legacy lookup never exercises filesystem error propagation (crates/ironclaw_approvals/src/policy.rs:560-618, confidence 79) — anchor: crates/ironclaw_approvals/src/policy.rs:560 (body only)
   Add filesystem_no_project_legacy_thread_scoped_lookup_propagates_backend_error covering a list_dir_bounded/read failure during legacy scan.
2. Medium No test proves canonical policy beats stale legacy fallback (crates/ironclaw_approvals/src/policy.rs:424-448, confidence 74) — anchor: crates/ironclaw_approvals/src/policy.rs:429 (body only)
   Add filesystem_lookup_prefers_canonical_policy_over_legacy_thread_scoped_record covering the coexistence case where canonical and legacy records both match.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[henrypark133]

Code Review (multi-agent)

Intent: Fix persistent always-allow approvals so they apply across threads by removing thread_id from durable approval scope keys.
Stats: 8 findings (from 8 raw, 8 after dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 2

conventions

1. Medium Scope docs leave manifest-policy contradiction (docs/reborn/contracts/approvals.md:267-270, confidence 75) - anchor: docs/reborn/contracts/AGENTS.md:19
   The new durable-scope text says an always-allow grant is reusable across threads/channels, but the same Current limits list still says durable reuse only applies to manifest default_permission=allow and that ask remains one-shot. That conflicts with the implementation contract in permission_mode_allows_persistent_approval, which treats Ask as eligible, and docs/reborn/contracts are authoritative when they disagree with code.
   Fix: Update the manifest-policy bullet to say allow and ask are eligible for durable reuse, or change the implementation if ask should remain one-shot.

tests

1. Medium No contention test for collapsed threadless filesystem writes (crates/ironclaw_approvals/src/policy.rs:347-357, confidence 75) - anchor: crates/ironclaw_approvals/src/policy.rs:354
   Removing thread_id from PersistentApprovalPolicyKey makes concurrent always-allow writes from different threads for the same user/agent/capability contend on one filesystem policy path and mutation lock. Adjacent tests cover sequential reuse, but none spawn concurrent allow calls that collapse to the same key and verify both succeed without CAS loss.
   Fix: Add a filesystem_policy_store_serializes_concurrent_threadless_allows test that runs two concurrent allow calls from different thread ids for the same user/agent/capability and verifies the surviving policy remains usable.
2. Low Path-cache bound test no longer creates distinct keys (crates/ironclaw_approvals/src/policy.rs:745-765, confidence 100) - anchor: crates/ironclaw_approvals/src/policy.rs:751 (no diff position - body only)
   The test varies only thread_id, but PersistentApprovalScope now ignores thread_id, so all iterations write the same policy key/path. The assertion can pass with a cache size of 1 and no longer exercises eviction beyond POLICY_PATH_CACHE_MAX_ENTRIES.
   Fix: Update filesystem_policy_store_bounds_policy_path_cache to create distinct threadless keys, for example by varying project_id, capability_id, or grantee after thread_id removal.
3. Low Mutation-lock eviction test reuses the same key (crates/ironclaw_approvals/src/policy.rs:810-832, confidence 100) - anchor: crates/ironclaw_approvals/src/policy.rs:817 (no diff position - body only)
   The test writes thread-a and thread-b with the same tenant/user/agent/projectless scope, but thread_id is no longer part of the key. It therefore reacquires the same mutation lock and no longer proves idle lock eviction across distinct policy keys.
   Fix: Update filesystem_policy_store_evicts_idle_mutation_locks to use two distinct non-thread policy keys while still proving idle lock cleanup.

local-patterns

1. Low Project-scope reload test is tagged with a stale criterion number (crates/ironclaw_approvals/src/policy.rs:960-962, confidence 75) - anchor: crates/ironclaw_approvals/src/policy.rs:960
   This test comment calls project-scoped cross-thread lookup criterion 5, while the same branch's criterion 5 is about intentionally not reading old local thread-scoped records. The mismatch makes it harder to navigate which tests cover the migration/non-compatibility decision versus the threadless lookup behavior.
   Fix: Rename the comment to describe project-scoped threadless lookup directly, or point it at the criterion that actually covers cross-thread reuse.
2. Low Thread-grantee comment overstates the invariant (crates/ironclaw_host_runtime/src/production.rs:1550-1554, confidence 75) - anchor: crates/ironclaw_host_runtime/src/production.rs:1550
   The new comment explains dropping Principal::Thread by saying persistent policies are always written from ApprovalRequest.requested_by as User or Extension, but the same helper still looks up Agent, Project, and Mission grantees just above it. That makes the local invariant harder to trust and can mislead the next change to this grantee list.
   Fix: Narrow the comment to the actual reason no Thread lookup is attempted, or remove the User/Extension parenthetical unless those are the only intended durable-policy grantees.
3. Low Spawn test is labeled as the wrong acceptance criterion (crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs:948-950, confidence 75) - anchor: crates/ironclaw_product_workflow/tests/approval_interaction_contract.rs:948
   The comment labels spawn-capability reuse as acceptance criterion 2, but criterion 2 in the PR context is WebUI-to-Slack reuse for the same user/agent. This makes the test coverage trail look like it proves the Slack criterion when it only proves spawn reuse.
   Fix: Drop the numbered criterion reference or relabel this as spawn-specific coverage rather than criterion 2.

maintainability

1. Low Inline the single-use versioned lookup helper (crates/ironclaw_approvals/src/policy.rs:472-482, confidence 75) - anchor: crates/ironclaw_approvals/src/policy.rs:472
   lookup_versioned_at is a new one-call helper that only forwards the already-computed scope/path into filesystem.get and deserialize_versioned_policy. After removing the legacy compatibility lookup, this extra hop no longer hides variation or deletes branching, so readers have to jump between two methods to follow the only filesystem lookup path.
   Fix: Move the filesystem.get(scope, path) and deserialize_versioned_policy body back into lookup_versioned, then delete lookup_versioned_at.

[henrypark133]

Medium - Scope docs leave manifest-policy contradiction.

The new durable-scope text says an always-allow grant is reusable across threads/channels, but the same Current limits list still says durable reuse only applies to manifest default_permission=allow and that ask remains one-shot. That conflicts with the implementation contract in permission_mode_allows_persistent_approval, which treats Ask as eligible, and docs/reborn/contracts are authoritative when they disagree with code.

Fix: Update the manifest-policy bullet to say allow and ask are eligible for durable reuse, or change the implementation if ask should remain one-shot.

[henrypark133]

Medium - No contention test for collapsed threadless filesystem writes.

Removing thread_id from PersistentApprovalPolicyKey makes concurrent always-allow writes from different threads for the same user/agent/capability contend on one filesystem policy path and mutation lock. Adjacent tests cover sequential reuse, but none spawn concurrent allow calls that collapse to the same key and verify both succeed without CAS loss.

Fix: Add a filesystem_policy_store_serializes_concurrent_threadless_allows test that runs two concurrent allow calls from different thread ids for the same user/agent/capability and verifies the surviving policy remains usable.

[henrypark133]

Low - Project-scope reload test is tagged with a stale criterion number.

This test comment calls project-scoped cross-thread lookup criterion 5, while the same branch's criterion 5 is about intentionally not reading old local thread-scoped records. The mismatch makes it harder to navigate which tests cover the migration/non-compatibility decision versus the threadless lookup behavior.

Fix: Rename the comment to describe project-scoped threadless lookup directly, or point it at the criterion that actually covers cross-thread reuse.

[henrypark133]

Low - Thread-grantee comment overstates the invariant.

The new comment explains dropping Principal::Thread by saying persistent policies are always written from ApprovalRequest.requested_by as User or Extension, but the same helper still looks up Agent, Project, and Mission grantees just above it. That makes the local invariant harder to trust and can mislead the next change to this grantee list.

Fix: Narrow the comment to the actual reason no Thread lookup is attempted, or remove the User/Extension parenthetical unless those are the only intended durable-policy grantees.

[henrypark133]

Low - Spawn test is labeled as the wrong acceptance criterion.

The comment labels spawn-capability reuse as acceptance criterion 2, but criterion 2 in the PR context is WebUI-to-Slack reuse for the same user/agent. This makes the test coverage trail look like it proves the Slack criterion when it only proves spawn reuse.

Fix: Drop the numbered criterion reference or relabel this as spawn-specific coverage rather than criterion 2.

[henrypark133]

Low - Inline the single-use versioned lookup helper.

lookup_versioned_at is a new one-call helper that only forwards the already-computed scope/path into filesystem.get and deserialize_versioned_policy. After removing the legacy compatibility lookup, this extra hop no longer hides variation or deletes branching, so readers have to jump between two methods to follow the only filesystem lookup path.

Fix: Move the filesystem.get(scope, path) and deserialize_versioned_policy body back into lookup_versioned, then delete lookup_versioned_at.

[henrypark133]

Code Review (multi-agent)

Intent: Persist "always allow" approvals across threads by removing thread_id from the durable approval scope.
Stats: 3 findings (from 5 raw, 3 after dedup/scope filtering) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0

conventions

1. Medium Scope docs leave manifest-policy contradiction (docs/reborn/contracts/approvals.md:267-270, confidence 75) - anchor: docs/reborn/contracts/AGENTS.md:19
   The new durable-scope text says an always-allow grant is reusable across threads/channels, but the same Current limits list still says durable reuse only applies to manifest default_permission=allow and that ask remains one-shot. That conflicts with permission_mode_allows_persistent_approval, which treats Ask as eligible, and docs/reborn/contracts are authoritative when they disagree with code.
   Fix: Update the manifest-policy bullet to say allow and ask are eligible for durable reuse, or change the implementation if ask should remain one-shot.

tests

1. Medium Thread-grantee lookup removal lacks caller coverage (crates/ironclaw_host_runtime/src/production.rs:1550-1555, confidence 75) - anchor: crates/ironclaw_host_runtime/src/production.rs:1550
   The runtime now explicitly omits Principal::Thread from persistent approval lookup candidates, but the host-runtime contract tests only cover accepted user/extension grantees and rejected tenant grantees. A regression that re-adds thread-grantee replay would not be caught.
   Fix: Add default_runtime_does_not_replay_thread_grantee_persistent_policy covering a manually seeded Principal::Thread policy ignored by invoke_capability.

maintainability

1. Low Delete the one-call lookup_versioned_at helper (crates/ironclaw_approvals/src/policy.rs:472-483, confidence 75) - anchor: crates/ironclaw_approvals/src/policy.rs:469
   After removing the legacy compatibility lookup, lookup_versioned_at has only one caller and just forwards filesystem.get plus deserialize_versioned_policy. Readers now have to follow an extra private method to understand the normal lookup path, but the helper no longer hides any variation.
   Fix: Inline the filesystem get and deserialize_versioned_policy call back into lookup_versioned, then remove lookup_versioned_at.

[henrypark133]

Medium - Scope docs leave manifest-policy contradiction.

The new durable-scope text says an always-allow grant is reusable across threads/channels, but the same Current limits list still says durable reuse only applies to manifest default_permission=allow and that ask remains one-shot. That conflicts with permission_mode_allows_persistent_approval, which treats Ask as eligible, and docs/reborn/contracts are authoritative when they disagree with code.

Fix: Update the manifest-policy bullet to say allow and ask are eligible for durable reuse, or change the implementation if ask should remain one-shot.

[henrypark133]

Medium - Thread-grantee lookup removal lacks caller coverage.

The runtime now explicitly omits Principal::Thread from persistent approval lookup candidates, but the host-runtime contract tests only cover accepted user/extension grantees and rejected tenant grantees. A regression that re-adds thread-grantee replay would not be caught.

Fix: Add default_runtime_does_not_replay_thread_grantee_persistent_policy covering a manually seeded Principal::Thread policy ignored by invoke_capability.

Also flagged by: local-patterns/Low

[henrypark133]

Low - Delete the one-call lookup_versioned_at helper.

After removing the legacy compatibility lookup, lookup_versioned_at has only one caller and just forwards filesystem.get plus deserialize_versioned_policy. Readers now have to follow an extra private method to understand the normal lookup path, but the helper no longer hides any variation.

Fix: Inline the filesystem get and deserialize_versioned_policy call back into lookup_versioned, then remove lookup_versioned_at.