fix(check): elongated pivot drift + counterfactual connector_detached

[xuanruli]

Summary

• rotation_pivot_drift: long-axis AABB stability (+ single-axis scale reject) so elongated rotators are no longer false negatives.
• connector_detached: redesigned around the documented defect — flag only when rendered endpoints (via CTM) miss anchors and user-space-as-screen endpoints would attach. Closed/near-zero user chords are skipped (not two-ended connectors).

This replaces the earlier spanning-host size gate (symptom patch). Decorative flow-lines, icon/filled arrows, and pre-reveal correctly-framed connectors fall out of the counterfactual naturally.

Deliberately out of scope

• Stale measure-while-offstage bugs (e.g. baking endpoints while icons are scale:0) — different root cause; needs a separate check if we want it.
• Anchors that only exist as sub-400px² pins / SVG-internal shapes (existing blind spot).

Prod replay

Re-ran 33 harvested Zephyr DC cases that previously emitted layout.connector_detached.

Caveat: hyperframes check skips browser/layout when lint has errors (duration: 0, samples: []). An earlier “0 findings on all 33” pass was partly invalid for 13 lint-short-circuited cases.

After forcing layout on those 13 (and reusing the 20 that already ran layout):

Result	Count
Layout ran	33
Clean (no connector_detached)	30
Still flags (counterfactual TPs)	3

Retained TPs (wrong SVG frame / user-as-screen would attach):

• 07de26a12a6747fabe2f451a — 3 findings (~194–211px)
• 0b11a3a3f3a1455e98e55f21 — 1 finding (~275px)
• 990a38923b4841e8a1fb26ca — 12 findings (hardcoded network coords)

Notable clean after forced layout (not arrow-gate FNs): b7d85dfe… (SVG↔SVG), 6af465e6… (borderline gap under old checker), plus the other short-circuited cases. Classic offset-SVG wrong-frame still covered by unit fixture.

Test plan

• Unit: foreign-frame #detached still flags; anchored sibling clean
• Unit: decorative arrow/flow (user-as-screen also misses) clean
• Unit: closed glyph (collapsed user chord) clean
• Unit: rendered-already-attached connector clean
• rotation_pivot_drift elongated spinner regression
• Prod batch /tmp/zephyr_cd_hunt → 30/33 clean; 3 retained counterfactual TPs after forced layout

[miguel-heygen]

Reviewed at exact head d21e8a942cf058523df146b69f62d9ce5a11f96e.

The CTM counterfactual is a strong way to distinguish a genuinely detached connector from one that is merely transformed, and moving the rotation test toward long-axis AABB stability correctly recognizes rigid elongated rotation. Two adversarial cases still create new false positives, though, and these findings are promoted into downstream repair work.

1. The connector counterfactual accepts a one-ended decorative arrow (layout-audit.browser.js:1311-1312). The current condition skips only when neither raw endpoint attaches, so one attached raw endpoint is enough to emit connector_detached. I reproduced this with a marker-ended path whose raw start is inside one anchor, raw end is in empty space, and whose SVG CTM translates both rendered endpoints away from all anchors: both rendered endpoints miss, the chord is long enough, and the detector flags it even though it is a one-ended decorative arrow. That contradicts the stated two-ended connector semantics and the PR claim that decorative arrows fall out naturally. Please require both raw endpoints to attach—preferably to distinct anchor candidates—before accepting the counterfactual, and add a regression where exactly one raw endpoint attaches while both rendered endpoints miss.

2. Two-axis scale/entrance motion now passes the rotation gate (checkPipeline.ts:614-636). The new rejection only catches the case where one axis stays within 1.15x while the other exceeds 1.6x; it no longer rejects substantial scaling on both axes. A direct detector call with (cx,cy,w,h,angle) samples (200,50,400,100,0), (170,100,340,200,45), (150,150,300,300,90) emits rotation_pivot_drift with 112px drift. This is an anchored scale/entrance shape, not pivot drift; the previous per-axis guard rejected it because the height ratio is 3x. Please preserve the elongated rigid-rotation gain while retaining a structural scale exclusion, and pin this two-axis case in the focused regression suite.

Exact-head CI is also not currently mergeable: Test, Producer: unit tests, and regression detection fail because this branch predates #2820 and does not schedule plan-v2-partial-color. Rebase onto merged main after #2820 and rerun the required lanes after the heuristic fixes.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The new heuristics correctly recover intended rigid rotations and CTM-detached connectors, but they also classify two-axis entrance scaling and one-ended decorative arrows as repairable defects; both false-positive paths need regression coverage before merge.

[james-russo-rames-d-jusso]

Reviewed at d21e8a942cf058523df146b69f62d9ce5a11f96e.

Two adjacent-but-independent detector improvements, both root-caused well. Elongated-rotator false-negative fixed by switching from both-axis stability to long-side stability + explicit single-axis-scale rejection (isPrimarilyAxisScale); counterfactual connector_detached narrows to the documented failure mode (rendered miss + user-as-screen would attach) and correctly falls decorative arrows / closed glyphs / pre-reveal correctly-framed connectors out of scope. Prod-replay methodology across 33 harvested Zephyr cases is unusually rigorous — 30/33 clean with the 3 retained TPs identified by hash and gap magnitude.

CI status. Producer: unit tests and regression / Detect changes are both red on the same Fixtures are neither scheduled nor excluded: plan-v2-partial-color failure that #2820 exists to fix (main is currently missing the schedule entry for the fixture #2814 added). Not related to this PR's content — will pass on rebase after #2820 merges.

Concerns

• Threshold conflation at layout-audit.browser.js:1288 — the same 32-px floor now gates two incompatible questions (attach-tolerance + minimum-chord). Legitimate short connectors between adjacent nodes could silently skip. See inline at :1302.
• Counterfactual gate is asymmetric — fires when both rendered endpoints miss but only at least one user endpoint attaches. See inline at :1311.

Notes

• pathUserEndpoints called twice per connector path (once directly at :1294, once inside pathScreenEndpoints at :1237). Micro-perf, inline at :1295.
• Prod-replay sanity: the 30 "clean after forced layout" cases include several that presumably flagged pre-#2819 as FPs (recall-improving good). Was there a manual pass confirming no pre-#2819 TP got silently de-flagged (new FN)? The Notable clean (not arrow-gate FNs) note in the PR body suggests yes — worth confirming in the thread if that's the shape of the check.
• [data-layout-allow-orbit] opt-out still respected browser-side at layout-audit.browser.js:1544 and :1711, so trimming the comment mention at checkPipeline.ts is fine.
• isRotationSizeStable and isPrimarilyAxisScale are both file-private with single callers each — the rewrite has no other consumers to break.
• The elongated-rotator test values (w:400,h:80 → w:340,h:340 → w:80,h:400 at 0°/45°/90°) are geometrically sound (400×cos45° + 80×sin45° ≈ 340) and the center drift (250 → 310 → 370) exceeds ROTATION_DRIFT_SIZE_FRACTION × longSide cleanly.

What I didn't verify

• Whether the renderedChord < threshold skip is actively problematic in prod data — depends on how many real connectors have <32px rendered chord. If Zephyr's 33 harvested cases have none, the threshold conflation is latent, not active.
• Downstream tooling parsing the connector_detached finding message text — grep-swept and none exists; all consumers discriminate on issue.code.

Nothing blocking. LGTM from my side once the #2820 rebase clears CI.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 The renderedChord < threshold skip conflates two incompatible thresholds. threshold at line 1288 is Math.max(32, Math.min(rootRect.width, rootRect.height) * 0.02) — a 32-px floor. It's used both here (minimum chord length to consider a path a real connector, targeting closed glyphs whose endpoints collapse to ~one point) and below at line 1308 (distanceToRect(point, rect) <= threshold — attachment tolerance, expected to be 20-40px for near-miss allowance).

Those want different scales:

• Closed-glyph rejection wants ~4–8px (any real chord that reduces to a single-point render is a filled shape).
• Attach-tolerance wants ~20-40px to admit near-misses as attached.

With the shared 32px floor, a legitimate short connector between adjacent nodes (e.g. two neighboring tiles ~30px apart) has a ~30px rendered chord and silently skips at this line before the counterfactual check ever runs. Whether this is actively hurting recall depends on whether Zephyr's dataset contains such connectors — the prod-replay validated 30/33 clean without surfacing this as an FN, so it may be latent for now. But the coupling is a landmine.

Suggested fix: introduce a separate MIN_CONNECTOR_CHORD_PX = 8 (or similar small constant) at the top of the function, use it here at line 1302, and keep threshold for attachment tolerance alone. — Review by Rames D Jusso

[xuanruli]

Valid — split out MIN_CONNECTOR_CHORD_PX = 8 for the closed-glyph gate; threshold remains attach-only.

[james-russo-rames-d-jusso]

🟠 The counterfactual gate is asymmetric between rendered and user checks. The full condition is:

if (attached(rendered.start) || attached(rendered.end)) continue;  // 1310
// Counterfactual: screen numbers pasted into `d` would land on anchors without CTM.
if (!attached(user.start) && !attached(user.end)) continue;         // 1313

= fires when both rendered endpoints miss AND at least one user endpoint attaches.

But the failure mode the check targets is "authors pasted screen coordinates into a d attribute inside a transformed SVG." In that failure mode, both endpoints would be pasted-from-screen — you'd expect BOTH user endpoints to land on anchors under the counterfactual, not just one. The current OR-gate on the user side admits cases where only one endpoint happens to sit near an anchor in user space (which can happen coincidentally for arrows aimed at a single node from an off-canvas origin, or for a connector whose author correctly authored one end and got the other wrong).

A stricter !attached(user.start) || !attached(user.end) (both must user-attach) is a closer fit to the documented failure mode. Trade-off is losing the partial-authoring cases as findings, which the prod-replay hash 07de26a12a… (3 findings ~194–211px) might or might not sit inside — if any of its 3 findings are single-user-attach, tightening to AND would drop them.

Empirically 30/33 clean says the current OR-gate isn't over-firing on the Zephyr sample. But the design choice deserves an explicit note in the comment (e.g. // OR gate: catches partial-paste cases; AND would reject them but keep authoring nuance.) so future readers understand the asymmetry is intentional rather than a copy-paste from the rendered check. — Review by Rames D Jusso

[xuanruli]

Valid — tightened to both user endpoints on distinct anchors (AND + distinct keys). One-ended decorative arrow regression added.

[james-russo-rames-d-jusso]

🟡 pathUserEndpoints is called twice per connector path per audit invocation — once directly at line 1294, once again inside pathScreenEndpoints at line 1237. Each invocation calls path.getTotalLength() (potentially expensive on complex paths — browsers may internally re-tessellate) and two path.getPointAtLength() calls. Micro-perf: two connector-paths per SVG × 100 SVGs per audit × 2 = 400 redundant DOM calls.

Cheap fix: pass user into pathScreenEndpoints as an already-computed arg, or inline the CTM transform into the caller once user is in hand:

const user = pathUserEndpoints(path);
if (!user) continue;
const rendered = user && path.getScreenCTM && svg.createSVGPoint
  ? applyCtm(user, path.getScreenCTM(), svg.createSVGPoint())
  : null;
if (!rendered) continue;

Or keep the two-function factoring and just take user as a parameter to pathScreenEndpoints. Non-blocking — audit invocations are rare enough that this won't show up in a flame graph. — Review by Rames D Jusso

[xuanruli]

Valid — pathScreenEndpoints(svg, path, user) now takes the already-computed user endpoints.

[xuanruli]

Addressed Miguel’s CHANGES_REQUESTED + James’s inline notes in fd88e2ef4 (rebased onto main / #2820):

• Connector: both user endpoints must attach to distinct anchors; MIN_CONNECTOR_CHORD_PX = 8 split from attach tolerance; pathScreenEndpoints takes precomputed user; one-ended arrow regression added.
• Rotation: isTwoAxisScale rejects two-axis entrance scale while keeping elongated rigid-spin TP; Miguel’s (400×100)→(300×300) case pinned.

[james-russo-rames-d-jusso]

Delta review of d21e8a94..fd88e2ef4 (R1 head → R2 head).

All three R1 concerns are addressed cleanly. The AND-gate fix also picks up Magi's one-ended decorative arrow finding, and the new isTwoAxisScale guard covers Magi's scale/entrance false-positive with the exact (200,50,400,100,0) → (170,100,340,200,45) → (150,150,300,300,90) regression pinned.

Verification

• Chord-floor split (layout-audit.browser.js:1288-1302) — MIN_CONNECTOR_CHORD_PX = 8 extracted, threshold documented as attach-only. Chord skip now uses the 8px floor, attach-tolerance stays at max(32, 2% root). Landmine gone.
• AND + distinct anchor gate (layout-audit.browser.js:1305-1324) — attachmentKey() returns a stable p{i} / c{i} per anchor; the paste-into-d check now requires both raw endpoints to key to distinct anchors. Stronger than "just AND" — same-anchor loops fall out. Matches Magi's ask.
• Single pathUserEndpoints call (layout-audit.browser.js:1293, pathScreenEndpoints(svg, path, user)) — no more double traversal per connector.
• Two-axis scale rejection (checkPipeline.ts:632-651) — isTwoAxisScale gates on maxArea/minArea > ROTATION_MAX_SIZE_RATIO AND max(first,last)/min(first,last) > ratio. Elongated rigid spin returns to its start area (mid-arc AABB peak collapses back), so first ≈ last keeps rigid rotators in scope; scale/entrance has first ≠ last and is rejected. The specific (400x100 → 340x200 → 300x300) case pinned in checkPipeline.rotationPivotDrift.test.ts:85-92.
• Regression coverage — one-ended decorative arrow test added at layout-audit.browser.test.ts:934-961; existing scaled-viewBox test adjusted to use distinct anchors (previously overlapping anchors would fail the new distinct-key gate — clean adjustment, not a semantic hole).

Rebase status

Now rebased past #2820 — Producer: unit tests, Test, Typecheck, regression all green on new head. Tests on windows-latest still pending as of this write.

Minor note (non-blocking)

isTwoAxisScale is conservative — it can suppress genuine pivot drift on a partial rigid-rotation arc (e.g. 0°→45° only, no return sweep) because start-to-end area diverges. That's on the safe side (loses recall, no new false positives), but worth mentioning if the finding surface ever seems to shrink after real-world rollout — you might want to also require angle-span ≥ some floor before the two-axis-scale gate applies.

LGTM from my side — no new blockers.

— Review by Rames D Jusso

[miguel-heygen]

Re-reviewed at exact head fd88e2ef4ed364456812de53f4497ec026563630.

R2 fixes the original concrete cases and all three Rames notes cleanly: both raw endpoints are now required, the monotonic two-axis entrance is rejected, the 8px chord floor is separated from attach tolerance, path geometry is computed once, the #2820 rebase is present, and exact-head CI is green. Two closely adjacent false-positive paths remain:

1. The distinct-anchor keys are not stable across attachment tiers (layout-audit.browser.js:1305-1319). The same DOM element is inserted into both painted and compact; attachmentKey returns pN when a point is inside its painted rect but cN when another point is just outside that same rect and within the compact tolerance. Those keys compare unequal even though they identify one anchor. I added a local adversarial regression with #n1 = [900,500..1060,660], raw endpoints (980,580) → (1080,580), an unrelated #n2, and CTM (80,227): start resolves to p0, end to c0, both rendered endpoints miss, and R2 emits connector_detached. This is still a one-ended decorative arrow around one real node. Please give each anchor one stable identity across containment and near-miss matching (for example, retain {element, rect} in both tiers or build one deduplicated anchor record), and pin this same-anchor/cross-tier case.

2. isTwoAxisScale only rejects monotonic endpoint-area change (checkPipeline.ts:629-643). A scale pulse that grows mid-trajectory and returns to its starting size has firstArea === lastArea, so the new guard treats it like rigid rotation even when the AABB dimensions are not rotation-consistent. The local samples (cx,cy,w,h,angle) = (200,50,400,100,0) → (170,150,340,300,45) → (200,50,400,100,90) produce a rotation_pivot_drift finding at 104px. This is newly admitted by the long-side heuristic; the pre-PR per-axis guard rejected the 3x height excursion. Please make the rigid-shape test structural rather than relying on first≈last area, and add a scale-pulse regression.

Focused head suites remain otherwise green: 77/77 browser-audit and 15/15 pivot tests. Each new case was added locally as one extra regression and failed independently exactly as described.

— Magi

Verdict: REQUEST CHANGES
Reasoning: R2 closes the reported examples, but duplicate anchor identities and endpoint-only area comparison still admit one-anchor arrows and returning scale pulses as repairable defects; both are new or directly adjacent false positives on an auto-fix input.

[miguel-heygen]

Re-reviewed at exact head 492b44835219613aebdd8043e984d42d3b80e244.

The connector side is now solid: both tiers return the same DOM element identity, the exact cross-tier regression is pinned, and the prior one-ended cases remain clean. The two requested R2 regressions pass, and the complete focused suites are 94/94 locally.

One root-cause blocker remains in the rotation gate: groupHasAxisSwap is an existential pair check, not a rigid-shape invariant across the trajectory (checkPipeline.ts:635-664). It fails in both directions:

• False positive with mixed rotate+scale. Samples (w,h,angle) = (400,100,0) → (100,400,90) → (400,300,180) contain one valid axis-swap pair, so groupHasAxisSwap returns true and the third scaled sample is ignored by the guard. With top-left-derived centers (200,50) → (50,200) → (200,150), exact-head detectRotationPivotDrift emits a 212px finding. The pre-PR per-axis guard rejected this scale excursion.
• False negative on the feature this PR adds. For a truly rigid 400×80 rectangle rotating only 0° → 20° → 40°, the mathematically expected AABBs are approximately 400×80 → 403.24×211.98 → 357.84×318.40. With a drifting center, exact head returns no finding: no sample pair is a 90°-class swap, and isPrimarilyAxisScale misclassifies the stable-width/growing-height AABB arc as scale. The angle spread is 40°, comfortably above the detector’s 20° spin floor.

Please validate every sample against one consistent underlying rigid rectangle using its angle/AABB geometry, rather than using the existence of any swapped pair as a proxy. If that reconstruction is too broad for this PR, splitting the connector fix from the elongated-rotation change would also keep the proven precision gain without weakening an auto-fix input.

Required CI is still settling; no failure is part of this finding.

— Magi

Verdict: REQUEST CHANGES
Reasoning: Stable connector identity is fixed, but the rotation classifier still admits scaled trajectories after one coincidental axis-swap and rejects legitimate partial-arc elongated rotation, so it does not yet establish the rigid-shape condition the new warning requires.

[miguel-heygen]

Re-reviewed at exact head 3fbf41b9080a216c70aced9f1f998015537cab23.

The angle-aware fixed-rectangle fit is the right structural replacement. Both R3 counterexamples now behave correctly (mixed rotate+scale → 0 findings, rigid 0°/20°/40° partial arc → 1 finding), the connector path remains correct, and the expanded focused suites pass 96/96 locally.

One narrow regression remains: an all-singular rigid trajectory has no estimator, so fitsOneRigidRectangle always returns false (checkPipeline.ts:631-669). unrotatedSizeFromSample deliberately returns null when |cos(2θ)| < 0.15; if every sample is a 45°-class orientation, the loop never validates anything. A rigid 400×80 rectangle sampled at 45° → 135° → 225° has the same exact AABB at every point (339.41×339.41). With centers (250,250) → (250,310) → (250,370), exact-head detectRotationPivotDrift returns [], even though this is a 180° elongated spin with 120px center drift. The pre-PR per-axis stability gate caught this case because both observed AABB dimensions are stable.

Please add a rank-deficient fallback for the no-invertible-reference case—for example, validate that all singular samples are near-square and share a consistent (elemW + elemH) projection/AABB within the same tolerance—and pin the 45°/135°/225° regression. Mixed singular + non-singular groups are already handled correctly by the current loop.

Required CI is still settling; no CI failure is part of this finding.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The structural model fixes the prior over- and under-admission, but it now drops rigid elongated rotations when the sampling phase lands entirely on the model’s singular 45° classes, regressing a case the original detector recognized.

[miguel-heygen]

Re-reviewed at exact head 188e14a218882d90e8179fdb0b9b7ca1ff3cf1e0.

R5 closes the remaining singular-phase regression without reopening the earlier false-positive paths. The fallback is narrowly gated to groups where every angle is in the rank-deficient 45° class, every projected AABB is near-square, and all projected dimensions agree within the existing rigidity tolerance. Mixed singular/non-singular groups continue through the structural fixed-rectangle estimator.

Verified locally:

• rigid elongated 45° → 135° → 225° trajectory: finding preserved;
• rigid 0° → 20° → 40° partial arc: finding preserved;
• two-axis scale/entrance: rejected;
• focused browser-audit + rotation suites: 97/97 green;
• targeted R5 adversarial trio: 3/3 green.

No remaining code blockers. Fresh exact-head CI is still settling, with all completed checks green at review time.

— Magi

Verdict: APPROVE
Reasoning: The all-singular fallback restores the lost rigid-rotation case using a constrained geometric proof, while the structural model and regressions continue to reject the previously identified scale and connector false positives.