Skip to content

Commit 53c8d83

Browse files
myhoaiclaude
andcommitted
fix(gooddata-eval): stop alert sim-user and checker 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. - 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 53c8d83

2 files changed

Lines changed: 260 additions & 18 deletions

File tree

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

Lines changed: 102 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,40 @@ 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 email {emails} failed: {exc}")
259266
return set()
267+
if not ids:
268+
print(f"[RECIPIENTS] No user has an email in {emails}; internal recipients cannot match them.")
269+
return ids
270+
271+
272+
def _resolve_current_user_id(host: str, token: str) -> str | None:
273+
"""Id of the user that owns ``token``, or None if the profile cannot be read.
274+
275+
The current user is matched by id, not email: that user may have no email at all.
276+
"""
277+
try:
278+
resp = httpx.get(
279+
f"{host.rstrip('/')}/api/v1/profile",
280+
headers={"Authorization": f"Bearer {token}"},
281+
timeout=30.0,
282+
)
283+
resp.raise_for_status()
284+
user_id = resp.json().get("userId")
285+
except Exception as exc:
286+
print(f"[RECIPIENTS] Resolving the current user failed: {exc}")
287+
return None
288+
return user_id if isinstance(user_id, str) and user_id else None
260289

261290

262-
def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None) -> bool:
291+
def _check_recipients(
292+
expected: CatalogMetricAlert,
293+
actual_args: dict,
294+
sdk: GoodDataSdk | None = None,
295+
current_user_id: str | None = None,
296+
) -> bool:
263297
if not expected.recipients:
264298
return True
265299
act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients"))
@@ -274,8 +308,6 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good
274308
act_recip = act_recip_raw
275309
else:
276310
act_recip = []
277-
if set(expected.recipients) == set(act_recip or []):
278-
return True
279311
act_internal_raw = actual_args.get("internal_recipients")
280312
# internal_recipients is declared `anyOf: [array of string, string, null]` in the
281313
# create_metric_alert tool schema -- a single id as a bare string is schema-legal,
@@ -287,8 +319,17 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: Good
287319
act_internal = act_internal_raw
288320
else:
289321
act_internal = []
322+
expected_emails = [r for r in expected.recipients if r != _CURRENT_USER_RECIPIENT]
323+
if _CURRENT_USER_RECIPIENT in expected.recipients:
324+
if current_user_id is None or current_user_id not in act_internal:
325+
return False
326+
act_internal = [i for i in act_internal if i != current_user_id]
327+
if not expected_emails:
328+
return not act_internal and not act_recip
329+
if set(expected_emails) == set(act_recip or []):
330+
return True
290331
if sdk is not None and act_internal:
291-
internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected.recipients)
332+
internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected_emails)
292333
if internal_recipient_ids & set(act_internal):
293334
return True
294335
return False
@@ -299,13 +340,16 @@ def generate_simulated_alert_response(
299340
expected: CatalogMetricAlert,
300341
conversation_history: list,
301342
question: str = "",
343+
current_user_id: str | None = None,
302344
) -> str:
303345
"""Stateful sim-user reply for alert-skill conversation (gpt-4o).
304346
305347
``question`` is the fixture's original request. The sim-user is first called with an empty
306348
history — the opening question went straight to the agent, never to the sim-user — so
307349
without it rule 5's "the filters your original request implies" refers to text the model
308350
cannot see. Optional (defaults to "") to keep the signature backwards compatible.
351+
352+
``current_user_id`` lets the sim-user recognise itself as the recipient of a "me" alert.
309353
"""
310354
if _OpenAI is None:
311355
raise RuntimeError(
@@ -333,7 +377,31 @@ def generate_simulated_alert_response(
333377
threshold = expected.threshold
334378
else:
335379
threshold = "not specified"
336-
recipients = ", ".join(expected.recipients) if expected.recipients else "not specified"
380+
wants_current_user = _CURRENT_USER_RECIPIENT in expected.recipients
381+
emails = [r for r in expected.recipients if r != _CURRENT_USER_RECIPIENT]
382+
recipient_names = list(emails)
383+
if wants_current_user:
384+
user_id_note = f", user id {current_user_id}" if current_user_id else ""
385+
recipient_names.insert(0, f"me (yourself, the current user{user_id_note})")
386+
recipients = ", ".join(recipient_names) if recipient_names else "not specified"
387+
if wants_current_user and not emails:
388+
# An email given here would make the agent send the alert to it instead of the current user.
389+
recipients_rule = (
390+
"4. The alert is for you yourself. Never give an email address and never ask for one to "
391+
"be added. A proposal whose recipient is you — shown as 'You', the current user or your "
392+
"own user id — has the correct recipient.\n"
393+
)
394+
elif wants_current_user:
395+
recipients_rule = (
396+
f"4. Proactively state the email recipients in your first reply: {', '.join(emails)}, "
397+
"and say the alert should also go to you yourself. A recipient shown as 'You', the "
398+
"current user or your own user id is you.\n"
399+
)
400+
else:
401+
recipients_rule = (
402+
"4. Proactively include your email recipient in your first reply. "
403+
" Do not wait for the agent to ask — state it alongside the metric and condition answers.\n"
404+
)
337405
trigger = expected.trigger
338406
filters = expected.filters
339407

@@ -378,14 +446,25 @@ def generate_simulated_alert_response(
378446
# and scoring an item against an interval it never asked for is the defect this rule
379447
# exists to undo.
380448
granularity = (expected.granularity or AnomalyDetectionGranularity.DAY).value
381-
anomaly_rule = (
449+
period_rule = (
382450
"7. This is an ANOMALY alert. Anomaly detection REQUIRES a time granularity, and that "
383451
f"granularity is NOT a date filter. State it in your first reply and repeat it whenever "
384452
f"asked: use {granularity} granularity. Rule 5 constrains filters on the metric only — it "
385453
"never applies to this detection interval, so never refuse to give one.\n"
386454
)
455+
elif operator in _RELATIVE_OPERATORS:
456+
# The item names no comparison date or period; delegating is an answer the agent accepts,
457+
# refusing loops until the turn budget runs out.
458+
period_rule = (
459+
"7. This is a period-over-period comparison alert. To define 'the previous period' the "
460+
"agent needs a comparison date and a period length (day, week, month, quarter or year), "
461+
"and those are NOT a date filter. Rule 5 constrains filters on the metric only — it never "
462+
"applies to the comparison. If the agent asks which date or which period the comparison "
463+
"should use, do not refuse and do not name one yourself: hand the choice back, for "
464+
"example 'Pick whichever date and period fit best.'\n"
465+
)
387466
else:
388-
anomaly_rule = (
467+
period_rule = (
389468
"7. Do not invent an evaluation period, a granularity or an 'evaluate each run on a X "
390469
"basis' instruction your goal never asked for.\n"
391470
)
@@ -409,11 +488,10 @@ def generate_simulated_alert_response(
409488
" when every one of those fields matches your goal.\n"
410489
" A field your goal reports as 'not specified' is one you have NO expectation about: "
411490
" 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"
491+
+ recipients_rule
414492
+ filters_rule
415493
+ 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."
494+
" Repeat it if the agent proposes a different cadence.\n" + period_rule + "Reply concisely and directly."
417495
)
418496

419497
messages: list = [{"role": "system", "content": system_prompt}]
@@ -590,6 +668,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert:
590668
recipients = [r.strip() for r in raw_recip.replace(";", ",").split(",") if r.strip()]
591669
else:
592670
recipients = list(raw_recip)
671+
recipients = [_CURRENT_USER_RECIPIENT if str(r).strip().lower() in _CURRENT_USER_MARKERS else r for r in recipients]
593672

594673
filters = _normalize_expected_filters(expected)
595674
attributes = _normalize_expected_attributes(expected)
@@ -675,6 +754,7 @@ def run_agentic_alert_skill(
675754
user_context=user_context,
676755
)
677756
sdk = GoodDataSdk.create(host, token)
757+
current_user_id = _resolve_current_user_id(host, token) if _CURRENT_USER_RECIPIENT in expected.recipients else None
678758

679759
def _run_once(conv_id: str) -> AlertRunResult:
680760
alert_id_to_delete: str | None = None
@@ -761,7 +841,11 @@ def _run_once(conv_id: str) -> AlertRunResult:
761841
# exit reason to classify the run as an error rather than an agent failure.
762842
try:
763843
follow_up = generate_simulated_alert_response(
764-
response_text, expected, conversation_history, question=question
844+
response_text,
845+
expected,
846+
conversation_history,
847+
question=question,
848+
current_user_id=current_user_id,
765849
)
766850
except Exception as exc: # noqa: BLE001 -- harness-side fault; end only this run
767851
print(f"[SIM-USER] Simulated reply failed for conversation {conv_id}: {exc}")
@@ -779,7 +863,8 @@ def _run_once(conv_id: str) -> AlertRunResult:
779863
trigger_correct=tool_called and _check_trigger(expected, actual_args),
780864
filters_correct=tool_called and _check_filters(expected, actual_args),
781865
metric_correct=tool_called and _check_metric(expected, actual_args),
782-
recipients_correct=tool_called and _check_recipients(expected, actual_args, sdk=sdk),
866+
recipients_correct=tool_called
867+
and _check_recipients(expected, actual_args, sdk=sdk, current_user_id=current_user_id),
783868
attributes_correct=tool_called and _check_attributes(expected, actual_args),
784869
granularity_correct=tool_called and _check_granularity(expected, actual_args),
785870
)

0 commit comments

Comments
 (0)