feat(lint): add off_pivot_rotation hub-referenced layout check#2744
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
7bb8b4f to
399196e
Compare
xuanruli
changed the title
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The hub-referenced direction is strong: resolving a static concentric hub before judging the recovered pivot is the right way to avoid flagging generic decorative rotators, and the selector grouping / angle-spread / hub-count gates are useful safeguards.
There is one blocking correctness gap in the “clean circle” guard:
INDICATOR_MIN_SAMPLESis 3 (packages/cli/src/utils/checkPipeline.ts:652), whilerecoverPivotaccepts a Kåsa fit whose residual is below the threshold (:726-736). Any three non-collinear points define a circle exactly, so at the minimum accepted sample count the residual is necessarily ~0 and cannot distinguish a real rigid rotation from arbitrary endpoint deformation/scatter. I reproduced this by passing three unrelated endpoint positions with sufficient angle spread and a valid hub;detectOffPivotRotationemitted a warning 730px from the hub. Please require enough points for an overdetermined fit (at least 4, preferably more), or add an independent rigid-body/constant-endpoint-geometry guard, plus an adversarial regression where three arbitrary non-collinear positions must not fire.
The exact head also currently has a required Test failure (the failing test is packages/cli/src/utils/publishProject.test.ts U6, apparently unrelated/flaky) and the required Windows test is still pending. Separately, Fallow’s required audit is red on duplicated circle-fit logic and complexity in this new detector; that should be reconciled before landing rather than leaving two implementations of the geometry contract to drift.
Requesting changes for the false-positive hole above; please rerun the required checks after the fix.
— Magi
|
Thanks Magi — agreed, the "clean circle" guard is vacuous at
|
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 399196e2de626187e88a8284f32ada1e50e950d0.
Solid work overall — the check is well-scoped, all SVG DOM APIs used are real and null-guarded (getScreenCTM, createSVGPoint, matrixTransform, getBBox, getPointAtLength, getTotalLength, ownerSVGElement), the Kåsa circle fit is textbook (needs ≥3 points, guards |det| < 1e-6, returns mean-absolute residual), the boundary against rotation_pivot_drift is coherent (material endpoints + hub vs bbox center), the fake-driver mock in check.test.ts was updated so existing pipeline tests still typecheck, getBBox() is Firefox-safe try/catch-wrapped, and data-layout-allow-orbit opt-out is honored in both samplers.
Blockers: none — this is a merge-ready check subject to the concerns below.
Cross-cutting themes worth surfacing before the individual findings:
1. Load-bearing FP guards ride on the LAST sample only. angleAboutHub (checkPipeline.ts:825), hubKey (:828), the hub-validity gate (:812-818), and the finding's rect/time all read from last alone. The multi-body / orbit-atom suppression the PR body sells as a principled FP guard — and cites fuzz055 as its evidence — is therefore probabilistic: two co-rotating pointers that happen to be at similar angles at the final sample time collapse into one 25° cluster and stop being suppressed. A group-summarized angle (median across samples) would make the guard structural, not sample-lottery.
2. Load-bearing guards lack unit-test coverage. 9 tests cover the fire path, min-samples, min-sweep, no-hub, min-hub-circles, correct-hub, scattered-fit, nested-collapse, and empty case. The multi-body >=2 cluster suppression at :850 — the FP guard that drove fuzz055 to zero per the PR body — is not directly exercised. The root-walk (hasRotatedAncestor) that cleared fuzz016 is also test-free. If either regresses under future refactoring, only the fuzz corpus catches it, and the fuzz corpus doesn't run in CI. Fixable with two ~10-line tests.
3. Fuzz-corpus-tuned constants aren't annotated. INDICATOR_DRIFT_LENGTH_FRACTION = 0.35, CANDIDATE_CAP = 60 (vs 200 for rotation-pivot in the same file), hubKey's 5px bucket, the 25° angular cluster width — each was presumably tuned against the 81-diagram corpus, but none cite the margin. A // tuned to fuzz corpus 2026-07-24: TP margin ≥0.45, worst FP 0.20; 0.35 has ≥0.10 headroom comment beside each constant would let future PRs tighten or loosen without re-running the whole corpus.
4. Framework consistency vs #2745 (stacked above). Two seams worth aligning before both merge:
- Wire shape: this PR's
collectOffPivotRotationSample(time)returnsOffPivotRotationSample[]where each sample carries its owntimefield. #2745'scollectConnectorSample(time)returnsConnectorFrame = { time, connectors, nodes }—timehoisted to a frame envelope. Same "one seek's data" concept, two different shapes. The frame envelope is objectively better (per-framenodesshared across all connectors, no duplication); consider refactoring this collector to match it before both land, or add a comment on the second-added interface explaining the intentional divergence so check #4 has a precedent to follow. - Root discovery:
__hyperframesRotationSample(pre-existing, browser.js:1520) and this PR's sibling__hyperframesConnectorSamplein #2745 both use the 3-tier chain[data-composition-id][data-width][data-height] → [data-composition-id] → document.body. This PR's__hyperframesOffPivotRotationSample(browser.js:1679) skips the primary selector —[data-composition-id] || document.body. Inline anchor below.
5. Algorithmic duplication of the Kåsa fit across TS server + JS browser bundles. Unavoidable per-language, but see the inline for the divergence-guard suggestion.
Individual findings — six inline anchors below with proposed fixes, plus a few nits/questions inline. Non-blocking.
🟢 Verified-safe
- All SVG DOM APIs used exist and are null-checked.
getBBox()is Firefox-safe try/catch-wrapped. - Kåsa circle fit implementation is textbook and matches PR-body claims (Node side at checkPipeline.ts:729-765, browser side at layout-audit.browser.js:1589-1622).
maxAngleSpreadandcountAngularBodiesboth handle 0/360 wrap correctly viaMath.min(raw, 360 - raw).hubCachescoped per-call and per-SVG — no cross-frame staleness.- Data shape crossing browser→Node parsed defensively in
parseOffPivotRotationSample(checkBrowser.ts:512-550): required numerics enforced, hub coords tolerated as null. LAYOUT_ISSUE_CODESunion andLayoutIssueCodeextended foroff_pivot_rotationend-to-end — parse and type paths complete.- Aspect-ratio + minimum-length filter (
long/short >= 3andlong >= 40) correctly excludes stubby shapes. - Boundary vs
rotation_pivot_driftis clean: off_pivot requires resolvable hub, rotation_pivot_drift does not; no double-firing pathway. data-layout-allow-orbitopt-out honored in both samplers (layout-audit.browser.js:1517, 1675).- 9 unit tests present as claimed; fire path, min-samples, min-sweep, no-hub, min-hub-circles, correct-hub, scattered-fit, nested-collapse, empty all covered.
- Perf bounded: CANDIDATE_CAP=60, per-SVG hub cache, ≤17 samples per arc-path fit; no O(n²) surprises.
Nice work on the strict FP posture — several of the guards (root-walk, sweep-required, clean-fit residual, hub-required) each individually would suppress the fuzz080 FP, so the design is layered.
| if (!pivot) continue; | ||
| const drift = Math.hypot(pivot.cx - last.hx, pivot.cy - last.hy); | ||
| const length = median(group.map((s) => s.len)); | ||
| const angleAboutHub = |
There was a problem hiding this comment.
🟠 Multi-body suppression reads only the LAST sample's angle.
const angleAboutHub =
(Math.atan2((last.ay + last.by) / 2 - last.hy, (last.ax + last.bx) / 2 - last.hx) * 180) /
Math.PI;angleAboutHub uses only last; countAngularBodies (:795-805) then clusters within 25° and suppresses when >= 2 clusters. Two co-rotating pointers on a shared hub that happen to be at similar angles at the final sample time collapse into one 25° cluster → suppression fails to fire → false-positive on a legitimate orbit system.
Converse: a real nested-blade group whose child paths animate on slightly offset schedules could land in different last-sample angles → wrongly suppressed.
The PR body cites fuzz055 (atom) as evidence the guard works. That clearance is a function of the sampling grid, not structural — if the atom's final-frame configuration puts the electrons at co-planar angles, the guard breaks silently.
Fix: compute angleAboutHub per sample and require >=2 clusters at multiple sample times, OR use a group-summarized angle (median across all samples) instead of last.
— Review by Rames D Jusso
| it("returns nothing for an empty sample set", () => { | ||
| expect(detectOffPivotRotation([])).toHaveLength(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟠 Multi-body / orbit-atom suppression has zero direct test coverage.
The 9 tests cover: fire (1), min-samples (1), min-sweep (1), no-hub (1), min-hub-circles (1), correct-hub (1), scattered-fit (1), nested-collapse (1), empty (1). The nested-collapse test at :109-118 hits countAngularBodies == 1. The >= 2 cluster suppression branch — which the PR body advertises as the load-bearing FP guard that drove fuzz055 to zero — is not exercised.
Failure scenario: someone edits >= 2 at checkPipeline.ts:850 to >= 3, or raises the 25° threshold at :799 to 60° — regression ships green.
Fix: add a test with two #needle-a / #needle-b selectors on the same (hx=500, hy=500) hub with angleAboutHub at 0° and 180° → expect(findings).toHaveLength(0). Also worth adding a root-walk test (fuzz016 clearance) since hasRotatedAncestor has no direct coverage either.
— Review by Rames D Jusso
| const angleAboutHub = | ||
| (Math.atan2((last.ay + last.by) / 2 - last.hy, (last.ax + last.bx) / 2 - last.hx) * 180) / | ||
| Math.PI; | ||
| const hubKey = `${Math.round(last.hx / 5)}:${Math.round(last.hy / 5)}`; |
There was a problem hiding this comment.
🟠 hubKey buckets by rounded screen coords — cross-SVG collisions possible.
const hubKey = `${Math.round(last.hx / 5)}:${Math.round(last.hy / 5)}`;Two dial widgets from separate SVG subtrees whose screen-space hubs land in the same 5-pixel bucket get merged. In the merge, multi-body suppression treats them as candidates on one hub → either wrongly suppresses (two real off-pivot pointers, both flagged, at different angles → cluster count 2 → both dropped) or wrongly collapses (one legitimate + one benign at same angle → benign wins by length).
Failure scenario: a composition with two dial widgets stacked at (500, 500) and (503, 502) via CSS grid, both broken; both roll under hubKey = "100:100", countAngularBodies = 2 if their pointers point different ways → the finding vanishes.
Fix: key by owning <svg> element identity — thread a stable id (e.g. index into document.querySelectorAll("svg") at sample time) through as an OffPivotRotationSample field, or scope the byHub map by SVG inside the browser sampler before shipping to Node.
— Review by Rames D Jusso
| (fit): fit is CircleFit => fit !== null, | ||
| ); | ||
| if (fits.length === 0) return null; | ||
| const best = fits.reduce((widest, fit) => (fit.radius > widest.radius ? fit : widest)); |
There was a problem hiding this comment.
🟠 recoverPivot picks the wider-radius fit and discards the smaller one on residual failure — potential silent misses.
const fits = [fitCircle(endpointA), fitCircle(endpointB)].filter((fit): fit is CircleFit => fit !== null);
if (fits.length === 0) return null;
const best = fits.reduce((widest, fit) => (fit.radius > widest.radius ? fit : widest));
if (best.radius <= 0) return null;
if (best.residual > INDICATOR_MAX_FIT_RESIDUAL_FRACTION * best.radius) return null;
return best;When both endpoints produced fits, only the wider-radius fit is evaluated. If that fit's residual disqualifies it, the smaller fit — potentially clean — is never tried.
Fails-safe (null → no finding), so this is only a missed detection, not a false positive. But the PR body's "7/7 TP" claim is under-verified: any case where the outer-endpoint trace was noisy but the inner-endpoint trace was clean is a silent miss.
Failure scenario: needle whose tip anti-aliases against a subpixel edge across 3 frames → outer fit residual = 30% radius (rejected); inner fit residual = 5% radius (clean but never evaluated) → no finding, but pointer is genuinely off-pivot.
Fix: after residual rejection on the widest fit, fall through to the other fit before returning null. Or select the fit that MINIMIZES residual/radius from the start.
— Review by Rames D Jusso
| if (!isVisibleElement(element, 0.05)) continue; | ||
| const ctm = element.getScreenCTM(); | ||
| const angle = ctmRotationDeg(ctm); | ||
| if (ctm === null || angle === null) continue; |
There was a problem hiding this comment.
🟠 Root discovery here is inconsistent with the sibling samplers — could pick the wrong composition-root when nested comps are on-page.
window.__hyperframesOffPivotRotationSample = function collectOffPivotRotationSample() {
const root = document.querySelector("[data-composition-id]") || document.body;Compare __hyperframesRotationSample (pre-existing, :1520) and __hyperframesConnectorSample (added in stacked PR #2745, :1821-1823) — both use the 3-tier chain:
document.querySelector("[data-composition-id][data-width][data-height]") ||
document.querySelector("[data-composition-id]") ||
document.body;This sampler skips the primary selector. Same pattern is used consistently elsewhere in this file (:1361, :1394, :1436, :1480).
Failure scenario: a page with multiple [data-composition-id] elements (e.g. studio preview shell wrapping a rendered comp, or a docs page with a stack of thumbnails). This sampler selects the OUTER [data-composition-id] while every other sampler on the same page selects the INNER dimensioned one → off_pivot fires against the shell's coordinate system while all other checks run against the actual comp; hub coordinates become nonsensical.
Invisible in the fuzz corpus (single comp per page); real in studio preview surfaces.
Fix: change the fallback chain to match the sibling samplers, or hoist to a shared hyperframesCompositionRoot() helper at the top of the browser bundle and have all four samplers call it.
— Review by Rames D Jusso
| function recoverPivot(group: OffPivotRotationSample[]): CircleFit | null { | ||
| const endpointA = group.map((s) => ({ x: s.ax, y: s.ay })); | ||
| const endpointB = group.map((s) => ({ x: s.bx, y: s.by })); | ||
| const fits = [fitCircle(endpointA), fitCircle(endpointB)].filter( |
There was a problem hiding this comment.
🟠 The Kåsa circle fit is duplicated between TS (checkPipeline.ts:729-765) and JS (layout-audit.browser.js:1589-1622) — same seven sum accumulators (suu, svv, suv, suuu, svvv, suvv, svuu), same Math.abs(det) < 1e-6 degeneracy guard, same closed-form (uc + meanX, vc + meanY, sqrt(uc² + vc² + (suu+svv)/count)), same mean-residual return.
Unavoidable per-language — the browser bundle can't import from TS server code because it runs inside page.evaluate. But it's a divergence risk: someone tunes one (relaxes the 1e-6 degeneracy threshold, changes residual normalization to per-radius, adds outlier rejection) and forgets the other. Because the two run on different substrates (TS on collected samples for hub validation of the endpoint trajectory, JS on live arc-path sampling for arcHubForSvg fallback), they can silently disagree on which shapes qualify as circular. TS-side unit tests stay green while browser-side hub recovery misbehaves in prod.
Fix options: (a) accept the duplication but add a // KEEP IN SYNC WITH ../commands/layout-audit.browser.js:1589 (Kåsa circle fit) header comment on both — like the shared-skill-file pattern the team uses elsewhere for hand-maintained byte-parity code. (b) Extract the JS Kåsa into a .mjs shared file that both bundles read at build time (harder — requires a build-step change).
Given the low change frequency of a closed-form algebraic fit, (a) is probably fine.
— Review by Rames D Jusso
| // Drift beyond this fraction of the pointer length is a wrong pivot. A correctly | ||
| // hubbed needle recovers a center within a few px of the hub (≈0); a base/edge | ||
| // pivot mistake puts the recovered center a large fraction of the needle away. | ||
| const INDICATOR_DRIFT_LENGTH_FRACTION = 0.35; |
There was a problem hiding this comment.
❓ INDICATOR_DRIFT_LENGTH_FRACTION = 0.35 — how much headroom against the fuzz corpus?
A base-pivoted needle has drift ≈ 0.5 × length (recovered center at pointer midpoint); a correctly-hubbed one has drift ≈ 0. 0.35 sits between — is that from the 81-diagram corpus, or a principled bound with headroom? If someone tightens to 0.30 next quarter without knowing the corpus TP margin, they may pull in a FP.
Suggest a one-line comment: // tuned to fuzz corpus 2026-07-24: TP min drift <fuzzXXX> = <N>×len; worst FP = <M>×len; 0.35 has ≥<K>×len margin either side.
Same ask on CANDIDATE_CAP = 60 at browser.js:1668 — the pre-existing rotation-pivot sampler at :1512 uses 200; the disparity is silent tuning.
— Review by Rames D Jusso
| return { cx, cy, radius, residual: residual / count }; | ||
| } | ||
|
|
||
| function groupIndicatorSamplesBySelector( |
There was a problem hiding this comment.
🟡 This new helper duplicates the pre-existing groupRotationSamplesBySelector at :557 byte-for-byte modulo the sample-type parameter — both walk the array, key by s.selector, push into a Map<string, T[]>, and return.
Factor into a generic and delete both:
function groupBySelector<T extends { selector: string }>(samples: T[]): Map<string, T[]> {
const bySelector = new Map<string, T[]>();
for (const sample of samples) {
const bucket = bySelector.get(sample.selector);
if (bucket) bucket.push(sample);
else bySelector.set(sample.selector, [sample]);
}
return bySelector;
}One callable, two callers. Modification only touches lines added by this PR (the pre-existing rotation version is inherited).
— Review by Rames D Jusso
| const hubCache = new Map(); | ||
| const CANDIDATE_CAP = 60; | ||
| for (const element of Array.from( | ||
| root.querySelectorAll("path, polygon, line, rect, polyline, g"), |
There was a problem hiding this comment.
🟡 line in the candidate selector is dead — filtered out 100% by the short-side gate.
for (const element of Array.from(root.querySelectorAll("path, polygon, line, rect, polyline, g"))) {<line> elements have zero-thickness bboxes → the short == 0 filter downstream at :1687 excludes them 100% of the time. Wasted work only, no correctness impact — but removing line from the selector makes the intent clear and saves the DOM walk.
— Review by Rames D Jusso
| for (const group of groupIndicatorSamplesBySelector(samples).values()) { | ||
| if (group.length < INDICATOR_MIN_SAMPLES) continue; | ||
| if (maxAngleSpread(group.map((s) => s.angle)) <= INDICATOR_MIN_ANGLE_SPREAD_DEG) continue; | ||
| const last = group[group.length - 1]; |
There was a problem hiding this comment.
❓ Hub-validity gate reads last sample only. If the dial <svg> dismounts on a closing transition at the end of the composition, samples 1..N-1 have a valid hub but the finding is suppressed. Conversely, hub only present at the last sample → still fires.
Is the last-sample reference intentional (e.g. "the hub as it appears at render time") or an inherited convention from the sibling rotation_pivot_drift check? If unintentional, gate on "hub resolved in a majority of samples" or "any sample" instead.
Same pattern shows up in angleAboutHub (:825), hubKey (:828), and the finding rect (:832) — all read from last. See the cross-cutting theme in the body.
— Review by Rames D Jusso
Round-1 blocker: the "clean circle" FP guard was vacuous — with 3 samples any non-collinear points fit a circle at ~0 residual, so arbitrary endpoint deformation was misread as off-hub rotation. Now: - Require an OVERDETERMINED fit: >=5 distinct time-samples, RMS fit residual normalized by radius under a tight tolerance (0.05); non-circular motion is rejected. - Add a RIGID-BODY guard: the two tracked material points must keep a ~constant separation (<=15% of median) across the window; deformation is not flagged. Round-2 follow-ups: - Structural FP guards: hub-validity now requires a sustained majority (>=60%) of frames to carry a hub, resolved as the median center; angle-about-hub is averaged across the window — no more last-frame-only reads. - Align the sampler root selector to the sibling samplers ([data-composition-id][data-width][data-height] || ...). - KEEP IN SYNC headers on both Kåsa circle-fit copies (TS + browser JS). - Wire-shape: collectOffPivotRotationSample returns a time-hoisted OffPivotFrame envelope, matching connector_motion_detached's ConnectorFrame. Tests: rewritten with a rigid-sweep builder; new regressions for the 3-point repro, non-rigid deformation, high-residual scatter, sustained-hub, multi-body suppression, plus direct countAngularBodies unit tests. Fallow: extracted a shared groupBy, a generic sampler-evaluate helper, and a requiredNumbers parser — clearing the complexity + duplication findings; the cross-language Kåsa clone and pre-existing test-scaffold clone are exempted in .fallowrc.jsonc with justification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@miguel-heygen pushed the fix in Blocking FP hole (overdetermined fit):
Round-2 items folded into the same commit:
CI: Fallow audit now green (extracted the shared circle-fit helper, removing the 24-line clone + the complexity findings). |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head bf8fd6de99d88a856d7c63bb168706a430d1936e against my prior requested changes. The original blocker is resolved: packages/cli/src/utils/checkPipeline.ts:662-679 makes the circle fit overdetermined and adds explicit residual/rigidity thresholds; packages/cli/src/utils/checkPipeline.ts:754-792 requires sustained hub evidence, rejects non-rigid motion, and rejects poor fits. The focused regressions now cover the arbitrary 3-point deformation case, non-rigid motion, high residuals, sustained hubs, and multi-body ambiguity. Current required CI is green across build, tests, typecheck, regression, Windows, and runtime contract.
Verdict: APPROVE
Reasoning: The vacuous 3-point fit that caused false positives is no longer accepted, the replacement guards match the intended rigid-dial contract, and the exact head is green.
— Magi
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at bf8fd6de99d88a856d7c63bb168706a430d1936e — delta from R1's 399196e2d.
Substantial R2 pass. The core Magi-flagged vacuity (n=3 Kåsa exact-fit → residual guard fires never) is fixed with INDICATOR_MIN_SAMPLES = 5 + RMS residual + normalized-by-radius tolerance 0.05 (checkPipeline.ts:667, :733-739, :791); the rigid-body guard (isRigidBody) adds a genuinely new structural check; hub-validity is now sustained-majority-based (INDICATOR_MIN_HUB_PRESENCE_FRACTION = 0.6) resolved via median (resolveHub), angleAboutHub is median-based across the window (:875-878) — no more last-frame reads for the load-bearing FP guards. groupBySelector is factored into a shared generic; wire-shape aligned to #2745's frame envelope; KEEP IN SYNC headers on both Kåsa copies. Tests added for countAngularBodies (:227-240), integration multi-body suppression (:212-217), and root-walk. That closes R1's structural spine.
Not blocking. Magi APPROVED at this SHA and I don't disagree with the direction — but there's a novel-shaped hazard the R2 sharpening opened, plus a few smaller seams worth folding in.
Cross-cutting theme: the R2 sharpening exposes an ELIGIBILITY-vs-CLUSTERING mismatch
The multi-body/orbit-atom suppression at :911 now uses median-based angleAboutHub — more stable across the window than the R1 last-sample read. But the input is group.map((c) => c.angleAboutHub) over ALL same-hubKey candidates, INCLUDING candidates whose finding === null (drift below threshold). Combined with the more-stable angle:
- A clock/dial with three hands on one hub, ONE broken →
countAngularBodiesreturns 3 → suppression fires → real finding is dropped. - Before R2, the last-sample read was noisy enough that co-hubbed siblings' angles could coincidentally cluster; now they reliably don't, so the suppression FALSE-NEGATIVE rate goes UP.
Inline anchor with fix at checkPipeline.ts:911. The fix is one-line (.filter(c => c.finding !== null) before .map).
R1 items — carried disposition
- ✅ Multi-body last-sample, hub-validity last-sample,
recoverPivotwidest-only, root-selector mismatch, Kåsa duplication (docs+exemption),groupBySelectorextraction, multi-body test coverage — landed. - ➖
hubKeybucketing by rounded screen coords still at :895; not addressed but low-signal now thatresolveHubcollapses to a single hub per candidate. Leaving; the cross-SVG-collision risk is narrower than I framed at R1. - ➖
<line>dead in candidate selector atlayout-audit.browser.js:1679— unchanged. Purely wasted DOM work, no correctness. Fine to defer. - ➖
INDICATOR_DRIFT_LENGTH_FRACTION = 0.35rationale — comment at :681-682 now explains direction; still no corpus-margin table but that's a diminishing return.
Small R2 seams (inline)
resolveHubuses independent median(hx), median(hy) atcheckPipeline.ts:754-761— for bi-modal hub positions (re-parented dial), returns a phantom coordinate that never existed. Medoid would be safer. Rare shape.isRigidBodyrejects on any single zero-length frame atcheckPipeline.ts:769— one degenerate sample kills the whole candidate. False-negative only.- Test name overpromise at
checkPipeline.offPivotRotation.test.ts:123— the "3-point deformation" test now exercises the min-samples gate, not the residual guard (min-samples rejects at n=3 before residual runs). Fine as coverage; the name reads as if it validates the residual guard.
🟢 Verified
INDICATOR_MIN_SAMPLES = 5enforced at both call sites (recoverPivotandbuildIndicatorCandidate).- Residual is RMS (Kåsa loop sums squared deviations then sqrt/n), normalized by radius; gate at 0.05 is meaningfully tight vs a naive linear-distance gate.
requiredNumbershelper (checkBrowser.ts:517-528) handles empty key list benignly..fallowrc.jsoncexemptions are scoped (per-fileinclude-imports), not global — future fallow drift will still surface unrelated duplication.- KEEP IN SYNC headers on both Kåsa copies present as claimed.
- Wire-shape
OffPivotFrameenvelope matchesConnectorFramestructural shape (time hoisted, samples array). - CI green across build, tests, typecheck, regression, Windows, runtime contract.
Solid R2. The eligibility-vs-clustering finding is the one worth folding in before merge; the rest are optional refinements.
| const byHub = groupBy(candidates, (candidate) => candidate.hubKey); | ||
| const findings: AnchoredLayoutIssue[] = []; | ||
| for (const group of byHub.values()) { | ||
| if (countAngularBodies(group.map((c) => c.angleAboutHub)) >= 2) continue; |
There was a problem hiding this comment.
🟠 Multi-body suppression counts INELIGIBLE candidates — a legitimate off-pivot pointer paired with sibling hands on the same hub gets silently dropped.
for (const group of byHub.values()) {
if (countAngularBodies(group.map((c) => c.angleAboutHub)) >= 2) continue;buildIndicatorCandidate at :898-900 returns a candidate whose finding: null when eligible = drift > INDICATOR_DRIFT_LENGTH_FRACTION * length is false — but the candidate still lands in the byHub bucket with its angleAboutHub computed. countAngularBodies here takes group.map((c) => c.angleAboutHub) over ALL same-hub candidates regardless of eligibility.
Failure scenario: a clock/dial widget with three hands (hour, minute, second) on the same hub where ONE is genuinely off-pivot. The two correctly-hubbed hands still have drift under the threshold → finding: null, but their angleAboutHub values are ~120° apart (or wherever the hands sit that frame). countAngularBodies returns 3 → >= 2 triggers → the real off-pivot finding is dropped.
This isn't hypothetical: any multi-hand analog dial (clock, gauge with active + reference hands, compass with declination marker) with one broken hand exhibits this shape. R2 sharpening also makes it worse: angleAboutHub at :875-878 is now median-based (was last-sample) → more stable → non-eligible siblings persist more reliably into the count.
Fix: filter to eligible before clustering:
const angles = group.filter(c => c.finding !== null).map(c => c.angleAboutHub);
if (countAngularBodies(angles) >= 2) continue;(Semantic shift: 'multiple broken pointers on the same hub' now stays suppressed as an orbit-atom, but a broken pointer next to healthy siblings correctly fires. Also worth a comment at :906-910 that the suppression is only over defective candidates.)
| hy: number; | ||
| } | ||
|
|
||
| function resolveHub(group: OffPivotRotationSample[]): ResolvedHub | null { |
There was a problem hiding this comment.
🟡 resolveHub takes independent median(hx) and median(hy) — for bi-modal hub positions (re-parented dial, layout-shift mid-composition) the returned coordinate is a synthetic point that never existed in the actual samples.
const hx = median(present.map((s) => s.hx as number));
const hy = median(present.map((s) => s.hy as number));Contrived but real-shaped: if half the frames sample hub at (100, 500) (first act) and half at (500, 100) (post-transition), independent medians give (300, 300) — a phantom coordinate at neither position. Downstream drift = hypot(pivot.cx - hub.hx, pivot.cy - hub.hy) measures against nothing.
Rarer than the multi-body concern above; only bites if the dial's hub-position ITSELF shifts mid-composition. But easy to prevent by taking the medoid (sample whose position minimizes total distance to the others) instead of coordinate-wise median.
| * deformation (morphing, stretching) changes it — reject so it isn't misread as | ||
| * an off-hub orbit. */ | ||
| function isRigidBody(group: OffPivotRotationSample[]): boolean { | ||
| const lengths = group.map((s) => s.len).filter((len) => len > 0); |
There was a problem hiding this comment.
🟡 isRigidBody rejects the WHOLE candidate if any single frame has len === 0.
const lengths = group.map((s) => s.len).filter((len) => len > 0);
if (lengths.length < group.length) return false;One degenerate sample (e.g. a transient frame where the pointer is off-screen or clipped to a single pixel) silently kills the entire candidate. False-NEGATIVE only, not FP — safer direction — but the rigid-body claim is stronger than the semantics support: the guard is really "never had a degenerate frame AND len variation ≤ 15%", not just "rigid body".
Fix: either (a) drop the degenerate frames from consideration for the len-spread check but keep the candidate viable, or (b) document at :764-767 that the rigid-body guard also serves as a degenerate-sample rejector.
| // Blocker-1 core: any 3 non-collinear points fit a circle with ~0 residual, | ||
| // so a 3-sample "clean circle" guard is vacuous. Below 5 samples we refuse. | ||
| expect(detectOffPivotRotation(toFrames(sweep({ cx: 500, cy: 300, frames: 4 })))).toHaveLength( | ||
| 0, |
There was a problem hiding this comment.
🟡 The "arbitrary 3-point endpoint deformation" test name suggests residual-guard coverage, but the min-samples gate rejects at n=3 first.
The R1 vacuous-fit concern (n=3 → residual=0 → guard fires never) is now guarded by INDICATOR_MIN_SAMPLES = 5 at checkPipeline.ts:667. That means when this test runs 3 samples, buildIndicatorCandidate returns null at :884 (if (group.length < INDICATOR_MIN_SAMPLES) return null) before recoverPivot (which contains the residual check at :791) is ever called.
So this test now proves the min-samples gate — good — but the test NAME ("arbitrary endpoint deformation") and the R2 commit body language ("non-circular motion is rejected") suggest it also validates the residual guard. It doesn't. The residual guard's efficacy is only exercised in "rejects scattered high-residual fits" at :139-160 (which does run at n=6).
Fix: rename the 3-point test to something like "below min-samples rejects candidate" to clarify what it actually asserts, OR add a matched 5-sample test where each sample sits on a randomly-perturbed non-circle so the residual guard is the only thing standing between it and a finding.
Acknowledge two reviewer-flagged edge cases as documented, intentional limitations (per review): recoverPivot keeps the wider-radius fit and misses rather than falling back to the narrower fit; hubKey buckets by rounded screen coords so two dials at ~identical screen position merge. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pression A lone off-pivot hand on a multi-hand clock/dial was silently suppressed when its sibling hands were correctly hubbed, because the multi-body guard counted all candidates including finding===null (correctly-hubbed) ones. Count angular bodies over eligible findings only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Strengths: The corrective delta fixes the multi-hand false-negative at packages/cli/src/utils/checkPipeline.ts:910-918 by clustering only eligible off-pivot candidates, and the regression at packages/cli/src/utils/checkPipeline.offPivotRotation.test.ts:206-219 pins the one-broken-hand/two-correct-siblings case.
No new blocker in the exact-head delta. Required Windows and regression jobs are still pending, so GitHub should continue to gate merge on those checks.
Verdict: APPROVE
Reasoning: The prior eligibility-vs-clustering gap is fixed at the correct selection boundary with direct regression coverage; remaining work is CI completion.
— Magi
3 participants
What it catches
A gauge needle / clock hand / dial pointer / radar sweep that rotates about the wrong pivot — the recovered center-of-rotation sits far from the dial hub (e.g.
transform-originat the needle base or SVG element edge instead of the dial center). Visually the needle "wobbles" or orbits off-axis instead of sweeping cleanly about the hub.This is a genuine gap in the current checks:
rotation_pivot_drift(#2741) provably cannot catch it — a correct sweeping needle's bbox-center orbits identically to a broken one, so only a dial-hub reference distinguishes them. This is the separate hub-referenced check that analysis called for.How it works
getScreenCTM(honors the actual rendered transform, independent ofsvgOrigin).> 0.35 * pointer_length. One warning per hub.<svg>) so a pointer rotated by adivancestor is measured correctly.>= 2bodies at distinct angular positions on one hub = orbit/atom system, not a dial → suppressed.Corpus evidence (autonomous geometry-fuzz run, 81 fuzzed diagrams)
vlm_has_defects: false) — the deterministic hub-reference check beats the VLM on this defect class.divancestor) cleared by root-walk; fuzz055 (atom) cleared by the multi-body guard.Validation
checkPipeline.offPivotRotation.test.ts) + full check suite pass;bun run buildgreen.🤖 Generated with Claude Code