issue-188: Implement the RPC client and error decoding in src/client.ts and src/errors.ts with unit tests #221

Merged

david merged 3 commits from feature/issue-188/implement-the-rpc-client-and-error-decoding-in-src into main

2026-09-14 23:37:53 +00:00

david commented

2026-09-14 23:37:53 +00:00

Owner

188: Implement the RPC client and error decoding in src/client.ts and src/errors.ts with unit tests

Closes #188 — 188.

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

Review findings

Now I'll apply the fixes. Starting with client.ts:Now errors.ts:Now add the regression tests. First client.test.ts:Now errors.test.ts:All 39 Penpot tests pass. Now the full suite (expecting the known pre-existing failure):Only the documented pre-existing local-install.test.mjs failure remains — no new regressions. Let me review the final diffs:All feasible findings are fixed and the Penpot suite is green (39/39); full suite shows only the documented pre-existing local-install.test.mjs failure. The commit-message finding requires rewriting git history, which this stage is explicitly forbidden from doing, so it remains unresolved.

Review Findings — 188

Summary

Fixed all code-level findings in client.ts and errors.ts, with regression tests for each: token redaction on the non-JSON success path, transport errors for failed body reads on 2xx, explicit T | undefined for empty/204 bodies, POST default when a body is supplied, Transit code extraction, status-aware Transit categorisation, a structural looksLikeTransit check, bounds-guarded Transit cache references, and corrected /api guidance. The Penpot suites pass 39/39; the full npm test run is 183 pass / 1 fail, the single failure being the pre-existing scripts/local-install.test.mjs:1875 issue unrelated to this diff. The commit-message finding could not be resolved without rewriting git history, which this stage is forbidden from doing.

Critical

extensions/penpot/src/client.ts:161 — the token is redacted only on the HTTP-error path (redactError is applied at line 237), so the non-JSON success-body error built in parseSuccessBody leaks the raw token into both message and rawBody. Reproduced: with PENPOT_TOKEN=secret-token-123 and a 200 text/plain body "leaked secret-token-123 here", penpotRequest("get-profile") returns message: "...non-JSON success body: leaked secret-token-123 here" and rawBody: "leaked secret-token-123 here". This directly violates the file's stated invariant ("The token is never interpolated into a message or error object") and the existing test only guards the 500 path. Suggested fix: build the error in parseSuccessBody without the token, then wrap every returned error in redactError(error, token) in penpotRequest (both the error branch and the final parseSuccessBody call), or pass token into parseSuccessBody; add a test mirroring penpotRequest never leaks the token into the returned error but with status: 200, content-type: text/html.

High

extensions/penpot/src/client.ts:227 — const rawText = await response.text().catch(() => ""); swallows a body-read failure (timeout/abort mid-read, truncated stream) and, on a 2xx, parseSuccessBody turns the empty string into { ok: true }. Reproduced with a 200 whose text() rejects: penpotRequest("get-file") returns {"ok":true} — a transport failure is reported as a successful read with no data, so a caller will act on undefined (or crash on value.id). Suggested fix: only fall back to "" when the response is non-ok; on a 2xx, surface the read failure as a transport error (e.g. try { rawText = await response.text() } catch (error) { return { ok: false, error: transportError(command, baseUrl, error) } }), and add a test for the aborted-body-read case.
extensions/penpot/src/client.ts:156 — return { ok: true, value: undefined as T } (and the type at line 24, { ok: true; value: T }) asserts a value of type T for an empty/204 body, so const r = await penpotRequest<Profile>("get-profile"); if (r.ok) r.value.id compiles but throws at runtime whenever the server returns an empty body. The test at client.test.ts (returns undefined for an empty success body) enshrines the unsafe behaviour rather than flagging it. Suggested fix: make the empty case explicit in the type — e.g. value?: T or PenpotResult<T | undefined> — or document/require callers to pass T | undefined; either way add a compile-time-visible distinction so callers must narrow before dereferencing.

Medium

extensions/penpot/src/errors.ts:367 — the Transit branch of decodeErrorBody returns only { category, message } and never populates DecodedErrorBody.code, even though the JSON branch and the client docstring advertise that callers (the M2 commit path watching for revn-conflict) can react to the server code. Reproduced: decodeErrorBody(400, "application/json", '["^ ","~:type","~:validation","~:code","~:revn-conflict","~:explain","bad revn"]') yields {category:"validation", message:"Transit-encoded error payload: bad revn …"} with no code. Suggested fix: extract the Transit map's code (decodeTransitMap(parsed.value)?.code) into the returned DecodedErrorBody, and add a client.test.ts case asserting result.error.code === "revn-conflict" round-trips from the response body.
extensions/penpot/src/errors.ts:270 — transitCategory ignores the HTTP status, so a 400 carrying a Transit body that lacks a ~:type/~:explain is categorised server, while the identical status via the JSON path is validation (statusCategory(400)). Reproduced: decodeErrorBody(400, "application/json", '["^ ","~#uri","https://x/a"]') → category: "server". Suggested fix: pass status into transitCategory and fall back to statusCategory(status) when the decoded payload does not explicitly say validation (i.e. return type === "validation" ? "validation" : statusCategory(status)).

Low

extensions/penpot/src/errors.ts:152 — looksLikeTransit uses trimmed.includes('"~:') / '"~#', so ordinary JSON error bodies whose string values start with a Transit marker are misclassified as Transit and their message is rewritten. Reproduced: decodeErrorBody(500, "application/json", '{"message":"~:foo"}') returns "Transit-encoded error payload: foo (raw body: {"message":"~:foo"})" instead of the server's message. Suggested fix: restrict the substring heuristic to keys/values that are themselves tagged (e.g. only treat it as Transit when a ~:/~# token occurs at a structural position, or require the parsed JSON to contain at least one ~-prefixed key), and add a regression test for a JSON body containing "~:.
extensions/penpot/src/errors.ts:229 — readEntry treats any integer in a cache-coded map as a cache reference and only pushes non-integer entries into cache, so a literal numeric value decodes to cache[n] (undefined) and later cache indices can desynchronise from Transit's own numbering. Reproduced: ["^ ","~:explain","oops","~:status",400] decodes status as undefined. Suggested fix: guard the lookup (typeof entry === "number" && entry >= 0 && entry < cache.length ? cache[entry] : entry) and, per the Transit cache-coding rules, only treat an integer as a reference when it is within the cache bounds; add a test with a numeric entry.
extensions/penpot/src/client.ts:204 — method = options.method ?? "GET" combined with the body handling at line 220 means penpotRequest("update-file", { body: {...} }) (body but no explicit method) sends a GET with a body, which fetch rejects with TypeError: Request with GET/HEAD method cannot have body; that is then surfaced as transport ("could not reach Penpot at …"), misattributing a caller error to the network. Suggested fix: default to POST when options.body !== undefined (or fail fast with a config/caller error explaining that a body requires method: "POST"), and add a test.
extensions/penpot/src/errors.ts:94 — NOT_FOUND_MESSAGE tells the user to "ensure PENPOT_URL is the instance base URL with no /api suffix", but env.ts:normalizeBaseUrl already strips a trailing /api before client.ts ever builds a URL (the client test at client.test.ts asserts this double-append cannot happen). The advice therefore contradicts actual behaviour and can send a user chasing a non-issue. Suggested fix: drop the /api-suffix clause or reword it to state that the suffix is normalised automatically, and keep the guidance to the command name / file id causes.
f73dc20 (commit message) — the commit is 188: Implement the RPC client …, but AGENTS.md specifies issue-<N>: <summary> for feature/bug/enhancement work (and the branch already correctly follows feature/issue-188/…). Suggested fix: amend the commit message to issue-188: Implement the RPC client and error decoding in src/client.ts and src/errors.ts with unit tests before the PR is opened.

Documentation

Updated files:

docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 188: Implement the RPC client and error decoding in src/client.ts and src/errors.ts with unit tests Closes #188 — [188](https://git.excelera.net/david/pi-extensions-and-skills/issues/188). > 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 50s. ## Review findings Now I'll apply the fixes. Starting with `client.ts`:Now `errors.ts`:Now add the regression tests. First `client.test.ts`:Now `errors.test.ts`:All 39 Penpot tests pass. Now the full suite (expecting the known pre-existing failure):Only the documented pre-existing `local-install.test.mjs` failure remains — no new regressions. Let me review the final diffs:All feasible findings are fixed and the Penpot suite is green (39/39); full suite shows only the documented pre-existing `local-install.test.mjs` failure. The commit-message finding requires rewriting git history, which this stage is explicitly forbidden from doing, so it remains unresolved. # Review Findings — 188 ## Summary Fixed all code-level findings in `client.ts` and `errors.ts`, with regression tests for each: token redaction on the non-JSON success path, transport errors for failed body reads on 2xx, explicit `T | undefined` for empty/204 bodies, `POST` default when a body is supplied, Transit `code` extraction, status-aware Transit categorisation, a structural `looksLikeTransit` check, bounds-guarded Transit cache references, and corrected `/api` guidance. The Penpot suites pass 39/39; the full `npm test` run is 183 pass / 1 fail, the single failure being the pre-existing `scripts/local-install.test.mjs:1875` issue unrelated to this diff. The commit-message finding could not be resolved without rewriting git history, which this stage is forbidden from doing. ## Critical - [x] `extensions/penpot/src/client.ts:161` — the token is redacted only on the HTTP-error path (`redactError` is applied at line 237), so the non-JSON *success*-body error built in `parseSuccessBody` leaks the raw token into both `message` and `rawBody`. Reproduced: with `PENPOT_TOKEN=secret-token-123` and a `200 text/plain` body `"leaked secret-token-123 here"`, `penpotRequest("get-profile")` returns `message: "...non-JSON success body: leaked secret-token-123 here"` and `rawBody: "leaked secret-token-123 here"`. This directly violates the file's stated invariant ("The token is never interpolated into a message or error object") and the existing test only guards the 500 path. Suggested fix: build the error in `parseSuccessBody` without the token, then wrap every returned error in `redactError(error, token)` in `penpotRequest` (both the error branch and the final `parseSuccessBody` call), or pass `token` into `parseSuccessBody`; add a test mirroring `penpotRequest never leaks the token into the returned error` but with `status: 200, content-type: text/html`. ## High - [x] `extensions/penpot/src/client.ts:227` — `const rawText = await response.text().catch(() => "");` swallows a body-read failure (timeout/abort mid-read, truncated stream) and, on a 2xx, `parseSuccessBody` turns the empty string into `{ ok: true }`. Reproduced with a `200` whose `text()` rejects: `penpotRequest("get-file")` returns `{"ok":true}` — a transport failure is reported as a successful read with no data, so a caller will act on `undefined` (or crash on `value.id`). Suggested fix: only fall back to `""` when the response is non-ok; on a 2xx, surface the read failure as a `transport` error (e.g. `try { rawText = await response.text() } catch (error) { return { ok: false, error: transportError(command, baseUrl, error) } }`), and add a test for the aborted-body-read case. - [x] `extensions/penpot/src/client.ts:156` — `return { ok: true, value: undefined as T }` (and the type at line 24, `{ ok: true; value: T }`) asserts a value of type `T` for an empty/`204` body, so `const r = await penpotRequest<Profile>("get-profile"); if (r.ok) r.value.id` compiles but throws at runtime whenever the server returns an empty body. The test at `client.test.ts` (`returns undefined for an empty success body`) enshrines the unsafe behaviour rather than flagging it. Suggested fix: make the empty case explicit in the type — e.g. `value?: T` or `PenpotResult<T | undefined>` — or document/require callers to pass `T | undefined`; either way add a compile-time-visible distinction so callers must narrow before dereferencing. ## Medium - [x] `extensions/penpot/src/errors.ts:367` — the Transit branch of `decodeErrorBody` returns only `{ category, message }` and never populates `DecodedErrorBody.code`, even though the JSON branch and the client docstring advertise that callers (the M2 commit path watching for `revn-conflict`) can react to the server `code`. Reproduced: `decodeErrorBody(400, "application/json", '["^ ","~:type","~:validation","~:code","~:revn-conflict","~:explain","bad revn"]')` yields `{category:"validation", message:"Transit-encoded error payload: bad revn …"}` with no `code`. Suggested fix: extract the Transit map's `code` (`decodeTransitMap(parsed.value)?.code`) into the returned `DecodedErrorBody`, and add a `client.test.ts` case asserting `result.error.code === "revn-conflict"` round-trips from the response body. - [x] `extensions/penpot/src/errors.ts:270` — `transitCategory` ignores the HTTP status, so a `400` carrying a Transit body that lacks a `~:type`/`~:explain` is categorised `server`, while the identical status via the JSON path is `validation` (`statusCategory(400)`). Reproduced: `decodeErrorBody(400, "application/json", '["^ ","~#uri","https://x/a"]')` → `category: "server"`. Suggested fix: pass `status` into `transitCategory` and fall back to `statusCategory(status)` when the decoded payload does not explicitly say `validation` (i.e. `return type === "validation" ? "validation" : statusCategory(status)`). ## Low - [x] `extensions/penpot/src/errors.ts:152` — `looksLikeTransit` uses `trimmed.includes('"~:')` / `'"~#'`, so ordinary JSON error bodies whose *string values* start with a Transit marker are misclassified as Transit and their message is rewritten. Reproduced: `decodeErrorBody(500, "application/json", '{"message":"~:foo"}')` returns `"Transit-encoded error payload: foo (raw body: {"message":"~:foo"})"` instead of the server's `message`. Suggested fix: restrict the substring heuristic to keys/values that are themselves tagged (e.g. only treat it as Transit when a `~:`/`~#` token occurs at a structural position, or require the parsed JSON to contain at least one `~`-prefixed key), and add a regression test for a JSON body containing `"~:`. - [x] `extensions/penpot/src/errors.ts:229` — `readEntry` treats any integer in a cache-coded map as a cache reference and only pushes non-integer entries into `cache`, so a literal numeric value decodes to `cache[n]` (`undefined`) and later cache indices can desynchronise from Transit's own numbering. Reproduced: `["^ ","~:explain","oops","~:status",400]` decodes `status` as `undefined`. Suggested fix: guard the lookup (`typeof entry === "number" && entry >= 0 && entry < cache.length ? cache[entry] : entry`) and, per the Transit cache-coding rules, only treat an integer as a reference when it is within the cache bounds; add a test with a numeric entry. - [x] `extensions/penpot/src/client.ts:204` — `method = options.method ?? "GET"` combined with the body handling at line 220 means `penpotRequest("update-file", { body: {...} })` (body but no explicit method) sends a GET with a body, which `fetch` rejects with `TypeError: Request with GET/HEAD method cannot have body`; that is then surfaced as `transport` ("could not reach Penpot at …"), misattributing a caller error to the network. Suggested fix: default to `POST` when `options.body !== undefined` (or fail fast with a `config`/caller error explaining that a body requires `method: "POST"`), and add a test. - [x] `extensions/penpot/src/errors.ts:94` — `NOT_FOUND_MESSAGE` tells the user to "ensure PENPOT_URL is the instance base URL with no /api suffix", but `env.ts:normalizeBaseUrl` already strips a trailing `/api` before `client.ts` ever builds a URL (the client test at `client.test.ts` asserts this double-append cannot happen). The advice therefore contradicts actual behaviour and can send a user chasing a non-issue. Suggested fix: drop the `/api`-suffix clause or reword it to state that the suffix is normalised automatically, and keep the guidance to the command name / file id causes. - [ ] `f73dc20` (commit message) — the commit is `188: Implement the RPC client …`, but AGENTS.md specifies `issue-<N>: <summary>` for `feature`/`bug`/`enhancement` work (and the branch already correctly follows `feature/issue-188/…`). Suggested fix: amend the commit message to `issue-188: Implement the RPC client and error decoding in src/client.ts and src/errors.ts with unit tests` before the PR is opened. ## Documentation Updated files: - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-14 23:37:53 +00:00