feat(capability-policy): per-capability default-policy source (§7)#5263
Conversation
…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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces the CapabilityDefaultPolicySource trait and its in-memory implementation StaticCapabilityDefaultPolicySource to decouple default policy lookups from capability registration, alongside a conservative global fallback policy and unit tests. The reviewer suggests optimizing the default_for method to return a reference (&CapabilityDefaultPolicy) instead of an owned value to avoid unnecessary cloning of heap-allocated fields. This optimization requires corresponding updates to the trait implementation and unit tests.
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.
| pub trait CapabilityDefaultPolicySource: Send + Sync { | ||
| /// The default policy for `capability`, or the global fallback when the | ||
| /// capability declares none. | ||
| fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy; | ||
| } |
There was a problem hiding this comment.
To avoid unnecessary cloning of CapabilityDefaultPolicy (which contains a heap-allocated serde_json::Value in its config field), consider defining default_for to return a reference &CapabilityDefaultPolicy instead of an owned value. Since the lookup is performed against a read-only source, returning a reference avoids overhead on what may be a frequent capability dispatch path.
| pub trait CapabilityDefaultPolicySource: Send + Sync { | |
| /// The default policy for `capability`, or the global fallback when the | |
| /// capability declares none. | |
| fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy; | |
| } | |
| pub trait CapabilityDefaultPolicySource: Send + Sync { | |
| /// The default policy for `capability`, or the global fallback when the | |
| /// capability declares none. | |
| fn default_for(&self, capability: &CapabilityId) -> &CapabilityDefaultPolicy; | |
| } |
References
- Prefer routing through centralized helper functions to maintain drift-resistance and avoid duplicating domain knowledge locally, even if it introduces minor performance overhead (like cloning) on non-hot paths. However, on hot paths (like frequent capability dispatch), avoiding cloning by returning references is preferred.
| impl CapabilityDefaultPolicySource for StaticCapabilityDefaultPolicySource { | ||
| fn default_for(&self, capability: &CapabilityId) -> CapabilityDefaultPolicy { | ||
| self.entries | ||
| .get(capability) | ||
| .cloned() | ||
| .unwrap_or_else(|| self.fallback.clone()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the implementation of CapabilityDefaultPolicySource to return a reference, matching the trait signature change and avoiding cloning the policy.
impl CapabilityDefaultPolicySource for StaticCapabilityDefaultPolicySource {
fn default_for(&self, capability: &CapabilityId) -> &CapabilityDefaultPolicy {
self.entries
.get(capability)
.unwrap_or(&self.fallback)
}
}| #[test] | ||
| fn default_source_returns_entry_then_fallback() { | ||
| let entry = CapabilityDefaultPolicy { | ||
| availability: Availability::Available, | ||
| identity: IdentityMode::None, | ||
| approval: PermissionMode::Allow, | ||
| config: json!({}), | ||
| }; | ||
| let source = StaticCapabilityDefaultPolicySource::new( | ||
| CapabilityDefaultPolicy::conservative_fallback(), | ||
| ) | ||
| .with_entry(cap(), entry.clone()); | ||
| assert_eq!(source.default_for(&cap()), entry); | ||
|
|
||
| let unknown = CapabilityId::new("shell.exec").expect("valid capability id"); | ||
| assert_eq!( | ||
| source.default_for(&unknown), | ||
| CapabilityDefaultPolicy::conservative_fallback() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Update the test to compare references instead of owned values, matching the updated default_for signature.
| #[test] | |
| fn default_source_returns_entry_then_fallback() { | |
| let entry = CapabilityDefaultPolicy { | |
| availability: Availability::Available, | |
| identity: IdentityMode::None, | |
| approval: PermissionMode::Allow, | |
| config: json!({}), | |
| }; | |
| let source = StaticCapabilityDefaultPolicySource::new( | |
| CapabilityDefaultPolicy::conservative_fallback(), | |
| ) | |
| .with_entry(cap(), entry.clone()); | |
| assert_eq!(source.default_for(&cap()), entry); | |
| let unknown = CapabilityId::new("shell.exec").expect("valid capability id"); | |
| assert_eq!( | |
| source.default_for(&unknown), | |
| CapabilityDefaultPolicy::conservative_fallback() | |
| ); | |
| } | |
| #[test] | |
| fn default_source_returns_entry_then_fallback() { | |
| let entry = CapabilityDefaultPolicy { | |
| availability: Availability::Available, | |
| identity: IdentityMode::None, | |
| approval: PermissionMode::Allow, | |
| config: json!({}), | |
| }; | |
| let source = StaticCapabilityDefaultPolicySource::new( | |
| CapabilityDefaultPolicy::conservative_fallback(), | |
| ) | |
| .with_entry(cap(), entry.clone()); | |
| assert_eq!(source.default_for(&cap()), &entry); | |
| let unknown = CapabilityId::new("shell.exec").expect("valid capability id"); | |
| assert_eq!( | |
| source.default_for(&unknown), | |
| &CapabilityDefaultPolicy::conservative_fallback() | |
| ); | |
| } |
References
- In tests, when setting up a state that depends on environmental factors, prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
| #[test] | ||
| fn source_default_feeds_the_fold() { | ||
| // An admin per-user grant flips the conservative hidden default to | ||
| // available for one user. | ||
| let source = StaticCapabilityDefaultPolicySource::new( | ||
| CapabilityDefaultPolicy::conservative_fallback(), | ||
| ); | ||
| let mut user = delta(PolicyScope::User { | ||
| user_id: uid("bob"), | ||
| }); | ||
| user.availability = Some(Availability::Available); | ||
| let eff = resolve_effective_policy(&source.default_for(&cap()), &[user]); | ||
| assert!(eff.available); | ||
| } |
There was a problem hiding this comment.
Update the test to pass the reference returned by default_for directly to resolve_effective_policy without borrowing it again.
| #[test] | |
| fn source_default_feeds_the_fold() { | |
| // An admin per-user grant flips the conservative hidden default to | |
| // available for one user. | |
| let source = StaticCapabilityDefaultPolicySource::new( | |
| CapabilityDefaultPolicy::conservative_fallback(), | |
| ); | |
| let mut user = delta(PolicyScope::User { | |
| user_id: uid("bob"), | |
| }); | |
| user.availability = Some(Availability::Available); | |
| let eff = resolve_effective_policy(&source.default_for(&cap()), &[user]); | |
| assert!(eff.available); | |
| } | |
| #[test] | |
| fn source_default_feeds_the_fold() { | |
| // An admin per-user grant flips the conservative hidden default to | |
| // available for one user. | |
| let source = StaticCapabilityDefaultPolicySource::new( | |
| CapabilityDefaultPolicy::conservative_fallback(), | |
| ); | |
| let mut user = delta(PolicyScope::User { | |
| user_id: uid("bob"), | |
| }); | |
| user.availability = Some(Availability::Available); | |
| let eff = resolve_effective_policy(source.default_for(&cap()), &[user]); | |
| assert!(eff.available); | |
| } |
|
🚅 Deployed to the ironclaw-pr-5263 environment in ironclaw-ci-preview
|
…§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>
Part of #5261 (epic) · continues #4628 · stacked on #5262 (base is
feat/capability-policy).Second independent slice — still depends only on
ironclaw_host_api, nodependency on #4544.
What this adds
The per-capability default-policy source (architecture doc §7), in the
ironclaw_capability_policycrate:CapabilityDefaultPolicySourcetrait —default_for(capability) -> CapabilityDefaultPolicy.StaticCapabilityDefaultPolicySource— in-memory entries over a globalfallback, seeded from manifest declarations at composition time.
CapabilityDefaultPolicy::conservative_fallback()— fail-closed default forunlisted capabilities (hidden · no credential · ask).
Why a source instead of a descriptor field
CapabilityDescriptorhas 49 construction sites, so adding adefault_policyfield would be a wide sweep. Sourcing the default by
CapabilityIdkeeps thisslice decoupled and #4544-independent; the manifest remains the source of truth.
Tests
cargo clippy -p ironclaw_capability_policy --all-features --tests -- -D warningsclean; 11 unit tests pass (3 new: entry-then-fallback lookup, the
conservative fallback shape, and the source default feeding the precedence fold).
🤖 Generated with Claude Code