diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py index 665d3681c..55bbd8f89 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_conversation_context.py +++ b/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]*\"|(? 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." diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 76f6b6baa..37907e4cc 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/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 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