feat(cli): Cleanup planner, handle unresolved RRs interactively.#3311
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. Comment |
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 refines the 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. A planner's path, once soft and wide, Now narrows, errors can't abide. Except for resources, unique and grand, With reasons clear, across the land. Go's strictness guides, a cleaner hand. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the migration planning logic to handle resolution failures via error propagation instead of storing unresolved status strings in the plan structures. This change applies to Actions, Subject Condition Sets, Subject Mappings, and Obligation Triggers, while Registered Resources now use a structured Unresolved type for specific conflict cases. Feedback includes correcting a typo in a new error constant, improving error message context with namespace IDs, and removing a development comment.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
402a472 to
16b705a
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
73dba6c to
6475ea0
Compare
Invalidated by push of 46251d0
## Summary This PR adds planner-time interactive review for registered resources that span multiple namespaces during namespaced policy migration. When --interactive is enabled, the planner now: - detects conflicting registered resources, - prompts the user to choose the target namespace, - filters the resource down to bindings for that namespace, - updates any required action resolution so the final plan can proceed cleanly. ## Prompt updates The interactive prompt was also cleaned up so that: - titles and descriptions render separately, - the registered resource identifier is shown in the title, - namespace options read clearly as migration choices. ## Scope This change is intentionally limited to registered resource conflict review only. It does not add: - broader planner summaries, - other interactive resolution flows, - non-RR interactive review behavior. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Interactive migration mode now available via `--interactive` flag. When enabled, users are prompted to manually review and resolve namespace conflicts for policy resources during migration. Users can select the appropriate destination namespace for each affected resource, with clear feedback on migration status. * **Tests** * Added comprehensive test coverage for interactive migration functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
46251d0 to
78f6276
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Summary
This PR tightens the namespacedpolicy planner so registered resources are the only construct that can remain soft-unresolved during planning. All other policy constructs now fail fast when their target namespace or required dependencies cannot be derived.
It also cleans up resolver guard behavior so parent resolver functions own nil/source validation, while child helpers focus on namespace and existing-target resolution. For registered resources, unresolved state is now represented internally as a typed object
with a machine-checkable reason code plus a human-readable message.
In addition, we have merged in changes for interactively handling
Registered Resourcesthat are unresolved. RR InteractiveWhat Changed
Why
The planner had accumulated multiple soft-unresolved paths that no longer matched the intended contract. This made the resolution flow harder to reason about and made tests rely on string matching for unresolved behavior.
This change makes the model stricter and simpler: