diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_filters.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_filters.py new file mode 100644 index 000000000..1ecbb5af6 --- /dev/null +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_filters.py @@ -0,0 +1,46 @@ +# (C) 2026 GoodData Corporation +"""Attribute filter selections and filter values, read the same way by the dashboard and document graders.""" + +from __future__ import annotations + + +def selection_of(attribute_filter: dict) -> tuple[str, list] | None: + """The elements an attribute filter restricts to, or ``None`` when it restricts nothing. + + An unrestricted filter is spelled as the absence of a selection, and an empty list means + the same thing, so both read as "all". + """ + state = attribute_filter.get("state") or {} + for kind in ("include", "exclude"): + values = state.get(kind) + if values: + return kind, list(values) + return None + + +def describe_selection(selection: tuple[str, list] | None) -> str: + return "all" if selection is None else f"{selection[0]} {selection[1]!r}" + + +def same_selection(actual: tuple[str, list] | None, expected: tuple[str, list] | None) -> bool: + """Whether two selections restrict to the same elements: an include or exclude list is a set, not a sequence.""" + if actual is None or expected is None: + return actual is None and expected is None + return actual[0] == expected[0] and same_unordered_values(expected[1], actual[1]) + + +def same_unordered_values(want: list, got: list) -> bool: + """One-to-one match in any order, with each pair of the same type. + + Not sorted: AAC allows `null` among the values, and Python cannot order it with strings. + Not stringified: `1` and `"1"` are different values. + """ + remaining = list(got) + for expected in want: + index = next( + (i for i, actual in enumerate(remaining) if type(actual) is type(expected) and actual == expected), None + ) + if index is None: + return False + del remaining[index] + return not remaining diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py index 1c7aaf771..eb6428005 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py @@ -11,6 +11,7 @@ import jsonpatch from gooddata_eval.core.agentic._failed_runs import build_failed_runs +from gooddata_eval.core.agentic._filters import describe_selection, same_unordered_values, selection_of from gooddata_eval.core.agentic._gate import ( DEFAULT_GATE, EvalGate, @@ -463,24 +464,6 @@ def _check_date_range(dashboard: dict, expected: dict | None, is_edit: bool) -> return [reason for m in mismatches for reason in m] -def _selection_of(attribute_filter: dict) -> tuple[str, list] | None: - """The elements an attribute filter restricts to, or ``None`` when it restricts nothing. - - An unrestricted filter is spelled as the absence of a selection, and an empty list means - the same thing, so both read as "all". - """ - state = attribute_filter.get("state") or {} - for kind in ("include", "exclude"): - values = state.get(kind) - if values: - return kind, list(values) - return None - - -def _describe_selection(selection: tuple[str, list] | None) -> str: - return "all" if selection is None else f"{selection[0]} {selection[1]!r}" - - def _check_filters(dashboard: dict, expected: list[dict]) -> list[str]: """Every expected filter must be on the dashboard, with the stated selection or condition. @@ -532,9 +515,9 @@ def _check_filters(dashboard: dict, expected: list[dict]) -> list[str]: continue matches = shown wanted = _expected_selection(exp) - if not any(_selection_of(f) == wanted for f in matches): - found = ", ".join(_describe_selection(_selection_of(f)) for f in matches) - failures.append(f"filter on {using!r} selects {found}, expected {_describe_selection(wanted)}") + if not any(selection_of(f) == wanted for f in matches): + found = ", ".join(describe_selection(selection_of(f)) for f in matches) + failures.append(f"filter on {using!r} selects {found}, expected {describe_selection(wanted)}") return failures @@ -565,7 +548,7 @@ def _filter_value_mismatch(actual: dict, expected: dict) -> list[str]: continue want, got = expected[key], actual.get(key) same = ( - _same_unordered_values(want, got) + same_unordered_values(want, got) if key == "values" and isinstance(want, list) and isinstance(got, list) else got == want ) @@ -574,25 +557,8 @@ def _filter_value_mismatch(actual: dict, expected: dict) -> list[str]: return mismatches -def _same_unordered_values(want: list, got: list) -> bool: - """One-to-one match in any order, with each pair of the same type. - - Not sorted: AAC allows `null` among the values, and Python cannot order it with strings. - Not stringified: `1` and `"1"` are different values. - """ - remaining = list(got) - for expected in want: - index = next( - (i for i, actual in enumerate(remaining) if type(actual) is type(expected) and actual == expected), None - ) - if index is None: - return False - del remaining[index] - return not remaining - - def _expected_selection(expected_filter: dict) -> tuple[str, list] | None: - """Read a fixture's filter entry into the same shape ``_selection_of`` produces. + """Read a fixture's filter entry into the same shape ``selection_of`` produces. Raises: ValueError: the entry states no selection the evaluator knows how to check. @@ -768,10 +734,10 @@ def _filter_mismatch(label: str, actual: dict, expected: dict) -> list[str]: if expected.get("type") == "date_filter": keys: tuple[str, ...] = _DATE_FILTER_KEYS elif expected.get("type") == "attribute_filter": - if _selection_of(actual) != _selection_of(expected): + if selection_of(actual) != selection_of(expected): return [ - f"{label} selects {_describe_selection(_selection_of(actual))}, " - f"expected {_describe_selection(_selection_of(expected))}" + f"{label} selects {describe_selection(selection_of(actual))}, " + f"expected {describe_selection(selection_of(expected))}" ] keys = ("using", "display_as") else: 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 b0cf53062..1c6ff2f90 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 @@ -11,6 +11,7 @@ from gooddata_eval.core.agentic._conversation_context import classify_reply from gooddata_eval.core.agentic._failed_runs import build_failed_runs +from gooddata_eval.core.agentic._filters import describe_selection, same_selection, selection_of from gooddata_eval.core.agentic._gate import ( DEFAULT_GATE, EvalGate, @@ -76,7 +77,11 @@ "rework", } ) -_REWORK_KEYS = frozenset({"base_document_id", "keeps_pages", "page_charts", "title", "filters"}) +_REWORK_KEYS = frozenset({"base_document_id", "keeps_pages", "page_charts", "new_page_charts", "title", "filters"}) +# A date filter's range keys, compared even when a rework item leaves them out: an all-time filter +# spells itself by leaving `from` and `to` out, so a drafted range must not pass it, and an offset +# means nothing without its granularity, so a range stated without one must not pass any granularity. +_DATE_FILTER_RANGE_KEYS = ("granularity", "from", "to") # Template tokens the document renders at export time ({documentName}, {periodStart}, ...). _PLACEHOLDER_RE = re.compile(r"\{\w+\}") _SUMMARY_SLOT = "summary" @@ -295,6 +300,13 @@ def _period_text(document: dict) -> str: return f"{bounds[0]} to {bounds[1]}" +def _expected_period_text(period: dict) -> str: + """A fixture's ``period``: ``start`` and ``end`` dates, or a relative range in the v2 grammar.""" + if period.get("type") == "relative": + return f"the relative range {period.get('granularity')} {period.get('from')} to {period.get('to')}" + return f"{period.get('start')} to {period.get('end')}" + + def render_document_text(document: dict) -> str: """The document as the judge reads it: title, period, and per content page its heading, charts and summaries.""" lines = [f"Document: {document.get('title')}", f"Period: {_period_text(document)}"] @@ -341,9 +353,15 @@ def _validate_rework(rework: Any) -> None: 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. + on it; the page may hold others too. ``new_page_charts`` lists the charts that must sit on a + visible page outside ``keeps_pages``, which is what "add a page" asks for; ``visualizations`` + alone accepts a chart pushed onto an old page. ``filters`` maps each filter id the document must + have, and no other, to the keys it is compared on, in the drafted document's ``content.filters`` + wire form. Each entry states at least the filter's ``type``. Other keys an entry leaves out are + not compared, so a converter that adds one does not fail the run, except a date filter's + ``granularity``, ``from`` and ``to``: leaving out ``from`` and ``to`` states all time, and leaving + out ``granularity`` states none. An attribute filter's ``state`` is compared on its selection: no + values means all. """ if not isinstance(rework, dict): raise ValueError(f"rework must be an object, got {type(rework).__name__}") @@ -371,12 +389,30 @@ def _validate_rework(rework: Any) -> None: 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 "new_page_charts" in rework: + new_page_charts = rework["new_page_charts"] + if not isinstance(new_page_charts, list) or not new_page_charts: + raise ValueError("rework.new_page_charts is empty; the new page check would pass vacuously") + if not all(isinstance(chart, str) and chart.strip() for chart in new_page_charts): + raise ValueError(f"every rework.new_page_charts entry must be a chart id, got {new_page_charts!r}") + if "keeps_pages" not in rework: + raise ValueError("rework.new_page_charts needs rework.keeps_pages to tell the open document's pages apart") 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}") + if "filters" in rework: + filters = rework["filters"] + if not isinstance(filters, dict): + raise ValueError(f"rework.filters must be the document's filters keyed by id, got {filters!r}") + for filter_id, entry in filters.items(): + if not isinstance(entry, dict) or not isinstance(entry.get("type"), str) or not entry["type"].strip(): + raise ValueError( + f"rework.filters[{filter_id!r}] must state at least the filter's 'type', got {entry!r}" + ) + problem = _state_problem(entry.get("state"), entry["type"]) + if problem is not None: + raise ValueError(f"rework.filters[{filter_id!r}].state is {entry['state']!r}, {problem}") def _validate_expectation(expected_output: Any) -> None: @@ -397,8 +433,16 @@ def _validate_expectation(expected_output: Any) -> None: raise ValueError(f"unknown expected_output key(s) {unknown}; known: {sorted(_EXPECTATION_KEYS)}") if _has_period(expected_output): period = expected_output.get("period") - if not isinstance(period, dict) or not period.get("start") or not period.get("end"): - raise ValueError(f"period needs both 'start' and 'end', got {period!r}") + if isinstance(period, dict) and period.get("type") == "relative": + if not isinstance(period.get("granularity"), str) or not period["granularity"].strip(): + raise ValueError(f"a relative period needs its 'granularity', got {period!r}") + if not all( + isinstance(period.get(bound), int) and not isinstance(period.get(bound), bool) + for bound in ("from", "to") + ): + raise ValueError(f"a relative period needs whole numbers 'from' and 'to', got {period!r}") + elif not isinstance(period, dict) or not period.get("start") or not period.get("end"): + raise ValueError(f"period needs both 'start' and 'end', or 'type': 'relative', got {period!r}") if _has_visualizations(expected_output): visualizations = expected_output.get("visualizations") if not isinstance(visualizations, list) or not visualizations: @@ -454,7 +498,8 @@ def _validate_rework_target(expected_output: dict, user_context: dict | None) -> Raises: ValueError: the item states a rework, and ``user_context.view.document`` is missing, - has no id, or has another id. + has no id, or has another id; or it states ``new_page_charts`` while the open document + sends no pages or has a hidden page, or ``keeps_pages`` leaves out one of its pages. """ rework = _rework(expected_output) if rework is None: @@ -468,6 +513,32 @@ def _validate_rework_target(expected_output: dict, user_context: dict | None) -> 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}") + if "new_page_charts" in rework: + # A new page is one outside keeps_pages, so an open page left off the list would count as new, + # and a chart pushed onto it would pass. A hidden open page cannot be listed, since a kept page + # must stay visible, so an item over a document with one cannot state new_page_charts. + definition = document.get("definition") + open_pages = _pages(definition) if isinstance(definition, dict) else [] + if not open_pages: + raise ValueError( + "rework.new_page_charts needs the open document's pages in user_context.view.document.definition" + ) + hidden = [page.get("id") for page in open_pages if isinstance(page, dict) and page.get("visible") is False] + if hidden: + raise ValueError( + f"rework.new_page_charts cannot be scored over an open document with hidden pages {hidden!r}: " + "showing one again would count as a new page" + ) + unlisted = [ + page.get("id") + for page in open_pages + if isinstance(page, dict) and page.get("id") not in rework["keeps_pages"] + ] + if unlisted: + raise ValueError( + f"rework.new_page_charts needs keeps_pages to list every page of the open document; " + f"missing {unlisted!r}" + ) def build_simulated_reply(expected_output: dict, *, asked: bool = True) -> str: @@ -487,7 +558,7 @@ def build_simulated_reply(expected_output: dict, *, asked: bool = True) -> str: segments.append(f"Please use these charts: {titles}.") period = expected_output.get("period") if isinstance(period, dict): - segments.append(f"Period: {period.get('start')} to {period.get('end')}.") + segments.append(f"Period: {_expected_period_text(period)}.") 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: @@ -510,6 +581,7 @@ class _Applies: rework: bool = False pages_kept: bool = False page_charts: bool = False + new_page_charts: bool = False title: bool = False filters: bool = False @@ -535,6 +607,7 @@ class DocumentEvaluation: reworks_open: bool = False pages_kept: bool = False page_charts_kept: bool = False + new_page_charts_placed: bool = False title_correct: bool = False filters_kept: bool = False judge_reasoning: str = "" @@ -587,6 +660,8 @@ def strict_checks(self) -> dict[str, bool]: checks["document_pages_kept"] = self.pages_kept if self.applies.page_charts: checks["document_page_charts_kept"] = self.page_charts_kept + if self.applies.new_page_charts: + checks["document_new_page_charts"] = self.new_page_charts_placed if self.applies.title: checks["document_title_correct"] = self.title_correct if self.applies.filters: @@ -635,10 +710,76 @@ def evaluate_document_response( return evaluation +def _state_problem(state: Any, kind: str) -> str | None: + """Why a filter's ``state`` cannot be read as a selection, or nothing. + + A missing or ``null`` state reads as all, as does an empty object; any other falsey value is malformed. + """ + if state is None: + return None + if not isinstance(state, dict): + return "expected an object" + for key in ("include", "exclude"): + if state.get(key) is not None and not isinstance(state[key], list): + return f"expected a list of values under {key!r}" + if state.get("include") and state.get("exclude"): + return "expected either 'include' or 'exclude', not both" + if kind == "attribute_filter" and not set(state) <= {"include", "exclude"}: + return "expected only 'include' or 'exclude'" + return None + + +def _filter_mismatch(filter_id: str, actual: Any, expected: dict) -> list[str]: + """Why one drafted filter differs from the expectation, or nothing. + + The kind must match. An attribute filter's ``state`` is compared on its selection, a date + filter on its range with absence significant, and any other key the expectation states as written. + """ + if not isinstance(actual, dict): + return [f"filter {filter_id!r} is {actual!r}, expected an object"] + kind = expected["type"] + if actual.get("type") != kind: + return [f"filter {filter_id!r} has type {actual.get('type')!r}, expected {kind!r}"] + keys = [key for key in expected if key != "type"] + if kind == "date_filter": + keys += [key for key in _DATE_FILTER_RANGE_KEYS if key not in expected] + reasons: list[str] = [] + for key in keys: + if key == "state" and kind == "attribute_filter": + problem = _state_problem(actual.get("state"), kind) + if problem is not None: + reasons.append(f"filter {filter_id!r} has state {actual.get('state')!r}, {problem}") + elif not same_selection(selection_of(actual), selection_of(expected)): + reasons.append( + f"filter {filter_id!r} selects {describe_selection(selection_of(actual))}, " + f"expected {describe_selection(selection_of(expected))}" + ) + elif actual.get(key) != expected.get(key): + reasons.append(f"filter {filter_id!r} has {key} {actual.get(key)!r}, expected {expected.get(key)!r}") + return reasons + + +def _new_page_misses(new_page_charts: list[str], keeps_pages: list[str], pages: list[Any]) -> list[str]: + """Why a chart the edit should have put on a new page is not on one.""" + kept = set(keeps_pages) + reasons: list[str] = [] + for chart in new_page_charts: + holders = [page.get("id") for page in pages if isinstance(page, dict) and chart in _page_visualizations(page)] + if any(page_id not in kept for page_id in holders): + continue + if holders: + reasons.append( + f"chart {chart!r} must be on a new page, but it is only on page(s) {holders!r} of the open document" + ) + else: + reasons.append(f"chart {chart!r} must be on a new page, but no visible page has it") + return reasons + + 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. +) -> tuple[bool, bool, bool, bool, bool, bool]: + """``(reworks_open, pages_kept, page_charts_kept, new_page_charts_placed, title_correct, filters_kept)`` of a v2 edit. A check the item does not state is False. """ @@ -675,6 +816,12 @@ def _score_rework( ) page_charts_kept = not moved + new_page_charts_placed = False + if "new_page_charts" in rework: + misses = _new_page_misses(rework["new_page_charts"], rework["keeps_pages"], pages) + failures.extend(misses) + new_page_charts_placed = not misses + title_correct = False if "title" in rework: title_correct = document.get("title") == rework["title"] @@ -684,11 +831,29 @@ def _score_rework( 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 + actual_filters = _filters(document) + if actual_filters is None: + actual_filters = {} + expected_filters = rework["filters"] + if not isinstance(actual_filters, dict): + reasons = [f"the document filters are {actual_filters!r}, expected an object keyed by filter id"] + else: + reasons = [ + f"the edit lost filter {filter_id!r}" + for filter_id in expected_filters + if filter_id not in actual_filters + ] + reasons += [ + f"the edit added filter {filter_id!r}" + for filter_id in actual_filters + if filter_id not in expected_filters + ] + for filter_id, expected in expected_filters.items(): + if filter_id in actual_filters: + reasons += _filter_mismatch(filter_id, actual_filters[filter_id], expected) + failures.extend(reasons) + filters_kept = not reasons + return reworks_open, pages_kept, page_charts_kept, new_page_charts_placed, title_correct, filters_kept def _score_response( @@ -706,6 +871,7 @@ def _score_response( 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, + new_page_charts=rework is not None and "new_page_charts" in rework, title=rework is not None and "title" in rework, filters=rework is not None and "filters" in rework, ) @@ -766,7 +932,8 @@ def _score_response( 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 + new_draft = reworks_open = pages_kept = page_charts_kept = new_page_charts_placed = False + title_correct = filters_kept = False if rework is None: new_draft = True base_document_id = document_part.get("base_document_id") @@ -783,18 +950,23 @@ def _score_response( 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( + reworks_open, pages_kept, page_charts_kept, new_page_charts_placed, title_correct, filters_kept = _score_rework( rework, document, document_part, pages, failures ) period_correct = False if applies.period: expected_period = expected_output["period"] - period_correct = _period_bounds(document) == (expected_period.get("start"), expected_period.get("end")) + if expected_period.get("type") == "relative": + actual = _period(document) if _is_v2(document) else {} + period_correct = actual.get("type") == "relative" and all( + actual.get(key) == expected_period[key] for key in ("granularity", "from", "to") + ) + else: + period_correct = _period_bounds(document) == (expected_period.get("start"), expected_period.get("end")) if not period_correct: failures.append( - f"the document covers {_period_text(document)}, " - f"expected {expected_period.get('start')} to {expected_period.get('end')}" + f"the document covers {_period_text(document)}, expected {_expected_period_text(expected_period)}" ) charts_matched = False @@ -833,6 +1005,7 @@ def _score_response( reworks_open=reworks_open, pages_kept=pages_kept, page_charts_kept=page_charts_kept, + new_page_charts_placed=new_page_charts_placed, title_correct=title_correct, filters_kept=filters_kept, failures=failures, diff --git a/packages/gooddata-eval/tests/test_agentic_document_skill.py b/packages/gooddata-eval/tests/test_agentic_document_skill.py index d0d58320c..a02b171f4 100644 --- a/packages/gooddata-eval/tests/test_agentic_document_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_document_skill.py @@ -421,7 +421,7 @@ def _rework_part(pages: list[dict] | None = None, **kwargs: Any) -> dict: return _v2_document_part(pages, **kwargs) -def _with_filters(part: dict, filters: dict) -> dict: +def _with_filters(part: dict, filters: Any) -> dict: part["document"]["content"]["filters"] = filters return part @@ -463,6 +463,21 @@ def test_an_edit_of_the_open_document_passes_every_rework_check() -> None: } +def test_every_rework_check_is_published_when_the_fixture_states_it() -> None: + pages = [ + _v2_cover_page(), + _v2_content_page("page2", _REVENUE_TREND), + _v2_content_page("page3", _RETURNS_BY_CATEGORY), + ] + rework = {**_NEW_PAGE_REWORK, "page_charts": {"page2": [_REVENUE_TREND]}, "filters": _US_ONLY} + part = _with_filters(_rework_part(pages), _drafted_country_filter()) + evaluation = _evaluate(part, expected={"rework": rework}, tool=_draft_result(page_count=3)) + assert evaluation.failures == [] + assert {"document_new_page_charts", "document_page_charts_kept", "document_filters_kept"} <= set( + evaluation.strict_checks + ) + + 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) @@ -609,12 +624,302 @@ def test_a_rename_that_left_the_old_title_fails() -> None: assert evaluation.failures == ["the document is titled 'Sales overview', expected 'E2 Sales Review'"] -def test_an_edit_that_changed_the_document_filters_fails() -> None: +def test_an_edit_that_dropped_a_document_filter_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}"] + assert evaluation.failures == ["the edit lost filter 'document_filter_customer_country'"] + + +# What a rework item states: the filter's kind, label and selection, not its display fields. +_US_ONLY = { + "document_filter_customer_country": { + "type": "attribute_filter", + "using": "label/customer_country", + "state": {"include": ["United States"]}, + } +} + + +def _drafted_country_filter(**changes: Any) -> dict: + drafted = { + "type": "attribute_filter", + "title": "Customer country", + "using": "label/customer_country", + "multiselect": False, + "state": {"include": ["United States"]}, + } + drafted.update(changes) + return {"document_filter_customer_country": {k: v for k, v in drafted.items() if v is not None}} + + +@pytest.mark.parametrize( + "drafted", + [ + _drafted_country_filter(), + _drafted_country_filter(localIdentifier="f1"), + _drafted_country_filter(title=None, multiselect=None), + _drafted_country_filter(title="Country"), + ], + ids=["as-drafted", "extra-key", "display-keys-dropped", "retitled"], +) +def test_a_filter_is_compared_only_on_the_keys_the_fixture_states(drafted: dict) -> None: + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _US_ONLY}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.failures == [] + assert evaluation.strict_checks["document_filters_kept"] is True + + +@pytest.mark.parametrize( + ("drafted", "failure"), + [ + ( + _drafted_country_filter(state={}), + "filter 'document_filter_customer_country' selects all, expected include ['United States']", + ), + ( + _drafted_country_filter(state={"include": []}), + "filter 'document_filter_customer_country' selects all, expected include ['United States']", + ), + ( + _drafted_country_filter(state={"include": ["Canada"]}), + "filter 'document_filter_customer_country' selects include ['Canada'], expected include ['United States']", + ), + ( + _drafted_country_filter(state={"exclude": ["United States"]}), + "filter 'document_filter_customer_country' selects exclude ['United States'], " + "expected include ['United States']", + ), + ( + _drafted_country_filter(using="label/customer_state"), + "filter 'document_filter_customer_country' has using 'label/customer_state', " + "expected 'label/customer_country'", + ), + ], + ids=["selection-dropped", "selection-emptied", "other-value", "inverted", "other-label"], +) +def test_a_filter_that_changed_what_it_selects_fails(drafted: dict, failure: str) -> None: + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _US_ONLY}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [failure] + + +def test_an_edit_that_added_a_document_filter_fails() -> None: + drafted = {**_drafted_country_filter(), "document_filter_product_category": {"type": "attribute_filter"}} + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _US_ONLY}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == ["the edit added filter 'document_filter_product_category'"] + + +def test_a_filter_of_another_kind_fails() -> None: + drafted = _drafted_country_filter(type="text_filter") + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _US_ONLY}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [ + "filter 'document_filter_customer_country' has type 'text_filter', expected 'attribute_filter'" + ] + + +_ALL_TIME = {"document_filter_date": {"type": "date_filter", "granularity": "GDC.time.year"}} + + +@pytest.mark.parametrize( + ("drafted", "failure"), + [ + ( + {"type": "date_filter", "granularity": "GDC.time.year", "from": -1, "to": -1}, + "filter 'document_filter_date' has from -1, expected None", + ), + ( + {"type": "date_filter", "granularity": "GDC.time.month"}, + "filter 'document_filter_date' has granularity 'GDC.time.month', expected 'GDC.time.year'", + ), + ], + ids=["range-added", "other-granularity"], +) +def test_a_date_filter_left_all_time_fails_on_a_drafted_range(drafted: dict, failure: str) -> None: + part = _with_filters(_rework_part(), {"document_filter_date": drafted}) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _ALL_TIME}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert failure in evaluation.failures + + +def test_a_date_range_stated_without_granularity_fails_on_a_drafted_granularity() -> None: + drafted = {"document_filter_date": {"type": "date_filter", "granularity": "GDC.time.month", "from": -1, "to": -1}} + part = _with_filters(_rework_part(), drafted) + expected = { + "rework": { + "base_document_id": _OPEN_DOCUMENT, + "filters": {"document_filter_date": {"type": "date_filter", "from": -1, "to": -1}}, + } + } + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == ["filter 'document_filter_date' has granularity 'GDC.time.month', expected None"] + + +def test_a_date_filter_kept_all_time_passes_with_an_unstated_key() -> None: + drafted = {"document_filter_date": {**_ALL_TIME["document_filter_date"], "date": "dataset/date"}} + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _ALL_TIME}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.failures == [] + assert evaluation.strict_checks["document_filters_kept"] is True + + +@pytest.mark.parametrize( + ("drafted", "problem"), + [ + (["United States"], "expected an object"), + ("United States", "expected an object"), + ({"include": 1}, "expected a list of values under 'include'"), + ({"exclude": "Canada"}, "expected a list of values under 'exclude'"), + ([], "expected an object"), + ("", "expected an object"), + (False, "expected an object"), + ( + {"include": ["United States"], "exclude": ["Canada"]}, + "expected either 'include' or 'exclude', not both", + ), + ({"values": ["United States"]}, "expected only 'include' or 'exclude'"), + ], + ids=[ + "list", + "string", + "include-not-a-list", + "exclude-not-a-list", + "empty-list", + "empty-string", + "false", + "both", + "unknown-key", + ], +) +def test_a_drafted_state_that_is_not_a_selection_fails(drafted: Any, problem: str) -> None: + part = _with_filters(_rework_part(), _drafted_country_filter(state=drafted)) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": _US_ONLY}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [f"filter 'document_filter_customer_country' has state {drafted!r}, {problem}"] + + +@pytest.mark.parametrize("drafted", [[], "", False], ids=["empty-list", "empty-string", "false"]) +def test_a_falsey_drafted_state_does_not_pass_as_all(drafted: Any) -> None: + unrestricted = {"type": "attribute_filter", "using": "label/customer_country", "state": {}} + part = _with_filters(_rework_part(), _drafted_country_filter(state=drafted)) + expected = { + "rework": {"base_document_id": _OPEN_DOCUMENT, "filters": {"document_filter_customer_country": unrestricted}} + } + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + + +def test_a_selection_is_compared_without_its_order() -> None: + both = { + "type": "attribute_filter", + "using": "label/customer_country", + "state": {"include": ["United States", "Canada"]}, + } + reordered = _drafted_country_filter(state={"include": ["Canada", "United States"]}) + part = _with_filters(_rework_part(), reordered) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": {"document_filter_customer_country": both}}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.failures == [] + assert evaluation.strict_checks["document_filters_kept"] is True + + +def test_a_selection_with_a_value_missing_fails() -> None: + both = { + "type": "attribute_filter", + "using": "label/customer_country", + "state": {"include": ["United States", "Canada"]}, + } + part = _with_filters(_rework_part(), _drafted_country_filter()) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": {"document_filter_customer_country": both}}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [ + "filter 'document_filter_customer_country' selects include ['United States'], " + "expected include ['United States', 'Canada']" + ] + + +@pytest.mark.parametrize("drafted", [[{"type": "attribute_filter"}], "none"], ids=["list", "string"]) +def test_drafted_filters_that_are_not_keyed_by_id_fail_an_expectation_of_none(drafted: Any) -> None: + part = _with_filters(_rework_part(), drafted) + expected = {"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": {}}} + evaluation = _evaluate(part, expected=expected) + assert evaluation.strict_checks["document_filters_kept"] is False + assert evaluation.failures == [f"the document filters are {drafted!r}, expected an object keyed by filter id"] + + +def test_a_text_filter_state_is_compared_as_written() -> None: + expected_filter = {"document_filter_city": {"type": "text_filter", "state": {"values": ["Prague"]}}} + drafted = {"document_filter_city": {"type": "text_filter", "state": {"values": ["Brno"]}}} + evaluation = _evaluate( + _with_filters(_rework_part(), drafted), + expected={"rework": {"base_document_id": _OPEN_DOCUMENT, "filters": expected_filter}}, + ) + assert evaluation.failures == [ + "filter 'document_filter_city' has state {'values': ['Brno']}, expected {'values': ['Prague']}" + ] + + +# ── a chart the edit adds on a new page ───────────────────────────────────── + +_NEW_PAGE_REWORK = { + "base_document_id": _OPEN_DOCUMENT, + "keeps_pages": ["page1", "page2"], + "new_page_charts": [_RETURNS_BY_CATEGORY], +} + + +def test_a_chart_on_a_new_page_passes() -> None: + pages = [ + _v2_cover_page(), + _v2_content_page("page2", _REVENUE_TREND), + _v2_content_page("page3", _RETURNS_BY_CATEGORY), + ] + evaluation = _evaluate(_rework_part(pages), expected={"rework": _NEW_PAGE_REWORK}, tool=_draft_result(page_count=3)) + assert evaluation.failures == [] + assert evaluation.strict_checks["document_new_page_charts"] is True + + +def test_a_chart_pushed_onto_an_old_page_fails() -> None: + pages = [_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND, _RETURNS_BY_CATEGORY)] + evaluation = _evaluate(_rework_part(pages), expected={"rework": _NEW_PAGE_REWORK}) + assert evaluation.strict_checks["document_new_page_charts"] is False + assert evaluation.failures == [ + "chart 'returns_by_category' must be on a new page, but it is only on page(s) ['page2'] of the open document" + ] + + +def test_a_chart_on_both_an_old_and_a_new_page_passes() -> None: + pages = [ + _v2_cover_page(), + _v2_content_page("page2", _REVENUE_TREND, _RETURNS_BY_CATEGORY), + _v2_content_page("page3", _RETURNS_BY_CATEGORY), + ] + evaluation = _evaluate(_rework_part(pages), expected={"rework": _NEW_PAGE_REWORK}, tool=_draft_result(page_count=3)) + assert evaluation.strict_checks["document_new_page_charts"] is True + + +def test_a_chart_on_a_hidden_new_page_fails() -> None: + hidden = _v2_content_page("page3", _RETURNS_BY_CATEGORY) + hidden["visible"] = False + pages = [_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND), hidden] + evaluation = _evaluate(_rework_part(pages), expected={"rework": _NEW_PAGE_REWORK}) + assert evaluation.strict_checks["document_new_page_charts"] is False + assert evaluation.failures == ["chart 'returns_by_category' must be on a new page, but no visible page has it"] def test_a_document_without_filters_keeps_an_expectation_of_none() -> None: @@ -626,7 +931,13 @@ def test_a_document_without_filters_keeps_an_expectation_of_none() -> None: 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"): + for name in ( + "document_pages_kept", + "document_page_charts_kept", + "document_new_page_charts", + "document_title_correct", + "document_filters_kept", + ): assert name not in evaluation.strict_checks assert evaluation.strict_pass @@ -801,6 +1112,51 @@ def test_a_rework_that_does_not_match_the_open_document_fails_before_any_request assert client.sent == [] +def _open_document_with(pages: list[dict]) -> dict: + return {"view": {"document": {"id": _OPEN_DOCUMENT, "definition": _v2_document(pages)}}} + + +def test_a_new_page_rework_must_list_every_open_page(monkeypatch: pytest.MonkeyPatch) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + user_context = _open_document_with([_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND)]) + expected = {"rework": {**_NEW_PAGE_REWORK, "keeps_pages": ["page1"]}} + with pytest.raises(ValueError, match=r"keeps_pages to list every page of the open document; missing \['page2'\]"): + run_agentic_document_skill("https://h", "tok", "ws", "Add a page", expected, user_context=user_context) + assert client.sent == [] + + +def test_a_new_page_rework_without_the_open_pages_fails_before_any_request(monkeypatch: pytest.MonkeyPatch) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + user_context = {"view": {"document": {"id": _OPEN_DOCUMENT, "title": "Q3 review"}}} + with pytest.raises(ValueError, match="needs the open document's pages"): + run_agentic_document_skill( + "https://h", "tok", "ws", "Add a page", {"rework": _NEW_PAGE_REWORK}, user_context=user_context + ) + assert client.sent == [] + + +def test_a_new_page_rework_over_a_hidden_open_page_fails_before_any_request(monkeypatch: pytest.MonkeyPatch) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + hidden = {**_v2_content_page("page3", _REVENUE_TREND), "visible": False} + user_context = _open_document_with([_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND), hidden]) + expected = {"rework": _NEW_PAGE_REWORK} + with pytest.raises(ValueError, match=r"open document with hidden pages \['page3'\]"): + run_agentic_document_skill("https://h", "tok", "ws", "Add a page", expected, user_context=user_context) + assert client.sent == [] + + +def test_a_new_page_rework_listing_every_open_page_reaches_the_chat(monkeypatch: pytest.MonkeyPatch) -> None: + client = _ScriptedChatClient([_drafting_turn()]) + _install_client(monkeypatch, client) + user_context = _open_document_with([_v2_cover_page(), _v2_content_page("page2", _REVENUE_TREND)]) + expected = {"rework": _NEW_PAGE_REWORK} + run_agentic_document_skill("https://h", "tok", "ws", "Add a page", expected, user_context=user_context) + assert client.sent == ["Add a page"] + + def test_a_rework_of_the_open_document_reaches_the_chat(monkeypatch: pytest.MonkeyPatch) -> None: client = _ScriptedChatClient([_drafting_turn()]) _install_client(monkeypatch, client) @@ -918,6 +1274,51 @@ def test_the_document_format_is_recorded_in_the_run_metadata( ), ({"rework": {"base_document_id": "q3", "title": " "}}, "rework.title must be"), ({"rework": {"base_document_id": "q3", "filters": ["customer_country"]}}, "rework.filters must be"), + ({"rework": {"base_document_id": "q3", "filters": {"f1": {}}}}, r"rework.filters\['f1'\] must state at least"), + ( + {"rework": {"base_document_id": "q3", "filters": {"f1": {"state": {"include": ["US"]}}}}}, + r"rework.filters\['f1'\] must state at least the filter's 'type'", + ), + ( + {"rework": {"base_document_id": "q3", "filters": {"f1": {"type": "attribute_filter", "state": ["US"]}}}}, + r"rework.filters\['f1'\].state is \['US'\], expected an object", + ), + ( + { + "rework": { + "base_document_id": "q3", + "filters": {"f1": {"type": "attribute_filter", "state": {"include": "US"}}}, + } + }, + r"rework.filters\['f1'\].state is \{'include': 'US'\}, expected a list of values under 'include'", + ), + ( + { + "rework": { + "base_document_id": "q3", + "filters": {"f1": {"type": "attribute_filter", "state": {"values": ["US"]}}}, + } + }, + r"rework.filters\['f1'\].state is \{'values': \['US'\]\}, expected only 'include' or 'exclude'", + ), + ( + {"rework": {"base_document_id": "q3", "keeps_pages": ["p2"], "new_page_charts": []}}, + "new_page_charts is empty", + ), + ( + {"rework": {"base_document_id": "q3", "keeps_pages": ["p2"], "new_page_charts": [" "]}}, + "every rework.new_page_charts entry must be a chart id", + ), + ({"rework": {"base_document_id": "q3", "new_page_charts": ["c1"]}}, "new_page_charts needs rework.keeps_pages"), + ({"period": {"type": "relative", "from": -6, "to": -1}}, "a relative period needs its 'granularity'"), + ( + {"period": {"type": "relative", "granularity": "GDC.time.month", "from": "-6", "to": -1}}, + "a relative period needs whole numbers", + ), + ( + {"period": {"type": "relative", "granularity": "GDC.time.month", "from": True, "to": -1}}, + "a relative period needs whole numbers", + ), ], ) def test_an_unusable_fixture_is_rejected_before_any_request(expected: dict, message: str) -> None: @@ -1625,6 +2026,43 @@ def test_a_relative_period_never_matches_the_expected_dates() -> None: ) +_RELATIVE = {"type": "relative", "granularity": "GDC.time.month", "from": -6, "to": -1} + + +def test_a_relative_period_kept_as_stated_passes() -> None: + evaluation = _evaluate(_v2_document_part(period=dict(_RELATIVE)), expected={"period": _RELATIVE}) + assert evaluation.failures == [] + assert evaluation.strict_checks["document_period_correct"] is True + + +@pytest.mark.parametrize( + ("period", "covers"), + [ + ({**_RELATIVE, "from": -12}, "the relative range GDC.time.month -12 to -1"), + ({**_RELATIVE, "granularity": "GDC.time.quarter"}, "the relative range GDC.time.quarter -6 to -1"), + ( + {"type": "absolute", "granularity": "GDC.time.date", "from": "2026-05-01", "to": "2026-10-31"}, + "2026-05-01 to 2026-10-31", + ), + ], + ids=["shifted", "other-granularity", "made-absolute"], +) +def test_a_relative_period_that_changed_fails(period: dict, covers: str) -> None: + evaluation = _evaluate(_v2_document_part(period=period), expected={"period": _RELATIVE}) + assert evaluation.strict_checks["document_period_correct"] is False + assert evaluation.failures == [f"the document covers {covers}, expected the relative range GDC.time.month -6 to -1"] + + +def test_a_relative_period_never_matches_a_v1_document() -> None: + evaluation = _evaluate(_document_part(), expected={"period": _RELATIVE}) + assert evaluation.strict_checks["document_period_correct"] is False + + +def test_the_simulated_reply_states_a_relative_period() -> None: + reply = build_simulated_reply({"period": _RELATIVE}) + assert "Period: the relative range GDC.time.month -6 to -1." in reply + + def test_a_period_that_is_not_an_object_fails_instead_of_crashing() -> None: part = _v2_document_part() part["document"]["period"] = "last six months"