issue-205: Implement file-library linking and validate the export/import round trip #238

Merged

david merged 3 commits from feature/issue-205/implement-file-library-linking-and-validate-the-ex into main

2026-09-15 04:51:01 +00:00

david commented

2026-09-15 04:51:00 +00:00

Owner

205: Implement file-library linking and validate the export/import round trip

Closes #205 — 205.

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 15m 03s.

Review findings

I'll start by exploring the repository structure and the changed files.Let me check the baseline test suite first, then start implementing.Now update the UUID_PATTERN imports:Now the tool descriptions and execute bodies:Now the execute bodies for link and unlink:Now the libraryCommand.test.ts (Medium finding):Now extend the stub model with the library-link state:Now the stub server routes:Now the penpot README (permission model + transitive semantics):All changes are in place and the gates are re-run. Final verification of the working tree:All gates re-run. Here is the annotated findings document:

# Review Findings — 205

## Summary

Fixed 7 of 8 findings; the eighth is credential-gated and recorded as open.

- **High — direct vs. transitive verification** (`libraries.ts`): `get-file-libraries` returns the transitive closure, so set membership is no longer treated as proof of a direct link. New `findIndirectSources` re-reads each *other* returned entry's own library list. An unlink whose library stays listed only through another linked library is now reported as applied with `stillReachableVia: [ids]` (only a listing the tool cannot account for fails); a link onto an already-reachable-but-not-direct library reports `reachableDirectly: false` with the sources; both tool descriptions and `penpot_list_libraries` now state that `has-file-libraries` counts **direct** links while `get-file-libraries` returns direct **and transitive** libraries. Covered by two new tests (`libraries.test.ts`) plus the exact-message assertion the Low finding asked for.
- **High — harness evidence**: `live-write-check.mjs` gained check 7 (list → link → list → unlink → list, printing raw entries at every step) and `live-write-stub.mjs` / `penpotStubModel.ts` now model the library links, so the flow runs end to end offline and under `npm test`; the recorded stub output and the exact live command (`… PENPOT_LIBRARY_FILE_ID=<library file> node …/live-write-check.mjs`) are in `findings.md`. The **live** before/after payloads still cannot be captured here — no `PENPOT_URL`/`PENPOT_TOKEN` — so that item stays unchecked.
- **Medium**: library-file permission wording corrected everywhere ("not written to, need not be designated, but Penpot requires **edit** permission on it; a 403 names that"); `src/libraryCommand.test.ts` added for the shared reader's tolerance branches; root README tool list and layout line updated.
- **Low**: `UUID_PATTERN` moved to `src/ids.ts` (imported by the three tools); both similarly named list tools now lead their descriptions with the distinction (own assets vs. other files linked); the unlink-failure test asserts the exact message.
- **Gates**: no build/lint/typecheck exists to run (`package.json` scripts are `install:local` and `test`; no root `tsconfig.json`), so `npm test` is the only gate — **636 tests, 635 pass, 1 fail**. The single failure (`scripts/local-install.test.mjs` asserting `pi.extensions` contains `extensions/mongodb/index.ts`) is pre-existing and `package.json` is untouched. While running the gate I also hit a pre-existing flake in `extensions/penpot/src/tools/exportLibrary.test.ts` (adm-zip stamps each entry with the current time and the test built the archive twice: 1/30 failures on a clean tree, 0/30 after using one buffer); fixed minimally in that test.

## Critical

*(none)*

## High

- [x] `extensions/penpot/src/tools/libraries.ts:439` — the unlink verification is unsound because `get-file-libraries` returns the **transitive** closure of linked libraries, not the direct set. Penpot 2.17.2's `backend/src/app/binfile/common.clj` (`sql:get-file-libraries`) is a `WITH RECURSIVE` query: for a consumer F that links M and M links L, `get-file-libraries(F)` includes L even though F has no direct link to L. Consequences on a path reachable through the tools' own output (`penpot_list_libraries` advertises transitive entries as "linked"): (a) `penpot_unlink_library(F, L)` sends the unlink, the delete removes nothing (or removes F→L while L stays reachable via M), the re-read still lists L, and the tool hard-fails with "the unlink did not take effect" although the requested unlink succeeded; (b) symmetrically, `penpot_link_library(F, L)` at `libraries.ts:350` reports "already linked … no change was sent" for a library that is only reachable transitively, so a genuinely direct link the user asked for is never made. Suggested fix: verify the direct relation rather than set membership — after the mutation, if the library is still listed, fetch `get-file-libraries` for each of the *other* returned entries: if one of them lists the library, treat the unlink as applied and return a `stillReachableVia: [ids]` note instead of failing; otherwise fail as today. For link, reword the taken-branch to "already reachable (directly or transitively)" and expose whether the entry is direct. Also state in the `penpot_list_libraries` description (`libraries.ts:280`) that `has-file-libraries` counts **direct** links while `get-file-libraries` returns direct **and transitive** libraries, since the two fields can describe different sets.
- [ ] `extensions/penpot/findings.md:957` — the issue's second half ("…and validate the export/import round trip") is not evidenced for the new code path. `findings.md` is honest that no live run was possible (no `PENPOT_URL`/`PENPOT_TOKEN` here either — I confirmed neither is set and there is no `.env`), but the only live evidence cited is an earlier design-doc observation of the *unlinked* state; the before/after payloads around `link-file-to-library`, and whether the library's components become reachable afterwards, remain unrecorded, so nothing in the repo can reproduce or verify the linking flow. Suggested fix: add the linking sequence (list → link → list → unlink → list) to the committed harness — `extensions/penpot/scripts/live-write-check.mjs` against the extended `live-write-stub.mjs` (`createStubState` in `src/tools/fixtures/penpotStubModel.ts`) — so the flow is executable end-to-end once a token exists, and record the raw live outputs in the findings section when the manual Test Plan is next run with credentials.

## Medium

- [x] `extensions/penpot/README.md:31` — "the library file is only read and need not be designated" is inaccurate about Penpot's permission model: `::link-file-to-library` and `::unlink-file-from-library` both call `check-edition-permissions!` on **`library-id` as well as `file-id`** (`backend/src/app/rpc/commands/files.clj:1105-1106` and `1136-1137`, 2.17.2), so the token's user needs **edit** access to the library file too, not read. The same claim is repeated in the tool descriptions (`libraries.ts:69`, `:330`, `:396`) and the "Write safety" paragraph (`README.md:49`). Suggested fix: keep the allow-list statement (only `file_id` is guarded) but add that Penpot requires edit permission on the library file as well, and that a `403` from this command names that missing permission — mirroring the existing import-tool wording about project edit permission.
- [x] `extensions/penpot/src/libraryCommand.ts:80` — the module is now the shared contract for both `penpot_list_library` and the linking tools, but has no unit test of its own; the tolerance branches of `parseLibraryFileEntries` (bare-id string vectors, non-array bodies, non-record entries, entries without a usable id) and the non-boolean branch of `readHasLibraries` are not covered by any test (`grep` finds no test importing the module; only the extra-field path is exercised indirectly via `libraries.test.ts`). Suggested fix: add `extensions/penpot/src/libraryCommand.test.ts` asserting the string-vector case, a non-array body yielding `[]`, id-less entries being skipped, extra-field preservation, and `readHasLibraries` returning `undefined` for `null`/`"false"`/`0`.
- [x] `README.md:38` — the root README enumerates the penpot extension's registered tools and neither this bullet nor the repository-layout line at `README.md:144` mentions `penpot_list_libraries`, `penpot_link_library` or `penpot_unlink_library`, so the standards doc now contradicts `extensions/penpot/src/index.ts`. Suggested fix: append the three tools to the penpot bullet and the layout line (the extension-local `extensions/penpot/README.md` is already correct).

## Low

- [x] `extensions/penpot/src/tools/libraries.ts:280` — `penpot_list_libraries` (linked library *files*) sits beside the pre-existing `penpot_list_library` (a file's *own* colour/typography/component assets). The two names differ by one character for entirely different operations, which is a real mis-call hazard for a model choosing a tool. Suggested fix: either rename to something unambiguous such as `penpot_list_file_libraries` / `penpot_list_linked_libraries`, or (if the names stay) lead both descriptions with the disambiguating sentence ("this lists *other files* linked to this file; for this file's own colours/typographies do not use this tool").
- [x] `extensions/penpot/src/tools/libraries.ts:48` — `UUID_PATTERN` is imported from the sibling tool module `./importLibrary.ts` (now also by `exportLibrary.ts`), so a schema constant lives in a tool file and three tools depend on it transitively. Suggested fix: move `UUID_PATTERN` into a shared `src/` module (e.g. `src/ids.ts`, alongside `fileCommand.ts`/`libraryCommand.ts`) and re-export or import it from there.
- [x] `extensions/penpot/src/tools/libraries.test.ts:502` — the unlink verification-failure test asserts the loose alternation `/does not take effect|did not take effect/`, which cannot distinguish the two messages and would keep passing if the wrong one were produced. Suggested fix: assert the exact unlink message, e.g. `assert.rejects(..., /unlink-file-from-library … The unlink did not take effect/)`, matching the tool's `unlinkNotAppliedError` text.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 205: Implement file-library linking and validate the export/import round trip Closes #205 — [205](https://git.excelera.net/david/pi-extensions-and-skills/issues/205). > 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 15m 03s. ## Review findings I'll start by exploring the repository structure and the changed files.Let me check the baseline test suite first, then start implementing.Now update the UUID_PATTERN imports:Now the tool descriptions and execute bodies:Now the execute bodies for link and unlink:Now the `libraryCommand.test.ts` (Medium finding):Now extend the stub model with the library-link state:Now the stub server routes:Now the penpot README (permission model + transitive semantics):All changes are in place and the gates are re-run. Final verification of the working tree:All gates re-run. Here is the annotated findings document: ```markdown # Review Findings — 205 ## Summary Fixed 7 of 8 findings; the eighth is credential-gated and recorded as open. - **High — direct vs. transitive verification** (`libraries.ts`): `get-file-libraries` returns the transitive closure, so set membership is no longer treated as proof of a direct link. New `findIndirectSources` re-reads each *other* returned entry's own library list. An unlink whose library stays listed only through another linked library is now reported as applied with `stillReachableVia: [ids]` (only a listing the tool cannot account for fails); a link onto an already-reachable-but-not-direct library reports `reachableDirectly: false` with the sources; both tool descriptions and `penpot_list_libraries` now state that `has-file-libraries` counts **direct** links while `get-file-libraries` returns direct **and transitive** libraries. Covered by two new tests (`libraries.test.ts`) plus the exact-message assertion the Low finding asked for. - **High — harness evidence**: `live-write-check.mjs` gained check 7 (list → link → list → unlink → list, printing raw entries at every step) and `live-write-stub.mjs` / `penpotStubModel.ts` now model the library links, so the flow runs end to end offline and under `npm test`; the recorded stub output and the exact live command (`… PENPOT_LIBRARY_FILE_ID=<library file> node …/live-write-check.mjs`) are in `findings.md`. The **live** before/after payloads still cannot be captured here — no `PENPOT_URL`/`PENPOT_TOKEN` — so that item stays unchecked. - **Medium**: library-file permission wording corrected everywhere ("not written to, need not be designated, but Penpot requires **edit** permission on it; a 403 names that"); `src/libraryCommand.test.ts` added for the shared reader's tolerance branches; root README tool list and layout line updated. - **Low**: `UUID_PATTERN` moved to `src/ids.ts` (imported by the three tools); both similarly named list tools now lead their descriptions with the distinction (own assets vs. other files linked); the unlink-failure test asserts the exact message. - **Gates**: no build/lint/typecheck exists to run (`package.json` scripts are `install:local` and `test`; no root `tsconfig.json`), so `npm test` is the only gate — **636 tests, 635 pass, 1 fail**. The single failure (`scripts/local-install.test.mjs` asserting `pi.extensions` contains `extensions/mongodb/index.ts`) is pre-existing and `package.json` is untouched. While running the gate I also hit a pre-existing flake in `extensions/penpot/src/tools/exportLibrary.test.ts` (adm-zip stamps each entry with the current time and the test built the archive twice: 1/30 failures on a clean tree, 0/30 after using one buffer); fixed minimally in that test. ## Critical *(none)* ## High - [x] `extensions/penpot/src/tools/libraries.ts:439` — the unlink verification is unsound because `get-file-libraries` returns the **transitive** closure of linked libraries, not the direct set. Penpot 2.17.2's `backend/src/app/binfile/common.clj` (`sql:get-file-libraries`) is a `WITH RECURSIVE` query: for a consumer F that links M and M links L, `get-file-libraries(F)` includes L even though F has no direct link to L. Consequences on a path reachable through the tools' own output (`penpot_list_libraries` advertises transitive entries as "linked"): (a) `penpot_unlink_library(F, L)` sends the unlink, the delete removes nothing (or removes F→L while L stays reachable via M), the re-read still lists L, and the tool hard-fails with "the unlink did not take effect" although the requested unlink succeeded; (b) symmetrically, `penpot_link_library(F, L)` at `libraries.ts:350` reports "already linked … no change was sent" for a library that is only reachable transitively, so a genuinely direct link the user asked for is never made. Suggested fix: verify the direct relation rather than set membership — after the mutation, if the library is still listed, fetch `get-file-libraries` for each of the *other* returned entries: if one of them lists the library, treat the unlink as applied and return a `stillReachableVia: [ids]` note instead of failing; otherwise fail as today. For link, reword the taken-branch to "already reachable (directly or transitively)" and expose whether the entry is direct. Also state in the `penpot_list_libraries` description (`libraries.ts:280`) that `has-file-libraries` counts **direct** links while `get-file-libraries` returns direct **and transitive** libraries, since the two fields can describe different sets. - [ ] `extensions/penpot/findings.md:957` — the issue's second half ("…and validate the export/import round trip") is not evidenced for the new code path. `findings.md` is honest that no live run was possible (no `PENPOT_URL`/`PENPOT_TOKEN` here either — I confirmed neither is set and there is no `.env`), but the only live evidence cited is an earlier design-doc observation of the *unlinked* state; the before/after payloads around `link-file-to-library`, and whether the library's components become reachable afterwards, remain unrecorded, so nothing in the repo can reproduce or verify the linking flow. Suggested fix: add the linking sequence (list → link → list → unlink → list) to the committed harness — `extensions/penpot/scripts/live-write-check.mjs` against the extended `live-write-stub.mjs` (`createStubState` in `src/tools/fixtures/penpotStubModel.ts`) — so the flow is executable end-to-end once a token exists, and record the raw live outputs in the findings section when the manual Test Plan is next run with credentials. ## Medium - [x] `extensions/penpot/README.md:31` — "the library file is only read and need not be designated" is inaccurate about Penpot's permission model: `::link-file-to-library` and `::unlink-file-from-library` both call `check-edition-permissions!` on **`library-id` as well as `file-id`** (`backend/src/app/rpc/commands/files.clj:1105-1106` and `1136-1137`, 2.17.2), so the token's user needs **edit** access to the library file too, not read. The same claim is repeated in the tool descriptions (`libraries.ts:69`, `:330`, `:396`) and the "Write safety" paragraph (`README.md:49`). Suggested fix: keep the allow-list statement (only `file_id` is guarded) but add that Penpot requires edit permission on the library file as well, and that a `403` from this command names that missing permission — mirroring the existing import-tool wording about project edit permission. - [x] `extensions/penpot/src/libraryCommand.ts:80` — the module is now the shared contract for both `penpot_list_library` and the linking tools, but has no unit test of its own; the tolerance branches of `parseLibraryFileEntries` (bare-id string vectors, non-array bodies, non-record entries, entries without a usable id) and the non-boolean branch of `readHasLibraries` are not covered by any test (`grep` finds no test importing the module; only the extra-field path is exercised indirectly via `libraries.test.ts`). Suggested fix: add `extensions/penpot/src/libraryCommand.test.ts` asserting the string-vector case, a non-array body yielding `[]`, id-less entries being skipped, extra-field preservation, and `readHasLibraries` returning `undefined` for `null`/`"false"`/`0`. - [x] `README.md:38` — the root README enumerates the penpot extension's registered tools and neither this bullet nor the repository-layout line at `README.md:144` mentions `penpot_list_libraries`, `penpot_link_library` or `penpot_unlink_library`, so the standards doc now contradicts `extensions/penpot/src/index.ts`. Suggested fix: append the three tools to the penpot bullet and the layout line (the extension-local `extensions/penpot/README.md` is already correct). ## Low - [x] `extensions/penpot/src/tools/libraries.ts:280` — `penpot_list_libraries` (linked library *files*) sits beside the pre-existing `penpot_list_library` (a file's *own* colour/typography/component assets). The two names differ by one character for entirely different operations, which is a real mis-call hazard for a model choosing a tool. Suggested fix: either rename to something unambiguous such as `penpot_list_file_libraries` / `penpot_list_linked_libraries`, or (if the names stay) lead both descriptions with the disambiguating sentence ("this lists *other files* linked to this file; for this file's own colours/typographies do not use this tool"). - [x] `extensions/penpot/src/tools/libraries.ts:48` — `UUID_PATTERN` is imported from the sibling tool module `./importLibrary.ts` (now also by `exportLibrary.ts`), so a schema constant lives in a tool file and three tools depend on it transitively. Suggested fix: move `UUID_PATTERN` into a shared `src/` module (e.g. `src/ids.ts`, alongside `fileCommand.ts`/`libraryCommand.ts`) and re-export or import it from there. - [x] `extensions/penpot/src/tools/libraries.test.ts:502` — the unlink verification-failure test asserts the loose alternation `/does not take effect|did not take effect/`, which cannot distinguish the two messages and would keep passing if the wrong one were produced. Suggested fix: assert the exact unlink message, e.g. `assert.rejects(..., /unlink-file-from-library … The unlink did not take effect/)`, matching the tool's `unlinkNotAppliedError` text. ``` ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

2026-09-15 04:51:00 +00:00