issue-192: Implement the penpot_list_library tool with name-to-id resolution and unit tests #225

Merged

david merged 2 commits from feature/issue-192/implement-the-penpot-list-library-tool-with-name-t into main

2026-09-15 00:12:35 +00:00

david commented

2026-09-15 00:12:34 +00:00

Owner

192: Implement the penpot_list_library tool with name-to-id resolution and unit tests

Closes #192 — 192.

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 7m 44s.

Review findings

I'll start by exploring the current state of the implementation files.Now let me read the library.ts file and fileShape.ts relevant sections.Let me look at parseAssetMap, asRecord, and the asset parsing in fileShape.ts.Let me look at the documentation lines to fix.Now I have enough understanding. Let me make the fixes, starting with the Medium fileShape.ts fix.Now the resolve.ts ordering fix:Now create the shared mapWithConcurrency module and update projects.ts:Now update library.ts. First the imports and constants:Now the documentation fixes:Let me verify localeCompare behavior empirically before writing the ordering test:Now add the tests. First the High-priority fuzzy tool-level test and Medium unnamed-asset test:Let me review the modified library.ts in full for consistency:Now let's run the tests. First the penpot suite:All 129 penpot tests pass. Let me check for any other references to the changed APIs and run the full suite:The full suite is 255/256 — the only failure is the pre-existing local-install.test.mjs:1875 mongodb manifest assertion, unrelated to this change. Let me verify TypeScript correctness of the generic helper if a compiler is available:Let me simplify the placeholder helper to avoid generic-spread type concerns:Let me review the complete diff:The diff looks clean. Let me confirm there are no other configured gates and re-run the full suite:All fixes are applied and the penpot suite is fully green (129/129). The full npm test remains at 255/256, with the only failure being the pre-existing, unrelated mongodb-manifest assertion in scripts/local-install.test.mjs (confirmed untouched by this diff).

Review Findings — 192

Summary

Gates: build — none configured (no build script; TypeScript is run directly via Node 22 type-stripping, and no tsc is installed, so there is no typecheck gate). test — npm test passed 252/253 before remediation and 255/256 after; the penpot suite (including the new resolve.test.ts and library.test.ts) is 129/129 green. The single failure (scripts/local-install.test.mjs:1875, asserts extensions/mongodb/index.ts is in pi.extensions) is pre-existing and unrelated — package.json is untouched by this diff. lint — none configured.

Remediation (single pass): added the missing fuzzy: true tool-level test (High); tightened extractLibraryAssets to require a real file map and added object-body coverage; mapped blank asset names to an (unnamed) placeholder; deduped library_file_ids and fanned them out with a shared bounded-concurrency helper; dropped the unused export on formatLibraryText; corrected the fuzzy/page-object wording across the tool description, both READMEs and the CHANGELOG; and made buildIndex's sort consistent with its (possibly folded) lookup key. All findings are resolved.

Critical

(none)

High

extensions/penpot/src/tools/library.test.ts:142 — the fuzzy parameter is declared, described in three docs, and wired to options.caseInsensitive, but no test invokes the tool with fuzzy: true (only the pure caseInsensitive paths in resolve.test.ts are covered), so the param → index → details/text wiring could regress silently. Suggested fix: add a tool-level test that runs with fuzzy: true over two case-variant names and asserts details.colors is keyed "flip7-gold" with both ids and that ambiguousNames/the banner include both original names.

Medium

extensions/penpot/src/fileShape.ts:527 — extractLibraryAssets only rejects non-records; any 2xx object body (e.g. {} or {"message":"…"}) is treated as a valid file with an empty library, so the tool reports "Colours: (none)" instead of the server anomaly its own doc comment promises (the test only feeds [1,2,3]). Suggested fix: require the body to actually be a file map — if (!isRecord(raw) || !isRecord(raw.data)) return undefined; (optionally also require asNonEmptyString(raw.id)) so a malformed body maps to invalidBodyError().
extensions/penpot/src/tools/library.ts:264 — assets whose name is missing/empty are indexed under "", producing a blank row ( → c1), details.colors key "", and a blank ambiguity banner (Ambiguous names (1): ) that reads as a bug; fileShape.parseAssetMap defaults the name to "". Suggested fix: in buildSource/formatTable, map an empty name to a (unnamed) placeholder before indexing/rendering (and exclude it from ambiguousNames), so blank-name assets never produce blank lines.

Low

extensions/penpot/src/tools/library.ts:465 — library_file_ids is fetched strictly sequentially and is never deduped (a repeated id, or the main file_id listed again, is fetched and rendered twice), whereas projects.ts:313 already provides a bounded-concurrency mapWithConcurrency helper for exactly this fan-out. Suggested fix: dedupe ids (drop any equal to params.file_id) and reuse the bounded-concurrency pattern from projects.ts.
extensions/penpot/src/tools/library.ts:300 — formatLibraryText is exported but used only inside this module, and its parameters are typed with the module-private BuiltSource/LibraryView interfaces, so the export is neither usable nor testable externally. Suggested fix: drop the export (keep it internal like the other formatters) or export BuiltSource/LibraryView alongside it.
extensions/penpot/src/tools/library.ts:440 — the tool description (and README.md:38, extensions/penpot/README.md:21, CHANGELOG.md:11) says fuzzy enables "case-insensitive matching", but this tool takes no name to match against: fuzzy only lowercases the details keys and folds case variants into one ambiguity group. Suggested fix: reword to "fold case-variant names into one ambiguous group and lowercase the index keys (lookups themselves are matched by the composition primitives)".
extensions/penpot/README.md:21 — "page objects are never fetched or returned" is inaccurate: penpot_list_library calls get-file, whose response body is the whole file including data.pages/data.pagesIndex; the tool simply never reads or returns them. Suggested fix: say "page objects are never read or returned" (same wording fix in CHANGELOG.md:11).
extensions/penpot/src/resolve.ts:66 — the doc claims index "Keys and groups are ordered alphabetically by name", but under caseInsensitive the key is lower-cased while the sort uses the original-case name, so key order is not strictly alphabetical (e.g. "Banana"/"apple" → keys banana, apple). Suggested fix: sort by the normalised key when caseInsensitive is set, or soften the doc to "groups preserve the case-sensitive name order".

## 192: Implement the penpot_list_library tool with name-to-id resolution and unit tests Closes #192 — [192](https://git.excelera.net/david/pi-extensions-and-skills/issues/192). > 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 7m 44s. ## Review findings I'll start by exploring the current state of the implementation files.Now let me read the library.ts file and fileShape.ts relevant sections.Let me look at `parseAssetMap`, `asRecord`, and the asset parsing in fileShape.ts.Let me look at the documentation lines to fix.Now I have enough understanding. Let me make the fixes, starting with the Medium `fileShape.ts` fix.Now the `resolve.ts` ordering fix:Now create the shared `mapWithConcurrency` module and update `projects.ts`:Now update `library.ts`. First the imports and constants:Now the documentation fixes:Let me verify `localeCompare` behavior empirically before writing the ordering test:Now add the tests. First the High-priority fuzzy tool-level test and Medium unnamed-asset test:Let me review the modified `library.ts` in full for consistency:Now let's run the tests. First the penpot suite:All 129 penpot tests pass. Let me check for any other references to the changed APIs and run the full suite:The full suite is 255/256 — the only failure is the pre-existing `local-install.test.mjs:1875` mongodb manifest assertion, unrelated to this change. Let me verify TypeScript correctness of the generic helper if a compiler is available:Let me simplify the placeholder helper to avoid generic-spread type concerns:Let me review the complete diff:The diff looks clean. Let me confirm there are no other configured gates and re-run the full suite:All fixes are applied and the penpot suite is fully green (129/129). The full `npm test` remains at 255/256, with the only failure being the pre-existing, unrelated mongodb-manifest assertion in `scripts/local-install.test.mjs` (confirmed untouched by this diff). # Review Findings — 192 ## Summary Gates: **build** — none configured (no `build` script; TypeScript is run directly via Node 22 type-stripping, and no `tsc` is installed, so there is no typecheck gate). **test** — `npm test` passed 252/253 before remediation and 255/256 after; the penpot suite (including the new `resolve.test.ts` and `library.test.ts`) is **129/129 green**. The single failure (`scripts/local-install.test.mjs:1875`, asserts `extensions/mongodb/index.ts` is in `pi.extensions`) is pre-existing and unrelated — `package.json` is untouched by this diff. **lint** — none configured. Remediation (single pass): added the missing `fuzzy: true` tool-level test (High); tightened `extractLibraryAssets` to require a real file map and added object-body coverage; mapped blank asset names to an `(unnamed)` placeholder; deduped `library_file_ids` and fanned them out with a shared bounded-concurrency helper; dropped the unused `export` on `formatLibraryText`; corrected the `fuzzy`/page-object wording across the tool description, both READMEs and the CHANGELOG; and made `buildIndex`'s sort consistent with its (possibly folded) lookup key. All findings are resolved. ## Critical *(none)* ## High - [x] `extensions/penpot/src/tools/library.test.ts:142` — the `fuzzy` parameter is declared, described in three docs, and wired to `options.caseInsensitive`, but no test invokes the tool with `fuzzy: true` (only the pure `caseInsensitive` paths in `resolve.test.ts` are covered), so the param → index → `details`/text wiring could regress silently. Suggested fix: add a tool-level test that runs with `fuzzy: true` over two case-variant names and asserts `details.colors` is keyed `"flip7-gold"` with both ids and that `ambiguousNames`/the banner include both original names. ## Medium - [x] `extensions/penpot/src/fileShape.ts:527` — `extractLibraryAssets` only rejects non-records; any 2xx *object* body (e.g. `{}` or `{"message":"…"}`) is treated as a valid file with an empty library, so the tool reports "Colours: (none)" instead of the server anomaly its own doc comment promises (the test only feeds `[1,2,3]`). Suggested fix: require the body to actually be a file map — `if (!isRecord(raw) || !isRecord(raw.data)) return undefined;` (optionally also require `asNonEmptyString(raw.id)`) so a malformed body maps to `invalidBodyError()`. - [x] `extensions/penpot/src/tools/library.ts:264` — assets whose `name` is missing/empty are indexed under `""`, producing a blank row (` → c1`), `details.colors` key `""`, and a blank ambiguity banner (`Ambiguous names (1): `) that reads as a bug; `fileShape.parseAssetMap` defaults the name to `""`. Suggested fix: in `buildSource`/`formatTable`, map an empty name to a `(unnamed)` placeholder before indexing/rendering (and exclude it from `ambiguousNames`), so blank-name assets never produce blank lines. ## Low - [x] `extensions/penpot/src/tools/library.ts:465` — `library_file_ids` is fetched strictly sequentially and is never deduped (a repeated id, or the main `file_id` listed again, is fetched and rendered twice), whereas `projects.ts:313` already provides a bounded-concurrency `mapWithConcurrency` helper for exactly this fan-out. Suggested fix: dedupe ids (drop any equal to `params.file_id`) and reuse the bounded-concurrency pattern from `projects.ts`. - [x] `extensions/penpot/src/tools/library.ts:300` — `formatLibraryText` is exported but used only inside this module, and its parameters are typed with the module-private `BuiltSource`/`LibraryView` interfaces, so the export is neither usable nor testable externally. Suggested fix: drop the `export` (keep it internal like the other formatters) or export `BuiltSource`/`LibraryView` alongside it. - [x] `extensions/penpot/src/tools/library.ts:440` — the tool description (and `README.md:38`, `extensions/penpot/README.md:21`, `CHANGELOG.md:11`) says `fuzzy` enables "case-insensitive matching", but this tool takes no name to match against: `fuzzy` only lowercases the `details` keys and folds case variants into one ambiguity group. Suggested fix: reword to "fold case-variant names into one ambiguous group and lowercase the index keys (lookups themselves are matched by the composition primitives)". - [x] `extensions/penpot/README.md:21` — "page objects are never fetched or returned" is inaccurate: `penpot_list_library` calls `get-file`, whose response body is the whole file including `data.pages`/`data.pagesIndex`; the tool simply never reads or returns them. Suggested fix: say "page objects are never read or returned" (same wording fix in `CHANGELOG.md:11`). - [x] `extensions/penpot/src/resolve.ts:66` — the doc claims index "Keys and groups are ordered alphabetically by name", but under `caseInsensitive` the key is lower-cased while the sort uses the original-case name, so key order is not strictly alphabetical (e.g. `"Banana"`/`"apple"` → keys `banana`, `apple`). Suggested fix: sort by the normalised key when `caseInsensitive` is set, or soften the doc to "groups preserve the case-sensitive name order".

david added 2 commits

2026-09-15 00:12:34 +00:00