From 7b6d54d15be32633c56c61de78cea16674673d24 Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Fri, 31 Jul 2026 15:23:39 +0800 Subject: [PATCH 1/6] fix(core): validate nested modules during module checks --- libraries/core/src/descriptor/expand.rs | 60 ++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 43cb3f028..8333d3687 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -163,7 +163,19 @@ pub fn check_module_file(module_path: &Path) -> eyre::Result<()> { let canonical = module_path .canonicalize() .with_context(|| format!("module file not found: {}", module_path.display()))?; - let module_file = load_module_file(&canonical)?; + let mut seen = HashSet::new(); + check_module_file_inner(&canonical, &mut seen) +} + +fn check_module_file_inner(canonical: &Path, seen: &mut HashSet) -> eyre::Result<()> { + if !seen.insert(canonical.to_path_buf()) { + bail!( + "circular module reference detected while checking module file: {}", + canonical.display() + ); + } + + let module_file = load_module_file(canonical)?; validate_module_header(&module_file.module)?; let module_dir = canonical .parent() @@ -240,6 +252,7 @@ pub fn check_module_file(module_path: &Path) -> eyre::Result<()> { &nested_module.module, &node.inputs, )?; + check_module_file_inner(&nested_canonical, seen)?; for output in &nested_module.module.outputs { inner_outputs.insert(output.to_string()); } @@ -257,6 +270,7 @@ pub fn check_module_file(module_path: &Path) -> eyre::Result<()> { } } + seen.remove(canonical); Ok(()) } @@ -2304,6 +2318,50 @@ nodes: assert!(msg.contains("nested"), "got: {msg}"); } + #[test] + fn check_module_file_rejects_invalid_nested_module() { + let tmp = TempDir::new().unwrap(); + + write_file( + tmp.path(), + "leaf.yml", + r#" +module: + name: leaf + inputs: [] + outputs: [out] + +nodes: + - id: worker + path: worker.py + outputs: + - other +"#, + ); + + let path = write_file( + tmp.path(), + "outer.yml", + r#" +module: + name: outer + inputs: [] + outputs: [out] + +nodes: + - id: nested + module: leaf.yml +"#, + ); + + let result = check_module_file(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("leaf"), "got: {msg}"); + assert!(msg.contains("out"), "got: {msg}"); + assert!(msg.contains("no inner node produces it"), "got: {msg}"); + } + /// Regression test for #2851: `check_module_file` must accept a nested /// module reference that points to a sibling directory inside the same /// project (e.g. `../shared/base.yml`). The real expansion path From e3bd3adc9b1f0748094ee0186491f416fe4279df Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Fri, 31 Jul 2026 15:28:41 +0800 Subject: [PATCH 2/6] fix(core): reject unknown module header fields --- libraries/core/src/descriptor/expand.rs | 28 +++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 8333d3687..00a4eba4f 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -40,6 +40,7 @@ type ModuleOutputMap = BTreeMap; /// Header section of a module definition file. #[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] struct ModuleHeader { name: String, #[serde(default)] @@ -2272,6 +2273,33 @@ nodes: assert!(result.unwrap_err().to_string().contains("missing")); } + #[test] + fn check_module_file_rejects_unknown_module_header_field() { + let tmp = TempDir::new().unwrap(); + let path = write_file( + tmp.path(), + "unknown_header_field.yml", + r#" +module: + name: bad + inputz: [data] + outputs: [out] + +nodes: + - id: worker + path: worker.py + outputs: + - out +"#, + ); + + let result = check_module_file(&path); + assert!(result.is_err()); + let msg = format!("{:?}", result.unwrap_err()); + assert!(msg.contains("inputz"), "got: {msg}"); + assert!(msg.contains("unknown field"), "got: {msg}"); + } + #[test] fn check_module_file_rejects_nested_module_missing_required_input() { let tmp = TempDir::new().unwrap(); From 9434b79b950c8350a9911c4e6be85a4c17eccee6 Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Fri, 31 Jul 2026 15:31:54 +0800 Subject: [PATCH 3/6] fix(core): reject ambiguous module outputs --- libraries/core/src/descriptor/expand.rs | 166 ++++++++++++++++++++---- 1 file changed, 143 insertions(+), 23 deletions(-) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 00a4eba4f..e11d86d4a 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -206,12 +206,15 @@ fn check_module_file_inner(canonical: &Path, seen: &mut HashSet) -> eyr // Runtime (operator) and legacy custom nodes declare their outputs in // config.outputs / run_config.outputs rather than the node-level `outputs` // set, so `node_output_refs` collects those too (see #2817). - let mut inner_outputs: BTreeSet = module_file - .nodes - .iter() - .filter(|n| n.module.is_none()) - .flat_map(|n| node_output_refs(n).into_iter().map(|(name, _)| name)) - .collect(); + let mut inner_outputs: BTreeMap> = BTreeMap::new(); + for node in module_file.nodes.iter().filter(|n| n.module.is_none()) { + for (name, output_ref) in node_output_refs(node) { + inner_outputs + .entry(name) + .or_default() + .push(format!("{}/{}", node.id, output_ref)); + } + } // Check nested module files exist and collect their declared outputs for node in &module_file.nodes { @@ -255,19 +258,33 @@ fn check_module_file_inner(canonical: &Path, seen: &mut HashSet) -> eyr )?; check_module_file_inner(&nested_canonical, seen)?; for output in &nested_module.module.outputs { - inner_outputs.insert(output.to_string()); + inner_outputs + .entry(output.to_string()) + .or_default() + .push(format!("{}/{}", node.id, output)); } } } for declared_output in &module_file.module.outputs { let output_str = declared_output.to_string(); - if !inner_outputs.contains(&output_str) { - bail!( - "module `{}` declares output `{}` but no inner node produces it", - module_file.module.name, - declared_output, - ); + match inner_outputs.get(&output_str) { + None => { + bail!( + "module `{}` declares output `{}` but no inner node produces it", + module_file.module.name, + declared_output, + ); + } + Some(producers) if producers.len() > 1 => { + bail!( + "module `{}` declares output `{}` but multiple inner nodes produce it: {}", + module_file.module.name, + declared_output, + producers.join(", "), + ); + } + Some(_) => {} } } @@ -680,24 +697,47 @@ fn expand_module_node( let mut output_map = ModuleOutputMap::new(); for declared_output in &module_file.module.outputs { let declared = declared_output.to_string(); - let target = final_nodes + let targets = final_nodes .iter() - .find_map(|n| { + .flat_map(|n| { + let declared = declared.clone(); node_output_refs(n) .into_iter() - .find(|(name, _)| *name == declared) - .map(|(_, output_ref)| UserInputMapping { - source: n.id.clone(), - output: output_ref.into(), + .filter(move |(name, _)| *name == declared) + .map(|(_, output_ref)| { + ( + format!("{}/{}", n.id, output_ref), + UserInputMapping { + source: n.id.clone(), + output: output_ref.into(), + }, + ) }) }) - .ok_or_else(|| { - eyre::eyre!( + .collect::>(); + let target = match targets.as_slice() { + [] => { + return Err(eyre::eyre!( "module `{}` declares output `{}` but no inner node produces it", module_file.module.name, declared_output, - ) - })?; + )); + } + [(_, target)] => target.clone(), + _ => { + let producers = targets + .iter() + .map(|(producer, _)| producer.as_str()) + .collect::>() + .join(", "); + bail!( + "module `{}` declares output `{}` but multiple inner nodes produce it: {}", + module_file.module.name, + declared_output, + producers, + ); + } + }; output_map.insert(declared, target); } @@ -2273,6 +2313,39 @@ nodes: assert!(result.unwrap_err().to_string().contains("missing")); } + #[test] + fn check_module_file_rejects_ambiguous_declared_output() { + let tmp = TempDir::new().unwrap(); + let path = write_file( + tmp.path(), + "ambiguous_output.yml", + r#" +module: + name: ambiguous + inputs: [] + outputs: [out] + +nodes: + - id: first + path: first.py + outputs: + - out + - id: second + path: second.py + outputs: + - out +"#, + ); + + let result = check_module_file(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("out"), "got: {msg}"); + assert!(msg.contains("multiple"), "got: {msg}"); + assert!(msg.contains("first"), "got: {msg}"); + assert!(msg.contains("second"), "got: {msg}"); + } + #[test] fn check_module_file_rejects_unknown_module_header_field() { let tmp = TempDir::new().unwrap(); @@ -3094,6 +3167,53 @@ nodes: assert_eq!(mapping.output.to_string(), "result"); } + #[test] + fn expand_rejects_ambiguous_module_output() { + let tmp = TempDir::new().unwrap(); + let base = tmp.path(); + + write_file( + base, + "mod.yml", + r#" +module: + name: ambiguous + inputs: [] + outputs: [out] + +nodes: + - id: first + path: first.py + outputs: + - out + - id: second + path: second.py + outputs: + - out +"#, + ); + + let desc = parse_descriptor( + r#" +nodes: + - id: m + module: mod.yml + - id: sink + path: sink.py + inputs: + value: m/out +"#, + ); + + let result = expand_modules(&desc, base); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("out"), "got: {msg}"); + assert!(msg.contains("multiple"), "got: {msg}"); + assert!(msg.contains("m.first"), "got: {msg}"); + assert!(msg.contains("m.second"), "got: {msg}"); + } + /// Regression test for #2817: `check_module_file` must accept a declared /// output produced by an operator or legacy custom inner node, and still /// reject one nothing produces. From 452255ae34b7f88c7062321ddc6ea0238099f70b Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Fri, 31 Jul 2026 15:34:33 +0800 Subject: [PATCH 4/6] fix(core): enforce module check depth limit --- libraries/core/src/descriptor/expand.rs | 56 +++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index e11d86d4a..4add9575d 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -165,10 +165,21 @@ pub fn check_module_file(module_path: &Path) -> eyre::Result<()> { .canonicalize() .with_context(|| format!("module file not found: {}", module_path.display()))?; let mut seen = HashSet::new(); - check_module_file_inner(&canonical, &mut seen) + check_module_file_inner(&canonical, 0, &mut seen) } -fn check_module_file_inner(canonical: &Path, seen: &mut HashSet) -> eyre::Result<()> { +fn check_module_file_inner( + canonical: &Path, + depth: u8, + seen: &mut HashSet, +) -> eyre::Result<()> { + if depth >= MAX_MODULE_DEPTH { + bail!( + "module nesting exceeds depth limit of {MAX_MODULE_DEPTH} while checking module file: {}", + canonical.display() + ); + } + if !seen.insert(canonical.to_path_buf()) { bail!( "circular module reference detected while checking module file: {}", @@ -256,7 +267,7 @@ fn check_module_file_inner(canonical: &Path, seen: &mut HashSet) -> eyr &nested_module.module, &node.inputs, )?; - check_module_file_inner(&nested_canonical, seen)?; + check_module_file_inner(&nested_canonical, depth + 1, seen)?; for output in &nested_module.module.outputs { inner_outputs .entry(output.to_string()) @@ -2463,6 +2474,45 @@ nodes: assert!(msg.contains("no inner node produces it"), "got: {msg}"); } + #[test] + fn check_module_file_rejects_depth_limit() { + let tmp = TempDir::new().unwrap(); + let base = tmp.path(); + + for i in 0..=MAX_MODULE_DEPTH { + let next = if i < MAX_MODULE_DEPTH { + format!(" - id: inner\n module: level{}_module.yml", i + 1) + } else { + " - id: worker\n path: worker.py\n outputs:\n - out".to_string() + }; + + write_file( + base, + &format!("level{i}_module.yml"), + &format!( + r#" +module: + name: level{i} + inputs: [] + outputs: [out] + +nodes: +{next} +"# + ), + ); + } + + let result = check_module_file(&base.join("level0_module.yml")); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("nesting exceeds depth limit") + ); + } + /// Regression test for #2851: `check_module_file` must accept a nested /// module reference that points to a sibling directory inside the same /// project (e.g. `../shared/base.yml`). The real expansion path From e63bf609f2de34e5d6ac8b15d43de7d69f3b7186 Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Fri, 31 Jul 2026 15:38:30 +0800 Subject: [PATCH 5/6] fix(core): preserve nested module output boundaries --- libraries/core/src/descriptor/expand.rs | 100 ++++++++++++++++++------ 1 file changed, 78 insertions(+), 22 deletions(-) diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 4add9575d..3670567f3 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -675,6 +675,8 @@ fn expand_module_node( // Collect nested output maps so sibling nodes can reference nested module // outputs correctly via rewrite_external_refs. let mut nested_output_maps: BTreeMap = BTreeMap::new(); + let mut direct_output_targets: BTreeMap> = + BTreeMap::new(); let mut final_nodes = Vec::new(); for inner_node in prefixed_nodes { if inner_node.module.is_some() { @@ -689,9 +691,24 @@ fn expand_module_node( prepend_module_build_to_node(nested_node, outer_build); } } + for (output, target) in &nested_omap { + direct_output_targets + .entry(output.clone()) + .or_default() + .push((format!("{nested_id}/{output}"), target.clone())); + } nested_output_maps.insert(nested_id, nested_omap); final_nodes.extend(nested); } else { + for (name, output_ref) in node_output_refs(&inner_node) { + direct_output_targets.entry(name).or_default().push(( + format!("{}/{}", inner_node.id, output_ref), + UserInputMapping { + source: inner_node.id.clone(), + output: output_ref.into(), + }, + )); + } final_nodes.push(inner_node); } } @@ -708,34 +725,16 @@ fn expand_module_node( let mut output_map = ModuleOutputMap::new(); for declared_output in &module_file.module.outputs { let declared = declared_output.to_string(); - let targets = final_nodes - .iter() - .flat_map(|n| { - let declared = declared.clone(); - node_output_refs(n) - .into_iter() - .filter(move |(name, _)| *name == declared) - .map(|(_, output_ref)| { - ( - format!("{}/{}", n.id, output_ref), - UserInputMapping { - source: n.id.clone(), - output: output_ref.into(), - }, - ) - }) - }) - .collect::>(); - let target = match targets.as_slice() { - [] => { + let target = match direct_output_targets.get(&declared).map(Vec::as_slice) { + None | Some([]) => { return Err(eyre::eyre!( "module `{}` declares output `{}` but no inner node produces it", module_file.module.name, declared_output, )); } - [(_, target)] => target.clone(), - _ => { + Some([(_, target)]) => target.clone(), + Some(targets) => { let producers = targets .iter() .map(|(producer, _)| producer.as_str()) @@ -3264,6 +3263,63 @@ nodes: assert!(msg.contains("m.second"), "got: {msg}"); } + #[test] + fn expand_rejects_nested_module_private_output_export() { + let tmp = TempDir::new().unwrap(); + let base = tmp.path(); + + write_file( + base, + "leaf.yml", + r#" +module: + name: leaf + inputs: [] + outputs: [public] + +nodes: + - id: worker + path: worker.py + outputs: + - public + - private +"#, + ); + + write_file( + base, + "outer.yml", + r#" +module: + name: outer + inputs: [] + outputs: [private] + +nodes: + - id: nested + module: leaf.yml +"#, + ); + + let desc = parse_descriptor( + r#" +nodes: + - id: m + module: outer.yml + - id: sink + path: sink.py + inputs: + value: m/private +"#, + ); + + let result = expand_modules(&desc, base); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("private"), "got: {msg}"); + assert!(msg.contains("no inner node produces it"), "got: {msg}"); + } + /// Regression test for #2817: `check_module_file` must accept a declared /// output produced by an operator or legacy custom inner node, and still /// reject one nothing produces. From 1c6da3b775f9c7097eb790e528b0977dae266c87 Mon Sep 17 00:00:00 2001 From: SunSunSun689 Date: Thu, 13 Aug 2026 10:48:14 +0800 Subject: [PATCH 6/6] fix: align module validation docs --- docs/modules.md | 17 ++++++++++++----- libraries/core/src/descriptor/expand.rs | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/modules.md b/docs/modules.md index 17e2d4187..370c6b4ae 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -63,6 +63,9 @@ A module file has two sections: | `inputs_optional` | list | no | Optional input ports (silently skipped if not wired) | | `outputs` | list | no | Output port names exposed to the parent dataflow | +Unknown fields in the module header are rejected whenever Dora loads the +module, including `dora expand`, `dora build`, and `dora run`. + ### `nodes:` list Standard node definitions, with one special syntax: **`_mod/port_name`** references a module input port. When expanded, `_mod/port_name` is replaced with whatever the parent wired to that port. @@ -151,7 +154,7 @@ Parameters are also injected as environment variables (`PARAM_SPEED`, `PARAM_MOD 2. Prefix all internal node IDs with `{module_id}.` (e.g., `nav_stack.planner`) 3. Replace `_mod/port_name` references with the actual sources from the parent's input map 4. Rewrite internal cross-references (e.g., `planner/path` becomes `nav_stack.planner/path`) -5. Map module-declared outputs to internal node outputs, so `nav_stack/cmd_vel` resolves to `nav_stack.controller/cmd_vel`. An inner node may produce a declared output from its node-level `outputs:`, from an `operator:`/`operators:` block, or from a legacy `custom:` block. For an output produced by one operator of a multi-operator `operators:` node, the resolved reference keeps the operator segment that runtime nodes require: `nav_stack/cmd_vel` resolves to `nav_stack.runtime/controller/cmd_vel` +5. Map module-declared outputs to direct child outputs, so `nav_stack/cmd_vel` resolves to `nav_stack.controller/cmd_vel`. A direct child may be a standard node, a runtime `operator:`/`operators:` node, a legacy `custom:` node, or a nested module. For an output produced by one operator of a multi-operator `operators:` node, the resolved reference keeps the operator segment that runtime nodes require: `nav_stack/cmd_vel` resolves to `nav_stack.runtime/controller/cmd_vel` 6. Replace the module node with the expanded flat nodes 7. Substitute `params:` values in `args:` fields and inject as env vars @@ -188,6 +191,11 @@ nodes: After expansion, node IDs are fully qualified: `outer.inner.some_node`. +Only outputs declared by a nested module are visible to its parent. A parent +module can wire or re-export `inner/processed` from the example above because +`processed` is listed in `inner_module.yml`'s `module.outputs`; it cannot reach +private outputs produced by nodes inside `inner_module.yml`. + ## Optional Inputs Declare inputs as optional when a module should work with or without certain connections: @@ -229,11 +237,10 @@ dora expand --module modules/transform_module.yml This checks: - Valid YAML structure -- Module header is present with `name`, `inputs`, `outputs` +- Module header is present with required `name`, optional `inputs`/`outputs`, and no unknown header fields - All `_mod/` references correspond to declared inputs or optional inputs -- Every declared output is produced by some inner node (counting `operator:`/`operators:` and legacy `custom:` outputs) -- No duplicate node IDs -- Internal wiring is consistent +- Every declared output is produced by a direct child node or by a nested module's declared output (counting `operator:`/`operators:` and legacy `custom:` outputs) +- Nested module files exist, are relative paths, are acyclic, and stay within the nesting depth limit ## Security diff --git a/libraries/core/src/descriptor/expand.rs b/libraries/core/src/descriptor/expand.rs index 3670567f3..99786d5c9 100644 --- a/libraries/core/src/descriptor/expand.rs +++ b/libraries/core/src/descriptor/expand.rs @@ -155,7 +155,7 @@ pub fn expand_modules_with_boundaries( /// Validate a module file in isolation without expanding it into a dataflow. /// /// Checks: -/// - Module header is well-formed (name, inputs, outputs) +/// - Module header is well-formed (required name, optional inputs/outputs) /// - All inner nodes are parseable /// - All `_mod/X` references point to declared inputs or optional inputs /// - All declared outputs are produced by some inner node (or nested module)