issue-187: Implement PENPOT_URL/PENPOT_TOKEN resolution in src/env.ts with unit tests #220

Merged

david merged 3 commits from feature/issue-187/implement-penpot-url-penpot-token-resolution-in-sr into main

2026-09-14 23:30:01 +00:00

david commented

2026-09-14 23:30:01 +00:00

Owner

187: Implement PENPOT_URL/PENPOT_TOKEN resolution in src/env.ts with unit tests

Closes #187 — 187.

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 3m 36s.

Review findings

I'll start by exploring the repository and the relevant files.Baseline confirmed: penpot tests pass (16/16); npm test has one pre-existing failure in scripts/local-install.test.mjs (mongodb removed from manifest in an earlier commit — not touched by this diff). Now applying fixes.The review's suggested regex spans lines and false-matches "http:" later in the file. I'll make the matcher line-bounded instead (still covering export … from and dynamic imports):All four findings are fixed and the penpot suite is green (18/18), now wired into npm test (145 tests, 144 pass — the single failure is the pre-existing, unrelated scripts/local-install.test.mjs mongodb-manifest assertion).

Review Findings — 187

Summary

Fixed all four findings. High: normalizeBaseUrl now strips all trailing slashes first (raw.trim().replace(/\/+$/, "")), then a trailing /api case-insensitively, then any slash left behind by that strip — https://host/api// and https://host/api/// now normalise to https://host and the function is genuinely idempotent; regression cases were added to both the dedicated test and the idempotency loop. Medium: npm test now includes extensions/penpot/src/*.test.ts, so those 18 tests actually run in the repo's documented gate (145 tests total, up from 127). Low (env.ts:83): resolveConfig now rejects any scheme other than http:/https: (verified by hand: ftp:, file:, urn: all return ok: false with the invalid-URL error), covered by a new parameterised test. Low (env.test.ts:151): the dependency-guard matcher now covers import … from, import "x", export … from and dynamic import("x") — note the finding's suggested regex was not used verbatim because [^'"]*? spans newlines: anchored at export function normalizeBaseUrl it ran forward to the unrelated "http:" literal, producing a false positive. I used a line-bounded variant ([^'"\n]*?) plus a separate dynamic-import pattern, and verified by hand that it does catch non-builtin specifiers from all four forms.

One issue remains outside this diff's scope and is not one of the findings: scripts/local-install.test.mjs asserts extensions/mongodb/index.ts is present in pi.extensions, which an earlier chore commit removed. That test file and the manifest are untouched here, so the npm test gate is still red for that pre-existing reason; I left it alone to keep this change reviewable. No lint/build/typecheck gate exists in this repo (no lint config, no tsconfig, no build script), so npm test is the only gate runnable.

Critical

(none)

High

extensions/penpot/src/env.ts:47 — normalizeBaseUrl strips only a single trailing slash before the /api check, so a doubled trailing slash defeats the api strip: https://host/api// → https://host/api (and https://host/api/// → https://host/api/), which would make the client request /api/api/rpc/command/.... This also contradicts the function's documented "Idempotent" claim (calling normalizeBaseUrl twice on https://host/api// yields two different values). It is reachable through resolveConfig({ PENPOT_URL: "https://host/api//", PENPOT_TOKEN: "t" }), which returns ok: true with the wrong base URL. Suggested fix: strip all trailing slashes first — url = url.replace(/\/+$/, "") (mirroring resolveBaseUrl in extensions/victorialogs/src/env.ts) — then strip a trailing /api case-insensitively, then strip one more trailing slash; add a regression test for https://host/api// and https://host/api/// to the existing normalizeBaseUrl/idempotency cases.

Medium

package.json:31 — the new test file is not wired into the npm test gate; the script only runs scripts/local-install.test.mjs extensions/learn-repo/*.test.ts, so extensions/penpot/src/env.test.ts never executes in the repo's documented test command (and there is no CI workflow to pick it up either). Suggested fix: extend the test script, e.g. node --test scripts/local-install.test.mjs extensions/learn-repo/*.test.ts extensions/penpot/src/*.test.ts. (The implementation plan defers manifest/test wiring to M9, but until then the tests added by this issue are silently unrun.)

Low

extensions/penpot/src/env.ts:83 — new URL(rawUrl) accepts any scheme, so ftp://host, file:///etc/passwd and even urn:isbn:123 pass validation and are reported as a usable baseUrl (resolveConfig returns ok: true), only failing later at request time. Suggested fix: after parsing, reject anything that is not http:/https: and return invalidUrlError(raw), e.g. const parsed = new URL(rawUrl); if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return { ok: false, error: invalidUrlError(rawUrl) };, plus a test for a non-http scheme.
extensions/penpot/src/env.test.ts:151 — the "env.ts imports no third-party package" guard only matches import … from "…" statements; it misses export … from "…" and dynamic import("…"), so it does not fully enforce the dependency-free constraint it claims to protect. Suggested fix: broaden the matcher to cover both forms, e.g. [...source.matchAll(/(?:^|\n)\s*(?:import|export)\s[^'"]*?['"]([^'"]+)['"]/gm)] plus [...source.matchAll(/\bimport\s*\(\s*['"]([^'"]+)['"]/g)], and assert the combined specifier list contains no non-node: entry.

Documentation

Updated files:

docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 187: Implement PENPOT_URL/PENPOT_TOKEN resolution in src/env.ts with unit tests Closes #187 — [187](https://git.excelera.net/david/pi-extensions-and-skills/issues/187). > 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 3m 36s. ## Review findings I'll start by exploring the repository and the relevant files.Baseline confirmed: penpot tests pass (16/16); `npm test` has one pre-existing failure in `scripts/local-install.test.mjs` (mongodb removed from manifest in an earlier commit — not touched by this diff). Now applying fixes.The review's suggested regex spans lines and false-matches `"http:"` later in the file. I'll make the matcher line-bounded instead (still covering `export … from` and dynamic imports):All four findings are fixed and the penpot suite is green (18/18), now wired into `npm test` (145 tests, 144 pass — the single failure is the pre-existing, unrelated `scripts/local-install.test.mjs` mongodb-manifest assertion). # Review Findings — 187 ## Summary Fixed all four findings. **High**: `normalizeBaseUrl` now strips *all* trailing slashes first (`raw.trim().replace(/\/+$/, "")`), then a trailing `/api` case-insensitively, then any slash left behind by that strip — `https://host/api//` and `https://host/api///` now normalise to `https://host` and the function is genuinely idempotent; regression cases were added to both the dedicated test and the idempotency loop. **Medium**: `npm test` now includes `extensions/penpot/src/*.test.ts`, so those 18 tests actually run in the repo's documented gate (145 tests total, up from 127). **Low (env.ts:83)**: `resolveConfig` now rejects any scheme other than `http:`/`https:` (verified by hand: `ftp:`, `file:`, `urn:` all return `ok: false` with the invalid-URL error), covered by a new parameterised test. **Low (env.test.ts:151)**: the dependency-guard matcher now covers `import … from`, `import "x"`, `export … from` and dynamic `import("x")` — note the finding's suggested regex was not used verbatim because `[^'"]*?` spans newlines: anchored at `export function normalizeBaseUrl` it ran forward to the unrelated `"http:"` literal, producing a false positive. I used a line-bounded variant (`[^'"\n]*?`) plus a separate dynamic-import pattern, and verified by hand that it does catch non-builtin specifiers from all four forms. One issue remains outside this diff's scope and is **not** one of the findings: `scripts/local-install.test.mjs` asserts `extensions/mongodb/index.ts` is present in `pi.extensions`, which an earlier chore commit removed. That test file and the manifest are untouched here, so the `npm test` gate is still red for that pre-existing reason; I left it alone to keep this change reviewable. No lint/build/typecheck gate exists in this repo (no lint config, no `tsconfig`, no build script), so `npm test` is the only gate runnable. ## Critical _(none)_ ## High - [x] `extensions/penpot/src/env.ts:47` — `normalizeBaseUrl` strips only a single trailing slash *before* the `/api` check, so a doubled trailing slash defeats the `api` strip: `https://host/api//` → `https://host/api` (and `https://host/api///` → `https://host/api/`), which would make the client request `/api/api/rpc/command/...`. This also contradicts the function's documented "Idempotent" claim (calling `normalizeBaseUrl` twice on `https://host/api//` yields two different values). It is reachable through `resolveConfig({ PENPOT_URL: "https://host/api//", PENPOT_TOKEN: "t" })`, which returns `ok: true` with the wrong base URL. Suggested fix: strip all trailing slashes first — `url = url.replace(/\/+$/, "")` (mirroring `resolveBaseUrl` in `extensions/victorialogs/src/env.ts`) — then strip a trailing `/api` case-insensitively, then strip one more trailing slash; add a regression test for `https://host/api//` and `https://host/api///` to the existing `normalizeBaseUrl`/idempotency cases. ## Medium - [x] `package.json:31` — the new test file is not wired into the `npm test` gate; the script only runs `scripts/local-install.test.mjs extensions/learn-repo/*.test.ts`, so `extensions/penpot/src/env.test.ts` never executes in the repo's documented test command (and there is no CI workflow to pick it up either). Suggested fix: extend the `test` script, e.g. `node --test scripts/local-install.test.mjs extensions/learn-repo/*.test.ts extensions/penpot/src/*.test.ts`. (The implementation plan defers manifest/test wiring to M9, but until then the tests added by this issue are silently unrun.) ## Low - [x] `extensions/penpot/src/env.ts:83` — `new URL(rawUrl)` accepts any scheme, so `ftp://host`, `file:///etc/passwd` and even `urn:isbn:123` pass validation and are reported as a usable `baseUrl` (`resolveConfig` returns `ok: true`), only failing later at request time. Suggested fix: after parsing, reject anything that is not `http:`/`https:` and return `invalidUrlError(raw)`, e.g. `const parsed = new URL(rawUrl); if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return { ok: false, error: invalidUrlError(rawUrl) };`, plus a test for a non-http scheme. - [x] `extensions/penpot/src/env.test.ts:151` — the "env.ts imports no third-party package" guard only matches `import … from "…"` statements; it misses `export … from "…"` and dynamic `import("…")`, so it does not fully enforce the dependency-free constraint it claims to protect. Suggested fix: broaden the matcher to cover both forms, e.g. `[...source.matchAll(/(?:^|\n)\s*(?:import|export)\s[^'"]*?['"]([^'"]+)['"]/gm)]` plus `[...source.matchAll(/\bimport\s*\(\s*['"]([^'"]+)['"]/g)]`, and assert the combined specifier list contains no non-`node:` entry. ## Documentation Updated files: - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-14 23:30:01 +00:00