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

generate_simulated_response() only saw the assistant's last message and the
ground-truth MAQL, and was instructed to force every clause of that MAQL to
be satisfied "even if the assistant's question doesn't explicitly ask about
it" -- so it would inject filters/constraints the user's original request
never mentioned, even when the assistant's proposal already matched it.

- Thread the original question through (metric_skill.py's
  _execute_single_metric_run already has it in scope; conversation.py's
  TurnDefinition.message carries the same for multi-turn conversations) and
  rewrite the prompt to agree when the original request is already
  satisfied, only adding a clause when it's a reasonable reading of that
  request -- not an unconditional replay of expected_outputs[0].
- Add an explicit branch for the dominant real case: the assistant asking a
  clarifying question with no proposal yet. Without it, the simulated user
  could trivially agree ("nothing proposed yet" == "satisfied") and stall
  the conversation, burning iterations without ever supplying the agent a
  usable answer.
- Replace fuzzy "is this filter a reasonable reading of the request"
  judgment with a deterministic _no_filter_hint(): when the ground-truth
  MAQL has no WHERE clause, the prompt explicitly tells the simulated user
  no filter is needed, closing the exact loophole that caused the bug.
  Matches WHERE as a standalone keyword outside {type/id} identifiers and
  quoted literals (reusing the existing _PROTECTED_RE / same rule as
  _casefold_outside_protected), so a substring like
  {metric/somewhere_sales} isn't mistaken for a real clause.
- conversation.py's metric branch (forwards to
  metric_skill.generate_simulated_response) had 0% test coverage behind a
  bare `except Exception: pass` -- a future signature mismatch would
  silently fall through to the generic fallback prompt. Log the exception
  and add a direct unit test for the branch.
- Restore the max_tokens >= 300 assertion, and reduce the new tests'
  reliance on exact prompt-prose assertions in favor of checking the
  interpolated data and the independently-testable _no_filter_hint() output.

Verified locally: ran the full agent_metric_skill (8 cases) and
agent_conversations (10 cases) suites against ecommerce_demo on
tavern-frank-test -- 18/18 passing with this fix.

QA-29094

Author: FrankHuynh

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

@@ -205,9 +205,9 @@ 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
+            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,7 +30,10 @@
 # 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).
-_PROTECTED_RE = re.compile(r"\{[^}]*\}|\"[^\"]*\"|'[^']*'")
+# The quoted-literal alternatives consume \X escape sequences (including an escaped
+# quote) so an escaped quote inside the literal doesn't end the match early and leak
+# the rest of the literal's text as unprotected.
+_PROTECTED_RE = re.compile(r"\{[^}]*\}|\"(?:[^\"\\]|\\.)*\"|'(?:[^'\\]|\\.)*'")
 
 
 def _strip_outer_parens(s: str) -> str:
@@ -99,7 +102,26 @@ class SimulatedResponseError(RuntimeError):
     """
 
 
-def generate_simulated_response(agent_message: str, expected_output: dict) -> str:
+def _no_filter_hint(expected_maql: str) -> str:
+    """Deterministic nudge for when the ground-truth MAQL has no 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). Strips {type/id} identifiers
+    and quoted literals first (same protected-span rule as `_casefold_outside_protected`) 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.
+    """
+    outside_protected = _PROTECTED_RE.sub(" ", expected_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 (e.g. cancelled orders, a status, a date range), say no filter is needed."
+    )
+
+
+def generate_simulated_response(agent_message: str, expected_output: dict, original_question: str) -> str:
     """Generate a user reply to keep the metric-skill conversation going (gpt-4o-mini).
 
     Raises:
@@ -119,12 +141,19 @@ def generate_simulated_response(agent_message: str, expected_output: dict) -> st
     expected_maql = expected_output.get("maql", "")
     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_filter_hint(expected_maql)}"
     )
     try:
         response = client.chat.completions.create(
@@ -281,7 +310,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, primary_expected, 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_filter_hint,
     _normalize_maql,
     evaluate_agentic_metric_skill,
     generate_simulated_response,
@@ -91,6 +92,38 @@ def test_normalize_maql_removes_select_wrapper():
     assert _normalize_maql("(SELECT {metric/abc})") == "{metric/abc}"
 
 
+def test_no_filter_hint_is_empty_when_the_ground_truth_maql_has_a_where_clause():
+    assert _no_filter_hint("SELECT {metric/foo} WHERE {label/status} = \"active\"") == ""
+
+
+def test_no_filter_hint_is_present_when_the_ground_truth_maql_has_no_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_filter_hint("SELECT SUM({fact/order_unit_quantity})")
+    assert hint != ""
+    assert "no filter is needed" in hint
+
+
+def test_no_filter_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_filter_hint("SELECT {metric/somewhere_sales}") != ""
+
+
+def test_no_filter_hint_ignores_where_inside_a_quoted_literal():
+    assert _no_filter_hint('SELECT {metric/x} = "somewhere nearby"') != ""
+
+
+def test_no_filter_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."""
+    maql = 'SELECT {metric/x} = "Jane\\"s store WHERE something"'
+    assert _no_filter_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 +144,73 @@ 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
@@ -228,7 +315,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 +495,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 +535,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():