fix(gooddata-eval): scope simulated-user pushback to the original request

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

@@ -205,9 +205,12 @@ def _get_sim_user_response(agent_message: str, turn: TurnDefinition, expected_ou
                 generate_simulated_response,
             )
 
-            return generate_simulated_response(agent_message, expected_output)
-        except Exception:
-            pass
+            # A conversation turn only ever carries one expected_output (no multi-candidate
+            # list like agent_metric_skill's fixtures) -- wrap it as a single-item list to
+            # match generate_simulated_response's signature.
+            return generate_simulated_response(agent_message, [expected_output], turn.message)
+        except Exception as exc:
+            print(f"[SIM-USER] metric branch failed for turn {turn.turn_id}: {exc}")
 
     # Generic fallback for other skill types or when expected_output is absent
     import os  # noqa: PLC0415

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

@@ -30,6 +30,11 @@
 # Everything else in MAQL (keywords, operators, numbers, punctuation) carries no
 # case-sensitive meaning, per the MAQL reference (SELECT/BY/WHERE/FOR PREVIOUS/etc.
 # are case-insensitive; only {..} identifiers and quoted literal values are not).
+# Feeds _normalize_maql, the scoring comparator (_best_maql_match) -- do not widen this
+# to handle \X escapes without confirming MAQL literals actually support backslash
+# escaping (unconfirmed; see PR #1760 review). A wrong guess here silently changes
+# maql_correct for the whole eval dataset, not just a hint. _no_where_clause_hint()
+# below has its own, separately-scoped regex for that reason.
 _PROTECTED_RE = re.compile(r"\{[^}]*\}|\"[^\"]*\"|'[^']*'")
 
 
@@ -99,9 +104,43 @@ class SimulatedResponseError(RuntimeError):
     """
 
 
-def generate_simulated_response(agent_message: str, expected_output: dict) -> str:
+# Separate from _PROTECTED_RE on purpose: this one only feeds a same-turn LLM-prompt hint
+# (see _no_where_clause_hint), never the scoring comparator, so it can afford to consume
+# \X escape sequences inside quoted literals without risking maql_correct semantics.
+_HINT_PROTECTED_RE = re.compile(r"\{[^}]*\}|\"(?:[^\"\\]|\\.)*\"|'(?:[^'\\]|\\.)*'")
+
+
+def _no_where_clause_hint(expected_maqls: list[str]) -> str:
+    """Deterministic nudge for when NONE of the accepted candidate MAQLs has a WHERE clause.
+
+    Without this, whether to add a filter is left entirely to the simulating LLM's judgment
+    of what the original request "implies" -- the same fuzzy reasoning that caused it to
+    inject an unrequested filter in the first place (QA-29094). Checks every candidate, not
+    just the first: _best_maql_match accepts any of them, so hinting off just candidate 0
+    would risk steering the agent away from a filtered candidate the scorer would still have
+    accepted (the mirror-image of the original bug). Strips {type/id} identifiers and quoted
+    literals first so a "where" substring inside one of those -- e.g.
+    `{metric/somewhere_sales}`, or a literal value containing the word -- doesn't get
+    mistaken for a real WHERE clause.
+    """
+    for maql in expected_maqls:
+        outside_protected = _HINT_PROTECTED_RE.sub(" ", maql)
+        if re.search(r"\bWHERE\b", outside_protected, re.IGNORECASE):
+            return ""
+    return (
+        " This metric needs no filter. If the assistant asks about excluding or filtering "
+        "anything, say no filter is needed."
+    )
+
+
+def generate_simulated_response(agent_message: str, expected_outputs: list[dict], original_question: str) -> str:
     """Generate a user reply to keep the metric-skill conversation going (gpt-4o-mini).
 
+    ``expected_outputs`` is the fixture's full candidate list (as accepted by
+    ``_best_maql_match``), not just the first one -- the ground-truth MAQL woven into the
+    prompt still comes from candidate 0, but the no-filter hint checks all of them (see
+    ``_no_where_clause_hint``).
+
     Raises:
         SimulatedResponseError: openai is not installed, OPENAI_API_KEY is unset, or the
             provider call failed.
@@ -116,15 +155,23 @@ def generate_simulated_response(agent_message: str, expected_output: dict) -> st
         raise SimulatedResponseError("OPENAI_API_KEY environment variable is not set")
 
     client = OpenAI(api_key=api_key)
-    expected_maql = expected_output.get("maql", "")
+    expected_maql = expected_outputs[0].get("maql", "") if expected_outputs else ""
+    expected_maqls = [eo.get("maql", "") for eo in expected_outputs]
     prompt = (
         f"You are simulating a user in a conversation with a BI assistant that creates metrics. "
+        f"The user's original request was: '{original_question}'. "
         f"The assistant said: '{agent_message}'. "
         f"The user's ground-truth intended metric is exactly this MAQL: {expected_maql}. "
-        f"Reply as the user. You MUST ensure every clause of that MAQL (including any WHERE/filter "
-        f"conditions) is eventually satisfied, and quote field/label identifiers verbatim from it -- "
-        f"never paraphrase or drop a clause, even if the assistant's question doesn't explicitly ask "
-        f"about it. If the assistant's offered options omit a required filter, add it yourself."
+        f"Reply as the user. If the assistant is asking a clarifying question rather than proposing "
+        f"a metric, answer that question directly using the ground-truth MAQL -- quote field/label "
+        f"identifiers verbatim -- instead of merely agreeing. "
+        f"If the assistant's proposal already satisfies the ORIGINAL REQUEST above, agree and confirm "
+        f"-- do not introduce new requirements the original request never mentioned. "
+        f"Only if the assistant's proposal is missing something the original request actually implies "
+        f"(e.g. a filter/clause from the ground-truth MAQL that is a reasonable reading of the original "
+        f"request), point it out and add it yourself, quoting field/label identifiers verbatim from the "
+        f"ground-truth MAQL."
+        f"{_no_where_clause_hint(expected_maqls)}"
     )
     try:
         response = client.chat.completions.create(
@@ -234,7 +281,6 @@ def _execute_single_metric_run(
     ``_delete_metric``) so it cannot leak into — and be reused by — a later test
     sharing the workspace.
     """
-    primary_expected = expected_outputs[0] if expected_outputs else {}
     metric_result: dict | None = None
     created_metric_ids: list[str] = []
     turns = 0
@@ -281,7 +327,7 @@ def _execute_single_metric_run(
             if _iteration >= max_iterations - 1:
                 break
             try:
-                current_question = generate_simulated_response(response_text, primary_expected)
+                current_question = generate_simulated_response(response_text, expected_outputs, question)
             except SimulatedResponseError as exc:
                 print(f"[SIM-USER] Simulated reply failed for conversation {conversation_id}: {exc}")
                 break

packages/gooddata-eval/tests/test_agentic_conversation.py

@@ -8,6 +8,7 @@
     ConversationFixture,
     TurnDefinition,
     TurnResult,
+    _get_sim_user_response,
     _resolve_refs,
     evaluate_agentic_conversation,
     run_agentic_conversation,
@@ -107,6 +108,30 @@ def test_resolve_refs_substitutes():
     assert result == {"maql": "SELECT {metric/foo}"}
 
 
+def test_get_sim_user_response_metric_branch_forwards_the_turn_message():
+    """QA-29094 follow-up: every test in this file patches out `_get_sim_user_response`
+    itself, so its metric branch (which forwards to
+    ``metric_skill.generate_simulated_response``) had 0% coverage -- a future signature
+    change there would raise inside the bare ``except Exception`` and silently fall through
+    to the generic fallback prompt instead of failing loudly."""
+    turn = TurnDefinition(
+        turn_id="t1",
+        message="I need a metric for total ordered units",
+        expected_skill="metric",
+        expected_output_type="metric",
+    )
+    expected_output = {"maql": "SELECT SUM({fact/order_unit_quantity})"}
+
+    with patch(
+        "gooddata_eval.core.agentic.metric_skill.generate_simulated_response",
+        return_value="Yes, that works.",
+    ) as mock_sim:
+        reply = _get_sim_user_response("Should I create this metric?", turn, expected_output)
+
+    assert reply == "Yes, that works."
+    mock_sim.assert_called_once_with("Should I create this metric?", [expected_output], turn.message)
+
+
 def test_run_agentic_conversation_single_turn():
     mock_client = MagicMock()
     mock_client.create_conversation.return_value = "conv-1"

packages/gooddata-eval/tests/test_agentic_metric_skill.py

@@ -13,6 +13,7 @@
     SimulatedResponseError,
     _delete_metric,
     _extract_metric_result,
+    _no_where_clause_hint,
     _normalize_maql,
     evaluate_agentic_metric_skill,
     generate_simulated_response,
@@ -91,6 +92,54 @@ def test_normalize_maql_removes_select_wrapper():
     assert _normalize_maql("(SELECT {metric/abc})") == "{metric/abc}"
 
 
+def test_no_where_clause_hint_is_empty_when_a_candidate_has_a_where_clause():
+    assert _no_where_clause_hint(['SELECT {metric/foo} WHERE {label/status} = "active"']) == ""
+
+
+def test_no_where_clause_hint_is_present_when_no_candidate_has_a_where_clause():
+    """QA-29094 follow-up: whether to add a filter must not be left to the simulating LLM's
+    judgment of what the original request "implies" -- that fuzzy reasoning is exactly what
+    caused it to inject an unrequested filter in the first place."""
+    hint = _no_where_clause_hint(["SELECT SUM({fact/order_unit_quantity})"])
+    assert hint != ""
+    assert "no filter is needed" in hint
+
+
+def test_no_where_clause_hint_stays_silent_if_any_candidate_has_a_where_clause():
+    """PR #1760 review (Henry): _no_where_clause_hint used to see only expected_outputs[0].
+    A fixture like agent_metric_skill_4.json lists an unfiltered candidate first and a
+    filtered one second -- both accepted by _best_maql_match. Hinting "no filter needed"
+    off candidate 0 alone would steer the agent away from the filtered candidate even
+    though the scorer would still take it -- the mirror image of the original QA-29094 bug.
+    """
+    candidates = [
+        "SELECT SUM({fact/order_unit_quantity})",
+        'SELECT SUM({fact/order_unit_quantity}) WHERE {label/order_status} = "Processed"',
+    ]
+    assert _no_where_clause_hint(candidates) == ""
+
+
+def test_no_where_clause_hint_ignores_where_inside_an_identifier():
+    """CodeRabbit finding on PR #1760: a naive substring check treats the "where" inside
+    an identifier like {metric/somewhere_sales} as a real WHERE clause and wrongly stays
+    silent -- it must be stripped as a protected span before matching."""
+    assert _no_where_clause_hint(["SELECT {metric/somewhere_sales}"]) != ""
+
+
+def test_no_where_clause_hint_ignores_where_inside_a_quoted_literal():
+    assert _no_where_clause_hint(['SELECT {metric/x} = "somewhere nearby"']) != ""
+
+
+def test_no_where_clause_hint_ignores_where_inside_a_literal_with_an_escaped_quote():
+    """CodeRabbit finding on PR #1760: an escaped quote inside a quoted literal ended the
+    protected-span match early, leaking the rest of the literal's text -- including a
+    standalone WHERE -- as unprotected. Uses _HINT_PROTECTED_RE (escape-aware), kept
+    separate from the shared _PROTECTED_RE that feeds the maql_correct comparator (PR
+    #1760 review, Henry) -- see test_normalize_maql_does_not_consume_escape_sequences."""
+    maql = 'SELECT {metric/x} = "Jane\\"s store WHERE something"'
+    assert _no_where_clause_hint([maql]) != ""
+
+
 def test_generate_simulated_response_prompt_preserves_maql_fidelity(monkeypatch):
     """Regression test for a live-reproduced bug: the old prompt ("reply briefly",
     no instruction to cover clauses the assistant didn't ask about) let the
@@ -111,19 +160,75 @@ def test_generate_simulated_response_prompt_preserves_maql_fidelity(monkeypatch)
     monkeypatch.setitem(sys.modules, "openai", fake_openai_module)
 
     expected_output = {"maql": 'SELECT {metric/spend_amount_-_cutcgco} WHERE {label/ecommerce_indicator_code} = "1"'}
-    generate_simulated_response("Which base metric should I use?", expected_output)
+    generate_simulated_response(
+        "Which base metric should I use?", [expected_output], "I need a metric for spend amount"
+    )
 
     call_kwargs = mock_client.chat.completions.create.call_args.kwargs
     sent_prompt = call_kwargs["messages"][0]["content"]
 
     assert expected_output["maql"] in sent_prompt
     assert "verbatim" in sent_prompt
-    assert "every clause" in sent_prompt
-    assert "WHERE" in sent_prompt or "filter" in sent_prompt.lower()
-    assert "reply briefly" not in sent_prompt.lower()
+    assert "filter" in sent_prompt.lower()
+    # Guards against a truncated reply mid-MAQL -- the LLM was cutting fidelity short under
+    # the old, lower budget before this was raised (see the docstring above).
     assert call_kwargs["max_tokens"] >= 300
 
 
+def test_generate_simulated_response_prompt_agrees_when_the_original_request_is_already_satisfied(monkeypatch):
+    """Regression test for QA-29094: the old prompt told the simulated user to force every
+    clause of the ground-truth MAQL regardless of what the original request actually asked
+    for, so it would inject filters/constraints the user never mentioned even when the
+    assistant's proposal already matched the request. The prompt must now carry the
+    original request and instruct the simulated user to agree when it's already satisfied.
+    """
+    monkeypatch.setenv("OPENAI_API_KEY", "test-key")
+    mock_client = MagicMock()
+    mock_response = MagicMock()
+    mock_response.choices = [MagicMock(message=MagicMock(content="ok"))]
+    mock_client.chat.completions.create.return_value = mock_response
+    fake_openai_module = types.SimpleNamespace(OpenAI=MagicMock(return_value=mock_client), OpenAIError=Exception)
+    monkeypatch.setitem(sys.modules, "openai", fake_openai_module)
+
+    original_question = "I need a metric for total ordered units called Total Order Quantity"
+    expected_output = {"maql": "SELECT SUM({fact/order_unit_quantity}) WHERE {fact/order_status} != 'cancelled'"}
+    generate_simulated_response("Should I create this metric?", [expected_output], original_question)
+
+    sent_prompt = mock_client.chat.completions.create.call_args.kwargs["messages"][0]["content"]
+
+    # Structural checks on the interpolated data -- robust to prompt-wording edits.
+    assert original_question in sent_prompt
+    assert expected_output["maql"] in sent_prompt
+    assert "reply briefly" not in sent_prompt.lower()
+    # A ground-truth MAQL with a WHERE clause must not trigger the no-filter-needed hint.
+    assert "no filter is needed" not in sent_prompt
+
+
+def test_generate_simulated_response_prompt_handles_a_clarifying_question(monkeypatch):
+    """QA-29094 follow-up: the two-branch prompt ("already satisfies" / "missing something")
+    both assume the assistant made a proposal -- but the dominant real case is the assistant
+    asking a clarifying question first (no proposal exists yet to judge as satisfying or not).
+    Without an explicit instruction, the simulating LLM could classify "nothing proposed yet"
+    as trivially "satisfied" and reply "yes, that works", leaving the agent no closer to a
+    usable metric and burning iterations."""
+    monkeypatch.setenv("OPENAI_API_KEY", "test-key")
+    mock_client = MagicMock()
+    mock_response = MagicMock()
+    mock_response.choices = [MagicMock(message=MagicMock(content="ok"))]
+    mock_client.chat.completions.create.return_value = mock_response
+    fake_openai_module = types.SimpleNamespace(OpenAI=MagicMock(return_value=mock_client), OpenAIError=Exception)
+    monkeypatch.setitem(sys.modules, "openai", fake_openai_module)
+
+    expected_output = {"maql": "SELECT SUM({fact/order_unit_quantity})"}
+    generate_simulated_response("Which base metric should I use?", [expected_output], "I need total ordered units")
+
+    sent_prompt = mock_client.chat.completions.create.call_args.kwargs["messages"][0]["content"]
+
+    assert "clarifying question" in sent_prompt
+    # No WHERE clause in the ground truth -- the no-filter hint must fire here too.
+    assert "no filter is needed" in sent_prompt
+
+
 def test_normalize_maql_is_case_insensitive_for_keywords():
     """Regression test for a live-reproduced bug: 'FOR PREVIOUS(...)' vs
     'FOR Previous(...)' scored as a mismatch even though MAQL keywords are
@@ -147,6 +252,19 @@ def test_normalize_maql_preserves_quoted_literal_case():
     assert _normalize_maql('WHERE {label/status} = "Active"') != _normalize_maql('WHERE {label/status} = "active"')
 
 
+def test_normalize_maql_does_not_consume_escape_sequences():
+    """PR #1760 review (Henry): _PROTECTED_RE feeds this comparator (via
+    _casefold_outside_protected), so it must NOT treat \\X as an escape sequence unless
+    MAQL literals are confirmed to support backslash escaping (unconfirmed). A `\\"`
+    inside a literal must still end that literal at the next real quote -- not swallow
+    everything up to the following quoted value, which would leave a real keyword like
+    AND uncasefolded and a later literal's case wrongly casefolded."""
+    maql = 'SELECT {metric/x} WHERE {label/path} = "C:\\" AND {label/y} = "Active"'
+    normalized = _normalize_maql(maql)
+    assert "and {label/y}" in normalized  # AND is a keyword outside the literal -- casefolded
+    assert '"Active"' in normalized  # the second literal's case is untouched -- not "active"
+
+
 def test_metric_run_result_fields():
     r = MetricRunResult(
         conversation_id="c1",
@@ -228,7 +346,7 @@ def test_run_agentic_metric_skill_closes_client_on_no_result():
     mock_client.close.assert_called_once()
     assert summary.pass_at_k is False
     assert summary.best.metric_created is False
-    mock_sim.assert_called_once_with("I will work on that.", {"maql": "SELECT {metric/foo}"})
+    mock_sim.assert_called_once_with("I will work on that.", [{"maql": "SELECT {metric/foo}"}], "Create metric foo")
 
 
 def test_run_agentic_metric_skill_uses_initial_conversation_for_run_0():
@@ -408,15 +526,15 @@ def test_generate_simulated_response_without_an_api_key():
         patch.dict(os.environ, {}, clear=True),
         pytest.raises(SimulatedResponseError, match="OPENAI_API_KEY"),
     ):
-        generate_simulated_response("Which brand field?", {"maql": "SELECT {metric/foo}"})
+        generate_simulated_response("Which brand field?", [{"maql": "SELECT {metric/foo}"}], "I need a metric for foo")
 
 
 def test_generate_simulated_response_without_the_openai_package():
     with (
         patch.dict(sys.modules, {"openai": None}),
         pytest.raises(SimulatedResponseError, match="openai package is required"),
     ):
-        generate_simulated_response("Which brand field?", {"maql": "SELECT {metric/foo}"})
+        generate_simulated_response("Which brand field?", [{"maql": "SELECT {metric/foo}"}], "I need a metric for foo")
 
 
 def test_run_agentic_metric_skill_fails_the_run_when_the_simulated_reply_cannot_be_generated():
@@ -448,7 +566,9 @@ def test_run_agentic_metric_skill_fails_the_run_when_the_simulated_reply_cannot_
     assert summary.best.metric_created is False
     assert summary.best.total_turns == 1.0
     mock_client.close.assert_called_once()
-    mock_sim.assert_called_once_with("Which brand field should I count?", {"maql": "SELECT {metric/foo}"})
+    mock_sim.assert_called_once_with(
+        "Which brand field should I count?", [{"maql": "SELECT {metric/foo}"}], "Create metric foo"
+    )
 
 
 def test_run_agentic_metric_skill_accumulates_reasoning_steps_across_iterations():