Fix module check nested validity

[SunSunSun689]

Summary

Tighten module validation so dora expand --module and real dataflow
expansion handle nested modules consistently.

Changes

• Recursively validate nested module files in check_module_file.

  • Reason: dora expand --module outer.yml could report an outer module
    as valid even when a nested module was invalid.

• Reject unknown fields inside the module: header.

  • Reason: typos like inputz were silently ignored instead of producing
    a parse error.

• Reject ambiguous module outputs.

  • Reason: if multiple inner nodes produced the same declared module
    output, expansion silently picked the first producer.

• Apply the existing module nesting depth limit to standalone module
  checks.

  • Reason: real dataflow expansion rejected over-deep module graphs, but
    dora expand --module accepted them.

• Preserve nested module output boundaries during real expansion.

  • Reason: a parent module could export an output from inside a nested
    module even if that nested module did not declare the output publicly.

[trunk-io]

✨ Submitted to Merge by @phil-opp. It will be added to the merge queue once all branch protection rules pass and there are no merge conflicts with the target branch. See more details here.

[phil-opp]

Automated review by Claude. No issues found.

This is the largest of the module-validation PRs and it holds up on close reading:

• #[serde(deny_unknown_fields)] on ModuleHeader catches header typos (e.g. inputz), covered by check_module_file_rejects_unknown_module_header_field.
• check_module_file becomes recursive via check_module_file_inner with a depth limit and a seen set. The success path calls seen.remove(canonical), so diamond dependencies (the same module referenced by two siblings) are still accepted while genuine cycles are rejected — a nice distinction. The depth test drives a linear chain of MAX_MODULE_DEPTH+1 and reaches the bail correctly.
• Declared outputs are tracked per-producer (BTreeMap<String, Vec<String>>), so an ambiguous declared output (>1 inner producer) is now rejected instead of silently resolving to the first match — enforced in both check_module_file and expand_module_node (direct_output_targets).
• Nested-module output boundaries are enforced: expand_module_node builds direct_output_targets from each nested module's declared output map (nested_omap) rather than scanning fully-expanded leaf nodes, so a parent can no longer re-export a nested module's private (non-declared) output. Real behavior fix, covered by expand_rejects_nested_module_private_output_export.
• check_inner_node_output_refs / check_nested_module_output_refs validate that inner input references point to outputs actually declared by the referenced sibling node / nested module. These stay consistent with node_output_refs (operator outputs addressed as op_id/output, others bare), so they don't false-positive on valid multi-operator references, and _mod/ / unknown sources are safely skipped.

Tests are meaningful and cover each new rejection path. One minor, non-blocking observation: check_module_file_inner calls load_module_file(&nested_canonical) to collect outputs and then recurses (which loads it again) — a redundant parse, not a correctness problem.

---

🤖 This is a fully automated review by Claude (Claude Code). No human has vetted these findings — please verify before acting on them.

Generated by Claude Code

---

Generated by Claude Code

[phil-opp]

🤖 Automated review by Claude — this comment is fully automated and has not been vetted by a human.

No issues in the diff itself — the ambiguous-output detection and the nested private-output boundary check look correct, and the added tests assert specific error substrings and would fail if the checks were removed.

One cross-PR hazard worth flagging, since this overlaps several sibling PRs editing the same function: this PR moves the body of check_module_file into the new check_module_file_inner, which loads the module file but does not call validate_module_header (added by #2907) or check_nested_module_required_inputs (added by #2912). Both of those PRs insert their calls into the old check_module_file at exactly this region. Whichever lands second will conflict here, and if this rewrite is taken as-is, those validations are dropped from the check_module_file path (the required/optional-input-conflict check and the nested-required-input check). Whoever merges should re-add those calls into check_module_file_inner and the nested-recursion site so the checks aren't lost.

More broadly, this and the other module PRs in the #2901–#2913 range all edit check_module_file / expand_module_node / substitute_params_in_node from the same base and won't apply as a clean independent stack — they'd be safer landed as one coordinated series with the overlaps reconciled.

---

Generated by Claude Code

[phil-opp]

docs/modules.md still documents the old rule. Its wiring checks duplicate #2903's, which are better — drop them. Also deny_unknown_fields on ModuleHeader applies to dora run, not just the linter.

[phil-opp]

🤖 Automated review by Claude — fully automated, no human reviewed this; please verify before acting.

The commits since my last review (docs alignment in docs/modules.md, plus the main merge and its formatting repair) look safe and address the outstanding points:

• The cross-PR hazard I flagged earlier did not materialize: check_module_file_inner in libraries/core/src/descriptor/expand.rs still calls validate_module_header before recursing, so the required/optional-input conflict check is preserved through the merge.
• The duplicate wiring checks are gone from the final diff (no check_inner_node_output_refs / check_nested_module_output_refs), leaving fix(core): validate module file internal wiring #2903's checks as the single source.
• docs/modules.md now matches the implemented rules (direct-child / nested-declared outputs, ambiguity rejection, and deny_unknown_fields applying to dora run / dora build, not just the linter).

No new issues in the diff.

---

Generated by Claude Code

[phil-opp]

🤖 This is a fully automated review by Claude. No human has verified these findings.

Re the earlier "cross-PR hazard" note — I don't think it's fully resolved, and I believe the most recent "did not materialize" conclusion checked the wrong function.

#2912 (merged to main ~11:21 UTC on 2026-08-13) added a check_nested_module_required_inputs call inside check_module_file, so the standalone linter rejects an outer module that fails to wire a nested module's required inputs. This branch merged main in earlier (~08:07 UTC), before #2912 landed, and rewrote the body into check_module_file_inner (libraries/core/src/descriptor/expand.rs:171), which calls validate_module_header but not check_nested_module_required_inputs (that function doesn't exist on this branch).

Consequences:

• Merging into current main will conflict in check_module_file; taking this rewrite as-is silently drops fix(core): validate nested module required inputs #2912's nested-required-input validation.
• Even setting the merge aside, it leaves a check-vs-run divergence: expand_module_node still enforces required inputs for module nodes (expand.rs:502), so dora run rejects a missing nested required input while dora expand --module would accept it — the same class of inconsistency this PR aims to close.

The prior "did not materialize" conclusion appears to have verified validate_module_header (the required/optional-input conflict check within a single header) rather than check_nested_module_required_inputs (#2912's distinct check that the parent provides a nested module's required inputs) — these are separate functions.

Suggest rebasing on current main and re-adding the check_nested_module_required_inputs call into check_module_file_inner's nested-module loop (~expand.rs:262).

Separately: the recursion/cycle/depth logic and the new ambiguity + private-output-boundary checks look correct and are well-tested.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review by Claude — fully automated, no human in the loop; treat the findings as suggestions to verify rather than as authority.

Re-reviewed after the rewrite (6 commits, +354/-35). This is a clear improvement on the previous single-commit version, and the earlier concern that it duplicated #2903 no longer applies — the rewrite dropped the wiring checks entirely, so #2903's collect_module_source_outputs / check_internal_wiring / duplicate-node-id detection now have no counterpart here. Both should land. They touch the same region of check_module_file, so expect a textual conflict.

I checked each commit against what is already on main (#2906, #2908, #2910, #2912 are all in this PR's base): none of the six duplicates them, and the new recursive path correctly calls validate_module_header and check_nested_module_required_inputs rather than re-implementing them.

Nothing below is a correctness objection.

1. The ambiguity rule is undocumented, and has no escape hatch

9434b79b9 changes dora run / build / check, not just the linter: a declared output with more than one producer is now a hard error where it was previously first-wins (expand.rs:728-750). The doc commit 1c6da3b77 covers the sibling boundary rule but not this one — neither step 5 nor the Validation list in docs/modules.md mentions it.

Worth documenting the consequence as well as the rule: module.outputs is a bare list, with no outputs: {out: nodeA/out} mapping syntax. So a module in which an internal signal happens to share a name with an exported one is now inexpressible — the author has to rename the internal one. The error message doesn't say that, and it is not obvious.

No in-tree descriptor regresses: examples/module-dataflow/modules/transform_module.yml has a single producer for its one declared output, and it is the only module file in the tree.

2. The seen handling is untested in both directions

452255ae3 and 7b6d54d15 add a cycle branch (expand.rs:183-188) and rely on seen.remove(canonical) at expand.rs:302 to keep diamonds (A→B→D, A→C→D) working. Neither is covered: grepping for circular only finds expand_circular_reference, which is the pre-existing expand-path test.

That remove is a one-line invariant that a future refactor would silently break, and it now sits on the success path only, so it is more fragile than before. Two small tests would pin it — one asserting A→B→A (or a self-include) is rejected, one asserting a diamond is still accepted.

3. guide/src/concepts/modules.md was not updated

It is a near-verbatim copy of docs/modules.md (the two already differ by ~2 lines from the #2817 edit). This PR edits exactly the paragraphs that are duplicated there, so the published mdBook copy drifts further. Mirroring the edit would keep them in step.

4. Please label the breaking changes in the PR description

Three tightenings reach the runtime path, not just the linter:

• ambiguous declared output: first-wins → hard error (expand.rs:728-750)
• a parent module can no longer re-export a nested module's undeclared output (expand.rs:694-711)
• #[serde(deny_unknown_fields)] on ModuleHeader (expand.rs:43) applies via load_module_file, which expand_module_node also uses (expand.rs:522) — so a third-party module file carrying e.g. description: in its header stops loading under dora run

On that last one: main already denies unknown fields at the ModuleFile level, and the four documented header fields exactly match ModuleHeader, so this removes an inconsistency rather than inventing a rule, and nothing in-tree or documented breaks. It is still worth calling out for anyone with module files outside this repo.

Smaller items

• expand.rs:721-724 — the comment still says "build output map from fully-expanded flat nodes", which is precisely what e63bf609f stopped doing.
• The cycle error at expand.rs:184-187 omits the offending node id and the "check that module files do not reference each other in a cycle" hint that the expand-path equivalent gives.
• Every nested file is still parsed twice per reference (load_module_file at expand.rs:263, then again at :190 inside the recursion), and the recursion adds no memoisation, so a shared module is re-walked once per path. Lint-only, so a fair follow-up rather than a blocker.
• expand.rs:730 uses return Err(eyre::eyre!(...)) while :743 uses bail! in the same match; worth picking one.

Coordination note

Whichever of this PR and #2903 lands second should restore the two docs/modules.md bullets removed here — "No duplicate node IDs" and "Internal wiring is consistent". They were inaccurate on main at the time (check_module_file did neither), which is why removing them was right; #2903 is what makes them true.

[phil-opp]

🤖 Automated review by Claude (fully automated — not yet checked by a human maintainer)

Re-checked after the 2026-08-18 merge from main, which resolved the conflict in libraries/core/src/descriptor/expand.rs. The merge is clean: check_module_file_inner now sits alongside #2903's collect_module_source_outputs / check_internal_wiring (called at expand.rs:213-218), and still invokes validate_module_header (190) and check_nested_module_required_inputs (269) before recursing — so the earlier cross-PR concern about dropped nested-required-input validation did not materialize. The ambiguity detection, nested-module private-output boundary enforcement (direct_output_targets), depth/cycle handling with seen.remove on the success path, and the added tests are all intact.

No new findings.

---

Generated by Claude Code

[phil-opp]

Automated review (Claude) · This is a fully automated review; it has not been vetted by a human.

Following up after the merge from main (a3a66ac). The one actionable concern from the earlier reviews — that this rewrite of the nested-module checks might drop #2912's check_nested_module_required_inputs — did not materialize: the merge integrated it, and check_module_file_inner now runs validate_module_header, the internal-wiring check (#2903), and the nested-required-inputs check (#2912) before recursing under the depth/seen guards. No new correctness issues from the merge; the accept/reject paths for the nested private-output boundary remain correctly tested.

The remaining items from the prior review are non-blocking: the first-wins ambiguity rule has no documented outputs: escape hatch and isn't mirrored in guide/src/concepts/modules.md, and the seen cycle/diamond path has no direct test.

---

Generated by Claude Code

[phil-opp]

Merged main (clean). Verified the recursion empirically rather than by reading: self-include, mutual cycle and non-root cycle all reject without hanging or overflowing; diamonds and repeated sibling modules still pass; and check-path/expand-path depth parity is exact (8 files OK, 9 fail on both). 296 tests pass, clippy clean.

Pushed fixes for the items still open from the 2026-08-17 review:

• docs/modules.md deleted two bullets that are now true. The diff removes "No duplicate node IDs" and "Internal wiring is consistent" — accurate when the commit was written, but fix(core): validate module file internal wiring #2903 is in this branch's base and check_module_file_inner now calls both collect_module_source_outputs (which bails on duplicate IDs) and check_internal_wiring. This is the restoration your coordination note asked for once fix(core): validate module file internal wiring #2903 landed. Restored.
• The exactly-one-producer rule was undocumented, and it's breaking on the dora run path. A two-stage camera → undistort pipeline where both stages emit image and the module exports image used to expand first-wins and now hard-fails. module.outputs is a plain list with no output: node/port mapping syntax, so the only remedy is renaming the internal signal — documented, along with the fact that it reaches dora run/dora build, not just dora expand.
• guide/src/concepts/modules.md wasn't mirrored. It's a near-verbatim copy and had drifted across four hunks, all from this PR, so the published mdBook documented the old step-5 rule, the old header-fields line and the old validation list. Re-mirrored.
• Nested failures had no breadcrumb. A failure three levels down surfaced as module 'l2' declares output 'out' but no inner node produces it — no file path, no include chain, just the header name of a module the user may not know exists. Harmless before this PR, since the linter only reported on the file you named; now every nested file is validated, so any of them can be the one that fails. The recursive call is wrapped in context naming the module and the referencing node. (The two tests that asserted on to_string() now format with {:#} to read the whole chain.)
• Cycle and diamond behavior was correct but completely untested. seen.remove on the success path is a one-line invariant nothing pinned — delete it and every shared nested module starts failing as a false "circular reference"; delete the insert guard and a self-including module recurses to the depth limit and reports a misleading depth error. Two tests added.
• Smaller: the check-path cycle error now carries the same hint: line as its expand-path twin; the "Phase 3: build output map from fully-expanded flat nodes" comment corrected, since building from fully-expanded leaves is precisely what this PR stopped doing; one return Err(eyre::eyre!(…)) aligned with the bail! in the neighbouring match arm.

Left for you:

• The PR description doesn't label the three breaking changes as reaching dora run/dora build.
• load_module_file redundancy: after the fix(core): validate module file internal wiring #2903 merge each nested file is parsed three times per reference (collect_module_source_outputs, the nested-outputs pass, then again inside the recursion). Lint-only cost, but it grew rather than shrank.
• Residual check-vs-run divergence, same class as what this PR closes: check_nested_module_required_inputs validates only the required direction, while the expand path additionally rejects inputs the nested module doesn't declare. Confirmed dora expand --module says fine where dora run fails with "module leaf does not declare input bogus". Not introduced here (fix(core): validate nested module required inputs #2912's scope) — worth a follow-up issue rather than silence.