diff --git a/docs/plugin-host-model-completion-plan.md b/docs/plugin-host-model-completion-plan.md index e39ffa5f8..475c529a3 100644 --- a/docs/plugin-host-model-completion-plan.md +++ b/docs/plugin-host-model-completion-plan.md @@ -284,3 +284,16 @@ the joined line without its parts, the two pieces after a break, the stretched a `create_entity("Hatch")`: its command needs an interactive boundary pick that no scripted step can supply. Not wrapped: PEDIT, path and 3-D arrays, and the remaining commands, which stay reachable through `doc.command` and `start_command`. +### 2026-09-21: nested input is validated (vertex validation gap) + +Found while writing the `explode` audit: an `LwPolyline` created from vertices that used the wrong key (`{'x': .., 'y': ..}` instead of +`{'location': ..}`) was accepted and became zero-coordinate vertices. The generated dict-to-struct conversions for nested records +defaulted every absent field and ignored unknown keys, so any typo inside a list of vertices, edges or faces vanished silently. +The generator now (1) refuses an unknown key in every nested record, naming the keys it takes, and (2) enforces a manifest list, +`required_struct_fields`, of the fields that define a nested record's geometry: `LwVertex.location`, `Vertex2D.location`, +`Vertex3D.location`, `Vertex3DPolyline.position`, `PolygonMeshVertex.location`, `PolyfaceVertex.location`, `MLineVertex.position`, +`LineEdge.start/end`, `CircularArcEdge.center/radius`, `EllipticArcEdge.center/major_axis_endpoint`, `PolylineEdge.vertices` and +`SectionSymbolPoint.point`. A `None` counts as missing. Everything else in a nested record still defaults when absent. Evidence: +`audit_python_nested_input_is_validated_over_real_ipc` (eight refusals with the right messages for LwPolyline, Polyline2D and +PolygonMesh; nothing invalid reaches the drawing; the valid forms still create), and every existing per-kind lifecycle audit still passes. + diff --git a/plugins/opencad-python/build/generate.rs b/plugins/opencad-python/build/generate.rs index b8b511810..88043c48e 100644 --- a/plugins/opencad-python/build/generate.rs +++ b/plugins/opencad-python/build/generate.rs @@ -40,6 +40,11 @@ pub struct Manifest { pub base_fields: Vec, #[serde(default)] pub overrides: std::collections::BTreeMap, + /// Fields of nested structs (a polyline vertex, a hatch edge) that must be + /// present in a scripted dict. Everything else in a nested struct still + /// defaults when absent, but an unknown key is always refused. + #[serde(default)] + pub required_struct_fields: std::collections::BTreeMap>, } fn default_base_fields() -> Vec { @@ -668,12 +673,24 @@ fn default_{snake}() -> {path} {{ ) } -fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry) -> String { +fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry, required: &[String]) -> String { let path = type_path(name); let snake = snake_case(name); let mut to_dict_body = String::new(); let mut from_dict_fields = String::new(); let mut default_fields = String::new(); + let mut allowed: Vec = Vec::new(); + for f in &info.fields { + if f.type_id.as_str() != "EntityCommon" { + allowed.push(format!("\"{}\"", f.name)); + } + } + let allowed_list = allowed.join(", "); + let required_list = required + .iter() + .map(|field| format!("\"{field}\"")) + .collect::>() + .join(", "); for f in &info.fields { if f.type_id.as_str() == "EntityCommon" { // A sub-record's own common data (layer, XDATA, handles) is not part @@ -717,6 +734,19 @@ fn gen_struct(name: &str, info: &TypeInfo, registry: &TypeRegistry) -> String { fn dict_to_{snake}(value: PyObjectRef, vm: &VirtualMachine) -> PyResult<{path}> {{ let dict = value.try_into_value::(vm)?; + let allowed: &[&str] = &[{allowed_list}]; + for key in dict.keys_vec() {{ + let key = key.try_into_value::(vm)?; + if !allowed.contains(&key.as_str()) {{ + return Err(vm.new_value_error(format!("{name} has no property {{key:?}}; it takes: {{}}", allowed.join(", ")))); + }} + }} + let required_fields: &[&str] = &[{required_list}]; + for required in required_fields.iter().copied() {{ + if dict.get_item_opt(required, vm)?.map_or(true, |v| vm.is_none(&v)) {{ + return Err(vm.new_value_error(format!("{name} needs {{required}}"))); + }} + }} Ok({path} {{ {from_dict_fields} }}) }} @@ -1050,7 +1080,16 @@ pub fn generate_entity_crud(manifest: &Manifest, registry: &TypeRegistry) -> Str Classify::UnitEnum => out.push_str(&gen_unit_enum(&name, info)), Classify::TaggedEnum => out.push_str(&gen_tagged_enum(&name, info, registry)), Classify::BitFlags => out.push_str(&gen_bitflags(&name, info)), - Classify::Struct => out.push_str(&gen_struct(&name, info, registry)), + Classify::Struct => { + let required = manifest.required_struct_fields.get(&name).cloned().unwrap_or_default(); + for field in &required { + assert!( + info.fields.iter().any(|f| &f.name == field), + "entity_manifest.json: required_struct_fields: {name} has no field {field}" + ); + } + out.push_str(&gen_struct(&name, info, registry, &required)) + } } } diff --git a/plugins/opencad-python/entity_manifest.json b/plugins/opencad-python/entity_manifest.json index 298087fed..a714143e4 100644 --- a/plugins/opencad-python/entity_manifest.json +++ b/plugins/opencad-python/entity_manifest.json @@ -973,5 +973,46 @@ } } } + }, + "required_struct_fields": { + "LwVertex": [ + "location" + ], + "Vertex2D": [ + "location" + ], + "Vertex3D": [ + "location" + ], + "Vertex3DPolyline": [ + "position" + ], + "PolygonMeshVertex": [ + "location" + ], + "PolyfaceVertex": [ + "location" + ], + "MLineVertex": [ + "position" + ], + "LineEdge": [ + "start", + "end" + ], + "CircularArcEdge": [ + "center", + "radius" + ], + "EllipticArcEdge": [ + "center", + "major_axis_endpoint" + ], + "PolylineEdge": [ + "vertices" + ], + "SectionSymbolPoint": [ + "point" + ] } } diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index 50be57934..95de91941 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -8159,6 +8159,61 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0))) assert!(!moved(&app.tabs[0].scene.document, 1007.0)); } + /// A scripted dict is checked all the way down: a nested record (a polyline + /// vertex, a mesh vertex) may not carry an unknown key or omit the field that + /// defines its geometry, so a typo never turns into a silent zero coordinate. + #[test] + fn audit_python_nested_input_is_validated_over_real_ipc() { + let Some(plugin_path) = std::env::var_os("OCS_TEST_PYTHON_PLUGIN") else { + return; + }; + let mut app = OpenCADStudio::new_for_test(); + app.tabs[0].is_start = false; + let mut host = HostSession::new(&mut app, 0); + let process = ocs_plugin_api::process::PluginProcess::spawn( + std::path::Path::new(&plugin_path), &mut host, crate::plugin::v4_support::notification_handler(), + ).unwrap(); + let script = std::env::temp_dir().join(format!("ocs_nested_{}.py", std::process::id())); + std::fs::write(&script, concat!( + "def P(x, y, z=0.0): return {'x': x, 'y': y, 'z': z}\n", + "doc = ocs.active_document\nL = doc.layers\n", + "refused, problems = [], []\n", + "def check(tag, fn, expect):\n", + " try:\n fn()\n except (RuntimeError, TypeError, ValueError) as error:\n", + " if expect in str(error):\n refused.append(tag)\n", + " else:\n problems.append('WRONGMESSAGE_' + tag)\n", + " else:\n problems.append('ACCEPTED_' + tag)\n", + "def lw(vertices): return lambda: doc.create_entity('LwPolyline', vertices=vertices)\n", + "ok = {'location': P(0, 0)}\n", + "check('lw_xy_keys', lw([ok, {'x': 5, 'y': 5}]), 'no property')\n", + "check('lw_empty_vertex', lw([ok, {}]), 'needs location')\n", + "check('lw_null_location', lw([ok, {'location': None}]), 'needs location')\n", + "check('lw_typo', lw([ok, {'location': P(1, 1), 'bulgee': 0.5}]), 'no property')\n", + "check('lw_message_lists_keys', lw([ok, {'lokation': P(1, 1)}]), 'location')\n", + "check('poly2d_missing_location', lambda: doc.create_entity('Polyline2D', vertices=[{'flags': 0}, {'location': P(1, 1)}]), 'needs location')\n", + "check('poly2d_typo', lambda: doc.create_entity('Polyline2D', vertices=[{'location': P(0, 0)}, {'loc': P(1, 1)}]), 'no property')\n", + "check('mesh_missing_location', lambda: doc.create_entity('PolygonMesh', vertices=[{}]), 'needs location')\n", + "good = doc.create_entity('LwPolyline', vertices=[ok, {'location': P(10, 0), 'bulge': 0.5}, {'location': P(10, 10)}])\n", + "good2 = doc.create_entity('Polyline2D', vertices=[{'location': P(0, 0)}, {'location': P(5, 5)}])\n", + "L.create('REPORT ' + str(len(refused)) + ' ~ ' + ' '.join(problems))\n", + )).unwrap(); + assert!(process.dispatch(&mut host, &format!("PY_RUN {}", script.display()), &mut |_| {}).unwrap()); + let _ = std::fs::remove_file(&script); + let name = host.document().layers.iter().map(|l| l.name.clone()).filter(|n| n.starts_with("REPORT")).last().expect("script report layer"); + let (refused, problems) = name["REPORT".len()..].split_once('~').unwrap(); + assert_eq!(problems.trim(), "", "problems: {problems}"); + assert_eq!(refused.trim(), "8"); + // Nothing invalid reached the drawing; the two valid polylines did. + let document = host.document(); + let lw: Vec<_> = document.entities().filter_map(|e| match e { EntityType::LwPolyline(p) => Some(p.clone()), _ => None }).collect(); + assert_eq!(lw.len(), 1, "only the valid LwPolyline was created"); + assert_eq!(lw[0].vertices.len(), 3); + assert_eq!(lw[0].vertices[1].bulge, 0.5); + assert_eq!(document.entities().filter(|e| matches!(e, EntityType::Polyline2D(_))).count(), 1); + assert!(!document.entities().any(|e| matches!(e, EntityType::PolygonMesh(_)))); + drop(process); + } + /// Exploration harness (not a gate): prints how the real commands answer /// each step so the wrappers are written against observed prompts. #[test]