Add eval-driven quality harness for Hallmark outputs#12
Conversation
Build a deterministic slop detector grounded in Impeccable's 37 patterns plus Hallmark's own gates, score self-contained fixtures across genres, and run a 10-cycle eval-driven hillclimb. Phase 1 (v1, cycles 1-5): close gaps the detector found, adding gates 70-77 to references/slop-test.md; fixtures climb 74.2 -> 98.3. Cycle 6: per "Your Evals Will Break", upgrade the eval to v2 -- six new detector rules (incl. hero-float/gate 54 the v1-perfect fixtures had been violating), a cross-fixture order parameter (macrostructure reuse), and two adversarial fixtures. Score honestly drops to 76.4. Phase 2 (v2, cycles 7-10): add gates 78-84 and climb back to 98.7, resisting a dark/neon/metric-hero brief. The skill gained 15 gates motivated by what the eval could measure. Full curve in evals/results/history.md.
Audit the in-repo Hallmark corpus (homepage + examples) surfaced a real false-positive rate. Fix the worst offenders so the signal is trustworthy: - placeholder-names: only flag actual placeholder names (Jane Doe, Acme, lorem ipsum), not ordinary words like "seamless"/"unleash" in prose. - ai-palette: require the violet->cyan *ramp*, not a single deliberate brand hue, so a midnight-violet brand is no longer flagged. - font counting: count a monospace family toward the budget only when used outside code (per gate 39); stop counting unused --font-mono tokens. - multi-theme scoping: resolve tokens from the active [data-theme] only, and label 22-theme / component-library stylesheets low-confidence instead of scoring them as one page. evals/audit-site.mjs inlines a page's linked stylesheets so the detector can score real shipped pages. Fixtures unchanged (all still 5.00/5 on v1 and v2); true positives (Inter in hyperlane, gradient text in bananastudio, "Acme" in tally) are retained while the false positives are removed.
|
@adewale is attempting to deploy a commit to the Together AI Team on Vercel. A member of the Team first needs to authorize it. |
mikekangdev
left a comment
There was a problem hiding this comment.
👍 APPROVED WITH COMMENTS
Found: 2 Must Fix, 9 Consider Changing, 2 Informational
Tiers by severity: critical/high → 🔴 Must Fix · medium → 🟡 Consider Changing · low/info → 🔵 Informational
Decided by policy rules: judgment-needs-human · reviewer-advises-feedback · default-outcome
What this PR does
This PR adds a self-contained eval harness — a deterministic slop detector, scoring core, non-mutating check script, and fixtures with frozen judge sidecars — for measuring how 'AI-generated' Hallmark's outputs look, plus the 15 slop-test gates that came out of using it. It touches no production code, ships nothing that runs at emit time, and the committed hillclimb history backs up the reported score trajectory, so this looks safe to merge as-is. The main things worth tightening are a couple of missing-input-validation spots (unvalidated judge scores can silently become NaN, and check.mjs crashes ungracefully on a bad fixture/judge file) rather than anything that undermines the harness's purpose. The rest of the feedback is minor readability nits and requests for more edge-case test coverage in a detector the PR itself already documents as heuristic and non-authoritative.
Reviewed by: 👨💻 Developer · 🧪 QA
Agent feedback
- 👨💻 Developer — Developer flagged a few moderate robustness and readability issues in the scoring core and detector — mainly unvalidated inputs and some hard-to-follow expressions — nothing that blocks this self-contained tooling addition.
- 🧪 QA — QA mostly asked for more edge-case test coverage in the detector's CSS/color heuristics, plus one real gap around check.mjs crashing on malformed fixture data; nothing that changes the harness's fitness for its stated advisory purpose.
🔴 Must Fix
| # | Finding | Recommendation |
|---|---|---|
| 1 | Structure score formula can't distinguish unstamped fixtures from duplicate macrostructuresevals/core.mjs:25 |
Add explicit tests or a test fixture set verifying: all-unstamped yields 0/5, all-duplicate yields 0/5, and 1 collision on 5 fixtures yields 2.5/5. Confirm the penalty ratio (1 collision = 1 duplicate, so 2 copies cost 2.5 points) matches the evaluation's intent. |
| 2 | check.mjs has no graceful handling for missing or malformed fixture/judge filesevals/check.mjs:38 |
Add error handling to evaluateCycle() to catch file-read or JSON-parse errors and report them clearly; or add a test fixture with a missing or corrupt judge sidecar to confirm the error message is helpful. |
Consider Changing & Informational
| # | Finding | Recommendation | |
|---|---|---|---|
| 1 | 🟡 | Missing validation for judge sidecar craft axis values before averaging.evals/core.mjs:49
|
Before line 49, validate that all CRAFT_AXES exist in judge and are numbers: for (const a of CRAFT_AXES) { if (judge[a] == null || typeof judge[a] !== 'number') throw new Error(...); }
|
| 2 | 🟡 | 2-value padding shorthand handling is correct but easy to misreadevals/detector.mjs:556
|
Change line 556 to top = pxOf(p[0], map); bottom = pxOf(p[0], map); if intent is symmetric, or clarify with a comment. If the intent is to use p[0] for both, the code is correct but the logic flow is confusing—consider a comment explaining 2-value padding applies the same to top and bottom. |
| 3 | 🟡 | Hero-metric rule buries its condition in a nested IIFEevals/detector.mjs:595
|
Extract the font-size check to a named helper function (e.g., hasBigFontSize(rule, map)) and call it from the predicate, or refactor the find logic to iterate and check both conditions in sequence for clarity. |
| 4 | 🟡 | 4-value padding shorthand path in heroPadding is untestedevals/detector.mjs:540
|
Add a test fixture (or extend an existing one) with hero padding in 4-value form (e.g. padding: 48px 24px 96px 24px;) and verify heroPadding correctly extracts the 3rd value; or clarify the assumption in a code comment and log a risk that 4-value padding may mis-report. |
| 5 | 🟡 | Flat-rule CSS splitter has known gaps with nested/@media rulesevals/detector.mjs:42
|
Document the limitation explicitly in a code comment, or add a test with a fixture containing escaped braces or nested rules to verify the detector's behavior is acceptable for the current scope. |
| 6 | 🟡 | Variable resolution depth limit of 8 may be insufficient for real CSS chains.evals/detector.mjs:68
|
Test resolveVar with a fixture that chains 9+ levels of var() nesting to confirm whether depth-8 is sufficient, or raise it and document the rationale. |
| 7 | 🟡 | oklchL, oklchC, oklchH parsers do not handle whitespace or unit variants in OKLCH syntax.evals/detector.mjs:77
|
Test oklchL/oklchC/oklchH against realistic OKLCH syntax variants (percentage lightness, hue in deg/turn, chroma with optional units, and slash-separated alpha) to confirm the parsers handle them or to document their limitations. |
| 8 | 🟡 | v2-icon-tile-above-heading scans only 110 chars before heading; may miss icon tiles on long lines.evals/detector.mjs:605
|
Document why 110 chars was chosen (line length? typical wrapping?), or test with a fixture that has an icon >110 chars before a heading to confirm this is not a false-negative risk for real Hallmark output. |
| 9 | 🟡 | interaction-emoji-icon rule strips style and svg tags before checking, but does not account for escaped entities.evals/detector.mjs:436
|
Test the emoji rule with both raw emoji glyphs and entity-encoded variants to confirm which forms are caught. If both should be caught, decode entities before the check. |
| 10 | 🔵 | Redundant note in everything-in-cards rule output.evals/detector.mjs:630
|
Change to: note: n > 6 ? \${n} card-like wrappers (exceeds 6)` : `${n} card-like wrappers (OK)``. |
| 11 | 🔵 | Division by zero risk in audit-site corpus mean calculation.evals/audit-site.mjs:77
|
Guard the avg calculation: const avg = (k) => rows.length ? (rows.reduce(...) / rows.length).toFixed(2) : '—'; or add a check before calling avg() to ensure rows is not empty, with an early return or warning message. |
✅ What checks out
check.mjs correctly stays on the non-mutating path (it never calls writeSnapshot/rebuildHistory, unlike run.mjs), matching the PR's stated local/CI-safe invariant, and the committed cycle snapshots back up the 74.2→98.3→76.4→98.7 score trajectory described in the PR body.
| for (const fx of fixtures) { | ||
| const det = analyze(path.join(HERE, fx.file), evalVersion); | ||
| const judge = readJson(path.join(HERE, fx.judge)); | ||
| const craftVals = CRAFT_AXES.map((a) => judge[a]); |
There was a problem hiding this comment.
🟡 Consider Changing: Missing validation for judge sidecar craft axis values before averaging.
The code maps judge[a] for each craft axis name but never validates that these keys exist or have numeric values. If a judge.json sidecar is missing a craft axis (e.g., missing 'honesty'), parseFloat on undefined silently returns NaN, which corrupts the aggregate craft score calculation. Line 49–50 computes craft from potentially undefined values.
Recommendation: Before line 49, validate that all CRAFT_AXES exist in judge and are numbers: for (const a of CRAFT_AXES) { if (judge[a] == null || typeof judge[a] !== 'number') throw new Error(...); }
| const p = pad[1].trim().split(/\s+(?![^(]*\))/); | ||
| let top, bottom; | ||
| if (p.length === 1) { top = bottom = pxOf(p[0], map); } | ||
| else if (p.length === 2) { top = bottom = pxOf(p[0], map); } |
There was a problem hiding this comment.
🟡 Consider Changing: 2-value padding shorthand handling is correct but easy to misread
The 2-value branch sets top = bottom = pxOf(p[0]), which matches CSS's top/bottom-then-left/right shorthand and is correct. The terse assignment just reads like a possible bug. A short comment clarifying the mapping would remove the ambiguity.
Recommendation: Change line 556 to top = pxOf(p[0], map); bottom = pxOf(p[0], map); if intent is symmetric, or clarify with a comment. If the intent is to use p[0] for both, the code is correct but the logic flow is confusing—consider a comment explaining 2-value padding applies the same to top and bottom.
| id: 'v2-hero-metric-stat', dim: 'layout', | ||
| label: 'Hero metric layout (big number + supporting stats)', | ||
| fn: ({ rules, map }) => { | ||
| const bad = rules.find((r) => /(stat|metric|kpi|figure-n|big-?num)/.test(r.sel) && (() => { const m = r.body.match(/font-size\s*:\s*([^;]+)/i); const px = m ? pxOf(m[1], map) : null; return px != null && px >= 36; })()); |
There was a problem hiding this comment.
🟡 Consider Changing: Hero-metric rule buries its condition in a nested IIFE
The rule's predicate embeds a full inline function to check font-size, making the intent hard to follow at a glance. Extracting it to a small named helper would make the rule self-explanatory.
Recommendation: Extract the font-size check to a named helper function (e.g., hasBigFontSize(rule, map)) and call it from the predicate, or refactor the find logic to iterate and check both conditions in sequence for clarity.
| label: 'Wrapping all content in cards', | ||
| fn: ({ html }) => { | ||
| const n = (html.match(/class="[^"]*\b(card|panel|tile|box)\b/gi) || []).length; | ||
| return { pass: n <= 6, note: n > 6 ? `${n} card-like wrappers` : `${n} card-like wrappers` }; |
There was a problem hiding this comment.
🔵 Informational: Redundant note in everything-in-cards rule output.
Line 630 returns the same note text for both pass and fail cases: note: n > 6 ? \${n} card-like wrappers` : `${n} card-like wrappers``. The ternary does not branch the output; both branches produce identical text. This is confusing—a pass should have a different note (e.g., 'cards under threshold') than a fail.
Recommendation: Change to: note: n > 6 ? \${n} card-like wrappers (exceeds 6)` : `${n} card-like wrappers (OK)``.
| const tag = r.multiTheme ? ` [multi-theme:${r.themeCount}, low-confidence]` : ''; | ||
| console.log(`${name(r.page).padEnd(34)} ${r.v1.toFixed(2).padStart(6)} ${r.v2.toFixed(2).padStart(6)} ${f}${tag}`); | ||
| } | ||
| const avg = (k) => (rows.reduce((a, r) => a + r[k], 0) / rows.length).toFixed(2); |
There was a problem hiding this comment.
🔵 Informational: Division by zero risk in audit-site corpus mean calculation.
Line 77 defines avg = (k) => (rows.reduce(...) / rows.length). If no pages are passed as arguments, rows is empty, rows.length is 0, and the average function will return Infinity, then toFixed(2) will produce 'Infinity'. This causes misleading console output with no error.
Recommendation: Guard the avg calculation: const avg = (k) => rows.length ? (rows.reduce(...) / rows.length).toFixed(2) : '—'; or add a check before calling avg() to ensure rows is not empty, with an early return or warning message.
| }); | ||
| } | ||
|
|
||
| // oklch lightness 0..1 (handles "oklch(.3 ...)" and "oklch(32% ...)") |
There was a problem hiding this comment.
🟡 Consider Changing: oklchL, oklchC, oklchH parsers do not handle whitespace or unit variants in OKLCH syntax.
The OKLCH colour-space parsers use fixed regex patterns (e.g., oklch\(\s*([0-9.]+%?) that assume lightness is a number or percentage with no other units. Modern CSS allows units like oklch(32% none 0deg) or oklch(0.5 / 0.8), but the parsers will not extract values if the syntax deviates. No test verifies behaviour on real colour values that use modern OKLCH syntax (e.g., percentage lightness, hue in degrees/turns, chroma with units).
Recommendation: Test oklchL/oklchC/oklchH against realistic OKLCH syntax variants (percentage lightness, hue in deg/turn, chroma with optional units, and slash-separated alpha) to confirm the parsers handle them or to document their limitations.
| return (src.match(/macrostructure:\s*([a-z0-9-]+)/i) || [])[1] || 'unstamped'; | ||
| } | ||
|
|
||
| function structureScoreFor(fixtures) { |
There was a problem hiding this comment.
🔴 Must Fix: Structure score formula can't distinguish unstamped fixtures from duplicate macrostructures
The order-parameter formula subtracts 2.5 per collision and per unstamped fixture, both clamped to 0. All-unstamped and all-duplicate corpora land on the same floor score, and no test confirms this matches the intended design.
Recommendation: Add explicit tests or a test fixture set verifying: all-unstamped yields 0/5, all-duplicate yields 0/5, and 1 collision on 5 fixtures yields 2.5/5. Confirm the penalty ratio (1 collision = 1 duplicate, so 2 copies cost 2.5 points) matches the evaluation's intent.
| fn: ({ html }) => { | ||
| const heads = [...html.matchAll(/<h[2-4][\s>]/gi)]; | ||
| for (const h of heads) { | ||
| const before = html.slice(Math.max(0, h.index - 110), h.index); |
There was a problem hiding this comment.
🟡 Consider Changing: v2-icon-tile-above-heading scans only 110 chars before heading; may miss icon tiles on long lines.
The rule checks for icon tiles (SVG or icon class) in a 110-character window before <h2-4> tags. If a fixture uses long prose or multi-attribute elements, the icon tile could be >110 chars before the heading and escape detection. The constant is not justified or parameterized.
Recommendation: Document why 110 chars was chosen (line length? typical wrapping?), or test with a fixture that has an icon >110 chars before a heading to confirm this is not a false-negative risk for real Hallmark output.
| const rows = []; | ||
| let failed = false; | ||
|
|
||
| for (const evalVersion of evalVersions()) { |
There was a problem hiding this comment.
🔴 Must Fix: check.mjs has no graceful handling for missing or malformed fixture/judge files
evaluateCycle reads fixture HTML and judge JSON without catching file or parse errors, so a bad file throws an unhandled exception instead of a clear message identifying which file is broken.
Recommendation: Add error handling to evaluateCycle() to catch file-read or JSON-parse errors and report them clearly; or add a test fixture with a missing or corrupt judge sidecar to confirm the error message is helpful.
|
|
||
| // ---- INTERACTION ------------------------------------------------------ | ||
| { | ||
| id: 'interaction-emoji-icon', dim: 'interaction', |
There was a problem hiding this comment.
🟡 Consider Changing: interaction-emoji-icon rule strips style and svg tags before checking, but does not account for escaped entities.
The rule removes <style> and <svg> blocks from HTML before checking for emoji glyphs, to avoid false positives from CSS or SVG source. However, if an emoji is HTML-entity-encoded (e.g. 😀 for 😀), the regex will not match it. Depending on whether Hallmark emits entity-encoded emoji or raw glyphs, this could be a false negative.
Recommendation: Test the emoji rule with both raw emoji glyphs and entity-encoded variants to confirm which forms are caught. If both should be caught, decode entities before the check.
What
Adds an eval-driven quality harness for Hallmark outputs, plus the slop-test gates that came out of the eval hillclimb.
Included pieces:
evals/detector.mjsevals/core.mjsnode evals/check.mjsevals/run.mjsevals/audit-site.mjsevals/.site-cache/Why
Hallmark's promise is that generated UI should look made, not generated. The existing checklist is useful, but it is hard to know whether new guidance actually improves outputs without a repeatable measurement loop.
This PR adds that loop. v1 covers deterministic checks mapped to the existing anti-slop standard. v2 adds adversarial fixtures and a cross-fixture structure/order-parameter check so the eval does not only reward one-page cleanliness while missing template reuse across outputs.
How to read the eval
node evals/check.mjsis non-mutating and intended for local/CI regression checks.node evals/run.mjs ...is the mutating command that writes cycle snapshots/history.node evals/audit-site.mjs ...is heuristic; it inlines local linked CSS, follows local@imports, and writes ignored snapshots underevals/.site-cache/.Results
The committed history documents the hillclimb:
74.2 -> 98.398.3 -> 76.476.4 -> 98.7Current non-mutating check:
Testing
Risk
Mitigations: