Module editing: fix dangling/cyclic/mis-wired reference bugs found by adversarial audit

src/core/tests/datamodel.test.ts

@@ -305,6 +305,82 @@ describe('Module', () => {
     expect(restored.modelName).toBe('SectorModel');
     expect(restored.references.length).toBe(1);
   });
+
+  it('roundtrips a freshly-drawn module (empty modelName, no references)', () => {
+    // The editor creates a module with an EMPTY modelName until the user
+    // assigns a target model -- the exact state that panicked the engine in
+    // c1c4c954. The empty modelName must survive serialization (the Rust
+    // `model_name` is a required String, so a dropped field is an FFI hard
+    // error) and deserialize back to empty.
+    const mod: Module = {
+      type: 'module',
+      ident: 'new_module',
+      modelName: '',
+      documentation: '',
+      units: '',
+      references: [],
+      data: undefined,
+      errors: undefined,
+      unitErrors: undefined,
+      uid: undefined,
+    };
+
+    const json = moduleToJson(mod);
+    expect(json.modelName).toBe('');
+    expect(json.references ?? []).toEqual([]);
+
+    const restored = moduleFromJson(json);
+    expect(restored.modelName).toBe('');
+    expect(restored.references).toEqual([]);
+  });
+
+  it('preserves placeholder references with empty src/dst', () => {
+    // The wiring UI persists partially-filled rows (empty src or dst) as the
+    // user builds up inputs; these must round-trip verbatim, not be coalesced
+    // or dropped.
+    const mod: Module = {
+      type: 'module',
+      ident: 'sector',
+      modelName: 'SectorModel',
+      documentation: '',
+      units: '',
+      references: [
+        { src: '', dst: '' },
+        { src: 'food', dst: '' },
+      ],
+      data: undefined,
+      errors: undefined,
+      unitErrors: undefined,
+      uid: 5,
+    };
+
+    const restored = moduleFromJson(moduleToJson(mod));
+    expect(restored.references).toEqual([
+      { src: '', dst: '' },
+      { src: 'food', dst: '' },
+    ]);
+  });
+
+  it('roundtrips a module-qualified dst reference', () => {
+    // The canonical reference dst is the module-qualified `{moduleIdent}·{port}`
+    // form (the engine strips the prefix to wire the input); the middot must
+    // survive the JSON round trip intact.
+    const mod: Module = {
+      type: 'module',
+      ident: 'sector',
+      modelName: 'SectorModel',
+      documentation: '',
+      units: '',
+      references: [{ src: 'food', dst: 'sector·sector_input' }],
+      data: undefined,
+      errors: undefined,
+      unitErrors: undefined,
+      uid: 5,
+    };
+
+    const restored = moduleFromJson(moduleToJson(mod));
+    expect(restored.references[0].dst).toBe('sector·sector_input');
+  });
 });
 
 describe('View Elements', () => {

src/diagram/ModuleDetails.tsx

@@ -18,7 +18,9 @@ import { STDLIB_PREFIX } from './module-navigation';
 import {
   addReference,
   getAvailableSrcVariables,
+  qualifyDst,
   removeReference,
+  unqualifyDst,
   updateReferenceDst,
   updateReferenceSrc,
 } from './module-wiring';
@@ -181,7 +183,9 @@ export function ModuleDetails(props: ModuleDetailsProps): React.ReactElement {
   };
 
   const handleDstChange = (index: number, newDst: string): void => {
-    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));
     onReferencesChange(variable.ident, updated);
   };
 
@@ -230,7 +234,7 @@ export function ModuleDetails(props: ModuleDetailsProps): React.ReactElement {
                   </td>
                   <td className={styles.wiringDropdown}>
                     <Autocomplete
-                      value={ref.dst || null}
+                      value={unqualifyDst(ref.dst) || null}
                       options={dstOptions}
                       onChange={(_: React.SyntheticEvent | null, newValue: string | null) => {
                         if (newValue) {

src/diagram/module-wiring.ts

@@ -6,6 +6,35 @@
 
 import type { ModuleReference } from '@simlin/core/datamodel';
 
+// A module reference `dst` is stored in the canonical module-qualified form
+// `{moduleIdent}·{port}`: the engine's `build_module_inputs` strips that prefix
+// to recover the child input port, and a BARE `dst` (just the port) silently
+// fails to wire -- the port reads its default and the simulation is quietly
+// wrong. The wiring UI works in bare port names, so persist the qualified form
+// and strip it for display. `·` (·) is the canonical separator the engine
+// canonicalizes to; a literal `.` is tolerated on read for a model imported
+// straight from XMILE that has not yet round-tripped through a patch.
+const MODULE_PORT_SEPARATOR = '·';
+
+/**
+ * Qualifies a bare child input-port name into the canonical `{moduleIdent}·{port}`
+ * reference `dst`. An empty port stays empty so new-row placeholders persist as
+ * empty (not a dangling `moduleIdent·`).
+ */
+export function qualifyDst(moduleIdent: string, port: string): string {
+  return port ? `${moduleIdent}${MODULE_PORT_SEPARATOR}${port}` : '';
+}
+
+/**
+ * Recovers the bare child input-port name from a reference `dst` for display:
+ * the segment after the final module separator (`·` or a legacy `.`), or the
+ * whole string when there is no separator (an already-bare or empty value).
+ */
+export function unqualifyDst(dst: string): string {
+  const sep = Math.max(dst.lastIndexOf(MODULE_PORT_SEPARATOR), dst.lastIndexOf('.'));
+  return sep >= 0 ? dst.slice(sep + 1) : dst;
+}
+
 /**
  * Returns true if a reference with the same src and dst already exists.
  */

src/diagram/tests/module-wiring-ui.test.tsx

@@ -272,7 +272,36 @@ describe('ModuleDetails wiring editor', () => {
       const dstOption = screen.getByText('input_food');
       fireEvent.click(dstOption);
 
-      expect(callbacks.onReferencesChange).toHaveBeenCalledWith('hares_mod', [{ src: 'food', dst: 'input_food' }]);
+      // The bare dropdown port is persisted in the canonical module-qualified
+      // form the engine wires against ({moduleIdent}·{port}).
+      expect(callbacks.onReferencesChange).toHaveBeenCalledWith('hares_mod', [
+        { src: 'food', dst: 'hares_mod·input_food' },
+      ]);
+    });
+
+    test('a module-qualified dst is displayed as the bare port name', () => {
+      const variable = makeModule('hares_mod', 'hares', {
+        references: [{ src: 'food', dst: 'hares_mod·input_food' }],
+      });
+      const project = makeProject([
+        makeModel('main', [variable, makeAux('food')]),
+        makeModel('hares', [makeAux('input_food', { canBeModuleInput: true })]),
+      ]);
+      const callbacks = defaultCallbacks();
+
+      render(
+        <ModuleDetails
+          variable={variable}
+          viewElement={makeViewElement('hares_mod')}
+          project={project}
+          currentModelName="main"
+          {...callbacks}
+        />,
+      );
+
+      // The qualified dst renders as the bare port, not the raw `hares_mod·input_food`.
+      const dstInput = screen.getByPlaceholderText('Select input') as HTMLInputElement;
+      expect(dstInput.value).toBe('input_food');
     });
   });
 

src/diagram/tests/module-wiring.test.ts

@@ -15,10 +15,44 @@ import {
   updateReferenceSrc,
   updateReferenceDst,
   getAvailableSrcVariables,
+  qualifyDst,
+  unqualifyDst,
 } from '../module-wiring';
 
 // -- Tests --
 
+describe('qualifyDst', () => {
+  it('prefixes a bare port with the module ident and the canonical separator', () => {
+    expect(qualifyDst('hares_mod', 'input_food')).toBe('hares_mod·input_food');
+  });
+
+  it('keeps an empty port empty (placeholder rows are not dangling prefixes)', () => {
+    expect(qualifyDst('hares_mod', '')).toBe('');
+  });
+});
+
+describe('unqualifyDst', () => {
+  it('recovers the bare port from a module-qualified dst', () => {
+    expect(unqualifyDst('hares_mod·input_food')).toBe('input_food');
+  });
+
+  it('tolerates a legacy period separator (XMILE-imported, pre-patch)', () => {
+    expect(unqualifyDst('hares_mod.input_food')).toBe('input_food');
+  });
+
+  it('returns an already-bare value unchanged', () => {
+    expect(unqualifyDst('input_food')).toBe('input_food');
+  });
+
+  it('returns empty for empty input', () => {
+    expect(unqualifyDst('')).toBe('');
+  });
+
+  it('round-trips with qualifyDst', () => {
+    expect(unqualifyDst(qualifyDst('m', 'port'))).toBe('port');
+  });
+});
+
 describe('isDuplicateReference', () => {
   it('returns false for empty references array', () => {
     expect(isDuplicateReference([], 'food', 'input_food')).toBe(false);

src/simlin-engine/src/analysis.rs

@@ -143,6 +143,27 @@ pub fn analyze_model(
     let json_model = model_snapshot(project, model_name)
         .ok_or_else(|| format!("model '{model_name}' not found in project"))?;
 
+    // A cyclic / self-referential module graph reachable from the analyzed model
+    // would panic the recursive causal-graph / layout queries this analysis
+    // drives. Surface it as an actionable `analysis_error` (the model snapshot is
+    // intact, loops empty), matching the non-compilable-model degradation rather
+    // than aborting (GH #806). Scoped to reachability from `model_name` so an
+    // unrelated draft cycle elsewhere does not block analyzing a valid model.
+    if let Some((_code, msg)) =
+        crate::db::project_module_graph(&*db, source_project).cycle_error_from(model_name)
+    {
+        return Ok(ModelAnalysis {
+            model: json_model,
+            time: vec![],
+            loop_dominance: vec![],
+            partitions: vec![],
+            dominant_loops_by_period: vec![],
+            analysis_error: Some(msg),
+            truncated: false,
+            agg_recovery_truncated: false,
+        });
+    }
+
     // Enable LTM discovery for this analysis; restore flags before returning
     // so the caller's db state stays clean.
     source_project.set_ltm_enabled(db).to(true);

src/simlin-engine/src/common.rs

@@ -2651,20 +2651,32 @@ pub fn topo_sort<'out>(
             return;
         }
         used.insert(ident);
+        // An ident with no dependencies entry is a dangling reference -- skip it
+        // rather than panicking. A dangling module reference (an empty or missing
+        // `model_name`) on the legacy `from_salsa` path can leave such an ident
+        // in the dependency set; it is not a real variable to place in the
+        // runlist, so dropping it keeps this test-only path from crashing with an
+        // "internal compiler error" on user-controllable input (GH #806). The
+        // production salsa path uses `topo_sort_str` and rejects a
+        // dangling/cyclic module graph up front, so valid runlists -- where every
+        // ident has a deps entry -- are unaffected (result == runlist).
         if let Some(deps) = dependencies.get(ident) {
             for dep in deps.iter() {
                 add(dependencies, result, used, dep)
             }
-        } else {
-            panic!("internal compiler error: unknown ident {}", ident.as_str());
+            result.push(ident);
         }
-        result.push(ident);
     }
 
     for ident in runlist.into_iter() {
         add(dependencies, &mut result, &mut used, ident);
     }
 
-    assert_eq!(runlist_len, result.len());
+    // For a well-formed runlist every ident has a dependencies entry and every
+    // dependency is itself in the runlist, so `result` holds exactly the runlist
+    // idents (`result.len() == runlist_len`). A dangling reference (GH #806) is
+    // skipped above, so the result may be shorter; we no longer assert equality
+    // (it would re-introduce a panic on the bad input this guards against).
+    debug_assert!(result.len() <= runlist_len);
     result
 }

src/simlin-engine/src/db.rs

@@ -63,9 +63,10 @@ pub use input::{
 mod query;
 pub(crate) use query::canonical_module_input_set;
 pub use query::{
-    ImplicitVarMeta, ParsedVariableResult, VariableDeps, model_implicit_var_info,
-    model_module_ident_context, model_module_map, parse_source_variable_with_module_context,
-    project_converted_dimensions, project_datamodel_dims, project_dimensions_context,
+    ImplicitVarMeta, ModuleReferenceGraph, ParsedVariableResult, VariableDeps,
+    model_implicit_var_info, model_module_ident_context, model_module_map,
+    parse_source_variable_with_module_context, project_converted_dimensions,
+    project_datamodel_dims, project_dimensions_context, project_module_graph,
     project_units_context, variable_dimensions, variable_direct_dependencies,
     variable_relevant_dimensions, variable_size,
 };
@@ -1092,6 +1093,10 @@ mod ltm_module_tests;
 #[cfg(test)]
 mod ltm_unified_tests;
 #[cfg(test)]
+mod module_cycle_tests;
+#[cfg(test)]
+mod module_wiring_tests;
+#[cfg(test)]
 mod prev_init_tests;
 #[cfg(test)]
 mod tests;

src/simlin-engine/src/db/assemble.rs

@@ -1921,6 +1921,18 @@ pub fn assemble_simulation(
         return Err(msg);
     }
 
+    // A cyclic / self-referential module graph reachable FROM THE MAIN MODEL
+    // would drive the recursive instance-enumeration, layout, and module-map
+    // queries into a salsa dependency-graph cycle panic. Reject it cleanly first
+    // (GH #806). Scoped to reachability from `main`: assembly starts from main
+    // and only recurses into its instantiated modules, so an unrelated draft
+    // cycle elsewhere in the project must not block compiling a valid main.
+    if let Some((_code, msg)) =
+        project_module_graph(db, project).cycle_error_from(main_model_canonical.as_ref())
+    {
+        return Err(msg);
+    }
+
     // Enumerate module instances by walking module variables recursively.
     // Each unique (model_name, input_set) pair gets its own CompiledModule.
     let module_instances = enumerate_module_instances(db, project, &main_model_name)?;