From 525d200ae2dbae232f0803abde9920fc1b474105 Mon Sep 17 00:00:00 2001 From: Ruishuo Chen Date: Thu, 16 Jul 2026 13:43:58 +0800 Subject: [PATCH] fix(paper2poster): close five holes where the poster gates pass without checking Each of these is a gate that reports success on a poster it did not actually verify, or a breaker that counts the wrong thing. Found by diffing this skill's gates against posterly's, then reproducing each against the shipped templates. 1. The broken-image check skipped SVGs (polish.py, slack.py). Both gates skipped the zero-natural-size check when the src looked like an SVG, on the stated theory that "a vector image can legitimately report zero natural size while rendering fine". Chromium -- the only renderer these gates run in -- never does that: it resolves an 'd SVG with no width/height/viewBox to the CSS default replaced size, 300x150. Zero means a 404 or an unparseable file. So the exemption only ever spared genuinely broken vector art and let it white-print, which the old comment half-admitted ("an SVG behind an extensionless URL still slips through"). Dropped it; the check no longer guesses from the src string at all. 2. polish exited 2 on a template this skill ships (render.py, preflight.py). The class->role shim bailed entirely the moment ANY data-measure-role existed. assets/layouts_portrait/full.html declares exactly one (`column`, on .method-hero) and leaves poster/columns/ cards to classes, so the shim did nothing, poster+card resolved to zero, and cmd_polish hard-failed its own precondition -- while `slack --with-polish` silently skipped the polish pass. A declared role is ADDITIVE (slack's own selector is a union of `.col`, `.mid-wide` and `[data-measure-role="column"]`), so the shim now fills in any conventional-class element that lacks a role and never re-tags one that has it. 3. The gates trusted a static estimate of the role markup (render.py, polish.py, slack.py). has_required_roles_in_html reads the file text, so it can claim a card the browser does not render (an element matching a conventional class may already declare a different role). polish would then measure nothing, emit no warnings and PASS -- invisible under --json, which drops the "cards checked: 0" line. Added render.count_roles(), an exact count on the live page, and both entry points now fail closed when poster/card/column resolve to zero there. The static scan stays as the cheap pre-browser fail-fast. 4. The circuit breaker counted invocations, not failures (slack.py). It incremented a monotonic on-disk counter before the browser launched and never cleared it, so a converged poster inherited the debt of the rounds that converged it, and a run that never produced geometry (nav timeout, MathJax settle failure) burned budget it never used. It now counts CONSECUTIVE non-converged measurements: read up front, written only once the verdict is known, cleared on convergence, with a 12h staleness expiry (falling back to the file's mtime for pre-upgrade state, which is exactly where a large stale count sits) and atomic writes. Under `--with-polish --strict` the polish gates now join the convergence decision -- otherwise a poster that is FULL everywhere but keeps failing e.g. ORPHAN cleared its budget every round and the breaker could never bound that loop at all. Their output is buffered so the breaker can see the verdict first, then replayed on its original stream at the position it always occupied. 5. New: CARD/INNER-VOID (polish.py). Nothing saw a void in the MIDDLE of a card. CARD/TRAILING skips any card whose justify-content distributes space, and reads ~0 when a tail is anchored to the bottom; Gate C's SPACE-BETWEEN only looks between cards in a column. The shipped .scan-hero and .scan-banner fall straight through that seam. The new check is pure geometry (largest inter-child gap minus the stated row-gap) and fires only on cards that hold content against their bottom edge -- a bottom-reaching tail, or vertical space-* distribution -- so a plain top-packed card stays CARD/TRAILING's job. Threshold calibrated on a composed poster: heading rhythm peaks at 6% of card height (`.section h2` ships a real 36px margin), a genuine void runs 33-83%; 0.15 sits between with >2x headroom either way. Adds tests/ (55 tests). The DOM tests skip themselves without Playwright. --- .../skills/paper2poster/SKILL.md | 2 +- .../paper2poster/references/staged_fill.md | 2 +- .../paper2poster/scripts/check_poster.py | 32 +- .../paper2poster/scripts/utils/polish.py | 291 ++++++- .../paper2poster/scripts/utils/preflight.py | 57 +- .../paper2poster/scripts/utils/render.py | 86 +- .../paper2poster/scripts/utils/slack.py | 327 ++++++-- .../skills/paper2poster/tests/test_gates.py | 791 ++++++++++++++++++ 8 files changed, 1477 insertions(+), 111 deletions(-) create mode 100644 ResearchStudio-Reel/skills/paper2poster/tests/test_gates.py diff --git a/ResearchStudio-Reel/skills/paper2poster/SKILL.md b/ResearchStudio-Reel/skills/paper2poster/SKILL.md index 195ea89..3a13de5 100644 --- a/ResearchStudio-Reel/skills/paper2poster/SKILL.md +++ b/ResearchStudio-Reel/skills/paper2poster/SKILL.md @@ -347,7 +347,7 @@ python ~/.claude/skills/paper2poster/scripts/check_poster.py slack \ `--with-polish` runs the fill gate (`slack`) and the visual-polish gate (`FIG/NARROW` etc.) on **one** rendered page — a single browser launch instead of two — and under `--strict` **both** must pass, so this one command replaces the old separate `slack` + `polish` calls. Do not stop iterating while it exits non-zero. `--strict` is the same measurement you read each pass, but with a hard exit code — there is no "acceptable SPARSE" or "figure too tight to fix" escape. Keep applying the modification methods (and, for a stubborn figure, the column-width nudge / vertical-room methods in `references/staged_fill.md`) until the gate passes. -**Converge fast, and bound the loop (critical for smaller models).** The `slack` report gives each off-band section a precise **`needPx`** delta — e.g. `key-result SPARSE grow +50px [+18..+83]`. Edit *by that number* with a continuous CSS lever (`margin-bottom` / `.col` gap / figure `max-height`), don't guess with whole text lines and overshoot the 0.05-wide FULL band. Track recent measurements and switch levers the instant a section ping-pongs `SPARSE`↔`SPILLAGE`. And the loop is **bounded**: if both gates aren't green after **~12 rounds / ~20 min**, render the best-measured state, mark the stage **DEGRADED** with the residual off-band section ids, and move on — never grind indefinitely. This is **script-enforced**: `slack` counts every call in `/.fill_budget.json` and **exits 3 with a `CIRCUIT BREAKER` banner** once it passes `--max-iterations` (default **80**) — an on-disk cap that survives context compaction, so a lost round-count can't make you grind. Treat **exit 3 as a hard stop** (render best state, mark DEGRADED). The exit gate stays strict; only the iteration count is capped. Full rules: the **"Convergence protocol"** at the top of `references/staged_fill.md`. +**Converge fast, and bound the loop (critical for smaller models).** The `slack` report gives each off-band section a precise **`needPx`** delta — e.g. `key-result SPARSE grow +50px [+18..+83]`. Edit *by that number* with a continuous CSS lever (`margin-bottom` / `.col` gap / figure `max-height`), don't guess with whole text lines and overshoot the 0.05-wide FULL band. Track recent measurements and switch levers the instant a section ping-pongs `SPARSE`↔`SPILLAGE`. And the loop is **bounded**: if both gates aren't green after **~12 rounds / ~20 min**, render the best-measured state, mark the stage **DEGRADED** with the residual off-band section ids, and move on — never grind indefinitely. This is **script-enforced**: `slack` counts *consecutive non-converged* measurements in `/assets/meta/.fill_budget.json` and **exits 3 with a `CIRCUIT BREAKER` banner** once it passes `--max-iterations` (default **80**) — an on-disk cap that survives context compaction, so a lost round-count can't make you grind. A converged measurement clears the count, and a call that produced no geometry at all (nav timeout, MathJax settle failure) doesn't count against it. Treat **exit 3 as a hard stop** (render best state, mark DEGRADED). The exit gate stays strict; only the iteration count is capped. Full rules: the **"Convergence protocol"** at the top of `references/staged_fill.md`. **When the `.grow` section is persistently `EMPTY` or `SPARSE`** even after exhausting its own Additional/optional content, don't keep stretching that one section — instead **refine the content of the other (non-grow) sections in the same column**: lift their `Additional` paragraphs into the rendered card, promote bullets from concise to expanded form, or fold in a paper-specific custom section. The `.grow` section then absorbs the residual slack naturally instead of inflating a single card with filler. diff --git a/ResearchStudio-Reel/skills/paper2poster/references/staged_fill.md b/ResearchStudio-Reel/skills/paper2poster/references/staged_fill.md index 1f20bf5..2268b3a 100644 --- a/ResearchStudio-Reel/skills/paper2poster/references/staged_fill.md +++ b/ResearchStudio-Reel/skills/paper2poster/references/staged_fill.md @@ -146,7 +146,7 @@ Weaker / smaller models stall here, and the reason is structural: the FULL band - **Column-stack coupling (rigid `headline-numbers` ⟷ `.grow` absorber).** A landscape column often stacks the *rigid* `headline-numbers` card — its big `.hero-val` is a fixed-pt block (160pt) that pokes past the card padding when the column is tight — above `ablation` and a `.grow` `takeaway`. Micro-shrinking the hero pt (160→140→120…) is whack-a-mole: every change re-disturbs the column's other cards, so you ping-pong for dozens of rounds. Instead, **lock the rigid card ONCE**: give `headline-numbers` a `min-height` ≈ its natural hero height (so the grid can't crush it), drop the hero to a single fitting pt, then **never touch it again** — let the column's `.grow` section absorb all remaining slack. If it still paints past the card, end it with a plain `

` (a real caption/footnote) so a text bottom-margin seats the content inside. - **Column-width nudges are GLOBAL — set them once, never inside the fill loop.** Resizing one outer-grid column re-flows *every* column, so a "shift width col3→col2" fix throws col2 off and you ping-pong col2⟷col3 forever (this is a distinct oscillation source from the per-section levers). Pick the column widths at render time; inside the loop, balance a section ONLY with *in-column* continuous levers (margin / `min-height` / font-size / content), never by resizing the outer columns. -**5. Hard circuit breaker — the loop is bounded, and the script enforces it.** Aim to converge within **~12 measure→edit rounds OR ~20 minutes**. That is the *soft target*; the *hard backstop* is built into `check_poster.py slack` itself. Every `slack` call increments a persistent counter in `/.fill_budget.json` and, once it passes `--max-iterations` (**default 80**), `slack` prints a `CIRCUIT BREAKER` banner and **exits 3** instead of its normal verdict. The counter lives on disk, so it **survives context compaction** — you cannot reset it by losing your in-prompt round count (the exact failure mode that lets a loop grind to 100 measurements). When you see **exit 3 / the breaker banner**, stop immediately: **render the best-measured state** (fewest off-band sections / smallest total `|needPx|`), record the stage as **DEGRADED** with the residual off-band section ids in the stage note, and **move on**. A poster that is 95% there in 12 rounds beats one that burns 90 minutes chasing the last 1px. The exit gate itself stays strict (`--strict` is unchanged) — this breaker bounds only the *iteration count*, never the quality target. (For a genuine fresh re-render of the same `poster_dir`, pass `--reset-budget` once to zero the counter; set `--max-iterations 0` only to disable the backstop deliberately.) +**5. Hard circuit breaker — the loop is bounded, and the script enforces it.** Aim to converge within **~12 measure→edit rounds OR ~20 minutes**. That is the *soft target*; the *hard backstop* is built into `check_poster.py slack` itself. Every `slack` call that measures the poster and finds it **not yet converged** increments a persistent counter in `/assets/meta/.fill_budget.json` and, once it passes `--max-iterations` (**default 80**), `slack` prints a `CIRCUIT BREAKER` banner and **exits 3** instead of its normal verdict. The counter lives on disk, so it **survives context compaction** — you cannot reset it by losing your in-prompt round count (the exact failure mode that lets a loop grind to 100 measurements). It counts **consecutive** non-converged rounds: a measurement where every section is FULL and every figure OK clears it (and, under `--with-polish --strict`, only if the polish gates passed too — a poster that is FULL everywhere but keeps failing e.g. `ORPHAN` is still a loop the breaker must bound), so a later touch-up on an already-converged poster starts from a clean budget rather than inheriting the debt of the rounds that got it there. A call that never produced geometry (nav timeout, MathJax settle failure, no columns found) is an environment problem and does not count against you, and a counter left idle for over 12h is treated as a previous session and dropped. When you see **exit 3 / the breaker banner**, stop immediately: **render the best-measured state** (fewest off-band sections / smallest total `|needPx|`), record the stage as **DEGRADED** with the residual off-band section ids in the stage note, and **move on**. A poster that is 95% there in 12 rounds beats one that burns 90 minutes chasing the last 1px. The exit gate itself stays strict (`--strict` is unchanged) — this breaker bounds only the *iteration count*, never the quality target. (For a genuine fresh re-render of the same `poster_dir`, pass `--reset-budget` once to zero the counter; set `--max-iterations 0` only to disable the backstop deliberately.) **6. Context discipline — do NOT re-`Read` the whole `poster.html` each round.** It's ~100 KB (~25–30k tokens); pulling it into context every round floods a smaller model's window and triggers **auto-compaction**, which wipes your precise per-section fill state — so the loop forgets what it already tried and **thrashes without ever converging** (measured: a 200 K-window model auto-compacted twice and never converged on papers a large-window model finished compaction-free). Each round needs only the **`slack` report**, which is now self-sufficient: besides each section's verdict + `needPx`, it prints an **`EDIT TARGETS`** block carrying the **verbatim source of every off-band section**. Lift your `Edit` `old_string` straight from that block — a short, unique sub-snippet (a bullet's words, a stat value, a `margin-bottom`) — and grow/shrink the section by `needPx` without ever opening `poster.html`. If you somehow need markup the report didn't surface, `Read` a **narrow `offset`/`limit` slice** or `grep` for it — never the whole file. Beyond the one initial orientation read, re-reading the full `poster.html` is the single biggest avoidable context cost in this loop; on a small-context model it is the difference between converging and thrashing. diff --git a/ResearchStudio-Reel/skills/paper2poster/scripts/check_poster.py b/ResearchStudio-Reel/skills/paper2poster/scripts/check_poster.py index 4095c71..cfbce39 100755 --- a/ResearchStudio-Reel/skills/paper2poster/scripts/check_poster.py +++ b/ResearchStudio-Reel/skills/paper2poster/scripts/check_poster.py @@ -94,15 +94,19 @@ def build_parser() -> argparse.ArgumentParser: ) ps.add_argument( "--max-iterations", type=int, default=80, - help="script-enforced circuit breaker: persist a per-poster " - "measurement count in /.fill_budget.json; once it " - "exceeds this cap, slack prints a STOP banner and exits 3 " - "(survives context compaction, unlike an in-prompt round count). " - "Default 80. Set <=0 to disable.", + help="script-enforced circuit breaker: persist a per-poster count of " + "CONSECUTIVE non-converged measurements in " + "/assets/meta/.fill_budget.json; once it exceeds this " + "cap, slack " + "prints a STOP banner and exits 3 (survives context compaction, " + "unlike an in-prompt round count). A converged measurement clears " + "the count, a run that produced no geometry (nav timeout, MathJax " + "settle failure, no columns found) does not count, and a state " + "file idle >12h is dropped. Default 80. Set <=0 to disable.", ) ps.add_argument( "--reset-budget", action="store_true", - help="zero the persistent .fill_budget.json counter before measuring " + help="clear the persistent .fill_budget.json counter before measuring " "(use for a genuine fresh re-render of the same poster_dir).", ) ps.add_argument( @@ -201,6 +205,22 @@ def build_parser() -> argparse.ArgumentParser: "to 0.05 to match the slack gate's 5pt FULL band — anything " "looser hides 5-9%% trailing voids the eye still sees", ) + ppl.add_argument( + "--max-card-inner-void", type=float, + default=_polish.DEFAULT_MAX_CARD_INNER_VOID, + help="warn (CARD/INNER-VOID) if a card has an inter-child gap " + "exceeding this fraction of its height " + "(default 0.15); catches a void in the MIDDLE of a card, " + "which CARD/TRAILING cannot see because a bottom-pinned " + "tail drives trailing to ~0", + ) + ppl.add_argument( + "--min-card-inner-void-px", type=float, + default=_polish.DEFAULT_MIN_CARD_INNER_VOID_PX, + help="CARD/INNER-VOID also requires the gap to exceed this many " + "px (default 24) so a sub-line gap on a small card cannot " + "trip the gate", + ) ppl.add_argument( "--max-widow-fraction", type=float, default=_polish.DEFAULT_MAX_WIDOW_FRACTION, diff --git a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/polish.py b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/polish.py index ae19985..4d3bce7 100644 --- a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/polish.py +++ b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/polish.py @@ -20,11 +20,23 @@ on a column with one short card produces a giant whitespace gap that reads as "this column ran out of things to say". Detected when the largest inter-card gap exceeds the column's stated - ``row-gap`` by > 5% of column height. + ``row-gap`` by > 5% of column height. Two card-level siblings catch + the same void inside a single card: ``CARD/TRAILING`` (blank BELOW + the last line of a stretched card) and ``CARD/INNER-VOID`` (an + oversized gap BETWEEN a card's stacked children -- either the blocks + above stop early while the last one reaches the card's content + bottom, or the card deals its leftover space out between them via a + vertical ``justify-content: space-*``). The latter covers what the + former structurally cannot: a bottom-anchored tail drives trailing + to ~0, and ``CARD/TRAILING`` skips space-distributing cards outright. Neither sees a + ``justify-content: center`` card, whose voids sit above and below its + children rather than between them. -Warns by default; ``--strict`` to exit non-zero. Hard-fails if the -poster has no ``[data-measure-role]`` markup at all — a polish PASS on -"0 figures, 0 columns, 0 stat elements" would be misleading. +Warns by default; ``--strict`` to exit non-zero. Hard-fails when the +poster resolves no ``poster`` / ``column`` / ``card`` roles — first from +a cheap static scan before the browser opens, then against the LIVE page, +since a PASS over "0 figures, 0 columns, 0 cards" would be a silent green +on a poster nobody checked. """ from __future__ import annotations @@ -51,6 +63,35 @@ DEFAULT_MAX_CARD_TRAILING = 0.05 DEFAULT_MAX_WIDOW_FRACTION = 0.20 +# Gate C (inner void): an oversized gap BETWEEN a card's stacked children -- +# below its last real block and above one pushed down by `margin-top: auto` +# or by a vertical `justify-content: space-*`. CARD/TRAILING cannot see it -- +# it measures blank space below the last content, which a bottom-pinned tail +# drives to ~0, and it skips space-distributing cards outright. Gate C's +# SPACE-BETWEEN only looks BETWEEN cards in a column. Detected by geometry +# (largest inter-child vertical gap minus the card's stated row-gap) so the +# mechanism does not matter. NOT covered: a `justify-content: center` card, +# whose voids sit above and below its children rather than between them (see +# the SCOPE note in _POLISH_JS). +# +# Calibration: the gap is measured between bounding boxes, so it INCLUDES the +# siblings' ordinary margins -- `.section h2 { margin: 0 0 0.45em }` at 60pt +# is a real 36px gap under every heading. Those cannot simply be subtracted: +# getComputedStyle resolves `margin-top: auto` to its USED value (the void +# itself), so netting margins out would zero the exact signal this gate +# exists for. Two conditions separate rhythm from void instead: +# 1. the card must either have a bottom-anchored tail (`tailPinned` in +# _POLISH_JS) or distribute space vertically (a `space-*` card reserves +# room at the ends, so nothing reaches the bottom by design) -- a plain +# top-packed card is CARD/TRAILING's job, not this gate's; +# 2. the gap must exceed this fraction of card height. Measured on the +# shipped templates, heading rhythm peaks at ~6% of card height (36px on +# the shortest ~600px card) while a real void runs 33-83%. 0.15 sits with +# >2x headroom on both sides. +# The px floor additionally keeps a sub-line gap on a small card quiet. +DEFAULT_MAX_CARD_INNER_VOID = 0.15 +DEFAULT_MIN_CARD_INNER_VOID_PX = 24.0 + # Trailing glyphs that orphan when wrapped: arrows, multiplicative # cross, division, plus-minus, footnote markers, degree, percent. @@ -266,6 +307,130 @@ }); }); + // ---- 4b) Inner-card void: oversized gap BETWEEN a card's stacked + // children ---- + // Gate C's SPACE-BETWEEN catches a gap BETWEEN cards in a column, and + // CARD/TRAILING catches blank BELOW a card's last line. Neither sees a + // void in the MIDDLE of a card: a card stretched by an equal-height row + // whose short content is top-packed with its tail pinned to the bottom + // (`margin-top: auto`, or `justify-content: space-*`) opens the slack + // between them, so trailing reads ~0 -- and CARD/TRAILING skips + // space-distributing cards by design, leaving them with no void check at + // all. This scan is pure geometry (largest inter-child vertical gap minus + // the stated row-gap) so the mechanism does not matter. + // + // SCOPE: gaps BETWEEN direct children only. A `justify-content: center` + // card whose short content floats with equal voids ABOVE and BELOW it has + // no oversized inter-child gap, so this gate does not see it either (and + // CARD/TRAILING skips it too) -- that hole is still open. A void nested + // inside a single wrapper child is likewise not seen. + const innerVoids = []; + document.querySelectorAll('[data-measure-role="card"]') + .forEach(card => { + const cs = window.getComputedStyle(card); + const cr = card.getBoundingClientRect(); + if (cr.height <= 0) return; + const gap = parseFloat(cs.rowGap || cs.gap || '0') || 0; + const contentBottom = cr.bottom + - (parseFloat(cs.paddingBottom) || 0) + - (parseFloat(cs.borderBottomWidth) || 0); + // Direct, in-flow element children with a real box. Skip abs/fixed -- + // a corner badge / floating Listen button is not flow content. Use + // getAttribute('class'): an SVG child's `.className` is an + // SVGAnimatedString and `.trim()` on it throws, which would take down + // the whole evaluate. + const kids = Array.from(card.children).map(c => { + const r = c.getBoundingClientRect(); + const ccs = window.getComputedStyle(c); + const cl = ((c.getAttribute('class') || '').trim() + .split(/\s+/)[0]) || ''; + return {tag: c.tagName.toLowerCase(), cls: cl, + top: r.top, bottom: r.bottom, h: r.height, + pos: ccs.position, + mb: parseFloat(ccs.marginBottom) || 0}; + }).filter(c => c.h > 0 && c.pos !== 'absolute' && c.pos !== 'fixed') + .sort((a, b) => a.top - b.top); + if (kids.length < 2) return; + // Does this card hold its content against its bottom edge at all? Only + // two shapes qualify, and a card must be one of them to be measured + // here: its tail is anchored to the content bottom (`tailPinned`), or + // it deals space out vertically (`distributes` below), which reserves + // room at the ends so nothing reaches the bottom by design. + // + // Everything else is a plain top-packed card whose content stops early + // -- CARD/TRAILING's territory, not this gate's. Excluding it is what + // makes the two gates partition the space instead of double-reporting, + // and it is what keeps an ordinary heading's 0.45em margin from being + // read as a "void". Note `tailPinned` establishes only that the last + // child REACHES the content bottom, never what put it there, so the + // warning reports that geometry rather than naming a mechanism. + // + // Compare MARGIN-box bottoms: `margin-top: auto` pushes the item's + // margin box against the container's content edge, so a tail carrying + // the templates' own `.section p { margin: 0 0 0.5em }` sits its + // BORDER box ~27px short of it and a border-box test misses the pin by + // a hair (measured: 27.0 vs a 26.9 tolerance on a 1347px card). + // + // `space-around` / `space-evenly` are exempt from the test: they + // deliberately reserve space at BOTH ends, so nothing is flush with the + // bottom and the pin check would reject them -- and CARD/TRAILING skips + // every `space-*` card, so requiring a pin here would leave them with no + // void check at all (the exact hole this gate was added to close). Their + // declared intent to distribute space IS the signal; the size of the + // resulting interior gap is what's in question. + // + // Only when it distributes on the VERTICAL axis, though. `justify-content` + // is inert on a block card and runs horizontally on a grid or a + // `flex-direction: row` card, yet computed style reports the declared + // value regardless -- so testing the string alone would waive the pin + // check for cards that distribute nothing downward, re-admitting the + // short-card heading false positive the pin check exists to stop. + const jc = cs.justifyContent || ''; + const disp = cs.display || ''; + const fdir = cs.flexDirection || ''; + const distributes = + jc.indexOf('space-') !== -1 + && (disp === 'flex' || disp === 'inline-flex') + && (fdir === 'column' || fdir === 'column-reverse'); + const lastBottom = Math.max.apply(null, kids.map(k => k.bottom + k.mb)); + const tailPinned = + (contentBottom - lastBottom) <= Math.max(2, 0.02 * cr.height); + if (!tailPinned && !distributes) return; + // Merge same-row children: walk in top order tracking the running MAX + // bottom seen so far, and count a gap only when a child STARTS below + // that max. A side-by-side row (figure beside text) is dominated by + // its tallest member, so a following block that clears the tall one is + // NOT a void -- this avoids measuring `next.top - shortSibling.bottom` + // across an already-filled row. + // NOTE: only DIRECT children are inspected; a void nested inside a + // single wrapper is not seen. + let rowMaxBottom = kids[0].bottom; + let rowMaxIdx = 0; + let maxGap = 0, pairBelow = -1, pairAbove = -1; + for (let i = 1; i < kids.length; i++) { + const g = kids[i].top - rowMaxBottom; + if (g > maxGap) { maxGap = g; pairBelow = i; pairAbove = rowMaxIdx; } + if (kids[i].bottom > rowMaxBottom) { + rowMaxBottom = kids[i].bottom; + rowMaxIdx = i; + } + } + const lab = k => k.tag + (k.cls ? '.' + k.cls : ''); + innerVoids.push({ + cls: (card.getAttribute('class') || ''), + section: card.getAttribute('data-section') || '', + card_h: cr.height, + stated_gap: gap, + excess: maxGap - gap, + above: pairAbove >= 0 ? lab(kids[pairAbove]) : '', + below: pairBelow > 0 ? lab(kids[pairBelow]) : '', + // Which shape this is, so the warning can describe the card it + // actually found instead of asserting a bottom-pinned tail that a + // space-around / space-evenly card does not have. + distributes: distributes ? jc : '', + }); + }); + // ---- 5)
as a direct child of a flex container ---- // A
that is an in-flow child of display:flex|inline-flex is // blockified into a flex ITEM and stops creating a line break -- so @@ -340,7 +505,7 @@ }); }); - return {figures, orphans, cols, cards, flexbr, widows}; + return {figures, orphans, cols, cards, innerVoids, flexbr, widows}; } """ @@ -357,8 +522,8 @@ def collect_polish_data(page) -> dict: """ injected = _render.inject_class_fallback_roles(page) if injected: - print("[polish] no data-measure-role found; " - "using class fallback (.poster/.col/.section)") + print("[polish] filled in missing data-measure-role attributes " + "from the class fallback (.poster/.col/.section)") data = page.evaluate(_POLISH_JS) # ---- Gate F: mid-wide structural integrity ---- # The merged-middle layout requires Method as the only direct .section @@ -420,6 +585,8 @@ def default_polish_args() -> argparse.Namespace: fig_min_ratio=DEFAULT_FIG_MIN_RATIO, max_space_between_fill=DEFAULT_MAX_SPACE_BETWEEN_FILL, max_card_trailing=DEFAULT_MAX_CARD_TRAILING, + max_card_inner_void=DEFAULT_MAX_CARD_INNER_VOID, + min_card_inner_void_px=DEFAULT_MIN_CARD_INNER_VOID_PX, max_widow_fraction=DEFAULT_MAX_WIDOW_FRACTION, strict=False, ) @@ -499,8 +666,33 @@ def cmd_polish(args: argparse.Namespace) -> int: return 1 collected = collect_polish_data(page) + # The disk precheck above only reads the file text -- it cannot tell + # which class-matched element already carries a role, so a poster + # whose `.section`s all declare some other role reads as "has cards" + # on disk and yields zero cards here. Without this the gates would + # measure nothing, emit no warnings, and PASS: a silent green on a + # poster nobody checked. The browser is already open, so ask it. + live_roles = _render.count_roles(page) browser.close() + # Fail closed on any missing required role. Neither "nothing resolved" + # nor "the query failed" is a basis for passing, and they are not + # distinguishable here -- guarding this on `and live_roles` (to treat an + # empty map as merely unknown) let a poster whose elements carry EMPTY + # role attributes sail through with every gate array empty. + live_missing = [r for r in ("poster", "card", "column") + if live_roles.get(r, 0) == 0] + if live_missing: + _eprint( + f"ERROR: the rendered poster resolves no {live_missing} " + f"element(s), so the polish gates have nothing to measure " + f"(the on-disk scan expected them -- an element matching a " + f"conventional class most likely declares a different " + f"data-measure-role). Fix the markup rather than trusting a " + f"PASS from this run." + ) + return 2 + return report_polish(collected, args, html_path) @@ -523,23 +715,24 @@ def report_polish(collected: dict, args: argparse.Namespace, nw = float(f["natural_w"]) nh = float(f["natural_h"]) role = f.get("role", "card") - src_l = str(f["src"]).lower() - # A vector image (SVG) can legitimately report zero natural size - # while rendering fine, so never flag it broken. Match the path - # extension (after stripping any ?query / #fragment) plus inline - # SVG data URIs. Imperfect: an SVG behind an extensionless URL - # still slips through; an `img.decode()`-based JS probe would be - # exact. Covers both card and hero (see _POLISH_JS). - src_path = src_l.split("?", 1)[0].split("#", 1)[0] - is_svg = ( - src_path.endswith((".svg", ".svgz")) - or src_l.startswith("data:image/svg") - ) - if (nw <= 0 or nh <= 0) and not is_svg: + # Zero natural size means the image FAILED TO LOAD -- including for + # SVG. This used to carry an `is_svg` exemption (by src extension / + # data-URI prefix) on the theory that "a vector image can legitimately + # report zero natural size while rendering fine". That is not true in + # the only renderer these gates ever run in: Chromium resolves an + # 'd SVG with no width/height/viewBox to the CSS default replaced + # size, 300x150 -- it never reports 0. What DOES report 0 is a 404 or + # an unparseable file. So the exemption only ever fired on genuinely + # broken vector art, and silently white-printed it -- the blind spot + # the old comment half-admitted ("an SVG behind an extensionless URL + # still slips through"). Dropping the exemption closes it in both + # directions and removes the src-string guessing entirely. + if nw <= 0 or nh <= 0: warns.append( f"FIG/BROKEN: '{ascii_safe(f['src'])}' has zero natural " - "size -- the image failed to load (missing file, 404, or " - "an unreachable remote URL); it will be blank in print." + "size -- the image failed to load (missing file, 404, an " + "unreachable remote URL, or an unparseable SVG); it will be " + "blank in print." ) continue # Hero figures get the broken-image check above, but the AR sizing @@ -583,8 +776,10 @@ def report_polish(collected: dict, args: argparse.Namespace, f"figure cap rules in references/visual_polish.md)." ) continue - if cw <= 0 or rw <= 0 or nw <= 0 or nh <= 0: + if cw <= 0 or rw <= 0: continue + # nw / nh are guaranteed positive here: the FIG/BROKEN check above now + # catches every zero-natural image (SVG included) and `continue`s. ar = nw / nh ratio = rw / cw # Unified fill floor: every card figure should paint at 90-100% of its @@ -698,6 +893,55 @@ def report_polish(collected: dict, args: argparse.Namespace, f"See Gate C in SKILL.md." ) + # ---- Gate C (one card): mid-card void between two stacked children ---- + iv_ratio = getattr(args, "max_card_inner_void", + DEFAULT_MAX_CARD_INNER_VOID) + iv_floor = getattr(args, "min_card_inner_void_px", + DEFAULT_MIN_CARD_INNER_VOID_PX) + for c in data.get("innerVoids", []): + ch = float(c["card_h"]) + excess = float(c["excess"]) + if ch <= 0 or excess <= iv_floor: + continue + if excess / ch <= iv_ratio: + continue + between = "" + if c.get("above") and c.get("below"): + between = (f" between <{ascii_safe(c['above'])}> and " + f"<{ascii_safe(c['below'])}>") + who = ascii_safe(str(c.get("section") or c.get("cls") or "?")) + jc = str(c.get("distributes") or "") + if jc: + cause = ( + f"the card sets `justify-content: {ascii_safe(jc)}`, so the " + f"space left over after its content is dealt out BETWEEN the " + f"children as visible holes" + ) + else: + # Say only what was measured. `tailPinned` proves the last child + # reaches the content bottom -- NOT that `margin-top: auto` put it + # there. Grid track stretch, a growable last child, or content that + # simply happens to fit all satisfy it, and naming a mechanism the + # card may not use sends the reader hunting for CSS that isn't + # there. + cause = ( + "the card's last child reaches its content bottom while the " + "blocks above it stop early, so the leftover space opens in " + "between -- typically an equal-height row of cards with " + "unequal content, where the short one's tail is pushed down " + "by `margin-top: auto` or by a stretched track" + ) + warns.append( + f"CARD/INNER-VOID: section '{who}' has a {excess:.0f} px gap " + f"({excess / ch * 100:.0f}% of card height, stated row-gap " + f"{c['stated_gap']:.0f} px){between} -- a void in the MIDDLE of " + f"the card: {cause}. CARD/TRAILING cannot see it (it skips " + f"space-distributing cards, and a pinned tail drives trailing to " + f"~0). Fill the card with real substance, or drop the " + f"bottom-pin / space-* distribution so it hugs its content. " + f"See Gate C in SKILL.md." + ) + # ---- Gate D:
inside a flex container ---- # A
that is a direct child of a flex container is blockified into # a flex item and creates NO line break, so intended multi-line text @@ -810,6 +1054,7 @@ def report_polish(collected: dict, args: argparse.Namespace, print(f" stat-like elements : {len(data.get('orphans', []))}") print(f" space-between cols : {len(data.get('cols', []))}") print(f" cards checked : {len(data.get('cards', []))}") + print(f" inner-void cards : {len(data.get('innerVoids', []))}") print(f" flex/
parents : {len(data.get('flexbr', []))}") print(f" widow candidates : {len(data.get('widows', []))}") print(f" warnings : {len(warns)}") diff --git a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/preflight.py b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/preflight.py index 0c496f9..bca9731 100644 --- a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/preflight.py +++ b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/preflight.py @@ -19,6 +19,7 @@ import argparse import re +from collections.abc import Callable from pathlib import Path from urllib.parse import unquote, urlsplit @@ -314,36 +315,62 @@ def has_required_roles_in_html(html_path: Path) -> dict[str, int]: instead of silently PASSing on "0 figures, 0 columns, 0 stat elements". - Compat fallback for paper2poster-style templates: if the file has NO - ``data-measure-role`` attributes at all, count the conventional CSS - classes that the runtime shim (``inject_class_fallback_roles``) will - map to roles. Without this, polish hard-fails on disk before the + Compat fallback for paper2poster-style templates: add the conventional + CSS classes that the runtime shim (``inject_class_fallback_roles``) + maps to each role. Without this, polish hard-fails on disk before the browser ever opens, even though the runtime would have populated the roles fine. + + The fallback is ADDITIVE per role, mirroring the runtime shim: an + explicit role does not turn the shim off for that role (in + ``assets/layouts_portrait/full.html`` the single declared + ``column`` on ``.method-hero`` sits alongside ordinary ``.col`` + columns, and both are measured). Bailing out on the FIRST attribute + seen reported ``poster=0, card=0`` for that template, which tripped + ``cmd_polish``'s "missing markup" hard-fail on a poster the runtime + would have measured fine. + + This is a coarse ESTIMATE, not a prediction of the runtime. Against the + lowercase, quoted markup this skill's templates emit it over-counts (a + regex cannot tell which class-matched element already carries a role, so + an element with both is counted twice), which is the harmless direction + for its consumer: ``cmd_polish``'s pre-browser "is this file measurable + at all?" check tests for zero, so erring high keeps it from refusing a + poster it could measure. It is NOT an upper bound for arbitrary HTML -- + a browser accepts markup this regex does not match (``CLASS=section``, + unquoted attribute values), where it would under-count instead. + + Either way it is only a fail-fast. Anything needing to know what the + poster ACTUALLY renders must ask the live page (``render.count_roles``), + which both ``cmd_polish`` and ``slack --with-polish`` now do before + trusting a PASS. """ raw = html_path.read_text(encoding="utf-8", errors="ignore") body = strip_for_lint(raw) counts: dict[str, int] = {role: 0 for role in KNOWN_ROLES} - found_any = False for m in re.finditer( r'data-measure-role\s*=\s*["\']([^"\']+)["\']', body ): - found_any = True role = m.group(1).strip() if role in counts: counts[role] += 1 - if found_any: - return counts - # Class-based fallback (paper2poster templates). + + # Class-based fallback (paper2poster templates). Keep the selectors in + # step with `render._COMPAT_ROLES_JS` -- if the two disagree, this + # static pre-check and the runtime shim disagree about the same file. def _count_class(name: str) -> int: return len(re.findall( r'class\s*=\s*["\'][^"\']*\b' + re.escape(name) + r'\b[^"\']*["\']', body, )) - counts["poster"] = _count_class("poster") - counts["column"] = _count_class("col") - counts["card"] = _count_class("section") - counts["banner"] = _count_class("titlebar") + _count_class("banner") - counts["footer-strip"] = _count_class("footer-strip") - counts["footer"] = _count_class("footer") + fallbacks: dict[str, Callable[[], int]] = { + "poster": lambda: _count_class("poster"), + "column": lambda: _count_class("col"), + "card": lambda: _count_class("section"), + "banner": lambda: _count_class("titlebar") + _count_class("banner"), + "footer-strip": lambda: _count_class("footer-strip"), + "footer": lambda: _count_class("footer"), + } + for role, count_fn in fallbacks.items(): + counts[role] += count_fn() return counts diff --git a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/render.py b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/render.py index 1165d06..2d55cc8 100644 --- a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/render.py +++ b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/render.py @@ -139,21 +139,42 @@ class SettleResult: the ``$…$`` is most likely prose, not math).""" -# JS shim: when the poster uses paper2poster's conventional class names +# JS shim: where the poster uses paper2poster's conventional class names # (.poster, .col, .section, .titlebar/.banner, .footer-strip/.footer) -# but lacks any `data-measure-role` attributes, copy the role over from -# the class so `slack`/`polish` can still reason about it. No-op when -# any `data-measure-role` already exists on the page — explicit markup -# wins. Paper2layout-style cards are `.section`, not `.card`; we map -# both. Headlines/titlebar map to `banner`/`header` so the alignment -# spread isn't anchored by the title strip. +# but the element carries no `data-measure-role`, copy the role over from +# the class so `slack`/`polish` can reason about it. Explicit markup wins +# PER ELEMENT: an element that declares a role is never re-tagged, but a +# declared role elsewhere does not switch the shim off (see the comment +# in _COMPAT_ROLES_JS). Paper2layout-style cards are `.section`, not +# `.card`; we map both. Headlines/titlebar map to `banner`/`header` so +# the alignment spread isn't anchored by the title strip. _COMPAT_ROLES_JS = r""" () => { - if (document.querySelector('[data-measure-role]')) return false; + // Fill in a role for every conventional-class element that does not + // already declare one. This used to bail entirely the moment ANY + // data-measure-role existed anywhere in the document, which broke on + // `assets/layouts_portrait/full.html`: it declares exactly one + // (`column`, on .method-hero) and leaves poster/columns/cards to + // classes, so the shim did nothing, poster+card resolved to zero, and + // `polish` hard-failed its "poster/columns/cards required" precondition + // on a template this skill ships (while `slack --with-polish` silently + // skipped the polish pass). + // + // An explicit role is ADDITIVE, not exclusive -- this mirrors slack's own + // selector (`.columns > .col, .columns > .mid-wide, + // [data-measure-role="column"]`), where .method-hero's declared role adds + // a column that no class matches, rather than declaring itself the only + // one. So do NOT skip a role just because some element already has it: + // that would leave portrait's ordinary `.col`s unmeasured by every gate + // that keys off the role (Gate C's space-between scan, for one). The + // per-element `hasAttribute` guard below is what protects author intent: + // an element that declares a role is never re-tagged. + let injected = false; const set = (sel, role) => { document.querySelectorAll(sel).forEach(el => { if (!el.hasAttribute('data-measure-role')) { el.setAttribute('data-measure-role', role); + injected = true; } }); }; @@ -164,20 +185,63 @@ class SettleResult: set('.col > .section, .section', 'card'); set('.footer-strip', 'footer-strip'); set('footer, .footer', 'footer'); - return true; + return injected; } """ def inject_class_fallback_roles(page) -> bool: - """Add `data-measure-role` attributes based on conventional class - names when the page has none. Returns True if any were injected.""" + """Add `data-measure-role` attributes, from conventional class names, + to elements that do not already declare one. Additive: a document that + declares some roles still gets the shim for the elements it left + untagged. Returns True if any were injected.""" try: return bool(page.evaluate(_COMPAT_ROLES_JS)) except Exception: return False +def count_roles(page) -> dict[str, int]: + """Exact per-role element counts on the LIVE page, after the shim has run. + + ``preflight.has_required_roles_in_html`` only estimates from the file + text, so it is only safe as a cheap pre-browser fail-fast. Once the + browser is open there is no reason to guess: a gate that needs to know + whether the poster really has cards should ask the DOM. + + Counts are keyed on the RAW attribute value, so they mean exactly what + the gates' `[data-measure-role="…"]` selectors mean -- see the note in the + query below. + + A required role is ABSENT whenever its key is missing -- which covers + both a poster that declares nothing and one whose values do not match + (an empty attribute keys ``""``, a padded one keys ``" card "``). + ``{}`` additionally means the query itself failed. Callers must FAIL + CLOSED on all of these -- a gate that cannot confirm the poster has + anything to measure has no basis for passing it, and conflating + "unknown" with "fine" is how an unmeasured poster earns a silent green. + """ + try: + return page.evaluate( + """() => { + // Key on the RAW attribute value -- no trim, no filtering. + // Every gate selects with an exact attribute match + // (`[data-measure-role="card"]`), which does NOT match + // ` card `. Normalising here would report a role the gates + // then fail to select, which is the fail-open this function + // exists to prevent: counts must mean what the selectors mean. + const o = {}; + document.querySelectorAll('[data-measure-role]').forEach(e => { + const r = e.getAttribute('data-measure-role') || ''; + o[r] = (o[r] || 0) + 1; + }); + return o; + }""" + ) or {} + except Exception: + return {} + + def open_print_emulated_page(p, viewport_px: tuple[int, int]): """Launch headless Chromium, open a context+page at the viewport, emulate print media. Returns ``(browser, ctx, page)``. diff --git a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/slack.py b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/slack.py index be4e0cf..d1ec5f6 100644 --- a/ResearchStudio-Reel/skills/paper2poster/scripts/utils/slack.py +++ b/ResearchStudio-Reel/skills/paper2poster/scripts/utils/slack.py @@ -88,10 +88,12 @@ from __future__ import annotations import argparse +import datetime as _dt import json import os import re import sys +import tempfile from pathlib import Path from typing import Any @@ -542,6 +544,95 @@ def _section_source_blocks( return out +#: A ``.fill_budget.json`` untouched for this long describes a PREVIOUS +#: working session, not the current loop, so its count is dropped. The +#: batch pipeline renders a poster in one sitting; a human re-opening the +#: same run_dir the next day should not inherit yesterday's debt. +BUDGET_STALE_AFTER_HOURS = 12.0 + + +def _load_budget(path: Path) -> int: + """Consecutive non-converged measurements recorded for this poster. + + Any unreadable / implausible / stale state degrades to ``0`` -- the + breaker must never invent a count that stops a legitimate loop, and + budget I/O must never raise into the fill gate. + """ + try: + data = json.loads(path.read_text(encoding="utf-8")) + count = int(data["count"]) + except (OSError, ValueError, TypeError, KeyError): + return 0 + if count < 0: + return 0 + updated = data.get("updated") + if updated is None: + # State written before this field existed. Fall back to the file's + # mtime rather than honouring the count forever: the pre-upgrade + # format is exactly where a large stale count is most likely to be + # sitting (the old counter never reset), and a months-old 80 would + # otherwise trip the breaker on the very first measurement after the + # upgrade. An in-flight legacy loop still keeps its count -- its file + # was touched minutes ago. + try: + ts = _dt.datetime.fromtimestamp(path.stat().st_mtime, + _dt.timezone.utc) + except OSError: + return 0 + else: + try: + ts = _dt.datetime.fromisoformat(str(updated)) + except (ValueError, TypeError): + return 0 + if ts.tzinfo is None: + ts = ts.replace(tzinfo=_dt.timezone.utc) + now = _dt.datetime.now(_dt.timezone.utc) + if ts > now + _dt.timedelta(minutes=5): + return 0 # clock skew, or a run_dir copied from another host + if (now - ts) > _dt.timedelta(hours=BUDGET_STALE_AFTER_HOURS): + return 0 # previous session + return count + + +def _write_budget(path: Path, count: int) -> None: + """Atomically persist ``count``. Never raises: a poster on a + read-only mount loses the breaker, not the gate verdict.""" + payload = json.dumps({ + "count": count, + "updated": _dt.datetime.now(_dt.timezone.utc) + .replace(microsecond=0).isoformat(), + }) + try: + fd, tmp = tempfile.mkstemp(prefix=path.name + ".", dir=str(path.parent)) + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(payload) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + except OSError: + pass + + +def _clear_budget(path: Path) -> None: + """Drop the state on a converged measurement or ``--reset-budget``. + + A count that survived a PASS would break the CONSECUTIVE-failure + contract on the next run, so fall back to zeroing the file in place + when it cannot be removed. + """ + try: + path.unlink() + except FileNotFoundError: + pass + except OSError: + _write_budget(path, 0) + + def cmd_slack(args: argparse.Namespace) -> int: pw = import_playwright() if pw is None: @@ -560,27 +651,28 @@ def cmd_slack(args: argparse.Namespace) -> int: # instruction) while keeping the deliverable top level clean. A fresh # run_dir starts at 0 automatically; --reset-budget resets it for a genuine # re-render. max-iterations <= 0 disables the breaker entirely. + # + # What is counted: CONSECUTIVE NON-CONVERGED measurements. The count is + # read here but only written once the outcome is known (see the + # "Circuit-breaker accounting" block after the verdict is computed), which + # gives three properties a straight "+1 per invocation at the top" lacked: + # - a converged measurement CLEARS the count, so the touch-up round after + # a poster already reached FULL cannot trip a breaker inherited from + # the rounds it took to get there; + # - a run that never produced geometry (nav timeout, MathJax settle + # failure, no columns found) is an environment problem and burns no + # budget -- it used to, because the increment landed before the page + # was even opened; + # - a state file older than BUDGET_STALE_AFTER_HOURS is a previous + # session and is dropped. _budget_meta = html_path.parent / "assets" / "meta" _budget_meta.mkdir(parents=True, exist_ok=True) _budget_path = _budget_meta / ".fill_budget.json" _max_iter = int(getattr(args, "max_iterations", 0) or 0) if getattr(args, "reset_budget", False): - try: - _budget_path.write_text(json.dumps({"count": 0})) - except Exception: - pass - _iter_count = 0 - if _max_iter > 0: - try: - _iter_count = int(json.loads(_budget_path.read_text()).get("count", 0)) - except Exception: - _iter_count = 0 - _iter_count += 1 - try: - _budget_path.write_text(json.dumps({"count": _iter_count})) - except Exception: - pass - _breaker_hit = bool(_max_iter > 0 and _iter_count > _max_iter) + _clear_budget(_budget_path) + _iter_count = _load_budget(_budget_path) if _max_iter > 0 else 0 + _breaker_hit = False resolved = _canvas.resolve_canvas( html_path, args.canvas, label="[slack]" @@ -633,20 +725,34 @@ def cmd_slack(args: argparse.Namespace) -> int: # poster lacks polish's required measure-role markup, skip (don't fail # the slack run — --with-polish is an opt-in convenience). polish_collected = None + polish_skipped: str | None = None if getattr(args, "with_polish", False): + # Ask the LIVE page which roles resolved, not the static regex + # estimate: that estimate only reads the file text, so a poster + # whose `.section`s all carry some other role reads as "has + # cards" on disk and yields zero cards in the browser. polish + # would then measure nothing, emit no warnings, and PASS -- a + # silent green on an unmeasured poster (invisible in --json, + # which drops the "cards checked: 0" line). An empty result + # means nothing resolved OR the query failed; both fail closed. + _roles = _render.count_roles(page) _missing_roles = [ - r for r in ("poster", "card", "column") - if _preflight.has_required_roles_in_html(html_path).get(r, 0) == 0 + r for r in ("poster", "card", "column") if _roles.get(r, 0) == 0 ] if _missing_roles: + polish_skipped = ( + f"poster resolves no {'/'.join(_missing_roles)} element(s) " + "at render time" + ) _eprint( - "[slack] --with-polish: skipping polish pass, poster is " - f"missing measure-role markup {_missing_roles}" + "[slack] --with-polish: skipping polish pass, " + f"{polish_skipped}" ) else: try: polish_collected = _polish.collect_polish_data(page) except Exception as _pe: # never let polish break the fill gate + polish_skipped = f"polish measurement failed ({_pe})" _eprint( f"[slack] --with-polish: polish measurement failed, " f"skipped ({_pe})" @@ -693,8 +799,9 @@ def _classify_figure( of the figure to its containing section's card, and a verdict: ``OK`` (fills >=90% on at least one axis), ``NARROW`` (below the floor on both axes), ``OVERFLOW`` (>100% on either axis), - or ``BROKEN`` (image failed to load -- zero natural size and - not an SVG). The denominator is the section card -- the same + or ``BROKEN`` (image failed to load -- zero natural size, which + in Chromium means a 404 / unparseable file, SVG or + raster). The denominator is the section card -- the same box used for the slack ``fullRatio`` -- so a figure reading 100% width here visibly fills its card edge-to-edge. """ @@ -704,12 +811,6 @@ def _classify_figure( rh = float(fig.get("rendered_h") or 0) nw = float(fig.get("natural_w") or 0) nh = float(fig.get("natural_h") or 0) - src_l = str(fig.get("src", "")).lower() - src_path = src_l.split("?", 1)[0].split("#", 1)[0] - is_svg = ( - src_path.endswith((".svg", ".svgz")) - or src_l.startswith("data:image/svg") - ) out: dict[str, Any] = { "src": fig.get("src", ""), "rendered_w": round(rw, 1), @@ -717,7 +818,14 @@ def _classify_figure( "natural_w": nw, "natural_h": nh, } - if (nw <= 0 or nh <= 0) and not is_svg: + # Zero natural size == failed to load, SVG included: Chromium (the only + # renderer this measures in) resolves a viewBox-less 'd SVG to the + # 300x150 CSS default replaced size and never reports 0. The old + # `is_svg` src-extension exemption therefore only ever spared genuinely + # broken vector art. Kept in step with polish.py Gate A by hand -- if + # one gate exempts and the other doesn't, they disagree about the same + # figure. + if nw <= 0 or nh <= 0: out["verdict"] = "BROKEN" out["w_ratio"] = 0.0 out["h_ratio"] = 0.0 @@ -986,6 +1094,80 @@ def _classify_figure( and (not_full or bad_figs or suppress_secs) ) + # Run the merged polish gates NOW, buffering their output, so their + # verdict can join the convergence decision below. They are printed later + # by `_finish_ok` at the position they have always occupied (after the + # slack report), or dropped in --json mode; buffering keeps the ordering + # while letting the breaker see the whole picture. Without this the + # accounting would run on slack's verdict alone, and a poster that is FULL + # everywhere but keeps failing a polish gate would clear its budget every + # round -- the loop `--with-polish --strict` is supposed to bound would + # never trip the breaker at all. + _strict = bool(getattr(args, "strict", False)) + _polish_rc = 0 + _polish_out = "" + _polish_err = "" + _polish_emitted = False + if polish_collected is not None: + import contextlib + import io + _pargs = _polish.default_polish_args() + _pargs.strict = _strict + # Buffer stderr as well as stdout -- under --strict report_polish + # writes its FAIL line to stderr, which would otherwise escape and + # land ahead of the slack report it is supposed to follow -- but keep + # the two SEPARATE so each is replayed on its original stream. Merging + # them would quietly move `[polish] FAIL` onto stdout and break anyone + # who greps stderr for it. + _obuf, _ebuf = io.StringIO(), io.StringIO() + with contextlib.redirect_stdout(_obuf), contextlib.redirect_stderr(_ebuf): + _polish_rc = _polish.report_polish(polish_collected, _pargs, + html_path) + _polish_out, _polish_err = _obuf.getvalue(), _ebuf.getvalue() + elif getattr(args, "with_polish", False) and _strict: + # --with-polish --strict asked for the polish gates to bind. They did + # not run, so there is no evidence they would pass: failing here is the + # only answer that keeps `--strict` meaning "verified". Advisory mode + # (no --strict) still degrades to a warning, as before. + _polish_rc = 1 + _polish_out = ( + "=== POLISH: NOT RUN ===\n" + f" {polish_skipped or 'polish pass unavailable'}.\n" + " --strict requires the polish gates to actually run, so this " + "counts as a FAILURE\n" + " rather than a silent pass. Add the measure-role markup (or the " + "conventional\n" + " .poster/.col/.section classes), or drop --with-polish to gate " + "on fill alone.\n" + ) + _polish_err = ( + "[polish] FAIL -- --strict and the polish gates could not run\n" + ) + + # Circuit-breaker accounting (see the budget block at the top of this + # function). Convergence is judged on the MEASUREMENT, not on + # ``strict_fail`` -- the staged-fill loop is free to run without + # ``--strict``, and a breaker that only counted under ``--strict`` would + # never fire for it. This is the same condition ``--strict`` gates on: + # every section FULL and every figure OK, plus (when merged in) the + # polish gates, whose failures are equally something the loop must edit + # its way out of. + _converged = not (not_full or bad_figs or suppress_secs) + if _polish_rc != 0: + _converged = False + if _converged: + # Clear even when the breaker is disabled (`--max-iterations 0`). + # "Converged clears the count" is the documented contract, and a + # disabled run that left a stale 7 on disk would hand it straight back + # to the next enabled run inside the staleness window. + _clear_budget(_budget_path) + _iter_count = 0 + elif _max_iter > 0: + _iter_count += 1 + _write_budget(_budget_path, _iter_count) + if _max_iter > 0: + _breaker_hit = _iter_count > _max_iter + # Structured EDIT TARGETS -- verbatim source of each off-band section, so the # staged-fill loop can edit straight from this report and never re-Read the # whole poster.html (the input-context blowup that compacts + thrashes small @@ -1007,12 +1189,26 @@ def _classify_figure( "max": _max_iter, "breaker": _breaker_hit, } + if getattr(args, "with_polish", False): + # Structured, so a --json consumer (which never sees the human polish + # text) can tell "polish passed" from "polish never ran" instead of + # inferring it from a bare exit code. + report["polish"] = { + "ran": polish_collected is not None, + "failed": _polish_rc != 0, + "skippedReason": polish_skipped, + } def _emit_breaker() -> None: + # Show WHY first. The breaker exists to survive context compaction, so + # this round is very likely the first one a fresh context ever sees -- + # returning the banner without the polish warnings that caused it would + # hand that context a clean-looking slack verdict and no evidence. + _emit_polish() _eprint("") _eprint( - f" CIRCUIT BREAKER -- {_iter_count}/{_max_iter} fill measurements " - "on this poster." + f" CIRCUIT BREAKER -- {_iter_count}/{_max_iter} consecutive " + "non-converged fill measurements on this poster." ) _eprint( " STOP iterating. Accept the current best state: render " @@ -1028,31 +1224,49 @@ def _emit_breaker() -> None: _eprint( " compaction -- you cannot reset it by forgetting. Exit code is 3." ) + _eprint( + " It clears itself on the first converged measurement, after " + f"{BUDGET_STALE_AFTER_HOURS:.0f}h idle," + ) + _eprint( + " or via --reset-budget -- which is for a deliberate re-render, " + "NOT for" + ) + _eprint(" grinding the same edits past the cap.") - def _finish_ok() -> int: - """Non-breaker success exit. In ``--with-polish`` mode, also emit the - polish gates on the SHARED render and fold their result: the merged - command exits non-zero if EITHER the fill gate (slack ``--strict``) or - a polish gate (under the same ``--strict``) fails -- matching the - staged-fill exit condition 'every section FULL and zero FIG/NARROW'. - Without ``--strict`` polish is advisory (prints warnings, exit stays - 0), exactly as a standalone ``polish`` run.""" - if polish_collected is None: - return 0 - _pargs = _polish.default_polish_args() - _pargs.strict = bool(getattr(args, "strict", False)) + def _emit_polish() -> None: + """Replay the buffered polish output at the position it has always + occupied: after the slack report, each stream on its own. In --json + mode stdout stays pure JSON for the downstream pipe, so both are + dropped and only `report["polish"]` + the exit code carry the verdict. + Idempotent -- several exit paths call it, only the first does work.""" + nonlocal _polish_emitted + if _polish_emitted or not (_polish_out or _polish_err): + return + _polish_emitted = True if getattr(args, "json", False): - # Keep stdout pure JSON (this branch pipes to another tool): run - # the gates for the exit code only, swallowing their human output. - import contextlib - import io - with contextlib.redirect_stdout(io.StringIO()): - _prc = _polish.report_polish(polish_collected, _pargs, html_path) - else: - print() - print("=== POLISH (same render pass -- --with-polish) ===") - _prc = _polish.report_polish(polish_collected, _pargs, html_path) - return 1 if _prc != 0 else 0 + return + print() + print("=== POLISH (same render pass -- --with-polish) ===") + sys.stdout.write(_polish_out) + # Flush before touching stderr so the two land in this order under a + # shell `>log 2>&1`, where they share one file description. + sys.stdout.flush() + if _polish_err: + sys.stderr.write(_polish_err) + sys.stderr.flush() + + def _finish_ok() -> int: + """Non-breaker success exit. Emits the polish gates that already ran on + the SHARED render (buffered above so the breaker could see their + verdict first) and folds their result: the merged command exits + non-zero if EITHER the fill gate (slack ``--strict``) or a polish gate + (under the same ``--strict``) fails -- matching the staged-fill exit + condition 'every section FULL and zero FIG/NARROW'. Without + ``--strict`` polish is advisory (prints warnings, exit stays 0), + exactly as a standalone ``polish`` run.""" + _emit_polish() + return 1 if _polish_rc != 0 else 0 if args.json_out: Path(args.json_out).write_text( @@ -1062,6 +1276,7 @@ def _finish_ok() -> int: if args.json: sys.stdout.write(json.dumps({**report, "editTargets": edit_targets}, indent=2) + "\n") + _emit_polish() if _breaker_hit: _emit_breaker() return 3 @@ -1205,6 +1420,10 @@ def _finish_ok() -> int: print() print("--- JSON ---") print(json.dumps(report)) + # Emit the polish result on EVERY exit path, not just the success one: + # when slack strict-fails too, its warnings are still the other half of + # what the round has to fix. + _emit_polish() if _breaker_hit: _emit_breaker() return 3 diff --git a/ResearchStudio-Reel/skills/paper2poster/tests/test_gates.py b/ResearchStudio-Reel/skills/paper2poster/tests/test_gates.py new file mode 100644 index 0000000..50864c3 --- /dev/null +++ b/ResearchStudio-Reel/skills/paper2poster/tests/test_gates.py @@ -0,0 +1,791 @@ +"""Regression tests for the check_poster gates. + +Each test pins a contract that was silently broken before -- a gate that +looked green because it never actually ran, or a breaker that counted the +wrong thing: + + * ``slack``'s circuit breaker counted every INVOCATION, incremented + before the browser even launched, and never cleared. So a converged + poster inherited the debt of the rounds that converged it, a run that + never got geometry (nav timeout, MathJax settle failure) burned budget + it never used, and (once the count became consecutive-failures) a poster + failing only a polish gate cleared its budget every round and could + never trip the breaker at all. + * The class->role compat shim bailed on the FIRST explicit + ``data-measure-role`` it saw. ``assets/layouts_portrait/full.html`` + declares exactly one (``column``, on ``.method-hero``), so poster+card + stayed at zero and ``polish`` hard-failed its own shipped template. + * ``polish`` skipped the broken-image check for anything whose src looked + like an SVG, on the theory that vector art can legitimately report zero + natural size. Chromium never does that -- it resolves a viewBox-less + ````'d SVG to 300x150 -- so the exemption only ever white-printed + genuinely broken vector art. + * The gates trusted a static regex estimate of the role markup, which can + claim a card the browser does not render -- so they could measure + nothing, emit no warnings, and PASS. + * Nothing checked for a void in the MIDDLE of a card: CARD/TRAILING + skips space-distributing cards and reads ~0 when a tail is pinned to + the bottom, and Gate C only looks between cards in a column. + +Run: pytest ResearchStudio-Reel/skills/paper2poster/tests/ -q + +The DOM tests need Playwright + Chromium and skip themselves when it is +unavailable; the rest are browser-free. +""" +from __future__ import annotations + +import argparse +import datetime as _dt +import json +import sys +from pathlib import Path + +import pytest + +SKILL = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(SKILL / "scripts")) + +from utils import polish as _polish # noqa: E402 +from utils import preflight as _preflight # noqa: E402 +from utils import render as _render # noqa: E402 +from utils import slack as _slack # noqa: E402 + + +@pytest.fixture(scope="session") +def chromium(): + """Skip unless Playwright + Chromium are usable. Holds NO live context: + the tests that drive `cmd_slack` need it to open its own + `sync_playwright()`, and the sync API cannot be re-entered on one + thread.""" + try: + from playwright.sync_api import sync_playwright + except ImportError: + pytest.skip("Playwright not installed") + try: + with sync_playwright() as p: + p.chromium.launch().close() + except Exception as exc: # no browser binary + pytest.skip(f"Chromium not available ({exc})") + return True + + +@pytest.fixture +def page(chromium): + """A blank Chromium page. Function-scoped so the context is closed before + any test that lets the tool launch its own browser.""" + from playwright.sync_api import sync_playwright + + with sync_playwright() as p: + browser = p.chromium.launch() + pg = browser.new_page(viewport={"width": 1200, "height": 900}) + yield pg + browser.close() + + +# -------------------------------------------------------------------------- +# slack: circuit-breaker state +# -------------------------------------------------------------------------- + +def test_budget_roundtrip(tmp_path: Path) -> None: + p = tmp_path / ".fill_budget.json" + assert _slack._load_budget(p) == 0 # missing == fresh + _slack._write_budget(p, 5) + assert _slack._load_budget(p) == 5 + + +def test_budget_clear_removes_state(tmp_path: Path) -> None: + """A count surviving a converged measurement would break the + CONSECUTIVE-failure contract on the next run.""" + p = tmp_path / ".fill_budget.json" + _slack._write_budget(p, 7) + _slack._clear_budget(p) + assert not p.exists() + assert _slack._load_budget(p) == 0 + + +def test_budget_expires_when_stale(tmp_path: Path) -> None: + """A counter idle past the window is a PREVIOUS session, not this loop.""" + p = tmp_path / ".fill_budget.json" + old = (_dt.datetime.now(_dt.timezone.utc) + - _dt.timedelta(hours=_slack.BUDGET_STALE_AFTER_HOURS + 1)) + p.write_text(json.dumps({"count": 9, "updated": old.isoformat()})) + assert _slack._load_budget(p) == 0 + fresh = (_dt.datetime.now(_dt.timezone.utc) + - _dt.timedelta(hours=_slack.BUDGET_STALE_AFTER_HOURS - 1)) + p.write_text(json.dumps({"count": 9, "updated": fresh.isoformat()})) + assert _slack._load_budget(p) == 9 + + +@pytest.mark.parametrize("payload", [ + "{ not json", + json.dumps({"count": -3}), + json.dumps({"nope": 1}), + json.dumps({"count": "many"}), +]) +def test_budget_bad_state_never_invents_a_count(tmp_path: Path, + payload: str) -> None: + """Unreadable state must degrade to 0, never to a phantom breaker that + stops a legitimate loop.""" + p = tmp_path / ".fill_budget.json" + p.write_text(payload) + assert _slack._load_budget(p) == 0 + + +def test_budget_future_timestamp_resets(tmp_path: Path) -> None: + """Clock skew, or a run_dir copied from another host.""" + p = tmp_path / ".fill_budget.json" + ahead = _dt.datetime.now(_dt.timezone.utc) + _dt.timedelta(hours=2) + p.write_text(json.dumps({"count": 9, "updated": ahead.isoformat()})) + assert _slack._load_budget(p) == 0 + + +def test_budget_fresh_legacy_state_is_honoured(tmp_path: Path) -> None: + """State in the pre-expiry format (no `updated` field) must not be + silently zeroed: an in-flight loop upgrading mid-run keeps its count. + Its mtime is seconds old, so it is not stale.""" + p = tmp_path / ".fill_budget.json" + p.write_text(json.dumps({"count": 4})) + assert _slack._load_budget(p) == 4 + + +def test_budget_old_legacy_state_expires_by_mtime(tmp_path: Path) -> None: + """The migration case: the OLD counter never reset, so a large stale + count is exactly what a pre-upgrade file holds. Without an `updated` + field to age it, fall back to the file's mtime -- otherwise a + months-old 80 trips the breaker on the first measurement after upgrade.""" + import os + p = tmp_path / ".fill_budget.json" + p.write_text(json.dumps({"count": 80})) + old = (_dt.datetime.now(_dt.timezone.utc) + - _dt.timedelta(hours=_slack.BUDGET_STALE_AFTER_HOURS + 5)).timestamp() + os.utime(p, (old, old)) + assert _slack._load_budget(p) == 0 + + +# -------------------------------------------------------------------------- +# slack: the breaker must see the MERGED verdict, not slack's alone +# -------------------------------------------------------------------------- + +# A poster tuned so every section is FULL (fullRatio ~0.909) with no figures, +# so `slack` alone reports a fully clean verdict -- while polish's ORPHAN gate +# fails on the trailing arrow. That combination is the whole point: budget +# accounting keyed on slack's verdict alone would clear the count every round +# and the breaker could never fire for a poster the loop cannot fix. +_ORPHAN_LINE = ('

Consistency regularization along the probability-flow path ' + 'keeps the student aligned with the teacher marginal at every ' + 'noise scale.

') +_CONVERGED_BUT_ORPHANED = """ + +
+
+
accuracy 1.18-1.30x ↑
+""" + "\n".join([_ORPHAN_LINE] * 16) + """ +
+
+""" + + +def _slack_args(html: Path, **over): + ns = argparse.Namespace( + html=str(html), canvas=None, settle_ms=120, mathjax_timeout_ms=15000, + json=False, json_out=None, strict=False, max_iterations=2, + reset_budget=False, with_polish=True, + ) + for k, v in over.items(): + setattr(ns, k, v) + return ns + + +@pytest.fixture +def orphaned_poster(tmp_path: Path, chromium) -> Path: + """Depends on `chromium`, NOT `page`: cmd_slack opens its own browser.""" + f = tmp_path / "poster.html" + f.write_text(_CONVERGED_BUT_ORPHANED) + return f + + +def _count(html: Path) -> int: + p = html.parent / "assets" / "meta" / ".fill_budget.json" + return _slack._load_budget(p) + + +def test_strict_polish_failure_counts_against_the_breaker( + orphaned_poster: Path, capsys) -> None: + """slack is CONVERGED here, so accounting on its verdict alone would + clear the budget every round -- an unbounded `--with-polish --strict` + loop, which is exactly what the breaker is supposed to bound.""" + codes = [] + for _ in range(3): + codes.append(_slack.cmd_slack(_slack_args(orphaned_poster, strict=True))) + capsys.readouterr() + assert _count(orphaned_poster) == 3 + assert codes[-1] == 3 # breaker tripped (2 allowed, 3rd trips) + assert codes[0] == 1 # earlier rounds are ordinary gate failures + + +def test_advisory_polish_does_not_burn_budget( + orphaned_poster: Path, capsys) -> None: + """Without --strict polish is advisory, so the same poster IS converged: + the count must clear and the command must succeed.""" + for _ in range(2): + assert _slack.cmd_slack(_slack_args(orphaned_poster)) == 0 + capsys.readouterr() + assert _count(orphaned_poster) == 0 + + +def test_converged_run_clears_budget_even_when_breaker_disabled( + orphaned_poster: Path, capsys) -> None: + """A converged measurement clears the count -- the documented contract, + with no exception for `--max-iterations 0`. Seed a real count first -- + without that, a regression to "disabled means don't touch the file" would + still pass, since there would be nothing to clear.""" + meta = orphaned_poster.parent / "assets" / "meta" + meta.mkdir(parents=True, exist_ok=True) + _slack._write_budget(meta / ".fill_budget.json", 7) + assert _count(orphaned_poster) == 7 # seeded + + _slack.cmd_slack(_slack_args(orphaned_poster, max_iterations=0)) + capsys.readouterr() + assert _count(orphaned_poster) == 0 + + +def test_polish_output_still_follows_the_slack_report( + orphaned_poster: Path, capsys) -> None: + """The polish gates now run BEFORE the breaker decision so their verdict + can feed it, but their output is buffered and replayed here -- readers + must not see it move above the slack report.""" + _slack.cmd_slack(_slack_args(orphaned_poster, max_iterations=0)) + out = capsys.readouterr().out + assert out.index("verdict:") < out.index("=== POLISH") + assert "ORPHAN" in out + + +def test_json_mode_keeps_stdout_pure_json( + orphaned_poster: Path, capsys) -> None: + """--json pipes to another tool: the buffered polish output must be + dropped, with only the exit code and report["polish"] carrying its + verdict.""" + rc = _slack.cmd_slack( + _slack_args(orphaned_poster, json=True, strict=True, max_iterations=0)) + out = capsys.readouterr().out + assert "=== POLISH" not in out and "ORPHAN" not in out + payload = json.loads(out[out.index("{"):]) + assert payload["polish"] == {"ran": True, "failed": True, + "skippedReason": None} + assert rc == 1 # polish still gated the exit + + +def test_breaker_round_still_shows_why_it_tripped( + orphaned_poster: Path, capsys) -> None: + """The breaker returns before _finish_ok, so the round that trips it must + still replay the polish warnings that caused it -- this is the round a + freshly compacted context is most likely to read first, and a bare banner + over a clean slack verdict is no evidence at all.""" + for _ in range(2): + _slack.cmd_slack(_slack_args(orphaned_poster, strict=True)) + capsys.readouterr() + rc = _slack.cmd_slack(_slack_args(orphaned_poster, strict=True)) + captured = capsys.readouterr() + assert rc == 3 + assert "ORPHAN" in captured.out + assert "CIRCUIT BREAKER" in captured.err + + +def test_strict_polish_fail_stays_on_stderr( + orphaned_poster: Path, capsys) -> None: + """report_polish writes its FAIL line to STDERR. It is buffered now -- but + replayed to STDERR, not folded into stdout: moving it would silently break + anyone grepping stderr for it.""" + _slack.cmd_slack(_slack_args(orphaned_poster, strict=True, + max_iterations=0)) + captured = capsys.readouterr() + assert "[polish] FAIL" in captured.err # still stderr, as before + assert "[polish] FAIL" not in captured.out # not folded into stdout + # and the human report it belongs to is replayed after the slack verdict + assert captured.out.index("verdict:") < captured.out.index("=== POLISH") + + +def test_polish_streams_interleave_correctly_when_merged( + orphaned_poster: Path) -> None: + """The real ordering contract, which capsys cannot test: it collects the + two streams separately, so it passes with or without the flush. Only a + genuinely merged pipe (`>log 2>&1`, what a user actually types) shows + whether stdout was flushed before stderr was written.""" + import subprocess + cli = SKILL / "scripts" / "check_poster.py" + proc = subprocess.run( + [sys.executable, str(cli), "slack", str(orphaned_poster), + "--with-polish", "--strict", "--max-iterations", "0", + "--settle-ms", "120"], + stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, + timeout=180, + ) + merged = proc.stdout + # Assert rather than skip: the `chromium` fixture already established that + # the environment works, so a missing heading here means the CLI failed or + # stopped running polish -- exactly the regression this test should catch, + # not a reason to go green. + assert "=== POLISH" in merged, ( + f"polish never ran (rc={proc.returncode}); output:\n{merged}" + ) + # The polish report must appear after the slack verdict, and its stderr + # FAIL line after the report body it belongs to -- not hoisted above both + # by an unflushed stdout buffer. + assert merged.index("verdict:") < merged.index("=== POLISH") + assert merged.index("=== POLISH") < merged.index("[polish] FAIL") + + +# --- --strict must not pass a polish gate that never ran ------------------ + +_NO_ROLES = """ + +
+

body

+
+""" + + +def test_strict_fails_when_polish_could_not_run(tmp_path: Path, chromium, + capsys) -> None: + """`--with-polish --strict` asks for the polish gates to bind. If they + never ran there is no evidence they would pass, so reporting success is + a fail-open. Previously `polish_collected is None` returned 0.""" + f = tmp_path / "poster.html" + f.write_text(_NO_ROLES) + rc = _slack.cmd_slack(_slack_args(f, strict=True, max_iterations=0)) + out = capsys.readouterr().out + assert rc == 1 + assert "POLISH: NOT RUN" in out + + +def test_advisory_mode_still_tolerates_a_skipped_polish( + tmp_path: Path, chromium, capsys) -> None: + """Without --strict, --with-polish stays the opt-in convenience it was: + a poster it cannot measure must not fail the fill gate.""" + f = tmp_path / "poster.html" + f.write_text(_NO_ROLES) + assert _slack.cmd_slack(_slack_args(f, max_iterations=0)) == 0 + + +# A poster the STATIC scan reads as measurable but the browser does not: the +# only `.section` declares `column`, so the regex (which cannot tell that an +# element already carries a role) counts it as a card while the shim leaves +# it alone and no card exists at render time. +_STATIC_LIES = """ +
+

body

+
+""" + + +def test_static_scan_can_overcount_cards(tmp_path: Path) -> None: + """Pins WHY the live check is needed, not that the estimate is exact.""" + f = tmp_path / "poster.html" + f.write_text(_STATIC_LIES) + assert _preflight.has_required_roles_in_html(f)["card"] > 0 + + +# Every element declares an EMPTY role. The shim skips them (hasAttribute is +# true), and `""` is not a role any gate selects -- so no required role is +# present even though every element carries the attribute. +_EMPTY_ROLE_ATTRS = """ +
+
+

body

+
+
+""" + + +# Roles padded with whitespace. `[data-measure-role="card"]` is an EXACT +# attribute match and does not select ` card `, so every gate finds nothing -- +# but a count that normalised the value would report all three roles present +# and wave the poster through. +_PADDED_ROLE_ATTRS = """ +
+
+

body

+
+
+""" + + +def test_count_roles_means_what_the_gate_selectors_mean(page) -> None: + """The counts exist to decide whether the gates have anything to select, + so they must agree with the gates' exact attribute selectors -- trimming + would report a `card` that `[data-measure-role="card"]` cannot find.""" + page.set_content(_PADDED_ROLE_ATTRS) + live = _render.count_roles(page) + assert live.get("card", 0) == 0 + selected = page.evaluate( + """() => document.querySelectorAll( + '[data-measure-role="card"]').length""" + ) + assert selected == live.get("card", 0) + + +@pytest.mark.parametrize("markup", [_STATIC_LIES, _EMPTY_ROLE_ATTRS, + _PADDED_ROLE_ATTRS]) +def test_standalone_polish_refuses_when_nothing_resolves_live( + tmp_path: Path, chromium, capsys, markup: str) -> None: + """The static scan says these posters have cards; the browser says they + have none. polish must not run its gates over zero cards and report + success -- that is a silent green on a poster nobody checked. The + empty-attribute case pins that a role no gate can select counts as + absent, not as present-but-blank; the padded case pins that the count is + not normalised out of agreement with the selectors.""" + _assert_polish_refuses(tmp_path, markup, capsys) + + +def test_polish_refuses_when_the_role_query_itself_fails( + tmp_path: Path, chromium, capsys, monkeypatch) -> None: + """An EMPTY live map means either "nothing resolved" or "the query + failed", and the two are indistinguishable -- so both must fail closed. + This is the case the empty/padded markup above does NOT pin: those return + `{"": 3}` / `{" card ": 1}`, which are truthy, so a regression to + `if live_missing and live_roles:` would still reject them. Only a + genuinely empty map separates the two guards.""" + monkeypatch.setattr(_render, "count_roles", lambda page: {}) + _assert_polish_refuses(tmp_path, _CONVERGED_BUT_ORPHANED, capsys) + + +def _assert_polish_refuses(tmp_path: Path, markup: str, capsys) -> None: + f = tmp_path / "poster.html" + f.write_text(markup) + pargs = _polish.default_polish_args() + pargs.strict = True + for k, v in (("html", str(f)), ("canvas", None), ("settle_ms", 120), + ("mathjax_timeout_ms", 15000)): + setattr(pargs, k, v) + rc = _polish.cmd_polish(pargs) + assert rc == 2 + assert "resolves no" in capsys.readouterr().err + + +# -------------------------------------------------------------------------- +# Class->role fallback: every required role must resolve at RENDER time +# -------------------------------------------------------------------------- + +_PORTRAIT = SKILL / "assets" / "layouts_portrait" / "full.html" + +_MIXED = ('
x
' + '
y
') +_CLASSES = ('
' + '
x
') +_EXPLICIT = ('
' + '
x
') + + +@pytest.mark.parametrize("layout", sorted( + (SKILL / "assets").rglob("layouts*/[!_]*.html"))) +def test_every_shipped_layout_satisfies_polish_precondition( + layout: Path) -> None: + """cmd_polish hard-fails (exit 2) unless poster+card+column all resolve. + The portrait layout declares only `column`, so an all-or-nothing + fallback reported poster=0/card=0 and failed a template we ship.""" + counts = _preflight.has_required_roles_in_html(layout) + missing = [r for r in ("poster", "card", "column") if counts.get(r, 0) == 0] + assert not missing, f"{layout.name} would exit 2: missing {missing}" + + +def test_declared_role_is_additive_not_exclusive(tmp_path: Path) -> None: + """A declared role must not switch the fallback OFF for that role. + Portrait declares `column` on .method-hero to ADD a column no class + matches (mirroring slack's `.col, .mid-wide, [role=column]` union); if + that one attribute suppressed the shim, the ordinary `.col`s would go + unmeasured by every role-keyed gate.""" + f = tmp_path / "m.html" + f.write_text(_MIXED) + counts = _preflight.has_required_roles_in_html(f) + assert counts["column"] == 2 # the declared one AND the .col + assert counts["poster"] == 1 + assert counts["card"] == 2 + + +@pytest.mark.parametrize("html", [_MIXED, _CLASSES, _EXPLICIT]) +def test_runtime_shim_resolves_required_roles(page, html: str) -> None: + """Whatever mix of declared roles and conventional classes a poster + uses, all three required roles must exist once the shim has run -- + otherwise polish refuses (or worse, measures nothing).""" + page.set_content(html) + _render.inject_class_fallback_roles(page) + live = _render.count_roles(page) + for role in ("poster", "column", "card"): + assert live.get(role, 0) > 0, f"{role} unresolved for {html[:40]}" + + +def test_fully_explicit_document_is_left_alone(page) -> None: + """A poster that owns all its roles must not be rewritten by the shim.""" + page.set_content(_EXPLICIT) + assert _render.inject_class_fallback_roles(page) is False + + +def test_shim_does_not_overwrite_a_declared_role(page) -> None: + """`.method-hero` is a `.col`-less column: the shim may add poster/card + around it but must never re-tag it. The per-element guard -- not a + per-role bail -- is what protects author intent.""" + page.set_content(_MIXED) + _render.inject_class_fallback_roles(page) + assert page.evaluate( + """() => document.querySelector('.method-hero') + .getAttribute('data-measure-role')""" + ) == "column" + + +def test_ordinary_cols_still_get_a_role_alongside_a_declared_one(page) -> None: + """The regression a per-role bail would cause: portrait declares one + `column` on .method-hero, so skipping the column fallback would leave its + real `.col`s roleless and invisible to every role-keyed gate (Gate C's + space-between scan queries `[data-measure-role="column"]` only).""" + page.set_content(_MIXED) + _render.inject_class_fallback_roles(page) + assert page.evaluate( + """() => document.querySelectorAll( + '[data-measure-role="column"]').length""" + ) == 2 # .method-hero (declared) AND .col (injected) + + +# -------------------------------------------------------------------------- +# polish Gate A: a zero-natural-size vector figure still gets judged +# -------------------------------------------------------------------------- + +def _card_with(src: str) -> str: + return ( + '
' + '
' + f'' + "
" + ) + + +def _natural(page, src: str) -> tuple[float, float]: + page.set_content(_card_with(src)) + page.wait_for_timeout(150) + _render.inject_class_fallback_roles(page) + figs = _polish.collect_polish_data(page)["data"]["figures"] + assert figs, "figure not collected" + return figs[0]["natural_w"], figs[0]["natural_h"] + + +def _svg_uri(body: str) -> str: + return "data:image/svg+xml;utf8," + body + + +_SVG_VIEWBOX = _svg_uri( + "" + "" +) +_SVG_NO_INTRINSIC = _svg_uri( + "" +) +_SVG_CORRUPT = _svg_uri("this is not svg at all") + + +@pytest.mark.parametrize("src", [_SVG_VIEWBOX, _SVG_NO_INTRINSIC]) +def test_valid_svg_never_reports_zero_natural_size(page, src: str) -> None: + """The premise the dropped `is_svg` exemption rested on -- "a vector + image can legitimately report zero natural size" -- is false in the only + renderer these gates run in. Chromium resolves a viewBox-less 'd SVG + to the 300x150 CSS default replaced size. If this ever fails, the + exemption needs reinstating and FIG/BROKEN needs an img.decode() probe + instead.""" + nw, nh = _natural(page, src) + assert (nw, nh) == (300, 150) + + +def test_corrupt_svg_reports_zero_and_must_be_called_broken(page) -> None: + """The flip side: zero natural size on an SVG means it FAILED, so the old + extension-based exemption only ever white-printed broken vector art.""" + assert _natural(page, _SVG_CORRUPT) == (0, 0) + + +def test_broken_svg_is_reported_broken_not_narrow(page, capsys) -> None: + """End-to-end through the real reporter: a corrupt SVG must surface as + FIG/BROKEN. Previously `is_svg` skipped the check and the figure passed + Gate A silently.""" + page.set_content(_card_with(_SVG_CORRUPT)) + page.wait_for_timeout(150) + _render.inject_class_fallback_roles(page) + collected = _polish.collect_polish_data(page) + _polish.report_polish(collected, _polish.default_polish_args(), + Path("poster.html")) + out = capsys.readouterr().out + assert "FIG/BROKEN" in out + assert "FIG/NARROW" not in out # never measure a blank image for fill + + +def test_valid_svg_is_not_called_broken(page, capsys) -> None: + """The safety property of dropping the exemption: real vector art must + still pass FIG/BROKEN untouched. It does, because Chromium gives it a + non-zero natural size -- the check needs no knowledge that it is an SVG.""" + page.set_content(_card_with(_SVG_VIEWBOX)) + page.wait_for_timeout(150) + _render.inject_class_fallback_roles(page) + collected = _polish.collect_polish_data(page) + _polish.report_polish(collected, _polish.default_polish_args(), + Path("poster.html")) + assert "FIG/BROKEN" not in capsys.readouterr().out + + +# -------------------------------------------------------------------------- +# polish Gate C: CARD/INNER-VOID +# -------------------------------------------------------------------------- + +_CARD_CSS = ("") + + +def _inner_voids(page, html: str) -> list[dict]: + page.set_content(_CARD_CSS + html) + _render.inject_class_fallback_roles(page) + data = _polish.collect_polish_data(page)["data"] + return [ + iv for iv in data["innerVoids"] + if iv["card_h"] > 0 + and iv["excess"] > _polish.DEFAULT_MIN_CARD_INNER_VOID_PX + and iv["excess"] / iv["card_h"] > _polish.DEFAULT_MAX_CARD_INNER_VOID + ] + + +def test_inner_void_fires_on_a_bottom_pinned_tail(page) -> None: + """The case the gate exists for: an equal-height row's short card pins + its tail with margin-top:auto, so CARD/TRAILING reads ~0.""" + voids = _inner_voids(page, ( + '
' + '
' + '

short

pinned

' + "
" + )) + assert len(voids) == 1 + assert voids[0]["section"] == "key-result" + + +@pytest.mark.parametrize("jc", ["space-between", "space-around", + "space-evenly"]) +def test_inner_void_fires_on_every_space_distributing_card(page, + jc: str) -> None: + """CARD/TRAILING skips any card whose justify-content distributes space, + which left those cards with no void check at all. `space-around` and + `space-evenly` also reserve space at the BOTTOM, so nothing is flush with + the content edge -- they must bypass the tail-pinned precondition, or + this gate re-opens the very hole it was added to close.""" + voids = _inner_voids(page, ( + '
' + f'
' + "

top

bottom

" + "
" + )) + assert len(voids) == 1 + + +def test_inner_void_silent_on_a_margin_spaced_card(page) -> None: + """paper2poster cards space children with margins, not row-gap, so the + stated gap is 0 and every normal paragraph gap reads as `excess`. The + ratio+px floors must keep an ordinary rhythm quiet.""" + assert _inner_voids(page, ( + '
' + '
' + "

one

two

three

" + "
" + )) == [] + + +@pytest.mark.parametrize("style", [ + "display:block", # justify-content is inert entirely + "display:flex;flex-direction:row", # justify-content runs horizontally +]) +def test_inner_void_ignores_space_between_off_the_vertical_axis( + page, style: str) -> None: + """`justify-content: space-between` only distributes downward on a flex + COLUMN. Computed style reports the declared value on a block or row card + too, so waiving the tail-pin check on the string alone would re-admit the + short-card heading false positive on cards that distribute nothing + vertically. + + (`display:grid` is deliberately not covered here: `align-content` defaults + to stretch, so a grid card really does deal leftover space into its rows + and open a genuine gap -- it reaches the gate through the ordinary + pinned-tail path, not this bypass, so it is not a case this test can + isolate.)""" + assert _inner_voids(page, ( + '" + '
' + f'
' + "

T

body

" + "
" + )) == [] + + +def test_inner_void_silent_on_production_heading_rhythm(page) -> None: + """The false positive this gate must not have. `.section h2` ships at + 60pt with `margin: 0 0 0.45em` (assets/styles/*.css), a real 36px gap + under EVERY heading. A top-packed card must stay quiet: its content + simply stops early, which is CARD/TRAILING's job, not this gate's. + Guarded by the tail-pinned requirement, so it holds at any card height + -- including a short card where 36px would clear the ratio.""" + voids = _inner_voids(page, ( + '" + '
' + '
' + "

Problem

one line of body copy

" + "
" + )) + assert voids == [] + + +def test_inner_void_ignores_absolutely_positioned_children(page) -> None: + """The floating Listen button sits at the card bottom but is not flow + content -- counting it would mask the void above it.""" + assert _inner_voids(page, ( + '
' + '

one

two

' + '' + "
" + )) == [] + + +def test_inner_void_merges_side_by_side_rows(page) -> None: + """A block clearing a tall floated sibling is not a void: the gap must + be measured from the row's tallest member, not its shortest.""" + assert _inner_voids(page, ( + '
' + '
' + '
tall
' + '
short

after

' + "
" + )) == []