issue-202: Implement the Transit decoder and SSE stream parser in src/binfile.ts with unit tests #235

Merged

david merged 3 commits from feature/issue-202/implement-the-transit-decoder-and-sse-stream-parse into main

2026-09-15 04:11:50 +00:00

david commented

2026-09-15 04:11:49 +00:00

Owner

202: Implement the Transit decoder and SSE stream parser in src/binfile.ts with unit tests

Closes #202 — 202.

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 10m 12s.

Review findings

I've applied the fixes. Let me verify the final test count and the pre-existing failure is unchanged.# Review Findings — 202

Summary

The new extensions/penpot/src/binfile.ts (Transit decoder + SSE parser) and its 33 unit tests are implemented cleanly, are HTTP-free, and satisfy all 9 acceptance criteria in the issue body. There is no build or lint gate configured in this repo (package.json only exposes install:local and test), so the only gate runnable was npm test: the focused node --test extensions/penpot/src/binfile.test.ts is 33/33 green, and the full npm test is 536/537 — the single failure (scripts/local-install.test.mjs:1875, mongodb missing from pi.extensions) is pre-existing and reproduces unchanged on origin/main, so it is unrelated to this diff. Findings below are code-quality/robustness issues; no correctness or data-corruption bug was found in the required paths.

Remediation: Fixed the unbounded SseStreamError message (capped via truncateExcerpt while keeping rawText intact), the silent drop of a trailing compact-map key (now throws TransitParseError), added the missing comment documenting why decodeTransit and errors.ts's decodeTransitMap intentionally differ, made an empty data: field dispatch, and corrected the test count in findings.md (now 36 tests after adding three regression tests). Two findings remain unresolved: the live raw-stream capture (needs PENPOT_URL/PENPOT_TOKEN and network, unavailable here) and the commit-message convention drift (the commit already exists and the drift is repo-wide). Full npm test is 539/540 with the same pre-existing mongodb manifest failure.

Critical

(no findings)

High

extensions/penpot/src/binfile.ts:128 — SseStreamError embeds formatFrames(frames) into the error message with no size cap, so a failed stream that accumulated many/large frames floods the agent context (measured: 40 × 5 KB progress frames → a 201 KB error message). This contradicts the extension's own convention in errors.ts (MAX_RAW_EXCERPT_CHARS = 500 / truncateExcerpt, imported by this very module). Suggested fix: run the rendered frames through truncateExcerpt(rawText) (optionally with a dedicated larger cap, e.g. truncateExcerpt(rawText, 4000)) before super(...), while keeping the untruncated frames/rawText fields for callers.

Medium

extensions/penpot/src/binfile.ts:266 — decodeCompactMap iterates for (index = 1; index + 1 < array.length; index += 2) and silently drops a trailing key in an odd-length map (probe: ["^ ","~:a","1","~:b"] → {"a":"1"}). This is exactly the malformed-input case the module says it fails loudly on, and a dropped key is a silent mis-decode. Suggested fix: after the loop, if (array.length - 1) % 2 !== 0 throw a TransitParseError ("compact Transit map has an odd value count") carrying raw.
extensions/penpot/src/binfile.ts:165 — The "unverified live capture" gap is real and load-bearing: the decoder hard-fails on every ^N cache code, and the issue's Test Plan explicitly required capturing a real export-binfile stream to confirm whether caching is used (findings.md documents that this was not done). The same applies to the URI rep — the Transit spec/transit-js emit a standard URI as "~r<uri>", which decodeScalarString rejects (line 186); only the design-doc-asserted custom ~#uri form is accepted. Suggested fix: add the live raw-stream capture the issue asks for (the curl recipe is already in findings.md) and, if the real stream turns out to use ^N or ~r, implement the reader cache / accept ~r; at minimum commit a captured fixture so the fallback decision is tested against real bytes rather than the issue text.
extensions/penpot/src/binfile.ts:158 — Two divergent Transit decoders now exist in the extension: this module's decodeTransit (throws on ^N, handles ~#uri/verbose maps) and errors.ts:247 decodeTransitMap (resolves integer ^N cache refs, no ~#uri). decodeTransitError deliberately routes through the latter, so an error body and an end payload with identical Transit syntax can decode differently. Suggested fix: either factor the shared scalar/tag/key-stripping into one helper both call, or add a short comment at decodeTransit documenting why the two decoders intentionally differ (and which one an error body uses).

Low

extensions/penpot/src/binfile.ts:447 — dispatchPending returns undefined when the joined data === "", so data: (empty) or a bare data line produces no frame, contrary to the SSE spec (a data field — even empty — should dispatch). For Penpot this is benign, but an empty end payload is silently reported as "stream ended without an end frame" instead of a decode error. Suggested fix: drop the data === "" guard and dispatch whenever dataLines.length > 0.
extensions/penpot/findings.md:642 — States "with 31 unit tests in binfile.test.ts", but the file contains 33 test(...) cases. Suggested fix: correct the count to 33 (or drop the exact number).
AGENTS.md:26 — The commit under review is 202: Implement the Transit decoder …, whereas the documented format is issue-<N>: <summary>. (Every historical commit in this repo uses <N>:, so this is a repo-wide drift from the written standard rather than something unique to this change.) Suggested fix: use issue-202: … for the squash/PR title, or update AGENTS.md to match actual practice.

Documentation

Updated files:

docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 202: Implement the Transit decoder and SSE stream parser in src/binfile.ts with unit tests Closes #202 — [202](https://git.excelera.net/david/pi-extensions-and-skills/issues/202). > 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 10m 12s. ## Review findings I've applied the fixes. Let me verify the final test count and the pre-existing failure is unchanged.# Review Findings — 202 ## Summary The new `extensions/penpot/src/binfile.ts` (Transit decoder + SSE parser) and its 33 unit tests are implemented cleanly, are HTTP-free, and satisfy all 9 acceptance criteria in the issue body. There is no build or lint gate configured in this repo (`package.json` only exposes `install:local` and `test`), so the only gate runnable was `npm test`: the focused `node --test extensions/penpot/src/binfile.test.ts` is 33/33 green, and the full `npm test` is 536/537 — the single failure (`scripts/local-install.test.mjs:1875`, `mongodb` missing from `pi.extensions`) is pre-existing and reproduces unchanged on `origin/main`, so it is unrelated to this diff. Findings below are code-quality/robustness issues; no correctness or data-corruption bug was found in the required paths. **Remediation:** Fixed the unbounded `SseStreamError` message (capped via `truncateExcerpt` while keeping `rawText` intact), the silent drop of a trailing compact-map key (now throws `TransitParseError`), added the missing comment documenting why `decodeTransit` and `errors.ts`'s `decodeTransitMap` intentionally differ, made an empty `data:` field dispatch, and corrected the test count in `findings.md` (now 36 tests after adding three regression tests). Two findings remain unresolved: the live raw-stream capture (needs `PENPOT_URL`/`PENPOT_TOKEN` and network, unavailable here) and the commit-message convention drift (the commit already exists and the drift is repo-wide). Full `npm test` is 539/540 with the same pre-existing `mongodb` manifest failure. ## Critical (no findings) ## High - [x] `extensions/penpot/src/binfile.ts:128` — `SseStreamError` embeds `formatFrames(frames)` into the error message with no size cap, so a failed stream that accumulated many/large frames floods the agent context (measured: 40 × 5 KB progress frames → a 201 KB error message). This contradicts the extension's own convention in `errors.ts` (`MAX_RAW_EXCERPT_CHARS = 500` / `truncateExcerpt`, imported by this very module). Suggested fix: run the rendered frames through `truncateExcerpt(rawText)` (optionally with a dedicated larger cap, e.g. `truncateExcerpt(rawText, 4000)`) before `super(...)`, while keeping the untruncated `frames`/`rawText` fields for callers. ## Medium - [x] `extensions/penpot/src/binfile.ts:266` — `decodeCompactMap` iterates `for (index = 1; index + 1 < array.length; index += 2)` and silently drops a trailing key in an odd-length map (probe: `["^ ","~:a","1","~:b"]` → `{"a":"1"}`). This is exactly the malformed-input case the module says it fails loudly on, and a dropped key is a silent mis-decode. Suggested fix: after the loop, if `(array.length - 1) % 2 !== 0` throw a `TransitParseError` ("compact Transit map has an odd value count") carrying `raw`. - [ ] `extensions/penpot/src/binfile.ts:165` — The "unverified live capture" gap is real and load-bearing: the decoder hard-fails on every `^N` cache code, and the issue's Test Plan explicitly required capturing a real `export-binfile` stream to confirm whether caching is used (findings.md documents that this was not done). The same applies to the URI rep — the Transit spec/`transit-js` emit a standard URI as `"~r<uri>"`, which `decodeScalarString` rejects (line 186); only the design-doc-asserted custom `~#uri` form is accepted. Suggested fix: add the live raw-stream capture the issue asks for (the curl recipe is already in `findings.md`) and, if the real stream turns out to use `^N` or `~r`, implement the reader cache / accept `~r`; at minimum commit a captured fixture so the fallback decision is tested against real bytes rather than the issue text. - [x] `extensions/penpot/src/binfile.ts:158` — Two divergent Transit decoders now exist in the extension: this module's `decodeTransit` (throws on `^N`, handles `~#uri`/verbose maps) and `errors.ts:247 decodeTransitMap` (resolves integer `^N` cache refs, no `~#uri`). `decodeTransitError` deliberately routes through the latter, so an error body and an `end` payload with identical Transit syntax can decode differently. Suggested fix: either factor the shared scalar/tag/key-stripping into one helper both call, or add a short comment at `decodeTransit` documenting why the two decoders intentionally differ (and which one an error body uses). ## Low - [x] `extensions/penpot/src/binfile.ts:447` — `dispatchPending` returns `undefined` when the joined `data === ""`, so `data:` (empty) or a bare `data` line produces no frame, contrary to the SSE spec (a data field — even empty — should dispatch). For Penpot this is benign, but an empty `end` payload is silently reported as "stream ended without an end frame" instead of a decode error. Suggested fix: drop the `data === ""` guard and dispatch whenever `dataLines.length > 0`. - [x] `extensions/penpot/findings.md:642` — States "with 31 unit tests in `binfile.test.ts`", but the file contains 33 `test(...)` cases. Suggested fix: correct the count to 33 (or drop the exact number). - [ ] `AGENTS.md:26` — The commit under review is `202: Implement the Transit decoder …`, whereas the documented format is `issue-<N>: <summary>`. (Every historical commit in this repo uses `<N>:`, so this is a repo-wide drift from the written standard rather than something unique to this change.) Suggested fix: use `issue-202: …` for the squash/PR title, or update AGENTS.md to match actual practice. ## Documentation Updated files: - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-15 04:11:49 +00:00