issue-190: Implement the penpot_list_projects tool (teams and projects) with unit tests #223

Merged

david merged 3 commits from feature/issue-190/implement-the-penpot-list-projects-tool-teams-and into main

2026-09-14 23:56:57 +00:00

david commented

2026-09-14 23:56:56 +00:00

Owner

190: Implement the penpot_list_projects tool (teams and projects) with unit tests

Closes #190 — 190.

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 9m 26s.

Review findings

I'll start by exploring the penpot extension to understand the current state.Now I understand the structure. Let me check the git status and the current test baseline.Now let me apply the fixes to projects.ts:Now the test file:Now let me add the new tests for bounded concurrency, cancellation, and malformed teams:Now the index.test.ts registration-order fix:All fixes are in place and the suite is green. Final verification:# Review Findings — 190

Summary

All eight findings were addressed in extensions/penpot/src/tools/projects.ts, extensions/penpot/src/tools/projects.test.ts and extensions/penpot/src/index.test.ts, taking the penpot suite from 78 to 81 tests (13 in projects.test.ts, all passing, stable across repeated runs). The Critical RPC-contract fix sends the kebab-case team-id query key (TEAM_ID_QUERY_KEY) and the stub now asserts that spelling; the High fix parses the real {type, isOwner, isAdmin, canEdit} permission record (rendering only granted flags as owner/admin/edit, never the type sentinel) with fixtures updated to the captured camelCase shape; the Medium abort finding is handled by throwIfAborted guards in execute/attachProjects plus a signal.aborted rethrow, with new tests for mid-fan-out cancellation and for the bounded fan-out (six teams, observed max 4 in flight, team order preserved); the Low findings are covered by filtering get-teams entries without an id, adding additionalProperties: false with the exact team_id spelling in the parameter description, and asserting registration membership rather than order. Gates: the repo has no build/lint/typecheck command (no eslint/biome/tsconfig, node_modules/.bin has neither tsc nor a linter), so only npm test was run: 208 tests, 207 pass, 1 fail — the sole failure (scripts/local-install.test.mjs:1875, extensions/mongodb/index.ts missing from package.json pi.extensions) is pre-existing on main, untouched by this diff and unrelated to this issue (it was also present before remediation: 205/204/1). Residual risk: no PENPOT_URL/PENPOT_TOKEN is configured in this environment, so the issue's live check ("confirm no validation error from the server") could not be run; the Critical fix follows the reviewer's source-level analysis of Penpot 2.17 rather than an observed live response.

Critical

extensions/penpot/src/tools/projects.ts:209 — query: { teamId: team.id } sends the query key teamId (camelCase), but on Penpot 2.17 get-projects declares [:map {:title "get-projects"} [:team-id ::sm/uuid]] (backend/src/app/rpc/commands/projects.clj), and GET query params are keywordized verbatim by yetti (parse-query-data default key-fn keyword), with no camelCase→kebab normalisation in wrap-params-validation (sm/decoder schema sm/json-transformer — penpot's transformer only adds a :map-of compiler). So GET /api/rpc/command/get-projects?teamId=<uuid> fails param validation ("missing required key team-id") and every team is returned as error: … could not be listed while the tool still reports success. Note the live instance's own OpenAPI documents get-projects as POST with a JSON body {"teamId": …} (body keys are kebabed by json/read-kebab-key), which is why the camelCase name works for a body but not for a GET query. Suggested fix: send the kebab query key — query: { "team-id": team.id } (or switch to { method: "POST", body: { teamId: team.id } } as the OpenAPI documents) — and update projects.test.ts:59, :159, :247, :294 to read/assert team-id so the stub asserts the real contract; then run the issue's live check ("confirm no validation error from the server") before merging.

High

extensions/penpot/src/tools/projects.ts:126 — the permissions record branch of toStringList treats the server's permission map as {permission: boolean}, but get-teams returns process-permissions output: {:type :membership, :is-owner …, :is-admin …, :can-edit …} (backend/src/app/rpc/commands/teams.clj), serialized camelCase as {"type":"membership","isOwner":…,"isAdmin":…,"canEdit":…}. The truthy-key filter therefore emits the non-permission sentinel type and drops false flags, so an owner sees Permissions: canEdit, isAdmin, isOwner, type and a read-only member sees exactly Permissions: type. This misreports the acceptance criterion "the caller's permissions". Suggested fix: parse the known shape explicitly (drop type, render only real capability flags, e.g. edit, admin, owner, or canEdit=true, isAdmin=false, …) instead of a generic truthy-key map, and update the projects.test.ts:141 fixture to the real camelCase shape (the current {"can-edit": true, "can-read": true} invents a can-read permission and hides the sentinel).

Medium

extensions/penpot/src/tools/projects.ts:212 — a caller cancellation is swallowed: the client maps an AbortError to a transport PenpotError (client.ts describeFetchError), isGlobalFailure (line 185) only treats config/401 as global, so an aborted fan-out keeps issuing requests on the already-aborted signal and returns a normal "success" result where every team carries a the request was aborted error instead of propagating the cancellation. Suggested fix: in attachProjects (and before continuing the fan-out), check signal?.aborted (or give aborts their own error category) and rethrow, so execute fails fast on cancellation; add a test that aborts mid-fan-out and asserts the tool throws rather than returning a listing.
extensions/penpot/src/tools/projects.test.ts:133 — no test exercises the bounded fan-out. MAX_CONCURRENT_TEAM_REQUESTS (projects.ts:42) is the only guard against an unbounded request burst and the issue explicitly requires it, yet every test uses at most two teams, so the concurrency logic can regress silently. Suggested fix: add a test with > 4 teams whose stub records in-flight count and start order, asserting the observed maximum never exceeds 4 and that result order matches team order.
extensions/penpot/src/tools/projects.test.ts:141 — the team fixtures are not the shape Penpot actually returns (hand-written kebab-case permission keys including a non-existent can-read, and features/permissions never exercised in their real {type, isOwner, isAdmin, canEdit} form), so the suite gives false confidence about output correctness. Suggested fix: replace the fixtures with a captured real get-teams payload (camelCase keys, type sentinel) and assert the rendered permission text; this is the same fix as the High finding above and should land with it.

Low

extensions/penpot/src/tools/projects.ts:149 — a get-teams entry without an id becomes id: "", which then triggers get-projects with team-id= (empty) and surfaces a server validation error as that team's error. Suggested fix: filter out entries with no usable id (or set the entry's error locally to a malformed-team message) rather than issuing a doomed request.
extensions/penpot/src/tools/projects.ts:49 — the permissive schema (no additionalProperties: false) means a caller passing teamId — the exact name Penpot's own OpenAPI uses for this parameter — is silently ignored and returns the full team listing instead of an error, which is a plausible slip for a model reading the API docs. Suggested fix: keep the schema additive but reject unknown keys with additionalProperties: false (as whoami.ts does), or accept teamId as an alias; at minimum state the exact spelling in the parameter description.
extensions/penpot/src/index.test.ts:74 — the registration test asserts the exact order of registered tool names, coupling the test to registration order rather than membership. Suggested fix: assert the set of names (e.g. sort before deepEqual, or assert includes for each expected name) so reordering registrations doesn't produce a spurious failure.

Documentation

Updated files:

docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 190: Implement the penpot_list_projects tool (teams and projects) with unit tests Closes #190 — [190](https://git.excelera.net/david/pi-extensions-and-skills/issues/190). > 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 9m 26s. ## Review findings I'll start by exploring the penpot extension to understand the current state.Now I understand the structure. Let me check the git status and the current test baseline.Now let me apply the fixes to `projects.ts`:Now the test file:Now let me add the new tests for bounded concurrency, cancellation, and malformed teams:Now the `index.test.ts` registration-order fix:All fixes are in place and the suite is green. Final verification:# Review Findings — 190 ## Summary All eight findings were addressed in `extensions/penpot/src/tools/projects.ts`, `extensions/penpot/src/tools/projects.test.ts` and `extensions/penpot/src/index.test.ts`, taking the penpot suite from 78 to 81 tests (13 in `projects.test.ts`, all passing, stable across repeated runs). The Critical RPC-contract fix sends the kebab-case `team-id` query key (`TEAM_ID_QUERY_KEY`) and the stub now asserts that spelling; the High fix parses the real `{type, isOwner, isAdmin, canEdit}` permission record (rendering only granted flags as `owner`/`admin`/`edit`, never the `type` sentinel) with fixtures updated to the captured camelCase shape; the Medium abort finding is handled by `throwIfAborted` guards in `execute`/`attachProjects` plus a `signal.aborted` rethrow, with new tests for mid-fan-out cancellation and for the bounded fan-out (six teams, observed max 4 in flight, team order preserved); the Low findings are covered by filtering `get-teams` entries without an id, adding `additionalProperties: false` with the exact `team_id` spelling in the parameter description, and asserting registration membership rather than order. **Gates:** the repo has no build/lint/typecheck command (no eslint/biome/tsconfig, `node_modules/.bin` has neither `tsc` nor a linter), so only `npm test` was run: **208 tests, 207 pass, 1 fail** — the sole failure (`scripts/local-install.test.mjs:1875`, `extensions/mongodb/index.ts` missing from `package.json` `pi.extensions`) is pre-existing on `main`, untouched by this diff and unrelated to this issue (it was also present before remediation: 205/204/1). **Residual risk:** no `PENPOT_URL`/`PENPOT_TOKEN` is configured in this environment, so the issue's live check ("confirm no validation error from the server") could not be run; the Critical fix follows the reviewer's source-level analysis of Penpot 2.17 rather than an observed live response. ## Critical - [x] `extensions/penpot/src/tools/projects.ts:209` — `query: { teamId: team.id }` sends the query key `teamId` (camelCase), but on Penpot 2.17 `get-projects` declares `[:map {:title "get-projects"} [:team-id ::sm/uuid]]` (`backend/src/app/rpc/commands/projects.clj`), and GET query params are keywordized verbatim by yetti (`parse-query-data` default `key-fn keyword`), with no camelCase→kebab normalisation in `wrap-params-validation` (`sm/decoder schema sm/json-transformer` — penpot's transformer only adds a `:map-of` compiler). So `GET /api/rpc/command/get-projects?teamId=<uuid>` fails param validation ("missing required key team-id") and every team is returned as `error: … could not be listed` while the tool still reports success. Note the live instance's own OpenAPI documents `get-projects` as `POST` with a JSON body `{"teamId": …}` (body keys are kebabed by `json/read-kebab-key`), which is why the camelCase name works for a body but not for a GET query. Suggested fix: send the kebab query key — `query: { "team-id": team.id }` (or switch to `{ method: "POST", body: { teamId: team.id } }` as the OpenAPI documents) — and update `projects.test.ts:59`, `:159`, `:247`, `:294` to read/assert `team-id` so the stub asserts the real contract; then run the issue's live check ("confirm no validation error from the server") before merging. ## High - [x] `extensions/penpot/src/tools/projects.ts:126` — the `permissions` record branch of `toStringList` treats the server's permission map as `{permission: boolean}`, but `get-teams` returns `process-permissions` output: `{:type :membership, :is-owner …, :is-admin …, :can-edit …}` (`backend/src/app/rpc/commands/teams.clj`), serialized camelCase as `{"type":"membership","isOwner":…,"isAdmin":…,"canEdit":…}`. The truthy-key filter therefore emits the non-permission sentinel `type` and drops false flags, so an owner sees `Permissions: canEdit, isAdmin, isOwner, type` and a read-only member sees exactly `Permissions: type`. This misreports the acceptance criterion "the caller's permissions". Suggested fix: parse the known shape explicitly (drop `type`, render only real capability flags, e.g. `edit`, `admin`, `owner`, or `canEdit=true, isAdmin=false, …`) instead of a generic truthy-key map, and update the `projects.test.ts:141` fixture to the real camelCase shape (the current `{"can-edit": true, "can-read": true}` invents a `can-read` permission and hides the sentinel). ## Medium - [x] `extensions/penpot/src/tools/projects.ts:212` — a caller cancellation is swallowed: the client maps an `AbortError` to a `transport` `PenpotError` (`client.ts` `describeFetchError`), `isGlobalFailure` (line 185) only treats `config`/`401` as global, so an aborted fan-out keeps issuing requests on the already-aborted signal and returns a normal "success" result where every team carries a `the request was aborted` error instead of propagating the cancellation. Suggested fix: in `attachProjects` (and before continuing the fan-out), check `signal?.aborted` (or give aborts their own error category) and rethrow, so `execute` fails fast on cancellation; add a test that aborts mid-fan-out and asserts the tool throws rather than returning a listing. - [x] `extensions/penpot/src/tools/projects.test.ts:133` — no test exercises the bounded fan-out. `MAX_CONCURRENT_TEAM_REQUESTS` (`projects.ts:42`) is the only guard against an unbounded request burst and the issue explicitly requires it, yet every test uses at most two teams, so the concurrency logic can regress silently. Suggested fix: add a test with > 4 teams whose stub records in-flight count and start order, asserting the observed maximum never exceeds 4 and that result order matches team order. - [x] `extensions/penpot/src/tools/projects.test.ts:141` — the team fixtures are not the shape Penpot actually returns (hand-written kebab-case permission keys including a non-existent `can-read`, and `features`/`permissions` never exercised in their real `{type, isOwner, isAdmin, canEdit}` form), so the suite gives false confidence about output correctness. Suggested fix: replace the fixtures with a captured real `get-teams` payload (camelCase keys, `type` sentinel) and assert the rendered permission text; this is the same fix as the High finding above and should land with it. ## Low - [x] `extensions/penpot/src/tools/projects.ts:149` — a `get-teams` entry without an `id` becomes `id: ""`, which then triggers `get-projects` with `team-id=` (empty) and surfaces a server validation error as that team's error. Suggested fix: filter out entries with no usable id (or set the entry's `error` locally to a malformed-team message) rather than issuing a doomed request. - [x] `extensions/penpot/src/tools/projects.ts:49` — the permissive schema (no `additionalProperties: false`) means a caller passing `teamId` — the exact name Penpot's own OpenAPI uses for this parameter — is silently ignored and returns the full team listing instead of an error, which is a plausible slip for a model reading the API docs. Suggested fix: keep the schema additive but reject unknown keys with `additionalProperties: false` (as `whoami.ts` does), or accept `teamId` as an alias; at minimum state the exact spelling in the parameter description. - [x] `extensions/penpot/src/index.test.ts:74` — the registration test asserts the exact order of registered tool names, coupling the test to registration order rather than membership. Suggested fix: assert the set of names (e.g. sort before `deepEqual`, or assert `includes` for each expected name) so reordering registrations doesn't produce a spurious failure. ## Documentation Updated files: - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-14 23:56:56 +00:00