From 130bdc4b0130ee012ba5981b123766417d60ecfc Mon Sep 17 00:00:00 2001 From: debabsah Date: Sun, 26 Jul 2026 13:31:58 -0700 Subject: [PATCH 1/3] fix: review burn-down tiers 1-2 (design brain 3) Tier 1 is the "tool contradicts itself" class; tier 2 is the coverage gap that undercut the product claim. Rulebook version 2 -> 3, because a new warn-severity rule can newly block a `--design strict` pipeline and consumers keying on the version deserve an honest signal. 1.1 `advise --strict` names its gate. `ok` stays error-driven by DESIGN-BRAIN sec.10's contract, so the exit code was the ONLY signal that --strict blocked; it now appends a `design_gate` entry to `errors`, matching check/apply. (The review first read the ok:true/ok:false difference between the two verbs as the defect. It is not - those are different payloads with different `ok` meanings. The missing cause was the gap.) 1.2 MCP error fallback reports the real version instead of a hardcoded "1". 1.3 Severities a rule can actually emit are declared and printed. FOUR rules vary severity per finding, not the two the review named: row-density and min-width escalate to error, row-fill and format-bands soften to info. Since `ok` is error-driven, the two that escalate understated exactly the case a reader most needs. A test now compares each declaration against the severity literals in the rule's own source. The doc also claimed sec.7's table was "GENERATED from the registry" while pointing at a placeholder snippet, so it was hand-maintained and had drifted. tools/gen_rule_table.py generates it for real; --check runs in the suite. 1.4 `decompile` says when its dataset index is truncated. Past the page cap a real dataset became "uuid not resolvable" and its chart was dropped: a wrong answer wearing the costume of an honest loss, which is the one failure this decompiler must never produce. 2.1 New rule `data.unwindowed-history` (warn). No chart time_range, no time_range filter at all, daily-or-finer grain: every load queries and draws the dataset's FULL history. The commonest real-world Superset failure, and the rulebook missed it entirely across 48 rules. Reported ONCE per dashboard rather than per chart, and deliberately silent when a time_range filter exists without a default - `filters.time-default` already names that one-line fix, and double-reporting one remedy at two severities is noise. The first cut did fire per chart and hit the shipped example twice; the example now advises clean honestly, not by suppression. Two existing tests changed because the new rule is real: the gate test's "clean" spec needed a defaulted time picker to actually be clean, and the since-version assertion is now tied to the version constant instead of a hand-listed tuple. 254 tests pass under both pytest invocations; params_drift clean against 4.1.4, 5.0.0 and 6.1.0; rule table check clean. --- .gitignore | 3 + README.md | 10 +- chartwright/cli.py | 12 ++ chartwright/decompile.py | 22 +++- chartwright/design/model.py | 34 +++++- chartwright/design/rules.py | 64 ++++++++-- chartwright/mcp_server.py | 4 +- docs/DESIGN-BRAIN.md | 86 ++++++++++---- docs/VERIFICATION.md | 2 +- tests/test_design_arch.py | 9 +- tests/test_design_gate.py | 4 + tests/test_docs.py | 12 ++ tests/test_review_tier_1_2.py | 214 ++++++++++++++++++++++++++++++++++ tools/gen_rule_table.py | 79 +++++++++++++ 14 files changed, 508 insertions(+), 47 deletions(-) create mode 100644 tests/test_review_tier_1_2.py create mode 100644 tools/gen_rule_table.py diff --git a/.gitignore b/.gitignore index d3b28a1..a021734 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,6 @@ profiles.toml .mcp.json .DS_Store + +# Local working notes (roadmaps, scratch plans). Never committed. +*.local.md diff --git a/README.md b/README.md index 06b9659..1a274ae 100644 --- a/README.md +++ b/README.md @@ -155,11 +155,11 @@ space under `K` deliberately empty. Every rule, drawn and explained: ## Testing and evidence Every push and every pull request runs the full offline suite on Linux and -Windows, plus the full pipeline (apply, -lifecycle soak, stale-tab adversary, fault injection) against real Superset -4.1.4, 5.0.0, and 6.1.0 containers. Chart options are checked against -Superset's own source for every supported version, so a Superset change is -caught in our tests before it reaches your dashboards. +Windows, plus the full pipeline (apply, lifecycle soak, stale-tab adversary, +fault injection) against real Superset 4.1.4, 5.0.0, and 6.1.0 containers. +Chart options are checked against Superset's own source for every supported +version, so a Superset change is caught in our tests before it reaches your +dashboards. Full evidence: [docs/VERIFICATION.md](https://github.com/debabsah/chartwright/blob/main/docs/VERIFICATION.md). Source citations for every Superset behavior the tool relies on: diff --git a/chartwright/cli.py b/chartwright/cli.py index fb6af61..007d0a1 100644 --- a/chartwright/cli.py +++ b/chartwright/cli.py @@ -277,6 +277,18 @@ def _main(argv: list[str] | None = None) -> None: payload["written"] = written if resolution is not None and resolution.errors: payload["resolution_errors"] = [e.as_dict() for e in resolution.errors] + if report.gate(args.strict): + # `ok` stays error-driven by contract (§10), so the exit code was + # the ONLY signal that --strict blocked. Name the cause the way + # check/apply do, or a caller sees exit 1 with nothing to read. + payload.setdefault("errors", []).append({ + "code": "design_gate", + "detail": "warn-severity findings block under --strict; fix them, run " + "`chartwright advise --fix`, or record deliberate exceptions " + "in the spec's design.ignore" + if report.counts["error"] == 0 else + "error-severity findings block; fix them or record deliberate " + "exceptions in the spec's design.ignore"}) print(json.dumps(payload, indent=2)) sys.exit(1 if report.gate(args.strict) else 0) diff --git a/chartwright/decompile.py b/chartwright/decompile.py index e70163d..d6fae40 100644 --- a/chartwright/decompile.py +++ b/chartwright/decompile.py @@ -611,11 +611,27 @@ def width_of(item) -> int: spec["filters"] = filters if not ordered: losses.append(Loss("dashboard", "no representable charts; spec is not valid for apply")) + truncated = getattr(lookup, "truncated", 0) + if truncated: + losses.append(Loss( + "dashboard", + f"the dataset index stopped at {truncated} datasets (page cap); any " + f"'dataset uuid not resolvable' loss above may be a dataset past the cap " + f"rather than a missing one -- re-check those charts before trusting this spec")) return DecompileResult(spec=spec, losses=losses, dataset_uuids=dataset_uuids) +PAGE_CAP = 200 # 20,000 datasets; a runaway guard, not an expected ceiling + + def live_dataset_lookup(client) -> DatasetLookup: - """uuid -> triple, resolved lazily against the live instance.""" + """uuid -> triple, resolved lazily against the live instance. + + Sets `lookup.truncated` when the runaway guard trips, so decompile can SAY + the index is incomplete. Without that, a dataset past the cap silently + became "uuid not resolvable" and its chart was dropped -- a wrong answer + dressed as an honest loss, which is the one failure this decompiler must + never produce.""" cache: dict[str, dict] | None = None def lookup(u: str) -> dict | None: @@ -638,10 +654,12 @@ def lookup(u: str) -> dict | None: "table": d["table_name"], } page += 1 - if page > 200: + if page > PAGE_CAP: + lookup.truncated = len(cache) break return cache.get(u) + lookup.truncated = 0 return lookup diff --git a/chartwright/design/model.py b/chartwright/design/model.py index 1523254..aa24748 100644 --- a/chartwright/design/model.py +++ b/chartwright/design/model.py @@ -12,7 +12,12 @@ from ..resolver import ResolvedDataset, Resolution from ..spec import DEFAULT_HEIGHT, DashboardSpec, MarkdownBlock -DESIGN_BRAIN_VERSION = "2" +# "3" = the post-review batch: the reconciled grid model and stricter +# table_visible_ratio, `polished` provenance in the payload, and the +# data.unwindowed-history rule. Bumped because all three change what a spec +# is told -- a new warn-severity rule can newly block a `--design strict` +# pipeline, so consumers keying on this get an honest signal. +DESIGN_BRAIN_VERSION = "3" KPI_TYPES = {"big_number_total", "big_number_trend"} TIMESERIES_TYPES = {"timeseries_line", "timeseries_bar", "timeseries_area", "timeseries_scatter"} @@ -211,27 +216,44 @@ def fix_height(self, chart, floor: float) -> dict: # -- registry ----------------------------------------------------------------- +SEVERITY_RANK = {"error": 0, "warn": 1, "info": 2} + + @dataclass class Rule: id: str - severity: str # the rule's DEFAULT level; individual findings may - # vary it (e.g. row-density escalates to error when - # a chart is starved), and the overlay's `severity` + severity: str # the rule's DEFAULT level; the overlay's `severity` # map overrides it per deployment. doc: str # one line; the brief prints these fixable: bool data_aware: bool # needs a live resolution (skipped offline) fn: Callable[[RuleContext], Iterator[Finding]] since: str = "1" # design_brain version that introduced the rule + # Every level this rule can actually emit, default first. Four rules vary + # it per finding (row-density escalates to error when a chart is starved; + # row-fill and format-bands soften to info), and `ok` is error-driven -- + # so a rule that can produce an error while advertising `warn` understates + # exactly the case a reader most needs to know about. Declared, printed in + # the generated table, and checked against the rule's source by a test. + severities: tuple[str, ...] = () + + def __post_init__(self) -> None: + self.severities = self.severities or (self.severity,) + + @property + def severity_label(self) -> str: + """'warn', or 'warn/error' when the rule varies it per finding.""" + rest = sorted(set(self.severities) - {self.severity}, key=SEVERITY_RANK.get) + return "/".join([self.severity, *rest]) RULES: dict[str, Rule] = {} def rule(id: str, severity: str, doc: str, fixable: bool = False, data_aware: bool = False, - since: str = "1"): + since: str = "1", severities: tuple[str, ...] = ()): def deco(fn): - RULES[id] = Rule(id, severity, doc, fixable, data_aware, fn, since) + RULES[id] = Rule(id, severity, doc, fixable, data_aware, fn, since, severities) return fn return deco diff --git a/chartwright/design/rules.py b/chartwright/design/rules.py index f580fd8..373d212 100644 --- a/chartwright/design/rules.py +++ b/chartwright/design/rules.py @@ -28,7 +28,8 @@ # -- size: minimum readable geometry ------------------------------------------ -@rule("size.min-width", "warn", "below 3/12 width a chart is unreadable; KPIs need 2/12", since="2") +@rule("size.min-width", "warn", "below 3/12 width a chart is unreadable; KPIs need 2/12", + since="2", severities=("warn", "error")) def min_width(ctx: RuleContext): for c in ctx.spec.charts: if c.type in ("pie", "heatmap"): @@ -328,7 +329,8 @@ def kpi_band(ctx: RuleContext): ) -@rule("layout.row-density", "warn", "too many axis charts side by side starves each of width") +@rule("layout.row-density", "warn", "too many axis charts side by side starves each of width", + severities=("warn", "error")) def row_density(ctx: RuleContext): # Horizontal SLOTS, not flattened charts: a stack of three charts occupies # one slot's width, so it counts once (the user already split vertically). @@ -348,7 +350,8 @@ def row_density(ctx: RuleContext): ) -@rule("layout.row-fill", "warn", "a row should fill the 12-column grid") +@rule("layout.row-fill", "warn", "a row should fill the 12-column grid", + severities=("warn", "info")) def row_fill(ctx: RuleContext): for si, sec in enumerate(ctx.sections): if sec.mode != "rows": @@ -635,7 +638,8 @@ def pivot_columns(ctx: RuleContext): ) -@rule("chart.format-bands", "warn", "conditional-formatting bands must tell one coherent story per metric", since="2") +@rule("chart.format-bands", "warn", "conditional-formatting bands must tell one coherent story per metric", + since="2", severities=("warn", "info")) def format_bands(ctx: RuleContext): for c in ctx.spec.charts: if c.type != "pivot_table" or not c.conditional_formatting: @@ -831,14 +835,20 @@ def grain_vs_range(ctx: RuleContext): ) +_FINE_GRAINS = (None, "PT1S", "PT1M", "PT1H", "P1D") + + +def _unwindowed(ctx: RuleContext) -> bool: + """No defaulted dashboard time filter, so first load spans ALL history.""" + return not any(f.type == "time_range" and f.default for f in ctx.spec.filters) + + @rule("chart.trend-grain", "info", "trend tiles at a fine grain over full history draw thousands of points in a small card", since="2") def trend_grain(ctx: RuleContext): - fine = (None, "PT1S", "PT1M", "PT1H", "P1D") - windowed = any(f.type == "time_range" and f.default for f in ctx.spec.filters) - if windowed: + if not _unwindowed(ctx): return for c in ctx.spec.charts: - if c.type == "big_number_trend" and c.time_grain in fine: + if c.type == "big_number_trend" and c.time_grain in _FINE_GRAINS: yield Finding( "chart.trend-grain", "info", c.name, ctx.where(c.name), f"sparkline at grain {c.time_grain or 'P1D (default)'} with no defaulted " @@ -847,6 +857,44 @@ def trend_grain(ctx: RuleContext): ) +@rule("data.unwindowed-history", "warn", + "timeseries charts with no way to bound the window draw ALL history at their grain", + since="3") +def unwindowed_history(ctx: RuleContext): + """The commonest real-world Superset dashboard failure, and the one the + rulebook missed entirely: nothing bounds the time window, so the board + queries the full table and draws a point per day on every load. + + Reported ONCE for the dashboard, not once per chart: it is a single + property of the dashboard with a single fix, and a six-timeseries board + would otherwise emit six warns for it. + + Deliberately silent when a time_range filter EXISTS without a default -- + `filters.time-default` already names that exact one-line fix, and + double-reporting one remedy at two severities is noise. Deployments that + want it to bite raise that rule via the overlay's `severity` map, which is + the mechanism sec.15.8 chose for precisely this. + + `data.grain-vs-range` is the sibling for when a range IS set: it can count + the points. This one cannot, because the span is "however much data + exists" -- which is what makes it dangerous. Not autofixable: every remedy + changes what data the chart shows (sec.2.2).""" + if not _unwindowed(ctx) or any(f.type == "time_range" for f in ctx.spec.filters): + return + exposed = [c.name for c in ctx.spec.charts + if c.type in TIMESERIES_TYPES and not c.time_range + and c.time_grain in _FINE_GRAINS] + if not exposed: + return + yield Finding( + "data.unwindowed-history", "warn", None, "filters", + f"{exposed} have no time_range and the dashboard has no time_range filter at " + f"all: at a daily-or-finer grain every load queries and draws the dataset's " + f"FULL history. Add a time_range filter WITH a default (e.g. 'Last quarter'), " + f"set the charts' time_range, or coarsen the grain", + ) + + # -- narrative & filters: polish ------------------------------------------------- _MINOR_WORDS = {"a", "an", "the", "of", "by", "vs", "and", "or", "in", "on", diff --git a/chartwright/mcp_server.py b/chartwright/mcp_server.py index cf3dd2c..4926537 100644 --- a/chartwright/mcp_server.py +++ b/chartwright/mcp_server.py @@ -60,7 +60,9 @@ def _advice(spec, resolution=None, audience: str | None = None) -> dict: try: return advise(spec, audience=audience, resolution=resolution).payload() except Exception as e: # noqa: BLE001 - return {"stage": "design", "ok": True, "design_brain": "1", + from .design import DESIGN_BRAIN_VERSION + + return {"stage": "design", "ok": True, "design_brain": DESIGN_BRAIN_VERSION, "counts": {"error": 0, "warn": 0, "info": 0}, "findings": [], "fixed": [], "ignored": [], "errors": [{"code": "overlay" if isinstance(e, ValueError) else "advice", diff --git a/docs/DESIGN-BRAIN.md b/docs/DESIGN-BRAIN.md index ffdc704..e5381cd 100644 --- a/docs/DESIGN-BRAIN.md +++ b/docs/DESIGN-BRAIN.md @@ -1,16 +1,22 @@ # The Design Brain -> **Status: SHIPPED — design brain 2.** This page is both the design and the +> **Status: SHIPPED — design brain 3.** This page is both the design and the > reference for the implementation in `chartwright/design/`. The decision log > at the bottom records every judgment call made without a review gate; §15 > records where the implementation deliberately deviates from the design > text. A verified multi-lens review of the first implementation produced the -> ranked roadmap in [DESIGN-BRAIN-V2.md](DESIGN-BRAIN-V2.md); all 37 items -> are executed, and §7's rule table is generated from the live registry so it -> cannot rot. Rendering-quality verification against a live Superset (UI -> eyeballing of advised-vs-unadvised dashboards) is still pending — the -> thresholds come from the skill's field notes and BI literature, not yet -> from side-by-side screenshots. +> ranked roadmap in [DESIGN-BRAIN-V2.md](DESIGN-BRAIN-V2.md) (36 of 37 items +> in the v2 batch, the last closed afterwards — see that page's status note); +> a later full review of core + brain produced the version-3 changes recorded +> in §15.11 onward. §7's rule table is now genuinely generated +> (`tools/gen_rule_table.py`, checked by `tests/test_docs.py`) — the v2 claim +> that it was pointed at a placeholder snippet, and the table had drifted. +> +> **Still pending: rendering-quality verification against a live Superset.** +> The thresholds come from the skill's field notes and BI literature, not +> from measured pixels, and `docs/CONTRACTS.md` carries no design entries +> while citing every other Superset behaviour to source. Treat the numbers as +> informed judgement until a visual harness lands. ## 1. Problem @@ -202,24 +208,28 @@ layers; the brief prints the merged values and height autofixes target them. Stable ids (`category.slug`) are the public API — `ignore`/`disable` lists and severity overrides key on them, and renames keep working through the -alias table. Severity is the rule's DEFAULT level: **error** = unreadable -for any audience; **warn** = below professional quality; **info** = polish -nudge (a few rules vary it per finding, and the overlay can override it per -deployment). "fix" marks the safe-autofix subset (presentation-only, §9). +alias table. Severity: **error** = unreadable for any audience; **warn** = +below professional quality; **info** = polish nudge. The overlay can override +it per deployment. A `sev` of `warn/error` means the rule's DEFAULT is `warn` +but it escalates per finding — worth reading closely, because `ok` is +error-driven, so those rules can fail a run while advertising `warn`. +"fix" marks the safe-autofix subset (presentation-only, §9). "data" marks rules that only run with a live resolution (`--profile`); several offline rules additionally sharpen or stand down when probes are -available (noted in their text). "v2" marks rules added by the -post-review batch (docs/DESIGN-BRAIN-V2.md). +available (noted in their text). `since` is the design-brain version that +introduced the rule: "2" the post-review batch +(docs/DESIGN-BRAIN-V2.md), "3" the review burn-down (§15.11 onward). -The table below is GENERATED from the registry — do not hand-edit it; -rerun the snippet in the comment and splice. +The table below is GENERATED from the registry by +`tools/gen_rule_table.py --write`; do not hand-edit it. `tests/test_docs.py` +fails when it drifts. - + | id | sev | fix | data | since | rule | |---|---|---|---|---|---| | `chart.dupe` | info | — | — | 1 | two charts answering the identical question is redundancy | -| `chart.format-bands` | warn | — | — | 2 | conditional-formatting bands must tell one coherent story per metric | +| `chart.format-bands` | warn/info | — | — | 2 | conditional-formatting bands must tell one coherent story per metric | | `chart.funnel-stages` | warn | — | ⚡ | 1 | funnels need 3-8 ordered stages | | `chart.heatmap-grid` | warn | — | ⚡ | 1 | a heatmap past ~400 cells is unreadable at any size | | `chart.histogram-bins` | info | — | — | 1 | histograms read best at 10-50 bins | @@ -237,6 +247,7 @@ rerun the snippet in the comment and splice. | `data.grain-vs-range` | warn | — | — | 1 | the time grain should yield a sane number of points for the range | | `data.row-limit-intent` | info | — | — | 1 | row limits doing design work should be deliberate, not defaults | | `data.top-n-sort` | warn | — | — | 2 | a limit without an order is a sample, not a ranking | +| `data.unwindowed-history` | warn | — | — | 3 | timeseries charts with no way to bound the window draw ALL history at their grain | | `filters.count` | warn | — | — | 2 | past ~6 select pickers a filter bar stops being navigable (and each costs a query on load) | | `filters.duplicate-column` | info | — | — | 2 | two filters on the same column fight each other | | `filters.range-default` | info | — | — | 2 | a range slider with no default bounds spans the whole domain | @@ -248,8 +259,8 @@ rerun the snippet in the comment and splice. | `layout.kpi-first` | warn | — | — | 1 | summary KPIs belong above detail charts (inverted pyramid) | | `layout.markdown-height` | info | ✔ | — | 2 | a one-line markdown header doesn't need a chart-sized block | | `layout.orphan-chart` | info | — | — | 1 | a lone narrow chart in its own row looks unfinished | -| `layout.row-density` | warn | — | — | 1 | too many axis charts side by side starves each of width | -| `layout.row-fill` | warn | — | — | 1 | a row should fill the 12-column grid | +| `layout.row-density` | warn/error | — | — | 1 | too many axis charts side by side starves each of width | +| `layout.row-fill` | warn/info | — | — | 1 | a row should fill the 12-column grid | | `layout.section-headers` | info | — | — | 1 | large flat dashboards need markdown signposts | | `layout.tab-balance` | info | — | — | 1 | tabs should carry comparable weight | | `narrative.big-number-format` | info | — | — | 1 | hero numbers deserve a number format | @@ -261,11 +272,13 @@ rerun the snippet in the comment and splice. | `size.hbar-window` | warn | ✔ | — | 1 | horizontal bars need ~0.5 units of height per bar | | `size.heatmap-geometry` | warn | ✔ | — | 1 | heatmaps need >= 5/12 width (7/12 with many columns) and 6 height | | `size.kpi-height` | warn | ✔ | — | 1 | big numbers read best at 2-6 units | -| `size.min-width` | warn | — | — | 2 | below 3/12 width a chart is unreadable; KPIs need 2/12 | +| `size.min-width` | warn/error | — | — | 2 | below 3/12 width a chart is unreadable; KPIs need 2/12 | | `size.pie-geometry` | warn | ✔ | — | 1 | pies need >= 5/12 width and 8 height or the ring shrinks and the legend crowds | -| `size.pivot-window` | warn | ✔ | — | 2 | a pivot's height should show a meaningful share of its row_limit | +| `size.pivot-window` | warn | — | — | 2 | a pivot's height should show a meaningful share of its row_limit | | `size.row-harmony` | warn | ✔ | — | 1 | charts sharing a row should share a height (Superset sizes the row to its tallest child) | -| `size.table-window` | warn | ✔ | — | 1 | a table's height should show a meaningful share of its row_limit | +| `size.table-window` | warn | — | — | 1 | a table's height should show a meaningful share of its row_limit | + + Anything fuzzier than this (reading order beyond KPI-first, grouping related metrics, matched granularity across a row, insight-stating titles) @@ -528,3 +541,32 @@ Recorded during the post-merge review burn-down: note instead of crashing check/apply, but under `strict` an unevaluable overlay blocks: previously one typo in an org-wide `design.yaml` silently disarmed the gate everywhere it was used. +15. **`advise --strict` names its gate.** `ok` stays error-driven by §10's + contract, so the exit code was previously the only signal that `--strict` + blocked. It now appends a `design_gate` entry to `errors`, matching + check/apply. (The review first read the `ok` difference between the two + verbs as the defect; it is not — they are different payloads with + different `ok` meanings. The missing cause was the real gap.) +16. **Severities a rule can actually emit are declared** (`severities=` on + `@rule`) and printed as `warn/error`. Four rules vary severity per + finding; the table showed all four as their default, and since `ok` is + error-driven, the two that escalate to `error` understated exactly the + case a reader most needs. A test compares each declaration against the + severity literals in the rule's own source, so a new escalation fails CI + until it is declared. +17. **§7's table is generated for real.** The doc claimed "GENERATED from the + registry" while pointing at a placeholder snippet, so it was + hand-maintained and had drifted. `tools/gen_rule_table.py --write` + produces it; `--check` runs in the test suite. +18. **`data.unwindowed-history` (new, warn).** The commonest real-world + Superset failure was uncovered: no chart `time_range`, no time_range + filter at all, daily-or-finer grain, so every load queries the dataset's + full history. Reported ONCE per dashboard, not per chart, and deliberately + silent when a time_range filter exists without a default — + `filters.time-default` already names that one-line fix, and double- + reporting one remedy at two severities is noise. Deployments that want + that case to bite raise it via the overlay `severity` map (§15.8). +19. **`decompile` says when its dataset index is truncated.** The lookup + stops at a page cap; past it, a real dataset became "uuid not resolvable" + and its chart was dropped — a wrong answer wearing the costume of an + honest loss, which is the one failure this decompiler must never produce. diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index c073c28..f949754 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -34,7 +34,7 @@ injection. | Layer | Proves | Where it runs | |---|---|---| -| Offline suite (236 tests, 26 modules) | Contract, determinism, round-trips, credentials | every push and every PR, Linux + Windows | +| Offline suite (254 tests, 27 modules) | Contract, determinism, round-trips, credentials | every push and every PR, Linux + Windows | | Chart-option contract | Every emitted chart option is declared by each version's plugin source | every push | | Live guarantee check | 15-chart apply, per-chart data check, ids stable across re-apply | every push, 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 push | diff --git a/tests/test_design_arch.py b/tests/test_design_arch.py index 3c076ff..ec1fef3 100644 --- a/tests/test_design_arch.py +++ b/tests/test_design_arch.py @@ -73,9 +73,14 @@ def test_version_constant_flows_to_payload(): assert rep.payload()["design_brain"] == DESIGN_BRAIN_VERSION -def test_every_v2_rule_carries_since(): - assert all(r.since in ("1", "2") for r in RULES.values()) +def test_every_rule_carries_a_known_since_version(): + """Tied to the version constant, not a hand-listed tuple, so bumping the + rulebook does not need an edit here -- but a typo'd `since` still fails.""" + known = {str(v) for v in range(1, int(DESIGN_BRAIN_VERSION) + 1)} + assert all(r.since in known for r in RULES.values()), { + r.id: r.since for r in RULES.values() if r.since not in known} assert any(r.since == "2" for r in RULES.values()) + assert any(r.since == DESIGN_BRAIN_VERSION for r in RULES.values()) def test_golden_dogfood_example_advises_clean(): diff --git a/tests/test_design_gate.py b/tests/test_design_gate.py index 67e6bba..cbdaa15 100644 --- a/tests/test_design_gate.py +++ b/tests/test_design_gate.py @@ -53,6 +53,10 @@ def test_clean_spec_does_not_block(design_dir): "dashboard": {"title": "T", "slug": "t"}, "charts": [{"type": "timeseries_line", "name": "L", "dataset": DS, "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8}], + # A defaulted time_range picker is what makes a timeseries dashboard + # clean: without one, data.unwindowed-history warns that every load + # draws the dataset's full history. + "filters": [{"type": "time_range", "name": "Date", "default": "Last month"}], "layout": {"rows": [["L"]]}, }) advice = _advice_payload(clean) diff --git a/tests/test_docs.py b/tests/test_docs.py index bbfe96a..b31b99e 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -7,6 +7,7 @@ """ import re +import sys from pathlib import Path import pytest @@ -16,6 +17,17 @@ _ROW = re.compile(r"Offline suite \((\d+) tests, (\d+) modules\)") +def test_design_brain_rule_table_matches_the_registry(): + """DESIGN-BRAIN.md section 7 claimed "GENERATED from the registry" while + pointing at a placeholder snippet, so it was hand-maintained and had + drifted (four rules that vary severity all printed as their default). + There is a real generator now, and this runs its --check path.""" + sys.path.insert(0, str(REPO)) + from tools.gen_rule_table import main as gen + + assert gen(["--check"]) == 0, "run: python tools/gen_rule_table.py --write" + + def test_no_test_module_imports_through_the_tests_package(): """`tests` has no __init__.py, so pytest puts tests/ on sys.path, not the repo root. `from tests.x import ...` resolves only under `python -m pytest` diff --git a/tests/test_review_tier_1_2.py b/tests/test_review_tier_1_2.py new file mode 100644 index 0000000..b3e9710 --- /dev/null +++ b/tests/test_review_tier_1_2.py @@ -0,0 +1,214 @@ +"""Review burn-down, tiers 1 and 2: places the tool contradicted itself, plus +the coverage gap that undercut the product claim. +""" + +import inspect +import json +import re + +import pytest + +from chartwright.design import advise +from chartwright.design.model import RULES, SEVERITY_RANK +from chartwright.design.presets import Overlay +from chartwright.spec import load_spec + +DS = {"database": "db", "table": "orders"} +EMPTY = Overlay() + + +def mk(charts, filters=None): + data = {"spec_version": "1", "dashboard": {"title": "T", "slug": "t"}, + "charts": charts, "layout": {"rows": [[c["name"] for c in charts]]}} + if filters: + data["filters"] = filters + return load_spec(data) + + +def ts(name, **kw): + return {"type": "timeseries_line", "name": name, "dataset": DS, + "metrics": ["COUNT(*)"], "time_column": "ts", "height": 8, **kw} + + +def rules_of(spec): + return sorted(f.rule for f in advise(spec, overlay=EMPTY).findings) + + +# -- 1.3 / 1.6: severity truth --------------------------------------------------- + + +def test_declared_severities_cover_what_each_rule_can_emit(): + """`ok` is error-driven, so a rule advertising `warn` while emitting + `error` understates the one case that fails a run. Checked against the + rule's own source: a new escalation fails here until it is declared.""" + literal = re.compile(r'"(error|warn|info)"') + undeclared = {} + for rid, r in RULES.items(): + emitted = set(literal.findall(inspect.getsource(r.fn))) + missing = emitted - set(r.severities) + if missing: + undeclared[rid] = sorted(missing) + assert not undeclared, ( + f"add severities=(...) to @rule for these: {undeclared}") + + +def test_severity_label_shows_escalation_default_first(): + assert RULES["size.min-width"].severity_label == "warn/error" + assert RULES["layout.row-fill"].severity_label == "warn/info" + assert RULES["chart.dupe"].severity_label == "info" + + +def test_escalating_rules_really_do_escalate(): + """The declaration is not decoration: min-width yields a hard error.""" + spec = mk([ts("A", width=1), ts("B", width=11)]) + errs = [f for f in advise(spec, overlay=EMPTY).findings + if f.rule == "size.min-width" and f.severity == "error"] + assert errs and advise(spec, overlay=EMPTY).ok is False + + +def test_every_rule_declares_a_sane_severity_set(): + for r in RULES.values(): + assert r.severities and r.severities[0] == r.severity + assert set(r.severities) <= set(SEVERITY_RANK) + + +# -- 2.1: the unwindowed-history coverage gap ------------------------------------ + + +def test_no_time_filter_at_all_is_flagged_once_for_the_dashboard(): + """The commonest real failure: nothing bounds the window, so every load + draws the dataset's full history at daily grain.""" + spec = mk([ts("A"), ts("B"), ts("C")]) + found = [f for f in advise(spec, overlay=EMPTY).findings + if f.rule == "data.unwindowed-history"] + assert len(found) == 1, "one dashboard-level finding, not one per chart" + assert found[0].chart is None and found[0].severity == "warn" + for name in ("A", "B", "C"): + assert name in found[0].detail + + +@pytest.mark.parametrize("charts,filters", [ + # a defaulted picker bounds the load + ([ts("A")], [{"type": "time_range", "name": "D", "default": "Last month"}]), + # an undefaulted picker is filters.time-default's job, not a second warn + ([ts("A")], [{"type": "time_range", "name": "D"}]), + # a coarse grain is not a point explosion + ([ts("A", time_grain="P1M")], None), + # the chart bounds itself + ([ts("A", time_range="Last month")], None), +]) +def test_unwindowed_history_stays_silent_when_covered(charts, filters): + assert "data.unwindowed-history" not in rules_of(mk(charts, filters)) + + +def test_undefaulted_picker_is_reported_exactly_once_by_the_other_rule(): + """No double-reporting of one remedy at two severities.""" + got = rules_of(mk([ts("A")], [{"type": "time_range", "name": "D"}])) + assert got == ["filters.time-default"] + + +def test_shipped_example_still_advises_clean(): + """The golden dogfood must stay clean HONESTLY, not by suppression.""" + from pathlib import Path + + ex = json.loads((Path(__file__).resolve().parent.parent / "examples" + / "nyc_taxi_operations.json").read_text(encoding="utf-8")) + rep = advise(load_spec(ex), overlay=EMPTY) + assert rep.counts == {"error": 0, "warn": 0, "info": 0}, [f.key for f in rep.findings] + + +# -- 1.1: advise --strict names its gate ------------------------------------------ + + +def test_advise_strict_gate_is_machine_readable(): + """`ok` stays error-driven by contract, so without this the exit code was + the only signal that --strict blocked.""" + from chartwright.cli import _main + + spec_warn = mk([ts("A", height=2)]) # below min axis height -> warn + rep = advise(spec_warn, overlay=EMPTY) + assert rep.counts["warn"] and rep.ok is True # ok unchanged, per DESIGN-BRAIN 10 + assert rep.gate(strict=True) and not rep.gate(strict=False) + assert _main is not None + + +def test_advise_strict_emits_design_gate_entry(tmp_path, capsys, monkeypatch): + from chartwright.cli import main + + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + spec = tmp_path / "s.json" + spec.write_text(json.dumps({ + "spec_version": "1", "dashboard": {"title": "T", "slug": "t"}, + "charts": [ts("A", height=2)], "layout": {"rows": [["A"]]}}), encoding="utf-8") + + with pytest.raises(SystemExit) as exc: + main(["advise", str(spec), "--strict"]) + assert exc.value.code == 1 + payload = json.loads(capsys.readouterr().out) + assert payload["ok"] is True # error-driven, unchanged + assert [e["code"] for e in payload["errors"]] == ["design_gate"] + assert "--strict" in payload["errors"][0]["detail"] + + +def test_advise_without_strict_has_no_gate_entry(tmp_path, capsys, monkeypatch): + from chartwright.cli import main + + monkeypatch.setenv("CHARTWRIGHT_DESIGN_DIR", str(tmp_path)) + spec = tmp_path / "s.json" + spec.write_text(json.dumps({ + "spec_version": "1", "dashboard": {"title": "T", "slug": "t"}, + "charts": [ts("A", height=2)], "layout": {"rows": [["A"]]}}), encoding="utf-8") + with pytest.raises(SystemExit) as exc: + main(["advise", str(spec)]) + assert exc.value.code == 0 + assert "errors" not in json.loads(capsys.readouterr().out) + + +# -- 1.2: one version constant ---------------------------------------------------- + + +def test_mcp_error_fallback_reports_the_real_version(): + """It hardcoded "1" while everything else reported the constant.""" + import chartwright.mcp_server as mcp + from chartwright.design import DESIGN_BRAIN_VERSION + + src = inspect.getsource(mcp) + assert '"design_brain": "1"' not in src + payload = mcp._advice(object()) # not a spec -> the except branch + assert payload["design_brain"] == DESIGN_BRAIN_VERSION + assert payload["errors"] + + +# -- 1.4: decompile says when its dataset index is truncated ----------------------- + + +def test_decompile_names_a_truncated_dataset_index(): + """Past the page cap a real dataset became 'uuid not resolvable' and its + chart was dropped: a wrong answer wearing the costume of an honest loss.""" + from chartwright.compiler import compile_bundle + from chartwright.decompile import decompile_bundle + from chartwright.testing import stub_resolution + + spec = mk([ts("A")]) + bundle = compile_bundle(spec, stub_resolution(spec)) + + def lookup(_u): + return None + lookup.truncated = 20000 + + losses = [x.what for x in decompile_bundle(bundle, lookup).losses] + assert any("dataset index stopped at 20000" in x for x in losses), losses + assert any("not resolvable" in x for x in losses) + + +def test_untruncated_decompile_says_nothing_about_the_index(): + from chartwright.compiler import compile_bundle + from chartwright.decompile import decompile_bundle + from chartwright.testing import stub_resolution + + spec = mk([ts("A")]) + ds = stub_resolution(spec).for_chart(spec.charts[0].dataset) + result = decompile_bundle( + compile_bundle(spec, stub_resolution(spec)), + lambda u: {"database": "db", "schema": None, "table": "orders"} if u == ds.uuid else None) + assert result.losses == [], [x.as_dict() for x in result.losses] diff --git a/tools/gen_rule_table.py b/tools/gen_rule_table.py new file mode 100644 index 0000000..1cfca9b --- /dev/null +++ b/tools/gen_rule_table.py @@ -0,0 +1,79 @@ +"""Generate the rulebook table in docs/DESIGN-BRAIN.md section 7 from the registry. + +The doc has claimed "GENERATED from the registry - do not hand-edit" since v2, +but the snippet it pointed at was a placeholder, so the table was in fact +hand-maintained and had drifted: four rules that can emit a severity other +than their default were all printed as plain `warn`, and `ok` is error-driven, +so the two that escalate to `error` understated exactly the case a reader most +needs to know about. + + python tools/gen_rule_table.py # print the table + python tools/gen_rule_table.py --write # splice it into the doc + python tools/gen_rule_table.py --check # exit 1 if the doc is stale + +`tests/test_docs.py` runs the --check path, so the claim is now true. +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO)) + +from chartwright.design.model import RULES # noqa: E402 +import chartwright.design.rules # noqa: E402,F401 (import for rule registration) + +DOC = REPO / "docs" / "DESIGN-BRAIN.md" +BEGIN = "" +END = "" + + +def render() -> str: + lines = ["| id | sev | fix | data | since | rule |", "|---|---|---|---|---|---|"] + for rid in sorted(RULES): + r = RULES[rid] + lines.append( + f"| `{r.id}` | {r.severity_label} | {'✔' if r.fixable else '—'} | " + f"{'⚡' if r.data_aware else '—'} | {r.since} | {r.doc} |" + ) + return "\n".join(lines) + + +def splice(text: str, table: str) -> str: + block = f"{BEGIN}\n\n{table}\n\n{END}" + if BEGIN in text and END in text: + return re.sub(re.escape(BEGIN) + r".*?" + re.escape(END), lambda _: block, + text, flags=re.S) + raise SystemExit(f"markers not found in {DOC}; add {BEGIN} / {END} around the table") + + +def main(argv=None) -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--write", action="store_true", help="splice the table into the doc") + ap.add_argument("--check", action="store_true", help="exit 1 if the doc is stale") + args = ap.parse_args(argv) + + table = render() + if not (args.write or args.check): + print(table) + return 0 + text = DOC.read_text(encoding="utf-8") + new = splice(text, table) + if args.check: + if new != text: + print("docs/DESIGN-BRAIN.md rule table is STALE; run: " + "python tools/gen_rule_table.py --write") + return 1 + print(f"rule table up to date ({len(RULES)} rules)") + return 0 + DOC.write_text(new, encoding="utf-8") + print(f"wrote {len(RULES)} rules into {DOC}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) From 470f178c75c2bee87e11cdaf36ab31ecc8775744 Mon Sep 17 00:00:00 2001 From: debabsah Date: Sun, 26 Jul 2026 14:00:15 -0700 Subject: [PATCH 2/3] docs: CONTRIBUTING named the pytest invocation that HIDES the CI failure mode The guide told contributors to run `python -m pytest tests/ -q`. CI runs the bare `pytest tests/ -q`. That is not cosmetic: `python -m` puts the current directory on sys.path and CI does not, so a test module importing through the `tests.` package passes locally and is uncollectable in CI. That exact gap let `tests/test_design_rules_v2.py` sit unrun until the pull_request trigger landed - 14 rule tests, green locally, never once executed by CI. The guide was pointing at the invocation that masked it. `tests/test_docs.py` already guards the specific import shape; this closes the habit that produced it. --- CONTRIBUTING.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5e26ca8..d6a16f6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,8 +26,14 @@ is also worth fixing. staying decompilable and every supported Superset version staying in contract (docs/CONTRACTS.md). - Every pull request runs the same gates as every push: the offline suite - (`python -m pytest tests/ -q`) and the params drift check + (`pytest tests/ -q`) and the params drift check (`python tools/params_drift.py --all`) must both pass. +- Run the suite exactly as written above, from the repo root. `python -m + pytest` also works, but it puts the current directory on `sys.path`, which + CI does not: a test module importing through the `tests.` package passes + that way and is uncollectable in CI. That gap once let a whole test module + sit unrun; `tests/test_docs.py` now guards the specific case, but the habit + is what keeps the two honest. By contributing, you agree that your contributions are licensed under the Apache License 2.0, the same license as the project. From 60a08a8a9eb8e66ba9334eeb645958a64ea3bbae Mon Sep 17 00:00:00 2001 From: debabsah Date: Mon, 27 Jul 2026 23:32:09 -0700 Subject: [PATCH 3/3] ci: run on every pull request; add CHANGELOG; number the release 0.2.0 Three release-hygiene items. CI was scoped to `pull_request: branches: [main]`, so a stacked pull request (one targeting another feature branch) ran no checks at all. That is precisely the case the trigger was added to prevent, and it silently skipped the docs branch. The filter is removed. Added CHANGELOG.md. The project had none, which is off-message for a tool whose pitch is that a dashboard should get the workflow code already has. Numbered the pending release 0.2.0 rather than 0.3.0. Only 0.1.0 has ever been tagged or published to PyPI, 0.2.0 was never used, and nothing in the docs referenced 0.3.0, so the gap was avoidable rather than forced. Note that the package version and DESIGN_BRAIN_VERSION are independent: the rulebook is at "3" and stays there. --- .github/workflows/ci.yml | 4 ++- CHANGELOG.md | 71 ++++++++++++++++++++++++++++++++++++++++ pyproject.toml | 2 +- 3 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 CHANGELOG.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 48324e7..ace2f93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,8 +6,10 @@ on: # Branches must clear the same bar as main BEFORE they merge: the offline # matrix (Linux + Windows, 3.11) and the live matrix (three Superset # versions) are exactly the checks a feature branch is most likely to break. + # Deliberately unfiltered by base branch: a stacked PR (one that targets + # another feature branch rather than main) would otherwise run no checks at + # all, which is the case this trigger exists to prevent. pull_request: - branches: [main] workflow_dispatch: jobs: diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..4aaa152 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,71 @@ +# Changelog + +Notable changes per release. Versions follow [semantic versioning](https://semver.org); +while the major version is 0, minor bumps may include breaking changes and say so here. + +## 0.2.0 (unreleased) + +The design brain, plus the fixes found reviewing it. + +### Added + +- **The design brain**, an optional layer that checks whether a dashboard + reads well, not just whether it imports. Off with `--design off`, which + restores byte-identical output. + - `chartwright brief` prints design guidance to read before writing a spec, + tuned to an audience preset (`executive`, `analytical`, `operational`). + - `chartwright advise` reviews a finished spec against 49 rules with stable + ids. `--fix` applies the safe geometry repairs; `--profile` adds checks + that need live metadata, such as a time axis on a non-temporal column. + - `chartwright redesign` decompiles a live dashboard, audits it, applies the + safe fixes, and writes the repaired spec. + - `chartwright calibrate` proposes recommended heights from the sizes you + have polished by hand and absorbed. + - `check` and `apply` gain `--design off|warn|strict`, defaulting to `warn`. + - An optional `design` block in the spec sets the audience and suppresses + individual rules per dashboard or per chart. + - `~/.config/chartwright/design.yaml` tunes thresholds, disables rules, and + appends house guidance for a whole deployment. + - Four MCP tools covering the same ground, taking the server from six to ten. +- `data.unwindowed-history` warns when nothing bounds a timeseries dashboard's + date range, so every load queries the dataset's full history. +- Apply-time warning when a table or pivot renders more rows than its + configured height can show, which otherwise hides them behind an inner + scrollbar with everything else looking healthy. + +### Fixed + +- `sort_by` on table charts had no effect. It compiled to a sort direction + with no sort key, so a `row_limit` returned arbitrary rows rather than a + ranking. It now compiles correctly in both aggregate and raw mode, is + validated against the dataset like every other reference, and survives a + decompile, so `chartwright plan` no longer reports a sorted table as + permanently changed. +- Rows whose widths were left implicit could sum to more than the twelve + column grid when a row held repeated markdown blocks. +- `chartwright absorb` now writes its report before touching the spec file, so + a reporting failure cannot follow a silent mutation. +- `chartwright decompile` says so when its dataset index is truncated, instead + of reporting a dataset it never looked at as unresolvable. +- Duplicate tab titles are rejected at validation rather than producing a + dashboard whose tabs cannot be told apart. +- Layout sketches are parsed once per holder rather than once per lookup. + +### Changed + +- 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 + example because `design.yaml` is malformed. It previously reported zero + findings and allowed the run. +- Tables and pivots are expected to show at least half the rows their + `row_limit` requests, up from a quarter. Specs with large explicit row + limits will see more findings than before. + +## 0.1.0 + +Initial release. Compile a spec into an Apache Superset dashboard, verify +every dataset, column, and metric before building, and check every chart +returns data afterwards. Decompile an existing dashboard back into a spec, +diff a spec against what is live with `chartwright plan`, and restore a +dashboard from an automatic backup. diff --git a/pyproject.toml b/pyproject.toml index 1f82f92..8d782ea 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "chartwright" -version = "0.3.0" +version = "0.2.0" description = "Build, verify, and maintain Apache Superset dashboards from small spec files." license = "Apache-2.0" readme = "README.md"