issue-210: Implement the penpot_add_svg tool with svg-raw fallback and conversion reporting #243

Merged

david merged 2 commits from feature/issue-210/implement-the-penpot-add-svg-tool-with-svg-raw-fal into main

2026-09-15 06:53:16 +00:00

david commented

2026-09-15 06:53:15 +00:00

Owner

210: Implement the penpot_add_svg tool with svg-raw fallback and conversion reporting

Closes #210 — 210.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 40m 43s.

Review findings

I'll start by exploring the repository structure and the relevant files.Now let me implement the fixes. Starting with the critical circle geometry in src/svg.ts:Now the tool file. First the header, schema, conversion error mapping and circle transform:Now the result text notes and the input/details wiring:Now let me add the tests. First the assertions in existing tests:Now the documentation surfaces. Root README:Now the penpot extension README row (correcting the svg-raw claim) and CHANGELOG:Now the skill workflow doc (Medium finding):All fixes are in place and the suite is green apart from a pre-existing unrelated failure. Here is the annotated findings document.

Review Findings — 210

Summary

Fixed every finding in a single pass. Critical: the <circle> path now stages a real box — CircleGeometry gained x/y/width/height (built by a new shared circleGeometryFrom, used by both applyMatrixToCircle and transformCircle) and buildShapeForDescriptor spreads the geometry's own selrect/points, so an SVG with a <circle> can no longer produce an invalid update-file payload; the vacuous test now asserts concrete numbers (x === 8, width === 8, the full selrect/points). High: the svg-raw claim is corrected everywhere it was model-facing (tool header, tool description, an explicit formatAddSvgText note, the extension README, the CHANGELOG and the skill), taking the finding's sanctioned documentation option rather than adding a transit writer; tests were added for hidden/opacity staging and for both linked-library read-failure paths (continue on a failed library get-file, abort on a failed get-file-libraries). Medium: root README tool list and repo layout updated, SKILL.md compose/lossy sections updated, the element-budget SvgError now maps to its own penpot_add_svg message (no dead-end maxNodes advice), and the SVG font-family is no longer copied over the default fontId/fontVariantId — the dropped families are reported in the text and in details.droppedFontFamilies. Low: scale uses exclusiveMinimum: 0, describeCounts no longer double-counts svg-raw as converted, and a zero-shape import states that nothing was staged. Gates: npm test → 820 tests, 818 pass / 1 fail / 1 skipped; the single failure (scripts/local-install.test.mjs:1860, the mongodb pi.extensions manifest check) is pre-existing and unrelated (no package.json change here). No build or lint script exists; a strict tsc pass over extensions/penpot/src/** was compared against a baseline HEAD tree and introduces no new type errors (it removes the 12 circle-geometry read errors). Nothing remains unresolved.

Critical

extensions/penpot/src/tools/svg.ts:626 — a converted <circle> is staged with no x/y/width/height and a NaN selrect/points. buildShapeForDescriptor reads geometry.x/.y/.width/.height, but a circle descriptor's geometry is a CircleGeometry (src/svg.ts:141, produced by applyMatrixToCircle at src/svg.ts:892 and transformCircle at tools/svg.ts:489) which only carries cx/cy/rx/ry/selrect/points, and baseShape re-derives selrect/points from the (undefined) box, discarding the circle's correct selrect. Verified against a stubbed transport: <svg viewBox="0 0 24 24"><circle cx="12" cy="8" r="4"/></svg> stages {"type":"circle","name":"svg dot","selrect":{"x2":null,"y2":null},"points":[{},…]} (JSON.stringify drops the undefined keys and turns NaN into null). Penpot requires :x/:y/:width/:height (common/src/app/common/types/shape.cljc:178 schema:shape-geom-attrs) and a complete grc/rect selrect, so any SVG containing a <circle> makes the whole update-file batch fail at penpot_commit ("missing required key: x"), while the tool reports success and details.changeCount counts it as a converted shape. The test meant to catch this (tools/svg.test.ts:640) compares selrect.x with obj.x, which are both undefined for a circle, so it passes vacuously. Suggested fix: give circles a real box before baseShape — add x: cx - rx, y: cy - ry, width: rx * 2, height: ry * 2 to CircleGeometry (set in transformCircle and applyMatrixToCircle, or normalise it in the tool) and spread the geometry's own selrect/points rather than letting baseShape re-derive them; then assert concrete numbers for the circle (e.g. x === 8, width === 8 for that fixture) so an undefined geometry can never pass again.

High

extensions/penpot/src/tools/svg.ts:22 — the header claims "unsupported artwork stays visible and editable-as-an-object", but a svg-raw created over this extension's JSON transport renders nothing in Penpot 2.17: the front-end gates the render on (contains? csvg/svg-tags tag) with svg-tags a set of keywords (common/src/app/common/svg.cljc:38, frontend/src/app/main/ui/shapes/svg_raw.cljs valid-tag? / (= tag :svg) / (? svg-tag? valid-tag?), with no :else branch), and the backend JSON reader keywordises only keys (backend/src/app/http/middleware.clj:58-60, common/src/app/common/json.cljc:78-95), while schema:svg-raw-attrs is an empty map so content is never coerced. findings.md:1605-1627 records this, but every model-facing surface still implies the fallback survives visually (the tool description at :825ff, the README row, the CHANGELOG "so no node is dropped silently") and the result text (:733-740) lists the skipped nodes without saying they will not draw. An agent importing a gradient/use/defs icon therefore commits an invisible import and reports success. Suggested fix: either send the update-file body as application/transit+json when the batch contains svg-raw content (the middleware already accepts transit — middleware.clj:53), or, if a transit writer is out of scope for this issue, correct the claim here and add an explicit line to formatAddSvgText such as "N node(s) are stored as svg-raw objects; Penpot 2.17 does not render them (its hiccup tag cannot be sent as a keyword over JSON) — treat them as lost artwork and re-work the source or export a raster instead".
extensions/penpot/src/tools/svg.ts:636 — hidden/opacity are mapped into the staged payload but nothing tests it: a <rect display="none"> must stage hidden: true and an <g opacity="0.5"> must stage opacity: 0.5 (verified by probe), yet svg.test.ts has no occurrence of either field, so the documented "a hidden layer does not reappear as visible artwork" guarantee (README/CHANGELOG) can regress silently. Suggested fix: add a test importing PRIMITIVES_SVG plus a display:none node and an opacity="0.5" group and assert obj.hidden === true / obj.opacity === 0.5 on the staged add-obj payloads.
extensions/penpot/src/tools/svg.ts:383 — the documented resilience path "a linked library that cannot be read is recorded and the import continues" has no test: the !result.ok branch and its libraryReadFailures reporting (plus the hard failTool on a get-file-libraries failure at :369) are only reachable through a stub that 500s a library get-file. Suggested fix: add a test whose get-file-libraries lists a library id, whose get-file for that id returns 500, and assert the import still stages every shape, that details.colors.libraryReadFailures names the library, and that the message reaches the result text (a second test can assert the get-file-libraries failure aborts with the server error and stages nothing).

Medium

README.md:39 — the root README enumerates every registered penpot tool (penpot_add_image, penpot_instance_component, …) and was updated by the previous tool issues, but this change updates only extensions/penpot/README.md; penpot_add_svg is missing here and in the repository-layout line at README.md:145. Suggested fix: add penpot_add_svg (imports an SVG as native shapes with an svg-raw fallback and conversion/colour-matching report) to the tool enumeration at README.md:39 and to the extensions/penpot/ line at README.md:145.
skills/penpot/SKILL.md:69 — the skill's "Compose the screen" list (the place an agent following the workflow looks) lists penpot_add_frame/add_rect/add_text/add_image/instance_component but not penpot_add_svg, even though the skill's own description advertises composing "from a prompt, an SVG or a screenshot" and plan milestone M8 requires the three creation paths to be documented. Suggested fix: add a bullet for penpot_add_svg (path or inline markup, native subset + svg-raw fallback, parent_id defaults to the root frame, link_colors matches library swatches) and a line in the "Lossy areas" section that unsupported nodes are stored as svg-raw and do not render in 2.17.
extensions/penpot/src/tools/svg.ts:323 — the converter's element-budget overflow is re-labelled with the wrong cause and an unactionable remedy: a large-but-valid SVG throws "the SVG is not a valid SVG document: SVG exceeds the 10000-element limit; raise maxNodes to convert it" (src/svg.ts:1691), but penpot_add_svg exposes no maxNodes parameter, so the caller has no way to follow the advice and is told a valid document is invalid. Suggested fix: map the budget SvgError to its own message in convertMarkup (e.g. "the SVG has more than 10 000 elements, which penpot_add_svg cannot import — simplify the source or split it"), and either raise/bound the limit explicitly or expose it as a tool parameter.
extensions/penpot/src/tools/svg.ts:664 — an SVG font-family is copied verbatim into the span while fontId/fontVariantId keep the defaults (sourcesanspro/regular), producing a mismatched font triple that no other tool emits (penpot_add_text has no font_family argument, and penpot_create_typography resolves the family against the instance's variants and records a substitution). Penpot loads text faces by (juxt :font-id :font-variant-id) (frontend/src/app/main/ui/shapes/text.cljs:17-35), so a family the instance does not have renders with a face that does not match the family shown for the layer, silently and with no substitution reported. Suggested fix: resolve the family through the same font path as penpot_create_typography (recording the substitution in details/the result text, like penpot_add_text's substitution note) or leave the span on the documented default triple and report the dropped family as a note.

Low

extensions/penpot/src/tools/svg.ts:142 — scale declares minimum: 0 while :825 requires > 0, so scale: 0 passes schema validation and is then rejected by a hand-rolled error, while a negative value is rejected by typebox with a different message. Suggested fix: use Type.Number({ exclusiveMinimum: 0, ... }) and drop the manual check (or keep both with identical wording).
extensions/penpot/src/tools/svg.ts:728 — describeCounts(report.counts) includes the svg-raw bucket, so a fallback-only import reads "Converted: 0 (svg-raw 2)" immediately above "Fallback (svg-raw): 2" (verified), double-reporting the fallbacks as converted. Suggested fix: filter svg-raw out of describeCounts (or rename the parenthetical to "shapes by type" and keep the counts disjoint from the Converted: number).
extensions/penpot/src/tools/svg.ts:725 — a valid source that yields no shapes (<svg viewBox="0 0 24 24"/>, <svg/>) returns a plain success naming "Staged 0 shapes" with changeCount: 0 and no hint that nothing was imported, which is easy for a model to miss. Suggested fix: when input.status.changeCount === 0, append a line such as "The SVG contained no importable artwork — nothing was staged (an empty or defs-only document is not an error)".

## 210: Implement the penpot_add_svg tool with svg-raw fallback and conversion reporting Closes #210 — [210](https://git.excelera.net/david/pi-extensions-and-skills/issues/210). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 40m 43s. ## Review findings I'll start by exploring the repository structure and the relevant files.Now let me implement the fixes. Starting with the critical circle geometry in `src/svg.ts`:Now the tool file. First the header, schema, conversion error mapping and circle transform:Now the result text notes and the input/details wiring:Now let me add the tests. First the assertions in existing tests:Now the documentation surfaces. Root README:Now the penpot extension README row (correcting the `svg-raw` claim) and CHANGELOG:Now the skill workflow doc (Medium finding):All fixes are in place and the suite is green apart from a pre-existing unrelated failure. Here is the annotated findings document. # Review Findings — 210 ## Summary Fixed every finding in a single pass. **Critical:** the `<circle>` path now stages a real box — `CircleGeometry` gained `x`/`y`/`width`/`height` (built by a new shared `circleGeometryFrom`, used by both `applyMatrixToCircle` and `transformCircle`) and `buildShapeForDescriptor` spreads the geometry's own `selrect`/`points`, so an SVG with a `<circle>` can no longer produce an invalid `update-file` payload; the vacuous test now asserts concrete numbers (`x === 8`, `width === 8`, the full selrect/points). **High:** the `svg-raw` claim is corrected everywhere it was model-facing (tool header, tool description, an explicit `formatAddSvgText` note, the extension README, the CHANGELOG and the skill), taking the finding's sanctioned documentation option rather than adding a transit writer; tests were added for `hidden`/`opacity` staging and for both linked-library read-failure paths (continue on a failed library `get-file`, abort on a failed `get-file-libraries`). **Medium:** root README tool list and repo layout updated, `SKILL.md` compose/lossy sections updated, the element-budget `SvgError` now maps to its own `penpot_add_svg` message (no dead-end `maxNodes` advice), and the SVG `font-family` is no longer copied over the default `fontId`/`fontVariantId` — the dropped families are reported in the text and in `details.droppedFontFamilies`. **Low:** `scale` uses `exclusiveMinimum: 0`, `describeCounts` no longer double-counts `svg-raw` as converted, and a zero-shape import states that nothing was staged. Gates: `npm test` → 820 tests, 818 pass / 1 fail / 1 skipped; the single failure (`scripts/local-install.test.mjs:1860`, the mongodb `pi.extensions` manifest check) is pre-existing and unrelated (no `package.json` change here). No build or lint script exists; a strict `tsc` pass over `extensions/penpot/src/**` was compared against a baseline `HEAD` tree and introduces no new type errors (it removes the 12 circle-geometry read errors). Nothing remains unresolved. ## Critical - [x] `extensions/penpot/src/tools/svg.ts:626` — a converted `<circle>` is staged with **no `x`/`y`/`width`/`height`** and a NaN `selrect`/`points`. `buildShapeForDescriptor` reads `geometry.x/.y/.width/.height`, but a circle descriptor's geometry is a `CircleGeometry` (`src/svg.ts:141`, produced by `applyMatrixToCircle` at `src/svg.ts:892` and `transformCircle` at `tools/svg.ts:489`) which only carries `cx/cy/rx/ry/selrect/points`, and `baseShape` re-derives `selrect`/`points` from the (undefined) box, discarding the circle's correct `selrect`. Verified against a stubbed transport: `<svg viewBox="0 0 24 24"><circle cx="12" cy="8" r="4"/></svg>` stages `{"type":"circle","name":"svg dot","selrect":{"x2":null,"y2":null},"points":[{},…]}` (`JSON.stringify` drops the `undefined` keys and turns `NaN` into `null`). Penpot requires `:x/:y/:width/:height` (`common/src/app/common/types/shape.cljc:178` `schema:shape-geom-attrs`) and a complete `grc/rect` selrect, so **any SVG containing a `<circle>` makes the whole `update-file` batch fail** at `penpot_commit` ("missing required key: x"), while the tool reports success and `details.changeCount` counts it as a converted shape. The test meant to catch this (`tools/svg.test.ts:640`) compares `selrect.x` with `obj.x`, which are *both* `undefined` for a circle, so it passes vacuously. Suggested fix: give circles a real box before `baseShape` — add `x: cx - rx, y: cy - ry, width: rx * 2, height: ry * 2` to `CircleGeometry` (set in `transformCircle` and `applyMatrixToCircle`, or normalise it in the tool) and spread the geometry's own `selrect`/`points` rather than letting `baseShape` re-derive them; then assert concrete numbers for the circle (e.g. `x === 8`, `width === 8` for that fixture) so an undefined geometry can never pass again. ## High - [x] `extensions/penpot/src/tools/svg.ts:22` — the header claims "unsupported artwork stays visible and editable-as-an-object", but a `svg-raw` created over this extension's JSON transport renders **nothing** in Penpot 2.17: the front-end gates the render on `(contains? csvg/svg-tags tag)` with `svg-tags` a set of **keywords** (`common/src/app/common/svg.cljc:38`, `frontend/src/app/main/ui/shapes/svg_raw.cljs` `valid-tag?` / `(= tag :svg)` / `(? svg-tag? valid-tag?)`, with no `:else` branch), and the backend JSON reader keywordises only keys (`backend/src/app/http/middleware.clj:58-60`, `common/src/app/common/json.cljc:78-95`), while `schema:svg-raw-attrs` is an empty map so `content` is never coerced. `findings.md:1605-1627` records this, but every model-facing surface still implies the fallback survives visually (the tool description at `:825`ff, the README row, the CHANGELOG "so no node is dropped silently") and the result text (`:733-740`) lists the skipped nodes without saying they will not draw. An agent importing a gradient/`use`/`defs` icon therefore commits an invisible import and reports success. Suggested fix: either send the `update-file` body as `application/transit+json` when the batch contains `svg-raw` content (the middleware already accepts transit — `middleware.clj:53`), or, if a transit writer is out of scope for this issue, correct the claim here and add an explicit line to `formatAddSvgText` such as "N node(s) are stored as svg-raw objects; Penpot 2.17 does not render them (its hiccup tag cannot be sent as a keyword over JSON) — treat them as lost artwork and re-work the source or export a raster instead". - [x] `extensions/penpot/src/tools/svg.ts:636` — `hidden`/`opacity` are mapped into the staged payload but nothing tests it: a `<rect display="none">` must stage `hidden: true` and an `<g opacity="0.5">` must stage `opacity: 0.5` (verified by probe), yet `svg.test.ts` has no occurrence of either field, so the documented "a hidden layer does not reappear as visible artwork" guarantee (README/CHANGELOG) can regress silently. Suggested fix: add a test importing `PRIMITIVES_SVG` plus a `display:none` node and an `opacity="0.5"` group and assert `obj.hidden === true` / `obj.opacity === 0.5` on the staged `add-obj` payloads. - [x] `extensions/penpot/src/tools/svg.ts:383` — the documented resilience path "a linked library that cannot be read is recorded and the import continues" has no test: the `!result.ok` branch and its `libraryReadFailures` reporting (plus the hard `failTool` on a `get-file-libraries` failure at `:369`) are only reachable through a stub that 500s a library `get-file`. Suggested fix: add a test whose `get-file-libraries` lists a library id, whose `get-file` for that id returns 500, and assert the import still stages every shape, that `details.colors.libraryReadFailures` names the library, and that the message reaches the result text (a second test can assert the `get-file-libraries` failure aborts with the server error and stages nothing). ## Medium - [x] `README.md:39` — the root README enumerates every registered penpot tool (`penpot_add_image`, `penpot_instance_component`, …) and was updated by the previous tool issues, but this change updates only `extensions/penpot/README.md`; `penpot_add_svg` is missing here and in the repository-layout line at `README.md:145`. Suggested fix: add `penpot_add_svg` (imports an SVG as native shapes with an `svg-raw` fallback and conversion/colour-matching report) to the tool enumeration at `README.md:39` and to the `extensions/penpot/` line at `README.md:145`. - [x] `skills/penpot/SKILL.md:69` — the skill's "Compose the screen" list (the place an agent following the workflow looks) lists `penpot_add_frame`/`add_rect`/`add_text`/`add_image`/`instance_component` but not `penpot_add_svg`, even though the skill's own description advertises composing "from a prompt, **an SVG** or a screenshot" and plan milestone M8 requires the three creation paths to be documented. Suggested fix: add a bullet for `penpot_add_svg` (path or inline markup, native subset + `svg-raw` fallback, `parent_id` defaults to the root frame, `link_colors` matches library swatches) and a line in the "Lossy areas" section that unsupported nodes are stored as `svg-raw` and do not render in 2.17. - [x] `extensions/penpot/src/tools/svg.ts:323` — the converter's element-budget overflow is re-labelled with the wrong cause and an unactionable remedy: a large-but-valid SVG throws "the SVG is not a valid SVG document: SVG exceeds the 10000-element limit; **raise maxNodes to convert it**" (`src/svg.ts:1691`), but `penpot_add_svg` exposes no `maxNodes` parameter, so the caller has no way to follow the advice and is told a valid document is invalid. Suggested fix: map the budget `SvgError` to its own message in `convertMarkup` (e.g. "the SVG has more than 10 000 elements, which penpot_add_svg cannot import — simplify the source or split it"), and either raise/bound the limit explicitly or expose it as a tool parameter. - [x] `extensions/penpot/src/tools/svg.ts:664` — an SVG `font-family` is copied verbatim into the span while `fontId`/`fontVariantId` keep the defaults (`sourcesanspro`/`regular`), producing a mismatched font triple that no other tool emits (`penpot_add_text` has no `font_family` argument, and `penpot_create_typography` resolves the family against the instance's variants and records a substitution). Penpot loads text faces by `(juxt :font-id :font-variant-id)` (`frontend/src/app/main/ui/shapes/text.cljs:17-35`), so a family the instance does not have renders with a face that does not match the family shown for the layer, silently and with no substitution reported. Suggested fix: resolve the family through the same font path as `penpot_create_typography` (recording the substitution in `details`/the result text, like `penpot_add_text`'s substitution note) or leave the span on the documented default triple and report the dropped family as a note. ## Low - [x] `extensions/penpot/src/tools/svg.ts:142` — `scale` declares `minimum: 0` while `:825` requires `> 0`, so `scale: 0` passes schema validation and is then rejected by a hand-rolled error, while a negative value is rejected by typebox with a different message. Suggested fix: use `Type.Number({ exclusiveMinimum: 0, ... })` and drop the manual check (or keep both with identical wording). - [x] `extensions/penpot/src/tools/svg.ts:728` — `describeCounts(report.counts)` includes the `svg-raw` bucket, so a fallback-only import reads "Converted: 0 (svg-raw 2)" immediately above "Fallback (svg-raw): 2" (verified), double-reporting the fallbacks as converted. Suggested fix: filter `svg-raw` out of `describeCounts` (or rename the parenthetical to "shapes by type" and keep the counts disjoint from the `Converted:` number). - [x] `extensions/penpot/src/tools/svg.ts:725` — a valid source that yields no shapes (`<svg viewBox="0 0 24 24"/>`, `<svg/>`) returns a plain success naming "Staged 0 shapes" with `changeCount: 0` and no hint that nothing was imported, which is easy for a model to miss. Suggested fix: when `input.status.changeCount === 0`, append a line such as "The SVG contained no importable artwork — nothing was staged (an empty or defs-only document is not an error)".

david added 2 commits

2026-09-15 06:53:15 +00:00