feat(reborn): capability-policy engine — delta store + resolver + identity/config/approval (epic #5261)#5344
feat(reborn): capability-policy engine — delta store + resolver + identity/config/approval (epic #5261)#5344zetyquickly wants to merge 7 commits into
Conversation
Settled Reborn-stack design for admin-shared tools/skills + per-user auth: one account type (User = secrets + tools + memory), Owner/Admin/Member roles, the four-dimension capability policy (availability/configuration/identity/approval) resolving capability default -> tenant -> user, plus a prior-art/compose-with map anchored to #4628 (#4544, #1626, #3289/#4354, #4527) and verified live-code seams. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
First independent slice of the #4628 continuation: the four-dimension policy vocabulary and the pure precedence cascade, with no dependency on #4544. Availability / IdentityMode (user-keyed vs admin-keyed) / config / approval (reusing the existing PermissionMode). resolve_effective_policy() is an order-independent, most-specific-wins fold with deep-merged config. PolicyResolver async port is the seam a #4544-backed adapter fills. Depends only on ironclaw_host_api; clippy -D warnings clean; 8 unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pability defaults A CapabilityDefaultPolicySource trait + in-memory StaticCapabilityDefaultPolicySource supplying the per-capability default policy (architecture doc §7) keyed by CapabilityId, over a conservative global fallback (hidden + ask). Sources the default without adding a field to the 49-construction-site CapabilityDescriptor, keeping the slice #4544-independent. clippy -D warnings clean; 3 new tests (11 total). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…§17) Design §1-§14 holds; §17 captures what's built (epic #5261 child issues) and the corrections implementation forced: first-party integrations are WASM extensions not MCP (§7/§12 fixed); per-user availability is a CapabilityPolicyDelta (#4544 can_be_mutated_by forbids an admin writing UserPrivate for others, so #4544 is tenant-shared + per-user rides deltas); the resolver is feature+IRONCLAW_REBORN_CAPABILITY_POLICY gated (default off); the scoped-lifecycle store roots under the /tenants durable mount (the /engine default has no backend); #5272 reworked to REST-created users; approval has no admin path yet; Reborn admin gate is host_api::UserRole not src/ownership. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5273) (#5288) The storage + resolution foundation for the configuration / identity / approval dimensions. Adds, in `ironclaw_capability_policy`: - `CapabilityPolicyDeltaStore` — durable store of per-(tenant, scope, capability) `CapabilityPolicyDelta` rows (the admin grants). Keyed with an explicit tenant (PolicyScope carries none) so deltas never leak across tenants; upsert/delete/`deltas_for`/`list_subject_deltas`. The admin REST surface (#5268) writes here; the resolver reads. Mirrors the #4544 store shape. - `InMemoryCapabilityPolicyDeltaStore` — in-memory backend for tests / local-dev (durable filesystem / libSQL backend is the follow-on). - `StoreBackedPolicyResolver` — implements the #5262 `PolicyResolver` port by folding the capability default (#5263 `CapabilityDefaultPolicySource`) with the subject's stored deltas via `resolve_effective_policy` into an `EffectivePolicy`. Resolution is live (reads the store every call). `deltas_for` pre-filters to the subject (tenant-wide row + the subject's own user row; project scope is dormant in v1). Availability in the resulting `EffectivePolicy` is the policy view (default + deltas); combining it with the installation view (#4544 / #5267) — plus injecting config, applying the identity ownership filter, and merging the approval admin layer — is the enforcement layer (the remaining #5273 work). Tests: 6 (upsert/read-back/delete, tenant-wide vs user-only visibility, cross- tenant isolation, default->tenant->user fold incl. config deep-merge + approval override, default-only when no deltas, subject-scoped listing). Crate green under `--all-features` clippy `-D warnings`. Part of #5261. Depends on #5262, #5263. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…r, identity/config/approval enforcement (epic #5261) Slices the #4544-INDEPENDENT capability-policy engine out of the tested integration branch onto #5262 (main + the ironclaw_capability_policy crate). Contains: - durable FilesystemCapabilityPolicyDeltaStore (libSQL/filesystem) + its contract test, behind the storage crate's new ironclaw_capability_policy dep; - StoreBackedPolicyResolver wired via capability_policy_engine.rs (local_dev_capability_policy_delta_store + build_capability_policy_resolver); - the identity/config/approval dispatch seams (PolicyResolverConfigSource, PolicyResolverAdminApprovalSource) wired through factory.rs + local_dev.rs. Builds on #5262. Independent of #4544: no scoped-lifecycle store, capability_surface_policy, or control-plane route modules are present, and the crate compiles + tests pass without any #4544 file. The acting-principal derivation the config seam needs is inlined into the engine so it carries no dependency on the availability surface resolver. Enforces three of the four capability-policy dimensions (identity, config, approval); availability stays AllowAll on this branch and lands in a separate availability PR. Gated by the `capability-policy` feature + the IRONCLAW_REBORN_CAPABILITY_POLICY env toggle (off by default). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5344
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a capability-policy crate plus durable storage, loop dispatch config merging, admin approval precedence, feature-gated Reborn wiring, and capability-keyed credential resolution. ChangesCapability policy rollout
Sequence Diagram(s)sequenceDiagram
participant HostRuntimeLoopCapabilityPort
participant PolicyResolverConfigSource
participant StoreBackedPolicyResolver
participant CapabilityPolicyDeltaStore
HostRuntimeLoopCapabilityPort->>PolicyResolverConfigSource: config_for(run_context, capability_id)
PolicyResolverConfigSource->>StoreBackedPolicyResolver: resolve(subject, capability_id)
StoreBackedPolicyResolver->>CapabilityPolicyDeltaStore: deltas_for(subject, capability_id)
CapabilityPolicyDeltaStore-->>StoreBackedPolicyResolver: CapabilityPolicyDelta[]
StoreBackedPolicyResolver-->>PolicyResolverConfigSource: EffectivePolicy
PolicyResolverConfigSource-->>HostRuntimeLoopCapabilityPort: JSON config patch or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)crates/ironclaw_reborn_composition/src/capability_policy_engine.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.11][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the four-dimension capability policy engine and its durable filesystem-backed delta storage for IronClaw Reborn. It defines the precedence cascade (default -> tenant -> project -> user) and integrates it into the capability dispatch, credential resolution, and approval flows. The feedback highlights two key improvements: first, optimizing deltas_for in the filesystem store to directly fetch the deterministic capability delta file instead of performing an inefficient directory query on the hot path; second, changing the logging level in filesystem_error from error! to debug! to prevent potential terminal corruption in REPL/TUI-reachable code.
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.
| async fn deltas_for( | ||
| &self, | ||
| subject: &PolicySubject, | ||
| capability: &CapabilityId, | ||
| ) -> Result<Vec<CapabilityPolicyDelta>, PolicyError> { | ||
| let mut found = Vec::new(); | ||
| for scope in subject_scopes(subject) { | ||
| for delta in self.deltas_in_scope(&subject.tenant_id, &scope).await? { | ||
| // Defense-in-depth: the path scheme already isolates by scope, | ||
| // but re-check capability + subject applicability before | ||
| // returning a row to the resolver. | ||
| if &delta.capability == capability | ||
| && scope_applies_to_subject(&delta.scope, subject) | ||
| { | ||
| found.push(delta); | ||
| } | ||
| } | ||
| } | ||
| Ok(found) | ||
| } |
There was a problem hiding this comment.
In deltas_for, querying the entire directory of deltas via deltas_in_scope and filtering them in memory is highly inefficient, especially since this method is called on the hot path of every capability dispatch. Since the path to the specific capability's delta is completely deterministic and known (delta_leaf_path), you can directly fetch the file using self.filesystem.get(&path). This avoids directory listing, pagination, and parsing of unrelated JSON files.
async fn deltas_for(
&self,
subject: &PolicySubject,
capability: &CapabilityId,
) -> Result<Vec<CapabilityPolicyDelta>, PolicyError> {
let mut found = Vec::new();
for scope in subject_scopes(subject) {
let path = delta_leaf_path(&self.root, &subject.tenant_id, &scope, capability)?;
match self.filesystem.get(&path).await {
Ok(entry) => {
let delta = parse_delta(&entry)?;
if scope_applies_to_subject(&delta.scope, subject) {
found.push(delta);
}
}
Err(FilesystemError::NotFound { .. }) => {}
Err(error) => return Err(filesystem_error("read capability policy delta", error)),
}
}
Ok(found)
}| fn filesystem_error(operation: &'static str, error: FilesystemError) -> PolicyError { | ||
| tracing::error!( | ||
| operation, | ||
| error = %error, | ||
| "capability policy delta store filesystem operation failed" | ||
| ); | ||
| PolicyError::Unavailable { | ||
| reason: format!("capability policy delta store failed to {operation}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Logging at levels above debug! (such as error!, warn!, or info!) in REPL/TUI-reachable code can print to stdout/stderr and corrupt the terminal interface. Please use tracing::debug! instead to ensure a clean terminal experience.
| fn filesystem_error(operation: &'static str, error: FilesystemError) -> PolicyError { | |
| tracing::error!( | |
| operation, | |
| error = %error, | |
| "capability policy delta store filesystem operation failed" | |
| ); | |
| PolicyError::Unavailable { | |
| reason: format!("capability policy delta store failed to {operation}"), | |
| } | |
| } | |
| fn filesystem_error(operation: &'static str, error: FilesystemError) -> PolicyError { | |
| tracing::debug!( | |
| operation, | |
| error = %error, | |
| "capability policy delta store filesystem operation failed" | |
| ); | |
| PolicyError::Unavailable { | |
| reason: format!("capability policy delta store failed to {operation}"), | |
| } | |
| } |
References
- Do not use warn! or info! logging in REPL/TUI-reachable code as they can corrupt the REPL/TUI. Use debug! logging instead, and ensure only sanitized identifiers are logged.
There was a problem hiding this comment.
Pull request overview
This PR adds the Reborn-side capability-policy “engine” pieces needed to enforce identity/config/approval policy at dispatch time: a durable policy-delta store, a store-backed resolver, and composition/runtime seams that inject policy config into capability inputs, apply admin approval precedence, and enforce identity mode during credential resolution (all gated by the capability-policy feature and the IRONCLAW_REBORN_CAPABILITY_POLICY env toggle).
Changes:
- Introduces the
ironclaw_capability_policycrate (policy vocabulary, precedence fold, delta store trait + store-backed resolver, deep-merge helper). - Adds a durable filesystem/libSQL/Postgres-backed
FilesystemCapabilityPolicyDeltaStoreinironclaw_product_workflow_storageplus a durable contract test. - Wires policy into Reborn composition/runtime: config deep-merge at dispatch, admin approval precedence, and identity-mode enforcement during
resolve_access_secret.
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/plans/2026-06-24-capability-policy-architecture.md | Adds/updates the capability-policy architecture plan and implementation status notes. |
| crates/ironclaw_reborn_composition/tests/product_live_adapters.rs | Updates adapter test config to carry optional policy config source under feature gate. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rs | Threads policy_config_source through local-dev runtime tests under feature gate. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rs | Threads policy_config_source through shell local-dev test wiring under feature gate. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev/refreshing_capability_port.rs | Plumbs optional policy config source into refreshing local-dev capability port factory. |
| crates/ironclaw_reborn_composition/src/runtime/local_dev.rs | Constructs PolicyResolverConfigSource and wires it into local-dev capability port creation. |
| crates/ironclaw_reborn_composition/src/profile_approval_authorization.rs | Adds AdminApprovalSource seam and applies admin approval precedence in the approval chain (with tests). |
| crates/ironclaw_reborn_composition/src/product_live_adapters.rs | Threads optional policy config source into product-live capability port factory (feature-gated field). |
| crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests/duplicate_selection.rs | Updates runtime-credential tests for new capability_id field in requests. |
| crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs | Adds identity-policy enforcement tests (feature-gated) and updates request construction for new capability_id. |
| crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs | Enforces identity mode via optional PolicyResolver and maps identity mandates to credential ownership/selection behavior. |
| crates/ironclaw_reborn_composition/src/product_auth_durable/tests.rs | Updates durable auth tests for new capability_id request field. |
| crates/ironclaw_reborn_composition/src/product_auth_durable/interactions.rs | Notes TODOs for tagging admin-keyed ownership once durable flows carry capability+resolver. |
| crates/ironclaw_reborn_composition/src/product_auth_durable/flows.rs | Notes TODOs for tagging admin-keyed ownership once OAuth flow carries capability+resolver. |
| crates/ironclaw_reborn_composition/src/local_dev_authorization.rs | Wires optional admin approval source into local-dev authorizer construction. |
| crates/ironclaw_reborn_composition/src/lib.rs | Adds capability_policy_engine module behind the capability-policy feature. |
| crates/ironclaw_reborn_composition/src/factory.rs | Builds single shared delta store + resolver handles; wires admin approval + identity enforcement into local runtime build. |
| crates/ironclaw_reborn_composition/src/capability_policy_engine.rs | New engine module: shared delta-store + resolver builder; adapters for config/approval seams; env activation gate. |
| crates/ironclaw_reborn_composition/Cargo.toml | Adds capability-policy feature and optional dependency wiring for policy/storage integration. |
| crates/ironclaw_product_workflow_storage/tests/durable_capability_policy_delta_contract.rs | New durable contract test for the filesystem/libSQL/Postgres delta store + resolver fold. |
| crates/ironclaw_product_workflow_storage/src/lib.rs | Exposes FilesystemCapabilityPolicyDeltaStore from the storage crate. |
| crates/ironclaw_product_workflow_storage/src/capability_policy_delta.rs | Implements durable CapabilityPolicyDeltaStore over RootFilesystem with tenant/scope/capability path scheme. |
| crates/ironclaw_product_workflow_storage/Cargo.toml | Adds ironclaw_capability_policy and hex dependencies for durable delta store implementation. |
| crates/ironclaw_loop_support/src/lib.rs | Re-exports LoopCapabilityConfigSource from capability port module. |
| crates/ironclaw_loop_support/src/capability_port.rs | Adds LoopCapabilityConfigSource and dispatch-time deep-merge of admin policy config into capability inputs (with replay-safe behavior + tests). |
| crates/ironclaw_loop_support/Cargo.toml | Adds dependency on ironclaw_capability_policy for shared deep-merge semantics. |
| crates/ironclaw_host_runtime/src/wasm_credentials.rs | Populates the new capability_id field when requesting credential resolution. |
| crates/ironclaw_host_runtime/src/obligations.rs | Extends RuntimeCredentialAccountRequest with capability_id and threads it through obligation handling. |
| crates/ironclaw_capability_policy/src/store.rs | Adds delta-store trait, in-memory store, and StoreBackedPolicyResolver implementation. |
| crates/ironclaw_capability_policy/src/lib.rs | New policy model crate: vocabulary, precedence fold, deep merge helper, default policy source, and resolver port. |
| crates/ironclaw_capability_policy/Cargo.toml | Defines the new ironclaw_capability_policy crate. |
| Cargo.toml | Adds crates/ironclaw_capability_policy to the workspace members. |
| Cargo.lock | Records new crate/dependency entries for the policy work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut found = Vec::new(); | ||
| for scope in subject_scopes(subject) { | ||
| for delta in self.deltas_in_scope(&subject.tenant_id, &scope).await? { | ||
| // Defense-in-depth: the path scheme already isolates by scope, | ||
| // but re-check capability + subject applicability before | ||
| // returning a row to the resolver. | ||
| if &delta.capability == capability | ||
| && scope_applies_to_subject(&delta.scope, subject) | ||
| { | ||
| found.push(delta); | ||
| } | ||
| } | ||
| } | ||
| Ok(found) |
| /// Stable per-row key component for a scope. Re-derived **verbatim** from | ||
| /// `ironclaw_capability_policy::store::scope_key` (private there). If that | ||
| /// definition changes (e.g. project scope goes live) this copy must change with | ||
| /// it — see the regression note in the module tests. | ||
| fn scope_key(scope: &PolicyScope) -> String { |
|
|
||
| | Concept | Status — reuse / extend | | ||
| |---|---| | ||
| | Tenant / User / roles / per-user secrets | **exist (#1626)** — `TenantId`/`UserId`, RBAC, admin secrets, multi-tenant isolation. Add `kind(person\|shared)`. **Admin gate today = `src/ownership::UserRole` (Owner/Admin/Regular) + `AdminScope`** — reuse it; `ProjectRole(Owner/Editor/Viewer)` is the separate `ironclaw_projects` model (see §16) | |
|
I would make the resolver contract dimension-explicit before this grows into the availability and control-plane layers. The durable delta store should be accepted only as policy input, not as proof that a capability is currently available or safe to execute. The policy decision should stay explainable as: identity matched, config dimension applied, approval dimension resolved, and availability either intentionally out of scope or explicitly evaluated by a later layer. The regression fixture I would keep is:
That gives the later control plane a clean contract: it can add admin/user surfaces and availability checks without changing what a durable policy grant already means. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, merge readiness, security review, production readiness, partnership, or customer interest. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_reborn_composition/src/profile_approval_authorization.rs (2)
280-303: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCheck explicit
Disabledbefore hard-floor/admin auto-allow.Line 288 can return
RequireApproval, and Line 294 can returnAllow, before Line 303 checks the explicit per-toolDisabledoverride. A disabled capability can therefore be prompted or run when it has a forced-gate effect or adminAllow, contradicting Line 299’s “deny outright” contract.As per coding guidelines, “Fail closed for auth, approvals, trust...”.
Proposed precedence fix
if matches!(admin_approval, Some(PermissionMode::Deny)) { return Decision::Deny { reason: DenyReason::PolicyDenied, }; } + + let tool_override = settings + .tool_override(&context.resource_scope, &descriptor.id) + .await; + if matches!(tool_override, Some(ToolPermissionOverride::Disabled)) { + return Decision::Deny { + reason: DenyReason::PolicyDenied, + }; + } + // Hard floor (`#4776/`#4959): forced-gate effects (Financial, …) ALWAYS // require an explicit approval and can NEVER be auto-approved — not by a // stored grant, not by the global switch, and NOT by an admin `Allow`. @@ if matches!(admin_approval, Some(PermissionMode::Allow)) { return decision; } // Decision precedence (high → low), `#4776`: // 1. Explicit per-tool `disabled` → deny outright (strongest user intent). - let tool_override = settings - .tool_override(&context.resource_scope, &descriptor.id) - .await; - if matches!(tool_override, Some(ToolPermissionOverride::Disabled)) { - return Decision::Deny { - reason: DenyReason::PolicyDenied, - }; - }🤖 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_reborn_composition/src/profile_approval_authorization.rs` around lines 280 - 303, Move the explicit per-tool Disabled check in profile_approval_authorization.rs ahead of both the forced-gate hard-floor path and the admin Allow auto-approve path inside the approval decision logic. Update the flow around gate_policy.effects_force_approval, admin_approval, and tool_override so a Disabled override always returns deny first, preserving the fail-closed precedence contract for explicit user intent before any approval or allow shortcuts.Source: Coding guidelines
201-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the admin source on the spawn caller path.
Line 210 now threads
admin_policyintoauthorize_spawn_with_trust, but the new admin tests only exercise dispatch. Add a spawn-path regression, ideally with a dispatch-only descriptor so theSpawnProcesselevation path is covered.As per path instructions, “Test through the caller: when a helper gates a side effect, require a test driving the real call site...”
Also applies to: 627-738
🤖 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_reborn_composition/src/profile_approval_authorization.rs` around lines 201 - 210, The spawn authorization path is now threading admin policy through authorize_spawn_with_trust, but there is no regression test covering the real spawn caller path. Add a test that drives the actual spawn flow via require_approval_for_profile_policy / authorize_spawn_with_trust using a dispatch-only descriptor so the SpawnProcess elevation path is exercised, and verify admin source handling on that path rather than only dispatch. Keep the new coverage aligned with the existing admin policy tests in profile_approval_authorization.rs and ensure the spawn-side call site is what triggers the assertion.Source: Path instructions
crates/ironclaw_reborn_composition/tests/product_live_adapters.rs (1)
1418-1440: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis only updates the fixture; it doesn’t prove the new branch works.
The new product-live seam is
policy_config_source -> ProductLiveLoopCapabilityPortFactory -> with_policy_config_source(...), but this test change hard-codesNone, so the feature path never executes. Please add a feature-gated caller test that builds adapters with a non-Noneconfig source and asserts it is consulted during a real dispatch.As per path instructions, “Test through the caller: when a helper gates a side effect, require a test driving the real call site (handler/factory/manager), not only the helper.”
🤖 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_reborn_composition/tests/product_live_adapters.rs` around lines 1418 - 1440, The fixture change in adapter_config only covers the default None path and does not exercise the new policy_config_source seam. Add a feature-gated test around ProductLiveLoopCapabilityPortFactory and with_policy_config_source that constructs a ProductLivePlannedRuntimeAdapterConfig with a real non-None policy_config_source, then drive a real dispatch through the caller path and assert the source is actually consulted. Keep the existing adapter_config helper as a fixture, but add coverage at the handler/factory/manager level where the side effect is triggered.Source: Path instructions
🤖 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_capability_policy/src/lib.rs`:
- Around line 99-107: PolicyScope currently exposes Project even though
PolicySubject and the tenant/user-only store matching paths cannot read it, so a
project-scoped delta can be accepted and then silently disappear. Update the
PolicyScope/PolicySubject flow in lib.rs and the store implementations so
Project is either explicitly rejected with a loud error until project identity
is supported, or fully threaded through deltas_for() and list_subject_deltas()
by adding project_id handling end-to-end. Use the existing PolicyScope,
PolicySubject, deltas_for, and list_subject_deltas symbols to keep the
read/write paths consistent and avoid silent no-op writes.
In `@crates/ironclaw_loop_support/src/capability_port.rs`:
- Around line 1731-1785: The intentional fail-open branch in capability_port’s
`config_for(...).await` error arm needs an inline `// silent-ok: ...`
justification comment naming the operation, and the cross-layer guarantee should
be backed by a test instead of only prose. Update the `Err(error)` branch near
the policy config merge path to keep the current debug-only handling but add the
required marker, and add/extend a caller-level test that exercises `config_for()
-> Err(_)` and verifies dispatch continues with the un-merged input.
In `@crates/ironclaw_product_workflow_storage/src/capability_policy_delta.rs`:
- Around line 76-85: In capability_policy_delta.rs, the pagination loop in the
delta listing logic currently treats FilesystemError::NotFound as an empty
result on every page, which can silently truncate a mid-scan listing. Update the
query handling in the loop so that NotFound is converted to Ok(deltas) only when
offset is still on the first page, and after any page has already been read
(offset > 0) return the filesystem error through filesystem_error("list
capability policy deltas", error) or equivalent fail-loud propagation. Keep the
behavior localized to the loop that builds deltas from self.filesystem.query so
later pages cannot be mistaken for a never-written prefix.
In `@crates/ironclaw_reborn_composition/src/capability_policy_engine.rs`:
- Around line 183-195: Resolver failures in `capability_policy_engine.rs` are
currently being converted into `None`, which is treated as “no admin opinion”
and can fall through the approval chain. Update the `Err(error)` branch in the
approval resolution flow to fail closed instead of returning empty state:
surface the fault as a hard denial or explicit error path in the capability
policy logic, and keep the `tracing::debug!` logging in place for diagnostics.
Use the existing approval-resolution entrypoint and `capability_policy_engine`
decision path to ensure resolver outages cannot bypass the admin approval
boundary.
In
`@crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs`:
- Around line 1333-1378: The policy fake in FakePolicyResolver::resolve
currently ignores the incoming CapabilityId, so the tests do not verify that
request.capability_id is actually propagated into policy resolution. Update the
fake to assert on or branch by the provided capability (for example, compare
_capability against the expected CapabilityId and return a distinct result for a
mismatched value), and add at least one test case that exercises a divergent
capability so regressions in capability threading are caught.
In `@crates/ironclaw_reborn_composition/src/profile_approval_authorization.rs`:
- Around line 13-29: Change AdminApprovalSource so resolver faults are not
treated the same as “no admin opinion”; update admin_approval to return a Result
that can distinguish an actual None from an error, and propagate failures from
the PolicyResolverAdminApprovalSource adapter instead of converting Err to None.
Then update the fallback logic in profile_approval_authorization to fail closed
on admin resolution errors (e.g. RequireApproval or Deny) rather than falling
through to user/profile approval, and keep the trait/docs aligned with the new
error semantics.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/profile_approval_authorization.rs`:
- Around line 280-303: Move the explicit per-tool Disabled check in
profile_approval_authorization.rs ahead of both the forced-gate hard-floor path
and the admin Allow auto-approve path inside the approval decision logic. Update
the flow around gate_policy.effects_force_approval, admin_approval, and
tool_override so a Disabled override always returns deny first, preserving the
fail-closed precedence contract for explicit user intent before any approval or
allow shortcuts.
- Around line 201-210: The spawn authorization path is now threading admin
policy through authorize_spawn_with_trust, but there is no regression test
covering the real spawn caller path. Add a test that drives the actual spawn
flow via require_approval_for_profile_policy / authorize_spawn_with_trust using
a dispatch-only descriptor so the SpawnProcess elevation path is exercised, and
verify admin source handling on that path rather than only dispatch. Keep the
new coverage aligned with the existing admin policy tests in
profile_approval_authorization.rs and ensure the spawn-side call site is what
triggers the assertion.
In `@crates/ironclaw_reborn_composition/tests/product_live_adapters.rs`:
- Around line 1418-1440: The fixture change in adapter_config only covers the
default None path and does not exercise the new policy_config_source seam. Add a
feature-gated test around ProductLiveLoopCapabilityPortFactory and
with_policy_config_source that constructs a
ProductLivePlannedRuntimeAdapterConfig with a real non-None
policy_config_source, then drive a real dispatch through the caller path and
assert the source is actually consulted. Keep the existing adapter_config helper
as a fixture, but add coverage at the handler/factory/manager level where the
side effect is triggered.
🪄 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: f4972e62-7d53-41b1-a56e-dae09deecb4f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (32)
Cargo.tomlcrates/ironclaw_capability_policy/Cargo.tomlcrates/ironclaw_capability_policy/src/lib.rscrates/ironclaw_capability_policy/src/store.rscrates/ironclaw_host_runtime/src/obligations.rscrates/ironclaw_host_runtime/src/wasm_credentials.rscrates/ironclaw_loop_support/Cargo.tomlcrates/ironclaw_loop_support/src/capability_port.rscrates/ironclaw_loop_support/src/lib.rscrates/ironclaw_product_workflow_storage/Cargo.tomlcrates/ironclaw_product_workflow_storage/src/capability_policy_delta.rscrates/ironclaw_product_workflow_storage/src/lib.rscrates/ironclaw_product_workflow_storage/tests/durable_capability_policy_delta_contract.rscrates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/capability_policy_engine.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/local_dev_authorization.rscrates/ironclaw_reborn_composition/src/product_auth_durable/flows.rscrates/ironclaw_reborn_composition/src/product_auth_durable/interactions.rscrates/ironclaw_reborn_composition/src/product_auth_durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests/duplicate_selection.rscrates/ironclaw_reborn_composition/src/product_live_adapters.rscrates/ironclaw_reborn_composition/src/profile_approval_authorization.rscrates/ironclaw_reborn_composition/src/runtime/local_dev.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/refreshing_capability_port.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/shell_tests.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/tests.rscrates/ironclaw_reborn_composition/tests/product_live_adapters.rsdocs/plans/2026-06-24-capability-policy-architecture.md
| /// Precedence scope of a [`CapabilityPolicyDelta`]. Higher rank overrides lower. | ||
| /// For v1 the `Tenant` scope is the default project (architecture doc §1, §8). | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum PolicyScope { | ||
| Tenant, | ||
| Project { project_id: ProjectId }, | ||
| User { user_id: UserId }, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail loud on unsupported Project scope.
PolicyScope advertises Project, but PolicySubject still has no project_id, and both store implementations only match tenant/user scopes. A project-scoped delta can therefore be written successfully yet never appear in deltas_for() or list_subject_deltas(), so the admin write becomes a silent no-op. Either reject Project deltas until project identity exists on the subject/read path, or thread project_id through that path in this PR.
As per path instructions, "Fail loud: flag silent-failure patterns ... Errors propagate with ? into thiserror types with context."
Also applies to: 220-225
🤖 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_capability_policy/src/lib.rs` around lines 99 - 107,
PolicyScope currently exposes Project even though PolicySubject and the
tenant/user-only store matching paths cannot read it, so a project-scoped delta
can be accepted and then silently disappear. Update the
PolicyScope/PolicySubject flow in lib.rs and the store implementations so
Project is either explicitly rejected with a loud error until project identity
is supported, or fully threaded through deltas_for() and list_subject_deltas()
by adding project_id handling end-to-end. Use the existing PolicyScope,
PolicySubject, deltas_for, and list_subject_deltas symbols to keep the
read/write paths consistent and avoid silent no-op writes.
Source: Path instructions
| // Fail-OPEN is structural here: a `config_for` fault must NOT end the | ||
| // turn (an `Err` maps to a terminal `HostUnavailable`, see | ||
| // `.claude/rules/agent-loop-capabilities.md`). We match the result | ||
| // explicitly — `Ok(Some)` merges, `Ok(None)` skips, and `Err` is | ||
| // logged at `debug!` (dispatch path: never `info!`/`warn!`) before | ||
| // continuing with the un-merged model input. | ||
| if let Some(config_source) = &self.policy_config_source { | ||
| match config_source | ||
| .config_for(&self.run_context, &request.capability_id) | ||
| .await | ||
| { | ||
| Ok(Some(config)) => { | ||
| ironclaw_capability_policy::deep_merge_into(&mut input, &config); | ||
| // The admin patch is overlaid AFTER the input was | ||
| // validated/normalized against the capability schema, so | ||
| // re-validate the merged payload exactly like the original | ||
| // input. A validation failure is model-visible and | ||
| // recoverable (the model can adjust its request), so route | ||
| // it to `Failed { InvalidInput }` — never an `Err` that | ||
| // would end the turn. | ||
| input = match prepare_provider_arguments_with_detail( | ||
| &input, | ||
| &capability.parameters_schema, | ||
| "capability input", | ||
| ) { | ||
| Ok(input) => input, | ||
| Err(error) => { | ||
| let result = Ok(CapabilityOutcome::Failed(CapabilityFailure { | ||
| error_kind: CapabilityFailureKind::InvalidInput, | ||
| safe_summary: error.error.safe_summary, | ||
| detail: error.detail, | ||
| })); | ||
| guard.commit(); | ||
| self.record_loop_completed( | ||
| &idempotency_key, | ||
| requested_invocation_id, | ||
| result.clone(), | ||
| )?; | ||
| return result; | ||
| } | ||
| }; | ||
| // Re-apply the sandbox-plan wrapping so the merged payload | ||
| // is normalized like the original (process-sandbox plans | ||
| // round-trip through `SandboxProcessPlan` validation). | ||
| input = host_runtime_input_for_capability(&request.capability_id, input)?; | ||
| } | ||
| Ok(None) => {} | ||
| Err(error) => { | ||
| tracing::debug!( | ||
| capability_id = request.capability_id.as_str(), | ||
| error = %error, | ||
| "policy config source faulted; continuing with un-merged \ | ||
| input (fail-open, #5261 D5)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Annotate and lock down the intentional fail-open branch.
Line 1778 deliberately drops a boundary-call error and keeps dispatching, but this branch is missing the repo-required // silent-ok: ... marker. The new tests also never exercise config_for() -> Err(_), so the cross-layer “fail-open and dispatch unmerged input” guarantee here is still comment-only.
As per coding guidelines, "When a fallback is genuinely acceptable, it must be justified inline with a // silent-ok: <reason> comment naming the operation" and "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent." As per path instructions, "Fail loud: flag silent-failure patterns" and "Test through the caller."
Also applies to: 4404-4605
🤖 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_loop_support/src/capability_port.rs` around lines 1731 -
1785, The intentional fail-open branch in capability_port’s
`config_for(...).await` error arm needs an inline `// silent-ok: ...`
justification comment naming the operation, and the cross-layer guarantee should
be backed by a test instead of only prose. Update the `Err(error)` branch near
the policy config merge path to keep the current debug-only handling but add the
required marker, and add/extend a caller-level test that exercises `config_for()
-> Err(_)` and verifies dispatch continues with the un-merged input.
Sources: Coding guidelines, Path instructions
| loop { | ||
| let entries = match self | ||
| .filesystem | ||
| .query(&dir, &Filter::All, Page::new(offset, Page::MAX_LIMIT)) | ||
| .await | ||
| { | ||
| Ok(entries) => entries, | ||
| Err(FilesystemError::NotFound { .. }) => return Ok(deltas), | ||
| Err(error) => return Err(filesystem_error("list capability policy deltas", error)), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only collapse NotFound to empty on the first page.
Once offset > 0, FilesystemError::NotFound no longer means "never-written prefix"; it means the listing changed mid-scan or the backend failed differently. Returning Ok(deltas) here converts that boundary failure into a partial policy snapshot, which can drop later tenant/user rows and mis-resolve availability or approval. Treat NotFound as empty only for the initial page and fail loud after that.
As per path instructions, "Fail loud: flag silent-failure patterns ... Errors propagate with ? into thiserror types with context."
🤖 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_product_workflow_storage/src/capability_policy_delta.rs`
around lines 76 - 85, In capability_policy_delta.rs, the pagination loop in the
delta listing logic currently treats FilesystemError::NotFound as an empty
result on every page, which can silently truncate a mid-scan listing. Update the
query handling in the loop so that NotFound is converted to Ok(deltas) only when
offset is still on the first page, and after any page has already been read
(offset > 0) return the filesystem error through filesystem_error("list
capability policy deltas", error) or equivalent fail-loud propagation. Keep the
behavior localized to the loop that builds deltas from self.filesystem.query so
later pages cannot be mistaken for a never-written prefix.
Source: Path instructions
| Err(error) => { | ||
| // Fail-SAFE (#5261 D5 approval): a resolver fault must NOT | ||
| // auto-approve (privilege escalation). Returning `None` makes | ||
| // the dispatch chain treat this as "no admin opinion" and fall | ||
| // through to the existing user/profile steps. Logged at debug so | ||
| // it never corrupts the REPL/TUI surface (see CLAUDE.md). | ||
| tracing::debug!( | ||
| %error, | ||
| capability = %capability_id.as_str(), | ||
| "capability policy approval resolution failed; deferring to user/profile chain" | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Resolver faults currently bypass the admin approval layer.
Err(_) => None is not fail-safe here. In this trait, None is the same value used for “no admin opinion” / Ask, so a resolver outage falls through to the user/profile chain and can still allow execution. For an approval boundary, that is a fail-open authorization path.
Suggested fix
Err(error) => {
tracing::debug!(
%error,
capability = %capability_id.as_str(),
"capability policy approval resolution failed; deferring to user/profile chain"
);
- None
+ Some(ironclaw_host_api::PermissionMode::Deny)
}As per coding guidelines, “Fail closed for auth, approvals...” and as per path instructions, the “Fail loud” invariant forbids swallowing boundary faults into empty state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Err(error) => { | |
| // Fail-SAFE (#5261 D5 approval): a resolver fault must NOT | |
| // auto-approve (privilege escalation). Returning `None` makes | |
| // the dispatch chain treat this as "no admin opinion" and fall | |
| // through to the existing user/profile steps. Logged at debug so | |
| // it never corrupts the REPL/TUI surface (see CLAUDE.md). | |
| tracing::debug!( | |
| %error, | |
| capability = %capability_id.as_str(), | |
| "capability policy approval resolution failed; deferring to user/profile chain" | |
| ); | |
| None | |
| } | |
| Err(error) => { | |
| // Fail-SAFE (`#5261` D5 approval): a resolver fault must NOT | |
| // auto-approve (privilege escalation). Returning `None` makes | |
| // the dispatch chain treat this as "no admin opinion" and fall | |
| // through to the existing user/profile steps. Logged at debug so | |
| // it never corrupts the REPL/TUI surface (see CLAUDE.md). | |
| tracing::debug!( | |
| %error, | |
| capability = %capability_id.as_str(), | |
| "capability policy approval resolution failed; deferring to user/profile chain" | |
| ); | |
| Some(ironclaw_host_api::PermissionMode::Deny) | |
| } |
🤖 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_reborn_composition/src/capability_policy_engine.rs` around
lines 183 - 195, Resolver failures in `capability_policy_engine.rs` are
currently being converted into `None`, which is treated as “no admin opinion”
and can fall through the approval chain. Update the `Err(error)` branch in the
approval resolution flow to fail closed instead of returning empty state:
surface the fault as a hard denial or explicit error path in the capability
policy logic, and keep the `tracing::debug!` logging in place for diagnostics.
Use the existing approval-resolution entrypoint and `capability_policy_engine`
decision path to ensure resolver outages cannot bypass the admin approval
boundary.
Sources: Coding guidelines, Path instructions
| #[async_trait::async_trait] | ||
| impl PolicyResolver for FakePolicyResolver { | ||
| async fn resolve( | ||
| &self, | ||
| _subject: &PolicySubject, | ||
| _capability: &CapabilityId, | ||
| ) -> Result<EffectivePolicy, PolicyError> { | ||
| match &self.outcome { | ||
| Ok(identity) => Ok(EffectivePolicy { | ||
| available: true, | ||
| identity: *identity, | ||
| approval: PermissionMode::Ask, | ||
| config: serde_json::Value::Null, | ||
| }), | ||
| Err(PolicyError::Unavailable { reason }) => Err(PolicyError::Unavailable { | ||
| reason: reason.clone(), | ||
| }), | ||
| Err(PolicyError::Internal { reason }) => Err(PolicyError::Internal { | ||
| reason: reason.clone(), | ||
| }), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn resolver_with_policy( | ||
| accounts: Arc<InMemoryAuthProductServices>, | ||
| policy: Arc<dyn PolicyResolver>, | ||
| ) -> ProductAuthRuntimeCredentialResolver { | ||
| resolver_with_accounts(accounts).with_policy_resolver(policy) | ||
| } | ||
|
|
||
| fn manual_token_request<'a>( | ||
| capability: &'a CapabilityId, | ||
| scope: &'a ResourceScope, | ||
| provider: &'a RuntimeCredentialAccountProviderId, | ||
| requester: &'a ExtensionId, | ||
| ) -> RuntimeCredentialAccountRequest<'a> { | ||
| RuntimeCredentialAccountRequest { | ||
| capability_id: capability, | ||
| scope, | ||
| provider, | ||
| setup: &RuntimeCredentialAccountSetup::ManualToken, | ||
| provider_scopes: &[], | ||
| requester_extension: requester, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the policy fake assert the incoming capability.
FakePolicyResolver::resolve ignores _capability, so this suite never proves that request.capability_id is the value reaching policy resolution. A regression that hard-codes or misthreads the capability would still pass every new test here. Make the fake discriminate on capability (or record/assert the observed CapabilityId) in at least one divergent-capability case.
🤖 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_reborn_composition/src/product_auth_runtime_credentials/tests.rs`
around lines 1333 - 1378, The policy fake in FakePolicyResolver::resolve
currently ignores the incoming CapabilityId, so the tests do not verify that
request.capability_id is actually propagated into policy resolution. Update the
fake to assert on or branch by the provided capability (for example, compare
_capability against the expected CapabilityId and return a distinct result for a
mismatched value), and add at least one test case that exercises a divergent
capability so regressions in capability threading are caught.
| /// Optional admin (org-wide) approval opinion for a capability, keyed by the | ||
| /// dispatch resource scope (#5261 D6). | ||
| /// | ||
| /// Deliberately local so this dep-light module never imports | ||
| /// `ironclaw_capability_policy`: the seam is a trait, and composition supplies | ||
| /// the concrete adapter over the shared `PolicyResolver` (see | ||
| /// `crate::capability_policy_engine::PolicyResolverAdminApprovalSource`). A | ||
| /// `None` return (no admin row, `Ask`, or a resolver fault) means "no admin | ||
| /// opinion" and the dispatch falls through to the existing user/profile chain | ||
| /// (fail-SAFE, #5261 D5 approval — never auto-approve on error). | ||
| #[async_trait] | ||
| pub(crate) trait AdminApprovalSource: Send + Sync { | ||
| async fn admin_approval( | ||
| &self, | ||
| scope: &ResourceScope, | ||
| capability_id: &CapabilityId, | ||
| ) -> Option<PermissionMode>; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not collapse admin resolver faults into “no admin opinion.”
Line 20 documents resolver faults as None, and Lines 264-271 make None fall through to user/profile settings; the upstream adapter in capability_policy_engine.rs:161-197 also maps resolver Err to None. With global auto-approve or always-allow enabled, an admin-policy outage can still permit a capability that an unavailable admin Deny would have blocked. Carry resolver errors through the trait and fail closed, e.g. Result<Option<PermissionMode>, _> with Err mapped to RequireApproval/Deny.
As per coding guidelines, “Fail closed for auth, approvals, trust...”. As per path instructions, “Fail loud: flag silent-failure patterns... Errors propagate with ? into thiserror types with context.”
Also applies to: 264-271
🤖 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_reborn_composition/src/profile_approval_authorization.rs`
around lines 13 - 29, Change AdminApprovalSource so resolver faults are not
treated the same as “no admin opinion”; update admin_approval to return a Result
that can distinguish an actual None from an error, and propagate failures from
the PolicyResolverAdminApprovalSource adapter instead of converting Err to None.
Then update the fallback logic in profile_approval_authorization to fail closed
on admin resolution errors (e.g. RequireApproval or Deny) rather than falling
through to user/profile approval, and keep the trait/docs aligned with the new
error semantics.
Sources: Coding guidelines, Path instructions
|
I would treat the new review findings as one class: policy resolution must not turn boundary uncertainty into usable authority. The fail-closed cases I would make explicit are:
The regression shape I would keep is a decision-record matrix: same capability, same subject, and same stored grant, then perturb exactly one boundary condition at a time and assert the terminal reason changes. That proves the resolver is not just returning allow or ask, but preserving which dimension supplied or withheld authority. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, merge readiness, security review, production readiness, partnership, or customer interest. |
|
🚅 Deployed to the ironclaw-pr-5344 environment in ironclaw-ci-preview
|
…le home (#5261) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/capability_policy_engine.rs (1)
33-54: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse one
PolicySubjectfor every policy dimension.This ships the skew it documents: config/availability use actor-first
principal_user_id, while approval/identity useResourceScope.user_id. For delegated turns, an admin grant can be evaluated against different users across dimensions. Derive one typed subject once and pass it through all policy adapters, or fail closed until delegated turns are wired; add a caller-level regression foractor != owner.As per coding guidelines, “Fail closed for auth, approvals...” and “Comments that promise guarantees across layers must either be enforced by code/tests or softened...” As per path instructions, “Test through the caller...”
🤖 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_reborn_composition/src/capability_policy_engine.rs` around lines 33 - 54, The policy subject is still being resolved differently across dimensions, so `principal_user_id`, `PolicyResolverAdminApprovalSource`, and `resolve_identity_mandate` can evaluate against different users on delegated turns. Make the subject resolution happen once at the caller level and thread the same typed `PolicySubject` through the availability, config, approval, and identity adapters, or fail closed for any `actor != resource owner` case until that path is implemented. Add a regression test through the caller that covers delegated turns and asserts all policy checks use the same subject.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/capability_policy_engine.rs`:
- Around line 33-54: The policy subject is still being resolved differently
across dimensions, so `principal_user_id`, `PolicyResolverAdminApprovalSource`,
and `resolve_identity_mandate` can evaluate against different users on delegated
turns. Make the subject resolution happen once at the caller level and thread
the same typed `PolicySubject` through the availability, config, approval, and
identity adapters, or fail closed for any `actor != resource owner` case until
that path is implemented. Add a regression test through the caller that covers
delegated turns and asserts all policy checks use the same subject.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5af903f-d4b0-441c-b3f2-67d7c2594ccd
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/capability_policy_engine.rs
|
I would tighten this into a single-subject invariant: every policy dimension must be evaluated against the same resolved subject for the same attempted capability action. The failure mode is subtle: if config and availability resolve against the actor while approval and identity resolve against the owner or resource scope, a delegated turn can combine a grant from one principal with identity or approval state from another. That is not a precedence issue; it is subject skew. The regression shape I would keep is:
That keeps policy explanation composable: dimension may differ, subject may not. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, merge readiness, security review, production readiness, partnership, or customer interest. |
we still need to have a full e2e tests to prove the whole thing works, thus this PR is needed |
|
Yes, the full end-to-end test is the right proof surface. I would make it prove the contract between layers, not just that the happy path returns success. The test should start from a persisted policy grant and drive one real attempted capability action through the same resolver, approval, and dispatch path the product will use. The assertion should be that the final decision record can explain each dimension separately: subject, scope, capability, config, approval mode, and dispatch result. The important negative cases are where the E2E test earns its keep:
That keeps the PR boundary clean. The durable delta store proves policy input. The resolver proves dimension separation. The approval path proves fail-closed authority. The dispatch path proves the action cannot consume a grant for the wrong subject or scope. Runtime availability can be layered later without changing what the policy grant means. Boundary: architecture and regression-test feedback only; no claim about running this branch, validating implementation behavior, implementation correctness, merge readiness, security review, production readiness, partnership, customer interest, official alignment, NearAI usage, Ironclaw usage, conformance certification, or Neura usage. |
Part of epic #5261. Builds on #5262 (main + the
ironclaw_capability_policycrate); independent of #4544 (the scoped-lifecycle admin store / control plane).What this contains
FilesystemCapabilityPolicyDeltaStore(libSQL/filesystem-backed) plus its contract test, behind the storage crate's newironclaw_capability_policydependency.StoreBackedPolicyResolver— wired throughcapability_policy_engine.rs(local_dev_capability_policy_delta_storebuilds the single shared delta store;build_capability_policy_resolverbuilds the resolver over it).PolicyResolverConfigSource(configuration dimension, fail-open) andPolicyResolverAdminApprovalSource(approval dimension, fail-safe) wired throughfactory.rs+runtime/local_dev.rs.Scope boundaries
capability_surface_policy, nocapability_admin_routes/capability_user_policy_routes/local_user_directorymodules. The branch compiles and its tests pass without any feat(reborn): scoped-lifecycle admin install store (#3288) — availability foundation for #5261 #4544 file present. The acting-principal derivation the config seam needs is inlined into the engine so it carries no dependency on the availability surface resolver.AllowAllon this branch (runtime.rsunchanged) and lands in a separate availability PR; the control plane (admin/user REST surface) is a separate PR.Gating
Gated by the
capability-policycargo feature plus theIRONCLAW_REBORN_CAPABILITY_POLICYenv toggle. Both default off, so substrate-only / non-policy builds keep prior behaviour and carry noironclaw_product_workflow_storagedependency.Gates run
cargo fmt --check— cleancargo clippy -p ironclaw_capability_policy --all-features --tests -- -D warnings— cleancargo clippy -p ironclaw_product_workflow_storage --all-features --tests -- -D warnings— cleancargo clippy -p ironclaw_reborn_composition --features capability-policy,webui-v2-beta --tests -- -D warnings— cleancargo clippy -p ironclaw_reborn_composition --tests -- -D warnings(feature off) — cleancargo test -p ironclaw_product_workflow_storage --features libsql --test durable_capability_policy_delta_contract— 1 passedcargo test -p ironclaw_reborn_composition --features capability-policy,webui-v2-beta --lib product_auth_runtime_credentials— 44 passedcargo test -p ironclaw_reborn_composition --features capability-policy,webui-v2-beta --lib profile_approval— 30 passed🤖 Generated with Claude Code