feat(cli): Interactive review for RR. - #3317
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 enhances the namespaced policy migration tool by adding an interactive review capability. When the planner encounters registered resources with conflicting namespace bindings, it now pauses to prompt the user for a target namespace. This ensures that the final migration plan is clean and actionable by resolving ambiguities at planner-time rather than failing or requiring manual intervention later. 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 planner pauses, choices wait, To fix the namespace, seal the fate. With interactive prompt in hand, We guide the resources to the land. Footnotes
|
![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 an interactive review mode for namespaced policy migration, allowing users to resolve conflicts when registered resources span multiple target namespaces. It adds a new InteractiveReviewer interface, a charmbracelet/huh based implementation for terminal prompts, and integrates this flow into the migration planner. Feedback includes a critical bug fix in the resource filtering logic where values were being cleared before iteration, a suggestion to cache network calls within the review loop to improve performance, and a recommendation to use native library methods for rendering prompt descriptions.
|
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 |
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:
|
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an interactive review workflow for the namespaced-policy migration command, enabling users to manually resolve conflicts when registered resources are associated with multiple namespaces. Key additions include an InteractiveReviewer interface, a concrete implementation using the huh library for terminal prompts, and integration into the migration planner. Review feedback highlights opportunities to improve code safety by properly initializing internal maps in the resolver struct, clarifying misleading error messages, and replacing manual field copying in cloneAction with a more robust cloning strategy.
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: 6
🤖 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/interactive_prompt.go`:
- Around line 45-82: Replace the string-select confirm UI in HuhPrompter.Confirm
with huh.NewConfirm: change the local choice variable from string to bool, build
the form with
huh.NewConfirm().Title(...).Description(...).Affirmative(confirmLabel).Negative(cancelLabel).Value(&choice)
(instead of huh.NewSelect), run the form the same way, and replace the
label-equality check (choice != confirmLabel) with a boolean check (if !choice {
return ErrInteractiveReviewAborted }). Keep the existing error handling around
form.Run() and the defaulting of confirmLabel/cancelLabel.
- Around line 45-113: The Confirm and Select methods currently ignore their
context parameter (signature uses unnamed _), and call form.Run(), which blocks;
rename the parameter to ctx context.Context in both HuhPrompter.Confirm and
HuhPrompter.Select and replace form.Run() with form.RunWithContext(ctx) so the
huh.Form honors cancellation/timeouts (references: HuhPrompter.Confirm,
HuhPrompter.Select, form.Run -> form.RunWithContext).
In `@otdfctl/migrations/namespacedpolicy/interactive_review.go`:
- Around line 31-43: The HuhInteractiveReviewer struct exports Prompter while
handler and pageSize are unexported—make Prompter unexported to be consistent
and only set via NewHuhInteractiveReviewer: change the field name to prompter
(lowercase) and update NewHuhInteractiveReviewer to set prompter accordingly;
also update any usage sites and the existing prompter() receiver (rename to
resolvePrompter() or inline its nil-check at call sites) so there are no
identifier collisions and the nil-check logic still runs.
- Around line 119-137: Add a one-line comment above the reset of
resource.Unresolved, resource.AlreadyMigrated, and resource.NeedsCreate
explaining that we intentionally clear those fields and then call
resolveExistingRegisteredResource (using registeredResources[chosen.GetId()]) to
re-resolve the resource for the newly chosen namespace
(filtered.GetId()/chosen.GetId()), matching the planner semantics implemented in
resolver.resolveRegisteredResource so the subsequent AlreadyMigrated/NeedsCreate
computation is correct.
- Around line 62-118: The reviewRegisteredResource function currently calls
retriever.listActionsForNamespaces and
retriever.listRegisteredResourcesForNamespaces and builds a new resolver every
time for the same chosen namespace; to fix, add a per-namespace cache (keyed by
chosen.GetId()) in the higher-level Review loop or in the reviewer struct and
use it from reviewRegisteredResource so you fetch and store for each namespace
only once per Review invocation: on first encounter call
listActionsForNamespaces/listRegisteredResourcesForNamespaces and save the
resulting customActions, standardActions and registeredResources (and the
constructed resolver) in the cache, and on subsequent calls for the same chosen
namespace reuse those cached values instead of re-calling the retriever or
rebuilding resolver (refer to functions reviewRegisteredResource,
retriever.listActionsForNamespaces,
retriever.listRegisteredResourcesForNamespaces, and resolver).
- Around line 194-221: The prompt builds an options slice in
registeredResourceConflictPrompt that appends the abort option but the consumer
later iterates a separate candidates list (not containing abort) and relies on
the selected value equality check (selected == interactiveReviewAbortOption) to
short-circuit; add a brief comment immediately above the abort-check in the
interactive review handler (the block that checks selected ==
interactiveReviewAbortOption before calling selectedNamespace) stating that the
abort option is appended to options and that the candidates list excludes it, so
the equality check must remain to correctly short-circuit — reference
registeredResourceConflictPrompt, SelectPrompt/options, candidates,
selectedNamespace, and interactiveReviewAbortOption.
🪄 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: 33e6538f-d39a-4551-a157-04937c350f36
📒 Files selected for processing (7)
otdfctl/cmd/migrate/namespaced_policy.gootdfctl/migrations/namespacedpolicy/execute.gootdfctl/migrations/namespacedpolicy/interactive_prompt.gootdfctl/migrations/namespacedpolicy/interactive_review.gootdfctl/migrations/namespacedpolicy/interactive_review_test.gootdfctl/migrations/namespacedpolicy/planner.gootdfctl/migrations/namespacedpolicy/planner_test.go
Invalidated by push of d8743f0
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
## 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 -->
## 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 Resources` that are unresolved. [RR Interactive](#3317) ## What Changed - Removed top-level Unresolved handling from actions, subject condition sets, subject mappings, and obligation triggers. - Kept registered resources as the only planner construct with soft unresolved behavior. - Added a typed RR unresolved model with: - Reason - Message - Preserved the existing artifact shape by continuing to emit the unresolved message as a string in the finalized plan. - Simplified action, subject condition set, and subject mapping resolver guard logic so invalid derived entries fail fast in the parent resolver. - Removed stale unresolved dependency/result handling from subject mapping dependency resolution. - Skipped registered resources with no AAVs during derivation and documented that behavior inline. ## 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: - non-RR derivation/resolution failures are hard failures - RR namespace conflicts remain the only reviewable unresolved case - tests can now assert a typed unresolved reason instead of brittle strings
2 participants
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:
Prompt updates
The interactive prompt was also cleaned up so that:
Scope
This change is intentionally limited to registered resource conflict review only.
It does not add:
Summary by CodeRabbit
Release Notes
New Features
--interactiveflag. 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