From e8f35dd19a3c03686f7cbb5049a837b4ddc3d372 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Thu, 30 Jul 2026 22:14:17 +0700 Subject: [PATCH 1/3] fix(gooddata-eval): treat alert proposal part as confirmation signal JIRA: QA-28774 Co-Authored-By: Claude Opus 5 (1M context) --- .../gooddata_eval/core/agentic/alert_skill.py | 25 ++++-- .../core/agentic/conversation.py | 6 +- .../src/gooddata_eval/core/chat/sse_client.py | 7 ++ .../src/gooddata_eval/core/models.py | 4 + .../tests/test_agentic_alert_skill.py | 85 +++++++++++++++++++ .../tests/test_agentic_conversation.py | 63 +++++++++++++- .../gooddata-eval/tests/test_sse_client.py | 38 +++++++++ 7 files changed, 221 insertions(+), 7 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index 3d3adb15a..ed00a2f44 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -311,11 +311,24 @@ def _extract_alert_call(tool_call_events: list[ToolCallEvent]) -> tuple[str | No return None, {}, False -def _is_asking_clarification(text: str) -> bool: - if not text: - return False - t = text.lower() - return "?" in t or "could you" in t or "please" in t or "clarif" in t +def render_alert_proposal(proposal: dict) -> str: + """Render an alert-proposal part as the text the simulated user reacts to. + + The alert skill's confirmation step deliberately emits no text part (GDAI-2032) — the + prompt and the CTA live only in the proposal payload, which the frontend renders as a + widget. Dumping the payload (rather than prose) keeps recipients, condition, trigger and + dashboard visible so the simulated user can still verify them against its goal, and does + not need updating whenever ``AlertProposal`` grows a field. + """ + cta = proposal.get("cta") or "Should I create this alert?" + summary = {k: v for k, v in proposal.items() if k != "cta"} + alert = dict(summary.get("alert") or {}) + # The AFM execution block is opaque wire dicts — noise that would crowd out the fields + # the simulated user actually has to check. + alert.pop("execution", None) + if alert: + summary["alert"] = alert + return f"{cta}\n\nAlert proposal:\n{json.dumps(summary, indent=2, sort_keys=True)}" def run_agentic_alert_skill( @@ -352,6 +365,8 @@ def _run_once(conv_id: str) -> AlertRunResult: alert_id_to_delete = alert_id break response_text = (chat_result.text_response or "").strip() + if not response_text and chat_result.alert_proposals: + response_text = render_alert_proposal(chat_result.alert_proposals[-1]) # Stop if agent gave a completely empty response (stuck) if not response_text and not chat_result.tool_call_events: break diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index 7d8d1d7c2..6b79b3279 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -11,6 +11,7 @@ from gooddata_sdk import GoodDataSdk from pydantic import BaseModel +from gooddata_eval.core.agentic.alert_skill import render_alert_proposal from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.models import ChatResult, ToolCallEvent @@ -322,7 +323,10 @@ def run_agentic_conversation( break response_text = (chat_result.text_response or "").strip() - if _is_asking_clarification(response_text) and clarification_turns < max_clarification_turns: + if not response_text and chat_result.alert_proposals: + response_text = render_alert_proposal(chat_result.alert_proposals[-1]) + asking = _is_asking_clarification(response_text) or bool(chat_result.alert_proposals) + if asking and clarification_turns < max_clarification_turns: clarification_turns += 1 total_clarification_turns += 1 current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index 1e43ab858..091436b27 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -102,6 +102,7 @@ class _SseAccumulator: text_parts: list[str] = field(default_factory=list) viz_reasoning_parts: list[str] = field(default_factory=list) visualizations: list[dict[str, Any]] = field(default_factory=list) + alert_proposals: list[dict[str, Any]] = field(default_factory=list) tool_call_events: list[dict[str, Any]] = field(default_factory=list) call_id_to_event_index: dict[str, int] = field(default_factory=dict) reasoning_steps: list[dict[str, Any]] = field(default_factory=list) @@ -125,6 +126,11 @@ def _handle_multipart(content: dict[str, Any], acc: _SseAccumulator) -> None: acc.viz_reasoning_parts.append(t) elif ptype == "visualization" and part.get("visualization"): acc.visualizations.append(part["visualization"]) + elif ptype == "alertProposal": + # Record the part even when the server could not resolve the proposal payload + # (``alertProposal: null``) — its mere presence is the confirmation signal, and + # the reader falls back to a default CTA. + acc.alert_proposals.append(part.get("alertProposal") or {}) def _handle_reasoning(content: dict[str, Any], acc: _SseAccumulator) -> None: @@ -161,6 +167,7 @@ def _handle_tool_result(content: dict[str, Any], acc: _SseAccumulator) -> None: def _build_chat_result(acc: _SseAccumulator) -> ChatResult: payload: dict[str, Any] = { "textResponse": "\n".join(acc.text_parts) or None, + "alertProposals": acc.alert_proposals, "toolCallEvents": acc.tool_call_events, "reasoningStepCount": len(acc.reasoning_steps), } diff --git a/packages/gooddata-eval/src/gooddata_eval/core/models.py b/packages/gooddata-eval/src/gooddata_eval/core/models.py index 981000e1a..336c313b9 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/models.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/models.py @@ -92,6 +92,10 @@ class ChatResult(BaseModel): text_response: str | None = Field(default=None, alias="textResponse") created_visualizations: CreatedVisualizations | None = Field(default=None, alias="createdVisualizations") + # Alert-proposal parts of the agent's multipart response. The alert skill's confirmation + # step emits ONLY this part (no text part), so its `cta` is the only "the agent is asking + # a question" signal the simulated-user loops can key off. + alert_proposals: list[dict] = Field(default_factory=list, alias="alertProposals") tool_call_events: list[ToolCallEvent] = Field(default_factory=list, alias="toolCallEvents") reasoning_step_count: int = Field(default=0, alias="reasoningStepCount") conversation_id: str | None = Field(default=None, alias="conversationId") diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index d6026da04..dfe037511 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -8,10 +8,23 @@ _deep_subset, _normalize_expected_output, _to_number, + render_alert_proposal, run_agentic_alert_skill, ) from gooddata_eval.core.models import ChatResult +_PROPOSAL = { + "title": "# of Orders Alert - Greater Than 500", + "cta": "Should I create this alert?", + "recipients": [{"email": "admin@gooddata.com"}], + "dashboard": {"id": "dash-1", "title": "Orders overview"}, + "alert": { + "trigger": "ALWAYS", + "condition": {"comparison": {"operator": "GREATER_THAN", "right": {"value": 500}}}, + "execution": {"measures": [{"opaque": "afm"}]}, + }, +} + def test_to_number_int(): assert _to_number("42") == 42 @@ -156,3 +169,75 @@ def test_run_agentic_alert_skill_creates_fresh_conversations_for_remaining_runs( ) assert mock_client.create_conversation.call_count == 2 assert mock_client.delete_conversation.call_count == 2 + + +def test_render_alert_proposal_keeps_verifiable_fields_and_drops_afm(): + rendered = render_alert_proposal(_PROPOSAL) + # The CTA leads so the simulated user reads it as a question. + assert rendered.startswith("Should I create this alert?") + # Rule 3 of the sim-user prompt requires verifying recipients against its goal. + assert "admin@gooddata.com" in rendered + assert "GREATER_THAN" in rendered + assert "Orders overview" in rendered + # Opaque AFM wire dicts must not crowd out the fields above. + assert "execution" not in rendered + + +def test_render_alert_proposal_falls_back_to_default_cta(): + assert render_alert_proposal({}).startswith("Should I create this alert?") + + +def test_run_agentic_alert_skill_answers_proposal_only_confirmation_turn(): + """GDAI-2032 regression: confirmation turn has no text part, only an alertProposal. + + Without the fallback the simulated user is handed an empty agent message, so the agent + never receives an explicit "yes" and create_metric_alert is never called. + """ + proposal_turn = ChatResult.model_validate( + { + "text_response": None, + "alertProposals": [_PROPOSAL], + "tool_call_events": [ + {"functionName": "prepare_metric_alert_proposal", "functionArguments": "{}", "result": None} + ], + } + ) + created_turn = ChatResult.model_validate( + { + "text_response": "Alert created.", + "tool_call_events": [ + { + "functionName": "create_metric_alert", + "functionArguments": '{"operator": "GREATER_THAN", "threshold": 500}', + "result": '{"id": "alert-1"}', + } + ], + } + ) + mock_client = MagicMock() + mock_client.send_message.side_effect = [proposal_turn, created_turn] + + with ( + patch("gooddata_eval.core.agentic.alert_skill.ChatClient", return_value=mock_client), + patch( + "gooddata_eval.core.agentic.alert_skill.generate_simulated_alert_response", + return_value="Yes, please proceed to create the alert.", + ) as mock_sim, + patch("gooddata_eval.core.agentic.alert_skill._delete_alert"), + ): + summary = run_agentic_alert_skill( + host="http://host", + token="tok", + workspace_id="ws1", + question="Notify me whenever the number of orders goes above 500", + expected_output={"operator": "GREATER_THAN", "threshold": 500}, + k=1, + max_iterations=6, + initial_conversation_id="conv-1", + ) + + agent_message = mock_sim.call_args.args[0] + assert "Should I create this alert?" in agent_message + assert "admin@gooddata.com" in agent_message + assert summary.best.eval.alert_created is True + assert summary.best.alert_id == "alert-1" diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 119146d3c..9d2234f33 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -10,7 +10,7 @@ _resolve_refs, run_agentic_conversation, ) -from gooddata_eval.core.models import ToolCallEvent +from gooddata_eval.core.models import ChatResult, ToolCallEvent def _skills_tc(*skills): @@ -291,3 +291,64 @@ def test_run_agentic_conversation_deletes_metrics_even_when_a_later_turn_raises( ) mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "m1") + + +def _alert_turn_fixture(): + return ConversationFixture( + id="conv-alert", + expected_skills=["alert"], + turns=[ + TurnDefinition( + turn_id="create_alert", + message="Now alert me when the metric drops below 100.", + expected_skill="alert", + expected_output_type="tool_call", + expected_tool_name="create_metric_alert", + ) + ], + ) + + +def test_run_agentic_conversation_treats_alert_proposal_as_a_clarification(): + """GDAI-2032 regression: a proposal-only turn has no text, so the old text-only check + stopped the turn instead of replying, and create_metric_alert never happened.""" + proposal_turn = ChatResult.model_validate( + { + "text_response": None, + "alertProposals": [{"cta": "Should I create this alert?", "recipients": [{"email": "a@b.com"}]}], + "toolCallEvents": [ + {"functionName": "set_skills", "functionArguments": '{"skills": ["alert"]}', "result": None}, + {"functionName": "prepare_metric_alert_proposal", "functionArguments": "{}", "result": None}, + ], + } + ) + created_turn = ChatResult.model_validate( + { + "text_response": "Alert created.", + "toolCallEvents": [ + {"functionName": "create_metric_alert", "functionArguments": "{}", "result": '{"id": "alert-1"}'} + ], + } + ) + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [proposal_turn, created_turn] + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + patch( + "gooddata_eval.core.agentic.conversation._get_sim_user_response", + return_value="Yes, please create it.", + ) as mock_sim, + ): + result = run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=_alert_turn_fixture(), + ) + + assert "Should I create this alert?" in mock_sim.call_args.args[0] + assert result.turn_results[0].clarification_turns_used == 1 + assert result.turn_results[0].skill_success is True diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index a4946e425..c5590e428 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -82,6 +82,44 @@ def test_parse_sse_lines_prefers_multipart_viz_over_adhoc_fallback(): assert result.created_visualizations.objects[0].id == "real" +def test_parse_sse_lines_collects_alert_proposal_without_text_part(): + """The alert skill's confirmation turn emits ONLY an alertProposal part (GDAI-2032). + + Pins the wire contract the simulated-user loops depend on: part ``type`` is + ``alertProposal`` and the payload lives under the ``alertProposal`` key. + """ + proposal = { + "title": "# of Orders Alert - Greater Than 500", + "cta": "Should I create this alert?", + "recipients": [{"email": "admin@gooddata.com"}], + "alert": {"trigger": "ALWAYS", "execution": {"measures": [{"opaque": "afm"}]}}, + } + lines = [ + 'data: {"item": {"role": "assistant", "content": {"type": "toolCall", "callId": "c1", ' + '"name": "prepare_metric_alert_proposal", "arguments": {}}}}', + f'data: {{"item": {{"role": "assistant", "content": {{"type": "multipart", ' + f'"parts": [{{"type": "alertProposal", "alertProposal": {json.dumps(proposal)}}}]}}}}}}', + ] + result = parse_sse_lines(lines) + assert result.text_response is None + assert result.alert_proposals == [proposal] + + +def test_parse_sse_lines_keeps_alert_proposal_part_when_payload_is_null(): + """Presence of the part is the confirmation signal even if the server did not resolve it.""" + lines = [ + 'data: {"item": {"role": "assistant", "content": {"type": "multipart", ' + '"parts": [{"type": "alertProposal", "alertProposal": null}]}}}', + ] + result = parse_sse_lines(lines) + assert result.alert_proposals == [{}] + + +def test_parse_sse_lines_has_no_alert_proposals_by_default(): + lines = ['data: {"item": {"role": "assistant", "content": {"type": "text", "text": "Done"}}}'] + assert parse_sse_lines(lines).alert_proposals == [] + + @pytest.mark.parametrize("code", [429, 502, 503, 504]) def test_parse_sse_lines_transient_status_codes(code): with pytest.raises(TransientChatError) as ei: From b1d50124090d1670d69bd022126ebc550f9c2549 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Fri, 31 Jul 2026 10:03:32 +0700 Subject: [PATCH 2/3] fix(gooddata-eval): drop AFM execution even when it is the only alert field JIRA: QA-28774 Co-Authored-By: Claude Opus 5 (1M context) --- .../src/gooddata_eval/core/agentic/alert_skill.py | 4 +++- packages/gooddata-eval/tests/test_agentic_alert_skill.py | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index ed00a2f44..1a7d2a188 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -326,7 +326,9 @@ def render_alert_proposal(proposal: dict) -> str: # The AFM execution block is opaque wire dicts — noise that would crowd out the fields # the simulated user actually has to check. alert.pop("execution", None) - if alert: + if "alert" in summary: + # Key off presence, not truthiness: an alert whose only key was `execution` must + # still be replaced, otherwise the original (execution-bearing) dict survives. summary["alert"] = alert return f"{cta}\n\nAlert proposal:\n{json.dumps(summary, indent=2, sort_keys=True)}" diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index dfe037511..fa5dcbedd 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -183,6 +183,13 @@ def test_render_alert_proposal_keeps_verifiable_fields_and_drops_afm(): assert "execution" not in rendered +def test_render_alert_proposal_drops_afm_when_execution_is_the_only_alert_field(): + # Truthiness-gated replacement used to leave the original execution-bearing dict in place. + rendered = render_alert_proposal({"alert": {"execution": {"measures": [{"opaque": "afm"}]}}}) + assert "execution" not in rendered + assert "opaque" not in rendered + + def test_render_alert_proposal_falls_back_to_default_cta(): assert render_alert_proposal({}).startswith("Should I create this alert?") From 56cbe098801d7c74325d1dd301314473cd815cba Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Fri, 31 Jul 2026 11:03:34 +0700 Subject: [PATCH 3/3] fix(ci): bound uv in the tox group so the test image keeps 0.11.x Unrelated to the rest of this PR; unblocks `unit-tests`, which every Python-changing PR now hits. Co-Authored-By: Claude Opus 5 (1M context) --- Dockerfile | 8 +++++--- pyproject.toml | 8 +++++++- uv.lock | 2 ++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/Dockerfile b/Dockerfile index 20edd2703..2664d3a5d 100644 --- a/Dockerfile +++ b/Dockerfile @@ -39,9 +39,11 @@ WORKDIR /data # to ensure consistent dependencies COPY pyproject.toml uv.lock ./ -# Install tox and tox-uv as system packages so they're available globally -# We use uv pip install to install packages from the tox dependency group in pyproject.toml -# by reading from the lock file which ensures consistent versions +# Install tox and tox-uv as system packages so they're available globally. +# NOTE: `uv pip install --group` reads the group's requirements from pyproject.toml but +# resolves them FRESH from the index -- it does NOT read uv.lock. Every version that must +# stay fixed therefore needs an explicit bound in the group itself; in particular `uv`, +# whose console script installs over the binary copied above. # Clean up dependency files after installation to reduce image size RUN set -x \ && uv pip install --system --group tox \ diff --git a/pyproject.toml b/pyproject.toml index 8e017109a..1ea745e35 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -76,7 +76,13 @@ release = [ ] tox = [ "tox~=4.56.1", - "tox-uv~=1.35.2" + "tox-uv~=1.35.2", + # tox-uv depends on the uv PyPI package without a version bound, and the Dockerfile + # installs this group with `uv pip install`, which resolves fresh instead of reading + # uv.lock. Without this bound the resolver picks the newest uv, whose console script + # then shadows the pinned binary in the image and trips required-version at runtime. + # Keep in sync with [tool.uv] required-version above. + "uv~=0.11.0", ] [tool.ruff] diff --git a/uv.lock b/uv.lock index e6d572865..2596bccdd 100644 --- a/uv.lock +++ b/uv.lock @@ -1148,6 +1148,7 @@ test = [ tox = [ { name = "tox" }, { name = "tox-uv" }, + { name = "uv" }, ] type = [ { name = "ty" }, @@ -1198,6 +1199,7 @@ test = [ tox = [ { name = "tox", specifier = "~=4.56.1" }, { name = "tox-uv", specifier = "~=1.35.2" }, + { name = "uv", specifier = "~=0.11.0" }, ] type = [{ name = "ty", specifier = "~=0.0.55" }]