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/.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/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/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. 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/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" 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())