Replace the chat panel's agent-cycling chip with a real model
picker: the chip shows the selected model's provider brand logo +
display name, and clicking it opens an upward dropdown listing
chat.available_models grouped by provider with a selected check
(mirrors the TS ai-chat-model-selector). Selecting a model also
re-syncs chat_selected_agent to its provider so the chat transport
targets the matching CLI. Outside clicks dismiss the picker.
The chat panel never had a model list — shell-core is transport-free
and cannot query a CLI. Add desktop-side cross-platform discovery:
Codex's models_cache.json (real, file-based), `opencode models`
(real, subprocess), and the documented CLI model names for Claude /
Gemini / Copilot which expose no listing interface — each gated on
the CLI actually being installed (PATH walk with platform exe
extensions). Discovery runs on a worker thread and drains into
chat.available_models. shell-core keeps only the ModelEntry type.
Move the per-frame paint pass to frame.rs to keep main.rs under cap.
Node.effects already round-trips through persistence and canonical
import, but the canvas never rendered it. Add a fill_drop_shadow
RenderBackend primitive (default impl approximates with a translucent
fill; native + web will override with skia MaskFilter blur) and paint
each DropShadow behind Frame / Rect / Ellipse fills, offset + blur
scaled by viewport zoom.
Canonical .op shadow colours use functional rgba()/rgb() notation, which
the hex-only parser rejected — valid shadows were imported as opaque
black, then dropped entirely after the prior fix. Add parse_css_color
covering both hex and functional forms. Move the effect-conversion
helpers into persistence_effects.rs so pen_doc_adapter.rs stays under
the 800-line cap.
`shadows_from_canonical` fell back to `[0,0,0,1.0]` (opaque black)
when `parse_hex` rejected a shadow colour — so an `rgba()`,
named, or short-hex colour imported as a wrong solid-black
shadow (codex stop-gate). Switch the fallback to `filter_map`
`None`: an unparseable colour drops that shadow entirely, which
is honest (no shadow) rather than misleading (wrong shadow).
Matches `first_solid_color`'s existing `parse_hex(...)?` discipline.
`pen_doc_adapter::node_to_payload` dropped every `PenEffect` —
a `.op` authored with shadows lost them on import (codex stop-gate;
the inverse of the save-path effects round-trip just added).
`shadows_from_canonical` reads each variant's effects (Frame /
Group / Rectangle carry them on `container`, leaf shapes directly;
IconFont / Ref have none) and maps `PenEffect::Shadow` →
`ShadowPayload`. Blur / background-blur are skipped — the shell's
`Effect` model is drop-shadow-only today.
First slice of the Effects gap — the property panel's 效果 section
was a header-only stub with no data behind it.
- `Effect` / `DropShadow` types in `document.rs`; `Node.effects:
Vec<Effect>` (offset_x/y + blur + color, doc-px). Both `Node`
builders + `deep_clone_with_new_ids` carry it.
- `persistence_effects.rs` — `ShadowPayload` + `effects_to_payload`
/ `effects_from_payload`; `NodePayload.effects` is `#[serde(default)]`
so legacy `.op` files still load. Carved into its own module so
`persistence.rs` stays under the 800-line cap.
- Round-trip tested through the real serde JSON path (2 tests).
858 workspace tests pass. Still pending (own slices): canvas
drop-shadow paint (needs a blur primitive on `RenderBackend`),
the property-panel 效果 editing rows, and a `set_node_shadow`
MCP tool. Shadows currently round-trip through save/load but do
not yet render.
- crates/CLAUDE.md MCP catalog was stale at 21 tools; replace the
outdated table with the current 77-tool categorized summary and
point at mcp_serve.rs TOOL_SCHEMAS as the source of truth.
- Refresh the mcp/ file-layout list (node_attr_tools,
selected_ops_tools, extra_read_tools, json_serializer, …).
- New "AI chat (real provider integration)" section documenting
begin_send / ChatProvider / chat_session / the model chip and
the Codex/OpenCode honest-error behavior.
- chat_session: gate the test-only `finished()` accessor behind
`#[cfg(test)]` to drop a dead-code warning.
When a send for an unwired agent (Codex / OpenCode) landed while a
previous turn was still streaming, `launch_if_pending` wrote the
error bubble but left `current` pointing at the old session — so
the next `pump` streamed the prior agent's deltas straight into
the fresh error message. Clear `current` before writing the error.
`provider_for_agent` silently substituted Claude Code when the
user picked Codex or OpenCode — so the transcript looked like the
chosen CLI answered when it hadn't. Codex stop-gate.
It now returns `Option`: `None` for Codex / OpenCode (HTTP-server
transport not yet bridged). `launch_if_pending` writes an explicit
`error: … not wired yet` into the assistant bubble and starts no
session, so the user knows their pick didn't run.
The chat panel's bottom-toolbar chip was a hardcoded "Default"
label. It now shows the selected CLI agent and cycles through the
connected ones on click.
- `UiState.chat_selected_agent` — index into `AgentProvider::ALL`
(Claude Code / Codex CLI / OpenCode / GitHub Copilot / Gemini).
- `Document::cycle_chat_agent` — advances to the next *connected*
agent (`agent_settings.connected`); walks all 5 when none are
connected so the user can pre-pick before connecting.
- `AIChatHit::CycleModel` — the chip's left 150 px of the input
toolbar; `apply_click` routes it to `cycle_chat_agent`.
- The chip label now renders `AgentProvider::name()` instead of
the static "Default".
- `chat_session::provider_for_agent` routes the selected agent to
its `ChatProvider`: Claude Code → SDK adapter, Copilot / Gemini
→ subprocess transport. Codex / OpenCode (HTTP-server CLIs whose
bridge isn't wired yet) fall back to Claude Code.
856 workspace tests pass (+2: cycle walks connected / wraps,
cycle with nothing connected walks all five).
The chat send path was hardwired to a stub (`ChatState::send` →
"(stub) Got it"). The real providers existed but main.rs never
called them.
- `ChatState::begin_send()` — native-host send: pushes the user
message + an empty assistant bubble, raises `pending_send`. The
web shell keeps the offline `send()` stub.
- `chat_session.rs` — `ChatSession` runs the turn on a background
thread (`ChatProvider::send` is a blocking iterator; draining it
on the UI thread would freeze the window). `poll()` is
non-blocking; `launch_if_pending` / `pump` are the desktop glue.
- main.rs — Enter key + Send-button click drain `pending_send`
into a `ChatSession` against `ClaudeCodeProvider`; the winit
loop pumps deltas into the transcript, waking ~30 fps mid-turn.
A missing / unauthenticated `claude` CLI now surfaces as an
`error: claude query: …` message instead of a fake stub reply.
854 workspace tests pass (+4: chat_session stream/error,
begin_send push/empty).
Append a fresh empty page + switch the active page to it. No
args. Mirrors TS `addPage()`. Applier returns false on
id-space exhaustion at `max_node_id() + 1`.
Wire shape:
args: {}
result: { "wrote": "true" }
command: `McpCommand::AddPage`
Companion to set_active_page (commit 1af880c2): LLMs can now
create + navigate pages without touching the UI. Future patches
can add rename_page / delete_page / duplicate_page to complete
the page CRUD (Document already has the mutators).
Desktop --mcp registry + tools/list schema + exact-count test
updated for 29 tools.
28th tool in the catalog. set_active_page lets LLMs switch which
page is the active target for subsequent inserts / batch_design /
design_* commands. Required arg: `index` (0-based u32). The
applier rejects out-of-range indices.
New `Document::set_active_page(idx) -> bool` mutator in
page_mutators.rs (peer of add_page / duplicate_page / remove_page).
Clears selection on switch so stale-selection ops don't carry
across pages.
Companion refactor: write_tools.rs was at 900 lines (over the
800-line cap) after the components CRUD shipped. Split out
components/page-related tools to `mcp/component_tools.rs`
(InstantiateComponent / CreateComponent / DeleteComponent /
RenameComponent / SetActivePage). write_tools.rs back to 686.
Desktop --mcp registry + tools/list schema + exact-count test
updated for 28 tools.
27th tool in the catalog. Completes the components MCP read
surface: list_components returns the catalog (name + id pairs);
get_component drills into one entry and returns:
- name (display name)
- kind ("frame" / "group" / etc — the component's root NodeKind)
- leaf_count (sum of leaves in the prototype subtree)
LLM workflow: list_components to discover the catalog →
get_component to size each candidate → instantiate_component to
drop a clone.
Wire shape:
args: { "component_id": "<positive u64>" }
result: { name, kind, leaf_count } or ToolFailed when the id
doesn't resolve.
The snapshot factory computes leaf_count at registration time
so the tool stays `&self`. NodeKind enum mapping shared with
the rest of the MCP surface ("frame" / "group" / "rect" / etc).
Desktop --mcp registry + tools/list schema + exact-count test
updated for 27 tools.
Adds the missing update verb to the components registry CRUD.
26 tools total now. The full lifecycle on the MCP side:
- list_components (read)
- create_component (create)
- rename_component (update — name only today; the prototype
subtree stays whatever node was promoted at create time)
- instantiate_component (clone onto page)
- delete_component (delete)
Wire shape:
args: { "component_id": "<positive u64>", "name": "<string>" }
result: { "wrote": "true" }
command: `McpCommand::RenameComponent { component_id, name }`
New `ComponentLibrary::rename(id, new_name) -> bool` mutator
rejects empty / whitespace-only names so list_components never
returns a stub a user can't recognize. Apply layer also
defensively rejects unknown ids.
Desktop --mcp registry + tools/list schema + exact-count test
updated for 26 tools.
Completes the components registry MCP lifecycle:
- list_components (read)
- create_component (create from existing Frame/Group node)
- instantiate_component (drop a clone onto the active page)
- delete_component (remove from registry)
25 tools total. Live instances already on the page are NOT
affected by delete — they're independent clones; the registry
only holds the prototype that future instantiate_component
calls would have used.
Wire shape:
args: { "component_id": "<positive u64>" }
result: { "wrote": "true" }
command: `McpCommand::DeleteComponent { component_id }`
New `ComponentLibrary::remove(id) -> bool` mutator returns
whether a component was removed; apply layer propagates that
so the LLM can tell whether the id resolved.
Desktop --mcp registry + tools/list schema + exact-count test
all updated for 25 tools.
Closes the components MCP lifecycle: list_components (read),
create_component (promote a node to a component),
instantiate_component (drop a clone onto the active page). 24th
tool in the catalog.
Wire shape:
args: { "node_id": "<positive u64>", "name": "<string>" }
result: { "wrote": "true" }
command: `McpCommand::CreateComponent { node_id, name }`
New `Document::create_component_from_node` mutator (peer of
the existing `create_component_from_selected`) takes the target
id explicitly instead of reading `self.selected`, because MCP
sessions have no UI selection state. Both share the same
container-kind guard (Frame / Group only) and replace-on-
duplicate-id semantics via `ComponentLibrary::insert`.
Apply path rejects unknown ids + non-container kinds.
Variables table rejects InstantiateComponent + CreateComponent
in its Pages-level guard (defense in depth).
Desktop --mcp registry + tools/list schema + exact-count test
all updated for 24 tools.
LLM workflow:
1. list_components to see what's registered
2. create_component on a Frame / Group to register it
3. instantiate_component to drop a clone of any registered component
Closes the matching write surface to list_components. 23rd tool
in the catalog. LLM clients can now (a) list registered
components via list_components and (b) drop a clone of any of
them onto the active page via instantiate_component.
Wire shape:
args: { "component_id": "<positive u64 id from list_components>" }
result: { "wrote": "true" }
command: `McpCommand::InstantiateComponent { component_id }`
The apply path routes through the existing
`Document::instantiate_component` mutator, which deep-clones
the component's root subtree with fresh ids past
`max_node_id()`, appends it to the active page, pushes a
history snapshot, and selects the new instance root. Returns
false when the component id is unknown.
Desktop --mcp registry + tools/list schema + exact-count test
all updated; handshake test now expects 23 tools.
Components panel UI is still pending (a future patch adds a
right-rail Components section); the MCP surface is the
LLM-driven workflow alternative until then.
Surfaces the existing Document.components ComponentLibrary via
MCP so LLM clients can discover what reusable Frame / Group
subtrees the user has saved. 22nd tool in the catalog (was 21).
Wire shape:
count — total component count
components — `;`-records of `name|id`, with the standard
2-level escape on each side (matches list_variables /
list_pages wire convention).
Closes the read-only MCP surface for components. Instance
insertion via MCP (the matching write tool) requires a JSON
Node descriptor and is the natural follow-up; the data model +
`Document::instantiate_component` already exist.
Desktop --mcp registry + tools/list schema updated; test
renamed to all_twenty_two_tools with exact-count guard intact.
Codex re-review on 3891ea07 BLOCKed 8 more bypass paths.
Fix them in three groups:
1. input_active() (widget_host/input.rs:13) now includes
variable_row_focus, so apply_copy / apply_cut / apply_paste /
apply_select_all / apply_reorder (which already gate on
input_active) automatically block while a row is focused —
no need for per-method commits.
2. State-mutating shortcuts that DON'T gate on input_active() get
explicit `commit_variable_row_focus_if_any()` at the top:
- apply_undo / apply_redo / apply_group / apply_ungroup
(shortcuts.rs:55–104)
- apply_toggle_code_panel (shortcuts.rs:122)
- apply_toggle_agent_settings (shortcuts.rs:132)
- apply_set_tool (shortcuts.rs:144)
- apply_boolean_op (widget_host.rs:338)
- apply_delete (keyboard.rs:149) — special: pops a char from
the variable draft instead of falling through to selection
delete, matching the apply_backspace behavior
3. Desktop entry points in main.rs that bypass apply_press AND
the shortcut methods (Cmd+S / Cmd+O / Cmd+Shift+S / Cmd+Shift+P):
call new `commit_variable_row_focus_if_any_pub()` proxy on
the host before persistence::handle_*.
The proxy is exposed because the internal helper is
`pub(in crate::widget_host)`.
377 shell-core tests still pass.
Codex re-review on 86167224 BLOCKed two more bypass paths:
- Right-click → `apply_right_press` (press.rs:71) opens layer
context menus and returns without going through apply_press
or apply_text. Add `commit_variable_row_focus_if_any()` at
the top.
- Cmd+J → `apply_toggle_chat` (shortcuts.rs:106) flips chat.focused
directly. Add the same commit before the toggle so the chat
takeover doesn't leave keystrokes routed into the variable
draft.
The same row re-click + escape cascade paths stay clean.
Codex re-review on 939c9742 BLOCKed: piggy-backing the
variable-row commit onto commit_property_focus_if_any only
covered call sites that ran when property_focus was already
set. A standalone variable_row_focus (no property focus) +
click on canvas / toolbar / topbar / chat / layer rail
bypassed the commit entirely, keeping keystrokes routed into
the variable draft.
Move the commit to the top of apply_press, BEFORE every
hit-test branch. Property-focus commit-on-blur still gated
on the property panel's x boundary (different layout — the
property panel sits below the variables panel in the same
right rail; clicks on the property panel area shouldn't
commit a variable edit and vice-versa, but the variables
editor lives within the right rail so any non-right-rail
click is "outside" for it, and any property-panel click is
"outside" for it too via the down-stream hit-test cascade).
Chat clicks enter through apply_press first (they call
apply_click as a sub-step, lines 438 + 527), so the top-of-
apply_press commit also closes the chat-tab focus leak that
codex flagged as B2.
Codex stop-time review on 0ccf10b9 flagged that the new
variable_row_focus state stayed set when the user clicked
outside the variables panel (canvas, toolbar, etc.), causing
subsequent keystrokes to leak into the variable draft instead
of routing to the canvas / chat / other surfaces.
Root cause: the existing 9+ outside-click handlers called
`commit_property_focus_if_any` but not the new
`commit_variable_row_focus_if_any`. Rather than patching all 9
call sites, piggyback on `commit_property_focus_if_any` — it
now commits the variable-row focus first, then proceeds to its
own logic. Every existing outside-click commit path now covers
both editor surfaces in one call.
The explicit double-call at the Number/String row click site
(property_dispatch.rs:308–309) is now redundant but not wrong,
so leave as documentation that the order matters when seeding
a fresh focus.
Closes MINOR #8 non-color variable editors UI gap. Clicking a
Number or String variable row in the VariablesPanel now opens
an inline edit field on that row.
Surface:
- `Document.ui.variable_row_focus: Option<VariableRowFocus>` with
Number(idx) / String(idx) variants — new enum in document.rs
- Reuses `property_input_draft` as the buffer (single caret-blink
anchor + input handler tier serves both PropertyFocus and
VariableRowFocus)
- `VariablesPanel` carries `editing_row` + `editing_draft`
snapshots from doc.ui in `for_document`
- Paint: when the row idx matches `editing_row`, replace the
value-preview label with the draft text + a thin foreground
underline as the active-focus affordance (no caret blink yet —
the property panel's caret machinery isn't shared; the
underline is sufficient discoverability)
- Input dispatch (`widget_host/keyboard.rs`):
* apply_text: new branch BEFORE property_focus. Number accepts
digit / leading `-` / one `.`; String accepts any non-control
char. Both push to property_input_draft.
* apply_backspace: new branch BEFORE property_focus, pops draft
* apply_send: routes to new `commit_variable_row_focus_if_any`
* apply_escape: clears focus + draft (before property_focus
tier so the escape cascade still feels right)
- Commit helper `commit_variable_row_focus_if_any` in
`property_dispatch.rs`: parses the draft per kind (f64 parse
for Number — rejects NaN/Inf; raw string for String), calls
`VariableTable::set_scalar`, pushes a history snapshot on success
so undo restores the prior value.
Color rows still open the color picker (unchanged); Boolean rows
still toggle on click (unchanged); Number / String now have an
inline editor instead of a no-op.
377 shell-core tests still pass.
Coarse perf regression test for the MINOR "1000+ node doc render
pan/zoom not benchmarked" gap. Three integration tests in
`crates/openpencil-shell-core/tests/large_document_perf.rs`:
- max_node_id × 100 on 1000-leaf doc < 50 ms (debug build)
- node_at_doc_point × 100 across the grid < 25 ms (debug build)
- apply_mcp_command(BatchInsert) of 1000 descriptors < 100 ms
Bounds are deliberately generous on debug builds — the job is to
catch a future 10× regression, not measure wall-clock precision.
Real benchmarks via criterion still pending (criterion is not a
workspace dep yet); these guards are the bounded alternative.
All three pass on the current build.
Bounded slice of the non-color variable editor UI gap. Clicking
a Boolean-kind variable row in the VariablesPanel now toggles
its value through `VariableTable::set_scalar`, honoring the
active-theme routing (subset match / no default clobber / no
other-axis shadow) so a flip under theme=dark writes only the
dark entry.
Color rows still open the color picker; Number / String rows
are explicit no-ops (the MCP path covers writes today via
set_variable_number / _string — an inline text input on the
row needs a draft buffer + caret + input dispatch that's not
worth the scope tonight).
Each toggle pushes a history snapshot so undo restores the prior
value. 377 shell-core tests still pass.
Closes MINOR #9 theme axis dropdown gap. Chip-click semantics
change from "cycle to next value" to "toggle dropdown for direct
pick", matching TS's behavior where users see all axis values in
a menu and can pick any one.
Surface change:
- `Document.ui.axis_dropdown_open: Option<String>` (axis name)
- `VariablesPanelHit::AxisDropdownItem { axis, value }` new hit kind
- `VariablesPanel` carries `dropdown_open` snapshot from doc.ui
- Paint: dropdown overlay anchored under the open chip; popover
card with rounded border, per-value rows, active-value
highlight via theme.muted
- Hit-test: dropdown overlay takes top-most priority so a click
on a value row wins over the chip / row beneath
- Host wiring (`property_dispatch.rs`):
* AxisChip click toggles axis_dropdown_open (same chip closes,
different chip switches)
* AxisDropdownItem click calls VariableTable::set_active_theme +
closes the dropdown + pushes a history snapshot
Test: `axis_dropdown_hit_routes_to_named_value` asserts that with
themes=[mode: light/dark/system] and the dropdown open, hits on
rows 0 and 2 return the correct (axis, value) pair.
377 shell-core tests pass (was 376).
Closes the auto-update banner MINOR gap. TS's electron-updater
shows a status banner with a dot + label; the Rust shell now
matches the visual surface while staying honest about the
backend:
- Green status dot + "Up to date" label paint side-by-side with
the "Auto-update" header in the System tab card.
- Description line: "Running the latest installed build. No
update channel is wired in this build — the next release
ships through your package manager / .dmg / .exe."
- Existing "not yet wired" sub-line kept as the smaller
follow-up so a user wondering "why no Check button?" sees
the answer in-place.
- Three new i18n keys (settings.system.upToDate / .upToDateDescription
/ .checkForUpdates) with EN + ZH translations.
- Card height grew 64 → 88 px to fit the new lines.
No togglable switch — the previous module comment was explicit
that flipping a boolean would lie to the user. No real
network check happens. When the updater backend lands, this
can grow back into a check-now button + real status field on
Document.ui.
Codex stop-gate review on 7cee053d flagged two concerns:
1. design_skeleton/content/refine tools/list schema descriptions
could mislead LLM clients into inferring differentiated apply
semantics from the phase label. Per the internal comment in
batch_design.rs, all three currently dispatch to BatchInsert
with phase metadata only — but the schema description is what
clients see, not the comment. Add explicit "Apply behavior is
identical to batch_design today (phase is metadata only)" to
each of the three schema descriptions.
2. The new tools_list test only verified each tool name via
`contains`. A future tool addition could silently pass this
test without updating the expected count. Add an
`assert_eq!(TOOL_SCHEMAS.len(), 21)` exact-count guard with
a clear error message ("add the new tool to this test").
Closes the layered design workflow MAJOR gap line. Three new
write tools (13th / 14th / 15th in the write catalog, 21 tools
total) mirror TS's pen-mcp phased generation workflow:
- design_skeleton — phase 1, structural scaffolding
- design_content — phase 2, fill content
- design_refine — phase 3, polish details
Each tool shares batch_design's wire shape (nodes_json carrying a
JSON array of leaf descriptors). Today every phase emits the same
McpCommand::BatchInsert — the phasing is metadata only, stamped
into the response payload as `phase=skeleton|content|refine` so
LLM clients can correlate the call back to its workflow phase.
A future patch may grow per-phase apply semantics (design_refine
patching existing nodes via UpdateNode batches, etc.) once the
richer commands exist.
mcp_serve.rs wires the three new snapshots into rebuild_registry
+ adds their tools/list schemas. The handshake test is renamed
`tools_list_response_includes_all_twenty_one_tools`.
Smoke test: three back-to-back tools/call invocations (skeleton
→ content → refine) return 3 valid responses each carrying the
correct phase label, and all 3 nodes persist to disk.
Codex stop-time review on d422f5ef flagged that the Color+Str
arm of VariableTable::set_scalar wrote any string into a Color
variable without hex validation. The tool snapshot for
set_variable_string filters Color names out, but a misrouted
command (or a forged McpCommand) would still mutate the Color
variable to garbage at apply time. Defense in depth required.
Fix: forbid Color from set_scalar entirely. Color variables MUST
go through set_color_hex which validates the hex up front.
McpCommand::SetVariableScalar against a Color variable now
returns false instead of corrupting the stored value.
Regression test `apply_mcp_command_rejects_color_through_scalar_path`
stamps a Color variable with `#abcdef`, fires a forged
SetVariableScalar with `String("garbage")`, and asserts the
Color value is untouched.
376 shell-core tests pass (was 375).
Closes the MCP side of the "non-color variable editors" MINOR
gap. Three new write tools (10th / 11th / 12th in the write
catalog) mirror set_variable_color for the Number / String /
Boolean variable kinds.
All three route through one shared apply-time mutator:
\`VariableTable::set_scalar(name, VariableScalar)\` (new in
\`document/variables.rs\`). This generalizes set_color_hex's
theme-routing logic (subset match / no default clobber / no
other-axis shadow) over any VariableScalar variant. Kind/scalar
mismatch is rejected at apply time as defense in depth even when
the tool's snapshot would have admitted it.
One unified McpCommand variant \`SetVariableScalar { name, scalar:
VariableScalarPayload }\` plus a wire-friendly payload enum
(Number(f64) / String(String) / Boolean(bool)) so the wire layer
doesn't need to reach into shell-core's document API.
The three tools share an arg shape (\`name\` + \`value\`):
- number: value parsed as finite f64, rejects NaN / Inf / non-numeric
- string: value taken verbatim
- boolean: value must be "true" or "false" (case-sensitive)
Code lives in a new \`mcp/scalar_vars.rs\` sibling so
\`write_tools.rs\` stays under the 800-line cap; tests in
\`mcp/scalar_vars_tests.rs\` (5 tests).
\`openpencil-desktop --mcp\` registry + tools/list schema updated
so external CLIs see all 18 tools (was 15). Handshake test
renamed to match.
375 shell-core tests pass (was 370 — 5 new scalar_vars tests).
Closes MAJOR item from the 2026-05-14 gap list. The ninth MCP
write tool. Mirrors TS \`batch_design\` for the leaf subset of
NodeKind (frame / group / rect / ellipse / polygon / line /
text / path).
Wire shape: scalar arg \`nodes_json\` carrying a JSON array of
\`{kind, name, x, y, width, height, fill_hex?}\` descriptors. The
shell-core parser rejects structured args at the top level (so
an LLM can't sneak nested objects past scalar contracts), but a
JSON array wrapped in a quoted string round-trips cleanly — the
tool parses the inner JSON itself.
Implementation lives in \`crates/openpencil-shell-core/src/mcp/
batch_design.rs\` (sibling carve-out keeps write_tools.rs under
the 800-line cap). New \`McpCommand::BatchInsert { items:
Vec<BatchInsertItem> }\` variant + apply branch in mcp_apply.rs
that:
- validates EVERY descriptor before any mutation (kind /
geometry / fill_hex)
- allocates fresh ids past max_node_id() up front for the whole
batch (checked_add per id; bail on overflow)
- only after all validation + allocation passes, attaches each
new node to the active page
A single bad entry rejects the whole batch — the LLM never sees
a partial design tree (defense in depth: same check at tool layer
+ applier).
Tool also handles wire-level JSON escaping: the shell-core
parser stores string values with escape sequences intact (\\\"
arrives as backslash-quote), so the tool runs an unescape pass
before its hand-rolled JSON-array parser. Handles \\\" / \\\\ /
\\n / \\t / \\r / \\/.
Tests in \`mcp/batch_design_tests.rs\` (9 tests):
- requires nodes_json arg
- rejects empty array (tool) + empty items (applier)
- rejects unknown kind, negative geometry, missing required keys,
malformed JSON (6 separate cases)
- parses minimal 2-node array + applies it with fresh ids
- atomicity on bad descriptor at the apply layer
\`openpencil-desktop --mcp\` registry + tools/list schema updated
so external CLIs see batch_design as the 15th tool.
Smoke test: \`tools/call batch_design\` with 3 leaf descriptors
(2 rect + 1 text) → \`{count:"3",wrote:"true"}\` response + all
3 nodes persisted to disk.
370 shell-core tests pass (was 361 — 9 new batch_design tests).
Codex stop-time review on 23b0bfba flagged that real MCP clients
(Claude Code, Codex, etc.) won't dispatch tools/call cold — they
open with \`initialize\` and \`tools/list\` to discover the
server. The previous --mcp implementation responded only to
tools/call, so real clients hung at handshake.
Add in-binary handlers for the MCP control-plane methods:
- \`initialize\` → protocolVersion "2024-11-05" + capabilities
(tools.listChanged=false) + serverInfo
- \`notifications/initialized\` (and bare \`initialized\`) →
absorbed silently (spec: notifications have no response)
- \`tools/list\` → response with all 14 tools + their JSON
inputSchemas (name / description / required args / enums for
kind + drop_children)
- \`ping\` → empty result
- everything else falls through to shell-core's
run_stdio_with_applier so the parser hardening + apply
discipline still apply
Top-level method/id sniffing uses the same key-walker pattern as
shell-core's arguments_field, so a nested key called "method" or
a string value of "initialize" can't shadow the real top-level
method. Four sniff/response unit tests cover the discipline.
End-to-end roundtrip verified with real stdin/stdout:
initialize → notifications/initialized → tools/list → tools/call
insert_node → tools/call list_pages → all five frames return
correct JSON-RPC + the inserted rect persists to disk.
Closes BLOCKER #1 from the 2026-05-14 gap report: the Rust
shell's MCP ToolRegistry now has a host. \`openpencil-desktop
--mcp <path>\` skips the winit event loop and runs a JSON-RPC
stdio server backed by that .op file. External CLIs (Claude
Code / Codex / Gemini / Copilot) can spawn the binary in this
mode the same way they spawn TS pen-mcp today.
Implementation (\`crates/openpencil-desktop/src/mcp_serve.rs\`,
~95 lines):
- Loads the .op file via the existing canonical persistence path
(made \`load_from_path\` pub).
- Re-builds the ToolRegistry against the live document between
every dispatched call so read-tool snapshots reflect the
latest state (write commands mutate the doc; reused registry
would have stale snapshots).
- Dispatches one JSON-RPC line at a time through
shell-core's run_stdio_with_applier so the parser-level
hardening (structured-arg rejection / top-level walker / no-
hang error path) protects this binary too.
- Apply closure mutates the doc + saves to the same path on
every successful write command. Save failures surface to
stderr but don't crash the loop.
Smoke test passed: list_pages → insert_node → list_pages
roundtrip through real stdin/stdout returns three JSON-RPC
responses, and the inserted rect persists to disk (verified
by grepping FromMCP out of the saved file).
361 shell-core tests still pass; workspace build clean.
Adds a new "MCP server (shell-core)" section to
crates/CLAUDE.md covering:
- The 14-tool catalog (6 read + 8 write) with arg shapes and
emitted McpCommand variants per tool.
- Pre-validate-then-mutate discipline at the apply layer.
- `replace_node`'s `drop_children` destructive-swap guard.
- Wire-format stop-gates (structured-arg rejection, top-level
`arguments` walker, no-hang on parse failure, read-only path
refusing write tools).
- mcp/ file layout including the sibling test files for
copy_node and replace_node.
- Pending: host wiring, batch_design / design_skeleton tools,
HttpServer transport.
Codex stop-time review on 4ff2b87f flagged that the stricter
parser created a new failure mode: when parse_tool_call returns
None (structured args, unparseable JSON, etc.), run_stdio just
\`continue\`s — JSON-RPC clients waiting on a response correlated
by id never get one and hang.
Fix:
- Expose \`parser::extract_request_id\` so the dispatcher can
recover the id when the full parse fails. Helper does only
what parse_tool_call's first three lines used to do; parse_tool_call
now calls it internally so there's a single source of truth.
- In \`run_stdio_with_applier\`, on \`parse_tool_call → None\`,
attempt to extract the id; if found, write a typed error
response (\`InvalidArgument\` + "malformed tool call:
unparseable or structured arguments"). If even the id is
missing, drop the line silently — there's nothing to
correlate against.
- Drop the now-dead \`extract_object_body\` helper (\`arguments_field\`
replaced it in the previous commit).
Tests:
- \`run_stdio_emits_error_when_parse_fails_so_clients_dont_hang\`
— structured \`drop_children\` arrives with valid id 42;
response echoes id 42 + carries an \`error\` field naming
"malformed tool call".
- \`run_stdio_skips_lines_without_an_id\` — id-less lines stay
silent (existing behavior, explicitly covered now).
361 shell-core tests pass (was 359).
Codex BLOCKed ee0671a7 on the substring-find inside
arguments_field: `body.find("\"arguments\"")` matched any
nested key or string value containing the literal `"arguments"`,
not just the top-level params.arguments field. Two valid-but-
unusual JSON shapes bypassed the rejection contract:
1. `meta:{"arguments":{}}` alongside `"arguments":"oops"` — the
substring scan hit meta.arguments as a Body and parsed it,
skipping the real top-level string-typed arguments which
should have rejected.
2. `name:"arguments"` with no real arguments field — false
positive Malformed instead of Missing.
Replace with a proper top-level key walker that:
- skips whitespace + commas between top-level pairs;
- reads each quoted key + its value, classifying the value
(string / object / array / number-bool-null);
- only treats `arguments` as a real hit when seen at depth 0;
- categorizes the matched value: object → Body, anything else
→ Malformed; missing entirely → Missing.
Three new regression tests in `mcp_tests.rs::
parse_tool_call_arguments_lookup_is_top_level_only` cover the
nested-shadow, string-value-collision, and deep-nested-key
cases.
359 shell-core tests pass.
Codex BLOCKed 0a434e6b on three items:
1. MCP arguments non-object downgrade — \`"arguments":"oops"\`
used to flow through extract_object_body returning None and
parse_tool_call downgrading to empty args. Replace the call
site with a tri-state \`arguments_field\` helper:
- Missing → empty args (legit MCP can omit \`arguments\`).
- Body(s) → parse the object body.
- Malformed → reject the parse.
2. Test coverage gap — the structured-rejection tests only
exercised the legacy/direct line-protocol shape. Add two new
tests under mcp_tests:
- parse_tool_call_rejects_structured_values_in_mcp_tools_call_shape:
nested object + array inside MCP \`arguments\` both reject.
- parse_tool_call_rejects_non_object_arguments_field: string /
number / array \`arguments\` reject; missing \`arguments\`
still parses as empty args.
3. Stale doc comment on parse_flat_object_body — said nested
values are "skipped", now correctly describes the wire-layer
rejection contract.
358 shell-core tests pass (was 356 — two new MCP tools/call
shape tests).
Codex re-review on c6e6a308 flagged the sentinel approach's
collision risk: a variable literally named "{...}" — or any
string-accepting arg with that value — was indistinguishable
from wire-malformed input. Move the rejection up to the wire
layer so no tool sees a structured value at all.
- `parse_flat_object_body` now returns `None` the moment it
sees `{` or `[` for any value (was: insert a sentinel).
- `parse_tool_call` previously masked None with
`.unwrap_or_default()`, which would have silently swallowed
the rejection back into empty args. Replace
`extract_params_object` with a tri-state `ParamsResult`
(Missing / Body / Malformed) so we can distinguish "no params
key" (legit empty args) from "params present but malformed"
(full call rejection). MCP `tools/call` path treats a missing
`arguments` key the same way (empty args), and propagates a
`None` from the inner parse as a full call rejection.
Tests:
- `parse_tool_call_rejects_structured_arg_values` (in
`mcp_tests.rs`) asserts both object and array values cause
`parse_tool_call` to return None, while scalar-only still
parses.
- `parser_refuses_structured_drop_children_at_wire_layer` (in
`mcp/replace_node_tests.rs`) end-to-end: real MCP JSON with
structured `drop_children` and structured `name` never reach
the tool layer.
- `replace_node_rejects_malformed_drop_children` drops the
obsolete sentinel cases — they can no longer reach the tool.
356 shell-core tests pass.
Codex re-review on fd0af355 walked one level deeper into the
parser and found that ANY tool's safe-default-on-missing logic
could be bypassed by sending the arg as a JSON object or array.
Example: `{"drop_children": {}}` arrived at ReplaceNode as
absent → defaulted to false → silent acceptance.
Fix at the parser layer so every write tool benefits at once:
- `parse_flat_object_body` (parser.rs:287) now inserts `"{...}"`
or `"[...]"` as a sentinel value for the key when it walks
past a nested literal, instead of dropping the key.
- Every existing tool's scalar validation (hex regex, i32 parse,
enum match, bool match) naturally rejects the sentinel as an
InvalidArgument, so no per-tool wiring is needed.
Tests:
- `parse_tool_call_skips_nested_object_values` rewritten to
`_surfaces_nested_values_as_sentinels` — asserts the present-
with-sentinel contract for both `{}` and `[]`.
- `parser_to_tool_chain_rejects_structured_drop_children` (new
in `mcp/replace_node_tests.rs`) — end-to-end: real MCP JSON
with structured `drop_children` goes through the parser into
ReplaceNode::call and lands on `InvalidArgument`. Sweeps both
object and array shapes.
- `replace_node_rejects_malformed_drop_children` now also sweeps
the sentinels themselves as malformed strings.
356 shell-core tests pass (one obsolete test replaced + one
end-to-end test added).
Codex re-review on 2f0200b0 flagged that drop_children="" was
silently treated as false, hiding a possible mis-serialized
confirmation from the caller. Drop the empty-string special
case — only "true" / "false" / missing are now accepted; anything
else returns InvalidArgument.
Test broadened: the malformed-string case now sweeps "yes" / ""
/ "TRUE" / "1" / "0" so every common mis-spelling is covered.
