issue-194: Implement the staged changeset store and the colour/typography primitives with unit tests #227

Merged

david merged 3 commits from feature/issue-194/implement-the-staged-changeset-store-and-the-colou into main

2026-09-15 00:39:05 +00:00

david commented

2026-09-15 00:39:05 +00:00

Owner

194: Implement the staged changeset store and the colour/typography primitives with unit tests

Closes #194 — 194.

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 13m 58s.

Review findings

Now let me apply the fixes. Starting with changeset.ts:Now tokens.ts — constrain text_transform, enforce non-negative revn, and reorder the typography fetch:Now index.ts and its test:Now the CHANGELOG entry:Now add the regression tests. First changeset.test.ts:Now tokens.test.ts — opacity-zero boundary tests and the phantom-changeset regression:Now the live harness. Let me check the Node version and add the get-font-variants read:Now let me run the penpot tests:# Review Findings — 194

Summary

Resolved the High changelog finding and every Medium/Low finding except the designated-target semantics decision (extensions/penpot/src/tools/tokens.ts:278), which belongs with the penpot_commit guard and needs a scope call on what "a file explicitly named by the user" means. Fixes applied: added the CHANGELOG.md ### Added bullet; made resolveFont treat the built-in Source Sans Pro as always installed (no false substitution); fetched font variants before creating the changeset so a failed penpot_create_typography leaves no phantom entry; registered a session_shutdown reset (with fakePi.on and a wiring test); made discardChangeset treat a zero-change changeset as nothing-to-discard; corrected the module header; constrained text_transform to Penpot's enum; bounded the invalid-colour excerpt via the shared truncateExcerpt; enforced non-negative revn; added the opacity-0 ??-chain tests and the phantom-changeset regression; and added get-font-variants to the live harness and the findings command table. Gates: npm test → 304 pass / 1 fail, the same pre-existing scripts/local-install.test.mjs:1875 mongodb-manifest assertion (package.json untouched by this diff). No build/lint/type-check command exists in this repo, so only npm test was runnable.

Critical

(none)

High

CHANGELOG.md:9 — no entry was added for the two new staging tools + penpot_discard under ## [Unreleased] → ### Added. Every prior penpot issue in this repo added a changelog bullet in the same commit (git log -- CHANGELOG.md: 189, 190, 191, 192, 193), and this is a user-facing feature; the docs-sync stage only covers README.md and docs/**/*.md, so it will not repair this. Suggested fix: add an ### Added bullet describing the staged penpot_create_color / penpot_create_typography / penpot_discard tools, the per-file changeset, hex validation and the font-substitution behaviour.

Medium

extensions/penpot/src/changeset.ts:409 — requesting the built-in family itself is reported as a substitution. get-font-variants never returns the built-in Source Sans Pro, so matches is empty for family: "Source Sans Pro"/"sourcesanspro" and the code takes the fallback branch, emitting a false claim. Repro (verified): resolveFont({family:"Source Sans Pro",weight:600}, []) → substitution: {requestedFamily:"Source Sans Pro", usedFamily:"Source Sans Pro", reason:"not installed on this instance"}, and the tool text says Substitution: Source Sans Pro requested → Source Sans Pro used (not installed on this instance). Suggested fix: treat the built-in family aliases as always installed (match request.family against SOURCE_SANS_PRO_VARIANTS/DEFAULT_FONT_DISPLAY_NAME before the fallback branch, or omit substitution when familyKey(request.family) equals the resolved family key).
extensions/penpot/src/tools/tokens.ts:406 — a failed penpot_create_typography leaves a phantom changeset behind. ensureChangeset() (line 405) creates and authorises the changeset before the fallible fetchInstalledFonts() (line 406); if that fetch fails the store keeps an entry for the file with changes: []. Because activeChangesetFileId() only returns files with changes.length > 0, a different file can then be staged, and the original file is afterwards refused with "file [A] cannot be staged: file [B] already has staged changes" even though nothing was ever staged for A. Verified by repro: stage typography for A with a failing get-font-variants, then stage a colour for B (allowed), then stage A again (refused). Suggested fix: fetch the font variants (and validate everything fallible) before ensureChangeset/authorizeTarget, or make the guard consult getChangeset(fileId) !== undefined instead of only non-empty changesets and clean up a created-but-empty changeset when the call throws.
extensions/penpot/src/changeset.ts:455 — the module-level store and authorizedTargets set have no session lifecycle hook, so staged changes and write authorisations survive into a new session in the same pi process (extension factories are cached per process by pi's loader, which only re-runs the index.ts factory — module state is not reset). This contradicts the module's own "deliberate session state" framing and silently widens the future designated-target guard. Suggested fix: in extensions/penpot/src/index.ts register pi.on("session_shutdown", () => resetChangesets()) (and extend fakePi in index.test.ts with on), or expose a resetChangesets() call from the entry point.
extensions/penpot/src/tools/tokens.ts:278 — authorizeTarget(fileId) is called from ensureChangeset for any file id the model passes, so "authorised by construction" means "the model named a file it can read". The design (docs/PENPOT_SKILL_DESIGN.md §9 / plan M2) requires writes only to files the extension created in a nominated project or a file explicitly named by the user; a guard built on this set cannot distinguish those, so it will authorise an arbitrary existing file. Suggested fix: decide the designated-target semantics before the penpot_commit guard lands — e.g. record only files created via create-file plus targets the user named, and require an explicit user-supplied confirmation flag rather than auto-authorising on staging.

Low

extensions/penpot/src/tools/tokens.ts:513 — an empty changeset is reported as a discard: details.discarded is true and details.status.changeCount is 0, while the text says No staged changes for file [..] — nothing to discard. (verified by repro; reachable via the phantom-changeset path above). Suggested fix: treat a zero-change changeset as discarded: false (or drop the empty changeset and report nothing to discard consistently) so details.discarded and the text agree.
extensions/penpot/src/changeset.ts:15 — the module header says one changeset per file means "two files can be staged independently and a whole design system can be previewed before anything is written", but assertSingleActiveTarget refuses a second file and extensions/penpot/README.md documents the refusal. Suggested fix: correct the header to state the store is per-file while the tools enforce a single active target (or relax the guard if multi-file staging was intended).
extensions/penpot/src/tools/tokens.ts:158 — text_transform is an unconstrained Type.String() while the sibling fields are validated (font_style is a union, opacity has min/max, the hex is validated). The server expects none | uppercase | lowercase | capitalize; an arbitrary string is staged verbatim. Suggested fix: Type.Optional(Type.Union([Type.Literal("none"), Type.Literal("uppercase"), Type.Literal("lowercase"), Type.Literal("capitalize")])), matching DEFAULT_TEXT_TRANSFORM.
extensions/penpot/src/changeset.ts:180 — the invalid-colour error interpolates the unbounded raw input (invalid colour "${raw}" …), unlike the client/errors modules which cap excerpts (MAX_RAW_EXCERPT_CHARS in errors.ts). A model-supplied long string (e.g. a pasted data URI) floods the context. Suggested fix: bound it, e.g. invalid colour "${truncateExcerpt(raw, 40)}".
extensions/penpot/src/tools/tokens.ts:226 — readRevn's doc says "A finite, non-negative number" but the implementation accepts any finite number, including negatives. Suggested fix: enforce revn >= 0 in the predicate or amend the comment.
extensions/penpot/src/tools/tokens.ts:372 — no test covers the ??-chain boundary where a falsy-but-valid opacity must win: #RRGGBB00 (alpha 0) and an explicit opacity: 0. The chain is correct today, but it is exactly the kind of ?? vs || case a later edit breaks. Suggested fix: add a tool-level test staging color: "#2BA8A200" and one with opacity: 0 asserting change.color.opacity === 0.
extensions/penpot/scripts/live-read-check.mjs:63 — the new get-font-variants read isn't in the committed live harness or the extensions/penpot/findings.md command table, although the repo's convention (issue 193) is a live smoke check per command; the non-empty variant row shape is only unit-stubbed. I verified field names (fontId/fontFamily/fontWeight/fontStyle, camelCased by sm/json-transformer) and the custom-<uuid> / <style>-<weight> derivation against Penpot 2.17.2 source, so the risk is low. Suggested fix: add the command to live-read-check.mjs (and the findings table) once a token is available.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md

## 194: Implement the staged changeset store and the colour/typography primitives with unit tests Closes #194 — [194](https://git.excelera.net/david/pi-extensions-and-skills/issues/194). > 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 13m 58s. ## Review findings Now let me apply the fixes. Starting with `changeset.ts`:Now `tokens.ts` — constrain `text_transform`, enforce non-negative `revn`, and reorder the typography fetch:Now `index.ts` and its test:Now the CHANGELOG entry:Now add the regression tests. First `changeset.test.ts`:Now `tokens.test.ts` — opacity-zero boundary tests and the phantom-changeset regression:Now the live harness. Let me check the Node version and add the `get-font-variants` read:Now let me run the penpot tests:# Review Findings — 194 ## Summary Resolved the High changelog finding and every Medium/Low finding except the designated-target semantics decision (`extensions/penpot/src/tools/tokens.ts:278`), which belongs with the `penpot_commit` guard and needs a scope call on what "a file explicitly named by the user" means. Fixes applied: added the `CHANGELOG.md` `### Added` bullet; made `resolveFont` treat the built-in Source Sans Pro as always installed (no false substitution); fetched font variants **before** creating the changeset so a failed `penpot_create_typography` leaves no phantom entry; registered a `session_shutdown` reset (with `fakePi.on` and a wiring test); made `discardChangeset` treat a zero-change changeset as nothing-to-discard; corrected the module header; constrained `text_transform` to Penpot's enum; bounded the invalid-colour excerpt via the shared `truncateExcerpt`; enforced non-negative `revn`; added the opacity-0 `??`-chain tests and the phantom-changeset regression; and added `get-font-variants` to the live harness and the findings command table. Gates: `npm test` → **304 pass / 1 fail**, the same pre-existing `scripts/local-install.test.mjs:1875` mongodb-manifest assertion (package.json untouched by this diff). No build/lint/type-check command exists in this repo, so only `npm test` was runnable. ## Critical - [ ] (none) ## High - [x] `CHANGELOG.md:9` — no entry was added for the two new staging tools + `penpot_discard` under `## [Unreleased]` → `### Added`. Every prior penpot issue in this repo added a changelog bullet in the same commit (`git log -- CHANGELOG.md`: 189, 190, 191, 192, 193), and this is a user-facing feature; the docs-sync stage only covers `README.md` and `docs/**/*.md`, so it will not repair this. Suggested fix: add an `### Added` bullet describing the staged `penpot_create_color` / `penpot_create_typography` / `penpot_discard` tools, the per-file changeset, hex validation and the font-substitution behaviour. ## Medium - [x] `extensions/penpot/src/changeset.ts:409` — requesting the built-in family itself is reported as a substitution. `get-font-variants` never returns the built-in Source Sans Pro, so `matches` is empty for `family: "Source Sans Pro"`/`"sourcesanspro"` and the code takes the fallback branch, emitting a false claim. Repro (verified): `resolveFont({family:"Source Sans Pro",weight:600}, [])` → `substitution: {requestedFamily:"Source Sans Pro", usedFamily:"Source Sans Pro", reason:"not installed on this instance"}`, and the tool text says `Substitution: Source Sans Pro requested → Source Sans Pro used (not installed on this instance)`. Suggested fix: treat the built-in family aliases as always installed (match `request.family` against `SOURCE_SANS_PRO_VARIANTS`/`DEFAULT_FONT_DISPLAY_NAME` before the fallback branch, or omit `substitution` when `familyKey(request.family)` equals the resolved family key). - [x] `extensions/penpot/src/tools/tokens.ts:406` — a failed `penpot_create_typography` leaves a phantom changeset behind. `ensureChangeset()` (line 405) creates *and authorises* the changeset before the fallible `fetchInstalledFonts()` (line 406); if that fetch fails the store keeps an entry for the file with `changes: []`. Because `activeChangesetFileId()` only returns files with `changes.length > 0`, a *different* file can then be staged, and the original file is afterwards refused with "file [A] cannot be staged: file [B] already has staged changes" even though nothing was ever staged for A. Verified by repro: stage typography for A with a failing `get-font-variants`, then stage a colour for B (allowed), then stage A again (refused). Suggested fix: fetch the font variants (and validate everything fallible) before `ensureChangeset`/`authorizeTarget`, or make the guard consult `getChangeset(fileId) !== undefined` instead of only non-empty changesets and clean up a created-but-empty changeset when the call throws. - [x] `extensions/penpot/src/changeset.ts:455` — the module-level store and `authorizedTargets` set have no session lifecycle hook, so staged changes and write authorisations survive into a new session in the same pi process (extension factories are cached per process by pi's loader, which only re-runs the `index.ts` factory — module state is not reset). This contradicts the module's own "deliberate session state" framing and silently widens the future designated-target guard. Suggested fix: in `extensions/penpot/src/index.ts` register `pi.on("session_shutdown", () => resetChangesets())` (and extend `fakePi` in `index.test.ts` with `on`), or expose a `resetChangesets()` call from the entry point. - [ ] `extensions/penpot/src/tools/tokens.ts:278` — `authorizeTarget(fileId)` is called from `ensureChangeset` for *any* file id the model passes, so "authorised by construction" means "the model named a file it can read". The design (`docs/PENPOT_SKILL_DESIGN.md` §9 / plan M2) requires writes only to files the extension created in a nominated project **or a file explicitly named by the user**; a guard built on this set cannot distinguish those, so it will authorise an arbitrary existing file. Suggested fix: decide the designated-target semantics before the `penpot_commit` guard lands — e.g. record only files created via `create-file` plus targets the user named, and require an explicit user-supplied confirmation flag rather than auto-authorising on staging. ## Low - [x] `extensions/penpot/src/tools/tokens.ts:513` — an empty changeset is reported as a discard: `details.discarded` is `true` and `details.status.changeCount` is `0`, while the text says `No staged changes for file [..] — nothing to discard.` (verified by repro; reachable via the phantom-changeset path above). Suggested fix: treat a zero-change changeset as `discarded: false` (or drop the empty changeset and report `nothing to discard` consistently) so `details.discarded` and the text agree. - [x] `extensions/penpot/src/changeset.ts:15` — the module header says one changeset per file means "two files can be staged independently and a whole design system can be previewed before anything is written", but `assertSingleActiveTarget` refuses a second file and `extensions/penpot/README.md` documents the refusal. Suggested fix: correct the header to state the store is per-file while the tools enforce a single active target (or relax the guard if multi-file staging was intended). - [x] `extensions/penpot/src/tools/tokens.ts:158` — `text_transform` is an unconstrained `Type.String()` while the sibling fields are validated (`font_style` is a union, opacity has min/max, the hex is validated). The server expects `none | uppercase | lowercase | capitalize`; an arbitrary string is staged verbatim. Suggested fix: `Type.Optional(Type.Union([Type.Literal("none"), Type.Literal("uppercase"), Type.Literal("lowercase"), Type.Literal("capitalize")]))`, matching `DEFAULT_TEXT_TRANSFORM`. - [x] `extensions/penpot/src/changeset.ts:180` — the invalid-colour error interpolates the unbounded raw input (`invalid colour "${raw}" …`), unlike the client/errors modules which cap excerpts (`MAX_RAW_EXCERPT_CHARS` in `errors.ts`). A model-supplied long string (e.g. a pasted data URI) floods the context. Suggested fix: bound it, e.g. `` invalid colour "${truncateExcerpt(raw, 40)}" ``. - [x] `extensions/penpot/src/tools/tokens.ts:226` — `readRevn`'s doc says "A finite, non-negative number" but the implementation accepts any finite number, including negatives. Suggested fix: enforce `revn >= 0` in the predicate or amend the comment. - [x] `extensions/penpot/src/tools/tokens.ts:372` — no test covers the `??`-chain boundary where a falsy-but-valid opacity must win: `#RRGGBB00` (alpha 0) and an explicit `opacity: 0`. The chain is correct today, but it is exactly the kind of `??` vs `||` case a later edit breaks. Suggested fix: add a tool-level test staging `color: "#2BA8A200"` and one with `opacity: 0` asserting `change.color.opacity === 0`. - [x] `extensions/penpot/scripts/live-read-check.mjs:63` — the new `get-font-variants` read isn't in the committed live harness or the `extensions/penpot/findings.md` command table, although the repo's convention (issue 193) is a live smoke check per command; the non-empty variant row shape is only unit-stubbed. I verified field names (`fontId`/`fontFamily`/`fontWeight`/`fontStyle`, camelCased by `sm/json-transformer`) and the `custom-<uuid>` / `<style>-<weight>` derivation against Penpot 2.17.2 source, so the risk is low. Suggested fix: add the command to `live-read-check.mjs` (and the findings table) once a token is available. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md

david added 3 commits

2026-09-15 00:39:05 +00:00