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 improves the robustness and maintainability of the namespaced-policy migration planner by tightening unit test coverage and simplifying internal design. Key changes include removing dead code, standardizing resolver behavior, and improving error handling with typed sentinels. These updates ensure more predictable behavior during the migration pipeline and provide better diagnostic information when issues arise. 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 tests were thin and sparse of late, We tightened up the migration state. With sentinels and logic clear, The code now runs without a fear. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 refactors the namespaced policy migration logic by removing action reference tracking and simplifying the resolution process. It introduces more robust nil checks when iterating over candidates and adds comprehensive unit tests for canonical equality, target derivation, and dependency resolution. One critical issue was identified where calling GetId() on potentially nil action objects in dependency resolution functions could lead to panics.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/derived_test.go`:
- Around line 380-439: Replace brittle require.EqualError usages with typed
error assertions: in tests that assert missing target namespace (e.g., within
TestDeriveTargetsFailsWhenNamespaceRefNotFound and any other tests referencing
ErrMissingTargetNamespace), use require.ErrorIs(err, ErrMissingTargetNamespace)
to check error identity and keep assert.Contains checks for contextual messages
(like the mapping id and host fragment) instead of asserting the exact error
string; update any require.EqualError(err, ...) calls that compare to
ErrMissingTargetNamespace to this pattern.
🪄 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: b0b6cd3a-e626-43c4-9350-68ceed36d150
📒 Files selected for processing (2)
otdfctl/migrations/namespacedpolicy/derived.gootdfctl/migrations/namespacedpolicy/derived_test.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
otdfctl/migrations/namespacedpolicy/interactive_review.go (2)
417-436:⚠️ Potential issue | 🟡 MinorDead branch:
else if item.Source == nilis unreachable.
resolvedActionByID(lines 451-459) only returns entries that already satisfyaction.Source != nil && action.Source.GetId() == sourceID. So wheneveritem != nilhere,item.Sourceis guaranteed non-nil and theelse ifblock can never execute.♻️ Proposed cleanup
item := resolvedActionByID(resolved.Actions, action.GetId()) if item == nil { source, err := cloneAction(action) if err != nil { return err } item = &ResolvedAction{ Source: source, Results: make([]*ResolvedActionResult, 0, 1), } resolved.Actions = append(resolved.Actions, item) - } else if item.Source == nil { - source, err := cloneAction(action) - if err != nil { - return err - } - - item.Source = source }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/interactive_review.go` around lines 417 - 436, The else-if branch checking "item.Source == nil" in the interactive_review logic is dead because resolvedActionByID only returns entries with a non-nil Source (see resolvedActionByID), so remove the unreachable else-if block and any associated cloneAction call; keep the initial creation path that appends a new ResolvedAction (using cloneAction) when item == nil, and ensure no other code expects the dead branch to run by leaving ResolvedAction.Source initialization only in the creation branch.
410-415:⚠️ Potential issue | 🟠 MajorUse the typed sentinel errors for dependency failures.
Lines 410–415 return plain
errors.Newwhere the PR's typed sentinel errors should be used. Per the PR objective to introduce typed sentinels for dependency failures, and consistent withErrNilResolvedTargetsalready returned at line 405, replace these with:
ErrMissingActionID(defined inplan.go)ErrNilInteractiveReviewHandler(defined ininteractive_review.go)This allows callers/tests to assert these failure modes via
errors.Is, completing the typed-error refactor for the hard-error paths documented in this function.Fix
if action == nil || strings.TrimSpace(action.GetId()) == "" { - return errors.New("registered resource binding action is missing") + return ErrMissingActionID } if actionResolver == nil { - return errors.New("action resolver required for plan resolution") + return ErrNilInteractiveReviewHandler }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/interactive_review.go` around lines 410 - 415, Replace the plain errors.New returns in the plan resolution checks with the typed sentinel errors: where the code currently returns errors.New("registered resource binding action is missing") change it to return ErrMissingActionID, and where it returns errors.New("action resolver required for plan resolution") change it to return ErrNilInteractiveReviewHandler; these symbols are defined in plan.go and interactive_review.go respectively and should be used alongside the existing ErrNilResolvedTargets at the earlier check so callers can use errors.Is to detect these dependency failures.
🤖 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_review.go`:
- Around line 71-81: The doc comment for reviewRegisteredResource uses a
non-idiomatic "// ! " prefix causing literal "!" to appear in godoc; remove the
"! " prefix from each line in that comment block so all lines begin with plain
"//" and match the style used elsewhere (e.g.,
ensureRegisteredResourceActionResolution) to render properly in go
doc/pkg.go.dev.
---
Outside diff comments:
In `@otdfctl/migrations/namespacedpolicy/interactive_review.go`:
- Around line 417-436: The else-if branch checking "item.Source == nil" in the
interactive_review logic is dead because resolvedActionByID only returns entries
with a non-nil Source (see resolvedActionByID), so remove the unreachable
else-if block and any associated cloneAction call; keep the initial creation
path that appends a new ResolvedAction (using cloneAction) when item == nil, and
ensure no other code expects the dead branch to run by leaving
ResolvedAction.Source initialization only in the creation branch.
- Around line 410-415: Replace the plain errors.New returns in the plan
resolution checks with the typed sentinel errors: where the code currently
returns errors.New("registered resource binding action is missing") change it to
return ErrMissingActionID, and where it returns errors.New("action resolver
required for plan resolution") change it to return
ErrNilInteractiveReviewHandler; these symbols are defined in plan.go and
interactive_review.go respectively and should be used alongside the existing
ErrNilResolvedTargets at the earlier check so callers can use errors.Is to
detect these dependency failures.
🪄 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: 6c9e995b-7ab9-4a30-b628-297d5f66e707
📒 Files selected for processing (7)
otdfctl/e2e/migrate-namespaced-policy.batsotdfctl/migrations/namespacedpolicy/finalize_plan.gootdfctl/migrations/namespacedpolicy/finalize_plan_test.gootdfctl/migrations/namespacedpolicy/interactive_review.gootdfctl/migrations/namespacedpolicy/plan.gootdfctl/migrations/namespacedpolicy/planner.gootdfctl/migrations/namespacedpolicy/planner_test.go
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:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdfctl/migrations/namespacedpolicy/interactive_review.go (1)
410-415:⚠️ Potential issue | 🟡 MinorUse typed sentinel error
ErrMissingActionIDfor consistency with the non-interactive resolver path.The typed sentinel
ErrMissingActionIDis defined inplan.go:16and used consistently throughoutresolved.go(lines 400, 423, 436) and tested witherrors.Is()assertions in tests. Line 411 should wrap or returnErrMissingActionIDinstead of a bareerrors.New("...")to ensure callers can useerrors.Is(err, ErrMissingActionID)on errors from the interactive review flow. This maintains consistency with the non-interactive resolver path and preserves the sentinel error contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/migrations/namespacedpolicy/interactive_review.go` around lines 410 - 415, Replace the bare error returned when action ID is missing with the typed sentinel ErrMissingActionID (from plan.go) so callers can use errors.Is; specifically, in the block that checks action == nil || strings.TrimSpace(action.GetId()) == "" (and not the actionResolver check), return or wrap ErrMissingActionID (e.g. fmt.Errorf("registered resource binding action is missing: %w", ErrMissingActionID) or simply return ErrMissingActionID) and add fmt to imports if you choose to wrap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdfctl/migrations/namespacedpolicy/interactive_review.go`:
- Around line 410-415: Replace the bare error returned when action ID is missing
with the typed sentinel ErrMissingActionID (from plan.go) so callers can use
errors.Is; specifically, in the block that checks action == nil ||
strings.TrimSpace(action.GetId()) == "" (and not the actionResolver check),
return or wrap ErrMissingActionID (e.g. fmt.Errorf("registered resource binding
action is missing: %w", ErrMissingActionID) or simply return ErrMissingActionID)
and add fmt to imports if you choose to wrap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7de7af3-f0df-46fc-8db3-0f705a7d0f2c
📒 Files selected for processing (2)
otdfctl/e2e/migrate-namespaced-policy.batsotdfctl/migrations/namespacedpolicy/interactive_review.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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdfctl/e2e/migrate-namespaced-policy.bats (1)
1605-1615: 🧹 Nitpick | 🔵 TrivialTrack inline-created NS_B canonical fixtures for per-test teardown.
existing_rr_b_idandexisting_rr_b_value_idare created inline here (bypassingcreate_global_registered_resource/create_registered_resource_valuebecause they need to be namespaced), but neither is passed totrack_registered_resource_id/track_registered_resource_value_id. The same omission exists forexisting_trigger_b_idat lines 1704-1707. Other pre-existing canonical fixtures in this file (e.g.existing_sm_b_scs_id,existing_mapping_standard_idat lines 1517-1518) are tracked via theircreate_*helpers.Consequence: these rows survive per-test
teardown()and are only reaped when NS_B is deleted inteardown_file. Within a single file run, leftover state in NS_B could subtly influence later tests (state deltas stay correct since they're relative, but any fixed-count or uniqueness assumption added later would be fragile).🧹 Proposed fix for lines 1605-1615 and 1704-1707
run_otdfctl_registered_resources create --name "$rr_b_name" --namespace "$NS_B_ID" --label "test_case=registered-resources" --label "fixture=${TEST_PREFIX}-existing-rr-b" --json - assert_success existing_rr_b_id=$(echo "$output" | jq -r '.id // empty') assert_not_equal "$existing_rr_b_id" "" + track_registered_resource_id "$existing_rr_b_id" run_otdfctl_registered_resource_values create --resource "$existing_rr_b_id" --value "$rr_b_value" --action-attribute-value "$ns_b_read_action_id;$ATTR_B_VAL_1_ID" --label "test_case=registered-resources" --label "fixture=${TEST_PREFIX}-existing-rr-b-value" --json - assert_success existing_rr_b_value_id=$(echo "$output" | jq -r '.id // empty') assert_not_equal "$existing_rr_b_value_id" "" + track_registered_resource_value_id "$existing_rr_b_value_id"And analogously for the obligation trigger:
run_otdfctl_obligation_triggers create --attribute-value "$ATTR_B_VAL_1_ID" --action "$ns_b_read_action_id" --obligation-value "$obligation_b_value_id" --client-id "$trigger_b_client_id" --label "test_case=obligation-triggers" --label "fixture=${TEST_PREFIX}-existing-trigger-b" --json - assert_success existing_trigger_b_id=$(echo "$output" | jq -r '.id // empty') assert_not_equal "$existing_trigger_b_id" "" + track_obligation_trigger_id "$existing_trigger_b_id"The redundant
assert_successremovals are a minor bonus —run_otdfctl_*already asserts success internally (lines 62-65, 67-70, 82-85).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdfctl/e2e/migrate-namespaced-policy.bats` around lines 1605 - 1615, The created namespaced fixtures existing_rr_b_id and existing_rr_b_value_id (from run_otdfctl_registered_resources/create and run_otdfctl_registered_resource_values/create) aren’t being tracked for per-test teardown; after each creation (where you set existing_rr_b_id and existing_rr_b_value_id) call track_registered_resource_id "$existing_rr_b_id" and track_registered_resource_value_id "$existing_rr_b_value_id" to ensure they get cleaned up, and do the same for existing_trigger_b_id (call track_trigger_id "$existing_trigger_b_id" after its creation). Optionally remove the redundant assert_success lines since the run_otdfctl_* helpers already assert success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdfctl/e2e/migrate-namespaced-policy.bats`:
- Around line 1605-1615: The created namespaced fixtures existing_rr_b_id and
existing_rr_b_value_id (from run_otdfctl_registered_resources/create and
run_otdfctl_registered_resource_values/create) aren’t being tracked for per-test
teardown; after each creation (where you set existing_rr_b_id and
existing_rr_b_value_id) call track_registered_resource_id "$existing_rr_b_id"
and track_registered_resource_value_id "$existing_rr_b_value_id" to ensure they
get cleaned up, and do the same for existing_trigger_b_id (call track_trigger_id
"$existing_trigger_b_id" after its creation). Optionally remove the redundant
assert_success lines since the run_otdfctl_* helpers already assert success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7042dcba-a2cc-4141-92f9-10d2acb9f20f
📒 Files selected for processing (1)
otdfctl/e2e/migrate-namespaced-policy.bats
2 participants
Summary
Tighten unit-test coverage and simplify design across the namespaced-policy migration planner. Package coverage 78.2% → 80.9%, with targeted behavioral gaps closed in the planner pipeline.
Design changes
ActionReference/ActionReferenceKindaccumulator andErrDuplicateCanonicalMatchsentinel (never surfaced to users or callers).resolveExisting*matchers now return the first canonical match instead of erroring on duplicates, keeping resolver behavior consistent across all target types.ErrMissingActionID,ErrUnresolvedActionDependency, etc.) so tests can assert viaerrors.Iswhile preserving contextual wrapping.deriveTargetsnow skip nil entries instead of mixing error/skip behavior.ensureRegisteredResourceActionResolution.Test additions
canonical.go: table-driven tests for all 5*CanonicalEqualwrappers covering nil, cosmetic-equivalence (case/whitespace/order), distinguishing cases, and empty-field gating.resolved.go: first-match-wins tests; typed-error assertions for all dependency validation paths; scope-gating drops; already-migrated routing for SCS/SM/OT.derived.go: nil-skip tests for all 5 candidate types; namespace-accumulator dedupe; FQN-fallback resolution.interactive_commit.go: abort propagation test (reviewer returnsErrInteractiveReviewAbortedand stops prompting).plan.go:LookupActionTargetandLookupSubjectConditionSetTargettable-driven tests (nil, empty inputs, match, not-found).summary.go:recordTargetStatustable-driven test covering all 4 status branches plus accumulation.Coverage deltas
canonical.goresolved.goplan.gosummary.goSummary by CodeRabbit
Bug Fixes
Refactor
Tests