issue-200: Implement penpot_add_image with media upload #233

Merged

david merged 3 commits from feature/issue-200/implement-penpot-add-image-with-media-upload into main

2026-09-15 03:45:03 +00:00

david commented

2026-09-15 03:45:02 +00:00

Owner

200: Implement penpot_add_image with media upload

Closes #200 — 200.

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 50s.

Review findings

I'll start by exploring the repository and understanding the relevant files.The changes are committed. Let me read the relevant files.Now let me make the fixes. Starting with the Medium findings.Now the media.ts changes:Now add tests. First client.test.ts:Now the High finding — add an add_image case to the live-write-check harness:Now run the test suite:Let me review the final state of the key changed sections:All gates run. Only the pre-existing scripts/local-install.test.mjs failure remains (mongodb missing from pi.extensions on main too), and all 370 penpot tests pass.

Review Findings — 200

Summary

Fixed the three Medium findings (upload timeout, tolerant/actionable response normalisation, enforced body/formData mutual exclusion) and all five Low findings (README typo and wording, stricter SVG sniff, missing test for the unsupported-mtype branch, zero-dimension fallback). Added unit tests for each behaviour change; node --test extensions/penpot/src/*.test.ts extensions/penpot/src/tools/*.test.ts → 370/370 pass. The High finding remains open: I added a live penpot_add_image check (check 5) to extensions/penpot/scripts/live-write-check.mjs so the upload can be verified end to end, but no Penpot token/instance is available in this environment, so the multipart request still has not actually been sent and the observed request/response is not recorded.

Critical

(no findings)

High

extensions/penpot/findings.md:459 — The multipart upload has never been sent to a live instance ("A real upload. No token was available, so the multipart request has never been sent"), and the issue's live test-plan steps 1–5 are explicitly left open. Every test stubs globalThis.fetch, so the real contract (/api/rpc/command/upload-file-media-object, content part name, is-local boolean coercion, camelCase response keys, metadata.id) is verified only from source reading. Suggested fix: before merge, run the upload against the scratch file with a real token (or add an add_image case to extensions/penpot/scripts/live-write-check.mjs) and record the observed request/response in findings.md.

Medium

extensions/penpot/src/media.ts:392 — sendMediaUpload calls penpotRequest without a timeoutMs, so the upload is bounded by the client's 30 s default (client.ts:38). A legitimate file up to the 30 MiB cap can exceed 30 s on a modest link; the abort fires mid-send and the server may have already created the media object, so the tool reports a transport failure while an orphaned media object exists. Suggested fix: pass an upload-specific timeoutMs (e.g. 120_000, or scale from prepared.bytes.byteLength) to the penpotRequest call in sendMediaUpload.
extensions/penpot/src/media.ts:297 — normalizeMediaResponse fails hard when name, mtype, width or height is absent after the upload has already created the media object, and the resulting error carries no media id — the caller gets a server error and an unrecoverable orphan. Penpot's own schema makes mtype optional ([:mtype {:optional true} [:maybe :string]]). Suggested fix: treat mtype as optional (omit it from metadata when absent) and, for the remaining required fields, include the created media id and a note that the media object exists in the file so the agent can reference or clean it up.
extensions/penpot/src/client.ts:233 — formData is documented as "mutually exclusive with body" but not enforced: if both are supplied, the JSON body is silently dropped and no Content-Type is set. Suggested fix: throw a clear error when options.body !== undefined && options.formData !== undefined (or return a validation-category PenpotError).

Low

README.md:38 — Double comma typo introduced in the penpot row: ...after the designated-target guard),, penpot_authorize_target``. Suggested fix: remove the extra comma.
README.md:38 — The penpot row now reads "penpot_add_rect / penpot_add_text / penpot_add_image (staged add-obj rect/text/image primitives with library-colour and library-typography reference resolution...)", but penpot_add_image has no colour/typography references. Suggested fix: split the sentence so only rect/text claim library-reference resolution, or reword to "rect/text library-reference resolution; penpot_add_image uploads the media object first, after the designated-target guard".
extensions/penpot/src/media.ts:135 — looksLikeSvg accepts any file whose first 4 KiB contains the substring <svg, so an HTML/JS file with an inline <svg> is classified as image/svg+xml and uploaded (the server then rejects it, wasting a round trip and surfacing a less actionable error). Suggested fix: after stripping a BOM, leading whitespace, the XML declaration and comments, require the first element to be <svg rather than matching anywhere in the head.
extensions/penpot/src/media.ts:310 — The !isImageMtype(mtype) branch in normalizeMediaResponse is untested (the suite covers the missing-field branch only). Suggested fix: add a case returning mtype: "application/pdf" and assert the "unsupported media type" server error.
extensions/penpot/src/tools/image.ts:88 — deriveImageSize accepts width: 0/height: 0 (the schema sets minimum: 0) and, with keep_aspect_ratio true, returns a 0×0 box (round(0 / ratio) === 0), staging a degenerate shape. Suggested fix: reject a zero/negative requested dimension, or fall back to the intrinsic axis when the supplied value is 0.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 200: Implement penpot_add_image with media upload Closes #200 — [200](https://git.excelera.net/david/pi-extensions-and-skills/issues/200). > 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 50s. ## Review findings I'll start by exploring the repository and understanding the relevant files.The changes are committed. Let me read the relevant files.Now let me make the fixes. Starting with the Medium findings.Now the media.ts changes:Now add tests. First client.test.ts:Now the High finding — add an `add_image` case to the live-write-check harness:Now run the test suite:Let me review the final state of the key changed sections:All gates run. Only the pre-existing `scripts/local-install.test.mjs` failure remains (mongodb missing from `pi.extensions` on `main` too), and all 370 penpot tests pass. # Review Findings — 200 ## Summary Fixed the three Medium findings (upload timeout, tolerant/actionable response normalisation, enforced `body`/`formData` mutual exclusion) and all five Low findings (README typo and wording, stricter SVG sniff, missing test for the unsupported-mtype branch, zero-dimension fallback). Added unit tests for each behaviour change; `node --test extensions/penpot/src/*.test.ts extensions/penpot/src/tools/*.test.ts` → 370/370 pass. The High finding remains open: I added a live `penpot_add_image` check (check 5) to `extensions/penpot/scripts/live-write-check.mjs` so the upload can be verified end to end, but no Penpot token/instance is available in this environment, so the multipart request still has not actually been sent and the observed request/response is not recorded. ## Critical (no findings) ## High - [ ] `extensions/penpot/findings.md:459` — The multipart upload has never been sent to a live instance ("A real upload. No token was available, so the multipart request has never been sent"), and the issue's live test-plan steps 1–5 are explicitly left open. Every test stubs `globalThis.fetch`, so the real contract (`/api/rpc/command/upload-file-media-object`, `content` part name, `is-local` boolean coercion, camelCase response keys, `metadata.id`) is verified only from source reading. Suggested fix: before merge, run the upload against the scratch file with a real token (or add an `add_image` case to `extensions/penpot/scripts/live-write-check.mjs`) and record the observed request/response in `findings.md`. ## Medium - [x] `extensions/penpot/src/media.ts:392` — `sendMediaUpload` calls `penpotRequest` without a `timeoutMs`, so the upload is bounded by the client's 30 s default (`client.ts:38`). A legitimate file up to the 30 MiB cap can exceed 30 s on a modest link; the abort fires mid-send and the server may have already created the media object, so the tool reports a transport failure while an orphaned media object exists. Suggested fix: pass an upload-specific `timeoutMs` (e.g. `120_000`, or scale from `prepared.bytes.byteLength`) to the `penpotRequest` call in `sendMediaUpload`. - [x] `extensions/penpot/src/media.ts:297` — `normalizeMediaResponse` fails hard when `name`, `mtype`, `width` or `height` is absent *after* the upload has already created the media object, and the resulting error carries no media id — the caller gets a server error and an unrecoverable orphan. Penpot's own schema makes `mtype` optional (`[:mtype {:optional true} [:maybe :string]]`). Suggested fix: treat `mtype` as optional (omit it from `metadata` when absent) and, for the remaining required fields, include the created media id and a note that the media object exists in the file so the agent can reference or clean it up. - [x] `extensions/penpot/src/client.ts:233` — `formData` is documented as "mutually exclusive with `body`" but not enforced: if both are supplied, the JSON `body` is silently dropped and no `Content-Type` is set. Suggested fix: throw a clear error when `options.body !== undefined && options.formData !== undefined` (or return a `validation`-category `PenpotError`). ## Low - [x] `README.md:38` — Double comma typo introduced in the penpot row: `...after the designated-target guard),, `penpot_authorize_target``. Suggested fix: remove the extra comma. - [x] `README.md:38` — The penpot row now reads "`penpot_add_rect` / `penpot_add_text` / `penpot_add_image` (staged `add-obj` rect/text/image primitives with library-colour and library-typography reference resolution...)", but `penpot_add_image` has no colour/typography references. Suggested fix: split the sentence so only rect/text claim library-reference resolution, or reword to "rect/text library-reference resolution; `penpot_add_image` uploads the media object first, after the designated-target guard". - [x] `extensions/penpot/src/media.ts:135` — `looksLikeSvg` accepts any file whose first 4 KiB contains the substring `<svg`, so an HTML/JS file with an inline `<svg>` is classified as `image/svg+xml` and uploaded (the server then rejects it, wasting a round trip and surfacing a less actionable error). Suggested fix: after stripping a BOM, leading whitespace, the XML declaration and comments, require the *first* element to be `<svg` rather than matching anywhere in the head. - [x] `extensions/penpot/src/media.ts:310` — The `!isImageMtype(mtype)` branch in `normalizeMediaResponse` is untested (the suite covers the missing-field branch only). Suggested fix: add a case returning `mtype: "application/pdf"` and assert the "unsupported media type" server error. - [x] `extensions/penpot/src/tools/image.ts:88` — `deriveImageSize` accepts `width: 0`/`height: 0` (the schema sets `minimum: 0`) and, with `keep_aspect_ratio` true, returns a 0×0 box (`round(0 / ratio) === 0`), staging a degenerate shape. Suggested fix: reject a zero/negative requested dimension, or fall back to the intrinsic axis when the supplied value is 0. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

2026-09-15 03:45:02 +00:00