fix(gooddata-eval): keep the simulated user from writing MAQL

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

@@ -37,14 +37,36 @@
 # Asked to pick from a list (which dashboard to bind an alert to), a real user picks one.
 NO_ANSWER_REPLY = "Use your best judgement and go ahead. If you need me to pick from a list, take the first option."
 
-# Part of the metric and alert skills' designed flow, not a clarification.
+# Part of the metric and alert skills' designed flow, not a clarification; so is a retry of a
+# corrected proposal. Keep `use` and `run` out: "Which one should I use?" is a choice question.
+_RETRY_VERBS = r"retry|try\s+(?:(?:it|this|that)\s+)?(?:again|once\s+more)|resubmit"
+_APPROVAL_VERBS = rf"go\s+ahead|proceed|create|save|update|apply|{_RETRY_VERBS}"
+_SHOULD_I = r"\b(?:should|shall|may|can)\s+i\s+"
+_WANT_ME_TO = r"\b(?:do\s+you\s+want|would\s+you\s+like)\s+me\s+to\s+"
 _CONFIRMATION_RE = re.compile(
-    r"\b(?:should|shall|may|can)\s+i\s+(?:go\s+ahead|proceed|create|save|update|apply|add|set\s+(?:it|this)\s+up)"
-    r"|\bdo\s+you\s+want\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)"
-    r"|\bwould\s+you\s+like\s+me\s+to\s+(?:go\s+ahead|proceed|create|save|update|apply)"
+    rf"{_SHOULD_I}(?:{_APPROVAL_VERBS}|add|set\s+(?:it|this)\s+up)\b"
+    rf"|{_WANT_ME_TO}(?:{_APPROVAL_VERBS})\b"
     r"|\bplease\s+confirm\b|\bconfirm\s+(?:if|whether|that)\b",
     re.IGNORECASE,
 )
+# "Should I retry with gross or net revenue?" offers a choice, so it stays a question, as does
+# a second option asked right after ("Should I retry? Or adjust the formula first?"). The
+# clause runs to the sentence end: a dot inside a token ("2.5") does not end it. Only a
+# prose "or"/"Or" counts: MAQL writes its OR in uppercase. An "or" that hands the turn back
+# to the user ("..., or would you like to adjust it?", "or not?") is not a second option.
+_NOT_AN_OPTION = (
+    r"not\b|(?:would|do)\s+you\s+(?:like|want|prefer)\s+to\b|(?:is|are)\s+there\b"
+    r"|anything\b|something\b|if\s+you\b|let\s+me\s+know\b"
+)
+_RETRY_CHOICE_RE = re.compile(
+    rf"(?:{_SHOULD_I}|{_WANT_ME_TO})(?:{_RETRY_VERBS})\b(?:[^?.!]|\.(?=\S))*"
+    rf"(?:[?.!]\s+)?(?:(?-i:\b[Oo]r\b)|\balternatively\b)(?!\s+(?:{_NOT_AN_OPTION}))",
+    re.IGNORECASE,
+)
+# Inline code, {type/id} references and quoted literals are the proposal itself; an OR
+# inside a formula is not a choice offered to the user. A single quote counts only outside a
+# word, so the apostrophes in "don't ... it's" do not mask the text between them.
+_FORMULA_SPAN_RE = re.compile(r"`+[^`]*`+|\{[^{}]*\}|\"[^\"\n]*\"|(?<!\w)'[^'\n]*'(?!\w)")
 
 # Checked after _CONFIRMATION_RE, so "please confirm" stays a confirmation.
 _REQUEST_RE = re.compile(
@@ -77,7 +99,7 @@ def classify_reply(chat_result: ChatResult, rendered_text: str) -> ReplyKind:
     text = (rendered_text or "").strip()
     if not text:
         return "no_action"
-    if _CONFIRMATION_RE.search(text):
+    if _CONFIRMATION_RE.search(text) and not _RETRY_CHOICE_RE.search(_FORMULA_SPAN_RE.sub(" ", text)):
         return "confirmation"
     if "?" in text or _REQUEST_RE.search(text):
         return "question"
@@ -238,6 +260,33 @@ def judge(self, history: str, latest: str, question: str) -> ClarificationVerdic
 
 _SIM_USER_MODEL = "gpt-4o"
 
+# The simulated user plays a business user: choosing the formula is the assistant's job.
+_NO_DEFINITIONS = (
+    "Never write MAQL, SQL, formulas or object definitions. If asked to approve one, reply that it is fine to go ahead."
+)
+# An uppercase SELECT counts once a reference, `(`, `*`, DISTINCT or a call follows it, so
+# "SELECT the second option" and "Select the first one" are left alone; in any case it counts
+# once an aggregation call follows, as in "select sum(revenue)". An
+# aggregation call is a formula even without SELECT: in uppercase ("SUM (units)") or in any
+# case with the parenthesis attached ("sum(units) / count(orders)"), so prose such as
+# "Count (all) is fine" is left alone. Lowercase SQL counts by its shape (`*`, a column list,
+# or a table followed by a clause), so "Select the first option from the list" is left alone.
+_MAQL_RE = re.compile(
+    r"\bSELECT\s+(?:[{<(*]|DISTINCT\b|\w+\s*\()|(?i:\bselect\s+(?:sum|count|avg|min|max|median|distinct)\s*\()"
+    r"|(?i:\bselect\s+(?:\*|[\w.]+(?:\s*,\s*[\w.]+)+)\s+from\b)"
+    r"|(?i:\bselect\b[^?!]*?\bfrom\s+[\w.]+\s+(?:where|group\s+by|order\s+by|join)\b)"
+    r"|\b(?:SUM|COUNT|AVG|MIN|MAX|MEDIAN)\s*\(|(?i:\b(?:sum|count|avg|min|max|median)\()"
+    r"|[{<](?:metric|fact|label|attribute|dataset)/"
+)
+
+
+def _without_maql(reply: str | None) -> str | None:
+    """A reply that slipped MAQL in past the prompt leaves the choice to the assistant.
+
+    Both callers answer a question, so the fallback must answer one too, not just approve.
+    """
+    return NO_ANSWER_REPLY if reply and _MAQL_RE.search(reply) else reply
+
 
 def _chat(prompt_system: str, prompt_user: str) -> str | None:
     try:
@@ -271,12 +320,12 @@ def reply_from_facts(transcript: str, question: str, facts: list[str]) -> str:
         return NO_ANSWER_REPLY
     reply = _chat(
         "You play a business user talking to an analytics assistant. Answer its question briefly, "
-        "using ONLY the facts listed below. Never invent requirements. If the facts do not answer "
-        f"the question, reply exactly: {NO_ANSWER_REPLY}",
+        f"using ONLY the facts listed below. Never invent requirements. {_NO_DEFINITIONS} If the facts "
+        f"do not answer the question, reply exactly: {NO_ANSWER_REPLY}",
         f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}\n\n"
         "Facts you know:\n" + "\n".join(f"- {f}" for f in facts),
     )
-    return reply or NO_ANSWER_REPLY
+    return _without_maql(reply) or NO_ANSWER_REPLY
 
 
 def reply_restating(transcript: str, question: str) -> str:
@@ -289,7 +338,7 @@ def reply_restating(transcript: str, question: str) -> str:
         "You play a business user talking to an analytics assistant. The assistant just asked for "
         "something that was already said or found earlier in the conversation. Reply briefly, "
         "starting with 'As I said earlier,' and repeat only that information, taken from the "
-        "conversation. Do not add anything new.",
+        f"conversation. Do not add anything new. {_NO_DEFINITIONS}",
         f"Conversation so far:\n{transcript}\n\nThe assistant now asks:\n{question}",
     )
-    return reply or "As I said earlier in this conversation — please use what we already established."
+    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

@@ -1450,6 +1450,37 @@ def test_output_correct_compares_maql_for_a_metric_turn() -> None:
     [
         (_result(text="Formula: SELECT 1. Should I create this metric?"), "confirmation"),
         (_result(text=None, alertProposals=[{"cta": "Create?"}]), "confirmation"),
+        (_result(text="Should I retry with this corrected formula?"), "confirmation"),
+        (_result(text="Do you want me to try again with the corrected formula?"), "confirmation"),
+        (_result(text="Would you like me to try it again?"), "confirmation"),
+        (_result(text="Shall I resubmit it?"), "confirmation"),
+        (_result(text="Shall I try this again?"), "confirmation"),
+        (_result(text="Should I address revenue or margin first?"), "question"),
+        (_result(text="Should I retry with the corrected formula or keep the original?"), "question"),
+        (
+            _result(text='Should I retry with `SELECT SUM({fact/x}) WHERE {label/a} = "A" OR {label/b} = "B"`?'),
+            "confirmation",
+        ),
+        (_result(text="Should I retry with {label/customer.region} or {label/region}?"), "question"),
+        (_result(text="Should I retry with a 2.5 threshold or a 3.0 one?"), "question"),
+        (_result(text="The create failed. Should I retry? Or adjust the formula first?"), "question"),
+        (_result(text="Should I retry? Alternatively, should I adjust the formula first?"), "question"),
+        (_result(text="Should I retry with select sum(units) where status = 'Processed or Shipped'?"), "confirmation"),
+        (_result(text="Should I retry with today's data or last week's?"), "question"),
+        (_result(text="The create failed. Should I retry? The formula is fixed."), "confirmation"),
+        (_result(text="Should I retry with this corrected formula, or would you like to adjust it?"), "confirmation"),
+        (_result(text="Should I retry, or not?"), "confirmation"),
+        (_result(text="Should I retry? Or is there anything else to change first?"), "confirmation"),
+        (_result(text="Should I try once more?"), "confirmation"),
+        (
+            _result(text='Should I retry with SELECT SUM({fact/x}) WHERE {label/a} = "A" OR {label/b} = "B"?'),
+            "confirmation",
+        ),
+        (_result(text='Should I retry with {label/status} = "Processed or Shipped"?'), "confirmation"),
+        (_result(text="Should I retry, or would you like me to adjust the formula first?"), "question"),
+        (_result(text="Would you like me to add a region filter?"), "question"),
+        (_result(text="Which one should I use?"), "question"),
+        (_result(text="Should I use gross or net revenue?"), "question"),
         (_result(text="Which revenue metric do you mean?"), "question"),
         (_result(text="Please select one of the five options by number, name, or ID."), "question"),
         (_result(text="Reply with the number of the dashboard to use."), "question"),
@@ -1567,6 +1598,49 @@ def test_context_mode_answers_a_confirmation_with_yes_and_does_not_count_it_agai
     assert result.turn_results[0].context_success is True
 
 
+def test_context_mode_approves_a_retry_after_a_rejected_metric_without_restating_its_formula() -> None:
+    maql = (
+        'SELECT (SELECT SUM({fact/order_unit_quantity}) WHERE {label/order_status} = "Processed") / {metric/net_orders}'
+    )
+    rejected = 'SELECT SUM({fact/order_unit_quantity}) WHERE {label/order_status} = "Processed" / {metric/net_orders}'
+    turns = [
+        TurnDefinition(
+            turn_id="create_metric",
+            message="Create a metric named Units Per Transaction",
+            expected_skill="metric",
+            expected_output_type="metric",
+            expected_output={"maql": maql},
+        )
+    ]
+    skills = _tool("set_skills", {"skill_names": ["metric"]})
+    failed = _tool("create_metric", {"maql": rejected}, {"data": {"isError": True, "error": {"text": "MAQL error"}}})
+    created = _tool("create_metric", {"maql": maql}, {"data": {"metric_id": "units_per_transaction", "maql": maql}})
+    judge = _FakeJudge(lost_context=True)
+    with patch("gooddata_eval.core.agentic.conversation.reply_restating", return_value=rejected) as restate:
+        result, client = _run(
+            [
+                _result(text=f"Formula: `{rejected}`\n\nShould I create this metric?", tools=[skills]),
+                _result(
+                    text=f"The first attempt was rejected. Formula: `{maql}`\n\n"
+                    "Should I retry with this corrected formula?",
+                    tools=[failed],
+                ),
+                _result(text="Created.", tools=[created]),
+            ],
+            turns,
+            judge=judge,
+        )
+    sent = [c.args[1] for c in client.send_message.call_args_list]
+    assert sent[1:] == [CONFIRMATION_REPLY, CONFIRMATION_REPLY]
+    assert not any("SELECT" in m or "{fact/" in m for m in sent)
+    restate.assert_not_called()
+    assert judge.calls == []
+    turn = result.turn_results[0]
+    assert turn.lost_context is False
+    assert turn.stalled is False
+    assert turn.output_correct is True
+
+
 def test_context_mode_records_an_iteration_limit_as_a_stall_and_nudges() -> None:
     turns = [_viz_turn("t1", expected=_viz())]
     incomplete = TurnIncompleteError("SSE error 502", status_code=502, reason="max_iterations")
@@ -1960,6 +2034,47 @@ def test_reply_restating_returns_the_llm_restatement(monkeypatch: pytest.MonkeyP
     assert reply_restating("USER: weakest? ASSISTANT: Express", "Which format?") == "As I said earlier, Express."
 
 
+@pytest.mark.parametrize(
+    "llm_reply",
+    [
+        'As I said earlier, the formula is: `SELECT SUM({fact/units}) WHERE {label/status} = "Processed"`.',
+        "Use <metric/net_orders> as the denominator.",
+        "As I said earlier, use select sum(units) where status = 'Processed'.",
+        "As I said earlier, it is SUM(units) / COUNT(orders).",
+        "As I said earlier, it is sum(units) / count(orders).",
+        "Use select * from orders.",
+        "Run select region, revenue from orders.",
+        "As I said earlier: select region from orders where status = 'Processed'.",
+    ],
+)
+def test_simulated_user_never_sends_maql(monkeypatch: pytest.MonkeyPatch, llm_reply: str) -> None:
+    monkeypatch.setenv("OPENAI_API_KEY", "sk-test")
+    openai_cls = _fake_openai(monkeypatch, _openai_returning(llm_reply))
+    assert reply_restating("h", "Which formula?") == NO_ANSWER_REPLY
+    assert reply_from_facts("h", "Which formula?", ["Processed orders only."]) == NO_ANSWER_REPLY
+    calls = openai_cls.return_value.chat.completions.create.call_args_list
+    assert len(calls) == 2
+    for call in calls:
+        assert "Never write MAQL" in call.kwargs["messages"][0]["content"]
+
+
+@pytest.mark.parametrize(
+    "llm_reply",
+    [
+        "Select the first option.",
+        "Select the first option from the list.",
+        "SELECT the second option.",
+        "Count (all) is fine.",
+    ],
+)
+def test_simulated_user_keeps_a_plain_reply_that_mentions_select(
+    monkeypatch: pytest.MonkeyPatch, llm_reply: str
+) -> None:
+    monkeypatch.setenv("OPENAI_API_KEY", "sk-test")
+    _fake_openai(monkeypatch, _openai_returning(llm_reply))
+    assert reply_from_facts("h", "Which one?", ["Any is fine."]) == llm_reply
+
+
 def test_simulated_user_reports_a_missing_openai_package(monkeypatch: pytest.MonkeyPatch) -> None:
     monkeypatch.setitem(sys.modules, "openai", None)
     assert ctx_mod._chat("s", "u") is None