issue-191: Implement the penpot_get_file tool (pages, objects and library assets) with unit tests #224

Merged

david merged 2 commits from feature/issue-191/implement-the-penpot-get-file-tool-pages-objects-a into main

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

david commented

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

Owner

191: Implement the penpot_get_file tool (pages, objects and library assets) with unit tests

Closes #191 — 191.

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

Review findings

I'll start by exploring the repo and understanding the current state.Now I'll apply the fixes. Starting with fileShape.ts:Now the tests in fileShape.test.ts:All gates pass. npm test runs 231 tests: 230 pass, 1 fails — scripts/local-install.test.mjs (the pre-existing "mongodb is declared" failure, untouched by this diff since no package.json change is involved). The penpot subset is 104/104.

Review Findings — 191

Summary

Fixed the High breadth-budget finding plus all three fixable Medium findings and all four Low findings: the object tree now has a total-node budget (MAX_OBJECT_TREE_NODES = 500) that marks the cut, children are ordered by the parent's server-maintained shapes array, shapes with an unknown parent survive as extra roots, a missing/non-numeric revn/vern is rejected as invalid-file instead of defaulted to 0, the NONE constant is now used (no dead code), the tool description names summary_only, and a CHANGELOG entry was added. Tests were added for each behaviour (breadth cut, shapes ordering, orphan visibility, non-numeric revision, wide-page text bound, real default details budget, and objectTree/objects absence on a default read). Left unresolved: the hand-authored fixture / missing live-capture validation (Medium), which cannot be verified without a reachable Penpot instance.

Critical

(none)

High

extensions/penpot/src/fileShape.ts:282 — the object tree is bounded only in depth (MAX_OBJECT_TREE_DEPTH at line 23); breadth is unbounded, so include_objects can flood the model-facing text channel. Verified: a page with 5,000 top-level layers produces 5008 text lines / 132,986 chars (and details was correctly withheld for exceeding the 150k budget, yet the text that reached the model was almost as large). On the "real design files" the module doc calls out, this defeats the stated budget. Suggested fix: add a total-node/character budget for the rendered tree (e.g. MAX_OBJECT_TREE_NODES), stop emitting nodes once reached, and mark the cut the same way truncated marks a depth cut; add a test with a wide page.

Medium

extensions/penpot/src/fileShape.ts:305 — child order is the object-map key order, not layer order: childrenByParent is built by iterating Object.entries(objects) and the server-maintained shapes array (the authoritative z-order/layer-panel order) is explicitly ignored. Verified: reversing the object-map order reverses the tree (Header/Sidebar/Footer → Footer/Sidebar/Header). Clojure map serialisation order is not meaningful, so the agent can read a "layer tree" in arbitrary order. Suggested fix: when a parent shape has a shapes array of child ids, order its children by that array, appending any map-order leftovers; keep map order as the fallback. The fixture already carries shapes arrays (Root Frame → Card, Card → rect/text) that no test exercises.
CHANGELOG.md:11 — no [Unreleased] → Added entry for penpot_get_file, while the previous two user-visible penpot tools (penpot_whoami, penpot_list_projects; lines 11–13) each added one. The file states "All notable changes to this project are documented in this file." Suggested fix: add a one-paragraph entry mirroring the 190 entry, describing the page_id/include_objects/summary_only switches and the read-only/revision-source role.
extensions/penpot/src/fileShape.ts:356 — a missing/non-numeric revn (and vern) is silently coalesced to 0, even though the tool's contract is that revn/vern are always in details because "the commit step tracks" them. This contradicts the repo's established "never fabricate data from a malformed success body" convention (whoami.ts rejects an empty 2xx body rather than inventing a profile; file.test.ts rejects a non-file body). Since Penpot silently accepts stale revisions, a 0 fallback handed to the write path is exactly the dangerous case the design warns about. Suggested fix: return invalid-file when revn is missing/non-finite (or make revn/vern optional and surface their absence), and test it.
extensions/penpot/src/tools/file.test.ts:35 / extensions/penpot/src/tools/fixtures/README.md:10 — the fixture is hand-authored from schemas, not captured (the issue's docs/reference/penpot-api/ download was absent). The parsing tests are therefore only as good as the author's assumptions: a wrong key/level (data.pages vs data.pagesIndex, page.objects, asset field names) would still pass green, so the plan's M1 "confirm the library listing matches what the Penpot UI shows" validation is not represented. Suggested fix: capture and scrub a real get-file response when an instance is reachable and re-key the assertions to it (or add an opt-in integration test gated on PENPOT_URL/PENPOT_TOKEN + a scratch file id, skipped by default as the plan specifies).

Low

extensions/penpot/src/fileShape.ts:428 — const NONE = "(none)" is dead code (unused; the empty-page branch in formatPages hardcodes "Pages: (none)"). Suggested fix: delete the constant, or use it in formatPages for the empty case.
extensions/penpot/src/fileShape.ts:316 — with the root frame present, topLevel is [root] only, so any shape whose parentId is not on the page is silently dropped from the tree while still counted by objectCount. Verified: {root, orphan(parentId: "missing-parent")} yields a tree of just Root Frame but objectCount: 2. Suggested fix: when the root exists, append parent-less/unknown-parent shapes as additional top-level roots (or document the omission).
extensions/penpot/src/tools/file.ts:114 — the tool-level description advertises page_id and include_objects but never mentions summary_only, so the switch is only discoverable from the parameter schema. Suggested fix: add a clause such as "and summary_only: false to render full asset records in the text".
extensions/penpot/src/tools/file.test.ts (whole file) — the MAX_DETAILS_OBJECT_CHARS (150k) default budget path and the objectsOmittedReason details field are never exercised end-to-end; only fileShape.test.ts covers omission via a 16-char override, and no test asserts objectTree is absent when include_objects is false. Suggested fix: add a tool-level test with a payload large enough to trip the real default (or expose the budget as a test seam), plus an assertion that details.pages[0].objectTree === undefined on the default read.

## 191: Implement the penpot_get_file tool (pages, objects and library assets) with unit tests Closes #191 — [191](https://git.excelera.net/david/pi-extensions-and-skills/issues/191). > 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 48s. ## Review findings I'll start by exploring the repo and understanding the current state.Now I'll apply the fixes. Starting with `fileShape.ts`:Now the tests in `fileShape.test.ts`:All gates pass. `npm test` runs 231 tests: 230 pass, 1 fails — `scripts/local-install.test.mjs` (the pre-existing "mongodb is declared" failure, untouched by this diff since no `package.json` change is involved). The penpot subset is 104/104. # Review Findings — 191 ## Summary Fixed the High breadth-budget finding plus all three fixable Medium findings and all four Low findings: the object tree now has a total-node budget (`MAX_OBJECT_TREE_NODES = 500`) that marks the cut, children are ordered by the parent's server-maintained `shapes` array, shapes with an unknown parent survive as extra roots, a missing/non-numeric `revn`/`vern` is rejected as `invalid-file` instead of defaulted to `0`, the `NONE` constant is now used (no dead code), the tool description names `summary_only`, and a CHANGELOG entry was added. Tests were added for each behaviour (breadth cut, `shapes` ordering, orphan visibility, non-numeric revision, wide-page text bound, real default details budget, and `objectTree`/`objects` absence on a default read). Left unresolved: the hand-authored fixture / missing live-capture validation (Medium), which cannot be verified without a reachable Penpot instance. ## Critical _(none)_ ## High - [x] `extensions/penpot/src/fileShape.ts:282` — the object tree is bounded only in **depth** (`MAX_OBJECT_TREE_DEPTH` at line 23); breadth is unbounded, so `include_objects` can flood the model-facing text channel. Verified: a page with 5,000 top-level layers produces **5008 text lines / 132,986 chars** (and `details` was correctly withheld for exceeding the 150k budget, yet the text that reached the model was almost as large). On the "real design files" the module doc calls out, this defeats the stated budget. Suggested fix: add a total-node/character budget for the rendered tree (e.g. `MAX_OBJECT_TREE_NODES`), stop emitting nodes once reached, and mark the cut the same way `truncated` marks a depth cut; add a test with a wide page. ## Medium - [x] `extensions/penpot/src/fileShape.ts:305` — child order is the object-map key order, not layer order: `childrenByParent` is built by iterating `Object.entries(objects)` and the server-maintained `shapes` array (the authoritative z-order/layer-panel order) is explicitly ignored. Verified: reversing the object-map order reverses the tree (Header/Sidebar/Footer → Footer/Sidebar/Header). Clojure map serialisation order is not meaningful, so the agent can read a "layer tree" in arbitrary order. Suggested fix: when a parent shape has a `shapes` array of child ids, order its children by that array, appending any map-order leftovers; keep map order as the fallback. The fixture already carries `shapes` arrays (`Root Frame` → Card, Card → rect/text) that no test exercises. - [x] `CHANGELOG.md:11` — no `[Unreleased] → Added` entry for `penpot_get_file`, while the previous two user-visible penpot tools (`penpot_whoami`, `penpot_list_projects`; lines 11–13) each added one. The file states "All notable changes to this project are documented in this file." Suggested fix: add a one-paragraph entry mirroring the 190 entry, describing the `page_id`/`include_objects`/`summary_only` switches and the read-only/revision-source role. - [x] `extensions/penpot/src/fileShape.ts:356` — a missing/non-numeric `revn` (and `vern`) is silently coalesced to `0`, even though the tool's contract is that `revn`/`vern` are always in `details` because "the commit step tracks" them. This contradicts the repo's established "never fabricate data from a malformed success body" convention (`whoami.ts` rejects an empty 2xx body rather than inventing a profile; `file.test.ts` rejects a non-file body). Since Penpot silently accepts stale revisions, a `0` fallback handed to the write path is exactly the dangerous case the design warns about. Suggested fix: return `invalid-file` when `revn` is missing/non-finite (or make `revn`/`vern` optional and surface their absence), and test it. - [ ] `extensions/penpot/src/tools/file.test.ts:35` / `extensions/penpot/src/tools/fixtures/README.md:10` — the fixture is hand-authored from schemas, not captured (the issue's `docs/reference/penpot-api/` download was absent). The parsing tests are therefore only as good as the author's assumptions: a wrong key/level (`data.pages` vs `data.pagesIndex`, `page.objects`, asset field names) would still pass green, so the plan's M1 "confirm the library listing matches what the Penpot UI shows" validation is not represented. Suggested fix: capture and scrub a real `get-file` response when an instance is reachable and re-key the assertions to it (or add an opt-in integration test gated on `PENPOT_URL`/`PENPOT_TOKEN` + a scratch file id, skipped by default as the plan specifies). ## Low - [x] `extensions/penpot/src/fileShape.ts:428` — `const NONE = "(none)"` is dead code (unused; the empty-page branch in `formatPages` hardcodes `"Pages: (none)"`). Suggested fix: delete the constant, or use it in `formatPages` for the empty case. - [x] `extensions/penpot/src/fileShape.ts:316` — with the root frame present, `topLevel` is `[root]` only, so any shape whose `parentId` is not on the page is silently dropped from the tree while still counted by `objectCount`. Verified: `{root, orphan(parentId: "missing-parent")}` yields a tree of just `Root Frame` but `objectCount: 2`. Suggested fix: when the root exists, append parent-less/unknown-parent shapes as additional top-level roots (or document the omission). - [x] `extensions/penpot/src/tools/file.ts:114` — the tool-level `description` advertises `page_id` and `include_objects` but never mentions `summary_only`, so the switch is only discoverable from the parameter schema. Suggested fix: add a clause such as "and summary_only: false to render full asset records in the text". - [x] `extensions/penpot/src/tools/file.test.ts` (whole file) — the `MAX_DETAILS_OBJECT_CHARS` (150k) default budget path and the `objectsOmittedReason` details field are never exercised end-to-end; only `fileShape.test.ts` covers omission via a 16-char override, and no test asserts `objectTree` is absent when `include_objects` is false. Suggested fix: add a tool-level test with a payload large enough to trip the real default (or expose the budget as a test seam), plus an assertion that `details.pages[0].objectTree === undefined` on the default read.

david added 2 commits

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