Persist sim options before StartSimul so the seed is honored (AB#2742) (#2)

* Persist sim options before StartSimul so the seed is honored (AB#2742)

run_simulation(seed=N) packed the seed correctly ([SeedFixed]:1 /
[seed0]:N, matching ModelRisk's PackToStringList), and the array
marshalled fine — verified by reading back ModelRisk's own parsed
SimOpt_SeedFixed=1 / SimOpt_Seed0=N after feeding the exact payload to
VoseSetSimulOptions12. But the simulation still ran non-reproducibly.

Root cause: ModelRisk's per-cell-twister Manual-Seed mode sources its
base seed from the WORKBOOK-persisted SimOpt_SeedFixed / SimOpt_Seed0
defined-names (written by SaveOptionsToWorkbook), NOT from the per-call
options passed to VoseStartSimulCustom12. The bridge set the per-call
[seed0] (ignored by the twister) but never persisted it to the workbook,
so Manual Seed stayed OFF -> Random mode -> different stream every run.

Fix: call VoseSetSimulOptions12 with the same options payload before
VoseStartSimulCustom12. That runs SaveOptionsToWorkbook, persisting
SimOpt_SeedFixed=1 / SimOpt_Seed0=N, which the twister then honors.

Verified end-to-end against the engine oracle (single-cell VoseUniform):
- two runs at seed=12345 -> byte-identical streams (was: differing)
- seed=12345 vs seed=555 -> different reproducible streams (seed now drives it)
- the seed=12345 stream matches mrengine's per-cell twister-0 reference
  (0.929616, 0.890155, 0.316376, 0.130707, 0.183919, 0.039759, ...) exactly,
  validating the byte-identical parity path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Update sim-controller tests for the SetSimulOptions-first sequence (AB#2742)

The seed-persist fix inserts VoseSetSimulOptions12 before
VoseStartSimulCustom12, so run_simulation now issues 3 Run() calls
(persist, start, save) instead of 2. Update the order/index assertions
accordingly and add an invariant that the persist call receives the same
options payload as the start call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Timour Koupeev <timour.koupeev@vosesoftware.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

src/modelrisk_mcp/bridge/simulation.py

@@ -124,6 +124,14 @@ class SimulationRunResult:
 # "Vose" + X (ModelRiskCloude/CLAUDE.md:63).
 _CMD_START_SIM = "VoseStartSimulCustom12"
 _CMD_GET_DATA_SZ = "VoseGetDataSZ12"
+# Persists the simulation options to the workbook as SimOpt_* defined-names via
+# SaveOptionsToWorkbook (SimulationCommonOptionsReadWrite.cpp). REQUIRED for the
+# seed: ModelRisk's per-cell-twister Manual-Seed mode reads its base seed from the
+# workbook-persisted SimOpt_SeedFixed / SimOpt_Seed0 names, NOT from the per-call
+# options handed to VoseStartSimulCustom12. Without this call the per-call
+# [SeedFixed]/[seed0] are parsed but ignored by the twister -> Random mode ->
+# non-reproducible streams (AB#2742; verified end-to-end against the engine oracle).
+_CMD_SET_SIM_OPTS = "VoseSetSimulOptions12"
 
 # Operation prefix the SimulationObj_VBA dispatcher matches on for the
 # save path. PackSessionName format: "h<hwnd>_<Operation>_<book_name>"
@@ -346,6 +354,10 @@ def _invoke_start_simulation(self, opts: SimulationOptions) -> None:
         self._ensure_xll_registered()
         try:
             options_2d = [opts.to_string_list()]  # 1 row x N cols
+            # Persist options (esp. SeedFixed/seed0) to the workbook FIRST so the
+            # per-cell-twister Manual-Seed engine actually honors the seed. See
+            # _CMD_SET_SIM_OPTS above for the full rationale (AB#2742).
+            app.api.Run(_CMD_SET_SIM_OPTS, options_2d)
             app.api.Run(_CMD_START_SIM, options_2d)
         except Exception as exc:
             raise SimulationFailedError(

tests/unit/test_simulation_controller.py

@@ -3,11 +3,15 @@
 The controller's job is to:
 1. Pack SimulationOptions into the exact `[Key]:Value` line format
    `CSimulationOptions::PackToStringList` (C++) emits.
-2. Call `Application.Run("VoseStartSimulCustom12", options_2d)` with a
-   1-row 2D string array.
-3. Call `Application.Run("VoseGetDataSZ12", session_name, target_path)`
+2. Call `Application.Run("VoseSetSimulOptions12", options_2d)` to persist
+   the options (esp. the seed) to the workbook's SimOpt_* defined-names —
+   required so the per-cell-twister Manual-Seed engine honors the seed
+   (AB#2742).
+3. Call `Application.Run("VoseStartSimulCustom12", options_2d)` with the
+   same 1-row 2D string array.
+4. Call `Application.Run("VoseGetDataSZ12", session_name, target_path)`
    with the session name in the form `h<hwnd>_SaveResultsToFile_<book>`.
-4. Verify the .vmrs file appeared, raising SimulationFailedError if not.
+5. Verify the .vmrs file appeared, raising SimulationFailedError if not.
 
 Tests use a fake ExcelBridge + a fake Application to record every Run
 call and let us assert exact-string conformance with the C++ side.
@@ -158,17 +162,20 @@ def fake_save(session: str, path: str) -> None:
         result = controller.run_simulation()
 
         assert [c[0] for c in recorder.calls] == [
+            "VoseSetSimulOptions12",
             "VoseStartSimulCustom12",
             "VoseGetDataSZ12",
         ]
-        # First call: 1-row 2D options array.
-        start_args = recorder.calls[0][1]
+        # Persist + start calls both carry the same 1-row 2D options array.
+        set_args = recorder.calls[0][1]
+        start_args = recorder.calls[1][1]
         assert len(start_args) == 1
         options_2d = start_args[0]
         assert len(options_2d) == 1, "options must be a 1-row 2D array"
         assert any(s.startswith("[Samples]:") for s in options_2d[0])
-        # Second call: session name + path.
-        save_args = recorder.calls[1][1]
+        assert set_args == start_args, "persist must get the same options as start"
+        # Third call: session name + path.
+        save_args = recorder.calls[2][1]
         assert save_args[0] == "h9999_SaveResultsToFile_model.xlsx"
         assert save_args[1] == str(target)
         # Result reflects the discovered file.
@@ -205,7 +212,8 @@ def fake_save(session: str, path: str) -> None:
 
         result = controller.run_simulation(save_to=str(target))
         assert result.vmrs_path == str(target)
-        assert recorder.calls[1][1][1] == str(target)
+        # calls: [0]=SetSimulOptions, [1]=StartSimul, [2]=GetDataSZ(session, path)
+        assert recorder.calls[2][1][1] == str(target)
 
     def test_raises_when_file_not_produced(self, tmp_path: Path) -> None:
         # on_save = None means the fake never writes the file.
@@ -234,8 +242,8 @@ def fake_save(session: str, path: str) -> None:
 
         result = controller.run_simulation(workbook_name="b.xlsx")
         assert result.workbook_name == "b.xlsx"
-        # Session name uses b.xlsx, not the active a.xlsx
-        assert "b.xlsx" in recorder.calls[1][1][0]
+        # Session name uses b.xlsx, not the active a.xlsx (save = 3rd call now)
+        assert "b.xlsx" in recorder.calls[2][1][0]
 
     def test_unknown_workbook_raises(self, tmp_path: Path) -> None:
         bridge = _FakeBridge(active=_make_wb("a.xlsx", tmp_path))