The earlier doc-backed fix left one leak: handleGetDesignMd/handleSetDesignMd
still called setDesignMdForPrompt(spec), which wrote into a process-level
module variable `_designMdContent` that get_design_prompt's "style" +
"design-md" sections read. Switching between documents kept the prior
file's policy; get_design_prompt itself had no filePath parameter so it
couldn't even identify the current document.
Fix:
- Delete `_designMdContent` / setDesignMdForPrompt / getDesignMdForPrompt.
- `buildDesignPrompt(section, designMdPolicy?)` takes policy as an explicit
stateless argument.
- Export `designMdSpecToPromptPolicy(spec)` — pure converter.
- Add `filePath` to get_design_prompt's schema. The route handler opens
the addressed document, derives the policy from `doc.designMd`, and
threads it into buildDesignPrompt. Add `design-md` to the section enum
(previously only returned via the "style" override).
- design-md.ts handlers no longer touch the old setter.
Verified by a two-file live smoke: set design.md on A → get_design_prompt
on B returns "No design.md loaded" with no A-specific tokens.
design.md was stored in a global Zustand store + per-file-key localStorage
in apps/web, and in a module-level cache in pen-mcp. Both leaked across
files: a newly-created document could pick up the previous file's dark
palette (async clearForNewDocument raced with AI chat reads; hydrate()
could rehydrate the last file's designMd on refresh; shared .pen files
lost the spec entirely because it wasn't inside the document).
Fix:
- Add `designMd?: DesignMdSpec` to PenDocument (pen-types). It now
serializes with .pen/.op and travels across sessions/users.
- Add `setDesignMd` action to document-store.
- Rewrite design-md-store as a thin mirror over document-store so the
legacy hook API still works. On document load it migrates any legacy
localStorage entry into the opened document and deletes the localStorage
key; hydrate() wipes the orphan `openpencil-design-md-current-key`.
- MCP handleGetDesignMd / handleSetDesignMd / handleExportDesignMd read
`doc.designMd` directly and persist via saveDocument. Removed the
process-level `_mcpDesignMd` cache.
Verified via MCP live round-trip: set on file A → persists to A's .op on
disk → new file B returns hasDesignMd:false (no leak).
pen-core's DEFAULT_FILL is gray-300 (#d1d5db) — any text/icon without
explicit fill renders light gray, unreadable on primary blue / dark /
white backgrounds. Set explicit foreground colors on toast text + icon,
fab icon, stepper step numbers, checkbox check, and segmented-control
labels. Added contrast-regression test to lock the invariant.
Feedback + floating + nav batch. progress_bar uses fixed bar_width so the
fill can be derived from value/100 (pen-core has no percent sizing).
Stepper connectors use fill_container so the bar splits evenly between
circles.
Layout-engine trap: a fill_container child inside a fit_content parent
resolves to the grandparent's width (pen-core engine.ts:182-187), so the
active tab's underline rect blew the whole tab up to bar width. Switched
every tab to width=fill_container (Twitter/Material pattern) so the bar
splits evenly and the underline sits correctly inside its slot.
PenStroke.thickness only supports number | [T,R,B,L] — {bottom:N} silently
resolves to 0 in resolveStrokeWidth so the old tab underline never rendered.
Switched active tab to a vertical frame with a sibling rectangle underline
(role=tab-underline), matching how add_divider_v0 already handles this.
Codex stop-hook: design-routes.ts reached 849 lines, violating the
repo's CLAUDE.md "Single files must not exceed 800 lines" rule.
Extract the 17 element-tool JSON schema definitions + names + dispatch
switch into a new file `routes/element-tool-defs.ts` (671 lines). The
core design-routes.ts keeps only:
- 2 core tool defs (get_design_prompt, batch_design)
- LAYERED_DESIGN_TOOLS spread
- D0 spike tool def + dispatch (gated)
- Combined DESIGN_TOOL_DEFINITIONS / DESIGN_TOOL_NAMES / handleDesignToolCall
that merges core + element-tool via re-export
design-routes.ts: 849 → 201 lines
element-tool-defs.ts: 0 → 671 lines (both under the 800 cap; room to
add ~3-5 more element tools before element-tool-defs itself needs
splitting by category — e.g. atom-tool-defs vs row-tool-defs)
handleDesignToolCall falls through via `if (ELEMENT_TOOL_NAMES.has(name))
return handleElementToolCall(name, a)` instead of an inlined 17-case
switch. Same dispatch semantics, much shorter file.
DESIGN_TOOL_NAMES kept as a single exported Set so existing callers
(server.ts, test files) still see all 22 tool names (5 core + 17 element)
via one import. ELEMENT_TOOL_NAMES also exported for tests that want to
assert the split explicitly.
180/180 pen-mcp tests pass unchanged. format + tsc green. Bundle
rebuilt.
Two composition primitives completing the "atoms + composition" tier.
- add_icon_label_v0: atomic icon + text horizontal pair (alignItems=
center, gap=8, fit_content). Building block for menu items,
breadcrumbs, status indicators. Narrow schema: icon always leads,
sizes fixed (icon 16, text 14/500), no alignment enum.
- add_list_row_v0: iOS/Material list row — optional leading icon +
vertical text stack (title + optional subtitle) + optional trailing
icon (typically chevron-right).
No-overlap invariant: middle text stack wrapped in VERTICAL
container with width=fill_container so long titles wrap vertically
instead of pushing the trailing icon out of frame — same pattern
as add_section_header_v0. overflow.md rule: text with
fill_container + fixed-width only propagates wrap height inside
vertical-layout parents. The vertical wrapper is what prevents the
overlap.
Tests: 11 new unit (5 icon-label + 6 list-row). List-row includes
an explicit no-overlap regression test asserting the text stack is
vertical + fill_container. contract test ELEMENT_TOOL_NAMES updated
15 → 17.
elements.md skill gets a new "Composition" category in the decision
tree (items 16-17) + 2 new PREFER phrases + 2 usage examples + role
list extended. d0 snapshot updated.
MCP live smoke: ListTools = 57 (40 baseline + 17 element);
icon_label produces 3-node tree; list_row full variant produces
6-node tree with text stack correctly vertical + fill_container.
180/180 pen-mcp tests pass. format + tsc green. Bundle rebuilt.
Codex stop-hook #17: after fix#16 made add_body_text_v0 always use
Inter for CJK body, one source still allowed the alternative:
cjk-typography.md:16 said "Body: 'Inter' (system CJK fallback) or
'Noto Sans SC'". Every other authority in the repo says body=Inter
unconditionally:
- text-rules.md (text section of get_design_prompt): body='Inter'
- skills/phases/planning/decomposition.md:45: "body='Inter'"
- packages/pen-mcp/src/tools/add-body-text-v0.ts: always 'Inter'
- skills/phases/generation/elements.md: "Inter everywhere"
- role-definitions.md:88: "body-text: lineHeight=1.5 (CJK: 1.6)"
(no font override)
cjk-typography's "or Noto Sans SC" was the lone dissenter — an AI
reading the domain skill would see a contradictory option that no
other skill or tool supports. Remove the alternative so the repo is
single-voiced.
Also clarify the heading vs body split in the last two bullets: the
script-specific Noto rule is HEADING-only; body is Inter + CJK
lineHeight/letterSpacing. Cross-reference the other authorities so
a future editor knows which rule sources must stay in sync.
253/253 tests pass (pen-mcp + pen-ai-skills). format green.
Codex stop-hook #16: CJK guidance was internally contradictory across
repo skills and the handler. Three sources disagreed:
- text-rules.md (design-prompt TEXT_RULES): "body='Inter'" — unqualified
- cjk-typography.md: body is "'Inter' (system CJK fallback) OR
'Noto Sans SC'" — permissive
- My previous add_body_text_v0 (e24c7fc): body was mapped per-script
to Noto Sans SC / JP / KR — this combination is NOT authorized
by any repo skill (cjk-typography only allows Inter or SC; never
lists JP/KR for body)
Authoritative rule: text-rules.md. Body is Inter regardless of
script. Inter has system CJK fallback at render time so a single
body face serves all scripts. ONLY headings dispatch to
script-specific Noto faces (Noto Sans SC for Chinese / JP for
Japanese / KR for Korean) — that's add_heading_v0's job; body
doesn't need the same split.
Fix:
- add_body_text_v0 handler: fontFamily always 'Inter'; script
detection is now used ONLY to decide lineHeight (1.5 Latin /
1.6 CJK) + letterSpacing (undefined Latin / 0 CJK)
- tool description: rewritten to explicitly note "body ALWAYS Inter"
and "only headings dispatch to Noto faces"
- elements.md usage examples: all 4 body examples now show Inter
output with only lineHeight varying by script; added inline
comment clarifying the text-rules.md derivation
- elements.md decision-tree item 15: reworded to "Inter everywhere"
- add-body-text-v0.test.ts: per-script tests now assert fontFamily
===Inter across zh/jp/ko + kanji+hiragana + mixed content
- d0 parity snapshot regenerated (description changes reach
pre-D0 definitions; snapshot works as designed, catches drift
and is updated deliberately)
169/169 pen-mcp tests pass. format + tsc green. Bundle rebuilt.
Codex stop-hook #15: previous fix (91b9054) updated the handlers to
dispatch by script (Chinese → SC / Japanese → JP / Korean → KR),
but four AI-consumable surfaces still said "all CJK → Noto Sans SC":
- add_body_text_v0 tool description: "Chinese/Japanese/Korean content
gets fontFamily=Noto Sans SC + lineHeight=1.6"
- add_heading_v0 tool description: listed only Latin presets; no
mention of CJK handling at all (silently stale)
- elements.md skill usage example: "// auto Noto Sans SC + 1.6" for a
Chinese string but no example showing JP/KR getting their own faces
- elements.md decision tree line 14-15: just said "CJK detection
(correct fontFamily)" — generic enough that drift was invisible
Fix:
- body_text description rewritten to enumerate the 4-way script
dispatch explicitly (SC/JP/KR/Inter), note JP precedence for
hiragana/katakana + kanji mixes, and document the letterSpacing=0
invariant
- heading description expanded to list both Latin and CJK preset
tables plus the script-specific font mapping with the explicit
"NEVER use SC for JP/KR" admonition lifted from text-rules.md
- elements.md gets 2 new usage examples (JP, KR) showing the
different Noto face selection so the AI reading the skill sees
non-SC CJK fonts in practice, not just in prose
- decision-tree lines for heading/body now name SC/JP/KR explicitly
- d0-parity-spike snapshot regenerated (get_design_prompt enum + tool
definitions have changed descriptions)
169/169 pen-mcp tests pass. format + tsc green. Bundle rebuilt.
Codex stop-hook #14: previous fix (8ca2cb9) mapped ALL CJK content to
'Noto Sans SC'. text-rules.md spec (and memory
project_pencil_optimization) requires script-specific fonts:
Chinese → Noto Sans SC
Japanese → Noto Sans JP
Korean → Noto Sans KR
Using SC for JP/KR has reasonable Unicode coverage but violates the
explicit font contract. Each Noto face ships with script-native
punctuation + glyph variants that the dedicated face renders
correctly.
Fix: extract script detection + font mapping into shared helpers in
element-tool-helpers.ts so add_heading_v0 and add_body_text_v0 stay
in sync:
detectCjkScript(s): 'chinese' | 'japanese' | 'korean' | null
Detection order:
1. Hiragana (U+3040-309F) / Katakana (U+30A0-30FF) → Japanese
(these scripts are UNIQUE to Japanese even when mixed with
Han ideographs — a heading like "今日は" is Japanese despite
having kanji, because hiragana "は" disambiguates)
2. Hangul Syllables (U+AC00-D7AF) → Korean
3. CJK Unified Ideographs / Symbols → Chinese (Simplified default)
4. Otherwise null
cjkFontFamily(script): 'Noto Sans SC'|'Noto Sans JP'|'Noto Sans KR'|undefined
Apply to both add_heading_v0 (CJK_BASE preset table + per-script
fontFamily injection) and add_body_text_v0 (fontFamily = cjkFont ??
'Inter'; letterSpacing = 0 only when CJK).
Tests: 3 new per-script font assertions in each tool's test file
(Japanese → Noto Sans JP, Korean → Noto Sans KR, plus the kanji
+hiragana disambiguation edge case). 169/169 pen-mcp suite pass.
format + tsc green. Bundle rebuilt.
Codex stop-hook #13: heading presets hardcoded Latin typography
(lineHeight 1.0/1.1/1.2/1.25, display letterSpacing -0.5) and violated
three documented CJK rules when called with Chinese/Japanese/Korean
content:
1. memory project_pencil_optimization: "CJK headings 1.3-1.4 (NOT
1.1-1.2 like Latin)"
2. text-rules.md: "CJK letterSpacing: 0, NEVER negative. Negative
letterSpacing causes CJK character overlap."
3. text-rules.md: "CJK font selection: heading=Noto Sans SC /
Noto Sans JP / Noto Sans KR. NEVER Space Grotesk or Manrope —
they have no CJK glyphs."
Fix: same auto-CJK detection used by add_body_text_v0 (regex scan for
\u3000-\u303f \u3040-\u309f \u30a0-\u30ff \u4e00-\u9fff \uac00-\ud7af).
CJK content selects a separate preset table:
display 48/700/1.3 Noto Sans SC (was 48/700/1.0/-0.5 Latin)
h1 32/700/1.3 Noto Sans SC (was 32/700/1.1)
h2 24/600/1.35 Noto Sans SC (was 24/600/1.2)
h3 20/600/1.4 Noto Sans SC (was 20/600/1.25)
All CJK presets drop letterSpacing entirely (never negative, never
overridden from theme). Latin presets unchanged.
Tests: 6 new CJK cases (zh/jp/ko/mixed/Latin-still-works +
per-level lineHeight verification). Loop test uses unique filenames
per iteration to sidestep openDocument cache reuse.
13/13 heading tests pass. 160/160 pen-mcp overall. format + tsc green.
Bundle rebuilt.
Three text-primitive tools, each encoding a documented Pencil-demo or
memory-noted non-Claude failure mode.
- add_text_button_v0: padding-based button (padding=[12,20],
cornerRadius=8, fit_content × 2, horizontal, centered). Pencil demo
pattern — height auto-derives from padding, no explicit height.
Optional leading icon at 16px. Narrow: single md preset (no size
enum). Label + optional icon = 1-2 children.
- add_heading_v0: typographic heading with 4-level preset enum
(display / h1 / h2 / h3; default h2). Each preset fixes fontSize
/ fontWeight / lineHeight / optional letterSpacing per memory
data (display=48/700/1.0/-0.5, h1=32/700/1.1, h2=24/600/1.2,
h3=20/600/1.25). Single text node output — the enum only changes
typography, not structure ("应拆尽拆" compliant). Prevents the
"default 1.5 lineHeight makes multi-word headings stack tight"
failure mode.
- add_body_text_v0: body text with AUTO CJK detection via regex
scan (/[\u3000-\u303f\u3040-\u309f\u30a0-\u30ff\u4e00-\u9fff
\uac00-\ud7af]/). CJK → fontFamily='Noto Sans SC' + lineHeight 1.6
+ letterSpacing 0 (memory: NEVER Space Grotesk/Manrope for CJK).
Latin → Inter + 1.5 + no letterSpacing override. Always sets
width=fill_container + textGrowth=fixed-width (intended for
vertical-layout parents per the documented rule). Mixed
content triggers CJK.
Tests: 18 new unit (4 + 7 + 7). CJK detection tested across Chinese /
Japanese / Korean / mixed. 160/160 pen-mcp suite green.
Contract test's ELEMENT_TOOL_NAMES updated 12 → 15. elements.md skill
gets a new "Text + button primitives" category in the decision tree
(items 13-15) + PREFER phrases + usage examples; regression test
dynamically derives expected names from registry so no stale-drift.
MCP live smoke (/tmp/claude/mcp-test-phase-1c-tools.ts): ListTools =
55, element tools = 15; display heading gets correct typography
preset; Latin body gets Inter+1.5; CJK body gets Noto Sans SC+1.6+0
letterSpacing.
format + tsc green. Bundle rebuilt.
Codex stop-hook #12: previous fix (e05ca79) introduced its own rot —
- Test "names EVERY production element tool" asserted
elementTools.length >= 12. A hardcoded count is exactly what the
whole regression suite is trying to prevent. If someone removes a
tool, the count drops to 11 and the test keeps passing (because
>=12 is a floor, not a target).
- get_design_prompt description said "12 tools covering rows[..]/
containers[..]/atoms[..]". "12 tools" is stale the moment we add
or remove a tool. The category names (rows/containers/atoms) are
also hardcoded — if we ship a new category, the description is
misleading.
Fix:
- Test: replace `>=12` with `>0`. Assertion is now: "registry has
at least one element tool AND every element tool is mentioned in
the elements skill." No hardcoded count.
- Description: replace the "12 tools covering [..]" enumeration
with "N-tool element-tool family reference — decision tree,
PREFER/FALLBACK rules, composition pattern; the section itself
enumerates the current tools." Specific names + counts live in
elements.md skill where the regression test keeps them in sync
with the registry.
- Add regression test asserting the description does NOT match
/\d+\s+tools?\s+(covering|in|across)/i — catches any future
reintroduction of a hardcoded count.
d0 snapshot updated to reflect the new description.
142/142 pen-mcp tests pass. format + tsc green. Bundle rebuilt.
Codex stop-hook #11: the last 3 tools (divider / badge / avatar) were
registered in MCP + tested in unit tests, but the AI-facing integration
layer was stale:
- elements.md skill still listed only 9 tools (missing divider / badge
/ avatar from decision tree, PREFER phrases, usage examples, and
role registry)
- get_design_prompt tool description still enumerated the ORIGINAL 5
element tools ("add_card_row_v0 / add_metric_row_v0 / …")
- Both are read directly by external MCP clients (Claude Code / Codex
/ Gemini CLI / Cursor) to decide which tool to pick — stale content
means the AI never learns the newer tools exist.
Fix:
- elements.md: add divider / badge / avatar to decision tree (new
"Atoms" category after rows + containers), PREFER phrases list,
usage example block, role guarantee list. Frontmatter description
updated to "12 tools" with category breakdown. Budget bumped
1500 → 1800 tokens to accommodate the 3 new sections.
- design-routes.ts: get_design_prompt description rewritten to
describe the element-tool family as "12 tools covering rows
[card/metric/nav_chip/stat_grid], containers [bottom_nav/
top_nav_bar/section_header/icon_button/activity_ring], atoms
[divider/badge/avatar]" — generic categorization plus named
examples, not a stale list that rots on every addition.
- d0-parity-spike snapshot updated (get_design_prompt definition
changed — intentional).
Regression tests to prevent future drift:
- "names EVERY production element tool": derives the expected list
dynamically from DESIGN_TOOL_DEFINITIONS, so any future element
tool added without updating elements.md trips the test. Was
previously hard-coded to 5 tool names.
- "description has no stale element-tool references": any
`add_*_v0` name appearing in get_design_prompt's description
must correspond to an actually-registered tool.
142/142 pen-mcp tests pass (was 141; +1 stale-guard for description).
format + tsc green. Bundle rebuilt.
Three low-risk single-/double-node tools completing the first
"应拆尽拆" batch. ListTools now 52 (40 baseline + 12 element).
- add_divider_v0: hairline rectangle (horizontal default: fill_container
width, height=1; vertical swaps axes). Memory-documented pattern
(Pencil reverse engineering): "Dividers: rectangle(h=1,
fill_container) or directional stroke". Ships colorless.
- add_badge_v0: short pill / tag (cornerRadius=999, padding=[4,10],
font 11/600). Forces the documented constraint (overflow.md):
CJK ≤8 chars / Latin ≤16 chars — longer labels should not be badges.
- add_avatar_v0: circular avatar with optional centered initial. Same
frame+cornerRadius=size/2+flex-centering pattern as activity_ring —
NEVER the ellipse+sibling text anti-pattern (layout.md §RING /
CIRCLE WITH CENTER CONTENT). Initial font auto-scales (size × 0.4,
floored at 12 for tiny avatars).
Tests: 16 new unit (6 divider + 4 badge + 6 avatar). contract test
updated from 9 → 12 tool names. 141/141 pen-mcp suite passes.
MCP live smoke (/tmp/claude/mcp-test-phase-1b-tools.ts): ListTools
= 52, all 3 new tools callable with expected node counts
(divider=1, badge=2, avatar-with-initial=2), avatar cornerRadius
verified as size/2.
format + tsc green. Bundle rebuilt.
Codex stop-hook #10: previous fix (72087f8) used
width=fill_container + textGrowth=fixed-width on the title text but
placed it DIRECTLY inside the horizontal section-header frame. Per
packages/pen-ai-skills/skills/phases/generation/overflow.md:
"Text in VERTICAL layout: width=fill_container + textGrowth=fixed-width.
In horizontal: width=fit_content." The layout engine only measures
wrap-grown height when text follows the vertical-layout rule. In our
horizontal header, a wrapped title rendered visually but did not
propagate its extra height to header.height=fit_content, so the
header stayed at single-line height and the NEXT sibling in the
parent vertical layout overlapped the wrapped lines.
Fix: introduce a fill_container + vertical + fit_content title
container that wraps the text node. The text now follows the
documented rule (fill_container + fixed-width in vertical parent),
wrap height is measured correctly, and header.height=fit_content
grows to match. Following content gets pushed down by the wrapped
height as intended.
Regression test: add a long-title case that asserts
- header.height === 'fit_content'
- title-container.layout === 'vertical'
- title-text.width === 'fill_container'
- title-text.textGrowth === 'fixed-width'
- header has NO space_between AND NO fixed height
Existing id-count test updated: 5 → 6 nodes (added title container).
7/7 section-header tests pass; 125/125 pen-mcp suite green. format +
tsc clean. Bundle rebuilt.
Codex stop-hook #9: previous implementation used
justifyContent:space_between with both children at natural
(fit_content) width. Long titles push past the action's starting
position and overlap it — flexbox space_between distributes REMAINING
space but does not clip items that collectively exceed the container
width.
Fix: title now takes width:fill_container + textGrowth:fixed-width so
it consumes all remaining horizontal space and wraps vertically when
too long (the header's height:fit_content accommodates wrapping).
Action stays width:fit_content on the right. Header adds gap:16 for
guaranteed visual breathing room. justifyContent:space_between is
intentionally removed — fill_container on one sibling makes it
redundant and the removal is what prevents the overlap.
Regression test: seed a header with a deliberately long title + short
action and assert title has fill_container/fixed-width, action has
fit_content, and header has NO justifyContent. 7/7 section-header
tests pass. 124/124 pen-mcp suite green.
Step 7 continues: 5 → 9 narrow element tools, each solving one
documented anti-pattern from pen-ai-skills prompt knowledge.
- add_stat_grid_v0: NON-scrolling 2-5 metric grid. Each cell uses
width=fill_container so the renderer auto-distributes space.
Directly solves the documented activity-rings overflow bug in
packages/pen-ai-skills/skills/phases/generation/layout.md
(three fixed 100px rings in a 279px inner card silently clip the
third; with fill_container the third fits by construction).
Different from add_metric_row_v0 which is a scrolling wrapper
with fixed-px items.
- add_section_header_v0: heading + optional trailing action ("See
all" / "View more"). Forces horizontal space_between alignItems=
center so action stays flush-right. Common dashboard pattern that
non-Claude models frequently vertical-stack instead.
- add_top_nav_bar_v0: mobile app bar. Leading icon (back/menu) +
centered title + trailing icon (search/more). Dual of
add_bottom_nav_v0. Empty slots become 44×44 spacers so the title
stays visually centered even with asymmetric icons.
- add_icon_button_v0: 44×44 icon-only button with flex centering.
Explicitly NOT layout=none (the documented anti-pattern in
memory: layout=none + nested absolute-positioned children renders
unreliably under Skia). Forces layout=horizontal + justifyContent
/alignItems=center.
All four follow the established element-tool pattern:
- Sugar route via insertElementTree (parent_id pre-check, DSL
escape pre-check, snapshot rollback, post-check parent-location
verification)
- assignIdsRecursively on the built subtree
- No union types in schema; narrow required params; optional
sizing/styling via follow-up batch_design U-op
Tests: 22 new unit (5+6+6+5 per tool) + element-tools-contract
updated to assert all 9 tools satisfy §4 invariants. All 124
pen-mcp tests pass.
skill file `elements.md` expanded from 5 → 9 tools (decision tree
updated, PREFER phrases mapped per tool, usage examples added,
role list extended). Registry regenerated from 44 → 44 skills
(same count, elements.md edit in place).
MCP live e2e smoke via StdioClientTransport confirms ListTools now
returns 49 tools (40 baseline + 9 element), all 4 new tools
callable, structural invariants verified on live output
(stat-grid cell.width=fill_container; icon-button layout \!= none;
3-slot top-nav structure; section-header action group).
format + tsc green. Bundle rebuilt.
Codex stop-hook #8: elements.md had `trigger: null` which makes
resolveSkills('generation', ...) unconditionally load its 1500-token
N-tool reference into every generation prompt — including the
embedded orchestrator in apps/web/src/services/ai that emits
single-shot JSON and CANNOT call MCP tools. The content was dead
weight there (orchestrator-sub-agent.ts:333 / ai-prompts.ts:132 both
build generation prompt via resolveSkills without any tool-use path).
Fix: trigger: { flags: [hasMcpTools] }. Skill only auto-loads when
caller explicitly declares MCP tools are available. No existing caller
sets this flag, so the embedded orchestrator prompt is now clean
again.
External MCP clients (Claude Code / Codex / Gemini CLI / Cursor) still
get the content via get_design_prompt(section='elements'), which uses
getSkillByName direct lookup and bypasses resolveSkills' trigger
filter. That contract is preserved.
Added explanatory HTML comment in the skill header so future editors
understand the gating + opt-in rule.
Tests: 4 new cases in design-prompt-elements.test.ts verify:
- getSkillByName returns skill regardless of flags (direct lookup)
- resolveSkills('generation') WITHOUT flag → elements excluded
- resolveSkills('generation') WITH {hasMcpTools:true} → elements included
- buildDesignPrompt('elements') works regardless of flag (bypass path)
14/14 design-prompt-elements tests pass; 186/186 across pen-mcp +
pen-ai-skills. format + tsc green. Bundle rebuilt.
Codex stop-hook #7: previous rollback (a0ace58) called
saveDocument(fp, restoredDoc) to trigger the live-canvas re-sync, but
saveDocument runs JSON.stringify(doc) with no indent and writes the
result to disk. That overwrites the snapshot we just restored via
writeFile — losing the user's original formatting (indentation, key
order, number representation, trailing newlines) and replacing it
with pen-mcp's canonical serialization.
Fix:
- Export pushLiveDocument from document-manager so callers can trigger
a live-canvas push WITHOUT re-writing disk
- Rollback path: writeFile(snapshot) + invalidateCache + openDocument +
pushLiveDocument. The file is restored byte-exact; pushLiveDocument
reads the parsed doc from cache and pushes it, no disk rewrite
Semantically equivalent to before for the live-canvas side (same push
with same restored doc) but now preserves the snapshot bytes on disk
exactly. Important for users who hand-edit .op files or whose original
document was produced by a different serializer (pen-core, Electron
save-as, etc.).
88/88 pen-mcp tests pass (existing "file bytes unchanged" assertions
already verify this; they simply now rely on a path that doesn't
re-serialize). format + tsc green.
Codex stop-hook #6: saveDocument is DUAL-WRITE for file-backed paths
(document-manager.ts:411) — it writes to disk AND calls
pushLiveDocument. handleBatchDesign uses saveDocument so a bad insert
gets pushed to the live canvas before our post-check runs. Prior
rollback (6dfa88b) only restored the file via writeFile, leaving the
live-canvas renderer showing the bad insert until the next refresh.
Fix: after writeFile + invalidateCache, call saveDocument(fp, restoredDoc)
in the rollback path. saveDocument will re-write the file (no-op, same
content) AND call pushLiveDocument again with the restored doc,
bringing the live canvas back in sync with disk. If the live-sync step
fails, re-throw with a diagnostic noting the live canvas may be stale
but disk is authoritative.
Wrap the re-sync in try/catch so a transient live-sync failure doesn't
swallow the original insert-failure reason. Disk is the source of
truth and is already restored when we hit this path.
If no sync URL is configured (common in unit-test contexts),
pushLiveDocument is a no-op — so this is safe in all scenarios.
Live-canvas-only paths (filePath='live://canvas') remain
non-rollback-able because pushLiveDocument is a one-way push without
a history mechanism; the pre-check is the primary defense there.
88/88 pen-mcp tests pass (rollback behavior unchanged at the
observable-test level since our tests don't configure a live sync
URL; the re-sync is correct by construction given saveDocument's
published semantics). format + tsc green.
Codex stop-hook #5: previous post-insert check (5311313) throws on
wrong-parent detection, but by the time we detect it batch_design has
ALREADY written the bad insert to disk / pushed to live canvas. The
throw is a post-facto notification, not a guarantee.
Three-layer defense:
1. **Pre-check (NEW, prevents disk write)**: simulateDslParentResolve
mirrors batch-design.ts:resolveRef (`raw.replace(/^"|"$/g, '')`, no
JSON-unescape). If JSON.stringify(parent_id) → quote-strip ≠
parent_id, the DSL round-trip is lossy → throw before ever calling
handleBatchDesign. Covers ids containing `"` or `\` (the cases
Codex #3 and #4 flagged).
2. **Snapshot + rollback (NEW, for file-backed docs)**: before calling
handleBatchDesign we readFile the target into a snapshot. If any
post-check (errors / missing insert / wrong parent) trips, we
writeFile the snapshot back + invalidateCache, then throw.
File-backed docs are now atomic at the element-tool boundary.
3. **Post-check (kept as defense-in-depth)**: node-in-tree +
parent-location verification. With pre-check in place these should
be unreachable, but they guard against future DSL-parser changes
or silent-failure modes we haven't anticipated.
Known limitation: live canvas cannot be atomically rolled back because
pushLiveDocument is a one-way push. The rollback path skips live with
a clear error message noting the insert may still be visible until
next refresh. Pre-check is the primary defense for live; post-check
rollback-best-effort applies only to filePath'd calls.
Tests updated: existing "weird quoted parent_id" and
"A\"B decoy collision" regression tests now hit pre-check (cleaner
error, same invariant: throw + file unchanged). 88/88 pen-mcp pass.
format + tsc green.
Codex stop-hook #4: the prior post-insert check (e797584) verified that
the inserted nodeId is findable anywhere in the tree, but did not
verify it landed under the REQUESTED parent. Silent wrong-parent
inserts are still possible if batch_design's resolveRef quote-strip
produces a literal that matches a DIFFERENT node than the one
ensureParentExists validated.
Concrete example: doc has nodes with ids `A"B` (3 chars: A, ", B) and
`A\"B` (4 chars: A, \, ", B). User passes parent_id='A"B'.
ensureParentExists does a raw-string `===` match and finds the first
node. insertElementTree does JSON.stringify → `"A\"B"` in DSL source.
batch_design's parseInsertArgs → resolveRef → `/^"|"$/g` quote-strip
→ literal `A\"B` (4 chars). insertNodeInTree matches the DECOY and
inserts under it. Prior post-check: node is in tree → passes. Actual:
wrong parent.
Fix: when args.parent_id is provided, post-check also walks
findParentInTree(postChildren, insertedId) and confirms the resolved
parent id === the requested parent_id. Mismatch → throw with clear
diagnostic.
Regression test: element-tools-contract.test.ts adds the A"B / A\"B
decoy scenario and asserts the tool throws (not silent success).
88/88 pen-mcp tests pass (+1 new wrong-parent guard). format + tsc
green. Bundle rebuilt.
Codex stop-hook #3: JSON.stringify(parent_id) in insertElementTree
doesn't prevent silent no-op. batch_design's resolveRef
(batch-design.ts:442-445) only does `/^"|"$/g` quote-stripping — it does
NOT JSON-unescape — so a parent_id containing `"` or `\` round-trips
as a different literal than what's actually stored in the document.
insertNodeInTree finds no match, silently returns the original tree,
and batch_design still reports success (pushes to results, bumps
nodeCount, errors stays empty).
ensureParentExists catches the "parent genuinely doesn't exist" case
via pre-check using the raw string for direct equality. It does NOT
catch the escape-mismatch case (which passes pre-check with the raw
string but fails in the DSL path).
Fix: insertElementTree now re-reads the document after handleBatchDesign
and confirms the inserted nodeId is actually findable via
findNodeInTree. If not present → throw with diagnostic info about
parent_id and pageId. This is the single source of truth for "did the
insert land?" regardless of what DSL parser subtleties occur
downstream.
Regression test: element-tools-contract.test.ts adds a case that seeds
a document with parent id containing a literal `"` (manually crafted —
nanoid never produces such ids, but imported/migrated docs might),
then calls handleAddBottomNavV0 with that parent_id. Previously the
tool would return success with an orphan record; now it throws and the
on-disk document is verified unchanged.
87/87 pen-mcp tests pass. format + tsc green. Bundle rebuilt.
Four cleanup items from the second independent Codex review, all
scoped to element tools + their contract tests.
P0.1 insertElementTree safety boundary (element-tool-helpers.ts:112-124):
- parent_id now JSON.stringify'd so ids containing quotes/backslashes
cannot escape DSL quoting and inject additional batch_design ops
- per-item batch_design errors are re-thrown with a concise summary
instead of silently surfaced via result.errors (N-tool single-insert
semantics require loud failure; callers can't distinguish "inserted
with errors" from "didn't insert at all")
P0.2 add_activity_ring_v0 narrowing (add-activity-ring-v0.ts):
- strip ring_color / text_size / text_weight params (were 6 business
params → now 3). Typography/color hardcoded (#000000, 16, 700) per
spec D6 Style-Guide-orthogonal invariant. Callers override via a
follow-up batch_design U-op, same as card/metric/nav_chip rows
- route schema + description + test updated accordingly
P0.3 test hardening:
- metric_row + nav_chip_row gain "every node has a unique non-empty id"
regression test matching the other 3 element tools (id coverage parity)
- all 5 element tools gain "throws on bogus parent_id AND leaves file
untouched" side-effect assertion. Previous tests only checked the
throw (mirroring helper impl); now tests verify the actual invariant
that matters to the user — the document doesn't get partially
mutated when validation rejects
P0.4 schemaVersion input property (design-routes.ts + new
element-tools-contract.test.ts):
- every element tool (5 production + 1 spike) now declares schemaVersion
as an input property (enum: ['1.0'], optional) per spec §4.2. Was
previously only mentioned in description strings, which clients
couldn't introspect
- new element-tools-contract.test.ts cross-validates §4.1 (additive),
§4.2 (schemaVersion property), §4.4 (filePath/parent_id accepted),
and "应拆尽拆" invariant (no children_type/variant unions leaked
back in)
86/86 pen-mcp tests pass (+6 new contract + 2 new id coverage + 5 new
side-effect). format + tsc green. Bundle rebuilt.
P1 items still open (server-side JSON Schema enforcement, role
registration for nav-chip-active/nav-item-active, live-canvas race
condition in ensureParentExists, saveDocument silent-failure path)
tracked separately.
Codex stop-hook review: the stale AI prompt in
packages/pen-ai-skills/skills/phases/generation/overflow.md still told
clients add_nav_chip_row_v0 items require both label + icon, even
though the tool now accepts label-only chips (fixed in 451474f).
Update the §HORIZONTAL SCROLL ROWS preferred-path bullet for
add_nav_chip_row_v0 to mark icon as optional and call out label-only
support ("All / Videos / Photos" text-only filter tags).
Codex stop-hook review: splitting add_scroll_row_v0 into narrow tools
regressed label-only chip rows. The original children_type='nav_item'
variant accepted { title } with no icon (text-only filter tags like
"All" / "Videos" / "Photos"), but the new add_nav_chip_row_v0 made icon
required, breaking that use case.
- tool handler: if item.icon absent, skip the icon_font child and emit
just the text label. Mirrors the original buildNavItem behavior from
packages/pen-mcp/src/tools/add-scroll-row-v0.ts (removed).
- route schema: items[].required changed from ['label', 'icon'] →
['label']; description notes label-only chips are supported.
- test: new "builds label-only chips when icon is omitted" case covering
3 plain-label items (All / Videos / Photos) asserting each chip has
exactly 1 text child, no icon_font. Existing icon-present test still
covers the with-icon path.
78/78 pen-mcp suite passes (was 77 + 1 new). format + tsc green. Bundle
rebuilt.
Per "应拆尽拆" guidance: remove children_type union from scroll-row
family. Each narrow tool now has ≤5 simple params, no union types, and
a single output pattern — so the LLM never has to decide between
variants inside a tool.
Replaced `add_scroll_row_v0({children_type, items})` with:
- add_card_row_v0({items: {title, subtitle?, icon?}}) — 140×160 cards
- add_metric_row_v0({items: {label, value, icon?}}) — 120×100 tiles,
value=28/700
- add_nav_chip_row_v0({items: {label, icon, active?}}) — 72 chips with
active state
Shared wrapper + id assignment + parent check + DSL insertion logic
extracted into element-tool-helpers.ts:
- buildScrollWrapper() — the fill_container+clipContent outer + inner
fit_content row (identical across all 3 row tools)
- assignIdsRecursively() — moved from duplicated copies in each tool
- insertElementTree() — centralizes the batch_design DSL shape
Also refactored add_bottom_nav_v0 + add_activity_ring_v0 to use the
shared helpers (DRY).
- overflow.md: §HORIZONTAL SCROLL ROWS preferred-path section now
points at the 3 narrow tools with a decision rule (what's in each
item → which tool) instead of add_scroll_row_v0+children_type param
- design-routes.ts: unregister add_scroll_row_v0, register 3 new
tools each with precise inputSchema (label/value/title fields match
the semantic role)
- Delete obsolete add-scroll-row-v0.ts + test + snapshot
Tests: 77/77 pass (was 86 before removing scroll-row tests). Each new
tool has 3-4 tests covering registration + structure + id coverage +
parent_id validation. Live MCP smoke confirmed 45 tools in ListTools
(40 baseline + 5 element) with all narrow tools functional.
format + tsc green.
Codex stop-hook review: add_bottom_nav_v0 (and siblings) advertised a
parent_id contract that could silently no-op. pen-core's
insertNodeInTree returns the original tree unchanged when parentId
doesn't match any node (tree-utils.ts:200-234), so batch_design's
downstream call produces a success-looking response {results, nodeCount}
with an orphaned node that never lands on disk.
- tools/element-tool-helpers.ts: new ensureParentExists() helper —
loads the target doc via openDocument + resolveDocPath, checks
getDocChildren(pageId) with findNodeInTree, throws a descriptive
Error listing parent_id and pageId if missing
- Apply to all 3 element tools (add_scroll_row_v0 / add_bottom_nav_v0 /
add_activity_ring_v0) at the top of each handler, before DSL
construction. Null parent_id short-circuits (root insertion is OK)
- routes/design-routes.ts: fix misleading "Page id" description on
add_bottom_nav_v0.parent_id → now matches siblings ("Target parent
node id (must exist...)")
- Tests: 4 new (throws on bogus parent_id for each of 3 tools, +
positive "inserts under valid parent" for add_scroll_row_v0). 86/86
pen-mcp suite pass
Bundle rebuilt. format + tsc green.
Step C of N-tool element design: expand MVP from 1 to 3 tools, each
targeting a documented non-Claude failure mode with file:line evidence.
- add_bottom_nav_v0: bottom tab bar. Solves layout.md §NO FIXED-POSITION
LAYOUT anti-pattern (empty spacer siblings after nav; bottom-nav is
inline flow, not position:fixed). Schema: items[]+height; output:
frame(role=bottom-tab-bar, width=fill_container, layout=horizontal,
justifyContent=space_around) with per-tab icon+label and
role=nav-item-active for current tab.
- add_activity_ring_v0: Apple-style progress ring with centered text.
Solves layout.md §RING / CIRCLE WITH CENTER CONTENT anti-pattern
(ellipse+sibling text stacks wrong; layout=none+absolute renders
unreliably). Schema: size/thickness/ring_color/center_text +
text_size/text_weight; output: frame(cornerRadius=size/2, stroke,
fill=[], layout=horizontal, alignItems=center, justifyContent=center)
with single text child.
Both tools follow D1=A Sugar route (internal handleBatchDesign call)
and assignIdsRecursively pattern from 9ba64b7.
Tests: 8 new (4 nav + 4 ring), 82/82 pen-mcp suite pass. format + tsc
green. Live MCP smoke test via StdioClientTransport confirmed ListTools
now 43 tools (40 baseline + 3 element), spike still hidden, both new
tools produce correct structure + unique node ids.
Step 3 (prompt path, MVP scope). Update §HORIZONTAL SCROLL ROWS to
split into two paths:
- Preferred (MCP tool): call add_scroll_row_v0 directly. External MCP
clients (Claude Code / Codex / Gemini CLI / Cursor) see the tool in
ListTools and the prompt now explicitly teaches WHEN to pick it
over hand-building JSON
- Fallback (hand-built JSON): existing structure teaching, unchanged,
used when MCP tool isn't available (embedded AI flow, JSON-only)
Embedded orchestrator integration (apps/web/src/services/ai) is out of
scope for this MVP step — it requires architectural work to flip from
one-shot JSON emission to MCP tool_use flow. External clients already
get full benefit from tool registration + this prompt update.
84/84 pen-ai-skills tests pass. format:check + tsc green.
Codex stop-hook review: child nodes were saved without ids. batch_design
only assigns an id to the top-level inserted node — nested children
(inner row, cards, texts, icon_fonts) come through the DSL unchanged,
which breaks any later tree operation that resolves by id (update /
delete / move / post-processing / spatial index lookup).
Fix: assignIdsRecursively walks the wrapper subtree before serializing
to DSL and stamps every node with generateId(). batch_design's own
overwrite of the top-level id is harmless.
Test: 4 new id-coverage tests (one per children_type + uniqueness
across a 3-card tree). Total 17 tests in add-scroll-row-v0, 74 in
pen-mcp suite. format + tsc green.
Step 2 of N-tool element design (spec v0 §7.1 pinned to this impl).
First production element-level tool, replacing the batch_design generic DSL
for the specific pattern LLMs most commonly get wrong: horizontal scroll
rows of cards / metric tiles / nav items.
Tool fixes the structure at schema level:
- Outer wrapper: fill_container + clipContent=true + vertical layout
- Inner row: fit_content + horizontal + gap + padding=[0,20]
- Children: fixed numeric width per variant (card=140 / metric=120 / nav=72)
Exactly matches packages/pen-ai-skills/skills/phases/generation/overflow.md
§HORIZONTAL SCROLL ROWS. Sugar over handleBatchDesign (D1=A route).
- tools/add-scroll-row-v0.ts: 230-line builder with 3 children_type presets
- routes/design-routes.ts: register in production DESIGN_TOOL_DEFINITIONS
- __tests__/add-scroll-row-v0.test.ts: 13 tests covering wrapper invariants,
per-type structure, icon/subtitle optionality, overrides, persistence,
golden snapshot
- d0-parity-spike.test.ts: rewrite "names exactly" assertion to "pre-D0
tools still present" so new production tools don't break baseline; scope
snapshot to pre-D0 5 tools only
70/70 pen-mcp tests pass. format:check + tsc --noEmit both green.
Codex stop-hook review: spike-only tool should not be exposed to
external MCP clients (Claude Code / Codex / Gemini CLI) by default.
- routes/design-routes.ts: move add_section_v0 out of DESIGN_TOOL_DEFINITIONS
into separate D0_SPIKE_TOOL_DEFINITIONS / D0_SPIKE_TOOL_NAMES /
handleD0SpikeToolCall exports, matching DEBUG_TOOL_* pattern
- server.ts: conditionally merge D0_SPIKE_TOOL_DEFINITIONS when
OPENPENCIL_D0_SPIKE=1, mirroring OPENPENCIL_DEBUG_TOOLS=1 gating
- tests: assert default DESIGN_TOOL_DEFINITIONS is unchanged (spike
not leaked) and D0_SPIKE_TOOL_DEFINITIONS contains the spike tool
(only reachable when flag set). Snapshot regenerated.
57/57 pen-mcp tests pass. format:check + tsc --noEmit + vitest all green.
Picks up c5d9e2a in the submodule: single-space placeholder text block
when an assistant turn's content is tool_use-only, fixing intermittent
HTTP 400s from MiniMax-M2 and similar reasoning-first models.
The previous commit renamed isBadgeOverlayNode → isOverlayNode without
a compat export, which would break external consumers of the published
@zseven-w/pen-core package on upgrade. Add a deprecated alias so the
old import name keeps resolving. JSDoc flags the behavior change —
the alias no longer matches role:'badge'|'pill'|'tag', since those are
inline-component roles and must flow in auto-layout.
isBadgeOverlayNode matched role:'badge'|'pill'|'tag' and pulled those
children out of their parent's auto-layout, rendering them at (0,0) of
the parent and stacking them on top of siblings. But in this repo
badge/pill/tag are inline-component roles (see role-resolver NAME_EXACT_MAP
and strip-redundant-section-fills PROTECTED_ROLES) — they're meant to
flow in layout like any other child.
Rename to isOverlayNode and narrow to role:'overlay'. Add matching
"Layout-escape roles" guidance in role-definitions.md so generation
prompts can reach the new opt-in. Inline roles now flow correctly;
true floating decorations (notification dots, corner ribbons) still
have a dedicated marker.
snapshot_layout now emits an `overlaps` array listing sibling pairs whose
rendered bounds intersect, so text-only agents can diagnose stacking bugs
without a screenshot. When the shared parent has `layout: "none"` the
reason string points at the real cause (absolute x/y stacking) instead
of letting models hedge with height/padding tweaks. Handler prompt adds
a matching diagnosis workflow so agents fix the parent layout rather
than resizing the overlapping children.
