From efa13020622644f27ac78d343c5a877940bdb980 Mon Sep 17 00:00:00 2001 From: debabsah Date: Sat, 3 Oct 2026 14:49:50 -0700 Subject: [PATCH] fix: plan returns missing references as a typed list A plan blocked by the spec put `[e.as_dict() ...]` as a Python repr inside `detail`. It now returns `resolution_errors`, the same list check and apply return (column_not_found carries `candidates`), and `detail` is a short sentence. Exit codes and the other fields are unchanged. --- CHANGELOG.md | 4 +++ chartwright/dashdiff.py | 9 ++++++- docs/VERIFICATION.md | 2 +- tests/test_plan_errors.py | 56 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 tests/test_plan_errors.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d361da7..1dabe0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -189,6 +189,10 @@ The design brain, plus the fixes found reviewing it. ### Changed +- **`plan` reports missing references as data.** When the spec names something the + instance doesn't have, `plan` is `blocked` and returns the same typed + `resolution_errors` list as `check` and `apply`; `detail` is a short sentence. It + used to put a Python-formatted list inside `detail`, which scripts couldn't parse. - A design finding that is withheld because a chart carries a hand-polished height is now reported rather than passing silently. - `--design strict` blocks when the advice cannot be evaluated at all, for diff --git a/chartwright/dashdiff.py b/chartwright/dashdiff.py index 47879bf..9a786bc 100644 --- a/chartwright/dashdiff.py +++ b/chartwright/dashdiff.py @@ -30,6 +30,9 @@ class Plan: cross_filters_changed: bool = False # live setting flipped in the UI, or the spec changed label_colors_changed: bool = False # the pinned series colours differ decompile_losses: list[dict] = field(default_factory=list) + # Why a plan is blocked by the spec: the same typed errors `check` and + # `apply` return (column_not_found carries `candidates`). Empty otherwise. + resolution_errors: list[dict] = field(default_factory=list) @property def clean(self) -> bool: @@ -57,6 +60,7 @@ def to_json(self) -> str: "cross_filters_changed": self.cross_filters_changed, "label_colors_changed": self.label_colors_changed, "decompile_losses": self.decompile_losses, + "resolution_errors": self.resolution_errors, }, indent=2, ) @@ -146,8 +150,11 @@ def plan(target: DashboardSpec, client: SupersetClient) -> Plan: resolution = resolve(target, client) if not resolution.ok: + n = len(resolution.errors) return Plan(dashboard="blocked", - detail=f"target spec has referential errors: {[e.as_dict() for e in resolution.errors]}") + detail=f"the spec names {n} thing{'s' if n != 1 else ''} this instance doesn't have; " + f"see resolution_errors", + resolution_errors=[e.as_dict() for e in resolution.errors]) live_result = decompile_live(target.dashboard.slug, client) try: diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index ff1dcca..7aa1451 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -35,7 +35,7 @@ injection. | Layer | Proves | Where it runs | |---|---|---| -| Offline suite (354 tests, 37 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 | +| Offline suite (357 tests, 38 modules) | Contract, determinism, round-trips, credentials | every PR and push to main, Linux + Windows, mcp 1 and 2 | | Chart-option contract | Every emitted chart option is declared by each version's plugin source | every PR and push to main | | Live guarantee check | 15-chart apply, per-chart data check, ids stable across re-apply | every PR and push to main, all 3 versions | | Lifecycle soak | 500 randomized edit cycles with invariants held | 500 cycles on 6.1.0 and 4.1.4 before release; 25 cycles per version on every PR and push to main | diff --git a/tests/test_plan_errors.py b/tests/test_plan_errors.py new file mode 100644 index 0000000..82400bd --- /dev/null +++ b/tests/test_plan_errors.py @@ -0,0 +1,56 @@ +"""When `plan` is blocked by the spec, it returns the same typed +`resolution_errors` list as `check` and `apply`, not a Python repr inside +`detail` that no script could parse.""" + +import json + +import chartwright.resolver as resolver +from chartwright import ids +from chartwright.dashdiff import plan +from chartwright.resolver import Resolution, ResolutionError +from chartwright.spec import load_spec + +SPEC = load_spec({ + "spec_version": "1", + "dashboard": {"title": "Orders", "slug": "orders"}, + "charts": [{"name": "Revenue by Region", "type": "bar", "x_column": "Region", + "metrics": ["SUM(amount)"], + "dataset": {"database": "warehouse", "table": "orders"}}], + "layout": {"rows": [["Revenue by Region"]]}, +}) + + +class Client: + def __init__(self, uuid): + self.uuid = uuid + + def find_dashboard_by_slug(self, slug): + return {"id": 3, "uuid": self.uuid} + + +def test_missing_references_come_back_as_a_typed_list(monkeypatch): + err = ResolutionError("column_not_found", "Revenue by Region", "Region", + "x_column 'Region' not on dataset 'orders'", ["region"]) + monkeypatch.setattr(resolver, "resolve", lambda spec, client: Resolution(errors=[err])) + p = plan(SPEC, Client(str(ids.dashboard_uuid("orders")))) + out = json.loads(p.to_json()) + assert out["dashboard"] == "blocked" and out["clean"] is False + assert out["resolution_errors"] == [err.as_dict()] + assert out["resolution_errors"][0]["candidates"] == ["region"] + assert "resolution_errors" in out["detail"] and "{" not in out["detail"] + + +def test_a_slug_chartwright_did_not_build_is_blocked_with_no_errors(): + out = json.loads(plan(SPEC, Client("someone-elses-uuid")).to_json()) + assert out["dashboard"] == "blocked" + assert out["resolution_errors"] == [] + + +def test_a_new_dashboard_plans_as_create_with_no_errors(): + class Empty: + def find_dashboard_by_slug(self, slug): + return None + + out = json.loads(plan(SPEC, Empty()).to_json()) + assert out["dashboard"] == "create" + assert out["resolution_errors"] == []