From 62416302baf22c78780698de0fb17f8f38e9bba1 Mon Sep 17 00:00:00 2001 From: Oanh Nguyen Date: Fri, 6 Nov 2026 11:19:44 +0700 Subject: [PATCH 1/2] feat(gooddata-eval): score a rework of the open document The document skill scored new drafts only: a run that edited the document open in the editor failed on base_document_id, so a dataset could not check the copilot's edits at all. An item can now state `rework` with the base_document_id of the open document. The run must then report that id and reworks_open_document, and it may also have to keep listed page ids (a hidden page counts as lost), keep listed charts on their pages, leave a stated title, and keep the document's content.filters. Each is a strict check published only when the item states it. A new draft must not claim reworks_open_document either: that, and a base_document_id on a new draft, now fail document_new_draft rather than document_not_saved, which keeps meaning only that the draft is not saved yet. A rework is scored on aac-v2 documents only: a v1 draft fails it once, saying so, rather than through every check. v1 is on its way out, and its parts carry the Report names the wire check already flags. A malformed rework is rejected before any request, and so is one whose base_document_id is not the id of user_context.view.document: otherwise every run would fail on base_document_id and read as a copilot regression. Expected filters are compared as sent, keyed by filter id, so they have to be copied from a real aac-v2 run. jira: QA-30024 risk: low Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011CpUh9hFjL8SbdpFxBGRXp --- .../core/agentic/document_skill.py | 242 +++++++++++++- .../tests/test_agentic_document_skill.py | 308 +++++++++++++++++- 2 files changed, 536 insertions(+), 14 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/document_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/document_skill.py index 743c137d2..b0cf53062 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/document_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/document_skill.py @@ -66,8 +66,17 @@ _COVER_PAGE = "cover" _CONTENT_PAGE = "content" _EXPECTATION_KEYS = frozenset( - {"period", "visualizations", "narrative", "expects_clarification", "asked_first_required", "clarification_reply"} + { + "period", + "visualizations", + "narrative", + "expects_clarification", + "asked_first_required", + "clarification_reply", + "rework", + } ) +_REWORK_KEYS = frozenset({"base_document_id", "keeps_pages", "page_charts", "title", "filters"}) # Template tokens the document renders at export time ({documentName}, {periodStart}, ...). _PLACEHOLDER_RE = re.compile(r"\{\w+\}") _SUMMARY_SLOT = "summary" @@ -186,6 +195,15 @@ def _heading(node: dict) -> str | None: return _written(node.get("heading")) +def _page_visualizations(page: Any) -> set[str]: + """Every visualization id placed in one page's layout.""" + if not isinstance(page, dict): + return set() + return { + visualization for node in _nodes(page.get("layout")) if (visualization := _visualization_id(node)) is not None + } + + def _visualizations_of(document: dict) -> set[str]: """Every visualization id placed anywhere in the layouts of the pages a reader sees.""" return { @@ -262,6 +280,13 @@ def _period_bounds(document: dict) -> tuple[Any, Any] | None: return period.get("from"), period.get("to") +def _filters(document: dict) -> Any: + """A v2 document's filters, keyed by filter id in ``content``; read from the top level when ``content`` is missing.""" + content = document.get("content") + container = content if isinstance(content, dict) else document + return container.get("filters") + + def _period_text(document: dict) -> str: bounds = _period_bounds(document) if bounds is None: @@ -305,6 +330,55 @@ def _asked_first_required(expected_output: dict) -> bool: return expected_output.get("asked_first_required") is True +def _rework(expected_output: dict) -> dict | None: + """The fixture's ``rework`` expectation: the run must edit the open document rather than draft a new one.""" + rework = expected_output.get("rework") + return rework if isinstance(rework, dict) else None + + +def _validate_rework(rework: Any) -> None: + """Reject a ``rework`` expectation the run could not score meaningfully. + + A rework is scored on aac-v2 documents only. ``keeps_pages`` checks that each page id is still + visible, not what the page shows. ``page_charts`` maps a kept page to the charts that must stay + on it; the page may hold others too. ``visualizations`` checks the charts anywhere in the document. + ``filters`` is compared with ``==`` against the drafted document's ``content.filters``, so it + must be copied from a real aac-v2 run: the wire form is not the shape a filter is authored in. + """ + if not isinstance(rework, dict): + raise ValueError(f"rework must be an object, got {type(rework).__name__}") + unknown = sorted(set(rework) - _REWORK_KEYS) + if unknown: + raise ValueError(f"unknown rework key(s) {unknown}; known: {sorted(_REWORK_KEYS)}") + base_document_id = rework.get("base_document_id") + if not isinstance(base_document_id, str) or not base_document_id.strip(): + raise ValueError(f"rework needs the base_document_id of the open document, got {base_document_id!r}") + if "keeps_pages" in rework: + keeps_pages = rework["keeps_pages"] + if not isinstance(keeps_pages, list) or not keeps_pages: + raise ValueError("rework.keeps_pages is empty; the pages check would pass vacuously") + if not all(isinstance(page_id, str) and page_id.strip() for page_id in keeps_pages): + raise ValueError(f"every rework.keeps_pages entry must be a page id, got {keeps_pages!r}") + if "page_charts" in rework: + page_charts = rework["page_charts"] + if not isinstance(page_charts, dict) or not page_charts: + raise ValueError("rework.page_charts is empty; the page charts check would pass vacuously") + kept = set(rework.get("keeps_pages") or []) + for page_id, charts in page_charts.items(): + if page_id not in kept: + raise ValueError(f"rework.page_charts page {page_id!r} must also be listed in rework.keeps_pages") + if not isinstance(charts, list) or not charts: + raise ValueError(f"rework.page_charts[{page_id!r}] must list the charts that stay on the page") + if not all(isinstance(chart, str) and chart.strip() for chart in charts): + raise ValueError(f"every rework.page_charts[{page_id!r}] entry must be a chart id, got {charts!r}") + if "title" in rework: + title = rework["title"] + if not isinstance(title, str) or not title.strip(): + raise ValueError(f"rework.title must be the title the edit leaves, got {title!r}") + if "filters" in rework and not isinstance(rework["filters"], dict): + raise ValueError(f"rework.filters must be the document's filters keyed by id, got {rework['filters']!r}") + + def _validate_expectation(expected_output: Any) -> None: """Reject a fixture the run could not score meaningfully, before the first API call. @@ -355,6 +429,8 @@ def _validate_expectation(expected_output: Any) -> None: raise ValueError(f"clarification_reply must be a non-empty reply, got {reply!r}") if expected_output.get("expects_clarification") is not True: raise ValueError("clarification_reply needs expects_clarification: true") + if "rework" in expected_output: + _validate_rework(expected_output["rework"]) def _validate_user_context(user_context: dict | None) -> None: @@ -370,6 +446,30 @@ def _validate_user_context(user_context: dict | None) -> None: raise ValueError("user_context.view.report is the Report name; send view.document") +def _validate_rework_target(expected_output: dict, user_context: dict | None) -> None: + """Reject a ``rework`` whose base id is not the open document the run sends, before the first API call. + + Otherwise every run would fail on ``base_document_id`` and read as a copilot regression. An + unsaved open document has no id, so a rework of one cannot be stated. + + Raises: + ValueError: the item states a rework, and ``user_context.view.document`` is missing, + has no id, or has another id. + """ + rework = _rework(expected_output) + if rework is None: + return + view = (user_context or {}).get("view") + document = view.get(_PART_TYPE) if isinstance(view, dict) else None + if not isinstance(document, dict): + raise ValueError("rework needs the open document in user_context.view.document") + open_id = document.get("id") + if not isinstance(open_id, str) or not open_id.strip(): + raise ValueError(f"rework needs the open document's id in user_context.view.document.id, got {open_id!r}") + if open_id != rework["base_document_id"]: + raise ValueError(f"rework.base_document_id {rework['base_document_id']!r} is not the open document {open_id!r}") + + def build_simulated_reply(expected_output: dict, *, asked: bool = True) -> str: """The reply the simulated user sends when the copilot asks back instead of drafting. @@ -388,7 +488,10 @@ def build_simulated_reply(expected_output: dict, *, asked: bool = True) -> str: period = expected_output.get("period") if isinstance(period, dict): segments.append(f"Period: {period.get('start')} to {period.get('end')}.") - segments.append("Anything else is up to you. Please create the document now.") + if _rework(expected_output) is not None: + segments.append("Anything else is up to you. Please make the change in the open document now.") + else: + segments.append("Anything else is up to you. Please create the document now.") return " ".join(segments) @@ -404,6 +507,11 @@ class _Applies: charts: bool narrative: bool = False asked_first: bool = False + rework: bool = False + pages_kept: bool = False + page_charts: bool = False + title: bool = False + filters: bool = False @dataclass @@ -423,6 +531,12 @@ class DocumentEvaluation: summaries_present: bool = False narrative_judged: bool = False asked_first: bool = False + new_draft: bool = False + reworks_open: bool = False + pages_kept: bool = False + page_charts_kept: bool = False + title_correct: bool = False + filters_kept: bool = False judge_reasoning: str = "" # Set when the judge returned something unreadable. The run then has no narrative verdict: # it is neither published as a 0 nor allowed to pass on the remaining checks. @@ -465,6 +579,18 @@ def strict_checks(self) -> dict[str, bool]: checks["document_narrative_judged"] = self.narrative_judged if self.applies.asked_first: checks["document_asked_first"] = self.asked_first + if self.applies.rework: + checks["document_reworks_open"] = self.reworks_open + else: + checks["document_new_draft"] = self.new_draft + if self.applies.pages_kept: + checks["document_pages_kept"] = self.pages_kept + if self.applies.page_charts: + checks["document_page_charts_kept"] = self.page_charts_kept + if self.applies.title: + checks["document_title_correct"] = self.title_correct + if self.applies.filters: + checks["document_filters_kept"] = self.filters_kept return checks @@ -509,6 +635,62 @@ def evaluate_document_response( return evaluation +def _score_rework( + rework: dict, document: dict, document_part: dict, pages: list[Any], failures: list[str] +) -> tuple[bool, bool, bool, bool, bool]: + """``(reworks_open, pages_kept, page_charts_kept, title_correct, filters_kept)`` of a v2 edit. + + A check the item does not state is False. + """ + base_document_id = document_part.get("base_document_id") + reworks_open_document = document_part.get("reworks_open_document") is True + expected_base = rework["base_document_id"] + if base_document_id != expected_base: + failures.append( + f"the edit must apply to the open document {expected_base!r}, " + f"but it reports base_document_id {base_document_id!r}" + ) + if not reworks_open_document: + failures.append("the edit must be written from the open document, but reworks_open_document is not true") + reworks_open = base_document_id == expected_base and reworks_open_document + + pages_kept = False + if "keeps_pages" in rework: + present = {page.get("id") for page in pages if isinstance(page, dict)} + lost = [page_id for page_id in rework["keeps_pages"] if page_id not in present] + failures.extend(f"the edit lost page {page_id!r} of the open document" for page_id in lost) + pages_kept = not lost + + page_charts_kept = False + if "page_charts" in rework: + by_id = {page.get("id"): page for page in pages if isinstance(page, dict)} + moved = [ + (chart, page_id) + for page_id, charts in rework["page_charts"].items() + for chart in charts + if chart not in _page_visualizations(by_id.get(page_id)) + ] + failures.extend( + f"chart {chart!r} is no longer on page {page_id!r} of the open document" for chart, page_id in moved + ) + page_charts_kept = not moved + + title_correct = False + if "title" in rework: + title_correct = document.get("title") == rework["title"] + if not title_correct: + failures.append(f"the document is titled {document.get('title')!r}, expected {rework['title']!r}") + + filters_kept = False + if "filters" in rework: + # A document without filters omits the key, which matches an expectation of none. + actual_filters = _filters(document) or {} + filters_kept = actual_filters == rework["filters"] + if not filters_kept: + failures.append(f"the document filters are {actual_filters!r}, expected {rework['filters']!r}") + return reworks_open, pages_kept, page_charts_kept, title_correct, filters_kept + + def _score_response( tool_result: dict | None, document_part: dict | None, @@ -516,10 +698,16 @@ def _score_response( *, skill_activated: bool, ) -> DocumentEvaluation: + rework = _rework(expected_output) applies = _Applies( period=_has_period(expected_output), charts=_has_visualizations(expected_output), narrative=_has_narrative(expected_output), + rework=rework is not None, + pages_kept=rework is not None and "keeps_pages" in rework, + page_charts=rework is not None and "page_charts" in rework, + title=rework is not None and "title" in rework, + filters=rework is not None and "filters" in rework, ) if tool_result is None: @@ -572,11 +760,32 @@ def _score_response( pages_consistent = bool(kinds) and kinds[0] == _COVER_PAGE and _CONTENT_PAGE in kinds not_saved = True - for key, wording in (("saved_document_id", "be saved yet"), ("base_document_id", "edit a saved document")): - value = document_part.get(key) - if value is not None: - failures.append(f"a new draft must not {wording}, but it reports {key} {value!r}") - not_saved = False + saved_document_id = document_part.get("saved_document_id") + if saved_document_id is not None: + draft = "a new draft" if rework is None else "an edit" + failures.append(f"{draft} must not be saved yet, but it reports saved_document_id {saved_document_id!r}") + not_saved = False + + new_draft = reworks_open = pages_kept = page_charts_kept = title_correct = filters_kept = False + if rework is None: + new_draft = True + base_document_id = document_part.get("base_document_id") + if base_document_id is not None: + failures.append( + f"a new draft must not edit a saved document, but it reports base_document_id {base_document_id!r}" + ) + new_draft = False + if document_part.get("reworks_open_document") is True: + failures.append("a new draft must not rework the open document, but it reports reworks_open_document") + new_draft = False + elif not _is_v2(document): + failures.append( + f"a rework is scored on aac-v2 documents only, but the draft is version {document.get('version')!r}" + ) + else: + reworks_open, pages_kept, page_charts_kept, title_correct, filters_kept = _score_rework( + rework, document, document_part, pages, failures + ) period_correct = False if applies.period: @@ -620,6 +829,12 @@ def _score_response( period_correct=period_correct, charts_matched=charts_matched, summaries_present=summaries_present, + new_draft=new_draft, + reworks_open=reworks_open, + pages_kept=pages_kept, + page_charts_kept=page_charts_kept, + title_correct=title_correct, + filters_kept=filters_kept, failures=failures, ) @@ -848,12 +1063,13 @@ def run_agentic_document_skill( the format to the server's ``enablePublisherV2`` feature flag. Raises: - ValueError: the fixture is unusable — see ``_validate_expectation`` and - ``_validate_user_context``, or ``document_format`` is not a format gen-ai - accepts. Raised before any chat request. + ValueError: the fixture is unusable — see ``_validate_expectation``, + ``_validate_user_context`` and ``_validate_rework_target``, or ``document_format`` + is not a format gen-ai accepts. Raised before any chat request. """ _validate_expectation(expected_output) _validate_user_context(user_context) + _validate_rework_target(expected_output, user_context) document_format = normalize_document_format(document_format) if judge is None and _has_narrative(expected_output): judge = LLMJudge(_NARRATIVE_EVALUATION_STEPS) @@ -938,9 +1154,9 @@ def evaluate_agentic_document_skill( Raises: DocumentSkillAssertionError: the gate did not pass. - ValueError: the fixture is unusable, see ``_validate_expectation`` and - ``_validate_user_context``. Raised before any chat request, so it means a - fixture to fix rather than a result to read. Also raised, equally before any + ValueError: the fixture is unusable, see ``_validate_expectation``, + ``_validate_user_context`` and ``_validate_rework_target``. Raised before any chat + request, so it means a fixture to fix rather than a result to read. Also raised, equally before any request, when the caller passes a ``document_format`` gen-ai does not accept. JudgeResponseError: the fixture states a narrative and the judge returned no readable verdict for any run -- an item without a result, not K failures. diff --git a/packages/gooddata-eval/tests/test_agentic_document_skill.py b/packages/gooddata-eval/tests/test_agentic_document_skill.py index 55e131550..d0d58320c 100644 --- a/packages/gooddata-eval/tests/test_agentic_document_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_document_skill.py @@ -70,6 +70,7 @@ def _document_part( period: dict | None = None, saved_document_id: str | None = None, base_document_id: str | None = None, + reworks_open_document: bool = False, document: dict | None | str = "default", ) -> dict: pages = pages if pages is not None else [_cover_page(), _content_page("page2", _REVENUE_TREND)] @@ -92,6 +93,7 @@ def _document_part( "page_count": len(pages) if page_count is None else page_count, "base_document_id": base_document_id, "saved_document_id": saved_document_id, + "reworks_open_document": reworks_open_document, } @@ -311,6 +313,7 @@ def test_a_drafted_document_returned_in_the_chat_item_passes(version: str) -> No "document_not_saved": True, "document_skill_activated": True, "document_wire_names": True, + "document_new_draft": True, } assert evaluation.strict_pass assert evaluation.failures == [] @@ -373,6 +376,7 @@ def test_a_cover_alone_is_not_a_document() -> None: def test_a_new_draft_must_not_already_be_saved() -> None: evaluation = _evaluate(_document_part(saved_document_id="sales_overview")) assert evaluation.strict_checks["document_not_saved"] is False + assert evaluation.strict_checks["document_new_draft"] is True assert evaluation.failures == [ "a new draft must not be saved yet, but it reports saved_document_id 'sales_overview'" ] @@ -380,12 +384,253 @@ def test_a_new_draft_must_not_already_be_saved() -> None: def test_a_new_draft_must_not_edit_a_saved_document() -> None: evaluation = _evaluate(_document_part(base_document_id="q3_review")) - assert evaluation.strict_checks["document_not_saved"] is False + assert evaluation.strict_checks["document_new_draft"] is False + assert evaluation.strict_checks["document_not_saved"] is True assert evaluation.failures == [ "a new draft must not edit a saved document, but it reports base_document_id 'q3_review'" ] +def test_a_new_draft_must_not_rework_the_open_document() -> None: + evaluation = _evaluate(_document_part(reworks_open_document=True)) + assert evaluation.strict_checks["document_new_draft"] is False + assert evaluation.strict_checks["document_not_saved"] is True + assert evaluation.failures == [ + "a new draft must not rework the open document, but it reports reworks_open_document" + ] + + +# ── rework of the open document ───────────────────────────────────────────── +# A rework is scored on aac-v2 documents only, so these build v2 parts. + +_OPEN_DOCUMENT = "q3_review" +# Keyed by filter id, as gen-ai drafts a v2 document's content.filters. +_OPEN_FILTERS = { + "document_filter_customer_country": { + "type": "attribute_filter", + "title": "Customer country", + "using": "label/customer_country", + "multiselect": False, + } +} + + +def _rework_part(pages: list[dict] | None = None, **kwargs: Any) -> dict: + kwargs.setdefault("base_document_id", _OPEN_DOCUMENT) + kwargs.setdefault("reworks_open_document", True) + return _v2_document_part(pages, **kwargs) + + +def _with_filters(part: dict, filters: dict) -> dict: + part["document"]["content"]["filters"] = filters + return part + + +def test_an_edit_of_the_open_document_passes_every_rework_check() -> None: + pages = [ + _v2_cover_page(), + _v2_content_page("page2", _REVENUE_TREND), + _v2_content_page("page3", _RETURNS_BY_CATEGORY), + ] + part = _with_filters(_rework_part(pages), _OPEN_FILTERS) + expected = { + "period": _PERIOD, + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page1", "page2"], + "page_charts": {"page2": [_REVENUE_TREND]}, + "title": "Sales overview", + "filters": _OPEN_FILTERS, + }, + } + evaluation = _evaluate(part, expected=expected, tool=_draft_result(page_count=3)) + assert evaluation.failures == [] + assert evaluation.strict_pass + assert evaluation.strict_checks == { + "document_drafted": True, + "document_part_present": True, + "document_ref_matches": True, + "document_pages_consistent": True, + "document_not_saved": True, + "document_skill_activated": True, + "document_wire_names": True, + "document_period_correct": True, + "document_reworks_open": True, + "document_pages_kept": True, + "document_page_charts_kept": True, + "document_title_correct": True, + "document_filters_kept": True, + } + + +def test_a_rework_of_a_v1_document_fails_once_and_says_why() -> None: + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "keeps_pages": ["page1"], "title": "Sales overview"}} + evaluation = _evaluate(_document_part(base_document_id=_OPEN_DOCUMENT), expected=expected) + assert evaluation.failures == ["a rework is scored on aac-v2 documents only, but the draft is version None"] + assert evaluation.strict_checks["document_reworks_open"] is False + assert evaluation.strict_checks["document_pages_kept"] is False + assert evaluation.strict_checks["document_title_correct"] is False + + +def test_a_new_draft_where_an_edit_was_expected_fails() -> None: + evaluation = _evaluate(_v2_document_part(), expected={"rework": {"base_document_id": _OPEN_DOCUMENT}}) + assert evaluation.strict_checks["document_reworks_open"] is False + assert evaluation.failures == [ + "the edit must apply to the open document 'q3_review', but it reports base_document_id None", + "the edit must be written from the open document, but reworks_open_document is not true", + ] + + +def test_an_edit_of_another_document_fails() -> None: + part = _rework_part(base_document_id="another_document") + evaluation = _evaluate(part, expected={"rework": {"base_document_id": _OPEN_DOCUMENT}}) + assert evaluation.strict_checks["document_reworks_open"] is False + assert evaluation.failures == [ + "the edit must apply to the open document 'q3_review', but it reports base_document_id 'another_document'" + ] + + +def test_an_edit_not_written_from_the_open_document_fails() -> None: + part = _rework_part(reworks_open_document=False) + evaluation = _evaluate(part, expected={"rework": {"base_document_id": _OPEN_DOCUMENT}}) + assert evaluation.strict_checks["document_reworks_open"] is False + assert evaluation.failures == [ + "the edit must be written from the open document, but reworks_open_document is not true" + ] + + +def test_an_edit_must_not_be_saved_yet() -> None: + part = _rework_part(saved_document_id=_OPEN_DOCUMENT) + evaluation = _evaluate(part, expected={"rework": {"base_document_id": _OPEN_DOCUMENT}}) + assert evaluation.strict_checks["document_not_saved"] is False + assert evaluation.failures == ["an edit must not be saved yet, but it reports saved_document_id 'q3_review'"] + + +def test_an_edit_that_drops_a_page_of_the_open_document_fails_and_names_it() -> None: + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "keeps_pages": ["page1", "page2", "page3"]}} + evaluation = _evaluate(_rework_part(), expected=expected) + assert evaluation.strict_checks["document_pages_kept"] is False + assert evaluation.failures == ["the edit lost page 'page3' of the open document"] + + +def test_a_hidden_page_counts_as_lost() -> None: + hidden = {**_v2_content_page("page3", _RETURNS_BY_CATEGORY), "visible": False} + pages = [_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND), hidden] + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "keeps_pages": ["page3"]}} + evaluation = _evaluate(_rework_part(pages), expected=expected) + assert evaluation.failures == ["the edit lost page 'page3' of the open document"] + + +def test_pages_in_sections_are_found() -> None: + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "keeps_pages": ["page1", "page2"]}} + evaluation = _evaluate(_rework_part(form="sections"), expected=expected) + assert evaluation.strict_checks["document_pages_kept"] is True + + +def test_an_edit_that_moved_a_chart_off_its_page_fails_and_names_both() -> None: + pages = [ + _v2_cover_page(), + _v2_content_page("page2", _RETURNS_BY_CATEGORY), + _v2_content_page("page3", _REVENUE_TREND), + ] + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page2"], + "page_charts": {"page2": [_REVENUE_TREND]}, + } + } + evaluation = _evaluate(_rework_part(pages), expected=expected, tool=_draft_result(page_count=3)) + assert evaluation.strict_checks["document_pages_kept"] is True + assert evaluation.strict_checks["document_page_charts_kept"] is False + assert evaluation.failures == ["chart 'revenue_trend' is no longer on page 'page2' of the open document"] + + +def test_a_kept_page_may_hold_more_charts_than_the_fixture_lists() -> None: + pages = [_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND, _RETURNS_BY_CATEGORY)] + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page2"], + "page_charts": {"page2": [_REVENUE_TREND]}, + } + } + evaluation = _evaluate(_rework_part(pages), expected=expected) + assert evaluation.strict_checks["document_page_charts_kept"] is True + assert evaluation.failures == [] + + +def test_a_lost_page_also_loses_the_charts_listed_for_it() -> None: + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page3"], + "page_charts": {"page3": [_REVENUE_TREND]}, + } + } + evaluation = _evaluate(_rework_part(), expected=expected) + assert evaluation.strict_checks["document_page_charts_kept"] is False + assert evaluation.failures == [ + "the edit lost page 'page3' of the open document", + "chart 'revenue_trend' is no longer on page 'page3' of the open document", + ] + + +def test_a_chart_on_a_hidden_page_is_not_kept() -> None: + hidden = {**_v2_content_page("page2", _REVENUE_TREND), "visible": False} + pages = [_v2_cover_page(), hidden, _v2_content_page("page3", _RETURNS_BY_CATEGORY)] + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page2"], + "page_charts": {"page2": [_REVENUE_TREND]}, + } + } + evaluation = _evaluate(_rework_part(pages), expected=expected) + assert evaluation.strict_checks["document_page_charts_kept"] is False + + +def test_page_charts_are_found_in_sections() -> None: + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page2"], + "page_charts": {"page2": [_REVENUE_TREND]}, + } + } + evaluation = _evaluate(_rework_part(form="sections"), expected=expected) + assert evaluation.strict_checks["document_page_charts_kept"] is True + + +def test_a_rename_that_left_the_old_title_fails() -> None: + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "title": "E2 Sales Review"}} + evaluation = _evaluate(_rework_part(), expected=expected) + assert evaluation.strict_checks["document_title_correct"] is False + assert evaluation.failures == ["the document is titled 'Sales overview', expected 'E2 Sales Review'"] + + +def test_an_edit_that_changed_the_document_filters_fails() -> None: + part = _with_filters(_rework_part(), {}) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _OPEN_FILTERS}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [f"the document filters are {{}}, expected {_OPEN_FILTERS!r}"] + + +def test_a_document_without_filters_keeps_an_expectation_of_none() -> None: + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": {}}} + evaluation = _evaluate(_rework_part(), expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is True + + +def test_rework_checks_are_published_only_for_what_the_fixture_states() -> None: + evaluation = _evaluate(_rework_part(), expected={"rework": {"base_document_id": _OPEN_DOCUMENT}}) + assert "document_reworks_open" in evaluation.strict_checks + for name in ("document_pages_kept", "document_page_charts_kept", "document_title_correct", "document_filters_kept"): + assert name not in evaluation.strict_checks + assert evaluation.strict_pass + + @pytest.mark.parametrize("version", _VERSIONS) def test_a_wrong_period_fails(version: str) -> None: evaluation = _evaluate(_versioned_part(version, period=("2025-07-01", "2025-12-31")), expected={"period": _PERIOD}) @@ -536,6 +781,35 @@ def test_a_user_context_with_view_report_fails_before_any_request(monkeypatch: p assert client.sent == [] +@pytest.mark.parametrize( + ("user_context", "message"), + [ + (None, "rework needs the open document in user_context.view.document"), + ({"view": {"dashboard": {"id": "d1"}}}, "rework needs the open document in user_context.view.document"), + ({"view": {"document": {"title": "Q3 review"}}}, "rework needs the open document's id"), + ({"view": {"document": {"id": "another_document"}}}, "rework.base_document_id 'q3_review' is not the open"), + ], +) +def test_a_rework_that_does_not_match_the_open_document_fails_before_any_request( + monkeypatch: pytest.MonkeyPatch, user_context: dict | None, message: str +) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT}} + with pytest.raises(ValueError, match=message): + run_agentic_document_skill("https://h", "tok", "ws", "Rename it", expected, user_context=user_context) + assert client.sent == [] + + +def test_a_rework_of_the_open_document_reaches_the_chat(monkeypatch: pytest.MonkeyPatch) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + user_context = {"view": {"document": {"id": _OPEN_DOCUMENT, "title": "Q3 review"}}} + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT}} + run_agentic_document_skill("https://h", "tok", "ws", "Rename it", expected, user_context=user_context) + assert client.sent == ["Rename it"] + + def test_the_document_format_reaches_the_chat_client(monkeypatch: pytest.MonkeyPatch) -> None: client = _ScriptedChatClient([_drafting_turn()]) built: list[dict[str, Any]] = [] @@ -619,6 +893,31 @@ def test_the_document_format_is_recorded_in_the_run_metadata( ({"asked_first_required": True}, "asked_first_required needs expects_clarification: true"), ({"clarification_reply": " "}, "clarification_reply must be a non-empty reply"), ({"clarification_reply": "Use 2026."}, "clarification_reply needs expects_clarification: true"), + ({"rework": True}, "rework must be an object"), + ({"rework": {}}, "rework needs the base_document_id"), + ({"rework": {"base_document_id": " "}}, "rework needs the base_document_id"), + ({"rework": {"base_document_id": "q3", "keeps_pages": [" "]}}, "every rework.keeps_pages entry"), + ({"rework": {"base_document_id": "q3", "keep_pages": ["page1"]}}, r"unknown rework key\(s\) \['keep_pages'\]"), + ({"rework": {"base_document_id": "q3", "keeps_pages": []}}, "rework.keeps_pages is empty"), + ({"rework": {"base_document_id": "q3", "keeps_pages": [""]}}, "every rework.keeps_pages entry"), + ( + {"rework": {"base_document_id": "q3", "keeps_pages": ["p2"], "page_charts": {}}}, + "rework.page_charts is empty", + ), + ( + {"rework": {"base_document_id": "q3", "page_charts": {"p2": ["c1"]}}}, + "must also be listed in rework.keeps_pages", + ), + ( + {"rework": {"base_document_id": "q3", "keeps_pages": ["p2"], "page_charts": {"p2": []}}}, + "must list the charts", + ), + ( + {"rework": {"base_document_id": "q3", "keeps_pages": ["p2"], "page_charts": {"p2": [" "]}}}, + "entry must be a chart id", + ), + ({"rework": {"base_document_id": "q3", "title": " "}}, "rework.title must be"), + ({"rework": {"base_document_id": "q3", "filters": ["customer_country"]}}, "rework.filters must be"), ], ) def test_an_unusable_fixture_is_rejected_before_any_request(expected: dict, message: str) -> None: @@ -652,6 +951,12 @@ def test_the_simulated_reply_leaves_out_what_the_fixture_does_not_state() -> Non assert build_simulated_reply({}) == "Anything else is up to you. Please create the document now." +def test_the_simulated_reply_to_an_edit_asks_for_the_change_in_the_open_document() -> None: + assert build_simulated_reply({"rework": {"base_document_id": "q3"}}) == ( + "Anything else is up to you. Please make the change in the open document now." + ) + + # ── conversation loop ─────────────────────────────────────────────────────── @@ -1008,6 +1313,7 @@ def test_every_scored_check_reaches_langfuse_with_the_draft_facts(monkeypatch: p "document_not_saved", "document_skill_activated", "document_wire_names", + "document_new_draft", "document_period_correct", "document_asked_first", "pass_at_k", From bea3f1f7281406a07032bca8d5d7bed1f0af6bed Mon Sep 17 00:00:00 2001 From: Oanh Nguyen Date: Fri, 6 Nov 2026 13:37:38 +0700 Subject: [PATCH 2/2] feat(gooddata-eval): send the document format on a general question A general question about the document open in user_context ran in the organization's default document format. On an organization that defaults to aac-v1, the agent got report_builder instead of document_builder and could read only the context summary of an aac-v2 document, so it could not say which charts the document shows. evaluate_agentic_general_question and run_agentic_general_question now take document_format, as the document skill does. It is sent on every turn, recorded in the dataset-run metadata, and an unknown value is rejected before any request. None keeps the server default. jira: QA-30024 risk: low Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011CpUh9hFjL8SbdpFxBGRXp --- .../core/agentic/general_question.py | 27 +++++++- .../tests/test_agentic_general_question.py | 66 +++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py index 5de9512d5..0a55b2a00 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/general_question.py @@ -27,7 +27,7 @@ ) from gooddata_eval.core.chat.render import render_answer_text from gooddata_eval.core.chat.sse_client import ChatClient -from gooddata_eval.core.config import ReasoningEffort +from gooddata_eval.core.config import DocumentFormat, ReasoningEffort, normalize_document_format from gooddata_eval.core.evaluators._llm_judge import JudgeResponseError, LLMJudge, score_run from gooddata_eval.core.langfuse.item_scope import item_scope from gooddata_eval.core.models import ( @@ -172,8 +172,18 @@ def run_agentic_general_question( reasoning_effort: ReasoningEffort | None = None, agent_id: str | None = None, user_context: dict | None = None, + document_format: DocumentFormat | None = None, ) -> AgenticGeneralQuestionSummary: - """Run the general-question agentic evaluation K times and return a summary.""" + """Run the general-question agentic evaluation K times and return a summary. + + ``document_format`` is sent on every turn of every run, see ``ChatClient``: a question about + the document open in ``user_context`` needs the format that document is in. None leaves it to + the server's ``enablePublisherV2`` feature flag. + + Raises: + ValueError: ``document_format`` is not a format gen-ai accepts. Raised before any chat request. + """ + document_format = normalize_document_format(document_format) run_results: list[GeneralQuestionResult] = [] client = ChatClient( host=host, @@ -182,6 +192,7 @@ def run_agentic_general_question( reasoning_effort=reasoning_effort, agent_id=agent_id, user_context=user_context, + document_format=document_format, ) judge = LLMJudge(_GENERAL_QUESTION_EVALUATION_STEPS) @@ -280,6 +291,7 @@ def evaluate_agentic_general_question( reasoning_effort: ReasoningEffort | None = None, submit_trace_link: SubmitTraceLink = run_trace_link_inline, user_context: dict | None = None, + document_format: DocumentFormat | None = None, gate: EvalGate = DEFAULT_GATE, ) -> AgenticEvalOutcome: """Run general-question evaluation, log to Langfuse, and raise GeneralQuestionAssertionError on failure. @@ -287,7 +299,17 @@ def evaluate_agentic_general_question( Returns the best run's outcome (reasoning_steps, conversation_id, response_id) as an AgenticEvalOutcome on success; on failure the same three values are attached to the raised exception as ``.reasoning_steps``/``.conversation_id``/``.response_id``. + + ``document_format`` (``aac-v1``/``aac-v2``) asks gen-ai for that document content format + on every turn and is recorded as ``document_format`` in the dataset-run metadata; None + leaves it to the server's ``enablePublisherV2`` feature flag. + + Raises: + ValueError: ``document_format`` is not a format gen-ai accepts. Raised before any request. """ + document_format = normalize_document_format(document_format) + if document_format is not None: + run_metadata_extra = {**(run_metadata_extra or {}), "document_format": document_format} identity = RunIdentity( host, token, @@ -311,6 +333,7 @@ def evaluate_agentic_general_question( reasoning_effort=reasoning_effort, agent_id=agent_id, user_context=user_context, + document_format=document_format, ) if langfuse is not None and dataset_item_id: diff --git a/packages/gooddata-eval/tests/test_agentic_general_question.py b/packages/gooddata-eval/tests/test_agentic_general_question.py index cdfc683b8..e670fb352 100644 --- a/packages/gooddata-eval/tests/test_agentic_general_question.py +++ b/packages/gooddata-eval/tests/test_agentic_general_question.py @@ -726,3 +726,69 @@ def test_an_item_with_no_gradeable_run_raises_instead_of_reporting_failures(): assert [r["error"] for r in err.value.failed_runs] == ["empty body twice", "no 'score' key"] assert (err.value.runs_passed, err.value.runs_effective) == (0, 2) assert len(err.value.run_latencies_s) == err.value.runs_effective + + +# --- document format ------------------------------------------------------------------- + + +def test_the_document_format_reaches_the_chat_client(): + client, judge = _pass_client_and_judge() + with ( + patch("gooddata_eval.core.agentic.general_question.ChatClient", return_value=client) as chat_client, + patch("gooddata_eval.core.agentic.general_question.LLMJudge", return_value=judge), + ): + run_agentic_general_question( + "https://h", "tok", "ws", "What period does it cover?", "2025", document_format="aac-v2" + ) + assert chat_client.call_args.kwargs["document_format"] == "aac-v2" + + +def test_no_document_format_leaves_it_to_the_server(): + client, judge = _pass_client_and_judge() + with ( + patch("gooddata_eval.core.agentic.general_question.ChatClient", return_value=client) as chat_client, + patch("gooddata_eval.core.agentic.general_question.LLMJudge", return_value=judge), + ): + run_agentic_general_question("https://h", "tok", "ws", "What is ETL?", "Extract, Transform, Load") + assert chat_client.call_args.kwargs["document_format"] is None + + +def test_an_unknown_document_format_fails_before_any_request(): + client, judge = _pass_client_and_judge() + with _patched(client, judge), pytest.raises(ValueError, match="aac-v3"): + evaluate_agentic_general_question("https://h", "tok", "ws", "What period?", "2025", document_format="aac-v3") + client.create_conversation.assert_not_called() + client.send_message.assert_not_called() + + +@pytest.mark.parametrize( + ("document_format", "extra", "expected"), + [ + ("aac-v2", None, {"document_format": "aac-v2"}), + ("aac-v2", {"leg": "luna"}, {"leg": "luna", "document_format": "aac-v2"}), + (None, {"leg": "luna"}, {"leg": "luna"}), + (None, None, None), + ], +) +def test_the_document_format_is_recorded_in_the_run_metadata(document_format, extra, expected): + client, judge = _pass_client_and_judge() + identities = [] + with ( + _patched(client, judge), + patch( + "gooddata_eval.core.agentic.general_question.submit_trace_scoring", + side_effect=lambda _link, identity, **_kw: identities.append(identity), + ), + ): + evaluate_agentic_general_question( + "https://h", + "tok", + "ws", + "What period?", + "2025", + langfuse=object(), + dataset_item_id="item-1", + run_metadata_extra=extra, + document_format=document_format, + ) + assert [identity.run_metadata_extra for identity in identities] == [expected]