Review burn-down tiers 1-2: severity truth, a real rule-table generator, and the unwindowed-history gap - #3
Merged
Merged
Conversation
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.
…ure 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tiers 1 and 2 of the post-merge review burn-down. Tier 1 is the "tool
contradicts itself" class; tier 2 is the coverage gap that undercut the
product claim.
Rulebook version 2 -> 3: a new warn-severity rule can newly block a
--design strictpipeline, so consumers keying ondesign_brainget anhonest signal. It also retroactively labels the threshold changes from #2,
which shipped unversioned.
Tier 1: places the tool told you two different things
1.1
advise --strictnames its gate.okstays error-driven byDESIGN-BRAIN sec.10's contract, so the exit code was the ONLY signal that
--strictblocked. It now appends adesign_gateentry toerrors, matchingcheck/apply.
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:
layout.row-densityandsize.min-widthescalate toerror,layout.row-fillandchart.format-bandssoften toinfo. Sinceokiserror-driven, the two that escalate understated exactly the case a reader most
needs to know about. The table now prints
warn/error, and a test compareseach declaration against the severity literals in the rule's own source, so a
new escalation fails CI until it is declared.
While fixing that: sec.7's table claimed "GENERATED from the registry - do
not hand-edit" while pointing at a placeholder snippet, so it was in fact
hand-maintained and had drifted.
tools/gen_rule_table.pygenerates it forreal (
--write/--check), andtests/test_docs.pyruns the check.1.4
decompilesays when its dataset index is truncated. Past the pagecap 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.
Tier 2: the coverage gap
New rule
data.unwindowed-history(warn). No charttime_range, notime_rangefilter at all, daily-or-finer grain: every load queries and drawsthe dataset's FULL history. This is the commonest real-world Superset
dashboard failure - the board that takes 40 seconds to load and draws
thousands of unreadable points - and the rulebook missed it entirely across
48 rules while shipping
narrative.title-style.Two design decisions worth reviewing:
a single fix; a six-timeseries board would otherwise emit six warns for it.
time_rangefilter exists without a default.filters.time-defaultalready names that exact one-line fix, anddouble-reporting one remedy at two severities is noise. Deployments that
want that case to bite raise it via the overlay
severitymap, which is themechanism sec.15.8 chose for precisely this.
The first cut fired per chart and hit the shipped NYC example twice. The
example now advises clean honestly, not by adding a suppression to it.
Two existing tests changed, because the new rule is real
test_design_gate.py's "clean" spec needed a defaulted time picker toactually be clean. That is the rule working.
since-version assertion is now tied toDESIGN_BRAIN_VERSIONinsteadof a hand-listed
("1", "2")tuple, so bumping the rulebook no longer needsan edit there while a typo'd
sincestill fails.Verification
runs, and
python -m pytest).params_drift --allclean against 4.1.4, 5.0.0 and 6.1.0.tools/gen_rule_table.py --checkclean.Still open (tracked, not in this PR)
rule reads the spec, never the rendered result; thresholds come from field
notes and BI literature, not measured pixels, and
docs/CONTRACTS.mdstillcarries no design entries while citing every other Superset behaviour to
source. The status block in DESIGN-BRAIN.md now says so plainly.
decompile | adviseacross a real instance) to measurefinding density before trusting
--design warnby default.rather than in warehouse work.
restorevalidates one YAML then trusts the rest of the archive.calibrate --writeclobbers comments in a hand-maintaineddesign.yaml.