feat(penpot-mcp): canvas primitives — list_*, create_page, create_frame, create_text #71

Merged
code-lead merged 5 commits from feat/penpot-canvas-tools into main 2026-04-19 00:12:14 +00:00
Collaborator

Refs #69 — partial close. Ships most canvas primitives; the two
remaining AC bullets (create_file, export_frame_png) are deferred
to their own tickets (#73, #74) so the umbrella issue isn't
auto-closed on merge with unchecked boxes.

Summary

Fork was token-CRUD only; every designer dispatch on area:design
issues hit the "no shape primitives" abort. This lands the minimum
set that makes the design-implement.md skill's happy path callable
end-to-end. Live-verified on the existing file from #55 with a
throwaway canvas-probe page — Penpot rendered the frame + text
exactly as #62's AC describes.

New tools

Read / navigate (thin RPC wrappers, one round-trip each):

  • list_teams()
  • list_projects(team_id)
  • list_files(project_id)

Canvas primitives (single update-file change-op each):

  • create_page(file_id, name)add-page
  • create_frame(file_id, page_id, name, x, y, w, h, fill_color?, parent_id?)add-obj (type=frame)
  • create_text(file_id, page_id, parent_id, content, x, y, w, h, font_family?, font_size?, font_weight?, fill_color?, text_align?, grows_type?)add-obj (type=text)

Latent bug also fixed

apply_changes wasn't sending vern or using camelCase sessionId.
The token write tools from 0.3 would have silently broken against
live Penpot on any server build that required vern; tests mocked
above the HTTP layer so this never surfaced. Both keys fixed in
api.py — transit-json decode was already landed in 0.4 via
Accept: application/json.

Deferred to follow-ups (tracked)

  • create_file#73. Penpot's create-file RPC has a
    richer param surface (migrations list, feature flags) that needs
    a dedicated pass. Operators seed files via the Penpot UI until
    that lands.
  • export_frame_png#74. Recent Penpot builds render PNGs
    client-side via wasm; no stable server-side export RPC on our
    instance. #74 captures the probe results and three viable
    implementation paths.

Test plan

  • penpot-mcp-server/tests/test_canvas.py — 15 unit tests, all pass.
    Each tool invocation is patched at api.apply_changes /
    api.command and the emitted change-op is asserted for shape,
    keys, and values. Module-level tool functions so tests import
    them directly (avoiding the FastMCP _tools private-attr trap
    that broke test_tokens.py).
  • penpot-mcp-server/tests/test_canvas_live.py — 1 live test
    (pytest -m live), gated behind PENPOT_ACCESS_TOKEN. Creates
    a temp page + frame + text on the claude-hooks — dashboard
    file, asserts shape count, cleans up in finally.
  • scripts/smoke-creds.sh — probe asserts the six canvas
    module-level functions exist in penpot_mcp.tools.canvas
    (regression gate for the empty-fork state that caused #62
    run #1).
  • just qa on the TS side — 244 tests, fmt/lint clean.
  • Live end-to-end on 689d7fa4-f94b-81d4-8007-e39c5c82f66c
    (the claude-hooks — dashboard file from #55): created page,
    frame with Tokyo Night bg, Fira Code 40/700 HELLO text,
    verified shape count.
  • After merge + image rebuild: re-dispatch #62 and/or #70 → designer
    should produce the Penpot artifacts on first try.

Follow-ups

  • #73create_file (carved out).
  • #74export_frame_png (carved out).
  • set_text_content — multi-paragraph / styled text (current tool
    is single-paragraph plain string). Not filed yet; wait for a real
    caller.
Refs #69 — partial close. Ships most canvas primitives; the two remaining AC bullets (`create_file`, `export_frame_png`) are deferred to their own tickets (#73, #74) so the umbrella issue isn't auto-closed on merge with unchecked boxes. ## Summary Fork was token-CRUD only; every designer dispatch on `area:design` issues hit the "no shape primitives" abort. This lands the minimum set that makes the `design-implement.md` skill's happy path callable end-to-end. Live-verified on the existing file from #55 with a throwaway `canvas-probe` page — Penpot rendered the frame + text exactly as #62's AC describes. ### New tools **Read / navigate** (thin RPC wrappers, one round-trip each): - `list_teams()` - `list_projects(team_id)` - `list_files(project_id)` **Canvas primitives** (single `update-file` change-op each): - `create_page(file_id, name)` → `add-page` - `create_frame(file_id, page_id, name, x, y, w, h, fill_color?, parent_id?)` → `add-obj` (type=frame) - `create_text(file_id, page_id, parent_id, content, x, y, w, h, font_family?, font_size?, font_weight?, fill_color?, text_align?, grows_type?)` → `add-obj` (type=text) ### Latent bug also fixed `apply_changes` wasn't sending `vern` or using camelCase `sessionId`. The token write tools from 0.3 would have silently broken against live Penpot on any server build that required `vern`; tests mocked above the HTTP layer so this never surfaced. Both keys fixed in `api.py` — transit-json decode was already landed in 0.4 via `Accept: application/json`. ### Deferred to follow-ups (tracked) - **`create_file`** → **#73**. Penpot's `create-file` RPC has a richer param surface (migrations list, feature flags) that needs a dedicated pass. Operators seed files via the Penpot UI until that lands. - **`export_frame_png`** → **#74**. Recent Penpot builds render PNGs client-side via wasm; no stable server-side export RPC on our instance. #74 captures the probe results and three viable implementation paths. ## Test plan - [x] `penpot-mcp-server/tests/test_canvas.py` — 15 unit tests, all pass. Each tool invocation is patched at `api.apply_changes` / `api.command` and the emitted change-op is asserted for shape, keys, and values. Module-level tool functions so tests import them directly (avoiding the FastMCP `_tools` private-attr trap that broke `test_tokens.py`). - [x] `penpot-mcp-server/tests/test_canvas_live.py` — 1 live test (`pytest -m live`), gated behind `PENPOT_ACCESS_TOKEN`. Creates a temp page + frame + text on the `claude-hooks — dashboard` file, asserts shape count, cleans up in `finally`. - [x] `scripts/smoke-creds.sh` — probe asserts the six canvas module-level functions exist in `penpot_mcp.tools.canvas` (regression gate for the empty-fork state that caused #62 run #1). - [x] `just qa` on the TS side — 244 tests, fmt/lint clean. - [x] Live end-to-end on `689d7fa4-f94b-81d4-8007-e39c5c82f66c` (the `claude-hooks — dashboard` file from #55): created page, frame with Tokyo Night bg, Fira Code 40/700 `HELLO` text, verified shape count. - [ ] After merge + image rebuild: re-dispatch #62 and/or #70 → designer should produce the Penpot artifacts on first try. ## Follow-ups - **#73** — `create_file` (carved out). - **#74** — `export_frame_png` (carved out). - `set_text_content` — multi-paragraph / styled text (current tool is single-paragraph plain string). Not filed yet; wait for a real caller.
feat(penpot-mcp): canvas primitives — list_*, create_page, create_frame, create_text
All checks were successful
qa / qa (pull_request) Successful in 2m32s
qa / dockerfile (pull_request) Successful in 10s
fc16ae1329
Closes the last operational gap on the designer pipeline (issue #69).
The fork's 0.4.x tool surface was token-CRUD only, so every designer
dispatch hit the "no shape primitives" diagnostic and aborted without
producing Penpot artifacts. This lands the minimum set that makes the
`design-implement.md` skill's happy path callable end-to-end:

Read / navigate (thin RPC wrappers, no DB pool):
- list_teams()
- list_projects(team_id)
- list_files(project_id)

Canvas primitives (single `update-file` change-op each):
- create_page(file_id, name) — emits add-page
- create_frame(file_id, page_id, name, x, y, w, h, fill_color?, parent_id?)
  — emits add-obj with type=frame, axis-aligned envelope (selrect +
  points + identity transform computed from x/y/w/h)
- create_text(file_id, page_id, parent_id, content, x, y, w, h, ...)
  — emits add-obj with type=text and Penpot's nested
  `root → paragraph-set → paragraph → text-node` content tree.
  Single-paragraph plain text only; styled / multi-paragraph content
  is deferred.

Fix — apply_changes sends `vern` + camelCase sessionId:

  The token write tools landed in 0.3 silently broke against live
  Penpot on any server build that required a vern field on
  update-file. Unit tests mocked above the HTTP layer so the gap
  never surfaced. Fixed the client here — transit-json decode was
  already fixed on 0.4's access-token refactor (`Accept:
  application/json`), the remaining key-case mismatch
  (`session-id` vs `sessionId`) is fixed in the same spot.

Tests:

  12 unit tests in tests/test_canvas.py cover:
  - read tools pass correct param keys (teamId, projectId — camelCase)
  - create_page emits a single add-page change with a fresh UUID each
    call, returned id matches the one sent
  - create_frame emits add-obj at page root by default, nests under
    parent_id when passed, omits fill when fill_color is None
  - create_frame geometry envelope is consistent (selrect bounds,
    4-corner points, identity transform) — Penpot rejects mismatches
  - create_text wraps content in the root/para/leaf tree and sets
    obj.growsType = "auto-width"
  - create_text defaults match upstream conventions
    (sourcesanspro / 14 / 400 / left / #000000)
  - obj.name is sliced to 40 chars so the layers panel stays readable

  Tool functions are module-level (not buried in a register() closure
  like tokens.py) so tests import them directly without FastMCP's
  private _tools attr lookup — keeps the tests future-proof against
  the next mcp library rename.

Live-verified end-to-end against the existing file from #55
(689d7fa4-f94b-81d4-8007-e39c5c82f66c): a `canvas-probe` page with
an 800×600 frame filled `#1A1B26` and a centred `HELLO` text at
Fira Code 40/700 `#C0CAF5` — exactly the AC on #62.

Not shipped here (tracked separately):
- create_file — richer param surface than the other writes
- export_frame_png — Penpot renders PNGs client-side via wasm on
  recent builds; no stable server-side export RPC to wrap
reviewer requested changes 2026-04-18 23:21:20 +00:00
Dismissed
reviewer left a comment

Review — feat(penpot-mcp): canvas primitives

CI green (run #1585, 2m44s).

Overall this is clean: the apply_changes bug fix is correct and well-documented, the module-level tool pattern avoids the _tools trap, the geometry scaffold is consistent, and the 12 unit tests give good coverage of the happy paths. Two items need addressing before merge.


1. create_textgrowsType: "auto-width" silently ignores the width parameter

File: penpot-mcp-server/src/penpot_mcp/tools/canvas.py, line 164

obj["growsType"] = "auto-width"

The function accepts explicit width and height arguments and passes them into _axis_aligned_shape, which stores them in the shape's geometry envelope (including selrect, points). But "auto-width" mode in Penpot tells the renderer to horizontally resize the text shape to fit its content, overriding width at render time. A caller expecting text to stay within a 200px column (e.g., for text wrapping inside a frame) will instead get a shape that expands past its container.

Fix option A — prefer "auto-height": fixed width, auto height. Penpot respects width for line wrapping and grows the box downward. This makes the width parameter meaningful.

obj["growsType"] = "auto-height"

Fix option B — keep "auto-width" but drop the width/height parameters from the signature, replacing them with just x/y, so callers don't pass dimensions that are silently ignored.

Fix option C — document (weakest, but acceptable if A/B would break the live E2E): add a note in the docstring that width and height are initial placement hints only, and that Penpot will override them based on content size at render time.

The live E2E ("verified shape count") wouldn't catch this because it checks whether the shape exists, not whether its bounding box matches the requested dimensions.


2. Missing pytest.mark.live test

File: penpot-mcp-server/tests/ — no pytest.mark.live test present

The issue AC states:

Live: add a throwaway pytest.mark.live test against the existing claude-hooks — dashboard file (689d7fa4-f94b-81d4-8007-e39c5c82f66c) that creates a new temp page, adds a frame + text, exports a PNG, and deletes the page. Gated behind PENPOT_ACCESS_TOKEN env — no-op when unset.

The PR description's test plan checks off a manual E2E run on that file, but the codified live test is neither present nor explicitly deferred (unlike create_file and export_frame_png, which have clear explanations for deferral). Without it, a future regression in apply_changes or the Penpot RPC schema would only surface on next designer dispatch rather than on the next pytest -m live run.

Add a tests/test_canvas_live.py with at minimum:

@pytest.mark.live
@pytest.mark.asyncio
@pytest.mark.skipif(not os.getenv("PENPOT_ACCESS_TOKEN"), reason="no live creds")
async def test_create_page_frame_text_roundtrip() -> None:
    ...  # create temp page, create frame, create text, assert shape count, delete page

AC coverage summary

Criterion Status
list_teams / list_projects / list_files
create_page
create_frame
create_text (see issue #1)
create_file deferred — reason given, acceptable
export_frame_png deferred — reason given, acceptable
Unit tests (12 tests)
Live pytest.mark.live test missing (issue #2)
README canvas section
CHANGELOG 0.5.0
apply_changes vern + camelCase fix
smoke-creds.sh shape-tool assertion deferred — mentioned as follow-up in PR body, acceptable
## Review — feat(penpot-mcp): canvas primitives CI ✅ green (run #1585, 2m44s). Overall this is clean: the `apply_changes` bug fix is correct and well-documented, the module-level tool pattern avoids the `_tools` trap, the geometry scaffold is consistent, and the 12 unit tests give good coverage of the happy paths. Two items need addressing before merge. --- ### 1. `create_text` — `growsType: "auto-width"` silently ignores the `width` parameter **File:** `penpot-mcp-server/src/penpot_mcp/tools/canvas.py`, line 164 ```python obj["growsType"] = "auto-width" ``` The function accepts explicit `width` and `height` arguments and passes them into `_axis_aligned_shape`, which stores them in the shape's geometry envelope (including `selrect`, `points`). But `"auto-width"` mode in Penpot tells the renderer to horizontally resize the text shape to fit its content, overriding `width` at render time. A caller expecting text to stay within a 200px column (e.g., for text wrapping inside a frame) will instead get a shape that expands past its container. **Fix option A — prefer `"auto-height"`:** fixed width, auto height. Penpot respects `width` for line wrapping and grows the box downward. This makes the `width` parameter meaningful. ```python obj["growsType"] = "auto-height" ``` **Fix option B — keep `"auto-width"` but drop the `width`/`height` parameters from the signature**, replacing them with just `x`/`y`, so callers don't pass dimensions that are silently ignored. **Fix option C — document** (weakest, but acceptable if A/B would break the live E2E): add a note in the docstring that `width` and `height` are initial placement hints only, and that Penpot will override them based on content size at render time. The live E2E ("verified shape count") wouldn't catch this because it checks whether the shape exists, not whether its bounding box matches the requested dimensions. --- ### 2. Missing `pytest.mark.live` test **File:** `penpot-mcp-server/tests/` — no `pytest.mark.live` test present The issue AC states: > Live: add a throwaway `pytest.mark.live` test against the existing `claude-hooks — dashboard` file (`689d7fa4-f94b-81d4-8007-e39c5c82f66c`) that creates a new temp page, adds a frame + text, exports a PNG, and deletes the page. Gated behind `PENPOT_ACCESS_TOKEN` env — no-op when unset. The PR description's test plan checks off a manual E2E run on that file, but the codified live test is neither present nor explicitly deferred (unlike `create_file` and `export_frame_png`, which have clear explanations for deferral). Without it, a future regression in `apply_changes` or the Penpot RPC schema would only surface on next designer dispatch rather than on the next `pytest -m live` run. Add a `tests/test_canvas_live.py` with at minimum: ```python @pytest.mark.live @pytest.mark.asyncio @pytest.mark.skipif(not os.getenv("PENPOT_ACCESS_TOKEN"), reason="no live creds") async def test_create_page_frame_text_roundtrip() -> None: ... # create temp page, create frame, create text, assert shape count, delete page ``` --- ### AC coverage summary | Criterion | Status | |---|---| | `list_teams` / `list_projects` / `list_files` | ✅ | | `create_page` | ✅ | | `create_frame` | ✅ | | `create_text` | ✅ (see issue #1) | | `create_file` | deferred — reason given, acceptable | | `export_frame_png` | deferred — reason given, acceptable | | Unit tests | ✅ (12 tests) | | Live `pytest.mark.live` test | ❌ missing (issue #2) | | README canvas section | ✅ | | CHANGELOG 0.5.0 | ✅ | | `apply_changes` vern + camelCase fix | ✅ | | `smoke-creds.sh` shape-tool assertion | deferred — mentioned as follow-up in PR body, acceptable |
@ -0,0 +161,4 @@
x=x,
y=y,
width=width,
height=height,
Collaborator

auto-width causes Penpot to ignore the width parameter at render time — the text box expands horizontally to fit its content. This makes the width argument misleading (caller passes 200px expecting text to wrap, gets a box that grows past the frame boundary instead). Consider switching to "auto-height" (fixed width, auto-grows downward), or at minimum document this behaviour in the docstring so callers know what to expect.

`auto-width` causes Penpot to ignore the `width` parameter at render time — the text box expands horizontally to fit its content. This makes the `width` argument misleading (caller passes 200px expecting text to wrap, gets a box that grows past the frame boundary instead). Consider switching to `"auto-height"` (fixed width, auto-grows downward), or at minimum document this behaviour in the docstring so callers know what to expect.
@ -0,0 +240,4 @@
FILE_ID, PAGE_ID, ROOT_FRAME_ID,
content=long_text, x=0, y=0, width=100, height=20,
)
assert calls[0][1][0]["obj"]["name"] == "x" * 40 # sliced to 40 chars
Collaborator

The AC requires a pytest.mark.live test gated behind PENPOT_ACCESS_TOKEN (create temp page → frame → text → assert shape count → delete page on file 689d7fa4-f94b-81d4-8007-e39c5c82f66c). This test file covers unit mocks only. Please add a tests/test_canvas_live.py with @pytest.mark.skipif(not os.getenv("PENPOT_ACCESS_TOKEN"), ...) so future regressions in the RPC schema are caught before dispatch.

The AC requires a `pytest.mark.live` test gated behind `PENPOT_ACCESS_TOKEN` (create temp page → frame → text → assert shape count → delete page on file `689d7fa4-f94b-81d4-8007-e39c5c82f66c`). This test file covers unit mocks only. Please add a `tests/test_canvas_live.py` with `@pytest.mark.skipif(not os.getenv("PENPOT_ACCESS_TOKEN"), ...)` so future regressions in the RPC schema are caught before dispatch.
charles referenced this pull request from a commit 2026-04-18 23:25:16 +00:00
review: apply reviewer fixes on #71
All checks were successful
qa / qa (pull_request) Successful in 2m30s
qa / dockerfile (pull_request) Successful in 10s
e0192cad41
Two items from the `reviewer` pass on PR #71:

1. `create_text` — `growsType: "auto-width"` silently ignored the
   `width` parameter. Penpot's auto-width resizes the box horizontally
   to fit content at render time, so a caller passing `width=200` got
   a shape that could outgrow its container. API dishonest: the
   signature promised `width` mattered.

   Switch the default `grows_type` to `"auto-height"` — keeps `width`
   fixed, grows the box vertically for wrapped lines. Right choice
   for the text-in-a-frame case. Add a `grows_type` kwarg with
   `"auto-height"` / `"auto-width"` / `"fixed"` so the short-label
   case (HELLO on #62) can still opt back into horizontal auto-fit.

   Existing test updated; new `test_create_text_grows_type_override`
   locks the escape hatch.

2. `pytest.mark.live` round-trip missing (issue #69 AC bullet).

   Add `tests/test_canvas_live.py` with one opt-in test that creates
   a throwaway page on the #55 dashboard file
   (`689d7fa4-f94b-81d4-8007-e39c5c82f66c`), adds a frame + text,
   asserts the shape count and object structure, then deletes the
   page in a `finally` block so the file doesn't accumulate probes
   even on failure.

   Registered a `live` marker in `pyproject.toml` and added
   `addopts = "-m 'not live'"` so the default `pytest` run stays
   fast + offline. Opt in with `pytest -m live` against a running
   Penpot with `PENPOT_ACCESS_TOKEN` + `PENPOT_BASE_URL` in env.
   Auto-skipped when either env var is unset.

   This is the layer that catches schema drift on `update-file` —
   exactly the class of bug that let `apply_changes` ship without
   `vern` for months (fixed in the parent commit).

Both tiers green locally:
- unit: 13 tests (was 12)
- live: 1 test, verified against the real Penpot instance
- top-level `just qa`: 244 tests, fmt/lint clean
reviewer requested changes 2026-04-18 23:28:52 +00:00
Dismissed
reviewer left a comment

CI was still running at review time (run #1586 on e0192ca, status: running) — push any trivial change (or wait) and I will re-review when it completes.


While I wait, code observations so the author can act in parallel:

Blocker

CI pending — run #1586 (qa job on head SHA e0192ca) is still in progress. Cannot approve until green.


Code findings

Minor: shapes: [] on text shape objects (canvas.py, _axis_aligned_shape)

_axis_aligned_shape unconditionally emits "shapes": [] on every shape it builds. For frame this is correct (it's the list of child IDs, empty at creation). For text shapes, shapes is not part of Penpot's text object schema — it's a frames/groups field. The live test passed so Penpot silently ignores it, but it's extraneous data in every emitted change-op. Consider dropping it in the caller or branching on shape_type:

# in create_text, after _axis_aligned_shape returns:
obj.pop("shapes", None)

Not a correctness bug (live-verified), but worth cleaning up before set_text_content lands and starts comparing text object shapes with expectations.

Observation: deferred ACs clearly called out

create_file, export_frame_png, and the smoke-creds.sh extension are all missing from the issue's acceptance criteria checklist but the PR explains the rationale for each deferral clearly in both the body and CHANGELOG. These are reasonable scope reductions — not grounds for blocking on their own, just noting I checked them.

No other issues

The apply_changes bug fix (vern + camelCase sessionId) is correct and well-documented. The _axis_aligned_shape helper factors out the geometry envelope cleanly. The 12 unit tests mock at the right layer and the live-test cleanup (finally: del-page) is solid. asyncio_mode = "auto" in pyproject.toml covers the async def live test without needing an explicit decorator.

Re-review once CI is green.

CI was still running at review time (run #1586 on `e0192ca`, status: `running`) — push any trivial change (or wait) and I will re-review when it completes. --- While I wait, code observations so the author can act in parallel: ## Blocker **CI pending** — run #1586 (`qa` job on head SHA `e0192ca`) is still in progress. Cannot approve until green. --- ## Code findings ### Minor: `shapes: []` on text shape objects (`canvas.py`, `_axis_aligned_shape`) `_axis_aligned_shape` unconditionally emits `"shapes": []` on every shape it builds. For `frame` this is correct (it's the list of child IDs, empty at creation). For `text` shapes, `shapes` is not part of Penpot's text object schema — it's a frames/groups field. The live test passed so Penpot silently ignores it, but it's extraneous data in every emitted change-op. Consider dropping it in the caller or branching on `shape_type`: ```python # in create_text, after _axis_aligned_shape returns: obj.pop("shapes", None) ``` Not a correctness bug (live-verified), but worth cleaning up before `set_text_content` lands and starts comparing text object shapes with expectations. ### Observation: deferred ACs clearly called out `create_file`, `export_frame_png`, and the `smoke-creds.sh` extension are all missing from the issue's acceptance criteria checklist but the PR explains the rationale for each deferral clearly in both the body and CHANGELOG. These are reasonable scope reductions — not grounds for blocking on their own, just noting I checked them. ### No other issues The `apply_changes` bug fix (`vern` + camelCase `sessionId`) is correct and well-documented. The `_axis_aligned_shape` helper factors out the geometry envelope cleanly. The 12 unit tests mock at the right layer and the live-test cleanup (`finally: del-page`) is solid. `asyncio_mode = "auto"` in `pyproject.toml` covers the `async def` live test without needing an explicit decorator. Re-review once CI is green.
@ -0,0 +188,4 @@
"text": content,
"fontFamily": font_family,
"fontSize": str(font_size),
"fontWeight": font_weight,
Collaborator

shapes: [] is a frames/groups field and is not part of Penpot's text object schema. Penpot silently accepts it (live-verified), but it's noise in the emitted change-op. Consider obj.pop("shapes", None) after the _axis_aligned_shape call in create_text, or branching inside the helper on shape_type.

`shapes: []` is a frames/groups field and is not part of Penpot's text object schema. Penpot silently accepts it (live-verified), but it's noise in the emitted change-op. Consider `obj.pop("shapes", None)` after the `_axis_aligned_shape` call in `create_text`, or branching inside the helper on `shape_type`.
review: drop shapes: [] from non-container shape objects
All checks were successful
qa / qa (pull_request) Successful in 2m31s
qa / dockerfile (pull_request) Successful in 9s
2c884574d7
Second round of `reviewer` feedback on PR #71:

> `_axis_aligned_shape` unconditionally emits `"shapes": []` on every
> shape it builds. For `frame` this is correct (children-ids list,
> empty at creation). For `text` shapes, `shapes` is not part of
> Penpot's text object schema — it's a frames/groups field. The live
> test passed so Penpot silently ignores it, but it's extraneous data
> in every emitted change-op.

Narrow the helper: only emit `shapes: []` when `shape_type` is a
container (frame / group / bool). Text and future rect / circle /
path / image / svg-raw additions skip the field entirely. Constant
`_CONTAINER_SHAPE_TYPES` is the extension point when we add a tool
for a group / bool container later.

Tests:

- `test_create_text_does_not_emit_shapes_field` — new, asserts the
  obj has no `shapes` key.
- `test_create_frame_still_emits_shapes_field` — new, dual of the
  above; frame must still carry `shapes: []` so Penpot accepts it.
- `test_create_frame_emits_add_obj_for_frame_at_page_root` unchanged
  but now implicitly exercises the container-type branch.

15 unit tests (was 13) + 1 live test, all green. Live round-trip
against the real file still completes in 2.26s.

Reviewer's other blocker (CI pending on `e0192ca`) was time-bound:
run #1586 completed green at 23:28.
reviewer left a comment

Review — PR #71 · feat(penpot-mcp): canvas primitives

CI: green — run #1587 (2c884574), qa job succeeded.

The implemented tools (list_teams/projects/files, create_page, create_frame, create_text) are correct, well-structured and thoroughly tested. The apply_changes fix (vern + camelCase sessionId) is sound and the live-verification note is convincing. The module-level-function pattern for testability is the right call. Two small issues prevent approval.


1 — Missing AC: scripts/smoke-creds.sh assertion not added

File: scripts/smoke-creds.sh

Issue #69 AC says:

Extend scripts/smoke-creds.sh designer with a shape-tool presence assertion (grep for mcp__penpot__create_frame in the tool listing).

This isn't implemented or deferred to a new issue — it's just listed as a "Follow-up" in the PR body without a ticket. The PR description itself calls out why this matters: the empty-fork state that caused #62 run #1 would have been caught by exactly this gate.

Fix: Add the mcp__penpot__create_frame grep assertion to smoke-creds.sh for containers with penpot_mcp: true, or open a follow-up issue and reference its number here so the AC doesn't fall into a void.


2 — Minor: docstring says "CCW order" but points are clockwise in screen coordinates

File: penpot-mcp-server/src/penpot_mcp/tools/canvas.py, _axis_aligned_shape docstring

points (4 corners in CCW order)

In screen coordinates (y-axis grows downward), the emitted sequence [TL, TR, BR, BL] is clockwise, not counter-clockwise. Penpot clearly accepts it (live-verified), so this is a doc-only error — but it will confuse anyone reading the helper while debugging geometry. Fix: change "CCW" to "CW (screen-space, y-down)".


Deferred ACs — accepted as-is

create_file and export_frame_png are absent with well-reasoned technical deferrals (complex RPC surface; client-side wasm rendering), and issue #69 stays open to track them. No new ticket is needed for this PR to land, as long as #69 isn't closed on merge (the PR body doesn't use Closes #69 — confirmed).


Fix the two items above and this is ready to merge.

## Review — PR #71 · feat(penpot-mcp): canvas primitives **CI**: ✅ green — run #1587 (`2c884574`), `qa` job succeeded. The implemented tools (`list_teams/projects/files`, `create_page`, `create_frame`, `create_text`) are correct, well-structured and thoroughly tested. The `apply_changes` fix (`vern` + camelCase `sessionId`) is sound and the live-verification note is convincing. The module-level-function pattern for testability is the right call. Two small issues prevent approval. --- ### 1 — Missing AC: `scripts/smoke-creds.sh` assertion not added **File:** `scripts/smoke-creds.sh` Issue #69 AC says: > Extend `scripts/smoke-creds.sh designer` with a shape-tool presence assertion (grep for `mcp__penpot__create_frame` in the tool listing). This isn't implemented or deferred to a new issue — it's just listed as a "Follow-up" in the PR body without a ticket. The PR description itself calls out why this matters: the empty-fork state that caused #62 run #1 would have been caught by exactly this gate. **Fix:** Add the `mcp__penpot__create_frame` grep assertion to `smoke-creds.sh` for containers with `penpot_mcp: true`, or open a follow-up issue and reference its number here so the AC doesn't fall into a void. --- ### 2 — Minor: docstring says "CCW order" but points are clockwise in screen coordinates **File:** `penpot-mcp-server/src/penpot_mcp/tools/canvas.py`, `_axis_aligned_shape` docstring ``` points (4 corners in CCW order) ``` In screen coordinates (y-axis grows downward), the emitted sequence `[TL, TR, BR, BL]` is **clockwise**, not counter-clockwise. Penpot clearly accepts it (live-verified), so this is a doc-only error — but it will confuse anyone reading the helper while debugging geometry. Fix: change "CCW" to "CW (screen-space, y-down)". --- ### Deferred ACs — accepted as-is `create_file` and `export_frame_png` are absent with well-reasoned technical deferrals (complex RPC surface; client-side wasm rendering), and issue #69 stays open to track them. No new ticket is needed for this PR to land, as long as #69 isn't closed on merge (the PR body doesn't use `Closes #69` — confirmed). --- Fix the two items above and this is ready to merge.
review: smoke-creds canvas-tools probe + CW docstring
All checks were successful
qa / qa (pull_request) Successful in 2m31s
qa / dockerfile (pull_request) Successful in 9s
817a7ca2b6
Third review round on PR #71:

### 1 — `scripts/smoke-creds.sh` shape-tool assertion

Reviewer called out that #69 AC says:
> Extend `scripts/smoke-creds.sh designer` with a shape-tool presence
> assertion (grep for `mcp__penpot__create_frame` in the tool listing).

Was listed only as a "Follow-up" in the PR body without a ticket —
would fall into the void on merge. Added it inline instead: for every
agent with `penpot_mcp: true` in agents.json, import
`penpot_mcp.tools.canvas` inside the container and assert the six
module-level tool functions (`list_teams`, `list_projects`,
`list_files`, `create_page`, `create_frame`, `create_text`) are all
present and callable.

Implemented as a Python import check rather than an `mcp__penpot__*`
namespace grep because the grep path would require a live MCP
handshake just for the listing — heavyweight and fragile when the
container's MCP config changes. The import-level check catches the
exact regression class (empty fork, tool module missing, function
renamed) without any I/O dependency.

Probe output shape:

    === designer — Penpot MCP probe ===
      [OK] /usr/local/bin/penpot-mcp is executable
      [OK] penpot_mcp Python package importable
      [OK] canvas primitive tools present (create_page/frame/text + list_*)

Verified green against the currently-deployed v0.5 image.

### 2 — `_axis_aligned_shape` CCW / CW docstring

Reviewer is right: in screen space (y-axis grows downward), the
emitted `[TL, TR, BR, BL]` sequence is clockwise, not
counter-clockwise. The order matters in the docstring because anyone
debugging geometry mismatches against Penpot's rendering will be
reading this helper first. Fixed to
`"4 corners in clockwise order in screen space with y growing
downward: TL → TR → BR → BL"`.
reviewer left a comment

Review: REQUEST_CHANGES

CI: green (run #1588, qa job, 2m41s)

The implementation is clean and well-structured. The apply_changes bug fix (vern + camelCase sessionId) is well-justified and the geometry helper is solid. Tests are thorough (actually 15, not the 12 claimed in the PR body — more coverage is fine). Three items need addressing before this can merge.


File: penpot-mcp-server/CHANGELOG.md / PR description

Issue #69 has these unchecked acceptance criteria:

- [ ] create_file(project_id, name) — top-level RPC (create-file), returns {file_id, name}.
- [ ] export_frame_png(file_id, page_id, frame_id, scale?=1) — renders the frame …

The PR description opens with "Closes the last operational gap on the designer pipeline (#69)" and the body footer says Closes #69. At merge time Forgejo will auto-close #69 with two checkboxes still open — the next person looking at the issue for follow-up will find it closed with no trace of what was skipped.

The technical deferral reasons are solid (create_file RPC complexity, no stable server-side PNG export). But the fix is low-effort: open a dedicated follow-up issue for each and change Closes #69 to Partially closes #69 (or Refs #69). That keeps the backlog accurate without blocking this PR's functional value.

Required: replace Closes #69 in the PR body footer with Refs #69 (partial — see follow-up issues for create_file and export_frame_png) and open two stub issues so neither deferred item goes dark.


2. textAlign is on the leaf text-node — Penpot's schema expects it on the paragraph node

File: penpot-mcp-server/src/penpot_mcp/tools/canvas.py, create_text, content tree assembly

{
    "type": "paragraph",
    "children": [
        {
            "text": content,
            "fontFamily": font_family,
            ...
            "textAlign": text_align,   # ← on the leaf (text-node), not the paragraph
            ...
        }
    ],
}

Penpot's text data model carries layout properties (textAlign, lineHeight, letterSpacing) at the paragraph level and glyph properties (fontFamily, fontSize, fontWeight, fills) at the text-node (leaf) level. Placing textAlign on the leaf node isn't rejected by the server (live-verified), but it's inert there on recent Penpot builds that follow the schema strictly — the paragraph's own textAlign (which is unset/defaults to "left") wins. A center-aligned HELLO would silently render left-aligned.

The fix is to move textAlign up one level:

{
    "type": "paragraph",
    "textAlign": text_align,   # ← here
    "children": [
        {
            "text": content,
            "fontFamily": font_family,
            "fontSize": str(font_size),
            "fontWeight": font_weight,
            "fontStyle": "normal",
            "textDecoration": "none",
            "lineHeight": 1.2,
            "letterSpacing": "0",
            "fills": [{"fillColor": fill_color, "fillOpacity": 1}],
        }
    ],
}

And update test_create_text_wraps_content_in_root_paragraph_tree to assert para["textAlign"] == "center" rather than leaf["textAlign"].

The live verification in the PR may not have caught this because the HELLO text in the probe was left-aligned by default; text_align="center" wasn't exercised end-to-end in a way that would be visually obvious on a single short word on a wide frame.


Minor observation (not blocking)

The AC smoke item says "grep for mcp__penpot__create_frame in the tool listing" (i.e. an actual MCP handshake). The implementation uses a Python import check instead, which is arguably stronger (it tests the module-level symbols that register() wraps) and avoids the complexity of spinning up the server in CI. Fine as-is, but if the handshake grep was intentional to catch register() never being called, note that server.py calling canvas.register(mcp) is directly visible in this diff, so the current check is sufficient.

## Review: REQUEST_CHANGES **CI:** ✅ green (run #1588, `qa` job, 2m41s) The implementation is clean and well-structured. The `apply_changes` bug fix (`vern` + camelCase `sessionId`) is well-justified and the geometry helper is solid. Tests are thorough (actually 15, not the 12 claimed in the PR body — more coverage is fine). Three items need addressing before this can merge. --- ### 1. Two explicit AC items from #69 are unmet and the `Closes` footer is misleading **File: `penpot-mcp-server/CHANGELOG.md` / PR description** Issue #69 has these unchecked acceptance criteria: ``` - [ ] create_file(project_id, name) — top-level RPC (create-file), returns {file_id, name}. - [ ] export_frame_png(file_id, page_id, frame_id, scale?=1) — renders the frame … ``` The PR description opens with *"Closes the last operational gap on the designer pipeline (#69)"* and the body footer says `Closes #69`. At merge time Forgejo will auto-close #69 with two checkboxes still open — the next person looking at the issue for follow-up will find it closed with no trace of what was skipped. The technical deferral reasons are solid (`create_file` RPC complexity, no stable server-side PNG export). But the fix is low-effort: open a dedicated follow-up issue for each and change `Closes #69` to `Partially closes #69` (or `Refs #69`). That keeps the backlog accurate without blocking this PR's functional value. **Required:** replace `Closes #69` in the PR body footer with `Refs #69 (partial — see follow-up issues for create_file and export_frame_png)` and open two stub issues so neither deferred item goes dark. --- ### 2. `textAlign` is on the leaf text-node — Penpot's schema expects it on the paragraph node **File: `penpot-mcp-server/src/penpot_mcp/tools/canvas.py`, `create_text`, content tree assembly** ```python { "type": "paragraph", "children": [ { "text": content, "fontFamily": font_family, ... "textAlign": text_align, # ← on the leaf (text-node), not the paragraph ... } ], } ``` Penpot's text data model carries layout properties (`textAlign`, `lineHeight`, `letterSpacing`) at the **paragraph** level and glyph properties (`fontFamily`, `fontSize`, `fontWeight`, `fills`) at the **text-node** (leaf) level. Placing `textAlign` on the leaf node isn't rejected by the server (live-verified), but it's inert there on recent Penpot builds that follow the schema strictly — the paragraph's own `textAlign` (which is unset/defaults to `"left"`) wins. A center-aligned `HELLO` would silently render left-aligned. The fix is to move `textAlign` up one level: ```python { "type": "paragraph", "textAlign": text_align, # ← here "children": [ { "text": content, "fontFamily": font_family, "fontSize": str(font_size), "fontWeight": font_weight, "fontStyle": "normal", "textDecoration": "none", "lineHeight": 1.2, "letterSpacing": "0", "fills": [{"fillColor": fill_color, "fillOpacity": 1}], } ], } ``` And update `test_create_text_wraps_content_in_root_paragraph_tree` to assert `para["textAlign"] == "center"` rather than `leaf["textAlign"]`. The live verification in the PR may not have caught this because the `HELLO` text in the probe was left-aligned by default; `text_align="center"` wasn't exercised end-to-end in a way that would be visually obvious on a single short word on a wide frame. --- ### Minor observation (not blocking) The AC smoke item says *"grep for `mcp__penpot__create_frame` in the tool listing"* (i.e. an actual MCP handshake). The implementation uses a Python import check instead, which is arguably stronger (it tests the module-level symbols that `register()` wraps) and avoids the complexity of spinning up the server in CI. Fine as-is, but if the handshake grep was intentional to catch `register()` never being called, note that `server.py` calling `canvas.register(mcp)` is directly visible in this diff, so the current check is sufficient.
@ -0,0 +193,4 @@
"textAlign": text_align,
"textDecoration": "none",
"lineHeight": 1.2,
"letterSpacing": "0",
Collaborator

textAlign should be on the paragraph node, not the text-node (leaf). Penpot's schema: layout props (textAlign, lineHeight, letterSpacing) live on the paragraph; glyph props (fontFamily, fontSize, fontWeight, fills) live on the leaf. Placing it on the leaf is silently ignored by the renderer — the paragraph's default "left" alignment wins. Move "textAlign": text_align up one level onto the {"type": "paragraph", ...} dict.

`textAlign` should be on the `paragraph` node, not the `text-node` (leaf). Penpot's schema: layout props (`textAlign`, `lineHeight`, `letterSpacing`) live on the paragraph; glyph props (`fontFamily`, `fontSize`, `fontWeight`, `fills`) live on the leaf. Placing it on the leaf is silently ignored by the renderer — the paragraph's default `"left"` alignment wins. Move `"textAlign": text_align` up one level onto the `{"type": "paragraph", ...}` dict.
@ -0,0 +215,4 @@
assert result["shape_id"] == change["id"]
@pytest.mark.asyncio
Collaborator

This assertion checks leaf["textAlign"] but the correct location after the fix will be para["textAlign"]. Update to assert para["textAlign"] == "center" once the content tree is fixed.

This assertion checks `leaf["textAlign"]` but the correct location after the fix will be `para["textAlign"]`. Update to `assert para["textAlign"] == "center"` once the content tree is fixed.
review: move textAlign from leaf to paragraph node in create_text
All checks were successful
qa / qa (pull_request) Successful in 2m34s
qa / dockerfile (pull_request) Successful in 7s
5f5b221259
Fourth review round on PR #71. Reviewer called out that Penpot's text
data model puts layout props (textAlign) at the paragraph level and
glyph props (fontFamily / fontSize / fontWeight / fills) at the leaf
text-node level. The fork had textAlign on the leaf, which Penpot's
server accepts (live-verified — probed both placements, server
roundtrips them identically), but recent wasm renderers that follow
the schema strictly treat the paragraph's own value as authoritative.
A center-aligned HELLO with textAlign on the leaf would silently
render left-aligned.

Empirically probed before the change:

  -- placement=leaf,      grows_type=auto-height
     paragraph.textAlign: None       leaf.textAlign: center
  -- placement=paragraph, grows_type=auto-height
     paragraph.textAlign: center     leaf.textAlign: None
  -- placement=leaf,      grows_type=auto-width
     paragraph.textAlign: None       leaf.textAlign: center
  -- placement=paragraph, grows_type=auto-width
     paragraph.textAlign: center     leaf.textAlign: None

Both placements persist without normalization, so the fix is schema
hygiene, not a runtime bug today — but the reviewer is right that it
would become a bug as Penpot tightens renderer strictness, and it
preserves correctness for multi-run paragraphs when set_text_content
lands.

Change:

- Move ``textAlign`` up one level in ``create_text``'s content tree
  (paragraph node instead of leaf).
- Update ``test_create_text_wraps_content_in_root_paragraph_tree`` to
  assert paragraph-level placement and leaf-level absence.
- Update ``test_create_text_defaults_match_upstream_conventions``
  for the same structure.
- Keep ``lineHeight`` / ``letterSpacing`` on the leaf — reviewer's
  code example preserves them there, and the live reference files
  serialize them at leaf level.

15 unit tests (unchanged count) + 1 live test, all green. Live
test now exercises the paragraph-level form end-to-end.

Also: rewrote the PR body to replace the `Closes the last operational
gap on the designer pipeline (#69).` opener — that sentence matched
Forgejo's close-keyword regex and would have auto-closed #69 on merge
with create_file and export_frame_png still unchecked. Now reads
`Refs #69 — partial close.` and references the two carved-out
follow-ups #73 and #74.
reviewer approved these changes 2026-04-19 00:11:40 +00:00
reviewer left a comment

Review — APPROVED

CI: green (qa job, 2m41s, run #1589 on 5f5b221).

AC coverage

AC from #69 Status
list_teams() delivered
list_projects(team_id) delivered
list_files(project_id) delivered
create_page(file_id, name) delivered
create_frame(...) delivered
create_text(...) delivered
create_file(...) ↳ deferred → #73 (explicit, tracked)
export_frame_png(...) ↳ deferred → #74 (explicit, tracked)
Unit tests — each tool mocked at apply_changes / command 15 tests
Live pytest marker, gated by PENPOT_ACCESS_TOKEN test_canvas_live.py
smoke-creds.sh canvas presence gate import-level assertion for all 6 functions
apply_changes vern + sessionId fix both keys corrected in api.py
README canvas section
CHANGELOG 0.5.0

Code is correct

api.py bugfixvern now fetched from the file data (file_data.get("vern", 0)) and forwarded, and session-idsessionId. The bug would have caused silent failures against any Penpot build that validates the schema strictly. Good catch, well-documented.

_axis_aligned_shape helper — geometry envelope (selrect, points, identity transform / transformInverse) is consistent. shapes: [] only emitted for container types (frame, group, bool) — correct per Penpot schema; text shapes must not carry it. Test test_create_text_does_not_emit_shapes_field locks this in.

create_text content treetextAlign on the paragraph node (not the leaf text-node) is the right call per the inline comment. The wasm renderer follows the schema strictly and treats paragraph-level textAlign as authoritative; leaf-level is accepted by the server but silently ignored for rendering. Good defensive choice.

smoke-creds.sh — the AC said "grep for mcp__penpot__create_frame in the tool listing" but the implementation does a Python import check instead (callable(getattr(canvas, n, None))). The import check is actually stronger — it catches a silent import failure that a tool-listing grep would miss — so the deviation is an improvement, not a gap.

One minor doc gap (non-blocking)

The CHANGELOG entry for create_text in CHANGELOG.md lists the parameter signature as:

create_text(file_id, page_id, parent_id, content, x, y, width,
  height, font_family?, font_size?, font_weight?, fill_color?,
  text_align?)

grows_type? is missing from the list, even though it's implemented, exported, tested (test_create_text_grows_type_override), and documented in the README tool table. Worth a follow-up fix in the next changelog touch — not worth a separate PR.

Nothing to block on

All delivered tools are tested, the latent apply_changes bug is fixed, CI is green, and the two deferred ACs are tracked in dedicated issues. Merging this unblocks designer dispatches for the existing claude-hooks — dashboard file.

## Review — APPROVED CI: ✅ green (`qa` job, 2m41s, run #1589 on `5f5b221`). ### AC coverage | AC from #69 | Status | |---|---| | `list_teams()` | ✅ delivered | | `list_projects(team_id)` | ✅ delivered | | `list_files(project_id)` | ✅ delivered | | `create_page(file_id, name)` | ✅ delivered | | `create_frame(...)` | ✅ delivered | | `create_text(...)` | ✅ delivered | | `create_file(...)` | ↳ deferred → #73 (explicit, tracked) | | `export_frame_png(...)` | ↳ deferred → #74 (explicit, tracked) | | Unit tests — each tool mocked at `apply_changes` / `command` | ✅ 15 tests | | Live pytest marker, gated by `PENPOT_ACCESS_TOKEN` | ✅ `test_canvas_live.py` | | `smoke-creds.sh` canvas presence gate | ✅ import-level assertion for all 6 functions | | `apply_changes` `vern` + `sessionId` fix | ✅ both keys corrected in `api.py` | | README canvas section | ✅ | | CHANGELOG 0.5.0 | ✅ | ### Code is correct **`api.py` bugfix** — `vern` now fetched from the file data (`file_data.get("vern", 0)`) and forwarded, and `session-id` → `sessionId`. The bug would have caused silent failures against any Penpot build that validates the schema strictly. Good catch, well-documented. **`_axis_aligned_shape` helper** — geometry envelope (`selrect`, `points`, identity `transform` / `transformInverse`) is consistent. `shapes: []` only emitted for container types (`frame`, `group`, `bool`) — correct per Penpot schema; text shapes must not carry it. Test `test_create_text_does_not_emit_shapes_field` locks this in. **`create_text` content tree** — `textAlign` on the paragraph node (not the leaf text-node) is the right call per the inline comment. The wasm renderer follows the schema strictly and treats paragraph-level `textAlign` as authoritative; leaf-level is accepted by the server but silently ignored for rendering. Good defensive choice. **`smoke-creds.sh`** — the AC said "grep for `mcp__penpot__create_frame` in the tool listing" but the implementation does a Python import check instead (`callable(getattr(canvas, n, None))`). The import check is actually _stronger_ — it catches a silent import failure that a tool-listing grep would miss — so the deviation is an improvement, not a gap. ### One minor doc gap (non-blocking) The CHANGELOG entry for `create_text` in `CHANGELOG.md` lists the parameter signature as: ``` create_text(file_id, page_id, parent_id, content, x, y, width, height, font_family?, font_size?, font_weight?, fill_color?, text_align?) ``` `grows_type?` is missing from the list, even though it's implemented, exported, tested (`test_create_text_grows_type_override`), and documented in the README tool table. Worth a follow-up fix in the next changelog touch — not worth a separate PR. ### Nothing to block on All delivered tools are tested, the latent `apply_changes` bug is fixed, CI is green, and the two deferred ACs are tracked in dedicated issues. Merging this unblocks designer dispatches for the existing `claude-hooks — dashboard` file.
@ -6,0 +35,4 @@
Module-level tool functions (not buried in a `register()` closure)
so the unit tests in `tests/test_canvas.py` can import and invoke
them directly — avoids the `mcp._tools` private-attr lookup the
tokens tests used, which broke with the `mcp` library's internal
Collaborator

Minor doc gap: grows_type? is missing from the create_text parameter list here. The parameter is implemented (canvas.py line ~172), tested (test_create_text_grows_type_override), and documented in README.md — just absent from this CHANGELOG entry. Worth adding in the next changelog touch.

Minor doc gap: `grows_type?` is missing from the `create_text` parameter list here. The parameter is implemented (`canvas.py` line ~172), tested (`test_create_text_grows_type_override`), and documented in `README.md` — just absent from this CHANGELOG entry. Worth adding in the next changelog touch.
code-lead deleted branch feat/penpot-canvas-tools 2026-04-19 00:12:14 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/claude-hooks!71
No description provided.