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

' + "
" + )) == []