Module editing: fix dangling/cyclic/mis-wired reference bugs found by adversarial audit#807
Conversation
Module wiring carries reference endpoints (a parent variable `src`, a module-qualified `module·port` `dst`) that the patch layer was not keeping consistent across structural edits: - apply_delete_variable pruned deleted flows from stock in/outflows and group members, but left module references whose `src` named the deleted variable. The dangling `src` became a dependency on a non-existent variable, so the whole project failed to compile with a confusing "missing variable" message. Prune those references too, mirroring the existing stock-flow/group cleanup. - apply_rename_variable rewrote module references only within the renamed variable's own model. Renaming a child model's input port therefore left every PARENT module's `dst` pointing at the old port name, and the parent silently fed the renamed port its default value -- wrong numbers, no error. Retarget the qualified `dst` suffix of every module instance that targets the renamed model. - canonicalize_module canonicalized only the module ident, leaving the reference endpoints verbatim. A non-canonical `src`/`dst` arriving via the FFI apply_patch (pysimlin upsert_module) then disagreed with the canonical idents every consumer compares against. Canonicalize the endpoints too, mirroring canonicalize_stock; empty placeholder endpoints are preserved. Tests drive each fix end to end (compile + simulate), asserting the wiring still carries the expected value (or cleanly drops to the port default).
A module instantiates another model by name (`Variable::Module.model_name`). Nothing in the engine checked that this instance graph is acyclic, so a self-referential module (a model containing a module that targets itself) or a mutually-recursive pair drove the recursive salsa queries `model_module_map` and `compute_layout` into a dependency-graph cycle -- which salsa turns into an unrecoverable panic. Reached from MCP edit_model/read_model and from importing a hand-authored recursive model, that panic aborts WASM and trips the same recursive-mutex cascade c1c4c95 fixed for the empty-model_name sibling of this class (the reachable cousin of GH #806; the frontend wouldCreateCycle only filters the dropdown and is bypassed by every non-UI path). Add project_module_cycle: a salsa-tracked query that builds the explicit module-instance adjacency with flat reads (no recursive salsa) and runs an iterative 3-color DFS, returning a typed CircularDependency error naming the cycle. The three production entry points consult it first and fail cleanly: assemble_simulation returns Err (mapped to NotSimulatable), collect_all_ diagnostics surfaces one project-level diagnostic and skips the per-model passes, and analyze_model degrades to an analysis_error with the snapshot intact. Implicit SMOOTH/DELAY/stdlib modules only reference leaf stdlib models, so they cannot close a user cycle and are omitted. Tests cover a self-referential module, a mutually-recursive pair, and a valid nested-module project (the guard must not false-positive on acyclic nesting), each across compile, diagnostics, and analysis.
A module reference is `{ src, dst }` where `dst` is the module-qualified
`{module}·{port}` form naming an input of the target model. `build_module_inputs`
SILENTLY DROPS a reference whose `dst` does not match an existing child input --
the port then reads its default and the simulation is quietly wrong, with no
error. The legacy monolithic compile path returned BadModuleInputDst /
BadModuleInputSrc here; the salsa path had dropped the check, so a stale `dst`
left by deleting a child input port, a hand-authored typo, or an unqualified
`dst` produced a plausible-but-wrong run.
Add model_module_wiring_diagnostics, a salsa-tracked pass driven by
model_all_diagnostics, that validates each explicit module variable's references
and emits a Warning (partial-result philosophy -- a mis-wired input should not
block the rest of the model) for a `dst` that resolves to no child input or a
bare `src` that names no variable in the enclosing model. It is deliberately
conservative to avoid false positives: empty placeholder endpoints (the new-row
UI pattern) are skipped, only an EXISTING target model is checked (an empty /
dangling model_name is a separate concern and the empty name is the normal
freshly-drawn state), and a `src` is checked only when it is a bare, non-
synthetic ident -- a qualified cross-module output or `$⁚` temporary is left to
the equation checker.
The module input-wiring UI dropdown yields a bare child port name (e.g.
`input_food`), and ModuleDetails persisted that bare value straight into the
reference `dst`. But the engine wires inputs by the canonical module-qualified
form `{moduleIdent}·{port}` -- `build_module_inputs` strips that prefix to
recover the port and SILENTLY DROPS a `dst` that lacks it. So every module input
wired through the editor failed to connect: the child port kept its default and
the simulation was quietly wrong, with no error. XMILE import already produces
the qualified form (`xmile::model` builds `module.port`), so this was an
editor-only divergence from the datamodel contract.
Add pure qualifyDst/unqualifyDst helpers (functional core) and use them in
ModuleDetails: persist `qualifyDst(moduleIdent, port)` on selection and display
`unqualifyDst(dst)` so the dropdown still shows the bare port. unqualifyDst
tolerates a legacy `.` separator for a model imported straight from XMILE that
has not yet round-tripped through a patch (the engine canonicalizes `.`->`·`).
The engine-side companion commits make this verifiable end to end: the patch
tests simulate a module wired with the qualified dst to a non-default value, and
the new wiring diagnostic warns when a dst (e.g. the old bare form) resolves to
no child input. Unit tests cover the helpers and the UI persist/display round
trip.
The only Module round-trip test exercised a fully-populated module, so the states that actually surface bugs were uncovered. Add cases for: a freshly-drawn module with an empty modelName and no references (the c1c4c95 panic state -- the empty modelName is a required Rust String, so a dropped field would be an FFI hard error), placeholder references with empty src/dst (the partially-filled wiring rows the UI persists), and a module-qualified `{moduleIdent}·{port}` dst (the canonical reference form, whose middot must survive the JSON round trip).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dde0ef42b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some((_code, msg)) = &project_module_cycle(db, project).cycle_error { | ||
| return Err(msg.clone()); |
There was a problem hiding this comment.
Because compile_project_incremental assembles only the named main_model_name, this project-wide cycle check now rejects simulations that would never traverse the cycle. For example, a project with a valid main model and an unused draft pair a -> b -> a will return NotSimulatable here before enumerate_module_instances starts from main, even though the recursive queries that can panic are only reachable for modules under the requested root. Please scope the cycle guard to models reachable from main_model_name for compilation.
Useful? React with 👍 / 👎.
| const updated = updateReferenceDst(variable.references, index, newDst); | ||
| // The dropdown yields a bare child port; persist the canonical | ||
| // module-qualified `{moduleIdent}·{port}` form the engine wires against. | ||
| const updated = updateReferenceDst(variable.references, index, qualifyDst(variable.ident, newDst)); |
There was a problem hiding this comment.
When a user wires a module input and later renames that module variable, this persisted dst keeps the old {moduleIdent}· prefix. The rename path only rewrites exact/self-qualified references, so after renaming m to n the engine builds inputs with prefix n·, drops the stale m·port reference, and the child port falls back to its default. Please update module-reference dst prefixes during module-variable renames or otherwise avoid storing a prefix that can go stale.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
==========================================
+ Coverage 90.84% 90.87% +0.03%
==========================================
Files 224 224
Lines 142647 143120 +473
==========================================
+ Hits 129581 130059 +478
+ Misses 13066 13061 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Reviewed the full diff. The patch is correct and the implementation aligns with the documented intent. Verification of each fix:
Test coverage is comprehensive — each fix ships with a regression test that reproduces the original failure end-to-end (delete simulates the bare-default fallback, rename verifies the new Verdict: correct. No blocking issues. ✅ |
Two issues the automated review flagged on the module-editing fixes:
- The module-cycle guard was project-wide, so a valid `main` model was rejected
(NotSimulatable) merely because an unused draft pair elsewhere (`a -> b -> a`)
formed a cycle that main never reaches. assemble_simulation only recurses into
modules instantiated from main, so the salsa panic the guard prevents is only
reachable for modules under the requested root. Replace the project-wide
ModuleCycleResult / project_module_cycle with project_module_graph +
ModuleReferenceGraph::cycle_reachable_from(root): compile and analyze scope the
check to the requested root, and collect_all_diagnostics checks per model
(reporting the cycle for affected models, running normal passes for the rest)
so a valid model's diagnostics are no longer hidden by an unrelated draft
cycle.
- Renaming a module VARIABLE left its own input references' dst carrying the old
`{module}·` prefix. The engine rebuilds inputs under the new prefix and drops
the stale reference, silently unwiring the input. apply_rename_variable now
reprefixes the renamed module's own reference dsts (complementing the existing
parent-dst retarget for renamed child input ports).
Regression tests cover an unused draft cycle not blocking a valid main (with
analysis of the cyclic model still degrading to an analysis_error), and renaming
a wired module variable keeping its input wired.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585b67da06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Variable::Module(module) = var { | ||
| module | ||
| .references | ||
| .retain(|reference| canonicalize(reference.src.as_str()) != ident); |
There was a problem hiding this comment.
When a module input source is stored in the main-model form ·foo (a shape explicitly accepted by build_module_inputs), deleting foo will keep that reference because this comparison checks canonical ·foo against foo. The retained reference is later interpreted as foo again, so the dangling dependency and compile failure this cleanup is meant to avoid still occur for XMILE/main-model references using the leading middot form.
Useful? React with 👍 / 👎.
…refs GH #806 flagged two unguarded panics on the legacy `Project::from_salsa` dependency-resolution path that fire on a dangling or empty module `model_name` (a freshly-drawn module, or a reference to a deleted model): `module_deps`' initial-branch `ctx.models[model_name]` HashMap index, and `topo_sort`'s "internal compiler error: unknown ident" panic (plus its trailing `assert_eq!(runlist_len, result.len())`, which a dangling dep also trips). This path is NOT dead code -- it builds the monolithic `Project`/`Module` that `TestProject::compile()` / `build_module()` and ~20 unit-inference tests run on, and it serves as an independent correctness ORACLE for the production salsa path (e.g. `db::invariance`'s `salsa_and_monolithic_paths_agree`). So rather than delete the oracle, guard the two sites #806 names, exactly as the issue proposes: `module_deps` skips a missing submodel's deps via `.get()` (mirroring the already- guarded twin in `module_output_deps`), and `topo_sort` treats an ident with no dependency entry as a dangling reference it drops from the runlist instead of panicking. Valid runlists are unaffected (`result == runlist`), so the oracle and every existing test keep their behavior; only the crash on bad input is removed. The production salsa path already rejects a dangling/cyclic module graph up front (unit inference was guarded in c1c4c95; cycle detection landed earlier in this branch), so this is defense-in-depth for the test-only path. A regression test drives `Project::from` with a module pointing at a missing model and asserts it constructs without panicking.
|
@codex review |
|
Code Review I performed a thorough review of this PR, tracing through the engine changes (patch.rs delete/rename/canonicalize, project_module_graph cycle detection, model_module_wiring_diagnostics, the cycle gates in assemble_simulation / analyze_model / collect_all_diagnostics) and the diagram changes (qualifyDst / unqualifyDst, the ModuleDetails persist/display round trip). No blocking issues found. Specifically:
Overall correctness: Correct. The patch is well-factored, comprehensively tested (every fix ships with a reproducing test), and the Codex-feedback follow-up commit addresses the two specific gaps it identified. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Code ReviewI reviewed this PR end-to-end. No bugs to flag. The five fixes hang together coherently:
Test coverage is thorough — each fix ships with a regression test exercising the original failure mode, plus the Overall correctness: correct |
1 participant
Why
Commit c1c4c95 fixed a WASM panic where a freshly-drawn module (empty
model_name) crashed unit inference. That was one instance of a broader dangling / empty / cyclic / mis-wired module-reference bug class. An adversarial audit of the module-editing feature (engine patch/compile pipeline, the JSON/FFI boundary, MCP, and the diagram controller) surfaced five more production-reachable issues plus one severe lurking bug; this PR fixes all of them, each with tests that reproduce the failure end to end.Fixes
engine: keep module references consistent under edits— module wiring stores a parentsrcand a module-qualifiedmodule·portdst, which the patch layer was not keeping consistent across structural edits:apply_delete_variablepruned deleted flows from stocks and groups but left module references whosesrcnamed the deleted variable, so deleting a wired-in parent variable left a dangling dependency and the whole project failed to compile with a confusing "missing variable" error. It now prunes those references too.apply_rename_variablerewrote module references only within the renamed variable's own model. Renaming a child model's input port left every parent module'sdstpointing at the old name, silently feeding the renamed port its default value (wrong numbers, no error). The rename now retargets the qualifieddstsuffix of every module instance that targets the renamed model.canonicalize_modulecanonicalized only the module ident, so a non-canonicalsrc/dstarriving via the FFIapply_patch(pysimlinupsert_module) disagreed with the canonical idents every consumer compares against. It now canonicalizes the endpoints, mirroringcanonicalize_stock.engine: reject cyclic module graphs instead of panicking— nothing checked that the module-instance graph is acyclic, so a self-referential or mutually-recursive module drove the recursive salsa queries (model_module_map,compute_layout) into a dependency-graph cycle, which salsa turns into an unrecoverable panic — reached from MCPedit_model/read_modeland from importing a hand-authored recursive model, aborting WASM. A newproject_module_cyclequery detects the cycle with flat reads + an iterative DFS (no recursive salsa) and the three compile / diagnostic / analysis entry points fail cleanly with aCircularDependencyerror instead. This is the production-reachable cousin of thefrom_salsapanic class tracked in #806 — #806's own dead legacy sites are not reachable from production and are left as tracked; the live path is what this guards.engine: warn on unresolved module input wiring—build_module_inputssilently drops a reference whosedstdoes not match an existing child input, so the port reads its default and the simulation is quietly wrong with no error. The legacy compile path returnedBadModuleInputDst/BadModuleInputSrc; the salsa path had dropped the check. A new diagnostic pass re-adds it as a Warning (partial-result philosophy — a mis-wired input should not block the rest of the model), deliberately conservative to avoid false positives (empty placeholder rows skipped, only existing target models checked, qualified/syntheticsrcs left alone).diagram: persist module input dst in the engine-qualified form(the lurking bug) — the wiring UI dropdown yields a bare child port name, andModuleDetailspersisted that bare value straight into the referencedst. But the engine wires inputs by the canonicalmodule·portform and silently drops adstthat lacks the prefix, so every module input wired through the editor failed to connect. XMILE import already produces the qualified form, so this was an editor-only divergence from the datamodel contract. The fix qualifies the port on persist and strips it on display (purequalifyDst/unqualifyDsthelpers), tolerating a legacy.separator on read.core: cover module serialization edge cases— the only Module round-trip test exercised a fully-populated module. Added cases for the empty-modelNamefreshly-drawn state (the c1c4c95 panic shape; the empty name is a required RustString), placeholder references with emptysrc/dst, and a module-qualifieddstwhose middot must survive the JSON round trip.Non-obvious decisions
wouldCreateCycleonly filters the model-reference dropdown and is bypassed by MCP and file-import, so the engine must own the guard for every entry point.dstis qualified in the editor, not bare. The canonical datamodel/XMILE representation ismodule·port; making the editor conform (rather than making the engine accept baredst) keeps a single representation across import/export/edit.Testing
Every fix ships with a test that reproduces the original failure: the engine patch tests compile and simulate a wired module (asserting the value flows, or cleanly drops to the port default); the cycle tests assert a clean error across compile, diagnostics, and analysis (and that valid nesting does not false-positive); the wiring-diagnostic tests pin bare/dangling
dstandsrcwarnings while a correct wiring and empty rows stay clean; the diagram tests cover the qualify/unqualify helpers and the UI persist/display round trip; and the core tests cover the serialization edge cases. Each commit was gated by the full pre-commit suite (Rust fmt/clippy/test, TypeScript lint/build/tsc/test, pysimlin).Related: #806 (the production-reachable cousin of that dangling-reference panic class is fixed here; #806's specific dead
from_salsasites remain tracked).