Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds a new prune planner to the CLI to facilitate the cleanup of legacy policy objects after they have been migrated to namespaced policies. It provides a structured way to determine if objects can be safely deleted, are blocked by existing usage, or require further manual review. The changes include new models for prune planning, a dedicated planner implementation, and comprehensive test coverage to ensure accurate classification of prune statuses. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The legacy code must fade away, To clear the path for a new day. With plans to prune and status set, We clean the mess, we have no regret. Footnotes
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a prune-planning subsystem for namespaced policy migrations: new prune plan types, a PrunePlanner with scope-specific classification logic, refactors Planner.Plan to separate resolve/finalize, and moves/adds small utility helpers; includes extensive tests for prune classification and edge cases. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PrunePlanner
participant Planner
participant PolicyClient
participant DB as "Policy DB"
Client->>PrunePlanner: Plan(ctx)
PrunePlanner->>PrunePlanner: Validate scopes (single scope)
alt Legacy-object scopes (Actions / ConditionSets)
PrunePlanner->>PolicyClient: Retrieve legacy objects
PolicyClient->>DB: Fetch objects
DB-->>PolicyClient: Objects
PolicyClient-->>PrunePlanner: Objects
PrunePlanner->>PolicyClient: List subject mappings/resources/triggers
PolicyClient->>DB: Scan usage relationships
DB-->>PolicyClient: Usage data
PolicyClient-->>PrunePlanner: Usage data
PrunePlanner->>PrunePlanner: Determine in-use status
PrunePlanner->>PrunePlanner: Enumerate migrated targets by namespace
PrunePlanner->>PrunePlanner: Classify each source (Delete / Blocked / Unresolved)
else Resolved-object scopes (Mappings / Resources / Triggers)
PrunePlanner->>Planner: Request resolved targets
Planner->>PolicyClient: Resolve + retrieve targets
PolicyClient->>DB: Fetch canonical objects
DB-->>PolicyClient: Objects
PolicyClient-->>Planner: Resolved targets
Planner-->>PrunePlanner: ResolvedTargets
PrunePlanner->>PrunePlanner: Map resolved targets to prune status
PrunePlanner->>PrunePlanner: Check migrated-from labels and canonical equality
PrunePlanner->>PrunePlanner: Classify (Delete / Blocked / Unresolved)
end
PrunePlanner-->>Client: Return PrunePlan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 18 minutes and 18 seconds.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 a pruning framework for legacy policy objects, adding the PrunePlanner and PrunePlan components to determine if migrated objects can be safely deleted. The implementation covers actions, subject condition sets, mappings, registered resources, and obligation triggers, and includes a refactor of the Planner to support these checks. Feedback indicates that defensive nil checks in the PrunePlanner.Plan method may be redundant and could be removed to simplify the code.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@otdfctl/migrations/namespacedpolicy/prune_planner.go`:
- Around line 670-678: The function singleMigratedTarget constructs a TargetRef
using namespace.GetId() and namespace.GetFqn() without checking for nil; update
singleMigratedTarget to guard against a nil namespace: if namespace is nil
return an empty TargetRef (or a TargetRef containing only ID as appropriate) or
construct a TargetRef with empty NamespaceID/NamespaceFQN when namespace is nil,
otherwise use namespace.GetId() and namespace.GetFqn(); ensure you reference the
TargetRef return and the singleMigratedTarget function so callers still receive
a valid, non-panicking result.
- Around line 562-585: The current matchedActionTargets function (and the
analogous function around lines 597-620) sets labelsMatch true if any canonical
target has a matching migrated_from label and also breaks out of the namespace
loop on the first canonical match, which can mask other mismatched/missing
labels; change the logic so that you evaluate all canonical matches instead of
breaking early and compute labelsMatch as "all canonical matches have
migrated_from == source.GetId()" (e.g., initialize labelsMatch = true and flip
to false if any canonical target's migratedFromID != source.GetId(), or collect
canonical matches and compare counts), remove the premature break so every
canonical target in actionsByNamespace[namespace.GetId()] is examined, and apply
the same fix to the other function referenced (the block at ~597-620) to ensure
correct delete/classify behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 417f5cee-9b5a-4d50-aa7e-abdb4b6b4b1c
📒 Files selected for processing (7)
otdfctl/migrations/namespacedpolicy/plan_utils.gootdfctl/migrations/namespacedpolicy/planner.gootdfctl/migrations/namespacedpolicy/prune_plan.gootdfctl/migrations/namespacedpolicy/prune_planner.gootdfctl/migrations/namespacedpolicy/prune_planner_test.gootdfctl/migrations/namespacedpolicy/resolved.gootdfctl/migrations/namespacedpolicy/retrieve.go
💤 Files with no reviewable changes (1)
- otdfctl/migrations/namespacedpolicy/retrieve.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
otdfctl/migrations/namespacedpolicy/prune_planner.go (1)
562-580:⚠️ Potential issue | 🟠 MajorInspect every canonical match in the namespace before classifying.
These loops still stop at the first canonical match. If a namespace contains multiple canonical duplicates, a later target with the correct
migrated_fromlabel is never seen, so the source can be classifiedunresolvedincorrectly and the returned target context is incomplete.Suggested fix
func matchedActionTargets(source *policy.Action, targetNamespaces []*policy.Namespace, actionsByNamespace map[string][]*policy.Action) ([]TargetRef, bool, bool, error) { targets := make([]TargetRef, 0) foundCanonical := false labelsMatch := false for _, namespace := range targetNamespaces { for _, target := range actionsByNamespace[namespace.GetId()] { if target == nil || !actionCanonicalEqual(source, target) { continue } if target.GetId() == "" { return nil, false, false, fmt.Errorf("%w: migrated target for source %q has empty id", ErrInvalidPruneResolvedTarget, source.GetId()) } foundCanonical = true targets = append(targets, singleMigratedTarget(target.GetId(), namespace)) if migratedFromID(target) == source.GetId() { labelsMatch = true } - break } } return targets, foundCanonical, labelsMatch, nil } func matchedSubjectConditionSetTargets(source *policy.SubjectConditionSet, targetNamespaces []*policy.Namespace, scsByNamespace map[string][]*policy.SubjectConditionSet) ([]TargetRef, bool, bool, error) { targets := make([]TargetRef, 0) foundCanonical := false labelsMatch := false for _, namespace := range targetNamespaces { for _, target := range scsByNamespace[namespace.GetId()] { if target == nil || !subjectConditionSetCanonicalEqual(source, target) { continue } if target.GetId() == "" { return nil, false, false, fmt.Errorf("%w: migrated target for source %q has empty id", ErrInvalidPruneResolvedTarget, source.GetId()) } foundCanonical = true targets = append(targets, singleMigratedTarget(target.GetId(), namespace)) if migratedFromID(target) == source.GetId() { labelsMatch = true } - break } } return targets, foundCanonical, labelsMatch, nil }Also applies to: 597-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/prune_planner.go` around lines 562 - 580, The loops in matchedActionTargets stop at the first canonical match, which misses other canonical targets (and any with the migrated_from label); remove the early break so you inspect every target in a namespace: for each target in actionsByNamespace[namespace.GetId()] if actionCanonicalEqual(source, target) then validate id, set foundCanonical=true, append singleMigratedTarget(target.GetId(), namespace) and set labelsMatch=true if migratedFromID(target)==source.GetId(); do not break so later canonical matches are collected. Apply the identical change to the second, similar loop block (the duplicate logic later in the file) so both places collect all canonical duplicates instead of exiting after the first match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@otdfctl/migrations/namespacedpolicy/prune_planner.go`:
- Line 16: The prune reason string
pruneStatusReasonMessageMigrationLabelNotFound currently covers both missing and
mismatched migrated_from labels; update the code to distinguish these cases by
adding a new constant (e.g., pruneStatusReasonMessageMigrationLabelMismatch =
"migrated target has wrong migration label") and use migratedFromID(target) !=
"" vs migratedFromID(target) == "" checks around the compare with sourceID to
emit either the existing "missing migration label" message or the new "wrong
migration label" message; update all usages (including where
pruneStatusReasonMessageMigrationLabelNotFound is currently used at the other
occurrences you noted) so operators see correct structured prune output.
---
Duplicate comments:
In `@otdfctl/migrations/namespacedpolicy/prune_planner.go`:
- Around line 562-580: The loops in matchedActionTargets stop at the first
canonical match, which misses other canonical targets (and any with the
migrated_from label); remove the early break so you inspect every target in a
namespace: for each target in actionsByNamespace[namespace.GetId()] if
actionCanonicalEqual(source, target) then validate id, set foundCanonical=true,
append singleMigratedTarget(target.GetId(), namespace) and set labelsMatch=true
if migratedFromID(target)==source.GetId(); do not break so later canonical
matches are collected. Apply the identical change to the second, similar loop
block (the duplicate logic later in the file) so both places collect all
canonical duplicates instead of exiting after the first match.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce06b228-8753-43ed-b1cc-6c37ad8dadea
📒 Files selected for processing (1)
otdfctl/migrations/namespacedpolicy/prune_planner.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
2 participants
Summary
Add a new prune planner for namespaced policy migration cleanup.
This branch introduces prune planning for:
It also exposes resolved migration state from the existing planner so prune logic can reuse it where appropriate.
Details
Supporting refactors
Summary by CodeRabbit
New Features
Refactor
Tests