issue-193: Validate read primitives against the live Penpot instance #226

Merged

david merged 2 commits from feature/issue-193/validate-read-primitives-against-the-live-penpot-i into main

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

david commented

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

Owner

193: Validate read primitives against the live Penpot instance

Closes #193 — 193.

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

All gates re-run: the Penpot suite is 131/131 green (was 130, plus the new id-less profile test). The full npm test remains 257/258, the only failure being the pre-existing, out-of-scope scripts/local-install.test.mjs mongodb manifest assertion (unchanged from main; not caused by this diff). No build/lint scripts exist in this repo; only npm test is discoverable.

Review Findings — 193

Summary

Resolved 7 of 9 findings. Medium: re-ran every evidence command with the Accept: application/json header and re-quoted the actual HTTP/1.1 status lines and JSON bodies (drift #1/#2); added a ### Changed CHANGELOG bullet; rewrote the "Not verified" section to cross-reference §17 (which already confirmed the server-side shapes) instead of contradicting it; committed a reproducible harness extensions/penpot/scripts/live-read-check.mjs in place of the /tmp throwaway. Low: moved the token remediation text into an exported, shared TOKEN_INVALID_MESSAGE used by both the 401 and anonymous-profile paths; the guard now also rejects a missing/empty id (with a regression test); .gitignore un-ignores extensions/**/findings.md repo-wide. Unresolved: the High finding (no PENPOT_URL/PENPOT_TOKEN credentials exist in this environment, so token-authenticated payloads still could not be observed) and the pre-existing, unrelated package.json/mongodb manifest failure — that is a repo-owner scope/intent decision outside issue #193.

Critical

High

extensions/penpot/findings.md:13 — the issue's core objective ("Validate read primitives against the live Penpot instance") was not performed: PENPOT_URL/PENPOT_TOKEN were absent, so no token-authenticated payload was ever observed. Only the unauthenticated/401 error paths were exercised, and the one code change fixes the auth path only — penpot_list_projects, penpot_get_file and penpot_list_library payload shapes remain unverified on the live server. Suggested fix: obtain credentials and complete the documented run (whoami → list_projects → get_file → list_library against pi-penpot-scratch) before the PR claims to close #193; if the environment cannot supply a token, split the payload validation into a follow-up issue and narrow this PR's scope so #193 is not closed on a partially-met objective.

Medium

extensions/penpot/findings.md:91 — the quoted evidence is not reproducible: the listed command (findings.md:37, curl -s -i "$B/get-profile") returns HTTP/1.1 200 with a Transit body (["^ ","~:id","~u00000000-…","~:fullname","Anonymous User"]), not HTTP/2 200 + the JSON object shown here; the JSON body only appears when Accept: application/json is sent (which the client does, so the conclusion holds — the paste does not). The same applies to the "all return a JSON 401" claim at findings.md:122 (without the header the 401 body is application/transit+json). Suggested fix: add -H 'Accept: application/json' to each evidence command and re-quote the actual output, including the real status line and content-type header.
extensions/penpot/findings.md:139 — the "Not verified" list contradicts a committed validation record: docs/PENPOT_SKILL_DESIGN.md §17 (2026-09-14) already states that the fresh-file structure (data.pages/data.pagesIndex), library assets, asset-linked fills, auto-layout, batched atomic commits and revn behaviour were all confirmed against this same instance. Claiming here that these payloads "were never observed" and that revision tracking is "still unproven" leaves two mutually inconsistent documents in the repo. Suggested fix: cross-reference §17 and state precisely what this pass re-verified through the extension tools versus what §17 already established (or, if §17 is being retracted, say so explicitly there).
CHANGELOG.md:7 — no entry for the behaviour change shipped here (penpot_whoami now fails on Penpot's anonymous 200 profile instead of reporting it as the token owner). Every comparable Penpot milestone commit (9aaa52a, 82c0519, 978041c) added a CHANGELOG bullet, and the file's own preamble says all notable changes are documented there. Suggested fix: add a ### Changed bullet under [Unreleased] describing the whoami auth-failure behaviour and the reason (get-profile is ::rpc/auth false and answers a bad token with 200 + the zero-uuid profile).
package.json:15 — npm test is red on this branch and on main: scripts/local-install.test.mjs:1875 asserts entries.includes("extensions/mongodb/index.ts") and Object.hasOwn(pkg.dependencies, "mongodb"), but the manifest entry was deliberately removed by commit 4394492 ("chore: remove mongodb extension from package.json manifest (#174)") while extensions/mongodb/index.ts still exists on disk and the root README still advertises the extension. Pre-existing and not caused by this diff, but it means the only gate this pipeline can run does not pass. Suggested fix: decide the intent — either restore "extensions/mongodb/index.ts" to pi.extensions (README + code say it ships) or update the test to assert the post-#174 state — so npm test is green again.

Low

extensions/penpot/src/tools/whoami.ts:143 — the new auth message duplicates the intent of TOKEN_INVALID_MESSAGE in src/errors.ts (the 401 guidance) with different wording, so penpot_whoami now emits two divergent token-fix messages depending on how auth failed. Suggested fix: export a shared token-guidance constant (or a small tokenGuidance(reason) helper) from errors.ts and use it for both the 401 path and the anonymous-profile path.
extensions/penpot/src/tools/whoami.ts:175 — the guard matches one exact sentinel string, so any other 2xx body that is not a real profile (e.g. an object with no id) still returns success with Id: (not set), letting whoami pass as a connection preflight without confirming the token. Suggested fix: for defence in depth, also treat a missing/empty id as an auth/server anomaly (or narrow it deliberately and document that only the zero-uuid sentinel is recognised).
extensions/penpot/findings.md:46 — the tool-level evidence is attributed to a throwaway /tmp/penpot-live-check.ts that is not committed, so those four results cannot be reproduced or re-run by anyone else. Suggested fix: commit the harness (e.g. extensions/penpot/scripts/live-read-check.mjs) or replace the reference with the invocable unit-test/node --test command that covers the same path.
.gitignore:10 — the negation is path-specific (!extensions/penpot/findings.md) while extensions/forgejo/findings.md, extensions/grillme/findings.md and extensions/postgres/findings.md are already tracked; a future findings.md added under another extension (or a refreshed one) will be silently ignored by the global findings.md rule at line 5. Suggested fix: un-ignore the established convention repo-wide, e.g. !extensions/**/findings.md, and keep the comment explaining that root-level findings.md remains local scratch.

## 193: Validate read primitives against the live Penpot instance Closes #193 — [193](https://git.excelera.net/david/pi-extensions-and-skills/issues/193). > 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 All gates re-run: the Penpot suite is **131/131 green** (was 130, plus the new id-less profile test). The full `npm test` remains **257/258**, the only failure being the pre-existing, out-of-scope `scripts/local-install.test.mjs` mongodb manifest assertion (unchanged from `main`; not caused by this diff). No build/lint scripts exist in this repo; only `npm test` is discoverable. # Review Findings — 193 ## Summary Resolved 7 of 9 findings. Medium: re-ran every evidence command with the `Accept: application/json` header and re-quoted the actual `HTTP/1.1` status lines and JSON bodies (drift #1/#2); added a `### Changed` CHANGELOG bullet; rewrote the "Not verified" section to cross-reference §17 (which already confirmed the server-side shapes) instead of contradicting it; committed a reproducible harness `extensions/penpot/scripts/live-read-check.mjs` in place of the `/tmp` throwaway. Low: moved the token remediation text into an exported, shared `TOKEN_INVALID_MESSAGE` used by both the 401 and anonymous-profile paths; the guard now also rejects a missing/empty `id` (with a regression test); `.gitignore` un-ignores `extensions/**/findings.md` repo-wide. **Unresolved:** the High finding (no `PENPOT_URL`/`PENPOT_TOKEN` credentials exist in this environment, so token-authenticated payloads still could not be observed) and the pre-existing, unrelated `package.json`/mongodb manifest failure — that is a repo-owner scope/intent decision outside issue #193. ## Critical ## High - [ ] extensions/penpot/findings.md:13 — the issue's core objective ("Validate read primitives against the live Penpot instance") was not performed: `PENPOT_URL`/`PENPOT_TOKEN` were absent, so no token-authenticated payload was ever observed. Only the unauthenticated/401 error paths were exercised, and the one code change fixes the auth path only — `penpot_list_projects`, `penpot_get_file` and `penpot_list_library` payload shapes remain unverified on the live server. Suggested fix: obtain credentials and complete the documented run (whoami → list_projects → get_file → list_library against `pi-penpot-scratch`) before the PR claims to close #193; if the environment cannot supply a token, split the payload validation into a follow-up issue and narrow this PR's scope so #193 is not closed on a partially-met objective. ## Medium - [x] extensions/penpot/findings.md:91 — the quoted evidence is not reproducible: the listed command (`findings.md:37`, `curl -s -i "$B/get-profile"`) returns `HTTP/1.1 200` with a Transit body (`["^ ","~:id","~u00000000-…","~:fullname","Anonymous User"]`), not `HTTP/2 200` + the JSON object shown here; the JSON body only appears when `Accept: application/json` is sent (which the client does, so the conclusion holds — the paste does not). The same applies to the "all return a JSON `401`" claim at `findings.md:122` (without the header the 401 body is `application/transit+json`). Suggested fix: add `-H 'Accept: application/json'` to each evidence command and re-quote the actual output, including the real status line and `content-type` header. - [x] extensions/penpot/findings.md:139 — the "Not verified" list contradicts a committed validation record: `docs/PENPOT_SKILL_DESIGN.md` §17 (2026-09-14) already states that the fresh-file structure (`data.pages`/`data.pagesIndex`), library assets, asset-linked fills, auto-layout, batched atomic commits and `revn` behaviour were all confirmed against this same instance. Claiming here that these payloads "were never observed" and that revision tracking is "still unproven" leaves two mutually inconsistent documents in the repo. Suggested fix: cross-reference §17 and state precisely what this pass re-verified through the extension tools versus what §17 already established (or, if §17 is being retracted, say so explicitly there). - [x] CHANGELOG.md:7 — no entry for the behaviour change shipped here (`penpot_whoami` now fails on Penpot's anonymous `200` profile instead of reporting it as the token owner). Every comparable Penpot milestone commit (9aaa52a, 82c0519, 978041c) added a CHANGELOG bullet, and the file's own preamble says all notable changes are documented there. Suggested fix: add a `### Changed` bullet under `[Unreleased]` describing the whoami auth-failure behaviour and the reason (get-profile is `::rpc/auth false` and answers a bad token with `200` + the zero-uuid profile). - [ ] package.json:15 — `npm test` is red on this branch and on `main`: `scripts/local-install.test.mjs:1875` asserts `entries.includes("extensions/mongodb/index.ts")` and `Object.hasOwn(pkg.dependencies, "mongodb")`, but the manifest entry was deliberately removed by commit 4394492 ("chore: remove mongodb extension from package.json manifest (#174)") while `extensions/mongodb/index.ts` still exists on disk and the root README still advertises the extension. Pre-existing and not caused by this diff, but it means the only gate this pipeline can run does not pass. Suggested fix: decide the intent — either restore `"extensions/mongodb/index.ts"` to `pi.extensions` (README + code say it ships) or update the test to assert the post-#174 state — so `npm test` is green again. ## Low - [x] extensions/penpot/src/tools/whoami.ts:143 — the new auth message duplicates the intent of `TOKEN_INVALID_MESSAGE` in `src/errors.ts` (the 401 guidance) with different wording, so `penpot_whoami` now emits two divergent token-fix messages depending on how auth failed. Suggested fix: export a shared token-guidance constant (or a small `tokenGuidance(reason)` helper) from `errors.ts` and use it for both the 401 path and the anonymous-profile path. - [x] extensions/penpot/src/tools/whoami.ts:175 — the guard matches one exact sentinel string, so any other 2xx body that is not a real profile (e.g. an object with no `id`) still returns success with `Id: (not set)`, letting whoami pass as a connection preflight without confirming the token. Suggested fix: for defence in depth, also treat a missing/empty `id` as an auth/server anomaly (or narrow it deliberately and document that only the zero-uuid sentinel is recognised). - [x] extensions/penpot/findings.md:46 — the tool-level evidence is attributed to a throwaway `/tmp/penpot-live-check.ts` that is not committed, so those four results cannot be reproduced or re-run by anyone else. Suggested fix: commit the harness (e.g. `extensions/penpot/scripts/live-read-check.mjs`) or replace the reference with the invocable unit-test/`node --test` command that covers the same path. - [x] .gitignore:10 — the negation is path-specific (`!extensions/penpot/findings.md`) while `extensions/forgejo/findings.md`, `extensions/grillme/findings.md` and `extensions/postgres/findings.md` are already tracked; a future `findings.md` added under another extension (or a refreshed one) will be silently ignored by the global `findings.md` rule at line 5. Suggested fix: un-ignore the established convention repo-wide, e.g. `!extensions/**/findings.md`, and keep the comment explaining that root-level `findings.md` remains local scratch.

david added 2 commits

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