issue-211: Validate SVG import (icon and illustration) against the live instance #244

Merged

david merged 2 commits from feature/issue-211/validate-svg-import-icon-and-illustration-against into main

2026-09-15 07:08:12 +00:00

david commented

2026-09-15 07:08:11 +00:00

Owner

211: Validate SVG import (icon and illustration) against the live instance

Closes #211 — 211.

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 14m 53s.

Review findings

Now let me apply the fixes. First, the viewBox stroke-scale regression test:Now the live-write-check "no matching asset" branch:I've applied the feasible fixes and re-run the only gate (npm test). The new viewBox regression test passes (test 369) and the stub harness still reports ALL CHECKS PASSED; the sole failure remains the pre-existing mongodb manifest assertion (scripts/local-install.test.mjs:1875), which this diff does not touch.

Review Findings — 211

Summary

Fixed the three code/test findings: added the missing convertSvg regression test for a stroke width scaled by the root viewBox viewport matrix (Medium), taught check 9's linked-swatch assertion to handle the "no #2BA8A2 asset in the file" case with a NOTE and a literal-fill assertion instead of a misleading failure (Low), and corrected the matrixScale doc plus the findings.md residual note to state that non-uniform axis-aligned scales also reach rects and circles (Low). Gate: npm test → 823 tests, 821 pass, 1 fail, 1 skipped; the single failure is the pre-existing, unrelated mongodb-manifest assertion also seen on the merge-base. Left unresolved: the Medium live-instance/UI validation (environment-blocked — no PENPOT_TOKEN/browser) and the Low branch/commit-naming deviation (this stage may not create branches or amend commits).

Critical

(none)

High

(none)

Medium

extensions/penpot/src/svg.test.ts:722 — the fix documents that "the viewBox viewport scale is covered by the same change because it is part of the root matrix", but the only converter regression test uses <g transform="scale(2)">; no test asserts a stroke width scaled by the root viewBox viewport matrix (the tool test uses PRIMITIVES_SVG, which has no width/height, so its root matrix is identity). Suggested fix: add a convertSvg test with <svg viewBox="0 0 10 10" width="20" height="20"><rect … stroke-width="3"/></svg> and assert strokeWidth === 6 (geometry is already 20) so the documented claim is pinned.
extensions/penpot/findings.md:1963 — the issue asks to validate the SVG import "against the live instance", but the live run and all five UI observations are still unperformed (environment-blocked: no PENPOT_TOKEN, instance only answers unauthenticated get-profile). The document says so plainly, but the issue's central criterion remains open. Suggested fix: before closing, run check 9 with credentials and paste the five UI observations into this section, or explicitly descope the issue to "source-verified + harness-covered" and split the UI pass into a follow-up.

Low

extensions/penpot/scripts/live-write-check.mjs:1422 — check 9's linked-swatch assertion is hard-coded to succeed only when check 1 has already committed pi-197-teal to a fresh scratch file (the dependency is noted in a comment, but the failure mode is handled only for the duplicate case, not for "no matching asset at all"). Running check 9 in isolation, or after check 1 fails before staging its colours, fails the assertion with a misleading message. Suggested fix: mirror the ambiguity branch — if no #2BA8A2 asset exists in the file, print a NOTE and assert the literal-fill outcome instead of failing.
extensions/penpot/src/svg.ts:384 — the matrixScale doc says "the x-axis length is used for paths, the only shapes that accept one", but a non-uniform axis-aligned scale (scale(sx sy), matrix(sx,0,0,sy,…)) also reaches convertRect and convertCircle/transformCircle, where the stroke is scaled by the x-factor while the geometry's ry is scaled by d. findings.md:1955 mentions only "a plain stroked rect". Suggested fix: clarify the comment (rotation/skew only ever reaches paths; non-uniform axis-aligned scale reaches rects/circles too) and add circles to the residual note.
AGENTS.md:26 — the branch/commit deviate from the documented issue-driven convention: the branch feature/issue-211/validate-svg-import-icon-and-illustration-against truncates the title slug, and the commit subject is 211: Validate SVG import … rather than the required issue-211: Validate SVG import …. Suggested fix: per AGENTS.md, use <type>/issue-<N>/<full-kebab-slug> and issue-<N>: <summary> for the commit/PR title (e.g. issue-211: Validate SVG import (icon and illustration) against the live instance).

## 211: Validate SVG import (icon and illustration) against the live instance Closes #211 — [211](https://git.excelera.net/david/pi-extensions-and-skills/issues/211). > 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 14m 53s. ## Review findings Now let me apply the fixes. First, the viewBox stroke-scale regression test:Now the live-write-check "no matching asset" branch:I've applied the feasible fixes and re-run the only gate (`npm test`). The new viewBox regression test passes (test 369) and the stub harness still reports `ALL CHECKS PASSED`; the sole failure remains the pre-existing `mongodb` manifest assertion (`scripts/local-install.test.mjs:1875`), which this diff does not touch. # Review Findings — 211 ## Summary Fixed the three code/test findings: added the missing `convertSvg` regression test for a stroke width scaled by the root `viewBox` viewport matrix (Medium), taught check 9's linked-swatch assertion to handle the "no `#2BA8A2` asset in the file" case with a NOTE and a literal-fill assertion instead of a misleading failure (Low), and corrected the `matrixScale` doc plus the `findings.md` residual note to state that non-uniform axis-aligned scales also reach rects and circles (Low). Gate: `npm test` → 823 tests, 821 pass, 1 fail, 1 skipped; the single failure is the pre-existing, unrelated `mongodb`-manifest assertion also seen on the merge-base. Left unresolved: the Medium live-instance/UI validation (environment-blocked — no `PENPOT_TOKEN`/browser) and the Low branch/commit-naming deviation (this stage may not create branches or amend commits). ## Critical - [ ] (none) ## High - [ ] (none) ## Medium - [x] `extensions/penpot/src/svg.test.ts:722` — the fix documents that "the viewBox viewport scale is covered by the same change because it is part of the root matrix", but the only converter regression test uses `<g transform="scale(2)">`; no test asserts a stroke width scaled by the root `viewBox` viewport matrix (the tool test uses `PRIMITIVES_SVG`, which has no `width`/`height`, so its root matrix is identity). Suggested fix: add a `convertSvg` test with `<svg viewBox="0 0 10 10" width="20" height="20"><rect … stroke-width="3"/></svg>` and assert `strokeWidth === 6` (geometry is already `20`) so the documented claim is pinned. - [ ] `extensions/penpot/findings.md:1963` — the issue asks to validate the SVG import "against the live instance", but the live run and all five UI observations are still unperformed (environment-blocked: no `PENPOT_TOKEN`, instance only answers unauthenticated `get-profile`). The document says so plainly, but the issue's central criterion remains open. Suggested fix: before closing, run check 9 with credentials and paste the five UI observations into this section, or explicitly descope the issue to "source-verified + harness-covered" and split the UI pass into a follow-up. ## Low - [x] `extensions/penpot/scripts/live-write-check.mjs:1422` — check 9's linked-swatch assertion is hard-coded to succeed only when check 1 has already committed `pi-197-teal` to a fresh scratch file (the dependency is noted in a comment, but the failure mode is handled only for the *duplicate* case, not for "no matching asset at all"). Running check 9 in isolation, or after check 1 fails before staging its colours, fails the assertion with a misleading message. Suggested fix: mirror the ambiguity branch — if no `#2BA8A2` asset exists in the file, print a NOTE and assert the literal-fill outcome instead of failing. - [x] `extensions/penpot/src/svg.ts:384` — the `matrixScale` doc says "the x-axis length is used for paths, the only shapes that accept one", but a non-uniform **axis-aligned** scale (`scale(sx sy)`, `matrix(sx,0,0,sy,…)`) also reaches `convertRect` and `convertCircle`/`transformCircle`, where the stroke is scaled by the x-factor while the geometry's `ry` is scaled by `d`. `findings.md:1955` mentions only "a plain stroked rect". Suggested fix: clarify the comment (rotation/skew only ever reaches paths; non-uniform axis-aligned scale reaches rects/circles too) and add circles to the residual note. - [ ] `AGENTS.md:26` — the branch/commit deviate from the documented issue-driven convention: the branch `feature/issue-211/validate-svg-import-icon-and-illustration-against` truncates the title slug, and the commit subject is `211: Validate SVG import …` rather than the required `issue-211: Validate SVG import …`. Suggested fix: per AGENTS.md, use `<type>/issue-<N>/<full-kebab-slug>` and `issue-<N>: <summary>` for the commit/PR title (e.g. `issue-211: Validate SVG import (icon and illustration) against the live instance`).

david added 2 commits

2026-09-15 07:08:11 +00:00