feat(mcp): assembly mutates in shared cad_submit map - #63
Conversation
jackControls
left a comment
There was a problem hiding this comment.
Requesting changes on the current stacked head. There are two data-loss blockers and one revision-accounting issue:
-
The carried assembly update schemas still allow destructive partial replacement:
assembly_update_componentrequires onlyid/name, andassembly_update_occurrenceonlyid/name/component_id, while serde defaults omitted ownership, transforms, parent, visibility, and grounding before the complete records are replaced. Require full replacement fields or use patch DTOs with preservation tests. -
For assembly and other non-solid inbox results,
applyInboxNowfalls back toloadDocument(). That action unconditionally writesdirty: false, so an MCP-applied live edit can be treated as saved and the user can close without an unsaved-changes warning. Refresh the affected state while preserving/settingdirty: true. -
The native inbox apply already increments
engine_revision, then the store changes caused byapplyInboxNowreach the subscription and callmcp_session_bridge_note_mutation, incrementing it again. Suppress the local mutation note for inbox-originated synchronization (and prefer a targeted refresh over the full document reload).
This also stacks on open #60 and carries #62 functionality; please rebase/deduplicate after the dependencies are corrected. I did not merge.
|
Addressed Jack’s three #63 blockers (local only; not merging):
Commits: |
Stack STEP import + solid move/copy ToolSpecs (jackControls#61) onto the fixed assembly submit-map tip (jackControls#63 @ 19800b9) so cad_submit accepts solid_import_step, solid_edit_import_step, solid_move_copy, and solid_edit_move_copy while attached. cad_script and cad_compare_solids stay read-safe control helpers (not inbox mutates). Upgrade cad_script to jackControls#61 @ 52266b2 behavior: attach/refresh seeds a cad_load_project_model baseline, and the e2e test actually replays the dumped script on a fresh CadServer (detach after attach so jackControls#60 session_read_only still holds). Bump MODELING_TOOL_COUNT and keep ToolSpec↔map sync.
jackControls
left a comment
There was a problem hiding this comment.
Re-reviewed at 19800b9. The latest PR-local fix looks correct: inbox refresh preserves dirty state and suppresses the duplicate revision note while apply is in progress. I cannot approve the stacked PR yet because it inherits PR60's unresolved atomic revision race and stuck conflicted/malformed head-of-queue behavior. Its MCP check is also failing on the inherited formatting diff. Please resolve/rebase those upstream blockers.
19800b9 to
45324f1
Compare
|
Reset onto #60 tip
|
There was a problem hiding this comment.
Re-reviewed at 45324f1. The PR-local work is sound: assembly mutates use the shared cad_submit map, dirty state remains set after inbox apply, the double revision bump is gone, and all 47 MCP-server tests pass.
This stack still inherits the two P1 coherency races from PR60: a mutation can occur between snapshot export and bridge write so stale geometry is published at the current revision, and switching retained project tabs does not rebind/advance the bridge so an op authored for tab A can dispatch through active_mut() into tab B. Assembly mutations make the second failure especially destructive. Please rebase after PR60 fixes those races and carries deterministic regression tests.
Rebase jackControls#63 onto jackControls#60 tip f241739. Mutate map entries and ToolSpecs already landed with jackControls#60/jackControls#62; keep unique classifier/lookup coverage so cad_submit accepts the six assembly modeling mutates while assembly_document and assembly_solution stay read-safe inspect tools.
45324f1 to
48222e0
Compare
|
Rebased onto #60 tip New head: Unique vs #60 (map entries + ToolSpecs already on the #60/#62 stack):
Tests: Please re-review. Still merge-after-#60. |
Rebase jackControls#63 onto jackControls#60 tip f241739. Mutate map entries and ToolSpecs already landed with jackControls#60/jackControls#62; keep unique classifier/lookup coverage so cad_submit accepts the six assembly modeling mutates while assembly_document and assembly_solution stay read-safe inspect tools.
48222e0 to
9cb5a63
Compare
cad_submit writes inbox/<seq>.json; the desktop applies via host::handle and publishes. MCP does not write model.json (no last-writer-wins).
Address Jack's three jackControls#60 blockers: 1. call_tool enforces session_read_only for direct mutates while attached (cad_submit / inspect / export stay allowed); writeback:true attach is rejected. Regression covers attach → direct mutate rejected → cad_submit accepted → detach restores headless mutation. 2. WindowPublisher.engine_revision is the authoritative OCC gate, advanced atomically with live engine apply and on immediate UI mutation notes (not heartbeat-debounce alone). Deterministic tests cover stale-base after UI note and two same-base ops. 3. Shared nbcad-mcp-mutate name→engine-method+payload map drives both cad_submit accept-list and Tauri inbox dispatch; unsupported names are rejected before enqueue; failed applies dead-letter to inbox/failed/ so the queue cannot wedge. Table-driven coverage of accepted mutates.
Advance session engine_revision in run_ui_mutation under the publisher lock across the live engine call so inbox OCC cannot race a later JS note. Dead-letter conflicting and malformed inbox heads to inbox/failed/ so the queue cannot wedge; next sequence can apply. Sync jackControls#61 mutates (move_copy/import_step) into the shared map after rebase onto main.
Reserve captures engine_revision and write rejects a snapshot if it moved during export. Bind the bridge to the active native project-session identity so an A inbox op cannot apply to B.
Rebase jackControls#63 onto jackControls#60 tip f241739. Mutate map entries and ToolSpecs already landed with jackControls#60/jackControls#62; keep unique classifier/lookup coverage so cad_submit accepts the six assembly modeling mutates while assembly_document and assembly_solution stay read-safe inspect tools.
9cb5a63 to
5c2ebc7
Compare
|
Rebased onto new #60 tip |
jackControls
left a comment
There was a problem hiding this comment.
Thanks for the updates. I rechecked the current head (5c2ebc7) and the dirty-state correction looks good. One refresh-state issue remains, and this branch also inherits the unresolved bridge blockers from PR60:
-
A targeted assembly refresh leaves an existing
jointMotionPreviewactive.refreshSessionFromMcp()advancesjointMotionPreviewGenerationand clearsjointPreviewSolution/mechanismPreview, but does not clear the already storedjointMotionPreview. Both viewport paths preferjointMotionPreview.solutionover the newly refreshedassemblySolution, so an external assembly mutation can be applied successfully while the old motion pose continues to render and its controls remain stale. Please setjointMotionPreview: nullin the assembly-refresh path and add a state-level regression test that begins with an active motion preview. -
This PR is stacked on the current PR60 head and therefore includes PR60's two outstanding P1 concurrency issues: the cross-tab publication reservation/write identity race and the non-atomic MCP inbox sequence allocation. Please rebase once those are corrected.
The focused build/tests pass, but they do not cover the stored-preview refresh case.
assembly_create_joint/update return a joint DTO, so applyInboxNow no longer falls through to loadDocument() (dirty:false). Port jackControls#63 refreshAfterInboxApply with dirty:true and clear stale jointMotionPreview.
Drop the extra helper; applyInboxNow uses the same scene+document guard as jackControls#63 so joint DTOs take refreshAfterInboxApply (dirty:true).
Carry reserved session_id + project_session_id on the write path so a delayed A export cannot consume B's reservation. cad_submit allocates inbox sequences with create_new so concurrent submits cannot share a seq.
Rebase jackControls#63 onto jackControls#60 tip f241739. Mutate map entries and ToolSpecs already landed with jackControls#60/jackControls#62; keep unique classifier/lookup coverage so cad_submit accepts the six assembly modeling mutates while assembly_document and assembly_solution stay read-safe inspect tools.
applyInboxNow no longer falls back to loadDocument() (which cleared dirty). Assembly ops get a targeted refreshAfterInboxApply with dirty:true; other non-solid results refresh live state the same way. inboxApplying still guards re-entry; native apply already owns the engine_revision bump.
5c2ebc7 to
c7aba14
Compare
|
Rebased onto #60 |
assembly_create_joint/update return a joint DTO, so applyInboxNow no longer falls through to loadDocument() (dirty:false). Port jackControls#63 refreshAfterInboxApply with dirty:true and clear stale jointMotionPreview.
Drop the extra helper; applyInboxNow uses the same scene+document guard as jackControls#63 so joint DTOs take refreshAfterInboxApply (dirty:true).
There was a problem hiding this comment.
Thanks for the update—the inherited publish/inbox races and dirty-state path are now addressed. One stale-preview blocker remains: refreshAfterInboxApply invalidates jointMotionPreviewGeneration but does not clear jointMotionPreview. The viewport resolves jointMotionPreview?.solution before assemblySolution, so after an attached assembly mutation it can continue rendering the old motion pose despite the refreshed solution. Please add jointMotionPreview: null beside jointPreviewSolution: null (as the combined PR already does) and ideally cover the targeted refresh path with a regression test.
refreshAfterInboxApply invalidated jointMotionPreviewGeneration but left the stored preview in place. Viewport prefers jointMotionPreview.solution over assemblySolution, so an attached assembly mutate could keep rendering the old motion pose. Clear it beside jointPreviewSolution.
|
Addressed the remaining stale-preview blocker.
Cheap regression: New tip: |
assembly_create_joint/update return a joint DTO, so applyInboxNow no longer falls through to loadDocument() (dirty:false). Port jackControls#63 refreshAfterInboxApply with dirty:true and clear stale jointMotionPreview.
Drop the extra helper; applyInboxNow uses the same scene+document guard as jackControls#63 so joint DTOs take refreshAfterInboxApply (dirty:true).
Sixth-pass: no applyInboxAll loadDocument path; leftover refreshAfterInboxApply stays the single jackControls#63 dirty:true contract. Lock cad_refresh vs cad_load_project_model joint parity and a part-document joint inbox typed reject.
The merge-base changed after approval.
df875a0
Summary
feat/mcp-ui-owned-apply/nbcad-mcp-mutate) and brings in the assembly ToolSpecs from feat(mcp): host-neutral assembly component tools #62.MUTATESmap socad_submitaccepts them while attached (UI inbox apply path).assembly_document/assembly_solutionstay read-safe inspect tools (not inbox mutates).Tools added to
crates/mcp-mutate(engine_methodmatcheshost.rs)assembly_create_componentassembly_create_componentassembly_update_componentassembly_update_componentassembly_create_occurrenceassembly_create_occurrenceassembly_update_occurrenceassembly_update_occurrenceassembly_set_occurrence_poseassembly_set_occurrence_poseassembly_set_occurrence_groundedassembly_set_occurrence_groundedStacking / merge order
GitHub could not use
feat/mcp-ui-owned-applyas base onjackControls/noBS-CAD(branch only exists on the fork / #60 head). Please merge after #60 (this branch is based on that tip). Includes #62 assembly ToolSpecs so ToolSpec ↔ mutate map stay in sync; #62 alone is not required if this lands after #60.Do not merge before #60.
Test plan
cargo test -p nbcad-mcp-mutatecargo testinmcp-server(42 passed), including:assembly_create_component_is_in_lookup_mutateassembly_create_component_accepted_by_cad_submit_classifierassembly_component_occurrence_grounded_roundtriptool_spec_mutates_match_shared_inbox_mapevery_shared_mutate_is_accepted_by_cad_submit_classifier