Skip to content

Commit a854adf

Browse files
myhoaiclaude
andcommitted
fix(gooddata-eval): stop alert eval failing correct runs
- Comparison alerts: the sim-user lets the agent pick the comparison date and period instead of refusing them as a date filter, which looped until the turn budget ran out. - An item can expect "Recipient(s)": "me". The checker matches the token's user by id from /api/v1/profile, since that user may have no email, and the sim-user no longer volunteers an email for it. If that id cannot be read, the item errors instead of failing. - Log when looking up recipients by email finds nobody or fails. jira: QA-29760 risk: low Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent abc7a06 commit a854adf

2 files changed

Lines changed: 322 additions & 18 deletions

File tree

‎packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py‎

Lines changed: 122 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from dataclasses import dataclass, field
1010
from typing import Any
1111

12+
import httpx
1213
from gooddata_sdk import GoodDataSdk
1314

1415
from gooddata_eval.core.agentic._catalog import AnomalyDetectionGranularity, CatalogMetricAlert
@@ -54,6 +55,11 @@
5455

5556
_TRIGGER_DISPLAY_TO_API = {"Every time": "ALWAYS", "One time": "ONCE"}
5657
_ALWAYS_TRIGGER_VALUES = {"Every time", "ALWAYS", "not specified"}
58+
_RELATIVE_OPERATORS = ("INCREASES_BY", "DECREASES_BY", "CHANGES_BY")
59+
60+
# Expected-recipient spellings that mean the owner of the API token.
61+
_CURRENT_USER_RECIPIENT = "me"
62+
_CURRENT_USER_MARKERS = {"me", "myself", "current user", "current_user"}
5763

5864
_TRIGGER_INSTRUCTIONS = {
5965
"ALWAYS": (
@@ -242,9 +248,9 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[
242248
`create_metric_alert` then addresses the alert by internal user id
243249
(`internal_recipients`), never by email, so an expected email has to be
244250
resolved before it can be compared against that field. Failures (no
245-
matching user, no permission, network error) are swallowed: the caller
246-
treats an empty result the same as "this delivery path doesn't match",
247-
which is correct -- it doesn't mean the alert itself failed.
251+
matching user, no permission, network error) are logged and return an
252+
empty set: the caller treats that the same as "this delivery path doesn't
253+
match", which is correct -- it doesn't mean the alert itself failed.
248254
"""
249255
if not emails:
250256
return set()
@@ -254,12 +260,48 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[
254260
escaped = [email.replace("\\", "\\\\").replace("'", "\\'") for email in emails]
255261
quoted = ",".join(f"'{email}'" for email in escaped)
256262
resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=in=({quoted})")
257-
return {u.id for u in (resp.data or [])}
258-
except Exception:
263+
ids = {u.id for u in (resp.data or [])}
264+
except Exception as exc:
265+
print(f"[RECIPIENTS] Looking up users by {len(emails)} recipient email(s) failed: {exc}")
259266
return set()
267+
if not ids:
268+
print(
269+
f"[RECIPIENTS] No user has any of the {len(emails)} recipient email(s); internal recipients cannot match."
270+
)
271+
return ids
272+
273+
274+
def _resolve_current_user_id(host: str, token: str) -> str | None:
275+
"""Id of the user that owns ``token``, or None if the profile cannot be read.
276+
277+
The current user is matched by id, not email: that user may have no email at all.
278+
"""
279+
try:
280+
resp = httpx.get(
281+
f"{host.rstrip('/')}/api/v1/profile",
282+
headers={"Authorization": f"Bearer {token}"},
283+
timeout=30.0,
284+
)
285+
resp.raise_for_status()
286+
user_id = resp.json().get("userId")
287+
except Exception as exc:
288+
print(f"[RECIPIENTS] Resolving the current user failed: {exc}")
289+
return None
290+
return user_id if isinstance(user_id, str) and user_id else None
291+
260292

293+
def _split_current_user(recipients: list[str]) -> tuple[bool, list[str]]:
294+
"""Whether ``recipients`` names the current user, and the email recipients besides it."""
295+
emails = [r for r in recipients if r != _CURRENT_USER_RECIPIENT]
296+
return len(emails) != len(recipients), emails
261297

262-
def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None) -> bool:
298+
299+
def _check_recipients(
300+
expected: CatalogMetricAlert,
301+
actual_args: dict,
302+
sdk: GoodDataSdk | None = None,
303+
current_user_id: str | None = None,
304+
) -> bool:
263305
if not expected.recipients:
264306
return True
265307
act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients"))
@@ -274,8 +316,6 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good
274316
act_recip = act_recip_raw
275317
else:
276318
act_recip = []
277-
if set(expected.recipients) == set(act_recip or []):
278-
return True
279319
act_internal_raw = actual_args.get("internal_recipients")
280320
# internal_recipients is declared `anyOf: [array of string, string, null]` in the
281321
# create_metric_alert tool schema -- a single id as a bare string is schema-legal,
@@ -287,8 +327,25 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good
287327
act_internal = act_internal_raw
288328
else:
289329
act_internal = []
330+
wants_current_user, expected_emails = _split_current_user(expected.recipients)
331+
if wants_current_user:
332+
if current_user_id is None or current_user_id not in act_internal:
333+
return False
334+
other_internal = set(act_internal) - {current_user_id}
335+
external = set(act_recip or [])
336+
if external - set(expected_emails):
337+
return False
338+
# An email not sent externally must arrive as its user's id (workspace-restricted channel).
339+
unmatched = [e for e in expected_emails if e not in external]
340+
if not unmatched:
341+
return not other_internal
342+
if sdk is None or not other_internal:
343+
return False
344+
return other_internal == _resolve_internal_recipient_ids(sdk, unmatched)
345+
if set(expected_emails) == set(act_recip or []):
346+
return True
290347
if sdk is not None and act_internal:
291-
internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected.recipients)
348+
internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected_emails)
292349
if internal_recipient_ids & set(act_internal):
293350
return True
294351
return False
@@ -299,13 +356,16 @@ def generate_simulated_alert_response(
299356
expected: CatalogMetricAlert,
300357
conversation_history: list,
301358
question: str = "",
359+
current_user_id: str | None = None,
302360
) -> str:
303361
"""Stateful sim-user reply for alert-skill conversation (gpt-4o).
304362
305363
``question`` is the fixture's original request. The sim-user is first called with an empty
306364
history — the opening question went straight to the agent, never to the sim-user — so
307365
without it rule 5's "the filters your original request implies" refers to text the model
308366
cannot see. Optional (defaults to "") to keep the signature backwards compatible.
367+
368+
``current_user_id`` lets the sim-user recognise itself as the recipient of a "me" alert.
309369
"""
310370
if _OpenAI is None:
311371
raise RuntimeError(
@@ -333,7 +393,30 @@ def generate_simulated_alert_response(
333393
threshold = expected.threshold
334394
else:
335395
threshold = "not specified"
336-
recipients = ", ".join(expected.recipients) if expected.recipients else "not specified"
396+
wants_current_user, emails = _split_current_user(expected.recipients)
397+
recipient_names = list(emails)
398+
if wants_current_user:
399+
user_id_note = f", user id {current_user_id}" if current_user_id else ""
400+
recipient_names.insert(0, f"me (yourself, the current user{user_id_note})")
401+
recipients = ", ".join(recipient_names) if recipient_names else "not specified"
402+
if wants_current_user and not emails:
403+
# An email given here would make the agent send the alert to it instead of the current user.
404+
recipients_rule = (
405+
"4. The alert is for you yourself. Never give an email address and never ask for one to "
406+
"be added. A proposal whose recipient is you — shown as 'You', the current user or your "
407+
"own user id — has the correct recipient.\n"
408+
)
409+
elif wants_current_user:
410+
recipients_rule = (
411+
f"4. Proactively state the email recipients in your first reply: {', '.join(emails)}, "
412+
"and say the alert should also go to you yourself. A recipient shown as 'You', the "
413+
"current user or your own user id is you.\n"
414+
)
415+
else:
416+
recipients_rule = (
417+
"4. Proactively include your email recipient in your first reply. "
418+
" Do not wait for the agent to ask — state it alongside the metric and condition answers.\n"
419+
)
337420
trigger = expected.trigger
338421
filters = expected.filters
339422

@@ -378,14 +461,25 @@ def generate_simulated_alert_response(
378461
# and scoring an item against an interval it never asked for is the defect this rule
379462
# exists to undo.
380463
granularity = (expected.granularity or AnomalyDetectionGranularity.DAY).value
381-
anomaly_rule = (
464+
period_rule = (
382465
"7. This is an ANOMALY alert. Anomaly detection REQUIRES a time granularity, and that "
383466
f"granularity is NOT a date filter. State it in your first reply and repeat it whenever "
384467
f"asked: use {granularity} granularity. Rule 5 constrains filters on the metric only — it "
385468
"never applies to this detection interval, so never refuse to give one.\n"
386469
)
470+
elif operator in _RELATIVE_OPERATORS:
471+
# The item names no comparison date or period; delegating is an answer the agent accepts,
472+
# refusing loops until the turn budget runs out.
473+
period_rule = (
474+
"7. This is a comparison alert: keep the comparison your original request describes. The "
475+
"agent may need a comparison date and a period length (day, week, month, quarter or "
476+
"year), and those are NOT a date filter. Rule 5 constrains filters on the metric only — "
477+
"it never applies to the comparison. If the agent asks which date or which period the "
478+
"comparison should use, do not refuse and do not name one yourself: hand the choice back, "
479+
"for example 'Pick whichever date and period fit best.'\n"
480+
)
387481
else:
388-
anomaly_rule = (
482+
period_rule = (
389483
"7. Do not invent an evaluation period, a granularity or an 'evaluate each run on a X "
390484
"basis' instruction your goal never asked for.\n"
391485
)
@@ -409,11 +503,10 @@ def generate_simulated_alert_response(
409503
" when every one of those fields matches your goal.\n"
410504
" A field your goal reports as 'not specified' is one you have NO expectation about: "
411505
" accept whatever the agent chose for it and never ask for it to be removed.\n"
412-
"4. Proactively include your email recipient in your first reply. "
413-
" Do not wait for the agent to ask — state it alongside the metric and condition answers.\n"
506+
+ recipients_rule
414507
+ filters_rule
415508
+ f"6. Proactively state how often you want to be alerted in your first reply: {trigger_request}. "
416-
" Repeat it if the agent proposes a different cadence.\n" + anomaly_rule + "Reply concisely and directly."
509+
" Repeat it if the agent proposes a different cadence.\n" + period_rule + "Reply concisely and directly."
417510
)
418511

419512
messages: list = [{"role": "system", "content": system_prompt}]
@@ -590,6 +683,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert:
590683
recipients = [r.strip() for r in raw_recip.replace(";", ",").split(",") if r.strip()]
591684
else:
592685
recipients = list(raw_recip)
686+
recipients = [_CURRENT_USER_RECIPIENT if str(r).strip().lower() in _CURRENT_USER_MARKERS else r for r in recipients]
593687

594688
filters = _normalize_expected_filters(expected)
595689
attributes = _normalize_expected_attributes(expected)
@@ -665,6 +759,12 @@ def run_agentic_alert_skill(
665759
) -> AgenticAlertSummary:
666760
"""Run the alert-skill agentic evaluation K times and return a summary."""
667761
expected = _normalize_expected_output(expected_output)
762+
current_user_id: str | None = None
763+
if _CURRENT_USER_RECIPIENT in expected.recipients:
764+
current_user_id = _resolve_current_user_id(host, token)
765+
if current_user_id is None:
766+
# A harness fault: report it as an item error, not as the agent's recipient mistake.
767+
raise RuntimeError("Cannot score a current-user alert: the token owner's user id could not be resolved")
668768
run_results: list[AlertRunResult] = []
669769
client = ChatClient(
670770
host=host,
@@ -761,7 +861,11 @@ def _run_once(conv_id: str) -> AlertRunResult:
761861
# exit reason to classify the run as an error rather than an agent failure.
762862
try:
763863
follow_up = generate_simulated_alert_response(
764-
response_text, expected, conversation_history, question=question
864+
response_text,
865+
expected,
866+
conversation_history,
867+
question=question,
868+
current_user_id=current_user_id,
765869
)
766870
except Exception as exc: # noqa: BLE001 -- harness-side fault; end only this run
767871
print(f"[SIM-USER] Simulated reply failed for conversation {conv_id}: {exc}")
@@ -779,7 +883,8 @@ def _run_once(conv_id: str) -> AlertRunResult:
779883
trigger_correct=tool_called and _check_trigger(expected, actual_args),
780884
filters_correct=tool_called and _check_filters(expected, actual_args),
781885
metric_correct=tool_called and _check_metric(expected, actual_args),
782-
recipients_correct=tool_called and _check_recipients(expected, actual_args, sdk=sdk),
886+
recipients_correct=tool_called
887+
and _check_recipients(expected, actual_args, sdk=sdk, current_user_id=current_user_id),
783888
attributes_correct=tool_called and _check_attributes(expected, actual_args),
784889
granularity_correct=tool_called and _check_granularity(expected, actual_args),
785890
)

0 commit comments

Comments
 (0)