feat(compiler): add automated vector master linter gate - #234
Merged
Merged
Conversation
There was a problem hiding this comment.
Blockers
tools/lib/vector-linter.mjs:L1,tools/test/vector-linter.test.mjs:L1: Both files start with a UTF-8 BOM (EF BB BF) before#!/usr/bin/env node..editorconfigsetscharset = utf-8(no BOM), they are the only BOM-prefixed scripts undertools/, and the shebang is no longer the first bytes, so running the file directly on Linux fails. Both files also lack the trailing newline (tools/lib/vector-linter.mjs:L258,tools/test/vector-linter.test.mjs:L369), which breaksinsert_final_newline = true. Re-save both as UTF-8 without BOM, LF line endings, ending in a newline.tools/test/vector-linter.test.mjs:L84: The master count is hardcoded to 30 (again at L88–L89). The next texture PR that merges breaks this suite,npm test, and theVector Master Linter Test SuiteCI step. Open #195 addstextures/block/cobblestone.svg, which would make it 31. Suite 1 also only walkstextures/block/, whilelint:svgcovers all oftextures/. Derive the expected set fromfindSvgFiles(DEFAULT_TEXTURES_DIR), then assert that it is non-empty and thatfilesCheckedequals its length.- PR description: The repo's
pull_request_template.mdis not used. The description has no Affected Assets, Type of Change, or Contributor Checklist, and no linked issue (Closes #N). It also starts with a stray BOM.CONTRIBUTING.md§4.3 requires the checklist. Rewrite the description on the template, fill in the checklist, and link the tracking issue.
Nits
tools/lib/vector-linter.mjs:L112:<filter><feImage href="x.png"/></filter>passes the linter. I reproduced this locally, and resvg rendersfeImageraster sources. Extend the raster check tofeImageand add a fixture for it.tools/lib/vector-linter.mjs:L118:\bid\s*=also matchesdata-id="g", sofill="url(#g)"passes when no element hasid="g"(reproduced). Anchor the match on attribute whitespace, for example(?:^|\s)id\s*=.CHANGELOG.md:L15: There is no[Unreleased]→### Addedentry for the linter gate. Prior tooling PRs (#227) added one. Add an entry.CONTRIBUTING.md:L104: The canvas and editor-residue rules are now machine-enforced. Namenpm run lint:svghere and add it to the Pull Request Checklist (L250), as the base-sync and tiling sections already do for their checks.- Commit
64ed447: The body is 5 lines, with lines up to 199 characters, which breaks N6-COMMIT-05 (CONTRIBUTING.md§3.5: a line or two, wrapped at 72). Trim the squash-merge message to the subject plus sign-off.
Signed-off-by: Bharath <bharathasl74185@gmail.com>
BharathASL
force-pushed
the
feat/compiler-vector-linter
branch
from
September 23, 2026 19:13
64ed447 to
e99be63
Compare
There was a problem hiding this comment.
Blockers
tools/lib/vector-linter.mjs:L88,L132,L143,L147: the attribute and element regexes use theiflag, but SVG is XML and case-sensitive, and resvg ignores the case variants that these regexes accept. So the gate passes masters that render wrong without any error, which is the failure it was added to catch. Checked against@resvg/resvg-js: a<linearGradient ID="g">counts as a definition forfill="url(#g)", but resvg does not resolve it, so the rect renders fully transparent. A<clippath id="c">counts as a clipPath, but resvg drops the clip and draws the element unclipped.viewbox="0 0 512 512"anddata-viewBox="0 0 512 512"both pass the canvas rule, although resvg ignores both. Fix: dropifrom theviewBox,idand<clipPath>patterns. AnchorviewBoxon preceding whitespace the wayidalready is. Add failing fixtures forviewbox=,data-viewBox=,ID=and<clippath>totools/test/vector-linter.test.mjs.
Nits
tools/lib/vector-linter.mjs:L122-L123,L156: onlyurl(#id)is checked for resolution. A same-documenthref/xlink:href="#id"on<use>, on a gradient's template link or on the<feImage>fragment this rule explicitly allows is never checked, and resvg drops it silently in exactly the same way (<feImage href="#nope">and<use href="#missing">both lint clean). Run these fragment hrefs through the samedefinedIdscheck.tools/lib/vector-linter.mjs:L156:url("#id"), the form that editors emit insidestyleattributes, does not match the URL pattern, so a dangling reference in that form lints clean. Decode"/'before matching, or accept them as the quote.
Signed-off-by: Bharath <bharathasl74185@gmail.com>
Contributor
Author
|
Resolved in 1fc7fb5:
|
There was a problem hiding this comment.
Blockers
tools/lib/vector-linter.mjs:L162:urlRegexstill has theiflag, soURL(#id)counts as a resolved reference. resvg 2.6.2 does not recognise the uppercase function. With@resvg/resvg-js,clip-path="URL(#c)"andstyle="clip-path:URL(#c)"lint clean, but the clip is dropped and the element draws unclipped.fill="URL(#g)", both inline and in a<style>rule, also lints clean and renders black instead of the gradient. This is the same case-variant false pass as the previous round, andCONTRIBUTING.md:L113now claims it cannot happen. Fix: matchurl(case-sensitively. Also reject anyurl(spelled in another case, because a case-insensitive match that acceptsURL(is exactly the problem. DropifromclipPathRefRegex(L186) as well. Add failing fixtures forURL(#id)in an attribute, instyle, and in<style>.
Nits
tools/lib/vector-linter.mjs:L186-L193: onlyclip-pathis checked against the type of the element it references.fill="url(#r)"where#ris a<rect>lints clean, and resvg renders the whole shape transparent.mask="url(#c)"pointing at a<clipPath>, andfilter="url(#g)"pointing at a gradient, also lint clean, and resvg renders the element invisible. Check the target type forfill/stroke,mask,filterandmarker-*the same way.tools/lib/vector-linter.mjs:L162,L176: references that are not a same-document fragment are never checked.<use href="other.svg#r"/>andfill="url(other.svg#g)"both lint clean. resvg drops the<use>and paints the fill black. Reject anyurl()orhrefon a non-<feImage>element that is not#id.tools/lib/vector-linter.mjs:L126,L176: thehrefpatterns only recognise the literalxlink:prefix.xmlns:x="http://www.w3.org/1999/xlink"with<feImage x:href="data:image/png;base64,...">lints clean, and resvg renders the embedded PNG, so the raster ban can be bypassed.x:href="#missing"on<use>also escapes the reference check. Accept any prefix onhref, or reject any xlink namespace prefix other thanxlink.tools/lib/vector-linter.mjs:L26-L30,L105: editor namespaces are detected by prefix, not by URI.xmlns:ns1="http://www.inkscape.org/namespaces/inkscape"passes. Illustrator never writesxmlns:illustrator: it writesxmlns:i="&ns_ai;"withns_ai=http://ns.adobe.com/AdobeIllustrator/10.0/, so real Illustrator residue passes, even thoughCONTRIBUTING.md:L113lists it as enforced. Match the Inkscape, Sodipodi and Adobe namespace URIs (including inside<!ENTITY>declarations) instead of the prefixes.
…nter Signed-off-by: Bharath <bharathasl74185@gmail.com>
Signed-off-by: Bharath <bharathasl74185@gmail.com>
Contributor
Author
|
Resolved in 9709dfe:
Every case reproduced against 1174805 moves the changelog entry to clear a conflict with |
There was a problem hiding this comment.
Blockers
None.
Nits
tools/lib/vector-linter.mjs:L330-L334:idTargetskeeps the first element for a duplicatedid, but resvg 2.6.2 resolvesurl()to the last one. With two<linearGradient id="g">, resvg paints the second gradient. A<clipPath id="c">followed by<rect id="c">lints clean, and resvg drops the clipped element. Two<mask id="m">behave the same way. No master has a duplicate today, but ore derivatives copy the stone base section, so a collision is a realistic mistake. Reject duplicateidvalues.tools/lib/vector-linter.mjs:L58-L67,L400-L402,CONTRIBUTING.md:L113: resvg 2.6.2 does apply themarkershorthand when it is written in a<style>rule..m{marker:url(#mk)}draws the marker. It only ignores the shorthand as a presentation attribute or in astyleattribute. The linter rejects all three forms, and the doc says resvg ignores the shorthand. Either acceptmarkerfor declarations that come from<style>, or describe the rejection as a policy choice, not resvg behaviour.tools/lib/vector-linter.mjs:L177,L403: the root namespace is never checked. For an SVG with noxmlns, everyurl()target fails with "is not a<linearGradient>... (found<linearGradient>)". A master with no references lints clean, even though resvg refuses to parse it. Add an explicit check that the root<svg>is in the SVG namespace, so the error names the actual cause.- PR description: the rule list still describes the first round: namespace detection by
xmlns:prefix, and a target-type check forclip-pathonly. Verification still reports117/117, but the suite now has 193 assertions. Update the description to match the code.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a static vector master linter,
tools/lib/vector-linter.mjs, and wires it intonpm testand CI as a blocking gate.npm run lint:svgwalks every SVG undertextures/and fails if any master:viewBoxother than exactly0 0 512 512;<!ENTITY>), or carriesinkscape:/sodipodi:editor elements;<image>or through a<feImage>whosehref(unprefixed or under any prefix bound to the xlink namespace) is not a same-document#id;<clipPath>without anid, aurl()orhrefreference that is not a same-document#idor resolves to nothing, or a reference whose target is the wrong element type (fill/stroke→ paint server,clip-path→<clipPath>,mask→<mask>,filter→<filter>,marker-start/mid/end→<marker>);url('#id')/url("#id")(including"/'entity forms), any case variant ofurl(, a space before(, upper-case property names; themarkershorthand is also rejected (resvg ignores it outside<style>rules).@resvg/resvg-jsdoes not fail on a dangling or unsupported reference, so a broken clip, mask, gradient or<use>reference changes the rendered texture without any build error. The linter tokenizes each master, tracks namespace scope, decodes entities and character references, and collects every attribute,styledeclaration and<style>rule, so only the forms resvg 2.6 actually renders pass. Each rejected form was confirmed against@resvg/resvg-js2.6.2.tools/test/vector-linter.test.mjs(npm run test:vector-linter) covers each rule with passing and failing fixtures, includingfeImagefile/xlink/data-URI/fragment cases,data-id, case-variant (viewbox,data-viewBox,ID,<clippath>), dangling fragmenthrefand quotedurl()cases. Its first suite lints every master undertextures/and derives the expected count fromfindSvgFiles(DEFAULT_TEXTURES_DIR), so a PR that adds a texture does not have to touch the test.CONTRIBUTING.mddocuments the enforced rules under Canvas & Grid Alignment and addsnpm run lint:svgto the Pull Request Checklist, andCHANGELOG.mdhas an[Unreleased]→Addedentry.No tracking issue exists for this gate. The closed toolchain epic #40 and its sub-tasks (#54–#59, #226) do not cover static SVG validation, and a search of the org's issues for linter, viewBox, editor namespace, raster and clipPath terms found nothing. No issue is linked.
Affected Assets
tools/lib/vector-linter.mjs(new)tools/test/vector-linter.test.mjs(new)package.json(lint:svg,test:vector-linter,npm testaggregator).github/workflows/ci.yml(Run Vector Master Linter, Vector Master Linter Test Suite)CONTRIBUTING.md,CHANGELOG.mdType of Change
feat: New vector texture master, blockstate, or compiler capabilityfix: Tiling fix, palette correction, or bugfixdocs: Documentation improvementchore/refactor: Maintenance, dependencies, or codebase cleanupContributor Checklist
Vector Texture Standards (if adding/modifying SVGs)
viewBox="0 0 512 512"). Not applicable: no SVGs added or modified. The linter now enforces this for every existing master.rx) matchtextures/block/stone.svgexactly. Not applicable: no ore added.npm run lint:svgpasses on all existing masters.Build & Verification
npm run buildlocally and verified that the pack compiles successfully.dist/. Not applicable: tooling-only change, no texture output changes.Git Hygiene & Standards
feat(textures): ...).git commit -s).mainwith no merge commits..editorconfig(2-space indent, LF endings, trailing newline).Verification
npm run lint:svg: all masters undertextures/pass.npm run test:vector-linter: 193/193.npm test: all suites pass.check-standards.py: no violations.