fix(lint): stop a leading <svg> defs block from being mistaken for the composition root#1867
Conversation
…e composition root Two independent post-release feedback reports of the same mechanism: a leading <svg> block (icon/gradient/filter <defs>, referenced by url(#id) elsewhere in the document) placed before the real [data-composition-id] root manufactures root_missing_composition_id + root_missing_dimensions on an otherwise-correct composition. Moving the <svg> after the root cleared both findings for each reporter. findRootTag returned the first body child that wasn't script/style/meta/ link/title, unconditionally — <svg> was never in that skip list, so a leading defs-only <svg> got treated as the root. Fix: skip a leading <svg> when it carries none of the composition markers itself (data-composition-id/data-width/data-height), so an intentionally SVG-rooted composition is still eligible as the root. The first attempt at this only skipped the <svg> open tag, which surfaced a second bug: extractOpenTags is a flat, nesting-unaware scan, so the very next tag it returns after skipping <svg> is the svg's own nested child (<defs>, <filter>, ...), not the sibling after </svg>. Track the svg's closing tag position and skip every tag before it, not just the <svg> tag itself. Tests: skips a leading svg defs block (no false root findings); still treats an <svg> as the root when data-composition-id/data-width/ data-height are declared directly on it. Full lint suite (308 tests) passes.
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 3ce9d844e9 (batch review, Group C lint quality; layering on Magi's own vetting).
Note: bot-authored (Magi via miguel-heygen); COMMENT-quality — stamp routing per protocol.
Summary — teaches findRootTag to skip a leading marker-less <svg> block (with its nested children, via skipBefore + closing-tag position) before falling through to the real composition root. Preserves the intentionally-SVG-rooted case (svg with data-composition-id/data-width/data-height on itself).
Verified
- The two-bug rediscovery in the diff comment is real:
extractOpenTagsis a flat regex scan, so skipping only the<svg>open tag would surface<defs>/<filter>as the next candidate. TheskipBefore = tag.index + closeMatch.index + closeMatch[0].lengthtrick advances past</svg>and every open tag inside — correct. - Malformed
<svg>(no closing tag) setsskipBefore = Infinity, which forcesfindRootTagto returnnullrather than misidentifying an svg-internal child. Safer default than the alternative. - Multiple consecutive leading
<svg>defs blocks: second<svg>lives after first</svg>, sotag.index >= skipBefore— the loop re-evaluates it, hits the same svg branch, updatesskipBeforeagain. Compounds cleanly. - The
<body data-composition-id="...">early-return path is unaffected (evaluated beforebodyContent). - Two tests cover both branches (skip-svg-defs + still-honor-svg-with-markers).
Concerns
- 🟡 The
// fallow-ignore-next-line complexitycomment is now needed because the function grew; fine as a signal, but if this file starts collecting morefallow-ignoremarkers it's worth a refactor pass to lift the SVG-skip predicate out. Not blocking.
Nits
- 🟡
/<\/svg\s*>/imatches</svg>and</svg >but not</svg\n>. In practice SVG closing tags don't have interior whitespace including newlines often — but\s*allows tabs/spaces without newlines. Consider\s*>or[^>]*>if you want to be more tolerant. Extremely unlikely to matter for lint input.
— Rames D Jusso
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVE — verified CI green (0 fail / 0 pending) + no open CR at this head. Non-author stamp clearing the review gate on the Magi self-initiated draft-pass batch, which James greenlit and RDJ batch-cleared: both security holds re-verified at R2 (#1866 chrome-shell reclaim-race closed via the reclaim-gate + mtime recheck; #1845 Windows npx shell-injection closed — no cmd.exe, node <npx-cli.js> with pure argv), zero drift on the other nine, all green.
Merge via Magi's normal path (no admin-merge). Ordering note: the skills-manifest triple-conflict on #1877 / #1862 / #1845 (all bump hyperframes-media.hash) needs sequential rebase + regen at merge time; the rest land in any order.
— Rames Jusso
3 participants
Root-caused from two independent post-release feedback reports of the exact same mechanism, one this run and one from an earlier loop run.
Root cause
findRootTagreturned the first body child that wasn'tscript/style/meta/link/title, unconditionally.<svg>was never in that skip list, so a leading defs-only<svg>(icon/gradient/filter plumbing referenced viaurl(#id)elsewhere in the document — a very ordinary pattern) got treated as the composition root, manufacturing both findings on an otherwise-correct composition. Moving the<svg>after the root cleared both findings for each reporter.Fix
Skip a leading
<svg>when it carries none of the composition markers itself (data-composition-id/data-width/data-height), so an intentionally SVG-rooted composition is still eligible as the root.The first attempt at this only skipped the
<svg>open tag, which surfaced a second, more subtle bug:extractOpenTagsis a flat, nesting-unaware regex scan, so the very next tag it returns after skipping<svg>is the svg's own nested child (<defs>,<filter>, ...) — not the sibling that follows</svg>. Fixed by tracking the svg's closing-tag position and skipping every tag before it, not just the<svg>tag itself.Tests
Skips a leading svg defs block (no false root findings); still treats an
<svg>as the root when composition markers are declared directly on it. Full lint suite (308 tests) passes.