Skip to content

Commit 24ef3c9

Browse files
myhoaiclaude
andcommitted
fix(gooddata-eval): keep the simulated user from writing MAQL
In context mode an agent that hit a MAQL error and asked "Should I retry with this corrected formula?" was classified as asking a question, not as asking for confirmation. The question went to the clarification judge, which graded it as lost context. The simulated user was then asked to restate what had been said earlier, and the only formula said earlier was the agent's own rejected proposal, so it sent that back. The turn failed even though the metric the agent finally created matched the expected one. - Treat retry / try (it|this|that) again / resubmit after "should/shall/may/can I", "do you want me to" and "would you like me to" as a confirmation, so the user approves the revised proposal with "Yes, go ahead." - A retry that offers a choice stays a question: "Should I retry with gross or net revenue?" and "Should I retry? Or adjust the formula first?". An OR inside an inline formula or a {type/id} reference is not a choice, and a dot inside a token ("2.5") does not end the sentence. - `use` and `run` stay out, because they would turn choice questions such as "Which one should I use?" into confirmations. - The verbs now end on a word boundary, so "Should I address ..." is no longer read as "Should I add". - Tell the simulated user in reply_restating and reply_from_facts never to write MAQL, SQL, formulas or object definitions. - Replace any generated reply that still contains MAQL or SQL with NO_ANSWER_REPLY. That covers uppercase SELECT, an uppercase aggregation call (SUM(...), COUNT(...), ...), "select" in any case before an aggregation call, lowercase SQL shaped as select * / a column list / a table followed by where, group by, order by or join, and {metric/...}, {fact/...}, {label/...}, {attribute/...} or {dataset/...} references in braces or angle brackets. A plain "Select the first option from the list." is left alone. Both callers answer a question, so the fallback has to answer one too, not just approve. Fixture set answers are sent unchanged. jira: QA-29876 risk: low Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f04c9ce commit 24ef3c9

2 files changed

Lines changed: 142 additions & 10 deletions

File tree

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

Lines changed: 49 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,28 @@
3737
# Asked to pick from a list (which dashboard to bind an alert to), a real user picks one.
3838
NO_ANSWER_REPLY = "Use your best judgement and go ahead. If you need me to pick from a list, take the first option."
3939

40-
# Part of the metric and alert skills' designed flow, not a clarification.
40+
# Part of the metric and alert skills' designed flow, not a clarification; so is a retry of a
41+
# corrected proposal. Keep `use` and `run` out: "Which one should I use?" is a choice question.
42+
_RETRY_VERBS = r"retry|try\s+(?:(?:it|this|that)\s+)?again|resubmit"
43+
_APPROVAL_VERBS = rf"go\s+ahead|proceed|create|save|update|apply|{_RETRY_VERBS}"
44+
_SHOULD_I = r"\b(?:should|shall|may|can)\s+i\s+"
45+
_WANT_ME_TO = r"\b(?:do\s+you\s+want|would\s+you\s+like)\s+me\s+to\s+"
4146
_CONFIRMATION_RE = re.compile(
42-
r"\b(?:should|shall|may|can)\s+i\s+(?:go\s+ahead|proceed|create|save|update|apply|add|set\s+(?:it|this)\s+up)"
43-
r"|\bdo\s+you\s+want\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)"
44-
r"|\bwould\s+you\s+like\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)"
47+
rf"{_SHOULD_I}(?:{_APPROVAL_VERBS}|add|set\s+(?:it|this)\s+up)\b"
48+
rf"|{_WANT_ME_TO}(?:{_APPROVAL_VERBS})\b"
4549
r"|\bplease\s+confirm\b|\bconfirm\s+(?:if|whether|that)\b",
4650
re.IGNORECASE,
4751
)
52+
# "Should I retry with gross or net revenue?" offers a choice, so it stays a question, as does
53+
# a second option asked right after ("Should I retry? Or adjust the formula first?"). The
54+
# clause runs to the sentence end: a dot inside a token ("2.5") does not end it.
55+
_RETRY_CHOICE_RE = re.compile(
56+
rf"(?:{_SHOULD_I}|{_WANT_ME_TO})(?:{_RETRY_VERBS})\b(?:[^?.!]|\.(?=\S))*(?:\bor\b|[?.!]\s+or\b)",
57+
re.IGNORECASE,
58+
)
59+
# Inline code and {type/id} references are the proposal itself; an OR inside a formula is
60+
# not a choice offered to the user.
61+
_FORMULA_SPAN_RE = re.compile(r"`+[^`]*`+|\{[^{}]*\}")
4862

4963
# Checked after _CONFIRMATION_RE, so "please confirm" stays a confirmation.
5064
_REQUEST_RE = re.compile(
@@ -77,7 +91,7 @@ def classify_reply(chat_result: ChatResult, rendered_text: str) -> ReplyKind:
7791
text = (rendered_text or "").strip()
7892
if not text:
7993
return "no_action"
80-
if _CONFIRMATION_RE.search(text):
94+
if _CONFIRMATION_RE.search(text) and not _RETRY_CHOICE_RE.search(_FORMULA_SPAN_RE.sub(" ", text)):
8195
return "confirmation"
8296
if "?" in text or _REQUEST_RE.search(text):
8397
return "question"
@@ -238,6 +252,31 @@ def judge(self, history: str, latest: str, question: str) -> ClarificationVerdic
238252

239253
_SIM_USER_MODEL = "gpt-4o"
240254

255+
# The simulated user plays a business user: choosing the formula is the assistant's job.
256+
_NO_DEFINITIONS = (
257+
"Never write MAQL, SQL, formulas or object definitions. If asked to approve one, reply that it is fine to go ahead."
258+
)
259+
# A bare SELECT counts only in uppercase, so a plain "Select the first one" is left alone; in
260+
# any case it counts once an aggregation call follows, as in "select sum(revenue)". An
261+
# uppercase aggregation call ("SUM(units) / COUNT(orders)") is a formula even without SELECT.
262+
# Lowercase SQL counts by its shape (`*`, a column list, or a table followed by a clause), so
263+
# "Select the first option from the list" is left alone.
264+
_MAQL_RE = re.compile(
265+
r"\bSELECT\b|(?i:\bselect\s+(?:sum|count|avg|min|max|median|distinct)\s*\()"
266+
r"|(?i:\bselect\s+(?:\*|[\w.]+(?:\s*,\s*[\w.]+)+)\s+from\b)"
267+
r"|(?i:\bselect\b[^?!]*?\bfrom\s+[\w.]+\s+(?:where|group\s+by|order\s+by|join)\b)"
268+
r"|\b(?:SUM|COUNT|AVG|MIN|MAX|MEDIAN)\s*\("
269+
r"|[{<](?:metric|fact|label|attribute|dataset)/"
270+
)
271+
272+
273+
def _without_maql(reply: str | None) -> str | None:
274+
"""A reply that slipped MAQL in past the prompt leaves the choice to the assistant.
275+
276+
Both callers answer a question, so the fallback must answer one too, not just approve.
277+
"""
278+
return NO_ANSWER_REPLY if reply and _MAQL_RE.search(reply) else reply
279+
241280

242281
def _chat(prompt_system: str, prompt_user: str) -> str | None:
243282
try:
@@ -271,12 +310,12 @@ def reply_from_facts(transcript: str, question: str, facts: list[str]) -> str:
271310
return NO_ANSWER_REPLY
272311
reply = _chat(
273312
"You play a business user talking to an analytics assistant. Answer its question briefly, "
274-
"using ONLY the facts listed below. Never invent requirements. If the facts do not answer "
275-
f"the question, reply exactly: {NO_ANSWER_REPLY}",
313+
f"using ONLY the facts listed below. Never invent requirements. {_NO_DEFINITIONS} If the facts "
314+
f"do not answer the question, reply exactly: {NO_ANSWER_REPLY}",
276315
f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}\n\n"
277316
"Facts you know:\n" + "\n".join(f"- {f}" for f in facts),
278317
)
279-
return reply or NO_ANSWER_REPLY
318+
return _without_maql(reply) or NO_ANSWER_REPLY
280319

281320

282321
def reply_restating(transcript: str, question: str) -> str:
@@ -289,7 +328,7 @@ def reply_restating(transcript: str, question: str) -> str:
289328
"You play a business user talking to an analytics assistant. The assistant just asked for "
290329
"something that was already said or found earlier in the conversation. Reply briefly, "
291330
"starting with 'As I said earlier,' and repeat only that information, taken from the "
292-
"conversation. Do not add anything new.",
331+
f"conversation. Do not add anything new. {_NO_DEFINITIONS}",
293332
f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}",
294333
)
295-
return reply or "As I said earlier in this conversation — please use what we already established."
334+
return _without_maql(reply) or "As I said earlier in this conversation — please use what we already established."

‎packages/gooddata-eval/tests/test_agentic_conversation.py‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1450,6 +1450,24 @@ def test_output_correct_compares_maql_for_a_metric_turn() -> None:
14501450
[
14511451
(_result(text="Formula: SELECT 1. Should I create this metric?"), "confirmation"),
14521452
(_result(text=None, alertProposals=[{"cta": "Create?"}]), "confirmation"),
1453+
(_result(text="Should I retry with this corrected formula?"), "confirmation"),
1454+
(_result(text="Do you want me to try again with the corrected formula?"), "confirmation"),
1455+
(_result(text="Would you like me to try it again?"), "confirmation"),
1456+
(_result(text="Shall I resubmit it?"), "confirmation"),
1457+
(_result(text="Shall I try this again?"), "confirmation"),
1458+
(_result(text="Should I address revenue or margin first?"), "question"),
1459+
(_result(text="Should I retry with the corrected formula or keep the original?"), "question"),
1460+
(
1461+
_result(text='Should I retry with `SELECT SUM({fact/x}) WHERE {label/a} = "A" OR {label/b} = "B"`?'),
1462+
"confirmation",
1463+
),
1464+
(_result(text="Should I retry with {label/customer.region} or {label/region}?"), "question"),
1465+
(_result(text="Should I retry with a 2.5 threshold or a 3.0 one?"), "question"),
1466+
(_result(text="The create failed. Should I retry? Or adjust the formula first?"), "question"),
1467+
(_result(text="The create failed. Should I retry? The formula is fixed."), "confirmation"),
1468+
(_result(text="Would you like me to add a region filter?"), "question"),
1469+
(_result(text="Which one should I use?"), "question"),
1470+
(_result(text="Should I use gross or net revenue?"), "question"),
14531471
(_result(text="Which revenue metric do you mean?"), "question"),
14541472
(_result(text="Please select one of the five options by number, name, or ID."), "question"),
14551473
(_result(text="Reply with the number of the dashboard to use."), "question"),
@@ -1567,6 +1585,49 @@ def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_agai
15671585
assert result.turn_results[0].context_success is True
15681586

15691587

1588+
def test_context_mode_approves_a_retry_after_a_rejected_metric_without_restating_its_formula() -> None:
1589+
maql = (
1590+
'SELECT (SELECT SUM({fact/order_unit_quantity}) WHERE {label/order_status} = "Processed") / {metric/net_orders}'
1591+
)
1592+
rejected = 'SELECT SUM({fact/order_unit_quantity}) WHERE {label/order_status} = "Processed" / {metric/net_orders}'
1593+
turns = [
1594+
TurnDefinition(
1595+
turn_id="create_metric",
1596+
message="Create a metric named Units Per Transaction",
1597+
expected_skill="metric",
1598+
expected_output_type="metric",
1599+
expected_output={"maql": maql},
1600+
)
1601+
]
1602+
skills = _tool("set_skills", {"skill_names": ["metric"]})
1603+
failed = _tool("create_metric", {"maql": rejected}, {"data": {"isError": True, "error": {"text": "MAQL error"}}})
1604+
created = _tool("create_metric", {"maql": maql}, {"data": {"metric_id": "units_per_transaction", "maql": maql}})
1605+
judge = _FakeJudge(lost_context=True)
1606+
with patch("gooddata_eval.core.agentic.conversation.reply_restating", return_value=rejected) as restate:
1607+
result, client = _run(
1608+
[
1609+
_result(text=f"Formula: `{rejected}`\n\nShould I create this metric?", tools=[skills]),
1610+
_result(
1611+
text=f"The first attempt was rejected. Formula: `{maql}`\n\n"
1612+
"Should I retry with this corrected formula?",
1613+
tools=[failed],
1614+
),
1615+
_result(text="Created.", tools=[created]),
1616+
],
1617+
turns,
1618+
judge=judge,
1619+
)
1620+
sent = [c.args[1] for c in client.send_message.call_args_list]
1621+
assert sent[1:] == [CONFIRMATION_REPLY, CONFIRMATION_REPLY]
1622+
assert not any("SELECT" in m or "{fact/" in m for m in sent)
1623+
restate.assert_not_called()
1624+
assert judge.calls == []
1625+
turn = result.turn_results[0]
1626+
assert turn.lost_context is False
1627+
assert turn.stalled is False
1628+
assert turn.output_correct is True
1629+
1630+
15701631
def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges() -> None:
15711632
turns = [_viz_turn("t1", expected=_viz())]
15721633
incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations")
@@ -1960,6 +2021,38 @@ def test_reply_restating_returns_the_llm_restatement(monkeypatch: pytest.MonkeyP
19602021
assert reply_restating("USER: weakest? ASSISTANT: Express", "Which format?") == "As I said earlier, Express."
19612022

19622023

2024+
@pytest.mark.parametrize(
2025+
"llm_reply",
2026+
[
2027+
'As I said earlier, the formula is: `SELECT SUM({fact/units}) WHERE {label/status} = "Processed"`.',
2028+
"Use <metric/net_orders> as the denominator.",
2029+
"As I said earlier, use select sum(units) where status = 'Processed'.",
2030+
"As I said earlier, it is SUM(units) / COUNT(orders).",
2031+
"Use select * from orders.",
2032+
"Run select region, revenue from orders.",
2033+
"As I said earlier: select region from orders where status = 'Processed'.",
2034+
],
2035+
)
2036+
def test_simulated_user_never_sends_maql(monkeypatch: pytest.MonkeyPatch, llm_reply: str) -> None:
2037+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test")
2038+
openai_cls = _fake_openai(monkeypatch, _openai_returning(llm_reply))
2039+
assert reply_restating("h", "Which formula?") == NO_ANSWER_REPLY
2040+
assert reply_from_facts("h", "Which formula?", ["Processed orders only."]) == NO_ANSWER_REPLY
2041+
calls = openai_cls.return_value.chat.completions.create.call_args_list
2042+
assert len(calls) == 2
2043+
for call in calls:
2044+
assert "Never write MAQL" in call.kwargs["messages"][0]["content"]
2045+
2046+
2047+
@pytest.mark.parametrize("llm_reply", ["Select the first option.", "Select the first option from the list."])
2048+
def test_simulated_user_keeps_a_plain_reply_that_mentions_select(
2049+
monkeypatch: pytest.MonkeyPatch, llm_reply: str
2050+
) -> None:
2051+
monkeypatch.setenv("OPENAI_API_KEY", "sk-test")
2052+
_fake_openai(monkeypatch, _openai_returning(llm_reply))
2053+
assert reply_from_facts("h", "Which one?", ["Any is fine."]) == llm_reply
2054+
2055+
19632056
def test_simulated_user_reports_a_missing_openai_package(monkeypatch: pytest.MonkeyPatch) -> None:
19642057
monkeypatch.setitem(sys.modules, "openai", None)
19652058
assert ctx_mod._chat("s", "u") is None

0 commit comments

Comments
 (0)