issue-204: Implement penpot_export_library (SSE URI plus artifact download) #237

Merged

david merged 3 commits from feature/issue-204/implement-penpot-export-library-sse-uri-plus-artif into main

2026-09-15 04:35:55 +00:00

david commented

2026-09-15 04:35:54 +00:00

Owner

204: Implement penpot_export_library (SSE URI plus artifact download)

Closes #204 — 204.

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 11m 33s.

Review findings

Now I'll make the code changes. Starting with exportLibrary.ts:Now update the tests:All findings are addressed and the suite is green apart from the documented pre-existing failure. Final annotated findings:

Review Findings — 204

Summary

All nine findings (1 High, 3 Medium, 5 Low) were remediated in a single pass. Added: a Transit-error-body test for the export RPC (exportLibrary.test.ts); a stat().isFile() guard in prepareExportTarget so a directory/non-regular out_path is refused before any request, plus a writeArtifact helper wrapping mkdir/writeFile so filesystem failures surface as the tool's categorised error; explicit redirect: "manual" following that re-requests the location without the Authorization header (comment, findings.md, extension README and CHANGELOG updated to match); penpot_export_library added to both root README rows; a UUID pattern on file_id (reusing UUID_PATTERN, now exported from importLibrary.ts) plus filename sanitisation in defaultExportPath; a 100 MiB streamed body cap with content-length pre-check; the final download URL surfaced in details/the summary (unused contentType dropped); a >MAX_LISTED_ENTRIES truncation test; and tests for the download transport failure and the body === null export response. Tests: extensions/penpot/src/tools/exportLibrary.test.ts now 35/35 passing; full npm test is 609/610, the sole failure being the pre-existing, unrelated scripts/local-install.test.mjs:1875 (mongodb missing from pi.extensions), reproduced at HEAD~1 by the reviewer. The untestable defensive streamed === undefined branch was left as-is (unreachable through penpotStreamRequest, mirrors the sibling tool).

Critical

High

extensions/penpot/src/tools/exportLibrary.ts:535 — mapExportFailure's Transit branch (isTransitBody(...) ? decodeTransitError(...) : decoded.message) has no test at all; the suite only exercises the 403 path and the download 401. Penpot answers RPC failures with application/transit+json bodies (recorded in findings.md), and the sibling tool covers exactly this case (importLibrary.test.ts:668, "import_library decodes a Transit error body into a readable message"), so a documented behaviour is unverified here. Suggested fix: add a test that stubs the export handler with a 400 application/transit+json body (e.g. {"~:type":"~:validation","~:message":"the artifact is not a valid penpot file"}) and asserts the decoded message is surfaced and the token is not echoed.

Medium

extensions/penpot/src/tools/exportLibrary.ts:810 — mkdir/writeFile are awaited with no try/catch, so a filesystem failure (EACCES, ENOSPC, read-only checkout) escapes as a raw Node error instead of the tool's categorised, actionable message; with overwrite: true the guard at :252 also skips the stat, so an out_path naming a directory (or any non-regular file) passes and fails at writeFile with EISDIR. prepareImportArtifact (importLibrary.ts:186-215) wraps every fs error and checks isFile(), so this deviates from the sibling tool. Suggested fix: in prepareExportTarget reject a target that exists and is not a regular file (stat().isFile()), and wrap the mkdir/writeFile pair in try/catch → failTool(...); consider writing to <path>.tmp then rename so an interrupted write cannot leave a truncated artifact that the overwrite guard then refuses.
extensions/penpot/src/tools/exportLibrary.ts:343 — redirect: "follow" relies on the runtime stripping Authorization on redirect. That holds cross-origin (verified against Node 22/undici here), but on a same-origin redirect the header is forwarded, so a same-origin pre-signed object store receives both the Authorization header and the presigned query and rejects it (S3/MinIO: "Only one auth mechanism allowed"), and the claim in the comment, findings.md and the CHANGELOG that "the token is not leaked to the object store" is only true cross-origin. Suggested fix: follow redirects explicitly (redirect: "manual") and re-issue the location request without the Authorization header, or at minimum qualify the comment and the docs to the cross-origin case.
README.md:38 — the root README enumerates every registered penpot tool (it lists penpot_import_library, penpot_add_image, etc., and the previous issue added its tool to the same rows), but neither this row nor the layout row at README.md:144 mentions penpot_export_library, while the extension README, CHANGELOG and findings.md were all updated. Suggested fix: add penpot_export_library (export a .penpot artifact to disk) to both README.md:38 and README.md:144.

Low

extensions/penpot/src/tools/exportLibrary.ts:194 — defaultExportPath interpolates the caller-supplied file_id straight into a filesystem path with no shape validation, so an id such as ../../foo would resolve outside skills/penpot/assets. Impact is minimal (the RPC rejects a non-UUID id before any write, and out_path is explicitly caller-chosen), but it is one schema keyword away from being closed. Suggested fix: add a UUID pattern to the file_id schema (reuse the UUID_PATTERN from importLibrary.ts:93) and/or sanitise the id before using it as a file name.
extensions/penpot/src/tools/exportLibrary.ts:376 — the whole archive is buffered in memory via await response.arrayBuffer() with no size cap, unlike penpot_import_library's 30 MiB local guard (importLibrary.ts:70) — and the design doc itself cites a 17 MB library artifact. Suggested fix: stream the body to the target file (or a temp file) and verify the magic bytes from the first chunk, or add a configurable byte cap.
extensions/penpot/src/tools/exportLibrary.ts:286 — DownloadedAsset.contentType and DownloadedAsset.url are populated at :384-385 but never consumed by the tool or asserted by any test; the post-redirect url is the useful diagnostic for exactly the redirect behaviour above. Suggested fix: surface url in details/the summary text (or drop both fields).
extensions/penpot/src/tools/exportLibrary.ts:491 — the truncation branch (truncated === true, MAX_LISTED_ENTRIES) is never exercised; the only assertions are truncated === false and entries.length <= MAX_LISTED_ENTRIES on a 5-entry archive. Suggested fix: add a summarizeArchive test with >500 entries asserting truncated === true and entries.length === MAX_LISTED_ENTRIES.
extensions/penpot/src/tools/exportLibrary.ts:346 — the download's transport-failure branch (downloadTransportError, including the timeout/abort wording) and the defensive streamed === undefined (:747) and streamed.body === null (:757) branches are uncovered. Suggested fix: stub the download handler to reject (or return a response whose body never ends with a short timeout) and assert the categorised transport message, plus one test for a null body on a 2xx export response.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 204: Implement penpot_export_library (SSE URI plus artifact download) Closes #204 — [204](https://git.excelera.net/david/pi-extensions-and-skills/issues/204). > 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 11m 33s. ## Review findings Now I'll make the code changes. Starting with `exportLibrary.ts`:Now update the tests:All findings are addressed and the suite is green apart from the documented pre-existing failure. Final annotated findings: # Review Findings — 204 ## Summary All nine findings (1 High, 3 Medium, 5 Low) were remediated in a single pass. Added: a Transit-error-body test for the export RPC (`exportLibrary.test.ts`); a `stat().isFile()` guard in `prepareExportTarget` so a directory/non-regular `out_path` is refused before any request, plus a `writeArtifact` helper wrapping `mkdir`/`writeFile` so filesystem failures surface as the tool's categorised error; explicit `redirect: "manual"` following that re-requests the `location` without the `Authorization` header (comment, `findings.md`, extension README and CHANGELOG updated to match); `penpot_export_library` added to both root README rows; a UUID `pattern` on `file_id` (reusing `UUID_PATTERN`, now exported from `importLibrary.ts`) plus filename sanitisation in `defaultExportPath`; a 100 MiB streamed body cap with `content-length` pre-check; the final download URL surfaced in `details`/the summary (unused `contentType` dropped); a >`MAX_LISTED_ENTRIES` truncation test; and tests for the download transport failure and the `body === null` export response. Tests: `extensions/penpot/src/tools/exportLibrary.test.ts` now 35/35 passing; full `npm test` is 609/610, the sole failure being the pre-existing, unrelated `scripts/local-install.test.mjs:1875` (mongodb missing from `pi.extensions`), reproduced at `HEAD~1` by the reviewer. The untestable defensive `streamed === undefined` branch was left as-is (unreachable through `penpotStreamRequest`, mirrors the sibling tool). ## Critical ## High - [x] extensions/penpot/src/tools/exportLibrary.ts:535 — `mapExportFailure`'s Transit branch (`isTransitBody(...) ? decodeTransitError(...) : decoded.message`) has no test at all; the suite only exercises the 403 path and the download 401. Penpot answers RPC failures with `application/transit+json` bodies (recorded in `findings.md`), and the sibling tool covers exactly this case (`importLibrary.test.ts:668`, "import_library decodes a Transit error body into a readable message"), so a documented behaviour is unverified here. Suggested fix: add a test that stubs the `export` handler with a 400 `application/transit+json` body (e.g. `{"~:type":"~:validation","~:message":"the artifact is not a valid penpot file"}`) and asserts the decoded message is surfaced and the token is not echoed. ## Medium - [x] extensions/penpot/src/tools/exportLibrary.ts:810 — `mkdir`/`writeFile` are awaited with no `try`/`catch`, so a filesystem failure (EACCES, ENOSPC, read-only checkout) escapes as a raw Node error instead of the tool's categorised, actionable message; with `overwrite: true` the guard at :252 also skips the `stat`, so an `out_path` naming a directory (or any non-regular file) passes and fails at `writeFile` with `EISDIR`. `prepareImportArtifact` (`importLibrary.ts:186-215`) wraps every fs error and checks `isFile()`, so this deviates from the sibling tool. Suggested fix: in `prepareExportTarget` reject a target that exists and is not a regular file (`stat().isFile()`), and wrap the `mkdir`/`writeFile` pair in `try`/`catch` → `failTool(...)`; consider writing to `<path>.tmp` then `rename` so an interrupted write cannot leave a truncated artifact that the overwrite guard then refuses. - [x] extensions/penpot/src/tools/exportLibrary.ts:343 — `redirect: "follow"` relies on the runtime stripping `Authorization` on redirect. That holds cross-origin (verified against Node 22/undici here), but on a **same-origin** redirect the header is forwarded, so a same-origin pre-signed object store receives both the `Authorization` header and the presigned query and rejects it (S3/MinIO: "Only one auth mechanism allowed"), and the claim in the comment, `findings.md` and the CHANGELOG that "the token is not leaked to the object store" is only true cross-origin. Suggested fix: follow redirects explicitly (`redirect: "manual"`) and re-issue the `location` request without the `Authorization` header, or at minimum qualify the comment and the docs to the cross-origin case. - [x] README.md:38 — the root README enumerates every registered penpot tool (it lists `penpot_import_library`, `penpot_add_image`, etc., and the previous issue added its tool to the same rows), but neither this row nor the layout row at README.md:144 mentions `penpot_export_library`, while the extension README, CHANGELOG and `findings.md` were all updated. Suggested fix: add `penpot_export_library` (export a `.penpot` artifact to disk) to both README.md:38 and README.md:144. ## Low - [x] extensions/penpot/src/tools/exportLibrary.ts:194 — `defaultExportPath` interpolates the caller-supplied `file_id` straight into a filesystem path with no shape validation, so an id such as `../../foo` would resolve outside `skills/penpot/assets`. Impact is minimal (the RPC rejects a non-UUID id before any write, and `out_path` is explicitly caller-chosen), but it is one schema keyword away from being closed. Suggested fix: add a UUID `pattern` to the `file_id` schema (reuse the `UUID_PATTERN` from `importLibrary.ts:93`) and/or sanitise the id before using it as a file name. - [x] extensions/penpot/src/tools/exportLibrary.ts:376 — the whole archive is buffered in memory via `await response.arrayBuffer()` with no size cap, unlike `penpot_import_library`'s 30 MiB local guard (`importLibrary.ts:70`) — and the design doc itself cites a 17 MB library artifact. Suggested fix: stream the body to the target file (or a temp file) and verify the magic bytes from the first chunk, or add a configurable byte cap. - [x] extensions/penpot/src/tools/exportLibrary.ts:286 — `DownloadedAsset.contentType` and `DownloadedAsset.url` are populated at :384-385 but never consumed by the tool or asserted by any test; the post-redirect `url` is the useful diagnostic for exactly the redirect behaviour above. Suggested fix: surface `url` in `details`/the summary text (or drop both fields). - [x] extensions/penpot/src/tools/exportLibrary.ts:491 — the truncation branch (`truncated === true`, `MAX_LISTED_ENTRIES`) is never exercised; the only assertions are `truncated === false` and `entries.length <= MAX_LISTED_ENTRIES` on a 5-entry archive. Suggested fix: add a `summarizeArchive` test with >500 entries asserting `truncated === true` and `entries.length === MAX_LISTED_ENTRIES`. - [x] extensions/penpot/src/tools/exportLibrary.ts:346 — the download's transport-failure branch (`downloadTransportError`, including the timeout/abort wording) and the defensive `streamed === undefined` (:747) and `streamed.body === null` (:757) branches are uncovered. Suggested fix: stub the `download` handler to reject (or return a response whose body never ends with a short timeout) and assert the categorised transport message, plus one test for a `null` body on a 2xx export response. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

2026-09-15 04:35:54 +00:00