Skip to content

fix(gooddata-eval): decode JSON-encoded alert recipients and require the exact recipient set - #1904

Open
myhoai wants to merge 1 commit into
masterfrom
QA-29902-alert-internal-recipients
Open

myhoai wants to merge 1 commit into
masterfrom
QA-29902-alert-internal-recipients

Conversation

@myhoai

@myhoai myhoai commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

agent_alert_skill was 0/21 on every haiku leg (anthropic and bedrock, medium and high) in nightly run 40229016049. Every run failed only recipients_correct. The agent and gen-ai were correct; the agentic checker (core/agentic/alert_skill.py) graded them wrong.

JSON-encoded recipients

  • haiku sends 'internal_recipients': '["admin"]' (a string); OpenAI models send ['admin']. The tool schema allows both (anyOf: [array of string, string, null]), and gen-ai decodes the string before calling the server (_as_list in gdc_mcp_client_factory.py).
  • The checker JSON-decoded a string external_recipients, but wrapped a string internal_recipients as one id, so ["admin"] never matched the current user admin.
  • Both fields now go through one helper, _recipient_list, that mirrors gen-ai: empty or "null" means none, a JSON list of strings is that list, anything else is a bare value. A non-string list entry now scores False instead of raising TypeError.

Exact recipient set for email-only alerts

  • When the expected recipients were emails only, one matching internal user id was enough to pass. An alert that missed a recipient, or added a stranger's id or an extra external address, still scored recipients_correct=True.
  • The email-only path now uses the same rules as the current-user ("me") path: no unexpected external address, every email not sent externally arrives as its user's id, no extra internal id. Each email is looked up on its own, as the "me" path already does, so a two-email expectation costs two user lookups instead of one.

Not changed: the single-shot scorer core/evaluators/alert_skill.py also fails "Recipient(s)": "me" for correct arguments. It reads only recipients/externalRecipients and has no current-user id. Nightly does not use it (tavern-e2e calls the agentic path), and fixing it needs the evaluator interface to carry the current user, so it is left for a separate change.

Test Plan

  • TEST_ENVS=py314 make -C packages/gooddata-eval test: 2439 passed, 1 skipped. Also with this commit applied on current master (after fix(gooddata-eval): assert alert comparison, title and dashboard #1902): 2482 passed, 1 skipped.
  • make -C packages/gooddata-eval format lint type-check: clean.
  • New tests: '["admin"]', 'admin' and ['admin'] pass for expected me; '["someone_else"]', '[]', 'null' and '' fail; JSON-string me + email; every external_recipients shape; and missing, extra and duplicate recipients on the email-only path. The JSON-string and strict email cases fail on the previous code.
  • Replay against the job logs of run 40229016049: all 84 logged haiku records (21 items x 4 legs) flip to recipients_correct=True, and no other check failed in them. The one logged gpt6luna record is unchanged. The threshold: 20 run of gpt6luna item 16 is not in the log, but the threshold check is untouched and still fails threshold=20.

Risk

nonprod. This changes only the eval package's scoring. Items that expect "me" (all of agent_alert_skill) now pass when the model JSON-encodes the recipient. An email-only item now fails when the agent leaves out a recipient or adds one.

JIRA: QA-29902

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Alert recipient checks now require expected recipients to match exactly across external addresses and internal users, preventing missing or extra recipients from being incorrectly accepted.
    • Expected email addresses must resolve to matching internal users when they are not listed as external recipients.
    • Recipient values in supported formats, including JSON-encoded arrays and delimited external addresses, are handled consistently.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: ebb01f7e-723c-4345-a1ac-8bc853fd649a
Commits

Reviewing files that changed from the base of the PR and between 243ec3e and af14e52.

Files selected for processing: 2
  • packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py
  • packages/gooddata-eval/tests/test_agentic_alert_skill.py

Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

Walkthrough

The alert evaluator now normalizes recipient values and checks expected external addresses and internal user IDs for exact matches. Tests cover supported input formats, user lookups, and malformed recipient entries.

Changes: Alert recipient matching

Layer / File(s) Summary
Normalize and match recipients
packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py, packages/gooddata-eval/tests/test_agentic_alert_skill.py
The evaluator normalizes recipient arguments and checks external addresses and resolved internal IDs for exact matches. Tests cover JSON-encoded values, delimited external recipients, per-email lookups, and malformed internal entries.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Merge Risk: Minimal · up to af14e

The recipient-matching change is ready to merge after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes both primary changes: decoding JSON-encoded alert recipients and enforcing an exact recipient set.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit sorts the names with care,
And checks each address in the pair.
The commas part, the JSON’s read,
Each user ID is checked for need.
Exact matches make the list complete,
Then hops away on quiet feet.

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.74%. Comparing base (71c9de7) to head (af14e52).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...eval/src/gooddata_eval/core/agentic/alert_skill.py 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1904      +/-   ##
==========================================
+ Coverage   85.65%   85.74%   +0.08%     
==========================================
  Files         343      343              
  Lines       25069    25155      +86     
==========================================
+ Hits        21472    21568      +96     
+ Misses       3597     3587      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant