feat(lint): add off_pivot_rotation hub-referenced layout check#2744
feat(lint): add off_pivot_rotation hub-referenced layout check#2744xuanruli wants to merge 2 commits into
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). |
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