Skip to content

engine: Variable::Module inside a macro-body model is silently dropped by the MDL writer (project_to_mdl reject gate scans only the main model) #566

@bpowers

Description

@bpowers

Summary

MdlWriter::write_macro_blocks emits each macro-marked model's body by iterating its non-parameter variables and calling write_variable_entry directly. write_variable_entry has a datamodel::Variable::Module(_) => return arm, so a Variable::Module living inside a macro-body model is silently dropped from the exported .mdl -- no error, no diagnostic. Meanwhile project_to_mdl's reject gate only scans the main model's variables for Variable::Module; it never inspects macro-marked (macro-body) models. So a Variable::Module in a macro body is neither rejected by the gate nor reconstructed nor errored -- it just vanishes, and any sibling binding auxes in that macro body would dangle on an unwritten module (corrupt .mdl).

This is the same failure class that commit 5b200eab8a0ef9a713b0720ea89038a4686ec585 ("engine: reject malformed macro clusters in MDL export") fixed -- silent macro-cluster corruption on MDL export -- but a different code path. 5b200eab hardened the reject gate + writer for a broken multi-output cluster in the main model; this gap is a Variable::Module living inside a macro-body model, which neither the gate nor the writer's reconstruction path covers.

Code paths (verified in code review on branch macros)

  • Silent drop: src/simlin-engine/src/mdl/writer.rs write_macro_blocks (~line 2738) -- the body loop (for var in &model.variables { if param_idents.contains(...) { continue; } write_variable_entry(&mut self.buf, var, &display_names); }, ~lines 2771-2777) calls write_variable_entry directly. build_multi_output_reconstructions (writer.rs:1454) is only invoked from the main-model body path (writer.rs:2855), never for macro-body models.
  • src/simlin-engine/src/mdl/writer.rs:867 -- datamodel::Variable::Module(_) => return in write_variable_entry is the silent drop.
  • Reject gate blind spot: src/simlin-engine/src/mdl/mod.rs:64-89 -- the gate iterates only main_model(project).variables (main_model = the single non-macro model, mod.rs:105). It checks is_macro_module to let a macro-backed Module through (on the assumption "the writer will reconstruct it"), but it never iterates macro-marked models' variables, so a Module inside a macro body is never examined here at all.

Why it matters

  • Silent data corruption (worst failure mode). A Variable::Module in a macro-body model is dropped with no error/warning, and sibling binding auxes in that macro body then reference a module that was never written -- a corrupt .mdl. This is exactly the failure class 5b200eab was filed and fixed to prevent, just via a code path that fix did not cover.
  • Consistency with the macro design. The Vensim macro design (docs/design-plans/2026-05-13-macros.md) and 5b200eab's own rationale reject every other unrepresentable MDL-export case with an actionable hard error; this path is the one place a macro-cluster-class problem still silently drops data instead of erroring.

Reachability / scope

  • 5b200eab's rationale argues this failure class is reachable for the primary user: "AI agents patch models programmatically via MCP" -- ProjectPatch delete/rename/upsert, JSON/protobuf round-trip then edit. A post-import MCP patch (or a future feature) that inserts or moves a Variable::Module into a macro-body model triggers it.
  • The macro design's "Additional Considerations" / the Claude review on PR Add Vensim macro (:MACRO:) support #564 lists "multi-output call inside a macro body" as a documented scoping limitation: the importer does not currently materialize a multi-output cluster into a macro body. So today this is a latent / defense-in-depth gap -- not hit by the six bundled test/test-models/tests/macro_* fixtures -- rather than a path the bundled corpus exercises. Closing it correctly needs a design decision (see below), which is why it is filed for tracking rather than fixed inline.

Possible remediation (do NOT perform as part of triage; needs a design decision)

Decide between (mirroring how 5b200eab handled the main-model case):

  1. Extend the reject gate (preferred, smallest, mirrors 5b200eab): in src/simlin-engine/src/mdl/mod.rs:64-89, additionally scan every macro-marked model's variables for Variable::Module and hard-error with an actionable message (naming the offending module + enclosing macro) instead of letting write_macro_blocks -> write_variable_entry drop it. This matches 5b200eab's "fail fast, don't silently drop" policy for the main model.
  2. Or: reconstruct/validate in write_macro_blocks: route macro-body Modules through build_multi_output_reconstructions-style validation/reconstruction so a faithfully-reconstructable multi-output cluster inside a macro body round-trips, and only the unreconstructable case hard-errors.

Either way it must not be silently dropped.

Add a regression test analogous to the existing project_to_mdl_rejects_macro_cluster_missing_* tests in src/simlin-engine/src/mdl/writer_tests.rs (~line 1742+), but for a Variable::Module placed inside a macro-body model (the existing three tests all place the broken cluster in the main model -- make_model(broken) is the non-macro model -- so none covers this path).

Relationship to existing issues / tracking

Discovery context

Identified during PR #564 (Vensim macro support) review-feedback work on branch macros, while verifying the scope of the 5b200eab reject-gate hardening. Component: simlin-engine / MDL writer. Relevant: PR #564, commit 5b200eab8a0ef9a713b0720ea89038a4686ec585.

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineIssues with the rust-based simulation engine

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions