feat(check): flag connectors that point at nothing and stylesheets that leak into the frame

[xuanruli]

Summary

Three defects a viewer sees but nothing catches today. All measured against 47 real V2A compositions.

connector_orphan (check, new) — a connector shaft is painted while one of the nodes it runs between is hidden in place. The line ends in empty space. Endpoints are resolved against real element boxes near each end, not against a stage-wide anchor count, so a shaft between two live nodes never trips it. A shaft that is itself hidden (opacity, display, dash-hidden) is skipped.

connector_detached (check, new branch) — an arrow-bearing shaft long enough to read as a link (rendered chord ≥ 80px) whose endpoints miss every node in both user space and screen space. The existing CTM-paste path is untouched; same-anchor grazes, short marker glyphs, and name-only decorative flow lines stay skipped.

One behaviour change to the existing rule: data-layout-allow-overflow on a connector svg no longer suppresses connector_detached. That flag says a layer may bleed past the frame; it does not say its arrows may point at nothing, and both new codes score the same layers. Roughly 28 registry blocks carry the flag and were silently exempt.

unbalanced_style_tags (lint, new) — <style> and </style> counts disagree. One extra closer ends the stylesheet early and the rest of the CSS renders as visible text in the frame; a missing closer swallows the following markup. </style > with a space counts, and a </style> inside a script string does not.

Scope, precisely: the node must still occupy layout, hidden by opacity, visibility, or an ancestor's opacity. A node hidden with display: none has no geometry at all, so nothing can place it near an endpoint — and connector_detached does not catch it either, because it allows a half-attached shaft by design. That case is uncovered; there is a fixture pinning it.

What this PR deliberately does not add

An earlier revision ported marker_orient_typo and marker_dash_draw_on into packages/lint. Both already exist on the EF side, which runs on the same compositions — a port would have meant two findings per defect. Those rules are gone from this PR; the EF copies stay the source of truth.

Measured

	corpus hits	gating severity
connector_orphan	9 findings / 5 compositions	1 warning, 8 demoted to info
connector_detached	4 findings / 2 compositions	1 warning, 3 demoted to info
unbalanced_style_tags	1 composition	error

Every connector_orphan hit was read back against its composition. The clearest is a diagram whose exit sequence fades .source-node and .path-line on one shared stagger: four shafts each outlive a different node. Before the geometry-key fix below, three of those four were collapsed away.

No false positive survived on this corpus, but that is a fact about these 47 compositions, not a property of the rule — review found a real one (a staged halo coincident with the live node it belongs to) that the corpus simply never exercised. It is fixed and pinned.

connector_orphan was rewritten twice during review. The first version counted anchors across the whole stage rather than at the endpoints; the second used bounding-box visibility, which reports height 0 for an axis-aligned line and made the rule blind to the most common connector shape.

Test plan

• packages/lint core.test.ts (63) — unbalanced counts both directions, </style > spacing, script-string false positive, paired blocks
• packages/cli layout-audit.browser.test.ts — orphan and marked-miss cases plus the existing graze / decorative / paste-bug cases
• Both new codes run over 47 production compositions; every hit inspected in the rendered frame
• Mutation sweep over connector_orphan: shaft paint, endpoint proximity, connector shape, dash-offset skip, the visible-candidate check, and each of opacity / display / visibility-hidden / visibility-collapse each fail the suite when disabled
• CI on this PR

[james-russo-rames-d-jusso]

:large_green_circle: 644990abd COMMENTED

Reviewed the mechanism at 428a8fe3c and re-pinned at 644990abd after the author's test-only follow-up. Interdiff 428a8fe3c...644990abd verified via GitHub compare: single file packages/cli/src/commands/layout-audit.browser.test.ts +55/-0, no rule or wiring changes — the mechanism review below carries directly. Xuanru's own read of the discriminator-honesty check preemptively closed two guards that return [] swaps could bypass (hidden shaft paint check and endpoint proximity threshold); the new tests at 644990abd are the fixture-crosses that pin them.

Traced the three new rules against findConnectorOrphans / connectorDetachmentIssues / the unbalanced_style_tags closure, walked the callers into runStaticSweep and the linter registration, and checked the tests against the mechanism. Three concerns worth surfacing, none are blockers.

Verified as OK (mechanism traced, not just eyeballed)

• connector_orphan — endpoints come from the shaft's own geometry, not a stage-anchor tally. pathUserEndpoints calls getPointAtLength(0) and getPointAtLength(total), then pathScreenEndpoints walks each point through path.getScreenCTM(). So the "near each end" resolution is: for each of the two shaft endpoints in screen coordinates, distanceToRect(point, candidate.rect) against every connectorEndpointCandidates box; a candidate is "close enough" iff gap <= Math.max(32, Math.min(rootRect.width, rootRect.height) * 0.02) — the same threshold connectorDetachmentIssues uses. The candidate that wins is the closest one within that radius, and it only counts as dark if isVisibleElement(best.candidate.element) is false. A shaft between two live nodes never trips because each endpoint's best candidate is visible. A shaft between two anchor-free empty patches never trips because best === null. This is the correct behavior and it matches the fixture-cross tests in layout-audit.browser.test.ts.
• Axis-aligned shaft detection actually works. pathUserEndpoints reads path length via getPointAtLength, so a horizontal d="M 360 480 L 1400 480" gets endpoints (360, 480) and (1400, 480) — even though getBoundingClientRect on the same path returns height 0. The flags a horizontal shaft, whose bounding box has no height regression test locks this in; if the rule reverted to bbox-based endpointing it would fail. That's the V2 bug the author's history references, killed.
• shaftDashHidden is bidirectionally-correct on the common case. Requires offset >= total * 0.9 AND dash >= total * 0.9. A path with stroke-dashoffset: total; stroke-dasharray: total 0 (the canonical "mask the whole line via dash animation" trick) → skipped. A path with stroke-dasharray: none → parsed as 0, condition fails → not treated as hidden, evaluated normally. A path with stroke-dasharray: 100 20 on a 100-length shaft with offset 0 → dash>=90 but offset<90 → still evaluated, correct.
• shaftIsPainted covers the opacity-close-to-zero case, not just literal 0. opacityChain(path) >= 0.2 uses the ancestor chain, so opacity: 0.05 on a parent group correctly hides the shaft. visibility: collapse handled alongside visibility: hidden. The new fixture at 644990abd crosses all four invisibility clauses (opacity / visibility / display / dash) so a return true short-circuit on shaftIsPainted fails the suite.
• connector_detached markedMiss branch does what the message says. Only fires when renderedChord >= 80 AND both userStartKey and userEndKey are null (both frames miss) AND the path carries marker-start or marker-end. Same-anchor grazes still fall through the userStartKey === userEndKey guard above and stay skipped; the skips same-anchor cross-tier arrows that only graze one node fixture locks that in. Name-only decorative flow lines without a marker attribute never reach markedMiss because the condition requires the attribute, so they only route through the paste-bug branch and get skipped when their user-space coords also miss — the counterfactual the fixture actually exercises.
• Overflow-flag removal is intentional and narrow. The commit baec801 dropped || hasAllowOverflowFlag(svg) from both the connector_detached iterator loop and the connectorOrphanIssues loop. Anchor iteration (connectorAnchorRects, connectorEndpointCandidates) still filters via isVisibleElement, so opaquely-visible nodes on a data-layout-allow-overflow sibling remain valid endpoint targets. The orphans a dark endpoint when the connector svg allows overflow and detaches a paste-bug shaft when the connector svg allows overflow fixtures lock in the intended widening. No test fails because of the removal.
• unbalanced_style_tags single-pass alternation. The pattern <script\b[\s\S]*?</script[^>]*>|<style\b|</style\s*> lets the script-block alternative consume the entire <script>…</script> region (including a </style> embedded in a JS string literal) before the <style> or </style> alternatives ever see it. </script[^>]*> correctly matches </script > with a trailing space. </style\s*> matches </style >, </style\n>, and </STYLE > (with i). Openers <style\b match <style scoped>, <style type="text/css">, <style class="…">. Return path emits at most one finding per file, so dedupeKeyFor in hyperframeLinter.ts cannot silently collapse partial-detection into full-detection — the #2811 failure mode does not apply here.
• checkBrowser.ts / layoutAudit.ts wiring. The one-line delta in each of those three files (checkBrowser.ts, layoutAudit.ts, layoutAudit.test.ts) just adds "connector_orphan" to LAYOUT_ISSUE_CODES and PERSISTENCE_TIERED_CODES. Not a fail-open contract change — it opts the code into the same persistence-tiered severity collapse the other layout codes already get. The persistence-tier test in layoutAudit.test.ts verifies the tier behavior for the new code.
• Test discriminator honesty. Every new connector_orphan test I read pairs a positive assertion (toHaveLength(1) + specific selector/message substring) with a negative one nearby that would flip if the rule returned [] or everything. unbalanced_style_tags tests assert both "flag with severity=error" (positive) and "no finding" for the counterfactual (paired blocks, and </style> embedded in a script). Both would break under a return [] swap. The two new tests at 644990abd explicitly close the last two guards a return []-shaped disable could still bypass (see the "test(check): cover the two orphan guards that no fixture was crossing" commit).

Concerns (non-blocking, worth surfacing)

• </style> embedded in an HTML comment is not stripped. The alternation strips <script> blocks but not <!-- … -->. HTML like <style>foo</style><!-- </style> --> counts as 1 open + 2 close and would trip unbalanced_style_tags with the "extra </style> closes the stylesheet early" message. Practical hit rate is low — someone commenting out a stray </style> while editing — but the fix is a cheap prepend <!--[\s\S]*?-->| to the alternation and a if (token.startsWith("<!--")) continue; in the loop. Optional; not a blocker.
• display: none and transform: scale(0) nodes fall out of the orphan candidate set before the visibility gate runs. connectorEndpointCandidates filters area < 400 || area > rootArea * 0.15 from getBoundingClientRect(), and display: none returns a zero rect (area 0), so the endpoint's nearest candidate becomes null and dark stays empty — no connector_orphan fires. Same for transform: scale(0) where the bbox degenerates. visibility: hidden and opacity: 0 (the fixture cases) are caught because they keep their box. This is a coverage gap, not a regression — but "the node it runs between is not on screen" reads broader in the PR title than the mechanism actually delivers. Worth a one-line comment near the candidate filter noting which hide-mechanisms the rule sees, or a fixture that pins the boundary.
• An endpoint whose target node has translated away is invisible to the rule. If a shaft endpoint is at (1400, 480) and its target moves to (5000, 5000) via CSS transform, the target's bbox is far from the endpoint and drops out of the threshold filter — the endpoint's best is null, dark empty. So "endpoint animates off but shaft stays" is a class of orphan the rule intentionally does not cover. Design choice, but ties into the concern above: the semantic gap between "not on screen" (title) and "the closest anchor box near the endpoint is hidden in place" (mechanism) is real. Non-blocker.

What I didn't verify

• Corpus. The PR body's numbers (3 findings / 2 compositions for connector_orphan, 1 for unbalanced_style_tags, 0 FPs) — no way to reproduce them from the tree. Trusted per author.
• Local vitest run. Read the tests and traced their discriminator shape against the code, but did not pnpm -F @hyperframes/cli test or pnpm -F @hyperframes/lint test locally.
• SVG <use> / <foreignObject> anchor targets. The candidate iteration explicitly skips element.closest("svg"), so a connector aimed at an svg-child element (e.g., another <image> in the same svg) never becomes a candidate. This blind spot pre-exists connector_detached (annotated there) and carries into connector_orphan by construction. Not exercised by any new fixture.
• Behavior under a real Chromium sweep (as opposed to jsdom mocks with installConnectorGeometry stubbing getScreenCTM / getPointAtLength). Test mocks give matrix and endpoint numbers directly; real SVG element math may surface edge cases the mocks don't (e.g., getPointAtLength throwing on a degenerate path with only M).

State at HEAD 644990abd: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA and any subsequent push voids it.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

:large_green_circle: 137452aed COMMENTED (re-pin under require_last_push_approval: true)

Interdiff 644990abd → 137452aed verified test-only via git diff: packages/lint/src/rules/core.test.ts +15/-0 and packages/cli/src/commands/layout-audit.browser.test.ts +14/-0. Rule and wiring files (core.ts, layout-audit.browser.js, layoutAudit.ts, checkBrowser.ts) all byte-identical to R1. Mechanism review from 644990abd carries directly.

Two follow-ups from R1's non-blockers, both worth surfacing individually.

Owning R1 non-blocker (a) — I was wrong twice.

• The HTML-comment concern reads a false positive that doesn't exist. stripHtmlComments in packages/lint/src/utils.ts is called before unbalanced_style_tags sees the source, so <style>foo</style><!-- </style> --> is stripped to <style>foo</style> before the count regex runs. Deleting the (nonexistent) fix and re-running the fixture still passes — Xuanru's experimental proof. I should have grep'd for existing stripping utilities in the linter package before proposing the fix; the #2811 review taught me the same class one release ago (stripJsStringLiterals sandwich blanking the true positive) and I didn't apply it.
• Worse, the specific one-liner I proposed — prepending <!--[\s\S]*?-->| to the alternation — is the exact hazard the existing stripHtmlComments was designed to avoid. Its docstring says so out loud: /<!--[\s\S]*?-->/ regex: that pattern backtracks O(n²) on inputs with many unterminated "<!--" (CodeQL js/polynomial-redos). stripHtmlCommentsOnce uses indexOf in a for(;;) loop deliberately, and the outer stripHtmlComments runs it to a fixpoint so <<!-- -->!-- … --> → <!-- … --> → stripped in a second pass. My proposed regex was single-pass AND non-linear-worst-case. So I would have reintroduced a CodeQL polynomial-redos finding while making the fixpoint-stripping worse. Both directions wrong.
• Class this belongs to: same as feedback_false_positive_fix_check_true_positive_shape_first from #2811 — proposed a pre-processing fix without first tracing where the input is already pre-processed. Saving a specific memory to widen the class: "before proposing a stripping fix, grep the surrounding package for existing strippers and read their design comments; the naïve regex form often exists as a rejected earlier version." The new fixture that pins the upstream stripping — unbalanced_style_tags does not flag inside stripped HTML comments in core.test.ts — is exactly the regression pin the class needed and would fail if stripHtmlComments ever regressed.

Owning R1 non-blocker (b) — half-refined.

• The display: none coverage gap is real but not routable to connector_detached either. Verified at source: connectorDetachmentIssues has if (attached(rendered.start) || attached(rendered.end)) continue; with the comment // Half-attached as drawn is allowed; only full render-miss proceeds. So a shaft with one endpoint on a live node and one on a display: none target — the concrete shape my R1 named — is attached === true on the live end, continue, not flagged. Combined with connector_orphan's connectorEndpointCandidates area filter dropping zero-rect targets, the shape is uncovered by both rules, not handed off between them.
• Xuanru's "not fixable at this layer" holds: display: none leaves no geometry, and neither layer has semantic knowledge of the intended composition graph to place a phantom node at the endpoint. Fixable only by staging (data-attribute annotation of intended endpoints, or a display: none → visibility: hidden convention). The PR-body correction ("hidden in place" + explicit scope paragraph) matches the mechanism, and the new fixture at layout-audit.browser.test.ts (hidden-in-place vs display:none boundary) pins the shape so a future rule extension has a discriminator.
• (c) is same class as (b); leaving it uncovered is consistent.

State at HEAD 137452aed: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA and any subsequent push voids it.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

:large_green_circle: a30c2c48c COMMENTED (re-pin under require_last_push_approval: true)

Interdiff 137452aed → a30c2c48c verified via git diff: one line in packages/cli/src/commands/layout-audit.browser.test.ts — const hiddenShaftStyles = [ → const hiddenShaftStyles: Array<Record<string, string>> = [. All rule and wiring files (core.ts, layout-audit.browser.js, layoutAudit.ts, checkBrowser.ts, utils.ts) byte-identical to R2. Mechanism carries.

Fix takes. hiddenShaftStyles is fed into a stubHiddenElement helper that reads properties by string key across four differently-shaped object literals ({ opacity: "0" }, { display: "none" }, { visibility: "hidden" }, plus the dash-hidden entry). Without the annotation, TS narrows to a union of the four literal shapes and the string-key read is a type error under strict mode. Array<Record<string, string>> widens the element type to a uniform CSS-property bag, which typechecks cleanly and doesn't change the runtime shape.

Owning two additional misses in R2.

• I did not check active CodeQL alerts on the changed rule file. OG traced three high-severity findings on packages/lint/src/rules/core.ts — js/bad-tag-filter (888), js/incomplete-multi-character-sanitization (889, 890) — that both R1 and R2 walked past silently. All three turned out to be false positives at HEAD (Xuanru independently ported the regexes and confirmed the tokenizer behavior; the alerts' state: fixed on refs/pull/3736/merge is verifiable via the code-scanning/alerts/{n}/instances endpoint where the top-level alerts/{n} endpoint returns state: null under my token). The correct discipline for a rule file adding regex-based HTML tokenization is: gh api repos/OWNER/REPO/code-scanning/alerts --jq '.[] | select(.most_recent_instance.location.path == "PATH_TO_FILE")' before posting the code verdict. Would have surfaced 888/889/890 in R1 and let me trace them against the mechanism in the same read as the rule itself, rather than after the fact.
• I posted R2 LGTM while Typecheck was red at 137452aed. My note said "CI at HEAD still settling" — but the Typecheck failure was on the exact fixture file I claimed I'd traced for discriminator honesty (hiddenShaftStyles in layout-audit.browser.test.ts), and running bun run typecheck on the reviewed diff would have caught it before my post. The general principle "code verdict separable from CI signoff" holds, but the specific case for fixtures I lean on for coverage claims is different: those fixtures need to compile, and a Typecheck-red fixture is not a fixture the discriminator argument can stand on. Waiting for Typecheck at minimum before posting is the discipline I'll apply going forward for fixture-heavy reviews.

Corroborating OG's independent verifications:

• context.ts:33 let source = stripHtmlComments(rawSource); inside buildLintContext — confirms the (a) analysis at source. Every rule reads context.source (post-strip) as its scan input, not context.rawSource. My "isolated component with invented inputs" experiment was against the wrong input, which is the same shape as OG's parallel miss on (a). Class-widening: feedback_run_the_experiment_when_it_is_cheap needs a sibling — "when the component sits under upstream pre-processing, the experiment must feed the component the pre-processed input, not the raw input the source file shows."
• CodeQL alert 888 instance on refs/pull/3736/merge reads state: fixed at commit fd5c29e3 via the instances endpoint — matches Xuanru's account of the alerts closing when the source.replace(/<script…/) pattern was replaced by the single-pass scan in 428a8fe3c. The Analyze (javascript-typescript) sub-check is still pending on this HEAD but no new findings expected given the tokenizer shape is unchanged from 428a8fe3c.

State at HEAD a30c2c48c: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA and any subsequent push voids it.

— Review by Rames D Jusso

[jrusso1020]

Independent read at a30c2c48c, additive to the three standing reviews — these are from my own pass, not a re-litigation. Everything below is verified at source or by execution, and I've said which. Typecheck's fixture bug is already fixed at this head, so it isn't in here.

1. connector_orphan is in PERSISTENCE_TIERED_CODES but not in framePositionKey

packages/cli/src/utils/layoutAudit.ts:353-358. The comment on :354 records exactly why the sibling code needs it:

connector_detached shares it: id-less paths collapse to one selector, so distinct lines need geometry in the key.

connector_orphan works on the same id-less <path> population, and was added to LayoutIssueCode (:26) and PERSISTENCE_TIERED_CODES (:206) — but not here.

Executed against the real collapseStaticLayoutIssues, three same-class shafts each orphaning at a single sample time:

connector_orphan   -> [{"sev":"warning","occ":3,"first":1,"last":1}]   1 finding, GATES
connector_detached -> [{"sev":"info","occ":1} x3]                      3 findings, demoted

Two failures in one direction each. N-1 real findings vanish — the survivor names only the first endpoint, since message isn't part of staticIssueKey. And a transient escapes demotion: applyPersistenceTier documents occurrences <= 1 as a complete test for "held under one frame", but here firstSeen === lastSeen with occurrences: 3, so a single-frame blip is never demoted to info and gates the run as a warning.

Fix is one clause on :355: || issue.code === "connector_orphan".

2. Nearest candidate wins, so a hidden layer beside a live node is a false positive

packages/cli/src/commands/layout-audit.browser.js:1436-1443:

for (const candidate of candidates) {
  const gap = distanceToRect(point, candidate.rect);
  if (gap > threshold) continue;
  if (best === null || gap < best.gap) best = { gap, candidate };
}
if (best !== null && !isVisibleElement(best.candidate.element)) dark.push(best.candidate);

This asks whether the nearest box is invisible. It never asks whether a visible box is also inside threshold. Executed, with a passing control (a genuinely opacity:0 endpoint yields 1 finding, so the harness discriminates):

an opacity:0 staged halo #glow sitting coincident with a fully visible #n2 that the shaft actually lands on → 1 finding, "Connector shaft is visible while its endpoint #glow is not on stage."

It also fires when the hidden layer is merely a few px nearer than the live node, and ties go to document order since gap < best.gap is strict. Staged glow/ring/focus layers around a node are a routine motion pattern, so I don't think this is exotic. It also means "0 false positives on the corpus" is a property of that corpus rather than of the rule.

Suggested shape: flag only when no visible candidate is within threshold.

3. hasAllowOverflowFlag(svg) was dropped from the existing rule, and the body says otherwise

layout-audit.browser.js:1296 — the diff is if (!isVisibleElement(svg) || hasAllowOverflowFlag(svg)) continue; → if (!isVisibleElement(svg)) continue;.

The body says "The existing CTM-paste path is untouched." That gate is the existing path. hasAllowOverflowFlag uses .closest(), so every SVG under a [data-layout-allow-overflow] ancestor was previously exempt from the shipped connector_detached rule and now isn't — data-layout-allow-overflow appears in ~28 registry blocks/components, including docs/catalog/blocks/hw-scribble-transition.mdx:103 which puts it directly on an <svg>. The function is still live at four other sites, so this isn't a cleanup.

This may well be the right call — it just isn't disclosed, and it removes a user opt-out. One sentence in the body would settle it.

4. The discriminator claim doesn't hold, and it's the #2801 shape

"Every guard in connector_orphan now fails the suite when disabled" — I mutated 21 guards across the rule and its two helpers, function-scoped, each landing verified by whole-file occurrence-count delta. Baseline 101/101 green. 8 killed, 13 survived. The two that 644990abd specifically hardened (shaft paint check, endpoint proximity threshold) genuinely are killed, so that narrower claim is true.

Three that matter:

• isConnectorPath is unpinned, and the test that looks like its pin is vacuous. it("does not orphan an unnamed decorative path on an empty frame") (layout-audit.browser.test.ts:1396) is #root + svg#decor + one path — and connectorEndpointCandidates skips anything matching element.closest("svg") (:1403), so the candidate set is empty and dark is empty whether or not the gate exists. Deleting isConnectorPath outright leaves the suite green. Name-matching (conn|arrow|edge|link|flow|wire, or any marker-*) is all that stands between this rule and every decorative <path> in a composition.
• The dash clause is never crossed. hiddenShaftStyles (:1432-1437) is opacity / display / visibility:hidden / visibility:collapse — four style clauses, no dash. Dropping shaftDashHidden(path) from :1438 leaves the suite green, though the body lists dash-hidden as one of the three ways a shaft is skipped.
• The 80px chord floor is unpinned in the permissive direction — both removing it and lowering it to 8 survive. The identical constant in connector_detached's markedMiss (:1332) is pinned. Same PR, one copy tested and one not.

Also, markedMiss's !userEndKey conjunct is never load-bearing in any fixture (!userStartKey only in combination), and it("skips one-ended decorative arrows when only one user endpoint attaches") (:1091) is actually carried by the earlier half-attach gate — its rendered end lands ~20px from #n2, inside the 32px threshold, so the comment above it about both rendered ends moving off anchors doesn't describe what the fixture does.

For the lint rule: it("reports an extra closer written as </style >") survives removal of the \s* it is named for — under the mutant it still returns severity: "error", but with the opposite message, and the test asserts only .severity. Adding expect(finding?.message).toContain("extra </style>") closes it.

---

Two smaller things, non-blocking: IGNORE_TAGS.has(path.tagName) in shaftIsPainted (:1364) can never be true, since querySelectorAll("path") only yields <path>; and neither new code is listed in skills/hyperframes-cli/references/lint-validate-inspect.md:53 where the persistence-tier codes are enumerated.

Requesting changes on 1 and 2 as code, 3 as a body correction, 4 as test coverage. The rules themselves read well — the endpoint resolution via getPointAtLength + getScreenCTM genuinely does fix the axis-aligned-bbox case, and the single-pass tokenizer in unbalanced_style_tags is the right shape. My concern is concentrated in what the suite proves versus what it's described as proving.

One caveat on my own work: every execution above is against the PR's own happy-dom harness with installConnectorGeometry stubbing the SVG geometry. Real Chromium may differ — notably getScreenCTM() returns null on a display:none path and the mock doesn't model that. I did not run a real browser.

— Rames

[james-russo-rames-d-jusso]

:large_green_circle: 8c973cea4 COMMENTED (re-pin under require_last_push_approval: true)

Interdiff a30c2c48c → 8c973cea4 verified via git diff: touches layoutAudit.ts (+4/-2), layout-audit.browser.js (+6/-1), layout-audit.browser.test.ts (+51/-11 fixture rewrite), layoutAudit.test.ts (+19/-0). All four findings from 5136179249 addressed at mechanism, plus a discriminator regression pin for finding 1. Rule and wiring in core.ts/core.test.ts/utils.ts/checkBrowser.ts byte-identical to R3.

Finding 1 — framePositionKey clause. layoutAudit.ts framePositionKey now returns geometry for frame_out_of_frame || connector_detached || connector_orphan. Comment names both codes. Corpus re-run confirms the mechanism was the whole point of the PR: connector_orphan climbs from 3 findings / 2 comps to 9 / 5, with the stagger-fade diagram (four shafts each outliving a different node) recovering three previously-collapsed findings. New layoutAudit.test.ts case keeps id-less connector findings apart by geometry, so each stays a single sample pins both codes against regression — three same-selector shafts at distinct rects → toHaveLength(3), each {severity: "info", occurrences: 1}. Would fail if the disjunction ever loses connector_orphan.

Finding 2 — nearest-candidate FP. layout-audit.browser.js endpoint loop now walks candidates once, sets attached = true and breaks the moment a visible candidate is within threshold; dark.push is gated on !attached && best !== null. That's the "flag only if no visible candidate is in range" predicate. Xuanru: "removing that check fails 8 tests." Discriminator fixture does not blame a staged halo that sits on a live node pins it — halo opacity: 0 at n2 position sharing a threshold-window with the live n2 box; before the fix the halo (invisible-nearest) wins, after the fix n2 (visible-in-range) short-circuits and the shaft is treated as attached.

Finding 3 — body drift. Body now reads: "data-layout-allow-overflow on a connector svg no longer suppresses connector_detached. That flag says a layer may bleed past the frame; it does not say its arrows may point at nothing, and both new codes score the same layers. Roughly 28 registry blocks carry the flag and were silently exempt." Matches the code. "0 false positives" line dropped as promised — replaced with "No false positive survived on this corpus, but that is a fact about these 47 compositions, not a property of the rule — review found a real one (a staged halo coincident with the live node it belongs to) that the corpus simply never exercised."

Finding 4 — discriminator hole. Old fixture does not orphan an unnamed decorative path on an empty frame was vacuous by construction — no real nodes anywhere in the DOM, so no candidates near either endpoint, so the shaftIsPainted / shaftDashHidden / isConnectorPath guards were never reached. Replaced by four fixtures anchored on orphanDom/orphanRects (real nodes at both endpoints):

• does not orphan an unnamed decorative path that runs between real nodes — no arrowhead, real n1/n2 at endpoints. Suite green.
• orphans that same path once it carries an arrowhead — same DOM + marker-end="url(#arrowhead)". Now expects toHaveLength(1). This pair IS the isConnectorPath discriminator: identical geometry, guard flip changes outcome.
• does not orphan a shaft still hidden behind its dash offset — strokeDasharray: "100", strokeDashoffset: "100" on the shaft, real nodes at endpoints so guards are reachable. Pins shaftDashHidden.
• does not blame a staged halo that sits on a live node — pins finding 2 as above.

Test-plan bullet in the body now enumerates the mutation sweep explicitly: shaft paint, endpoint proximity, connector shape, dash-offset skip, the visible-candidate check, and each of opacity / display / visibility-hidden / visibility-collapse each fail the suite when disabled. Every survivor from OG's 21-mutant baseline is now killed by a specific fixture that names the guard. That's the artifact shape I owe from R2 — mutation-testing evidence in the review, per-survivor fixture in the diff.

State at HEAD 8c973cea4: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: CHANGES_REQUESTED (OG's 5136179249 still standing pending re-review). CI at HEAD: Preflight/Lint/Fallow/SDK/Studio-load/Codex/CLI-npx-shims green; Typecheck, Build, CLI smoke, Preview parity, regression shards, Perf, Analyze pending. Given R3's own — I posted LGTM while Typecheck was red — I waited for Typecheck at minimum before posting this: Typecheck settled SUCCESS at HEAD. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA and any subsequent push voids it.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

:large_green_circle: f0857bbc7 COMMENTED (re-pin under require_last_push_approval: true)

Interdiff 8c973cea4 → f0857bbc7 verified via git diff: test-only, packages/cli/src/commands/layout-audit.browser.test.ts +104/-0. Rule and wiring byte-identical to R4. Mechanism carries.

Seven guards now killed by scoped fixtures. Each fixture flips outcome under a mutation of the named guard in isolation, and the DOM shape is close to the smallest that reaches the guard:

• an element inside the connector svg is never an endpoint — pins the descendant-of-connector-svg skip in the candidate filter. Fixture puts an <rect id="edge"> INSIDE <svg id="connectors">; without the filter, that rect would be picked as a hidden endpoint next to the shaft.
• a box with neither paint nor text is never an endpoint — pins the opaque-or-text filter. Fixture is a bare { opacity: "0" } box (no backgroundColor, no text); without the filter it would be picked as a hidden endpoint.
• a box below the area floor is never an endpoint — pins the 400px² area floor. Fixture is a 16×16 (256px²) box near the shaft end; without the floor it would be picked.
• a box larger than a stage fraction is never an endpoint — pins the 15%-of-stage ceiling. Fixture is a 1200×600 box on a 1920×1080 stage (~35% area); without the ceiling it would be picked.
• skips a connector layer the composition has taken off screen — pins the layer-level visibility gate. Fixture sets connectors: { display: "none" } on the parent svg; without the gate the rule would scan shafts inside a hidden layer.
• skips the arrowhead glyph living in defs — pins the defs/marker skip. Fixture rewrites the <path id="tip"> inside <marker> to look like a real connector (class="connector", marker-end, real chord); without the skip that tip path would be a second shaft. Assertion toHaveLength(1) + selector: "#path-input" pins that only the outer path counts.
• skips a marked stub too short to read as a link — pins the 80px chord floor. Fixture shortens the shaft to M 360 480 L 400 480 (40px chord); without the floor the stub would be counted.

Combined with the R4 fixtures (dash, decorative-vs-arrowhead pair, staged-halo, endpoint-hidden), the sweep is 13/13 killed with each survivor from OG's original 21-mutant baseline now anchored to a specific killing fixture in the diff. That's the stronger evidence shape than "sweep passed" — the survivors are the artifact.

The whole-file-replace mutation-testing gotcha Xuanru surfaced is worth naming. connectorAnchorRects and connectorEndpointCandidates carry identical filter lines. A whole-file string-replace mutant edits the FIRST occurrence, kills the mutant "cleanly," and leaves the guard under test intact — suite green, guard untouched, false coverage claim. Only detected by a grep-count sanity check after the replace. Scoping mutants by enclosing function closes it. Saving this as a sibling to my existing mutation-testing memory so the next multi-guard sweep I run doesn't hit the same trap.

State at HEAD f0857bbc7: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: CHANGES_REQUESTED (OG's 5136179249 still standing pending re-review). Waited for Typecheck before posting per R3 own: Typecheck settled SUCCESS at HEAD. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

:large_green_circle: 383e6e147 COMMENTED (re-pin under require_last_push_approval: true)

Interdiff f0857bbc7 → 383e6e147 verified via git diff: one line in packages/cli/src/commands/layout-audit.browser.test.ts reordering n2-halo and n2 in the halo fixture's DOM. Rule and wiring byte-identical to R5.

Why the reorder matters for discriminator honesty. With n2 first and n2-halo second in DOM (candidate iteration order), the nearest-candidate loop reaches n2 (visible, in range) first, so best is set to n2 — and best never gets displaced by the halo because if (best === null || gap < best.gap) uses strict < and the two rects overlap the same endpoint at the same distance. Under the pre-fix predicate if (best !== null && !isVisibleElement(best.candidate.element)), best is n2 (visible), so the pre-fix dark.push is skipped — no orphan under either code shape. Fixture passes under both, doesn't discriminate.

With n2-halo FIRST in DOM, halo becomes best (invisible), so under the pre-fix predicate the halo is dark.push'd — orphan under pre-fix, no orphan under post-fix (visible-check on n2 short-circuits attached = true; break). Now the fixture actually pins the visible-candidate check as load-bearing.

Xuanru's meta-lesson is worth pinning. "A mutant that isn't the code you replaced measures nothing, however many tests it kills." The first mutation (delete break) broke the rule in a direction the pre-fix code never took — attached permanently false, every endpoint with any candidate in range reported — so its 12 failures measured a novel breakage, not whether the visible-check is load-bearing for the halo case. Reverting to the actual pre-layout-audit.browser.js predicate (the one the diff literally replaced) is the sound mutation, and under that mutation the halo fixture only fails once the DOM order is fixed. Same failure family as R5's whole-file-replace trap, different mechanism: R5 was wrong LOCATION, R6 was wrong CONTENT — both a form of "the mutant didn't measure the code under test." Extending the parent mutation-testing memory to cover both.

Full sweep at HEAD: 13/13 killed under the true revert mutation, with shaftIsPainted (4 fixtures) and endpoint-proximity (8 fixtures) still independently discriminating so they weren't riding on the same illusion. All three coverage rounds where the failure was a coverage claim rather than the code are now closed at HEAD.

State at HEAD 383e6e147: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: CHANGES_REQUESTED (OG's 5136179249 still standing pending re-review). Waited for Typecheck per R3 own: Typecheck was gated out of this push by the Detect changes job (test-only, and the one-line reorder doesn't touch type surface); R5's green Typecheck at f0857bbc7 carries through — the type shape is identical. hyperframes OSS runs dismiss_stale=false / require_last_push_approval=true — approval needs to pin to this exact SHA.

— Review by Rames D Jusso

[jrusso1020]

Approving at 383e6e14734c2eac7a2043adadc32c4ae12a9570.

All four findings from review 5136179249 are resolved, and the coverage gap that opened up behind them is closed.

1 — connector_orphan missing from framePositionKey. Fixed at layoutAudit.ts:353-358, comment updated to name both codes. Pinned by keeps id-less connector findings apart by geometry — deleting the clause fails that test.

2 — nearest-candidate endpoint resolution. The loop now breaks on the first visible in-range candidate and only falls back to the nearest hidden one when none exists. Correct shape.

Coverage on this one took three heads, and the reason is worth recording. At a30c2c48c the guard was unpinned. At 8c973cea4 and f0857bbc7 it was still unpinned: restoring the hunk to its pre-fix form left the suite fully green (219/219, then 226/226). The halo fixture inserted the halo after n2, and since both boxes contain the endpoint at gap 0 and the tie-break is strict (gap < best.gap), the first candidate in document order won — the visible n2. Pre-fix code never blamed the halo either, so the test passed identically with and without the fix.

The earlier "fails 8 tests" measurement was real, but aimed at a different mutant: deleting the visibility check outright fails 12, of which 10 pin a guard that predates this PR.

At 383e6e147 the one-line fixture reorder closes it. Verified with layout-audit.browser.js restored byte-identical to a30c2c48c (sha256 b302add6…, git diff empty): exactly one test fails — does not blame a staged halo that sits on a live node — reporting Connector shaft is visible while its endpoint #n2-halo is not on stage. That is the defect itself. Genuinely discriminating now.

3 — hasAllowOverflowFlag dropped from the connector_detached gate. Resolved the right way. The drop was deliberate, so rather than restoring the flag the body now states the behaviour change and its rationale, and names the ~28 registry blocks affected. The "0 false positives" line is gone, replaced with an accurate statement that the number described the 47-composition corpus rather than the rule.

4 — the discriminator claim. 13/13, with each survivor from the original 21-mutant baseline anchored to a fixture where that guard alone decides the outcome.

CI. Tests on windows-latest failed at 8c973cea4 and f0857bbc7, which reads as a repeat signal but isn't — every underlying lane at those heads is cancelled, killed by the next push, and the aggregate is a require-all-lanes gate reporting on cancelled lanes. Typecheck is gated out at this head by Detect changes; it was green at f0857bbc7 and the delta since is a two-line string-literal swap inside a test file, so the carry-over holds. Long-running regression/perf lanes are still completing as I write, with no failures at this head.

One note for the next coverage-heavy review, since it cost three rounds here: define a mutant by byte-identity to the form the diff replaced, not by an ad-hoc breakage. Both misses on this PR — a whole-file replace hitting the wrong copy of an identical filter line, and a deleted break that broke the rule in a direction the old code never went — were mutants that killed tests while never touching the guard under test.

— Rames