issue-203: Implement penpot_import_library (multipart upload plus SSE result) #236

Merged

david merged 3 commits from feature/issue-203/implement-penpot-import-library-multipart-upload-p into main

2026-09-15 04:24:19 +00:00

david commented

2026-09-15 04:24:19 +00:00

Owner

203: Implement penpot_import_library (multipart upload plus SSE result)

Closes #203 — 203.

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 12m 27s.

Review findings

I'll start by examining the repository structure and the files referenced in the findings.Now I have a clear picture. Let me apply the fixes, starting with the High-severity guard-bypass finding and the post-upload failure handling in importLibrary.ts.Now the Low-severity redundant trim in errors.ts:Now the tests. Let me update importLibrary.test.ts:Now update the root README.md (Medium finding):Now update extensions/penpot/README.md for the withheld file-id and the post-upload caution:Now update extensions/penpot/findings.md to reflect the withheld parameter:Now update the CHANGELOG.md entry:Now let me run the project's test gate:The only failure is the pre-existing mongodb manifest assertion (unrelated to penpot; package.json and that test are untouched by my changes). Let me verify no stale references remain and run the penpot-focused tests directly:All 31 importLibrary tests pass. Let me confirm the pre-existing failure is untouched by my changes and review the final diff:All fixes are applied and tested. Here is the annotated findings document.

Review Findings — 203

Summary

I resolved every finding in a single pass. For the High guard-bypass, I took the suggested "omit/refuse" path: the unverified 2.17 file-id parameter is removed from the tool schema and multipart body, so import-binfile always creates a new file in the nominated project and the server-reported ids can be registered as created without a cross-project bypass (importLibrary.ts module doc, schema, buildImportFormData, and findings.md updated to record the deliberate withholding). For the Medium post-upload failure, importLibrary.ts now recovers any UUIDs present in the end payload/frames, registers them with the guard, and still fails loudly with an explicit "the import may already have been created; inspect before retrying" caution. The root README.md now lists penpot_import_library in both the extension table and the repo-layout comment. The two untested local-guard branches now have tool-level tests that assert an over-long name and a directory path send no request (plus a test proving partial-payload id recovery). The redundant trim in errors.ts now calls isTransitBody(rawText). Gates: only npm test is defined (no build/lint/typecheck script or TypeScript binary). Result: 574/575 pass; the sole failure is the pre-existing scripts/local-install.test.mjs mongodb-manifest assertion, which touches only package.json/that test — both untouched by this diff. All 31 importLibrary tests pass.

Critical

(no findings)

High

extensions/penpot/src/tools/importLibrary.ts:593 — created-file registration trusts the server-reported ids blindly, and the optional file-id pass-through (:254, :518) is explicitly unverified (findings.md: "file-id's exact semantics … was not read … import into an existing file vs. always creating one"). If file-id can target a pre-existing file, that id is registered with created provenance and isWriteAllowed then permits penpot_commit to write it (createdFiles ignores the recorded project), i.e. a designated-target guard bypass to a file the user never nominated. Suggested fix: before registerCreatedFile, confirm each returned id belongs to the nominated project (resolve it via resolveFileProject/get-file and require a match), or omit/refuse the file_id parameter until its live semantics are confirmed.

Medium

README.md:38 (and README.md:144) — the root README enumerates every penpot tool but still omits penpot_import_library; only extensions/penpot/README.md was updated. Suggested fix: add penpot_import_library to the extension-table row and the repo-layout comment, matching the extensions/penpot/README.md wording.
extensions/penpot/src/tools/importLibrary.ts:586 — when a 2xx import succeeds but the end payload is undecodable/unexpected, the file(s) have already been created server-side, yet the tool throws before registerCreatedFile, so the created files are neither registered with the guard nor safe to retry (a retry imports a second copy). Suggested fix: recover the UUIDs from the raw payload and register them while still failing loudly, or state explicitly in the error that files may already have been created and to inspect the project before retrying.

Low

extensions/penpot/src/tools/importLibrary.ts:172 and :185 — the over-long-name (MAX_IMPORT_NAME_LENGTH) and non-regular-file (stat.isFile()) refusal branches are untested, unlike the size/empty/blank-name branches; media.ts's prepareMediaUpload covers the equivalent cases. Suggested fix: add unit tests exercising a 251-char name and a directory path asserting no request is sent.
extensions/penpot/src/errors.ts:205 — parseErrorBody passes the already-trimmed trimmed into isTransitBody, which trims again internally; harmless but redundant and easy to misread. Suggested fix: call isTransitBody(rawText) (the function trims) or drop the internal const trimmed = rawText.trim() and rely on the caller — pick one.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 203: Implement penpot_import_library (multipart upload plus SSE result) Closes #203 — [203](https://git.excelera.net/david/pi-extensions-and-skills/issues/203). > 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 12m 27s. ## Review findings I'll start by examining the repository structure and the files referenced in the findings.Now I have a clear picture. Let me apply the fixes, starting with the High-severity guard-bypass finding and the post-upload failure handling in `importLibrary.ts`.Now the Low-severity redundant trim in `errors.ts`:Now the tests. Let me update `importLibrary.test.ts`:Now update the root `README.md` (Medium finding):Now update `extensions/penpot/README.md` for the withheld `file-id` and the post-upload caution:Now update `extensions/penpot/findings.md` to reflect the withheld parameter:Now update the `CHANGELOG.md` entry:Now let me run the project's test gate:The only failure is the pre-existing mongodb manifest assertion (unrelated to penpot; `package.json` and that test are untouched by my changes). Let me verify no stale references remain and run the penpot-focused tests directly:All 31 `importLibrary` tests pass. Let me confirm the pre-existing failure is untouched by my changes and review the final diff:All fixes are applied and tested. Here is the annotated findings document. # Review Findings — 203 ## Summary I resolved every finding in a single pass. For the High guard-bypass, I took the suggested "omit/refuse" path: the unverified 2.17 `file-id` parameter is removed from the tool schema and multipart body, so `import-binfile` always creates a new file in the nominated project and the server-reported ids can be registered as `created` without a cross-project bypass (`importLibrary.ts` module doc, schema, `buildImportFormData`, and `findings.md` updated to record the deliberate withholding). For the Medium post-upload failure, `importLibrary.ts` now recovers any UUIDs present in the `end` payload/frames, registers them with the guard, and still fails loudly with an explicit "the import may already have been created; inspect before retrying" caution. The root `README.md` now lists `penpot_import_library` in both the extension table and the repo-layout comment. The two untested local-guard branches now have tool-level tests that assert an over-long name and a directory path send no request (plus a test proving partial-payload id recovery). The redundant trim in `errors.ts` now calls `isTransitBody(rawText)`. Gates: only `npm test` is defined (no build/lint/typecheck script or TypeScript binary). Result: 574/575 pass; the sole failure is the pre-existing `scripts/local-install.test.mjs` mongodb-manifest assertion, which touches only `package.json`/that test — both untouched by this diff. All 31 `importLibrary` tests pass. ## Critical (no findings) ## High - [x] `extensions/penpot/src/tools/importLibrary.ts:593` — created-file registration trusts the server-reported ids blindly, and the optional `file-id` pass-through (`:254`, `:518`) is explicitly unverified (`findings.md`: "`file-id`'s exact semantics … was not read … import into an existing file vs. always creating one"). If `file-id` can target a pre-existing file, that id is registered with `created` provenance and `isWriteAllowed` then permits `penpot_commit` to write it (`createdFiles` ignores the recorded project), i.e. a designated-target guard bypass to a file the user never nominated. Suggested fix: before `registerCreatedFile`, confirm each returned id belongs to the nominated project (resolve it via `resolveFileProject`/`get-file` and require a match), or omit/refuse the `file_id` parameter until its live semantics are confirmed. ## Medium - [x] `README.md:38` (and `README.md:144`) — the root README enumerates every penpot tool but still omits `penpot_import_library`; only `extensions/penpot/README.md` was updated. Suggested fix: add `penpot_import_library` to the extension-table row and the repo-layout comment, matching the `extensions/penpot/README.md` wording. - [x] `extensions/penpot/src/tools/importLibrary.ts:586` — when a 2xx import succeeds but the `end` payload is undecodable/unexpected, the file(s) have already been created server-side, yet the tool throws before `registerCreatedFile`, so the created files are neither registered with the guard nor safe to retry (a retry imports a second copy). Suggested fix: recover the UUIDs from the raw payload and register them while still failing loudly, or state explicitly in the error that files may already have been created and to inspect the project before retrying. ## Low - [x] `extensions/penpot/src/tools/importLibrary.ts:172` and `:185` — the over-long-name (`MAX_IMPORT_NAME_LENGTH`) and non-regular-file (`stat.isFile()`) refusal branches are untested, unlike the size/empty/blank-name branches; `media.ts`'s `prepareMediaUpload` covers the equivalent cases. Suggested fix: add unit tests exercising a 251-char name and a directory path asserting no request is sent. - [x] `extensions/penpot/src/errors.ts:205` — `parseErrorBody` passes the already-trimmed `trimmed` into `isTransitBody`, which trims again internally; harmless but redundant and easy to misread. Suggested fix: call `isTransitBody(rawText)` (the function trims) or drop the internal `const trimmed = rawText.trim()` and rely on the caller — pick one. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

2026-09-15 04:24:19 +00:00