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.
Codex stop-time review on replace_node flagged that a doc-comment
warning isn't enough — the tool would still silently delete a
container's subtree if the LLM picked the wrong target. Promote
the safety from advisory to enforced:
- Thread `drop_children: bool` through `McpCommand::ReplaceNode`.
- Apply path: if the target has children AND `drop_children` is
false, refuse the swap before allocating an id or mutating
pages. Container subtrees are preserved by default.
- Tool side: parse optional `drop_children` arg ("true"/"false");
default false; malformed string returns `InvalidArgument` with
a message naming the arg.
Tests cover the four reachable shapes: leaf swap with default
`drop_children=false` still works (pre-existing behavior); a
container swap without consent refuses; the same swap with
`drop_children=true` succeeds; a malformed `drop_children` arg
fails parsing.
355 shell-core tests pass (was 352 — three new guard tests).
Codex review on e51b979f flagged that the replace_node doc
comment didn't mention the destructive behavior on containers.
Add an explicit "Destructive on containers" note to both the
McpCommand variant and the tool struct, pointing readers at
update_node for in-place patches.
No behavior change.
Eighth write tool in the catalog. Required args: node_id, kind,
name, x, y, width, height. Optional: fill_hex. Builds a fresh
node with a non-colliding id and swaps it into the same parent
slot the target node currently occupies, preserving sibling
order.
Bounded scope: only leaf-style fields land on the replacement.
Full subtree replacement requires a JSON Node parser that
doesn't live on this side yet. The current contract matches TS
`replace_node` for primitives, minus children.
Apply path follows the same pre-validate-then-mutate discipline
as `update_node` and `move_node`: kind / geometry / fill_hex /
target existence / id space — every check before any mutation.
A bad fill_hex never leaves the document half-touched (covered
by `apply_mcp_command_replace_node_atomic_on_invalid_fill_hex`).
Tests live in `mcp/replace_node_tests.rs` (matches the
`copy_node_tests.rs` sibling pattern; keeps `write_tools_tests.rs`
under cap).
Codex review on 100eb78a flagged that the McpCommand::CopyNode
doc comment promised a `new_root_id` field that the tool never
emits. The tool returns `{"wrote": "true"}` like every other
write command because `ToolOutcome` is built before apply runs —
the allocator that mints fresh ids is reachable only from the
host applier, not the tool.
Rewrite the doc comment to describe the actual contract instead
of a feature that's wishful. Threading the allocator back to the
tool to surface real clone ids is a future patch.
Seventh write tool in the catalog. Validates node_id +
target_parent_id args, then issues `McpCommand::CopyNode` which
the host applies via `Document::apply_mcp_command`. The applier
walks the source subtree, allocates fresh ids past `max_node_id()`
for every cloned node, and attaches the clone under the target
(page root when `target_parent_id == 0`).
Allows `node_id == target_parent_id` so an LLM can duplicate a
container's contents under itself (valid copy semantics; differs
from move_node which rejects that case).
Tests live in a new `mcp/copy_node_tests.rs` sibling — bundling
them into `write_tools_tests.rs` would have pushed it past the
800-line cap.
Codex stop-gate caught a real correctness bug: `move_node`'s
apply path was:
1. validate source exists + cycle check
2. detach the source (vec.remove → owned Node)
3. find_node_mut_in_doc(target) — if None, return false
When target_parent_id was unknown, step 2 had already removed
the source from its parent. The owned Node fell off the end of
the function and was DROPPED — silently destroying the source.
The existing unknown-id test only covered the path where the
old `find_node_in_doc` on the cycle check incidentally caught
it; an LLM passing a bogus target_parent_id would lose data.
Fix: every validation (source exists, target exists, cycle
guard, page-root path's active page exists) runs upfront. Only
after all checks pass does the detach + reattach proceed. The
two halves of the mutation are no longer separable from each
other — match-statement style atomicity, no orphan path.
Test (1 added, 343 shell-core total):
- `apply_mcp_command_move_node_preserves_source_when_target_unknown`
builds the exact codex scenario: MoveNode { node_id: 11
(sample Title), target_parent_id: 99999 (doesn't exist) }.
Asserts:
1. apply returns false (no successful move),
2. Frame's child count is unchanged (Title still there),
3. Title is still in Frame's children vec,
4. Title appears EXACTLY ONCE in the doc (the dropped-
source bug would have left zero occurrences).
Pre-fix assertion #4 would have failed with count = 0.
The same pre-validate-then-mutate pattern is now consistent
across update_node (atomic on invalid geometry, 9e601573),
insert_node (id-space-exhausted guard, 62a2fb5b), and
move_node (this fix). DeleteNode is naturally atomic — single
remove_in_subtree call.
Sixth MCP write tool, completing the node-lifecycle quartet
(insert + update + delete + move). LLMs can now reparent nodes
across the document tree — from page root to a group, between
groups, or back to the root.
`McpCommand::MoveNode { node_id, target_parent_id }`:
- `target_parent_id == 0` reparents to the active page root.
Non-zero ids must resolve to an existing node.
- Cycle guard at apply time: if target_parent is a descendant
of source, the move would orphan + cycle the subtree, so
apply returns false.
- Same-id reparent (node_id == target_parent_id) is rejected
at the tool layer with InvalidArgument.
`src/document/mcp_apply.rs`:
- MoveNode branch on Document::apply_mcp_command. Runs the
cycle guard via `find_node_in_doc` + `subtree_contains`
BEFORE detaching, so a rejected cycle leaves state intact.
- `detach_node(doc, id)` + `detach_from_subtree(vec, id)`
walkers — locate the node, `vec.remove(idx)` it, return
the owned Node. Reattach via push to the new parent's
children.
- `find_node_in_doc(doc, id)` + `find_in_subtree_ref(slice,
id)` — immutable variants for the cycle check (can't share
a mutable borrow with the subsequent detach).
- `subtree_contains(node, target) -> bool` — recursive scan
of a node's id + descendants.
`src/document/variables.rs`:
- VariableTable::apply_mcp_command's "Pages-level commands not
mine" arm grew MoveNode to keep the exhaustive match.
`src/mcp/write_tools.rs`:
- `MoveNode` (stateless) + `move_node_snapshot()` factory.
Validates both node_id (positive u64) + target_parent_id
(any u64, 0 ⇒ page root) and rejects same-id pair.
Tests (6 added, 342 shell-core total):
- move_node_validates_args — missing both / missing target /
node_id=0 / node_id == target → InvalidArgument.
- move_node_returns_command_with_zero_target_for_page_root —
target_parent_id=0 carries through unchanged.
- apply_mcp_command_move_node_reparents_to_page_root — sample
doc; Title(11) under Frame(10); after move target_parent_id=0
Title is at page root + Frame no longer carries it.
- apply_mcp_command_move_node_reparents_to_another_node —
Title(11) → Button group(12); Title leaves Frame, lives
under Button.
- apply_mcp_command_move_node_rejects_cycle — Frame(10) ↓
Button(12) → MoveNode { node_id: 10, target_parent_id: 12 }
would cycle. Apply returns false + tree is structurally
intact (Frame at root, Button as child).
- apply_mcp_command_move_node_rejects_unknown_id — both
unknown-source and unknown-target branches.
MCP write surface: set_variable_color, set_active_axis_value,
insert_node, update_node, delete_node, move_node. The remaining
TS pen-mcp catalog: copy_node, replace, batch_design,
design_skeleton.
Codex stop-gate caught a real correctness bug: `update_node`'s
apply path was mutating bounds.origin.x and bounds.origin.y BEFORE
checking width / height for negative values. A request like:
UpdateNode { x: Some(999), y: Some(999), width: Some(-1), ... }
would move the node to (999, 999) AND then reject the resize,
leaving the node half-updated. Worse, in the wire-level flow the
client receives `host rejected command` (Internal demotion), so
they think nothing changed — but actually the move did apply.
Fix: run ALL field validation BEFORE the mutable borrow + writes.
The negative-width / negative-height checks run upfront alongside
the existing fill_hex parse. After the validation block, no
early-return is possible, so every set of writes either applies
in full or doesn't apply at all.
Tests (2 added, 336 shell-core total):
- `apply_mcp_command_update_node_is_atomic_on_invalid_geometry`
— sample doc's Title (id 11) bounds snapshotted; UpdateNode
with valid x/y/name + negative width. Asserts apply returns
false AND every field matches pre-call state. Pre-fix the
test would have observed x=999, y=999, and the rename.
- `apply_mcp_command_update_node_is_atomic_on_invalid_hex` —
parallel atomicity for the fill_hex path; valid x/y/name +
bad hex. Same pre/post bounds + name assertion.
The same atomicity invariant already held for InsertNode (single
mutation point at the end of the branch) and DeleteNode (atomic
by definition — single `retain` call).
Two more MCP write tools, mirroring the TS pen-mcp catalog's most-
common mutations beyond insert_node. Both follow the established
write architecture: tool validates args + returns `OkWithCommand`;
host applier mutates the live document; stdio applier path demotes
to Internal on apply-time rejection.
`McpCommand` (in `src/mcp.rs`):
- `UpdateNode { node_id, x, y, width, height, name, fill_hex }`.
Every patch field is Option<…>; None leaves the live value
unchanged. Bounds writes replace coords piecemeal so a caller
can move (x, y) without resizing or vice versa.
- `DeleteNode { node_id }`. Removes the node + all its
descendants from its parent.
`src/document.rs`:
- `NodeId::new_opt(u64) -> Option<Self>`. Non-panicking sibling
of `new()` — returns None for id 0 (NONE sentinel). Used by
the write tools to validate arbitrary wire-supplied ids
without panicking.
`src/document/mcp_apply.rs`:
- Document::apply_mcp_command branches grow UpdateNode +
DeleteNode. UpdateNode pre-validates the optional fill_hex
BEFORE the mutable borrow on pages so a bad hex doesn't
partially mutate the node. Width / height patches reject
negative values.
- `find_node_mut_in_doc(doc, id)` + `find_in_subtree(slice, id)`
+ `remove_in_subtree(vec, id)` recursion helpers that walk
every page's tree. find_in_subtree uses the "position-then-
index" split so the borrow checker doesn't see overlapping
iter_mut ranges.
`src/document/variables.rs`:
- VariableTable::apply_mcp_command's UpdateNode + DeleteNode
arms return false (Pages-level commands aren't theirs to
apply), preserving the exhaustive match.
`src/mcp/write_tools.rs` (NEW, 437 lines):
- All MCP write tools moved out of `tools.rs` (which would
have grown past the 800-line cap): SetVariableColor,
SetActiveAxisValue, InsertNode, UpdateNode, DeleteNode +
their factories + shared parse helpers (validate_hex,
parse_i32_arg, parse_opt_i32, ALLOWED_KINDS).
- tools.rs now holds only read tools (510 lines, under cap).
- get_active_theme_snapshot stays in tools.rs (read-side
factory for the GetActiveTheme tool which is also a read
tool).
`src/mcp/write_tools_tests.rs` (NEW, 532 lines):
- All write-tool tests moved out of tools_tests.rs (which was
900 lines after the write-tool tests were appended). Both
test files now under cap.
- mcp.rs registers `#[cfg(test)] mod write_tools_tests;`
alongside its sibling tools_tests.
`src/mcp.rs`:
- Re-export surface split into read-side (from tools::) and
write-side (from write_tools::) so the public API stays
flat (`mcp::SetVariableColor`, `mcp::UpdateNode`, etc.).
Tests (10 added, 334 shell-core total):
- update_node tools: required-node-id, id-format validation
(must be positive u64), empty-patch error, partial-patch
happy path, apply routes to the right node + leaves
unspecified fields untouched, apply rejects unknown id.
- delete_node tools: required-arg + id-format validation;
apply removes the node from its parent + descendants;
apply rejects unknown id.
MCP write surface now: set_variable_color, set_active_axis_value,
insert_node, update_node, delete_node. Remaining TS pen-mcp
catalog: move_node, copy_node, replace, batch_design,
design_skeleton.
Codex stop-gate caught: `next_node_id_seed` used
`max_node_id().saturating_add(1)`. When a live node sits at
`u64::MAX`, saturating_add wraps back to u64::MAX — so the
allocator would return the SAME id as the existing node and the
push would create a duplicate NodeId. Document::find would then
return whichever was first, silently masking or partially
mutating the original.
Fix: switch to `checked_add(1)`. The seed now returns `Option<u64>`
— `None` when the id space is exhausted. `Document::apply_mcp_
command`'s InsertNode branch surfaces the None as `false` (apply
failure), and the run_stdio_with_applier path already demotes
that to `ToolErrorCode::Internal` so the client sees a clear
"host rejected command" rather than a fake success that doesn't
actually create anything new.
Test (1 added, 326 shell-core total):
- `apply_mcp_command_insert_rejects_when_id_space_exhausted` —
plants a live node at `u64::MAX`, attempts InsertNode, asserts
apply returns `false` AND `pages[0].children` length is
unchanged AND the live u64::MAX node's name is preserved
(so we know the duplicate id didn't accidentally overwrite
it). Pre-fix this test would have passed `apply` returning
`true` while corrupting state.
The bound is comfortable in practice — a document would need 2^64
live nodes to hit it. The fix is defensive correctness, not a
performance concern.
Third MCP write tool, completing the "create + style + theme" write
surface for LLM clients. `insert_node` is the TS pen-mcp
equivalent's flagship capability — without it, LLMs can only
modify existing nodes; with it, they can build entire designs.
`crates/openpencil-shell-core/src/mcp.rs`:
- `McpCommand::InsertNode { kind, name, x, y, width, height,
fill_hex }`. fill_hex is `Option<String>` — color-bearing
shapes can carry a fill; structural nodes (frame/group)
pass None.
`crates/openpencil-shell-core/src/document/mcp_apply.rs` (NEW):
- `Document::apply_mcp_command(cmd)` — lifted to Document level
so InsertNode can reach Pages + the id allocator (variable +
theme commands still route to `var_table.apply_mcp_command`).
- `Document::next_node_id_seed()` — allocates a fresh id past
`max_node_id() + 1`, saturating_add-guarded so u64::MAX
returns 1 instead of colliding with NodeId::NONE.
- `parse_node_kind(s)` — accepts the same lowercase strings
the read-side tools (get_node, get_selection) emit, so an
LLM can round-trip a node's kind through read → modify →
re-insert without re-encoding.
- Pulled out of mutators.rs to keep that file under 800 lines.
`crates/openpencil-shell-core/src/mcp/tools.rs`:
- `InsertNode` (stateless tool struct) + `insert_node_snapshot()`
factory. No document snapshot needed — the tool doesn't need
to know the current state; the host's applier handles id
allocation + bounds installation.
- `McpTool::call` validates:
- All required args present (kind / name / x / y / width /
height); each `MissingArgument` carries the missing
name.
- kind in ALLOWED_KINDS (frame / group / rect / ellipse /
polygon / line / text / path).
- x / y / width / height parse as decimal i32 (negative x/y
allowed for nodes placed off the page-origin; width /
height must be non-negative).
- Optional fill_hex parses as #rgb / #rrggbb / #rrggbbaa.
- Returns `OkWithCommand(InsertNode)` on success.
Tests (6 added, 325 shell-core total):
- insert_node_validates_required_args — missing kind →
MissingArgument; invalid kind → InvalidArgument.
- insert_node_validates_numeric_args — non-numeric x →
InvalidArgument; negative width → InvalidArgument.
- insert_node_validates_optional_fill_hex — bad hex →
InvalidArgument.
- insert_node_returns_command_with_parsed_args — happy path;
every field round-trips into the McpCommand payload.
- apply_mcp_command_routes_insert_node — end-to-end: doc.empty()
→ apply InsertNode → new node lives on active page, bounds +
fill flow through, name matches.
- apply_mcp_command_rejects_invalid_node_kind — bad kind at
apply time → false (defensive re-check beyond the tool's
validation, so host-only call sites are safe too).
mutators.rs trimmed from 814 → 798 (apply_mcp_command extracted +
some redundant doc comments compacted). All OP-owned files now
under the 800 cap except 3 pre-existing tech-debt violators
(codegen.rs 867, widget_host/press.rs 840,
widgets/canvas_viewport.rs 839).
MCP write surface now: 3 first-party writes (set_variable_color,
set_active_axis_value, insert_node). The TS pen-mcp catalog still
needs: update_node, delete_node, move_node, copy_node, replace,
batch_design, design_skeleton — each extends McpCommand + the
same applier pattern.
Second MCP write tool, completing the variable + theme write
surface. Mirrors `cycle_active_axis_value` (which the
VariablesPanel chip click drives) but PINS the value rather than
cycles — LLM clients use it when they want to land on a specific
axis state ("switch to dark mode") instead of stepping through
options ("flip to whatever's next").
`crates/openpencil-shell-core/src/mcp/tools.rs`:
- `SetActiveAxisValue { axes: BTreeMap<String, Vec<String>> }`
snapshot. Mirrors `var_table.themes` keyed by axis name.
- `set_active_axis_value_snapshot(doc)` factory.
- `McpTool::call` validates:
- Both `axis` and `value` args present
(`MissingArgument` on omission).
- Axis exists in the themes table (`ToolFailed`).
- Value is in `themes[axis].values` (`InvalidArgument`,
error message lists allowed values).
- Returns `OkWithCommand(SetActiveAxisValue { axis, value })`.
The McpCommand variant has existed since the write arch
landed (0f09671a); `Document::apply_mcp_command` already
routes it to `var_table.active_theme.insert`.
- Re-validates at apply time so a stale snapshot can't slip
an unauthorized value through (host applier rejects with
`false`, which the stdio path demotes to Internal).
Tests (5 added, 319 shell-core total):
- `set_active_axis_value_validates_args_and_returns_command`
— happy path; OkWithCommand variant + correct payload.
- `set_active_axis_value_errors_on_missing_args` — both args
required, message names the missing one.
- `set_active_axis_value_errors_on_unknown_axis` — ToolFailed
+ error names the axis.
- `set_active_axis_value_errors_on_value_not_in_axis` —
InvalidArgument; error message lists every allowed value.
- `apply_mcp_command_routes_set_active_axis_value` —
end-to-end happy path + the stale-state rejection branch
(invalid value at apply time returns `false`).
MCP write tool surface now:
- set_variable_color (Color hex through var_table)
- set_active_axis_value (theme axis through active_theme map)
- insert_node, update_node_position, etc. land later — each
extends `McpCommand` + the existing applier pattern.
Codex stop-gate caught: the write architecture commits (0f09671a +
63387f3f) pushed mcp.rs from 663 → 904 lines and tools.rs from 419
→ 1088. Both well over the 800 ceiling.
Split tests to sibling files, mirroring the
`layer_panel_tests` / `property_panel_tests` / `variables_tests`
pattern already used elsewhere in shell-core.
src/mcp.rs 904 → 347 (impl spine only)
src/mcp_tests.rs (new, 560)
src/mcp/tools.rs 1088 → 610 (impl spine only)
src/mcp/tools_tests.rs (new, 479)
`src/lib.rs` gains `#[cfg(test)] mod mcp_tests;` next to
`pub mod mcp;` (matching the inline form for tests_geometry /
tests_mutators).
`src/mcp.rs` gains `#[cfg(test)] mod tools_tests;` next to
`pub mod tools;` (one-line form keeps mcp.rs under cap).
Visibility lift: `escape_record_field` bumped from private to
`pub(crate)` so the sibling tests file can drive the
list_variables backward-compat assertions.
No behavior change — every test moved verbatim. 314 shell-core
tests still pass. All OP-owned files under 800 lines except the
3 long-standing pre-existing violators (codegen.rs 867,
widget_host/press.rs 840, widgets/canvas_viewport.rs 839) which
are tracked separately and not touched by this session's edits.
Codex stop-gate caught: the previous commit (0f09671a) shipped the
`OkWithCommand` write path but `run_stdio` would dispatch a write
tool, get back `ToolResponse::Ok { command: Some(_), .. }`, and
write `result:{wrote:true}` to the wire WITHOUT applying the
command. Clients saw success for a mutation that never happened.
Split run_stdio into a read-only path + a write-aware path:
`run_stdio(registry, reader, writer)` — read-only. Calls the new
applier-aware variant with an applier that always returns false,
so any `OkWithCommand` is demoted to `ToolErrorCode::Internal`
("host rejected command: ..."). Clients can't see misleading
success on this path.
`run_stdio_with_applier(registry, reader, writer, F)` — accepts
`F: FnMut(&McpCommand) -> bool`. For each dispatched ToolCall:
- read tools (no command) → response written verbatim.
- write tools, applier returns true → response written as
success (the host has applied + the client gets the tool's
`result` payload).
- write tools, applier returns false → demoted to
`Internal` with `host rejected command: <Debug>` so the
client knows the mutation didn't land.
The real `openpencil-mcp` binary wires this with `Document::
apply_mcp_command` as the closure — same as the existing
applier signature `VariableTable::apply_mcp_command(&cmd) -> bool`.
Tests (3 added, 314 shell-core total):
- `run_stdio_demotes_write_tool_response_to_error_without_applier`
— the codex repro: read-only stdio with a registered write
tool. Sends a valid set_variable_color request; asserts the
wire output carries `code: -32603` (Internal) + the "host
rejected command" sentinel.
- `run_stdio_with_applier_applies_write_command_then_writes_success`
— applier returns true; the closure receives the command
exactly once (verified by collecting into a Vec); the wire
response is a clean Ok with no error code.
- `run_stdio_with_applier_demotes_when_applier_rejects` —
applier returns false (simulates host state drift); response
demotes to Internal so the client knows the write didn't
land.
The MCP write architecture is now end-to-end safe: tools validate,
the registry surfaces commands, stdio applies them through a
host-supplied closure or refuses to claim success.
Closes the architectural gap for MCP write tools that's been
deferred across the session. The McpTool trait stays `&self` (so
trait-object registry + Send + Sync bounds work cleanly), but
ToolOutcome grows a third variant that lets validate-only tools
describe a mutation the host applies later.
`crates/openpencil-shell-core/src/mcp.rs`:
- `ToolOutcome::OkWithCommand(BTreeMap, McpCommand)` — write
tools return this from `call`. The registry's dispatch lifts
the command into `ToolResponse::Ok { id, result, command:
Some(...) }` so the caller doesn't have to re-walk the tool
list to learn what was queued.
- `McpCommand` enum — typed mutation requests. v1 variants:
SetVariableColor { name, hex }
SetActiveAxisValue { axis, value }
Each maps to an existing Document mutator (set_color_hex /
set_active_theme) so the correctness chain (subset matching,
no-default-clobber, no-other-axis-shadow, var_table in the
history snapshot) carries forward verbatim.
- `ToolResponse::Ok` grew an optional `command: Option<
McpCommand>` field. Pre-existing read tools fill `None`.
- `response_to_json` ignores `command` when serialising
(host-only consumption — wire format unchanged).
`crates/openpencil-shell-core/src/mcp/tools.rs`:
- `SetVariableColor` tool. Validates `name` exists + is
Color-kind (snapshot from var_table) + `hex` parses as
`#rgb` / `#rrggbb` / `#rrggbbaa`. Returns
`OkWithCommand({wrote: true}, SetVariableColor)`.
- `set_variable_color_snapshot(doc)` factory — same pattern
as the read tools, snapshotted at host registration time.
- `validate_hex` lenient on case, requires leading `#`.
`crates/openpencil-shell-core/src/document/variables.rs`:
- `VariableTable::apply_mcp_command(&cmd)` — the host applier.
Branches per command variant, delegates to the underlying
mutator. Returns true when the table actually changed (caller
pushes an undo snapshot via the existing history machinery).
- `SetActiveAxisValue` rejects values not in `theme_axis.values`
so an LLM can't drift the active map into invalid states.
Tests (5 added, 311 shell-core total):
- set_variable_color_validates_args_and_returns_command — the
happy path; OkWithCommand variant + correct McpCommand
payload.
- set_variable_color_errors_on_missing_args — both `name`
AND `hex` are required.
- set_variable_color_errors_on_unknown_variable — ToolFailed +
error message names the missing variable.
- set_variable_color_errors_on_invalid_hex — fuzz across 4 bad
inputs (no `#`, too short, bogus chars), all → InvalidArgument.
- apply_mcp_command_routes_set_variable_color_to_var_table —
end-to-end: build command → apply → resolve_color reads back
the new value.
MCP surface now: 6 read tools + 1 write tool + architectural
support for arbitrary future writes. `insert_node`,
`update_node_position`, and the rest of the TS pen-mcp write
catalog plug into the same `McpCommand` enum + `apply_mcp_command`
applier.
Codex stop-gate (third pass on the comma-escape work): the
previous commit (525acbb6) used `escape_layered_field` for BOTH
output fields of get_active_theme, but `axes` is structurally a
2-level format (`;`-records of `|`-pairs) — exactly like
list_variables. Standard `unescape_record_field` decoders strip
only `\\` / `\;` / `\|`, so any `\,` we emit shows up as a
literal 2-char `\,` sequence in the client's decoded output.
Now:
`axes` — `escape_record_field` (3-char set: `\;|`)
↳ wire-compatible with list_variables decoders.
`options` — `escape_layered_field` (4-char set: `\;|,`)
↳ the only field that needs comma protection,
because the inner value list joins with `,`.
The two encodings cohabit cleanly: a client written for
list_variables can decode `axes` without changes; a client that
wants `options` knows to expect the deeper escape set + uses the
layered_split helper.
Test (1 added, 306 shell-core total):
- `get_active_theme_axes_field_does_not_escape_commas` —
active theme with axis name + value both containing commas
(`"axis,with,commas"` = `"value,with,commas"`). Asserts the
`axes` output substring is exact, AND that no `\,` sequence
appears anywhere in the field.
Existing tests still pass:
- `get_active_theme_round_trips_comma_in_value` (options
field; commas escape correctly via the layered set).
- `list_variables_does_not_escape_commas_backward_compat`
(records-and-pairs format unaffected).
