feat(penpot-mcp): canvas primitives — list_*, create_page, create_frame, create_text #71
No reviewers
Labels
No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
charles/claude-hooks!71
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/penpot-canvas-tools"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs #69 — partial close. Ships most canvas primitives; the two
remaining AC bullets (
create_file,export_frame_png) are deferredto 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:designissues hit the "no shape primitives" abort. This lands the minimum
set that makes the
design-implement.mdskill's happy path callableend-to-end. Live-verified on the existing file from #55 with a
throwaway
canvas-probepage — Penpot rendered the frame + textexactly 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-filechange-op each):create_page(file_id, name)→add-pagecreate_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_changeswasn't sendingvernor using camelCasesessionId.The token write tools from 0.3 would have silently broken against
live Penpot on any server build that required
vern; tests mockedabove the HTTP layer so this never surfaced. Both keys fixed in
api.py— transit-json decode was already landed in 0.4 viaAccept: application/json.Deferred to follow-ups (tracked)
create_file→ #73. Penpot'screate-fileRPC has aricher 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 PNGsclient-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.commandand the emitted change-op is asserted for shape,keys, and values. Module-level tool functions so tests import
them directly (avoiding the FastMCP
_toolsprivate-attr trapthat broke
test_tokens.py).penpot-mcp-server/tests/test_canvas_live.py— 1 live test(
pytest -m live), gated behindPENPOT_ACCESS_TOKEN. Createsa temp page + frame + text on the
claude-hooks — dashboardfile, asserts shape count, cleans up in
finally.scripts/smoke-creds.sh— probe asserts the six canvasmodule-level functions exist in
penpot_mcp.tools.canvas(regression gate for the empty-fork state that caused #62
run #1).
just qaon the TS side — 244 tests, fmt/lint clean.689d7fa4-f94b-81d4-8007-e39c5c82f66c(the
claude-hooks — dashboardfile from #55): created page,frame with Tokyo Night bg, Fira Code 40/700
HELLOtext,verified shape count.
should produce the Penpot artifacts on first try.
Follow-ups
create_file(carved out).export_frame_png(carved out).set_text_content— multi-paragraph / styled text (current toolis single-paragraph plain string). Not filed yet; wait for a real
caller.
Review — feat(penpot-mcp): canvas primitives
CI ✅ green (run #1585, 2m44s).
Overall this is clean: the
apply_changesbug fix is correct and well-documented, the module-level tool pattern avoids the_toolstrap, 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 thewidthparameterFile:
penpot-mcp-server/src/penpot_mcp/tools/canvas.py, line 164The function accepts explicit
widthandheightarguments and passes them into_axis_aligned_shape, which stores them in the shape's geometry envelope (includingselrect,points). But"auto-width"mode in Penpot tells the renderer to horizontally resize the text shape to fit its content, overridingwidthat 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 respectswidthfor line wrapping and grows the box downward. This makes thewidthparameter meaningful.Fix option B — keep
"auto-width"but drop thewidth/heightparameters from the signature, replacing them with justx/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
widthandheightare 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.livetestFile:
penpot-mcp-server/tests/— nopytest.mark.livetest presentThe issue AC states:
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_fileandexport_frame_png, which have clear explanations for deferral). Without it, a future regression inapply_changesor the Penpot RPC schema would only surface on next designer dispatch rather than on the nextpytest -m liverun.Add a
tests/test_canvas_live.pywith at minimum:AC coverage summary
list_teams/list_projects/list_filescreate_pagecreate_framecreate_textcreate_fileexport_frame_pngpytest.mark.livetestapply_changesvern + camelCase fixsmoke-creds.shshape-tool assertion@ -0,0 +161,4 @@x=x,y=y,width=width,height=height,auto-widthcauses Penpot to ignore thewidthparameter at render time — the text box expands horizontally to fit its content. This makes thewidthargument 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 charsThe AC requires a
pytest.mark.livetest gated behindPENPOT_ACCESS_TOKEN(create temp page → frame → text → assert shape count → delete page on file689d7fa4-f94b-81d4-8007-e39c5c82f66c). This test file covers unit mocks only. Please add atests/test_canvas_live.pywith@pytest.mark.skipif(not os.getenv("PENPOT_ACCESS_TOKEN"), ...)so future regressions in the RPC schema are caught before dispatch.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 (
qajob on head SHAe0192ca) is still in progress. Cannot approve until green.Code findings
Minor:
shapes: []on text shape objects (canvas.py,_axis_aligned_shape)_axis_aligned_shapeunconditionally emits"shapes": []on every shape it builds. Forframethis is correct (it's the list of child IDs, empty at creation). Fortextshapes,shapesis 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 onshape_type:Not a correctness bug (live-verified), but worth cleaning up before
set_text_contentlands and starts comparing text object shapes with expectations.Observation: deferred ACs clearly called out
create_file,export_frame_png, and thesmoke-creds.shextension 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_changesbug fix (vern+ camelCasesessionId) is correct and well-documented. The_axis_aligned_shapehelper 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"inpyproject.tomlcovers theasync deflive 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,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. Considerobj.pop("shapes", None)after the_axis_aligned_shapecall increate_text, or branching inside the helper onshape_type.shapes: []from non-container shape objectsReview — PR #71 · feat(penpot-mcp): canvas primitives
CI: ✅ green — run #1587 (
2c884574),qajob succeeded.The implemented tools (
list_teams/projects/files,create_page,create_frame,create_text) are correct, well-structured and thoroughly tested. Theapply_changesfix (vern+ camelCasesessionId) 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.shassertion not addedFile:
scripts/smoke-creds.shIssue #69 AC says:
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_framegrep assertion tosmoke-creds.shfor containers withpenpot_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_shapedocstringIn 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_fileandexport_frame_pngare 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 useCloses #69— confirmed).Fix the two items above and this is ready to merge.
Review: REQUEST_CHANGES
CI: ✅ green (run #1588,
qajob, 2m41s)The implementation is clean and well-structured. The
apply_changesbug fix (vern+ camelCasesessionId) 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
Closesfooter is misleadingFile:
penpot-mcp-server/CHANGELOG.md/ PR descriptionIssue #69 has these unchecked acceptance criteria:
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_fileRPC complexity, no stable server-side PNG export). But the fix is low-effort: open a dedicated follow-up issue for each and changeCloses #69toPartially closes #69(orRefs #69). That keeps the backlog accurate without blocking this PR's functional value.Required: replace
Closes #69in the PR body footer withRefs #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.
textAlignis on the leaf text-node — Penpot's schema expects it on the paragraph nodeFile:
penpot-mcp-server/src/penpot_mcp/tools/canvas.py,create_text, content tree assemblyPenpot'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. PlacingtextAlignon 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 owntextAlign(which is unset/defaults to"left") wins. A center-alignedHELLOwould silently render left-aligned.The fix is to move
textAlignup one level:And update
test_create_text_wraps_content_in_root_paragraph_treeto assertpara["textAlign"] == "center"rather thanleaf["textAlign"].The live verification in the PR may not have caught this because the
HELLOtext 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_framein 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 thatregister()wraps) and avoids the complexity of spinning up the server in CI. Fine as-is, but if the handshake grep was intentional to catchregister()never being called, note thatserver.pycallingcanvas.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",textAlignshould be on theparagraphnode, not thetext-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_alignup one level onto the{"type": "paragraph", ...}dict.@ -0,0 +215,4 @@assert result["shape_id"] == change["id"]@pytest.mark.asyncioThis assertion checks
leaf["textAlign"]but the correct location after the fix will bepara["textAlign"]. Update toassert para["textAlign"] == "center"once the content tree is fixed.create_file(project_id, name)#73export_frame_png(file_id, page_id, frame_id, scale?)#74Review — APPROVED
CI: ✅ green (
qajob, 2m41s, run #1589 on5f5b221).AC coverage
list_teams()list_projects(team_id)list_files(project_id)create_page(file_id, name)create_frame(...)create_text(...)create_file(...)export_frame_png(...)apply_changes/commandPENPOT_ACCESS_TOKENtest_canvas_live.pysmoke-creds.shcanvas presence gateapply_changesvern+sessionIdfixapi.pyCode is correct
api.pybugfix —vernnow fetched from the file data (file_data.get("vern", 0)) and forwarded, andsession-id→sessionId. The bug would have caused silent failures against any Penpot build that validates the schema strictly. Good catch, well-documented._axis_aligned_shapehelper — geometry envelope (selrect,points, identitytransform/transformInverse) is consistent.shapes: []only emitted for container types (frame,group,bool) — correct per Penpot schema; text shapes must not carry it. Testtest_create_text_does_not_emit_shapes_fieldlocks this in.create_textcontent tree —textAlignon 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-leveltextAlignas authoritative; leaf-level is accepted by the server but silently ignored for rendering. Good defensive choice.smoke-creds.sh— the AC said "grep formcp__penpot__create_framein 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_textinCHANGELOG.mdlists the parameter signature as: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_changesbug is fixed, CI is green, and the two deferred ACs are tracked in dedicated issues. Merging this unblocks designer dispatches for the existingclaude-hooks — dashboardfile.@ -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 invokethem directly — avoids the `mcp._tools` private-attr lookup thetokens tests used, which broke with the `mcp` library's internalMinor doc gap:
grows_type?is missing from thecreate_textparameter list here. The parameter is implemented (canvas.pyline ~172), tested (test_create_text_grows_type_override), and documented inREADME.md— just absent from this CHANGELOG entry. Worth adding in the next changelog touch.