Skip to content

fix(cli): keep overlap waivers local to marked text - #3464

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/layout-overlap-validation
Aug 24, 2026
Merged

fix(cli): keep overlap waivers local to marked text#3464
miguel-heygen merged 4 commits into
mainfrom
fix/layout-overlap-validation

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

What

  • Make data-layout-allow-overlap apply only to the marked text block.
  • Add a regression proving a scene/root marker cannot hide descendant text collisions.
  • Correct the mirrored changelog-video build contract: local overlap markers, an intentional caption-rail exemption, and an explicit bottom-10% caption-zone gate across 25 seeks.
  • Pin the original y≈970–1088 / 1080px scene-card case so it fails while the caption rail itself remains exempt.
  • Update CLI guidance and the published skill manifest.

Why

The overlap waiver used closest(), so placing it on a mock-slide or composition wrapper silently disabled collision detection for every descendant. The changelog skill explicitly prescribed that broad marker. Separately, the original card only breached the canvas by 8px—below the generic painted-panel floor—so the changelog gate needs its known caption-rail band enabled rather than relying on generic overflow detection.

Together these gaps allowed hyperframes check to report a false-clean layout while unrelated text collided or scene content entered the caption rail.

How

The browser overlap audit now checks hasAttribute() on each solid text block. Intentional layering remains waivable on the exact participant, while parent/root markers no longer exempt the subtree.

The changelog scaffold marks #cap-line with data-layout-allow-caption-zone, removes the slide-root overlap waiver, and its required check command samples the reserved bottom 10% at 25 evenly distributed timeline points (maximum 2.4s gap at the 60s skill duration cap).

Test plan

  • Overlap regression verified red before implementation and green after
  • Exact 970–1088 caption-zone regression added
  • Changelog skill contract regression verified red before the skill repair and green after
  • Full CLI suite: 2,817 passed, 3 skipped
  • bun run --cwd packages/cli typecheck
  • bun run build
  • bun run lint
  • bun run format:check
  • Skill mirrors verified byte-identical
  • Documentation updated

Separate from the preview/seam-gate fix in #3463.

@mintlify

mintlify Bot commented Aug 24, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Aug 24, 2026, 5:31 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@jrusso1020

Copy link
Copy Markdown
Collaborator

Follow-up on the same scene-4 case, after tracing why check missed it originally. The code here is right; the concern is the recommended invocation in the skill docs.

The caption gate runs only at the declared seeks, not at the merged layout grid.
checkPipeline.ts:152 merges captionSamples into layoutSamples, which reads like the gate gets the union. It doesn't: geometryRequest (checkPipeline.ts:216) returns null unless grid.captionSamples.includes(time) matches exactly. So caption_zone_collision is evaluated at exactly the 10 declared times and nowhere else.

Those 10 times are a fixed fraction list, so spacing scales with duration.
gateSampleTimes maps each fraction to fraction * duration (checkPipeline.ts:125). seek=.05,.15,…,.95 is therefore a uniform grid of period 0.10 * duration. On the 53.0s changelog film that is one probe every 5.3s.

Consequence on the exact defect that motivated this PR. The scene-4 card is visible 2.86s. A 2.86s window against a 5.3s grid intersects with probability 2.86 / 5.3 ≈ 54%. So the gate catches the case it was built for a bit better than a coin flip, and gets worse as films get longer: at 90s the spacing is 9s and the same defect drops to ~32%.

The good news, since it is easy to assume otherwise: caption_zone_collision is deliberately not in PERSISTENCE_TIERED_CODES (layoutAudit.ts:195-205), so applyPersistenceTier returns it untouched and the occurrences <= 1 info cap never applies. One hit is enough, and it reports at the flag's severity=error. The gap is purely detection probability, not severity.

Two ways to close it, both cheap:

  1. Beat-aligned absolute seeks. The slides already carry data-start and captions.json carries the caption timings, so the interesting instants are known at build time and coverage becomes deterministic rather than probabilistic.
  2. Or keep the uniform grid and just make spacing shorter than the shortest element lifetime. For a 53s film with ~2.9s beats that means ≥ 19 seeks rather than 10.

(1) is the one I would take, since it stays correct as film length changes and (2) has to be re-tuned every time the format does.

Verified at origin/main 95e1ac9 and against this PR at e9bc6b16; the 2.86s and 53.0s figures come from the fire-5 changelog build.

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

The 25-point grid checks out: .02,.06,…,.98 is step 0.04, 25 probes, max interior gap 0.04 * duration, and the leading/trailing gaps are 0.02 * duration so they're strictly smaller. At 60s that's 2.4s < 2.86s, deterministic. Mechanical beats hand-calculated fractions, agreed.

One edge worth closing while you're in here: the 60s cap is prose, not an invariant. SKILL.md:51 says "Budget: 45-60s total", and I couldn't find anything that enforces it — no duration guard in the skill scripts, none in the CLI. Fire 5 came in at 53.0s against that same budget, and theme count moves with commit volume week to week.

That matters because the guarantee is duration-dependent in exactly the way the ten-point grid was, just with a friendlier constant. At 75s the gap is 3.0s and the original 2.86s case silently goes back to being a coin flip — and a contract test that pins "25 points" plus "≤2.4s max gap" computed at a fixed duration would still be green while it happened.

Two ways to make the test mean what it says:

  1. Derive the seek count from the actual duration against a max-gap target, so the invariant is "gap ≤ 2.4s" rather than "25 points happen to give that at 60s".
  2. Or keep the fixed 25 and assert the duration bound where the seeks are chosen, so an over-budget film fails loudly instead of quietly losing coverage.

Either is fine. The thing I'd avoid is having the only thing standing between the grid and under-sampling be a sentence in a docs budget.

— Rames

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Re-verified at head dbad0749a.

The change is right. hasAllowOverlapFlag moving from closest() to hasAttribute() has exactly one call site, isSolidTextBlock (layout-audit.browser.js:582), so the blast radius is contained to which elements get collected as overlap participants. The comment states the rationale accurately, and tightening is the correct direction for an audit tool.

The regression witness is non-vacuous. With rootAttrs: "data-layout-allow-overlap", the two descendants (100-500 x 100-200 and 300-700 x 120-220) genuinely overlap. Under the old closest() both inherited the root waiver, isSolidTextBlock returned false for each, and zero blocks were collected, so expect(...).toBe(true) fails pre-fix. The installOverlapStyles / installOverlapGeometry extraction preserves the original call order (styles, geometry, installAuditScript, runAudit), so it is behavior-neutral.

data-layout-allow-caption-zone is actually wired, not inert: layout-audit.browser.js:109 reads it via closest(), so the #cap-line assertion in changelogSkillContent.test.ts pins a real exemption rather than a name nothing consumes.

One non-blocking follow-up. The waiver semantics changed globally, but only the changelog-video skill was swept. Of the 38 in-repo uses of data-layout-allow-overlap, every one marks the leaf text element except registry/blocks/message-thread-reveal/message-thread-reveal.html:910, where #mtr-ec-wrap carries it as a container. That wrapper has text descendants (#mtr-ec-fam, three .ec-label pairs, #mtr-ec-cta) which inherited the waiver before and no longer do. They sit at distinct absolute left offsets so they may well not collide, and nothing in CI audits that block, but it is the one site where this change alters behavior outside the files you touched.

CI at this head is green across the board, with Tests on windows-latest still running.

— Rames

@miguel-heygen
miguel-heygen merged commit e5a5e6b into main Aug 24, 2026
48 checks passed
@miguel-heygen
miguel-heygen deleted the fix/layout-overlap-validation branch August 24, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants