ActionHandler⟷RBAC⟷orchestration spine: §4 trait + authorize_scoped + commit_via + OgarRbac (F1–F4)#601
Merged
Conversation
The lance-graph half of the integration plan, 5+3-hardened then built one-agent-per-file (commented drafts, central uncomment+reconcile+compile). F1 contract::rbac — §4 ClassRbac default methods (roles_reaching/row_scope/ field_mask) + ScopeSpec (axis-3 Copy token) + ScopeSpec::intersect. All DEFAULT so ClassGrants + PROBE-OGAR-RBAC-AUTHORIZE compile/pass unchanged. Reuses contract::class_view::FieldMask (added FieldMask::union for the projection-fold). roles_reaching is a default-empty CONJECTURE hook (axis-2 hierarchy not implemented this PR). F2 lance-graph-rbac::authorize_scoped + ScopedDecision — the §5 two-stage: stage-1 reuses authorize() unchanged; stage-2 folds the granting subset (actor_roles AND grant_permits, NOT roles_reaching) into a restrictive-AND row-scope + union field-mask. AccessDecision frozen (3 variants incl Escalate). F3 contract::action::ActionInvocation::commit_via<R: ClassRbac> — the ClassRbac convergence of commit's inline ActorContext gate. commit untouched. PINNED: no is_admin bypass (admin must be a granted role); ActorId(&str) not ActorContext; None required_role => proceed (parity). F4 lance-graph-ogar::OgarRbac<S: GrantSource> — keystone Q5 as a LOCAL newtype (impl ClassRbac for OgarClassView would be an orphan-rule E0117). Owns no grant data; reads from an injected GrantSource — the §6 project_role.granted evaporation seam (body unchanged when the fixture flips to the tenant resolver). Tests: contract 735, rbac 23 (probe green), ogar rbac_impl 3. clippy+fmt clean. Deferred (OGAR repo): MARS class mint + gen_statem->ActionDef lift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYvNjD8M8LMNYbRy3gq2FP
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 56 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds the ActionHandler ↔ RBAC ↔ Orchestration integration spine: extends ChangesActionHandler ↔ RBAC ↔ Orchestration Integration Spine
Sequence Diagram(s)sequenceDiagram
participant Caller
participant authorize_scoped
participant authorize
participant ClassRbac
rect rgba(70, 130, 180, 0.5)
Note over Caller, ClassRbac: Stage 1 — Gate decision
Caller->>authorize_scoped: actor, class, Operation::Act
authorize_scoped->>authorize: gate check
authorize-->>authorize_scoped: AccessDecision
end
rect rgba(180, 70, 70, 0.5)
Note over authorize_scoped, ClassRbac: Stage 2 — Scope/mask fold (Allow only)
alt non-Allow
authorize_scoped-->>Caller: ScopedDecision{Deny, scope:None, mask:FULL}
else Allow
loop each actor role where grant_permits
authorize_scoped->>ClassRbac: row_scope(role, class) → AND-intersect
authorize_scoped->>ClassRbac: field_mask(role, class) → OR-union
end
authorize_scoped-->>Caller: ScopedDecision{Allow, scope, field_mask}
end
end
sequenceDiagram
participant Executor
participant ActionInvocation
participant OgarRbac
participant GrantSource
Executor->>ActionInvocation: commit_via(def, rbac, actor_id, impact, guard, now_ms)
ActionInvocation->>ActionInvocation: sticky terminal guard
ActionInvocation->>ActionInvocation: def predicate/class match
ActionInvocation->>OgarRbac: actor_roles(actor_id)
OgarRbac->>GrantSource: roles_of(actor_id)
GrantSource-->>OgarRbac: &[RoleId]
OgarRbac-->>ActionInvocation: &[RoleId]
ActionInvocation->>OgarRbac: grant_permits(role, class, Act{predicate})
OgarRbac->>GrantSource: grants_of(role) → grants_permit
OgarRbac-->>ActionInvocation: bool
ActionInvocation->>ActionInvocation: optional guard field check
ActionInvocation->>ActionInvocation: map GateDecision → ActionState
ActionInvocation-->>Executor: Flow / Pending / Cancelled / Failed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.claude/board/INTEGRATION_PLANS.md (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatus field should be "Active" for an in-PR plan, not "shipped".
Line 13 sets Status to "shipped", but the PR has not merged yet. Comparing to other in-flight plans in this index (e.g., lines 758: "Status:** Active (draft, pre-execution)", line 852: "Status:** Active"), the convention is to use "Active" for code that is ready but not yet merged. Once the PR merges and code is live, the status changes to "Shipped". The notation "(PLAN; shipped)" may have been intended as "plan document ready", but to avoid ambiguity with the standard Status vocabulary, recommend changing to "Active".
🤖 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 @.claude/board/INTEGRATION_PLANS.md around lines 1 - 14, The status field in the integration plan header "integration-actionhandler-rbac-orchestration-v1" is marked as "(PLAN; shipped)" but should reflect that this plan is still in-flight since the PR has not yet merged. Change the status notation from "(PLAN; shipped)" to "(PLAN; Active)" to align with the established convention in the file where "Active" indicates code ready but not yet merged (as seen in other in-flight plans), and "Shipped" is reserved for code that is live in production..claude/plans/integration-actionhandler-rbac-orchestration-v1.md (1)
245-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueActivation steps should mention LATEST_STATE.md update.
Line 249 says "prepend INTEGRATION_PLANS.md + a PR_ARC entry in the SAME commit", but File 3 (.claude/board/LATEST_STATE.md) also receives a new entry at lines 13–16. For clarity, the activation steps should note that LATEST_STATE.md is updated in the same commit. This aligns with the append-only rule and the governance model described in INTEGRATION_PLANS.md § "How to use this file."
🤖 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 @.claude/plans/integration-actionhandler-rbac-orchestration-v1.md around lines 245 - 250, The Activation section's board-hygiene step currently only mentions prepending INTEGRATION_PLANS.md and a PR_ARC entry in the same commit, but omits mention of LATEST_STATE.md which also receives a new entry. Add a clarification to the board-hygiene step (line 249) to explicitly note that LATEST_STATE.md is updated in the same commit, ensuring all three file updates (INTEGRATION_PLANS.md, PR_ARC entry, and LATEST_STATE.md append) are documented together to align with the governance model and append-only rule.crates/lance-graph-contract/src/class_view.rs (1)
137-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd local coverage for the new mask primitive.
unionis now a contract-level primitive; please add a small#[cfg(test)]case in this module for overlap/identity behavior instead of relying only on RBAC-side usage.Example focused coverage
+#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn field_mask_union_or_combines_positions() { + let left = FieldMask::from_positions(&[0, 2]); + let right = FieldMask::from_positions(&[2, 3]); + + let union = left.union(right); + + assert!(union.has(0)); + assert!(union.has(2)); + assert!(union.has(3)); + assert_eq!(union.count(), 3); + assert_eq!(union.union(FieldMask::EMPTY), union); + } +}As per coding guidelines,
crates/**/*.rs: “Add Rust unit tests alongside implementations via#[cfg(test)]modules.”🤖 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/lance-graph-contract/src/class_view.rs` around lines 137 - 144, The union method on the mask primitive needs local unit test coverage to verify its behavior. Add a #[cfg(test)] module at the end of the class_view.rs file that tests the union method with multiple test cases covering overlap behavior (two masks with overlapping bits should produce a mask containing all bits from both) and identity behavior (union with an empty mask should return the original mask, and union of identical masks should return that same mask).Source: Coding guidelines
🤖 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 @.claude/plans/integration-actionhandler-rbac-orchestration-v1.md:
- Around line 123-136: The hardening section (F1 and F2 in the plan document)
contradicts the canonical signatures defined later in the same document. The F1
section currently states to "Drop the ScopeSpec new type" but the canonical
signature at lines 196-197 explicitly defines ScopeSpec with fields tenant:
Option<u64> and predicate_key: u32, and the actual implementation uses it
throughout. Similarly, F2 currently states scope: Option<TenantId> but the
canonical signature at line 205 specifies scope: Option<ScopeSpec>. Correct the
hardening text to match the canonical signatures: update F1 to describe ADDING
ScopeSpec (not dropping it) with the exact structure { tenant: Option<u64>,
predicate_key: u32 }, and update F2 to specify scope: Option<ScopeSpec> instead
of Option<TenantId> in the ScopedDecision struct definition.
In `@crates/lance-graph-contract/src/action.rs`:
- Around line 347-356: The role authorization check in the block around line
347-356 only uses `def.required_role` as a boolean gate to decide whether to
perform a check, but does not validate the actor against the actual specific
role value contained in `def.required_role`. Instead of checking if any actor
role permits the Act operation, extract the actual required role from
`def.required_role` and verify that the actor's roles include that specific
required role, similar to how the `commit` function performs its specific-role
authorization check.
In `@crates/lance-graph-contract/src/rbac.rs`:
- Around line 50-67: The intersect method in the Scope struct currently has
incorrect logic for handling conflicting tenants. When both self and other have
different tenant values (Some(a) and Some(b) where a != b), the method returns
Some(a) instead of representing the conflict/empty intersection. Fix this by
checking if the two tenant values are equal when both are Some: if they match,
return that tenant; if they differ, return None to represent the empty/conflict
scope (since a restrictive AND with conflicting tenants should result in an
empty scope). Additionally, update the test case around lines 430-445 that
currently expects the "self wins" behavior to instead verify that conflicting
tenants are properly represented as an empty/conflict state.
In `@crates/lance-graph-rbac/src/authorize.rs`:
- Around line 166-168: The issue is that unwrap_or_default() is converting None
(global scope sentinel) into Some(ScopeSpec::default()), breaking the API
contract where scope == None should indicate unrestricted global access. In the
scope aggregation logic (around the fold operation mentioned), replace the
unwrap_or_default() call with conditional logic that only intersects concrete
scopes when both are Some, preserving None as the final result when all roles
have global scope. This ensures that allowed decisions with only global roles
correctly report scope == None as documented in the ScopedDecision API.
---
Nitpick comments:
In @.claude/board/INTEGRATION_PLANS.md:
- Around line 1-14: The status field in the integration plan header
"integration-actionhandler-rbac-orchestration-v1" is marked as "(PLAN; shipped)"
but should reflect that this plan is still in-flight since the PR has not yet
merged. Change the status notation from "(PLAN; shipped)" to "(PLAN; Active)" to
align with the established convention in the file where "Active" indicates code
ready but not yet merged (as seen in other in-flight plans), and "Shipped" is
reserved for code that is live in production.
In @.claude/plans/integration-actionhandler-rbac-orchestration-v1.md:
- Around line 245-250: The Activation section's board-hygiene step currently
only mentions prepending INTEGRATION_PLANS.md and a PR_ARC entry in the same
commit, but omits mention of LATEST_STATE.md which also receives a new entry.
Add a clarification to the board-hygiene step (line 249) to explicitly note that
LATEST_STATE.md is updated in the same commit, ensuring all three file updates
(INTEGRATION_PLANS.md, PR_ARC entry, and LATEST_STATE.md append) are documented
together to align with the governance model and append-only rule.
In `@crates/lance-graph-contract/src/class_view.rs`:
- Around line 137-144: The union method on the mask primitive needs local unit
test coverage to verify its behavior. Add a #[cfg(test)] module at the end of
the class_view.rs file that tests the union method with multiple test cases
covering overlap behavior (two masks with overlapping bits should produce a mask
containing all bits from both) and identity behavior (union with an empty mask
should return the original mask, and union of identical masks should return that
same mask).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c0a6ab7-0238-409b-a7e6-3def3b9ce813
📒 Files selected for processing (9)
.claude/board/INTEGRATION_PLANS.md.claude/board/LATEST_STATE.md.claude/plans/integration-actionhandler-rbac-orchestration-v1.mdcrates/lance-graph-contract/src/action.rscrates/lance-graph-contract/src/class_view.rscrates/lance-graph-contract/src/rbac.rscrates/lance-graph-ogar/src/lib.rscrates/lance-graph-ogar/src/rbac_impl.rscrates/lance-graph-rbac/src/authorize.rs
…ope fold, required_role, plan doc)
Thread 2 (action.rs): commit_via now honors the SPECIFIC required_role value —
the actor must hold that exact role AND it must grant Act on the object class,
not merely 'any granted role'.
Thread 3 (rbac.rs ScopeSpec): add `deny: bool` so a two-tenant intersection is
the EMPTY scope, not 'self wins'. Self-wins silently widened row visibility to a
tenant the other granting role never authorized. `deny` is the absorbing element
of intersect; ScopeSpec::DENY is the canonical empty scope. CodeRabbit's suggested
None-for-conflict was unsafe — None=global would widen, not restrict.
Thread 4 (authorize.rs fold): intersect only CONCRETE Some row-scopes; a None
(global) role no longer materializes as ScopeSpec::default() and forces the Some
branch. The None sentinel ('no restriction') is preserved through the fold.
Thread 1 (plan doc): the v2 hardening prose said 'drop ScopeSpec, use
Option<TenantId>' contradicting the canonical signatures block (which kept
ScopeSpec). Corrected F1/F2 prose + added the deny field to the canonical block.
contract 735, rbac 23 green; fmt + clippy -D warnings clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EYvNjD8M8LMNYbRy3gq2FP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the ActionHandler ⟷ RBAC ⟷ orchestration chain on the lance-graph side. Built from
.claude/plans/integration-actionhandler-rbac-orchestration-v1.md— 5+3-hardened (5 research savants + 3 brutally-honest reviewers reviewed the plan; every P0 they caught is fixed) then implemented one Sonnet agent per file (commented drafts → central uncomment + reconcile + compile).What's here (F1–F4)
contract::rbac— the §4ClassRbacsurface:roles_reaching/row_scope/field_maskas default methods (soClassGrants+ the greenPROBE-OGAR-RBAC-AUTHORIZEcompile/pass unchanged) +ScopeSpec(axis-3Copytoken) with a restrictiveintersect. Reuses the existingclass_view::FieldMask(addedFieldMask::unionfor the projection-fold).roles_reachingis a default-empty CONJECTURE hook (axis-2 role-hierarchy not implemented this PR).lance-graph-rbac::authorize_scoped+ScopedDecision— the §5 two-stage: stage-1 reusesauthorize()unchanged; stage-2 folds the granting subset (actor_roles ∧ grant_permits, notroles_reaching) into a restrictive-AND row-scope + union field-mask.AccessDecisionis frozen (3 variants inclEscalate).ActionInvocation::commit_via<R: ClassRbac>— the ClassRbac convergence ofcommit's inlineActorContextgate.commituntouched. Pinned: nois_adminbypass (admin must be a granted role),ActorId(&str) notActorContext,Nonerequired_role ⇒ proceed (parity).lance-graph-ogar::OgarRbac<S: GrantSource>— keystone Q5 as a local newtype (impl ClassRbac for OgarClassViewwould be an orphan-rule E0117). Owns no grant data; reads from an injectedGrantSource— the §6project_role.grantedevaporation seam (body unchanged when the fixture flips to the tenant resolver).Companion
F5
run_cycle(the end-to-end spine) + F6dispatch_viaare in rs-graph-llm (depends oncommit_via): AdaWorldAPI/rs-graph-llm PR.Tests / gates
contract735 ·rbac23 (probe green) ·ogarrbac_impl 3 — all green; clippy-D warnings+ fmt clean on the touched crates.Deferred (OGAR repo, noted in the plan)
MARS class mint in
ogar_vocab::class_ids+ thegen_statem → ActionDefbehavioral lift (the threeogar-from-elixirtodo!()s). The spine is generic over the trait, so MARS plugs in unchanged.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation