fix(gooddata-eval): check internal_recipients in alert recipients comparison - #1702
fix(gooddata-eval): check internal_recipients in alert recipients comparison#1702Tomkess wants to merge 3 commits into
Conversation
…parison create_metric_alert addresses a notification one of two ways: `recipients`/ `external_recipients` (raw email addresses) when the channel can send externally, or `internal_recipients` (internal GoodData user ids, never emails) when the channel is restricted to workspace-registered users. _check_recipients only ever read recipients/external_recipients, so any alert delivered the internal way always failed this check regardless of what the fixture expected -- confirmed live: a real, correctly-delivered alert with internal_recipients=['user.<uuid>'] still scored recipients_correct=False, because the code was comparing against a key that's never populated for that delivery path. Resolves the expected email to its internal user id via the Users entities API (GET /entities/users?filter=email==...), lazily -- only when the plain comparison already failed and internal_recipients is actually present, so no unconditional network call is added to the hot path (existing run_agentic_alert_skill tests never mock GoodDataSdk, only ChatClient). Same shape of gap as #1699 (alert_proposals as a confirmation signal): gooddata-eval's evaluator hadn't been taught to read a real tool-response shape yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAlert recipient validation now supports external email matches and internal GoodData user IDs. The evaluator passes the SDK for ID resolution. Tests cover matches, mismatches, missing SDKs, and lookup failures. ChangesAlert recipient validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 1
🤖 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 `@packages/gooddata-eval/tests/test_agentic_alert_skill.py`:
- Around line 68-72: Update
test_check_recipients_matches_external_recipients_without_sdk to pass a mock SDK
object, then assert its get_all_entities_users method was not called while
retaining the direct recipient-match assertion, so the fast path verifies no
user lookup occurs.
🪄 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: 3daa5727-084a-4e5d-a0f0-6041480bac56
📒 Files selected for processing (2)
packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.pypackages/gooddata-eval/tests/test_agentic_alert_skill.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1702 +/- ##
==========================================
+ Coverage 78.30% 78.35% +0.05%
==========================================
Files 271 271
Lines 18689 18705 +16
==========================================
+ Hits 14634 14656 +22
+ Misses 4055 4049 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CodeRabbit review: without an sdk arg, the test couldn't catch a regression where a Users lookup runs before the direct recipient match. Pass a mock sdk and assert get_all_entities_users is not called.
CI's format-check job was failing since these files predated the project's line-length config. Reformat to match.
1 participant
Summary
create_metric_alertaddresses a notification one of two ways:recipients/external_recipients— raw email addresses, when the channel can send externally.internal_recipients— internal GoodData user ids (never emails), when the channel is restricted to workspace-registered users._check_recipientsonly ever readsrecipients/external_recipients. Any alert delivered the internal way always fails this check, regardless of what the fixture expects, because the code compares against a key that's never populated for that delivery path.Confirmed live against a real workspace whose email channel only allows internal users: a real, correctly-delivered alert with
still scored
recipients_correct=False.Same category of gap as #1699 (
alert_proposalsas a confirmation signal) — the evaluator hadn't been taught to read a real tool-response shape yet.Changes
_check_recipientsgains an optionalsdkparam. When the plain email/external comparison fails andinternal_recipientsis present, it resolves the expected email(s) to internal user id(s) via the Users entities API (GET /entities/users?filter=email==...) and compares against that instead.internal_recipientsis actually present, so no unconditional network call lands on the hot path. This matters because the existingrun_agentic_alert_skilltests never mockGoodDataSdk(onlyChatClient) — an eager/unconditional lookup would have broken them.Test plan
TypeErroron the addedsdkkwarg) before the fix, pass after.gooddata-evalsuite: 247 passed, same 9 pre-existing failures onmastertoo (missingopenaiextra in this env, unrelated) — confirmed viagit stashonmaster.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit