From 5b2a50b209ae83be34f625a87c21a4d2809f653c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 17:53:19 -0700 Subject: [PATCH 01/11] core: round-trip variable compat to avoid silent data loss Model content is persisted as a protobuf the engine serializes, and edits apply as full-replace upsert ops keyed by ident. datamodel.ts converts each variable to and from that JSON, so any wire field it failed to read in and write back out was silently dropped the moment a user edited the variable, corrupting real models on a routine edit. Round-trip the previously-dropped fields for Stock/Flow/Aux/Module: ACTIVE INITIAL (compat.activeInitial), the Vensim EXCEPT default equation and its has_except_default flag, per-element graphical functions and per-element ACTIVE INITIAL, external-data references (compat.data_source), and module compat (canBeModuleInput/isPublic/dataSource). The arrayed elements map now carries per-element data instead of bare equation strings. Editor.tsx built several upsert payloads by hand, dropping the same fields; rebase them on the shared *ToJson serializers. JSON key names were verified against the engine's json.rs serializer, and an end-to-end test drives the real WASM engine to prove the fields survive an upsert round-trip. --- src/core/datamodel.ts | 249 +++++++++++++--- src/core/jest.config.js | 4 + .../tests/datamodel-roundtrip-e2e.test.ts | 265 ++++++++++++++++++ src/core/tests/datamodel.test.ts | 247 +++++++++++++++- src/diagram/Editor.tsx | 72 ++--- src/diagram/tests/flow-attach.test.ts | 2 + .../tests/module-details-utils.test.ts | 3 + src/diagram/tests/module-details.test.tsx | 7 + src/diagram/tests/module-wiring-ui.test.tsx | 3 + .../tests/variable-details-display.test.ts | 2 + src/engine/src/json-types.ts | 22 ++ 11 files changed, 794 insertions(+), 82 deletions(-) create mode 100644 src/core/tests/datamodel-roundtrip-e2e.test.ts diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index e4249c42f..4859af789 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -22,6 +22,7 @@ import { type JsonGraphicalFunctionScale, type JsonArrayedEquation, type JsonElementEquation, + type JsonDataSource, type JsonView, type JsonViewElement, type JsonStockViewElement, @@ -146,6 +147,56 @@ export function graphicalFunctionToJson(gf: GraphicalFunction): JsonGraphicalFun return result; } +// DataSource: external-data reference (Vensim GET DIRECT DATA / CONSTANTS / +// LOOKUPS / SUBSCRIPT). Carried on a variable's compat so an edit to any other +// field does not drop the reference. The wire shape and `kind` values are +// defined by the Rust json::JsonDataSource serializer (src/simlin-engine/src/json.rs). + +export type DataSourceKind = 'data' | 'constants' | 'lookups' | 'subscript'; + +export interface DataSource { + readonly kind: DataSourceKind; + readonly file: string; + readonly tabOrDelimiter: string; + readonly rowOrCol: string; + readonly cell: string; +} + +function dataSourceKindFromJson(kind: string): DataSourceKind { + // Mirror the Rust data_source_from_json fallback: any unrecognized kind + // (including the default "data") maps to 'data'. + switch (kind) { + case 'constants': + return 'constants'; + case 'lookups': + return 'lookups'; + case 'subscript': + return 'subscript'; + default: + return 'data'; + } +} + +export function dataSourceFromJson(json: JsonDataSource): DataSource { + return { + kind: dataSourceKindFromJson(json.kind), + file: json.file ?? '', + tabOrDelimiter: json.tabOrDelimiter ?? '', + rowOrCol: json.rowOrCol ?? '', + cell: json.cell ?? '', + }; +} + +export function dataSourceToJson(ds: DataSource): JsonDataSource { + return { + kind: ds.kind, + file: ds.file, + tabOrDelimiter: ds.tabOrDelimiter, + rowOrCol: ds.rowOrCol, + cell: ds.cell, + }; +} + // Equation types export interface ScalarEquation { @@ -159,32 +210,100 @@ export interface ApplyToAllEquation { readonly equation: string; } +// A single element of an arrayed (per-element) equation. Carries the +// element's equation plus any per-element graphical function and ACTIVE +// INITIAL equation (the only per-element compat field the engine round-trips). +export interface ArrayedElement { + readonly equation: string; + readonly graphicalFunction: GraphicalFunction | undefined; + readonly activeInitial: string | undefined; +} + export interface ArrayedEquation { readonly type: 'arrayed'; readonly dimensionNames: readonly string[]; - readonly elements: ReadonlyMap; + readonly elements: ReadonlyMap; + // The EXCEPT default equation applied to elements not listed in `elements`, + // and the flag indicating it is an EXCEPT default. `hasExceptDefault` is only + // meaningful when `defaultEquation` is set. + readonly defaultEquation: string | undefined; + readonly hasExceptDefault: boolean; } export type Equation = ScalarEquation | ApplyToAllEquation | ArrayedEquation; +// Build the datamodel ArrayedEquation from its JSON form, preserving per-element +// equations, graphical functions, and ACTIVE INITIAL equations as well as the +// EXCEPT default. Shared by stocks, flows, and auxes (the per-element shape is +// identical across variable kinds). +export function arrayedEquationFromJson(json: JsonArrayedEquation): ArrayedEquation { + const dimensionNames: readonly string[] = json.dimensions ?? []; + const elements = new Map( + (json.elements ?? []).map( + (el: JsonElementEquation) => + [ + el.subscript, + { + equation: el.equation, + graphicalFunction: el.graphicalFunction ? graphicalFunctionFromJson(el.graphicalFunction) : undefined, + // The engine only round-trips activeInitial out of a per-element compat. + activeInitial: el.compat?.activeInitial, + }, + ] as [string, ArrayedElement], + ), + ); + // Mirror the engine: a legacy payload without hasExceptDefault infers `true` + // when a default equation is present (preserving pre-flag behavior). + const hasExceptDefault = json.hasExceptDefault ?? json.equation !== undefined; + return { + type: 'arrayed', + dimensionNames, + elements, + defaultEquation: json.equation, + hasExceptDefault, + }; +} + +// Serialize a datamodel ArrayedEquation back to JSON. Only emits the EXCEPT +// default flag when a default equation is present (mirroring the engine, where +// the flag is meaningless without one). +export function arrayedEquationToJson(eq: ArrayedEquation): JsonArrayedEquation { + const result: JsonArrayedEquation = { + dimensions: [...eq.dimensionNames], + elements: [...eq.elements].map(([subscript, el]) => { + const jsonEl: JsonElementEquation = { subscript, equation: el.equation }; + if (el.graphicalFunction) { + jsonEl.graphicalFunction = graphicalFunctionToJson(el.graphicalFunction); + } + if (el.activeInitial) { + jsonEl.compat = { activeInitial: el.activeInitial }; + } + return jsonEl; + }), + }; + if (eq.defaultEquation !== undefined) { + result.equation = eq.defaultEquation; + result.hasExceptDefault = eq.hasExceptDefault; + } + return result; +} + function stockEquationFromJson( initialEquation: string | undefined, arrayedEquation: JsonArrayedEquation | undefined, ): Equation { if (arrayedEquation) { - const dimensionNames: readonly string[] = arrayedEquation.dimensions ?? []; + // Per-element equations imply an 'arrayed' equation; otherwise it's applyToAll. + // Known inert divergence: a payload with an EXCEPT default but zero elements + // routes to applyToAll here (the engine routes Some([]) to Arrayed), dropping + // hasExceptDefault. This is semantically equivalent (a default with no excepted + // subscripts applies to all elements either way) and real models never emit it. if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { - return { - type: 'arrayed', - dimensionNames, - elements: new Map( - arrayedEquation.elements.map((el: JsonElementEquation) => [el.subscript, el.equation] as [string, string]), - ), - }; + return arrayedEquationFromJson(arrayedEquation); } else { return { type: 'applyToAll', - dimensionNames, + dimensionNames: arrayedEquation.dimensions ?? [], equation: arrayedEquation.equation ?? '', }; } @@ -203,23 +322,18 @@ function auxEquationFromJson( } if (arrayedEquation) { - const dimensionNames: readonly string[] = arrayedEquation.dimensions ?? []; + // Per-element equations imply an 'arrayed' equation; otherwise it's applyToAll. + // Known inert divergence: a payload with an EXCEPT default but zero elements + // routes to applyToAll here (the engine routes Some([]) to Arrayed), dropping + // hasExceptDefault. This is semantically equivalent (a default with no excepted + // subscripts applies to all elements either way) and real models never emit it. if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { - return { - equation: { - type: 'arrayed', - dimensionNames, - elements: new Map( - arrayedEquation.elements.map((el: JsonElementEquation) => [el.subscript, el.equation] as [string, string]), - ), - }, - graphicalFunction, - }; + return { equation: arrayedEquationFromJson(arrayedEquation), graphicalFunction }; } else { return { equation: { type: 'applyToAll', - dimensionNames, + dimensionNames: arrayedEquation.dimensions ?? [], equation: arrayedEquation.equation ?? '', }, graphicalFunction, @@ -243,12 +357,7 @@ function stockEquationToJson(equation: Equation): { initialEquation?: string; ar }, }; } else if (equation.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...equation.dimensionNames], - elements: [...equation.elements].map(([subscript, eqn]) => ({ subscript, equation: eqn })), - }, - }; + return { arrayedEquation: arrayedEquationToJson(equation) }; } return {}; } @@ -264,12 +373,7 @@ function auxEquationToJson(equation: Equation): { equation?: string; arrayedEqua }, }; } else if (equation.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...equation.dimensionNames], - elements: [...equation.elements].map(([subscript, eqn]) => ({ subscript, equation: eqn })), - }, - }; + return { arrayedEquation: arrayedEquationToJson(equation) }; } return {}; } @@ -287,6 +391,10 @@ export interface Stock { readonly nonNegative: boolean; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + // Vensim ACTIVE INITIAL: the variable's separate initialization equation. + readonly activeInitial: string | undefined; + // External-data reference (Vensim GET DIRECT DATA/CONSTANTS/LOOKUPS/SUBSCRIPT). + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -303,6 +411,8 @@ export interface Flow { readonly nonNegative: boolean; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + readonly activeInitial: string | undefined; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -318,6 +428,8 @@ export interface Aux { readonly gf: GraphicalFunction | undefined; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + readonly activeInitial: string | undefined; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -344,6 +456,12 @@ export interface Module { readonly documentation: string; readonly units: string; readonly references: readonly ModuleReference[]; + // The engine reads only canBeModuleInput, isPublic, and dataSource out of a + // module's compat (From in json.rs uses defaults for the rest), so + // ACTIVE INITIAL and nonNegative are intentionally absent here. + readonly canBeModuleInput: boolean; + readonly isPublic: boolean; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -385,6 +503,8 @@ export function stockFromJson(json: JsonStock): Stock { nonNegative: json.compat?.nonNegative || json.nonNegative || false, canBeModuleInput: json.compat?.canBeModuleInput ?? false, isPublic: json.compat?.isPublic ?? false, + activeInitial: json.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -429,6 +549,18 @@ export function stockToJson(stock: Stock): JsonStock { } result.compat.isPublic = true; } + if (stock.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = stock.activeInitial; + } + if (stock.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(stock.dataSource); + } if (stock.documentation) { result.documentation = stock.documentation; } @@ -453,6 +585,8 @@ export function flowFromJson(json: JsonFlow): Flow { nonNegative: json.compat?.nonNegative || json.nonNegative || false, canBeModuleInput: json.compat?.canBeModuleInput ?? false, isPublic: json.compat?.isPublic ?? false, + activeInitial: json.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -498,6 +632,18 @@ export function flowToJson(flow: Flow): JsonFlow { } result.compat.isPublic = true; } + if (flow.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = flow.activeInitial; + } + if (flow.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(flow.dataSource); + } if (flow.documentation) { result.documentation = flow.documentation; } @@ -519,6 +665,8 @@ export function auxFromJson(json: JsonAuxiliary): Aux { gf: graphicalFunction, canBeModuleInput: json.compat?.canBeModuleInput ?? false, isPublic: json.compat?.isPublic ?? false, + activeInitial: json.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -558,6 +706,18 @@ export function auxToJson(aux: Aux): JsonAuxiliary { } result.compat.isPublic = true; } + if (aux.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = aux.activeInitial; + } + if (aux.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(aux.dataSource); + } if (aux.documentation) { result.documentation = aux.documentation; } @@ -572,6 +732,9 @@ export function moduleFromJson(json: JsonModule): Module { documentation: json.documentation ?? '', units: json.units ?? '', references: (json.references ?? []).map((ref: JsonModuleReference) => moduleReferenceFromJson(ref)), + canBeModuleInput: json.compat?.canBeModuleInput ?? false, + isPublic: json.compat?.isPublic ?? false, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -593,6 +756,24 @@ export function moduleToJson(mod: Module): JsonModule { if (mod.units) { result.units = mod.units; } + if (mod.canBeModuleInput) { + if (!result.compat) { + result.compat = {}; + } + result.compat.canBeModuleInput = true; + } + if (mod.isPublic) { + if (!result.compat) { + result.compat = {}; + } + result.compat.isPublic = true; + } + if (mod.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(mod.dataSource); + } if (mod.documentation) { result.documentation = mod.documentation; } diff --git a/src/core/jest.config.js b/src/core/jest.config.js index 3001c4d78..5e51aeb1b 100644 --- a/src/core/jest.config.js +++ b/src/core/jest.config.js @@ -9,7 +9,11 @@ const config = { testMatch: ['/tests/**/*.test.ts'], moduleFileExtensions: ['ts', 'js'], moduleNameMapper: { + // Mirror @simlin/engine's own exports aliases so a core test can drive the + // engine at runtime: these internal subpaths resolve to platform-specific + // node files that do not live under the literal subpath. '^@simlin/engine/internal/wasm$': '/../engine/lib/internal/wasm.node.js', + '^@simlin/engine/internal/backend-factory$': '/../engine/lib/backend-factory.node.js', '^@simlin/engine/(.*)$': '/../engine/lib/$1.js', '^@simlin/engine$': '/../engine/lib/index.js', }, diff --git a/src/core/tests/datamodel-roundtrip-e2e.test.ts b/src/core/tests/datamodel-roundtrip-e2e.test.ts new file mode 100644 index 000000000..614df86f4 --- /dev/null +++ b/src/core/tests/datamodel-roundtrip-e2e.test.ts @@ -0,0 +1,265 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +// End-to-end regression for silent variable data-loss through the REAL engine. +// +// A pure fromJson<->toJson test (datamodel.test.ts) can pass while still using +// a wrong JSON key name, because both halves agree on the typo. These tests +// instead drive the actual Rust serializer (src/simlin-engine/src/json.rs) via +// the WASM engine, so a key name that does not match the serializer is caught: +// the engine drops the unknown field and the survival assertion fails. +// +// The scenario mirrors the editor's persistence path exactly. The editor loads +// a serialized project, builds the in-memory datamodel, and -- on any edit -- +// re-serializes a variable as a FULL upsert (replace-by-UID, see +// src/simlin-engine/src/patch.rs). Any wire field datamodel.ts fails to +// round-trip is therefore dropped the moment the user edits an unrelated field. + +import * as fs from 'fs'; +import * as path from 'path'; + +import { Project, configureWasm, ready, resetWasm } from '@simlin/engine'; +import type { JsonProjectPatch } from '@simlin/engine'; + +import { projectFromJson, projectToJson, auxToJson, stockToJson, moduleToJson } from '../datamodel'; + +async function loadWasm(): Promise { + const wasmPath = path.join(__dirname, '..', '..', 'engine', 'core', 'libsimlin.wasm'); + const wasmBuffer = fs.readFileSync(wasmPath); + await resetWasm(); + configureWasm({ source: wasmBuffer }); + await ready(); +} + +// A project whose variables carry every field that datamodel.ts previously +// dropped. Key names are camelCase to match the Rust serializer. +const BASE_PROJECT = JSON.stringify({ + name: 'roundtrip', + simSpecs: { startTime: 0, endTime: 10, dt: '1' }, + dimensions: [{ name: 'region', elements: ['north', 'south'] }], + models: [ + { + name: 'main', + stocks: [ + { + name: 'level', + inflows: [], + outflows: [], + initialEquation: '0', + // [1] ACTIVE INITIAL on a stock. + compat: { activeInitial: '5' }, + }, + ], + flows: [], + auxiliaries: [ + { + name: 'imported', + equation: '0', + // [5] external-data reference (GET DIRECT DATA). + compat: { + dataSource: { + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }, + }, + }, + { + name: 'arrayed', + arrayedEquation: { + dimensions: ['region'], + // [3] EXCEPT default equation + flag. + equation: '99', + hasExceptDefault: true, + elements: [ + { + subscript: 'north', + equation: '1', + // [4] per-element graphical function + per-element ACTIVE INITIAL. + graphicalFunction: { yPoints: [0, 1, 2], xScale: { min: 0, max: 2 }, yScale: { min: 0, max: 2 } }, + compat: { activeInitial: '3' }, + }, + ], + }, + }, + ], + modules: [ + { + name: 'sub_inst', + modelName: 'sub', + // Module compat: the engine reads canBeModuleInput, isPublic, dataSource. + compat: { + canBeModuleInput: true, + isPublic: true, + dataSource: { + kind: 'constants', + file: 'c.csv', + tabOrDelimiter: ',', + rowOrCol: '1', + cell: 'A1', + }, + }, + }, + ], + }, + { + name: 'sub', + stocks: [], + flows: [], + auxiliaries: [{ name: 'out', equation: '1' }], + }, + ], +}); + +interface ParsedAux { + name: string; + compat?: { activeInitial?: string; dataSource?: Record }; + arrayedEquation?: { + equation?: string; + hasExceptDefault?: boolean; + elements?: { + subscript: string; + equation: string; + compat?: { activeInitial?: string }; + graphicalFunction?: { yPoints?: number[] }; + }[]; + }; +} +interface ParsedStock { + name: string; + compat?: { activeInitial?: string }; +} +interface ParsedModule { + name: string; + compat?: { canBeModuleInput?: boolean; isPublic?: boolean; dataSource?: Record }; +} +interface ParsedModel { + name: string; + stocks: ParsedStock[]; + auxiliaries: ParsedAux[]; + modules?: ParsedModule[]; +} +interface ParsedProject { + models: ParsedModel[]; +} + +async function serializeProject(json: string): Promise { + const project = await Project.openJson(json); + try { + return JSON.parse(await project.serializeJson()) as ParsedProject; + } finally { + await project.dispose(); + } +} + +function findAux(parsed: ParsedProject, name: string): ParsedAux { + const aux = parsed.models[0].auxiliaries.find((a) => a.name === name); + if (!aux) { + throw new Error(`aux ${name} not found`); + } + return aux; +} + +function assertAllFieldsPresent(parsed: ParsedProject): void { + const stock = parsed.models[0].stocks.find((s) => s.name === 'level'); + expect(stock?.compat?.activeInitial).toBe('5'); + + const imported = findAux(parsed, 'imported'); + expect(imported.compat?.dataSource).toEqual({ + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }); + + const arrayed = findAux(parsed, 'arrayed'); + expect(arrayed.arrayedEquation?.equation).toBe('99'); + expect(arrayed.arrayedEquation?.hasExceptDefault).toBe(true); + const north = arrayed.arrayedEquation?.elements?.find((e) => e.subscript === 'north'); + expect(north?.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(north?.compat?.activeInitial).toBe('3'); + + const module = parsed.models[0].modules?.find((m) => m.name === 'sub_inst'); + expect(module?.compat?.canBeModuleInput).toBe(true); + expect(module?.compat?.isPublic).toBe(true); + expect(module?.compat?.dataSource).toEqual({ + kind: 'constants', + file: 'c.csv', + tabOrDelimiter: ',', + rowOrCol: '1', + cell: 'A1', + }); +} + +describe('datamodel round-trip through the real engine serializer', () => { + beforeAll(async () => { + await loadWasm(); + }); + + it('the engine preserves the fields the editor must round-trip (validates key names)', async () => { + // Sanity: the serializer keeps every field when the JSON uses these keys. + assertAllFieldsPresent(await serializeProject(BASE_PROJECT)); + }); + + it('survives a datamodel fromJson -> toJson cycle re-fed to the engine', async () => { + const engineJson = await serializeProject(BASE_PROJECT); + + // The exact editor in-memory transform: engine JSON -> datamodel -> JSON. + const datamodel = projectFromJson(engineJson as never); + const rebuiltJson = JSON.stringify(projectToJson(datamodel)); + + // Re-open the datamodel-produced JSON in the engine and re-serialize: a key + // datamodel.ts got wrong on read OR write would be gone by now. + assertAllFieldsPresent(await serializeProject(rebuiltJson)); + }); + + it('survives a full upsert replace (the literal data-loss bug)', async () => { + const project = await Project.openJson(BASE_PROJECT); + try { + const datamodel = projectFromJson(JSON.parse(await project.serializeJson()) as never); + const model = datamodel.models.get('main'); + if (!model) { + throw new Error('main model missing'); + } + + const level = model.variables.get('level'); + const imported = model.variables.get('imported'); + const arrayed = model.variables.get('arrayed'); + const subInst = model.variables.get('sub_inst'); + if ( + level?.type !== 'stock' || + imported?.type !== 'aux' || + arrayed?.type !== 'aux' || + subInst?.type !== 'module' + ) { + throw new Error('expected variables missing'); + } + + // Rebuild each variable as a full upsert -- exactly what the editor sends + // when the user edits any single field. allowErrors so model-validity + // diagnostics never mask the survival assertion. + const patch: JsonProjectPatch = { + models: [ + { + name: 'main', + ops: [ + { type: 'upsertStock', payload: { stock: stockToJson(level) } }, + { type: 'upsertAux', payload: { aux: auxToJson(imported) } }, + { type: 'upsertAux', payload: { aux: auxToJson(arrayed) } }, + { type: 'upsertModule', payload: { module: moduleToJson(subInst) } }, + ], + }, + ], + }; + await project.applyPatch(patch, { allowErrors: true }); + + assertAllFieldsPresent(JSON.parse(await project.serializeJson()) as ParsedProject); + } finally { + await project.dispose(); + } + }); +}); diff --git a/src/core/tests/datamodel.test.ts b/src/core/tests/datamodel.test.ts index 5d5dff23d..f31498ae4 100644 --- a/src/core/tests/datamodel.test.ts +++ b/src/core/tests/datamodel.test.ts @@ -48,6 +48,8 @@ import type { ScalarEquation, ApplyToAllEquation, ArrayedEquation, + ArrayedElement, + DataSource, SimSpecs, Dimension, Source, @@ -68,6 +70,9 @@ import type { JsonFlowViewElement, JsonLinkViewElement, JsonCloudViewElement, + JsonStock, + JsonAuxiliary, + JsonModule, } from '@simlin/engine'; import { defined, type Series } from '../common'; @@ -141,6 +146,8 @@ describe('Stock', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -202,6 +209,8 @@ describe('Flow', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -239,6 +248,8 @@ describe('Aux', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -271,6 +282,8 @@ describe('Aux', () => { gf, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -296,6 +309,9 @@ describe('Module', () => { documentation: 'A sector submodel', units: '', references: [{ src: 'input_var', dst: 'sector_input' }], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -327,6 +343,9 @@ describe('Module', () => { documentation: '', units: '', references: [], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -356,6 +375,9 @@ describe('Module', () => { { src: '', dst: '' }, { src: 'food', dst: '' }, ], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -380,6 +402,9 @@ describe('Module', () => { documentation: '', units: '', references: [{ src: 'food', dst: 'sector·sector_input' }], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -389,6 +414,30 @@ describe('Module', () => { const restored = moduleFromJson(moduleToJson(mod)); expect(restored.references[0].dst).toBe('sector·sector_input'); }); + + it('round-trips module compat (canBeModuleInput, isPublic, dataSource)', () => { + // The engine's From (json.rs) reads these three compat fields and + // defaults the rest, so a module edit must preserve exactly these. + const json: JsonModule = { + name: 'sector', + modelName: 'SectorModel', + compat: { + canBeModuleInput: true, + isPublic: true, + dataSource: { kind: 'constants', file: 'c.csv', tabOrDelimiter: ',', rowOrCol: '1', cell: 'A1' }, + }, + }; + const restored = moduleFromJson(json); + expect(restored.canBeModuleInput).toBe(true); + expect(restored.isPublic).toBe(true); + expect(restored.dataSource?.kind).toBe('constants'); + expect(restored.dataSource?.file).toBe('c.csv'); + + const out = moduleToJson(restored); + expect(out.compat?.canBeModuleInput).toBe(true); + expect(out.compat?.isPublic).toBe(true); + expect(out.compat?.dataSource).toEqual(json.compat!.dataSource); + }); }); describe('View Elements', () => { @@ -837,6 +886,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -859,6 +910,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -880,6 +933,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: true, canBeModuleInput: true, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -924,6 +979,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -945,6 +1002,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: true, canBeModuleInput: false, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -988,6 +1047,8 @@ describe('canBeModuleInput and isPublic', () => { gf: undefined, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1008,6 +1069,8 @@ describe('canBeModuleInput and isPublic', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1035,6 +1098,8 @@ describe('Arrayed Equations', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1061,16 +1126,20 @@ describe('Arrayed Equations', () => { equation: { type: 'arrayed', dimensionNames: ['regions'], - elements: new Map([ - ['north', '50'], - ['south', '75'], + elements: new Map([ + ['north', { equation: '50', graphicalFunction: undefined, activeInitial: undefined }], + ['south', { equation: '75', graphicalFunction: undefined, activeInitial: undefined }], ]), + defaultEquation: undefined, + hasExceptDefault: false, }, documentation: 'Demand by region', units: '', gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1087,8 +1156,8 @@ describe('Arrayed Equations', () => { const eq = restored.equation as ArrayedEquation; expect(eq.dimensionNames).toEqual(['regions']); expect(eq.elements.size).toBe(2); - expect(eq.elements.get('north')).toBe('50'); - expect(eq.elements.get('south')).toBe('75'); + expect(eq.elements.get('north')?.equation).toBe('50'); + expect(eq.elements.get('south')?.equation).toBe('75'); }); }); @@ -1150,6 +1219,8 @@ describe('Model', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1166,6 +1237,8 @@ describe('Model', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1209,6 +1282,8 @@ describe('Project', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1225,6 +1300,8 @@ describe('Project', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1240,6 +1317,8 @@ describe('Project', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1347,6 +1426,8 @@ describe('projectAttachData', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1461,3 +1542,159 @@ describe('projectAttachData', () => { expect(v?.data).toBeUndefined(); }); }); + +// These tests pin the wire fields that an editor upsert (a full variable +// replace by UID) previously dropped, because datamodel.ts neither read them +// on the way in nor wrote them on the way out. The JSON key names are the +// source of truth in the Rust serializer src/simlin-engine/src/json.rs +// (all #[serde(rename_all = "camelCase")]): +// - Compat: activeInitial, nonNegative, canBeModuleInput, isPublic, dataSource +// - JsonDataSource: kind ("data"/"constants"/"lookups"/"subscript"), file, +// tabOrDelimiter, rowOrCol, cell +// - ArrayedEquation: dimensions, equation (the EXCEPT default), elements, +// hasExceptDefault +// - ElementEquation: subscript, equation, compat, graphicalFunction +// The engine-driven round-trip in datamodel-roundtrip-e2e.test.ts re-checks +// these names against the real serializer, so a wrong key here cannot pass +// silently. +describe('variable compat round-trip (silent data-loss regression)', () => { + it('round-trips activeInitial on a stock through compat', () => { + const json: JsonStock = { + name: 'level', + inflows: [], + outflows: [], + initialEquation: '0', + compat: { activeInitial: 'starting_level' }, + }; + const restored = stockFromJson(json); + expect(restored.activeInitial).toBe('starting_level'); + expect(stockToJson(restored).compat?.activeInitial).toBe('starting_level'); + }); + + it('round-trips activeInitial on a flow and an aux through compat', () => { + const flow = flowFromJson({ name: 'rate', equation: '1', compat: { activeInitial: '2' } }); + expect(flow.activeInitial).toBe('2'); + expect(flowToJson(flow).compat?.activeInitial).toBe('2'); + + const aux = auxFromJson({ name: 'a', equation: '1', compat: { activeInitial: '3' } }); + expect(aux.activeInitial).toBe('3'); + expect(auxToJson(aux).compat?.activeInitial).toBe('3'); + }); + + it('round-trips compat.dataSource (GET DIRECT DATA) on an aux', () => { + const json: JsonAuxiliary = { + name: 'imported', + compat: { + dataSource: { + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }, + }, + }; + const restored = auxFromJson(json); + const ds = restored.dataSource as DataSource; + expect(ds.kind).toBe('data'); + expect(ds.file).toBe('data.xlsx'); + expect(ds.tabOrDelimiter).toBe('Sheet1'); + expect(ds.rowOrCol).toBe('A'); + expect(ds.cell).toBe('B2'); + + const out = auxToJson(restored).compat?.dataSource; + expect(out).toEqual(json.compat!.dataSource); + }); + + it('maps each dataSource kind and falls back to "data" for unknown', () => { + for (const kind of ['data', 'constants', 'lookups', 'subscript'] as const) { + const aux = auxFromJson({ + name: 'x', + compat: { dataSource: { kind, file: 'f', tabOrDelimiter: '', rowOrCol: '', cell: '' } }, + }); + expect(aux.dataSource?.kind).toBe(kind); + } + const unknown = auxFromJson({ + name: 'x', + compat: { dataSource: { kind: 'bogus', file: 'f', tabOrDelimiter: '', rowOrCol: '', cell: '' } }, + }); + expect(unknown.dataSource?.kind).toBe('data'); + }); + + it('round-trips the EXCEPT default equation and hasExceptDefault flag', () => { + const json: JsonAuxiliary = { + name: 'arr', + arrayedEquation: { + dimensions: ['dim'], + equation: '99', + hasExceptDefault: true, + elements: [{ subscript: 'a', equation: '1' }], + }, + }; + const restored = auxFromJson(json); + const eq = restored.equation as ArrayedEquation; + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(true); + + const out = auxToJson(restored).arrayedEquation!; + expect(out.equation).toBe('99'); + expect(out.hasExceptDefault).toBe(true); + }); + + it('infers hasExceptDefault from a present default when the flag is absent', () => { + // Mirrors the engine: legacy JSON without the flag infers true when a + // default equation is present. + const restored = auxFromJson({ + name: 'arr', + arrayedEquation: { dimensions: ['dim'], equation: '7', elements: [{ subscript: 'a', equation: '1' }] }, + }); + expect((restored.equation as ArrayedEquation).hasExceptDefault).toBe(true); + + // No default equation => no flag emitted on the way out. + const noDefault = auxFromJson({ + name: 'arr', + arrayedEquation: { dimensions: ['dim'], elements: [{ subscript: 'a', equation: '1' }] }, + }); + const eq = noDefault.equation as ArrayedEquation; + expect(eq.defaultEquation).toBeUndefined(); + expect(eq.hasExceptDefault).toBe(false); + const out = auxToJson(noDefault).arrayedEquation!; + expect(out.equation).toBeUndefined(); + expect(out.hasExceptDefault).toBeUndefined(); + }); + + it('round-trips per-element graphical functions and per-element activeInitial', () => { + const json: JsonAuxiliary = { + name: 'arr', + arrayedEquation: { + dimensions: ['dim'], + elements: [ + { + subscript: 'a', + equation: '1', + graphicalFunction: { yPoints: [0, 1, 2], xScale: { min: 0, max: 2 }, yScale: { min: 0, max: 2 } }, + compat: { activeInitial: 'a0' }, + }, + { subscript: 'b', equation: '2' }, + ], + }, + }; + const restored = auxFromJson(json); + const eq = restored.equation as ArrayedEquation; + const a = eq.elements.get('a') as ArrayedElement; + expect(a.equation).toBe('1'); + expect(a.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(a.activeInitial).toBe('a0'); + const b = eq.elements.get('b') as ArrayedElement; + expect(b.graphicalFunction).toBeUndefined(); + expect(b.activeInitial).toBeUndefined(); + + const out = auxToJson(restored).arrayedEquation!; + const outA = out.elements!.find((e) => e.subscript === 'a')!; + expect(outA.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(outA.compat?.activeInitial).toBe('a0'); + const outB = out.elements!.find((e) => e.subscript === 'b')!; + expect(outB.graphicalFunction).toBeUndefined(); + expect(outB.compat).toBeUndefined(); + }); +}); diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index 1e3e12806..5b5a9ea13 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -14,7 +14,13 @@ import Button from './components/Button'; import { canonicalize } from '@simlin/core/canonicalize'; import { Project as EngineProject } from '@simlin/engine'; -import type { JsonProjectPatch, JsonModelOperation, JsonSimSpecs, JsonArrayedEquation } from '@simlin/engine'; +import type { + JsonProjectPatch, + JsonModelOperation, + JsonSimSpecs, + JsonArrayedEquation, + JsonModule, +} from '@simlin/engine'; import { stockFlowViewToJson } from './view-conversion'; import { Project, @@ -36,6 +42,7 @@ import { flowToJson, auxToJson, moduleToJson, + arrayedEquationToJson, type ModuleReference, } from '@simlin/core/datamodel'; import { defined, exists, setsEqual } from '@simlin/core/common'; @@ -1553,15 +1560,10 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }, }; } else if (eq.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...eq.dimensionNames], - elements: [...eq.elements.entries()].map(([subscript, eqStr]) => ({ - subscript, - equation: eqStr, - })), - }, - }; + // Use the shared serializer so per-element graphical functions, per-element + // ACTIVE INITIAL equations, and the EXCEPT default round-trip through the + // upsert (a hand-rolled mapping here previously dropped them). + return { arrayedEquation: arrayedEquationToJson(eq) }; } return { equation: '' }; }; @@ -1620,14 +1622,15 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }, }; } else if (variable.type === 'module') { - // Modules have no equations or graphical functions -- only units and docs + // Modules have no equations or graphical functions -- only units and docs. + // Use moduleToJson to preserve all fields (including compat flags + // canBeModuleInput, isPublic, dataSource), then override edited fields. + const base = moduleToJson(variable); op = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), + ...base, units: newUnits ?? variable.units ?? undefined, documentation: newDocs ?? variable.documentation ?? undefined, }, @@ -1732,15 +1735,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override the model ref. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, + ...moduleToJson(variable), modelName: newModelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), - units: variable.units || undefined, - documentation: variable.documentation || undefined, }, }, }; @@ -1762,13 +1763,12 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override units/docs. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), + ...moduleToJson(variable), units: newUnits ?? variable.units ?? undefined, documentation: newDocs ?? variable.documentation ?? undefined, }, @@ -1794,15 +1794,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override references. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, + ...moduleToJson(variable), references: newReferences.map((ref) => ({ src: ref.src, dst: ref.dst })), - units: variable.units || undefined, - documentation: variable.documentation || undefined, }, }, }; @@ -1830,26 +1828,14 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac newModelName = getUniqueDuplicateName(moduleIdent, project); } - // Look up existing module to preserve metadata through the model reference change + // Look up existing module to preserve metadata (including compat) through the + // model reference change; moduleToJson carries every field forward. const model = getModel(); const existingModule = model?.variables.get(moduleIdent); - const modulePayload: { - name: string; - modelName: string; - references?: { src: string; dst: string }[]; - units?: string; - documentation?: string; - } = { - name: moduleIdent, - modelName: newModelName, - }; - if (existingModule && existingModule.type === 'module') { - if (existingModule.references.length > 0) { - modulePayload.references = existingModule.references.map((ref) => ({ src: ref.src, dst: ref.dst })); - } - if (existingModule.units) modulePayload.units = existingModule.units; - if (existingModule.documentation) modulePayload.documentation = existingModule.documentation; - } + const modulePayload: JsonModule = + existingModule && existingModule.type === 'module' + ? { ...moduleToJson(existingModule), name: moduleIdent, modelName: newModelName } + : { name: moduleIdent, modelName: newModelName }; const patch: JsonProjectPatch = { projectOps: [{ type: 'addModel', payload: { name: newModelName } }], diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index d2efdb16a..fd48f2f17 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -109,6 +109,8 @@ function makeStockVar(ident: string, inflows: string[] = [], outflows: string[] nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/diagram/tests/module-details-utils.test.ts b/src/diagram/tests/module-details-utils.test.ts index c39231e69..6a66dddca 100644 --- a/src/diagram/tests/module-details-utils.test.ts +++ b/src/diagram/tests/module-details-utils.test.ts @@ -65,6 +65,9 @@ function makeModule(ident: string, modelName: string): Module { documentation: '', units: '', references: [], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/diagram/tests/module-details.test.tsx b/src/diagram/tests/module-details.test.tsx index 393779f8d..17799163a 100644 --- a/src/diagram/tests/module-details.test.tsx +++ b/src/diagram/tests/module-details.test.tsx @@ -20,6 +20,8 @@ function makeAux(ident: string, overrides?: Partial): Aux { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -40,6 +42,8 @@ function makeStock(ident: string, overrides?: Partial): Stock { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -56,6 +60,9 @@ function makeModule(ident: string, modelName: string, overrides?: Partial = {}): Variable { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/engine/src/json-types.ts b/src/engine/src/json-types.ts index 9eabcef10..41885c713 100644 --- a/src/engine/src/json-types.ts +++ b/src/engine/src/json-types.ts @@ -30,6 +30,21 @@ export interface JsonGraphicalFunction { yScale?: JsonGraphicalFunctionScale; } +/** + * External-data reference (Vensim GET DIRECT DATA / CONSTANTS / LOOKUPS / + * SUBSCRIPT). Field names and the `kind` string values match the Rust + * `json::JsonDataSource` serializer in src/simlin-engine/src/json.rs + * (`#[serde(rename_all = "camelCase")]`; kind is one of + * `data`/`constants`/`lookups`/`subscript`). + */ +export interface JsonDataSource { + kind: string; + file: string; + tabOrDelimiter: string; + rowOrCol: string; + cell: string; +} + /** * Vensim compatibility options for a variable. */ @@ -38,6 +53,7 @@ export interface JsonCompat { nonNegative?: boolean; canBeModuleInput?: boolean; isPublic?: boolean; + dataSource?: JsonDataSource; } /** @@ -58,6 +74,12 @@ export interface JsonArrayedEquation { equation?: string; compat?: JsonCompat; elements?: JsonElementEquation[]; + /** + * Whether `equation` is an EXCEPT default applied to elements not covered + * by `elements` (Vensim EXCEPT). Matches the Rust `hasExceptDefault` field; + * only meaningful when a default `equation` is present. + */ + hasExceptDefault?: boolean; } /** From 56c26bb35760924e84b26b0e997882bbec56009d Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 18:12:29 -0700 Subject: [PATCH 02/11] diagram: record undo history for canvas content and layout edits Creating, deleting, moving, and connecting elements on the canvas were not undoable. Those handlers commit through applyPatchOrReportError (which does not refresh or record) and/or updateView (which recorded with recordHistory:false), so only equation/table/module/rename/sim-spec edits -- which go through applyPatch/refreshFromEngine -- advanced the undo buffer. This regressed the pre-refactor behavior where every create/delete/move was individually undoable, and made accidental deletes unrecoverable. Add a recordHistory option to ProjectController.updateView and pass it from the six discrete-edit handlers (create, delete, element/label move, flow attach, link attach). Each records exactly one entry capturing the engine state after both the content patch and the view update. The per-frame viewport stream (pan/zoom/momentum/resize) keeps going through queueViewUpdate, which still records nothing, so a momentum flick cannot evict real edits from the small (5-entry) undo buffer. Update the now-stale generation/panel-keying invariants: discrete layout edits bump projectGeneration and remount open detail panels, which is safe because a canvas edit first blurs and commits the side-panel editor. --- src/diagram/CLAUDE.md | 6 +- src/diagram/Editor.tsx | 19 +++--- src/diagram/project-controller.ts | 24 +++++-- src/diagram/tests/project-controller.test.ts | 72 ++++++++++++++++++++ 4 files changed, 105 insertions(+), 16 deletions(-) diff --git a/src/diagram/CLAUDE.md b/src/diagram/CLAUDE.md index a4b22838c..ecba3b89e 100644 --- a/src/diagram/CLAUDE.md +++ b/src/diagram/CLAUDE.md @@ -67,9 +67,9 @@ Most project/engine invariants now live in `ProjectController` (`project-control - **External view overrides a live gesture** (Canvas, issue #707): a `useEffect` keyed on `[props.view, liveViewport]` compares `props.view`'s offset/zoom VALUE against `refs.viewBaseline` (tracked while idle). While a gesture is live, `props.view` is expected to stay put (a gesture never commits mid-flight), so any value change seen with `liveViewport` still set is external (centerVariable, module navigation, undo) and drops the live viewport + cancels pending momentum/deferred commits -- the external view wins, with no stray commit. A self-commit clears `liveViewport` in the same React commit as its optimistic `props.view` update, so it is never misread as external; comparing by value (not snapshot identity) ignores a content-equal republished view. If a pointer-driven viewport gesture (drag-pan or pinch) is still physically in progress when this fires, it is also abandoned (interaction -> idle; `panBaseOffset`/`mouseDownPoint`/`pointerId`/`activePointers` cleared) -- otherwise a continued move would recreate `liveViewport` from the stale press-time anchor and the pointer-up could commit that abandoned gesture back over the external view. - **Optimistic view updates** (controller): `updateView()`/`queueViewUpdate()` call `applyOptimisticView()` (synchronous snapshot replace of the active model's view + `projectVersion += 0.001`) *before* awaiting the engine round-trip. Any new view-modifying handler must go through these controller methods to avoid flicker. - **updateProject preserves the live view** (controller): `ProjectController.updateProject()` rebuilds `project` from the engine's serialized JSON, then merges via `preserveLiveView()` so the active model's view comes from the live snapshot (the most recent optimistic view). Without this, a slow engine round-trip racing with a newer pan/move would snap the diagram back to the engine's older view. The live view is round-tripped through JSON to re-link element `var` refs and stock inflow/outflow UIDs against the incoming variables. -- **View-only updates never record undo history** (controller): the `queueViewUpdate` path calls `updateProject(serialized, { recordHistory: false, scheduleSave: false })` -- it refreshes `project` and bumps `projectVersion` but must not touch `projectHistory`/`projectOffset`: viewBox/zoom are serialized into the protobuf, so recording them would let a single momentum flick evict every real edit from the `MaxUndoSize` (5) buffer. Real edits go through `advanceProjectHistory` (project-history.ts), which discards the redo branch when editing after an undo. -- **Version vs generation bookkeeping** (controller): `projectVersion` carries the fractional cache-key scheme (`+0.01` on content edits/undo, `+0.001` on view-only updates; Canvas keys render caches off it). `projectGeneration` increments *only* when project content changes (history-recording edits and undo/redo), never on view-only updates or save-version writes. -- **Details panels are keyed by `projectGeneration`, not `projectVersion`** (Editor render): VariableDetails/ModuleDetails seed their Slate editors from props in **lazy `useState` initializers** (one run per mount, the function-component equivalent of the old constructor seed), so key-driven remounts -- not prop-sync effects -- are what refreshes them. The Editor builds the panel key from `controllerSnapshot.projectGeneration`; keying on `projectVersion` would remount an open panel on every pan frame and autosave completion, discarding in-progress edits. +- **Viewport-only updates never record undo history; discrete element edits do** (controller): there are two view-mutating paths and they record differently. The `queueViewUpdate` path (pan/zoom/momentum/resize, and the internal navigation/replay viewport restores) calls `updateProject(serialized, { recordHistory: false, scheduleSave: false })` -- it refreshes `project` and bumps `projectVersion` but must NEVER touch `projectHistory`/`projectOffset`: viewBox/zoom are serialized into the protobuf, so recording them would let a single momentum flick evict every real edit from the `MaxUndoSize` (5) buffer. The `updateView` path is for DISCRETE element/structure edits (create, delete, element/group move, label move, flow/link attach); each such edit passes `{ recordHistory: true }` so it becomes individually undoable, producing exactly one entry per edit. For the delete/flow-attach/create handlers that apply a content patch via `applyPatchOrReportError` BEFORE calling `updateView`, the snapshot recorded in that final `updateView` captures the engine state after BOTH the content patch and the view update -- one entry, no double-recording. Bare `updateView()` (no opts) stays non-recording. All recorded edits go through `advanceProjectHistory` (project-history.ts), which discards the redo branch when editing after an undo. +- **Version vs generation bookkeeping** (controller): `projectVersion` carries the fractional cache-key scheme (`+0.01` on content edits/undo, `+0.001` on view-only updates; Canvas keys render caches off it). `projectGeneration` increments on every history-recording edit and on undo/redo -- that is, both model-content edits (equation/table/module/sim-specs/rename via `applyPatch`/`applyPatchAndRefresh`) AND discrete layout edits (element/label move, flow/link attach, create, delete) that call `updateView` with `recordHistory: true`. It never increments on the viewport-only stream (`queueViewUpdate`: pan/zoom/momentum/resize) or on save-version writes. +- **Details panels are keyed by `projectGeneration`, not `projectVersion`** (Editor render): VariableDetails/ModuleDetails seed their Slate editors from props in **lazy `useState` initializers** (one run per mount, the function-component equivalent of the old constructor seed), so key-driven remounts -- not prop-sync effects -- are what refreshes them. The Editor builds the panel key from `controllerSnapshot.projectGeneration`; keying on `projectVersion` would remount an open panel on every pan frame and autosave completion, discarding in-progress edits. Discrete layout edits (element/label move, flow/link attach) now bump `projectGeneration` and so remount an open panel; this is safe because a canvas-driven edit first blurs the side-panel editor, which commits its in-progress text (the same blur-commit the click-away flows already rely on) before the move records. - **Save queue releases `inSave` in a `finally` block** (controller): a thrown save (e.g. host-side network failure) must not leave `inSave === true`, otherwise every subsequent edit silently queues forever. A save requested while one is in flight queues exactly one flush; the queued retry uses `version ?? currVersion` so a save that errored before the server returned a new version still attempts to flush the next edit. - **Engine lifecycle / StrictMode safety** (controller + Editor): the controller's `disposed` flag latches once; every async continuation short-circuits on it and releases any engine it opened (collapsing the old `unmounted`/`openInitialProjectTimer`/`undoRedoTimer` machinery). The Editor constructs a controller once in its `refs`-init guard and its mount effect rebuilds one whenever a prior cleanup disposed and cleared it, so a React 18 StrictMode mount -> unmount -> mount cycle builds a *fresh* controller on the second mount. - **Module navigation stack** (controller): the controller owns `modelName` and an immutable `modelStack` of `ModuleStackEntry` (each storing the child model name, module ident, and the parent's selection/viewBox/zoom). All navigation goes through `pushModule`/`popModule`/`navigateToLevel` pure functions; the controller's navigation methods restore the parent viewport internally via `queueViewUpdate` (its `modelName` updates synchronously first, so `getView()` resolves to the restored model with no deferral) and the model-scoped error cache via `refreshCachedErrors`. They return a `NavigationOutcome` with the selection the Editor should adopt. The undo-driven navigation reset (restored project lacks the viewed model -> reset to 'main') bumps `navResetSeq`; a post-commit `useEffect` in the Editor clears selection/details/tool exactly once per bump (an ordinary undo preserves them). diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index 5b5a9ea13..de53197aa 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -651,8 +651,11 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac r.controller?.scheduleSimRun(); }; - const updateView = async (view: StockFlowView): Promise => { - await r.controller?.updateView(view); + // Discrete element/structure edits pass { recordHistory: true } so each one + // becomes individually undoable; the per-frame viewport stream goes through + // queueViewUpdate (never records). See the controller's updateView doc. + const updateView = async (view: StockFlowView, opts?: { recordHistory?: boolean }): Promise => { + await r.controller?.updateView(view, opts); }; const queueViewUpdate = async (view: StockFlowView): Promise => { @@ -845,7 +848,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac await applyPatchOrReportError(patch, 'delete'); } - await updateView({ ...view, elements, nextUid }); + await updateView({ ...view, elements, nextUid }, { recordHistory: true }); scheduleSimRun(); }, []); @@ -860,7 +863,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac return { ...element, labelSide: side }; }); - await updateView({ ...view, elements }); + await updateView({ ...view, elements }, { recordHistory: true }); }, [], ); @@ -913,7 +916,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac } } - await updateView({ ...view, nextUid: result.nextUid, elements: [...result.elements] }); + await updateView({ ...view, nextUid: result.nextUid, elements: [...result.elements] }, { recordHistory: true }); setState({ selection, flowStillBeingCreated: inCreation, @@ -960,7 +963,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac } view = { ...view, nextUid, elements }; - await updateView(view); + await updateView(view, { recordHistory: true }); setState({ selection }); }, []); @@ -1036,7 +1039,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac // even when the upsert errored). await applyPatchOrReportError(patch, 'variable creation'); - await updateView({ ...view, nextUid, elements }); + await updateView({ ...view, nextUid, elements }, { recordHistory: true }); setState({ selection: new Set(), }); @@ -1056,7 +1059,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }); const elements = view.elements.map((el) => updatedElements.get(el.uid) ?? el); - await updateView({ ...view, elements }); + await updateView({ ...view, elements }, { recordHistory: true }); }, [], ); diff --git a/src/diagram/project-controller.ts b/src/diagram/project-controller.ts index 960406d00..7dd574580 100644 --- a/src/diagram/project-controller.ts +++ b/src/diagram/project-controller.ts @@ -667,11 +667,25 @@ export class ProjectController { } /** - * Optimistic view update: reflect the new view in the snapshot immediately - * (so the UI never flashes stale positions), then round-trip through the - * engine. View edits never record undo history. + * Optimistic view update for a DISCRETE element/structure edit (create, + * delete, element/group move, label move, flow/link attach): reflect the new + * view in the snapshot immediately (so the UI never flashes stale positions), + * then round-trip through the engine. + * + * `recordHistory` controls whether this edit advances the undo buffer; it + * defaults to false so the bare call stays a non-recording view refresh. Each + * discrete user edit passes `recordHistory: true` so it becomes individually + * undoable. For the handlers that apply a content patch (via + * applyPatchOrReportError) BEFORE calling this, the snapshot serialized here + * captures the engine state AFTER both the content patch and the view update, + * so a single recorded entry covers the whole edit -- no double-recording. + * + * The per-frame viewport stream (pan/zoom/momentum/resize) does NOT come + * through here -- it uses queueViewUpdate, which never records, so a momentum + * flick cannot evict real edits from the small undo buffer. */ - async updateView(view: StockFlowView): Promise { + async updateView(view: StockFlowView, opts: { recordHistory?: boolean } = {}): Promise { + const { recordHistory = false } = opts; this.applyOptimisticView(view); const engine = this.engine; @@ -687,7 +701,7 @@ export class ProjectController { this.reportError(err.message ?? 'Unknown error during view update'); return; } - await this.updateProject(await engine.serializeProtobuf(), { scheduleSave: true, recordHistory: false }); + await this.updateProject(await engine.serializeProtobuf(), { scheduleSave: true, recordHistory }); } /** diff --git a/src/diagram/tests/project-controller.test.ts b/src/diagram/tests/project-controller.test.ts index 87c3f2dd0..587b905d2 100644 --- a/src/diagram/tests/project-controller.test.ts +++ b/src/diagram/tests/project-controller.test.ts @@ -204,6 +204,78 @@ describe('ProjectController optimistic view updates', () => { await controller.dispose(); }); + it('updateView records undo history only when recordHistory is set', async () => { + // Discrete element/structure edits (create/delete/move/flow-attach/etc.) + // funnel their final engine state through updateView and must each produce + // exactly one undo entry. A plain updateView (the legacy default) and the + // viewport-only queueViewUpdate path must record nothing. + const engine = makeFakeEngine(); + const { config } = makeControllerConfig({ engine, format: 'protobuf', initialData: snap(1) }); + const controller = new ProjectController(config); + await controller.openInitialProject(); + + const view = controller.getView() as StockFlowView; + + // A plain updateView (no opts) refreshes the project but records nothing. + const genBefore = controller.getSnapshot().projectGeneration; + await controller.updateView({ ...view, zoom: 2 }); + expect(controller.canUndo()).toBe(false); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore); + + // queueViewUpdate (pan/zoom/momentum) likewise records nothing. + await controller.queueViewUpdate({ ...view, zoom: 3 }); + expect(controller.canUndo()).toBe(false); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore); + + // A discrete edit (recordHistory: true) advances history exactly once. + await controller.updateView({ ...view, zoom: 4 }, { recordHistory: true }); + expect(controller.canUndo()).toBe(true); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore + 1); + + // undo then redo round-trips the cursor. + controller.undoRedo('undo'); + await flushTimers(); + expect(controller.canRedo()).toBe(true); + controller.undoRedo('redo'); + await flushTimers(); + expect(controller.canRedo()).toBe(false); + await controller.dispose(); + }); + + it('undo after a recordHistory updateView reopens the engine from the pre-edit snapshot', async () => { + // Prove restoration, not just the cursor move: the undo reopen must pull the + // serialized snapshot captured BEFORE the edit back into the engine. + const openedWith: Uint8Array[] = []; + let counter = 100; + const engine = makeFakeEngine({ protobuf: () => new Uint8Array([counter++]) }); + const config = { + initialProjectVersion: 1, + input: { format: 'protobuf' as const, data: new Uint8Array([1]) }, + openProtobuf: async (data: Uint8Array) => { + openedWith.push(data); + return engine; + }, + openJson: async () => engine, + save: async () => 1, + onError: () => {}, + }; + const controller = new ProjectController(config); + await controller.openInitialProject(); + // History head is the post-open snapshot ([100]). + const view = controller.getView() as StockFlowView; + await controller.updateView({ ...view, zoom: 9 }, { recordHistory: true }); + // The edit recorded a fresh head, so there is a distinct pre-edit snapshot + // to restore (without this the undo would merely clamp to the only entry). + expect(controller.canUndo()).toBe(true); + + controller.undoRedo('undo'); + await flushTimers(); + + // The reopen restored the pre-edit snapshot ([100]) into the engine. + expect(openedWith[openedWith.length - 1]).toEqual(new Uint8Array([100])); + await controller.dispose(); + }); + it('stashes the queued view when no engine is installed yet and replays it on reopen', async () => { // Drive queueViewUpdate before any engine exists (controller just // constructed), then confirm the next engine pulls the queued view. From 7f015d3bdd216350735b53525b6589acf1d2b709 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 18:21:53 -0700 Subject: [PATCH 03/11] deploy: pin engines.pnpm in the staged server manifest The staged GAE deploy generates a self-contained server package.json that pins packageManager: pnpm@. App Engine's Node buildpack reads engines.pnpm and gives it precedence over packageManager/corepack, and an exact version short-circuits its registry resolution; without engines.pnpm the buildpack can fall back to its bundled pnpm and re-resolve or reject the frozen lockfile, failing the first real staged deploy. Derive engines.pnpm from the same packageManager spec (single source of truth), stripping any +sha512 build suffix so the bare exact version takes the buildpack's fast path. Preserve any engines the server already declared, with the pnpm pin winning. --- scripts/deploy-staging-manifests.mjs | 25 ++++++++++++- .../tests/deploy-staging-manifests.test.mjs | 36 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/scripts/deploy-staging-manifests.mjs b/scripts/deploy-staging-manifests.mjs index 3711a88b4..2baab6b8a 100644 --- a/scripts/deploy-staging-manifests.mjs +++ b/scripts/deploy-staging-manifests.mjs @@ -81,7 +81,9 @@ function rewriteDepBlock(deps, fileRef, context) { * * @param serverPkg parsed src/server/package.json * @param opts.vendorDir relative dir the workspace packages are vendored under (default ./vendor) - * @param opts.packageManager optional packageManager string to pin (e.g. "pnpm@10.6.0") + * @param opts.packageManager optional packageManager string to pin (e.g. "pnpm@10.6.0"). + * When it is a `pnpm@` spec, engines.pnpm is also pinned to that + * version (the GAE buildpack reads engines.pnpm, not corepack/packageManager). */ export function buildStagingServerManifest(serverPkg, opts = {}) { const { vendorDir = './vendor', packageManager } = opts; @@ -114,8 +116,29 @@ export function buildStagingServerManifest(serverPkg, opts = {}) { manifest.optionalDependencies = optionalDependencies; } + // Carry forward any engines the server declared, then pin engines.pnpm from + // the packageManager version. App Engine's Node buildpack reads engines.pnpm + // and gives it precedence over packageManager/corepack (implemented in + // GoogleCloudPlatform/buildpacks pnpm.go/registry.go), where an exact version + // short-circuits registry resolution to exactly that pnpm. Without it the + // buildpack may fall back to its bundled pnpm and re-resolve or reject our + // frozen lockfile, failing the deploy. The explicit pin wins over any + // inherited pnpm range so the buildpack uses exactly the locked version. + // packageManager is still emitted verbatim (corepack wants the full spec incl. + // the +sha512 hash); the bare version is required here because a +hash + // constraint would miss the exact-semver fast path. + const engines = { ...(serverPkg.engines ?? {}) }; if (packageManager) { manifest.packageManager = packageManager; + // packageManager spec: `pnpm@` optionally with a `+` build + // suffix (e.g. `pnpm@10.6.0+sha512.`). engines wants the bare version. + const m = /^pnpm@([^+\s]+)/.exec(packageManager.trim()); + if (m) { + engines.pnpm = m[1]; + } + } + if (Object.keys(engines).length > 0) { + manifest.engines = engines; } return manifest; } diff --git a/scripts/tests/deploy-staging-manifests.test.mjs b/scripts/tests/deploy-staging-manifests.test.mjs index 54b891967..18e5d3684 100644 --- a/scripts/tests/deploy-staging-manifests.test.mjs +++ b/scripts/tests/deploy-staging-manifests.test.mjs @@ -69,6 +69,42 @@ describe('buildStagingServerManifest', () => { assert.equal(buildStagingServerManifest(serverPkg).packageManager, undefined); }); + it('pins engines.pnpm from the packageManager version (the GAE buildpack reads engines, not corepack)', () => { + const out = buildStagingServerManifest(serverPkg, { packageManager: 'pnpm@10.6.0' }); + assert.equal(out.engines.pnpm, '10.6.0'); + }); + + it('strips the +sha512 build-metadata suffix when deriving engines.pnpm', () => { + const out = buildStagingServerManifest(serverPkg, { + packageManager: 'pnpm@10.6.0+sha512.abc123def456', + }); + assert.equal(out.engines.pnpm, '10.6.0'); + // packageManager itself is preserved verbatim (corepack still wants the hash). + assert.equal(out.packageManager, 'pnpm@10.6.0+sha512.abc123def456'); + }); + + it('adds no engines when no packageManager is provided and the server declares none', () => { + assert.equal(buildStagingServerManifest(serverPkg).engines, undefined); + }); + + it('preserves the server-declared engines when no packageManager is provided', () => { + const withEngines = { ...serverPkg, engines: { node: '>=20' } }; + const out = buildStagingServerManifest(withEngines); + assert.deepEqual(out.engines, { node: '>=20' }); + }); + + it('merges the pnpm pin into the server-declared engines, with the pin winning', () => { + const withEngines = { ...serverPkg, engines: { node: '>=20', pnpm: '>=9' } }; + const out = buildStagingServerManifest(withEngines, { packageManager: 'pnpm@10.6.0' }); + assert.deepEqual(out.engines, { node: '>=20', pnpm: '10.6.0' }); + }); + + it('adds no engines.pnpm for a non-pnpm packageManager, but keeps packageManager', () => { + const out = buildStagingServerManifest(serverPkg, { packageManager: 'yarn@4.1.0' }); + assert.equal(out.packageManager, 'yarn@4.1.0'); + assert.equal(out.engines, undefined); + }); + it('produces a manifest with no residual workspace: protocol', () => { const out = buildStagingServerManifest(serverPkg); // Should not throw. From 0b65dad47abda0c38593aaeee11ed91cc3b65d65 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 18:40:34 -0700 Subject: [PATCH 04/11] deploy: add canary deploy script with Firebase domain automation Testing a --no-promote canary end-to-end is blocked by Firebase OAuth: a versioned *.appspot.com URL is not in Firebase's Authorized domains, so signInWithRedirect rejects it and the operator cannot log in to exercise the real product before switching traffic. Add scripts/deploy-canary.mjs (pnpm deploy:canary): build + deploy the staged server with --no-promote (without overriding the version name), discover that version's URL via gcloud app browse, and add its host to the Identity Toolkit authorizedDomains via a read-modify-write PATCH (updateMask=authorizedDomains) that can never wipe the existing list. A --cleanup mode de-authorizes the host and stops the version -- but refuses to stop a version that is currently serving traffic, so running cleanup after promoting the canary cannot take the site down. It uses the operator's own gcloud credentials (firebaseauth.admin), deliberately not the CI deploy service account, and never promotes traffic. The pure core (host/URL parsing, domain merge, arg parsing, traffic-share lookup) is unit-tested; the gcloud/fetch shell is kept thin. Documented in docs/dev/deploy.md. --- docs/dev/deploy.md | 22 ++ package.json | 1 + scripts/deploy-canary.mjs | 555 +++++++++++++++++++++++++++ scripts/tests/deploy-canary.test.mjs | 203 ++++++++++ 4 files changed, 781 insertions(+) create mode 100644 scripts/deploy-canary.mjs create mode 100644 scripts/tests/deploy-canary.test.mjs diff --git a/docs/dev/deploy.md b/docs/dev/deploy.md index 10e0a6c4f..c588357e3 100644 --- a/docs/dev/deploy.md +++ b/docs/dev/deploy.md @@ -63,6 +63,28 @@ gcloud app services set-traffic default --splits==1 (`--no-promote` flows through to `gcloud app deploy` via the script's `"$@"` passthrough; the script still runs `deploy:clean` afterward via the trap.) +### Canary deploy + Firebase login: `pnpm deploy:canary` + +The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithRedirect` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. + +[`scripts/deploy-canary.mjs`](/scripts/deploy-canary.mjs) (`pnpm deploy:canary`) closes that gap. It: + +1. Reads the target project (`gcloud config get-value project`; override with `--project ` or `SIMLIN_CANARY_PROJECT`) and prints a warning that it touches **production** Firebase config and deploys (but does **not** promote traffic). +2. Builds + deploys via `scripts/deploy-web-staged.sh --no-promote` (it does **not** pass `--version`, so gcloud auto-generates the version id). +3. Discovers the just-deployed version id (`gcloud app versions list --sort-by=~version.createTime --limit=1`) and its region-aware URL (`gcloud app browse --no-launch-browser --version=`). +4. Adds that version's host to the Identity Toolkit `authorizedDomains` list via a **read-modify-write**: GET the config, append the host to the full existing list, then PATCH it back with `?updateMask=authorizedDomains`. The PATCH replaces the whole repeated field, so it always sends the complete current-plus-new list -- it never wipes `localhost` / `firebaseapp.com` / `app.simlin.com`. It prints the before/after list. +5. Prints the canary URL, the smoke-test checklist, the exact `gcloud app services set-traffic default --splits==1` promote command, and the exact cleanup command. **It does not promote traffic.** + +After testing, clean up: + +```bash +pnpm deploy:canary --cleanup # de-authorize the host + stop the version +``` + +Cleanup is the inverse: it removes the host from `authorizedDomains` (same read-modify-write) and stops the version (it also prints the `gcloud app versions delete` command if you want to free the slot). + +This deliberately uses **your own** credentials (`gcloud auth print-access-token`), not the CI deploy service account: mutating Firebase auth config needs `roles/firebaseauth.admin`, an operator-level grant intentionally kept off the CI SA. The deploy/authorize and the traffic promote are separate, explicit steps so traffic is never switched implicitly. Keep the canary host authorized only while you are testing it -- leaving stale appspot hosts in `authorizedDomains` needlessly widens the OAuth surface. + ## What gets uploaded, and what runs on the instance `gcloud app deploy` uploads the whole repo except `.gcloudignore` entries: `node_modules`, `target/`, `test/`, `/build*`, `scripts/`, `.github/`, `website/`, `examples/`, `src/jupyter/`, `src/app/public`, `src/app/build*`, `src/server/public`, `src/server/config`, `src/app/firebase.json`, and `.app.prod.yaml` itself. Not excluded, and load-bearing: diff --git a/package.json b/package.json index 8fd7f8477..c61f33155 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "clean": "pnpm -r run clean && cargo clean", "deploy:web": "bash scripts/deploy-web.sh", "deploy:web:staged": "bash scripts/deploy-web-staged.sh", + "deploy:canary": "node scripts/deploy-canary.mjs", "build:deploy-staging": "node scripts/build-deploy-staging.mjs", "start": "node src/server/lib", "test:scripts": "node --test scripts/tests/*.test.mjs", diff --git a/scripts/deploy-canary.mjs b/scripts/deploy-canary.mjs new file mode 100644 index 000000000..37e33522d --- /dev/null +++ b/scripts/deploy-canary.mjs @@ -0,0 +1,555 @@ +#!/usr/bin/env node +// +// Deploy HEAD to Google App Engine as a NON-TRAFFIC canary, then authorize the +// canary's versioned host in Firebase so the operator can exercise the REAL +// product end-to-end -- including Google sign-in -- before switching 100% of +// production traffic. +// +// Why this script exists: +// - `gcloud app deploy --no-promote` creates a version reachable only at a +// versioned `*.appspot.com` URL (e.g. https://-dot-...). +// That host is NOT in Firebase's Authorized domains list, so Firebase OAuth +// (signInWithRedirect via auth.simlin.com) rejects the calling origin and the +// operator cannot actually log in to test the canary. +// - The fix is to add the canary's host to the Identity Toolkit +// `authorizedDomains` list for the duration of the test, then remove it. +// +// This deliberately uses the OPERATOR's own credentials (`gcloud auth +// print-access-token`), NOT the CI deploy service account: mutating Firebase +// auth config requires roles/firebaseauth.admin, which is an operator-level +// grant we intentionally keep off the CI deploy SA. +// +// It does NOT promote traffic. Promoting is a separate, explicit command the +// script prints for you to run after the smoke test passes. +// +// Usage: +// node scripts/deploy-canary.mjs [--project ] # deploy canary + authorize +// node scripts/deploy-canary.mjs --cleanup [--project ] +// +// The build/deploy itself is delegated to scripts/deploy-web-staged.sh (the +// self-contained staged deploy); this script adds only the --no-promote canary +// orchestration + Firebase authorized-domain management around it. +// +// --------------------------------------------------------------------------- +// The file is split into a PURE CORE (exported, unit-tested in +// scripts/tests/deploy-canary.test.mjs) and an IMPERATIVE SHELL (all the +// gcloud/fetch side effects). main() runs only when the file is executed +// directly, so importing it for tests has no side effects. +// --------------------------------------------------------------------------- + +import { spawnSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(SCRIPT_DIR, '..'); + +const IDENTITY_TOOLKIT_BASE = 'https://identitytoolkit.googleapis.com/admin/v2'; + +// =========================================================================== +// PURE CORE -- no I/O, deterministic, unit-tested. +// =========================================================================== + +/** + * Reduce a URL (or already-bare host) to its bare hostname: scheme, path, + * query, and any port are stripped. Firebase `authorizedDomains` entries are + * bare hostnames (the OAuth check compares window.location.hostname), so the + * port is intentionally dropped. Accepts input without a scheme so a host can + * be passed straight through. Throws on empty/non-string input. + */ +export function hostFromUrl(input) { + if (typeof input !== 'string' || input.trim() === '') { + throw new Error('hostFromUrl: expected a non-empty URL or host string'); + } + const trimmed = input.trim(); + // Give the URL parser a scheme to chew on when one is absent, so a bare host + // like "foo.appspot.com" parses instead of throwing. + const hasScheme = /^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.test(trimmed); + const url = new URL(hasScheme ? trimmed : `https://${trimmed}`); + return url.hostname; +} + +/** + * Return a new authorized-domains list that includes `host`, deduped and with + * existing order preserved. A no-op (returns an equivalent deduped copy) when + * `host` is already present. Never mutates the input. + * + * This is the read side of the read-modify-write that guarantees the PATCH + * never wipes the existing list: the caller passes the full current list (from + * a successful GET); this appends to it rather than replacing it. + */ +export function addAuthorizedDomain(domains, host) { + const base = Array.isArray(domains) ? domains : []; + const seen = new Set(); + const out = []; + for (const d of [...base, host]) { + if (!seen.has(d)) { + seen.add(d); + out.push(d); + } + } + return out; +} + +/** + * Return a new authorized-domains list with every occurrence of `host` + * removed. A no-op (returns a copy) when `host` is absent. Never mutates the + * input. Like addAuthorizedDomain, this operates on the FULL current list so + * the resulting PATCH replaces the field without losing other entries. + */ +export function removeAuthorizedDomain(domains, host) { + const base = Array.isArray(domains) ? domains : []; + return base.filter((d) => d !== host); +} + +/** First non-blank, trimmed line of text (or '' for blank/non-string input). */ +export function firstNonEmptyLine(text) { + if (typeof text !== 'string') return ''; + for (const line of text.split('\n')) { + const t = line.trim(); + if (t !== '') return t; + } + return ''; +} + +/** + * Extract the first http(s) URL from arbitrary command output. `gcloud app + * browse --no-launch-browser` prints a human sentence followed by the URL, so + * we cannot just trim the whole output. Returns undefined when none is found. + */ +export function extractUrl(text) { + if (typeof text !== 'string') return undefined; + const m = text.match(/https?:\/\/\S+/); + return m ? m[0] : undefined; +} + +/** + * Fraction of `default`-service traffic currently allocated to `versionId`, + * read from the `split.allocations` map (versionId -> fraction) returned by + * `gcloud app services describe`. Returns 0 when the version is absent or the + * map is missing/malformed, and treats a non-finite-number allocation as 0 + * (never truthy) so the caller's serving check can't be fooled by a string. + * + * Cleanup uses this to REFUSE to stop a version that is serving traffic: once + * the operator promotes the canary, it IS production, and stopping it is a full + * outage. De-authorizing the host is always safe; only the stop is dangerous. + */ +export function versionTrafficShare(allocations, versionId) { + if (!allocations || typeof allocations !== 'object') return 0; + const share = allocations[versionId]; + return typeof share === 'number' && Number.isFinite(share) ? share : 0; +} + +/** + * Parse argv (without node/script) into { mode, project, version }. + * mode: 'deploy' (default) | 'cleanup' | 'help' + * Supports `--flag value` and `--flag=value`. Throws on unknown flags and on + * --cleanup without a version id (failing loud beats a confusing later error). + */ +export function parseArgs(argv) { + const args = { mode: 'deploy', project: undefined, version: undefined }; + const list = [...argv]; + while (list.length > 0) { + const token = list.shift(); + const eq = token.startsWith('--') ? token.indexOf('=') : -1; + const flag = eq >= 0 ? token.slice(0, eq) : token; + const inlineVal = eq >= 0 ? token.slice(eq + 1) : undefined; + // Do not consume a following flag as this flag's value: `--cleanup + // --project p` must leave version unset (and report the missing version), + // not swallow `--project`. + const takeVal = () => { + if (inlineVal !== undefined) return inlineVal; + return list.length > 0 && !list[0].startsWith('--') ? list.shift() : undefined; + }; + switch (flag) { + case '--cleanup': + args.mode = 'cleanup'; + args.version = takeVal(); + break; + case '--project': + args.project = takeVal(); + break; + case '--help': + case '-h': + args.mode = 'help'; + break; + default: + throw new Error(`unknown argument: ${token}`); + } + } + if (args.mode === 'cleanup' && (!args.version || args.version.startsWith('--'))) { + throw new Error('--cleanup requires a version id, e.g. --cleanup 20260627t123456'); + } + return args; +} + +// =========================================================================== +// IMPERATIVE SHELL -- gcloud + fetch side effects. Kept deliberately small and +// obvious; the testable logic lives in the pure core above. +// =========================================================================== + +function die(msg) { + console.error(`\nERROR: ${msg}`); + process.exit(1); +} + +/** + * Run a command, inheriting stdio by default so the operator watches progress. + * With { capture: true } stdout is returned as a string (stderr still streams). + * Fails fast with a clear message on a missing binary or non-zero exit. + */ +function run(cmd, cmdArgs, { capture = false } = {}) { + const res = spawnSync(cmd, cmdArgs, { + cwd: REPO_ROOT, + encoding: 'utf8', + stdio: capture ? ['inherit', 'pipe', 'inherit'] : 'inherit', + }); + if (res.error) { + if (res.error.code === 'ENOENT') { + const hint = cmd === 'gcloud' ? ' Install the Google Cloud SDK and authenticate: gcloud auth login.' : ''; + die(`'${cmd}' was not found on PATH.${hint}`); + } + die(`failed to run ${cmd}: ${res.error.message}`); + } + if (res.status !== 0) { + die(`\`${cmd} ${cmdArgs.join(' ')}\` exited with status ${res.status}`); + } + return capture ? res.stdout : ''; +} + +/** Resolve the GCP project: explicit override wins, else the gcloud default. */ +function resolveProject(override) { + if (override) return override; + const project = firstNonEmptyLine(run('gcloud', ['config', 'get-value', 'project'], { capture: true })); + if (!project || project === '(unset)') { + die('no gcloud project configured. Pass --project or run: gcloud config set project '); + } + return project; +} + +/** The operator's own OAuth access token (NOT the CI deploy SA's). */ +function accessToken() { + const token = firstNonEmptyLine(run('gcloud', ['auth', 'print-access-token'], { capture: true })); + if (!token) { + die('could not obtain an access token. Run: gcloud auth login'); + } + return token; +} + +/** Build + deploy the staged server with --no-promote (traffic stays put). */ +function deployStagedNoPromote() { + console.log('\n==> Building and deploying the staged server with --no-promote (no traffic switch)\n'); + // Reuse the proven staged build/deploy orchestration; do NOT pass --version, + // so gcloud auto-generates the version id we then discover below. + run('bash', [path.join(REPO_ROOT, 'scripts/deploy-web-staged.sh'), '--no-promote']); +} + +/** + * The id of the most-recently-created `default` version. We query by createTime + * immediately after our deploy rather than scraping the deploy output (gcloud's + * human output format is not a stable contract). Tradeoff: if another deploy of + * the same service races this one, the newest version could be theirs -- this + * is a single-operator manual canary tool, so that race is acceptable; the + * printed URL lets the operator sanity-check the id before promoting. + */ +function latestVersionId(project) { + const out = run( + 'gcloud', + [ + 'app', + 'versions', + 'list', + '--service=default', + '--sort-by=~version.createTime', + '--limit=1', + '--format=value(id)', + `--project=${project}`, + ], + { capture: true }, + ); + const id = firstNonEmptyLine(out); + if (!id) { + die('could not determine the deployed version id from `gcloud app versions list`'); + } + return id; +} + +/** + * The canary's URL + bare host, via `gcloud app browse` (region-aware -- we do + * NOT hand-construct the appspot host, since the region segment varies). + */ +function versionUrlAndHost(project, id) { + const out = run( + 'gcloud', + ['app', 'browse', '--no-launch-browser', '--service=default', `--version=${id}`, `--project=${project}`], + { capture: true }, + ); + const url = extractUrl(out); + if (!url) { + die(`could not extract the canary URL from \`gcloud app browse\` output:\n${out}`); + } + return { url, host: hostFromUrl(url) }; +} + +/** Stop a version's instances (cleanup). Delete is mentioned as an option. */ +function stopVersion(project, id) { + run('gcloud', ['app', 'versions', 'stop', id, '--service=default', `--project=${project}`]); +} + +/** + * The `default` service's current traffic split as a { versionId: fraction } + * map. Read from `gcloud app services describe` so cleanup can avoid stopping a + * version that is serving traffic. Returns {} if the service has no split yet. + */ +function serviceTrafficAllocations(project) { + const out = run('gcloud', ['app', 'services', 'describe', 'default', '--format=json', `--project=${project}`], { + capture: true, + }); + let parsed; + try { + parsed = JSON.parse(out); + } catch { + die(`could not parse \`gcloud app services describe default\` JSON output:\n${out}`); + } + return parsed?.split?.allocations ?? {}; +} + +/** + * GET the project's Identity Toolkit config. This MUST succeed before any + * PATCH: the read-modify-write below depends on a real current list, never an + * assumed-empty one. Returns the parsed config object. + */ +async function getIdentityConfig(project, token) { + const res = await fetch(`${IDENTITY_TOOLKIT_BASE}/projects/${encodeURIComponent(project)}/config`, { + headers: { authorization: `Bearer ${token}` }, + }); + if (!res.ok) { + const body = await res.text(); + const hint = + res.status === 403 + ? ' (does your account have roles/firebaseauth.admin? this needs the operator creds, not the CI deploy SA)' + : ''; + die(`Identity Toolkit GET config failed: ${res.status} ${res.statusText}${hint}\n${body}`); + } + // Parse defensively: a 200 whose body isn't JSON is a real (if rare) failure + // we want to surface clearly, not a confusing low-level throw. + const text = await res.text(); + try { + return JSON.parse(text); + } catch { + die(`Identity Toolkit GET config returned ${res.status} with a non-JSON body:\n${text}`); + } +} + +/** + * PATCH authorizedDomains with `?updateMask=authorizedDomains`. The mask scopes + * the write to exactly that field, but the API REPLACES the whole repeated + * field with the body's list -- so `domains` MUST be the full desired list + * (current entries + the change), which the pure add/remove helpers produce + * from a freshly GET-ed current list. Sending only the new host here would wipe + * localhost / firebaseapp.com / app.simlin.com. + */ +async function patchAuthorizedDomains(project, token, domains) { + const res = await fetch( + `${IDENTITY_TOOLKIT_BASE}/projects/${encodeURIComponent(project)}/config?updateMask=authorizedDomains`, + { + method: 'PATCH', + headers: { authorization: `Bearer ${token}`, 'content-type': 'application/json' }, + body: JSON.stringify({ authorizedDomains: domains }), + }, + ); + if (!res.ok) { + const body = await res.text(); + die(`Identity Toolkit PATCH config failed: ${res.status} ${res.statusText}\n${body}`); + } + // The response body is discarded, so do NOT parse it: a successful PATCH whose + // body isn't JSON must not be reported as a failure AFTER the change applied. +} + +function printDomains(label, domains) { + console.log(` ${label}:`); + for (const d of domains) { + console.log(` - ${d}`); + } + if (domains.length === 0) { + console.log(' (none)'); + } +} + +function printHelp() { + console.log( + [ + 'Deploy a NON-TRAFFIC canary to App Engine and authorize its host in Firebase.', + '', + 'Usage:', + ' node scripts/deploy-canary.mjs [--project ]', + ' Build + deploy HEAD with --no-promote, then add the canary host to', + ' Firebase authorizedDomains so you can log in and smoke-test it.', + '', + ' node scripts/deploy-canary.mjs --cleanup [--project ]', + ' Remove that version host from authorizedDomains, then stop the version', + ' UNLESS it is serving traffic (a promoted canary is production; the stop', + ' is refused so cleanup can never cause an outage).', + '', + 'Project defaults to `gcloud config get-value project`; override with', + '--project or the SIMLIN_CANARY_PROJECT env var. Mutating Firebase auth', + 'config requires roles/firebaseauth.admin on YOUR account (this uses your', + 'own gcloud credentials, not the CI deploy service account). Traffic is', + 'never promoted; the deploy + promote are separate steps.', + ].join('\n'), + ); +} + +async function runDeployMode(project, token) { + // Preflight: validate Firebase access BEFORE the (long) deploy, so a missing + // firebaseauth.admin grant fails in seconds rather than after an upload. + console.log('==> Preflight: reading current Firebase authorized domains'); + const preflight = await getIdentityConfig(project, token); + printDomains('current authorizedDomains', preflight.authorizedDomains ?? []); + + deployStagedNoPromote(); + + const versionId = latestVersionId(project); + const { url, host } = versionUrlAndHost(project, versionId); + console.log(`\n==> Canary version: ${versionId}`); + console.log(` Canary URL: ${url}`); + console.log(` Canary host: ${host}`); + + // Read-modify-write against a FRESH GET (the config could have changed during + // the deploy), then PATCH the full merged list. + console.log('\n==> Authorizing the canary host in Firebase'); + const current = await getIdentityConfig(project, token); + const before = current.authorizedDomains ?? []; + const after = addAuthorizedDomain(before, host); + printDomains('before', before); + if (after.length === before.length) { + console.log(` ${host} is already authorized -- no change.`); + } else { + await patchAuthorizedDomains(project, token, after); + printDomains('after', after); + } + + printPostDeploy(project, versionId, url); +} + +function printPostDeploy(project, versionId, url) { + const projectFlag = `--project=${project}`; + console.log( + [ + '', + '============================================================', + 'Canary is deployed and authorized -- traffic NOT switched.', + '============================================================', + '', + `Canary URL: ${url}`, + '', + 'Smoke test the canary (against the URL above):', + ` - curl -sI ${url}/ -> 200 HTML, links a hashed /static/js/index..js`, + ` - curl -sI ${url}/static/js/sd-component.js -> 200 (the embed component)`, + ` - curl -sI ${url}/static/wasm/.module.wasm -> 200, content-type: application/wasm`, + ' - In a browser: LOG IN WITH GOOGLE (this is what the authorized-domain step enables),', + ' land on Home with no console errors.', + ' - Open an example model and confirm it simulates; edit + save + reload persists.', + '', + 'When the smoke test PASSES, switch 100% of traffic to the canary:', + ` gcloud app services set-traffic default --splits=${versionId}=1 ${projectFlag}`, + '', + 'Cleanup -- two cases:', + ` (a) ABANDONING the canary (did NOT promote): de-authorizes the host AND stops`, + ' the version, which is safe because it serves no traffic:', + ` node scripts/deploy-canary.mjs --cleanup ${versionId} ${projectFlag}`, + ' (b) AFTER PROMOTING: this version is now production. The same cleanup command', + ' is safe to run -- it will de-authorize the host but REFUSE to stop a', + ' serving version. To reclaim resources, stop the PREVIOUS (now-idle)', + ' version instead, not this one:', + ` gcloud app versions list --service=default ${projectFlag}`, + ` gcloud app versions stop --service=default ${projectFlag}`, + '', + 'NOTE: keep the canary host authorized only as long as you are testing it;', + 'leaving stale appspot hosts in authorizedDomains widens the OAuth surface.', + ].join('\n'), + ); +} + +async function runCleanupMode(project, token, versionId) { + // Derive the host the same way the deploy path did (browse is region-aware). + // Do this BEFORE stopping the version so browse can still resolve it. + const { host } = versionUrlAndHost(project, versionId); + console.log(`==> Cleaning up canary version ${versionId} (host ${host})`); + + console.log('\n==> Removing the canary host from Firebase authorized domains'); + const current = await getIdentityConfig(project, token); + const before = current.authorizedDomains ?? []; + const after = removeAuthorizedDomain(before, host); + printDomains('before', before); + if (after.length === before.length) { + console.log(` ${host} was not authorized -- no change.`); + } else { + await patchAuthorizedDomains(project, token, after); + printDomains('after', after); + } + + // Guard the stop: if the operator promoted this canary, it is now serving + // production traffic and stopping it is a full-site outage. De-authorizing the + // host above is always safe; only the stop is dangerous, so we refuse it here + // rather than blindly running it. + const share = versionTrafficShare(serviceTrafficAllocations(project), versionId); + if (share > 0) { + const pct = (share * 100).toFixed(share < 0.01 ? 2 : 0); + console.log( + [ + '', + `REFUSING TO STOP version ${versionId}: it is serving ${pct}% of default-service traffic.`, + 'De-authorized the host only. If you promoted the canary, it IS production now --', + 'stopping it would take the site down. To reclaim resources, stop the PREVIOUS', + '(now-idle) version instead:', + ` gcloud app versions list --service=default --project=${project}`, + ` gcloud app versions stop --service=default --project=${project}`, + ].join('\n'), + ); + return; + } + + console.log('\n==> Stopping the canary version (it is serving no traffic)'); + stopVersion(project, versionId); + console.log( + [ + '', + `Done. Version ${versionId} is stopped and its host is de-authorized.`, + 'To remove the version entirely (frees the slot toward the GAE version cap):', + ` gcloud app versions delete ${versionId} --service=default --project=${project}`, + ].join('\n'), + ); +} + +async function main() { + const args = parseArgs(process.argv.slice(2)); + if (args.mode === 'help') { + printHelp(); + return; + } + + const project = resolveProject(args.project ?? process.env.SIMLIN_CANARY_PROJECT); + + console.log('============================================================'); + console.log(`Project: ${project}`); + console.log('WARNING: this touches PRODUCTION Firebase auth config and'); + console.log('deploys to production App Engine. It does NOT promote traffic.'); + console.log('============================================================\n'); + + const token = accessToken(); + + if (args.mode === 'cleanup') { + await runCleanupMode(project, token, args.version); + } else { + await runDeployMode(project, token); + } +} + +// Run only when executed directly, so importing the pure helpers in tests has +// no side effects (mirrors the guard-free build-deploy-staging.mjs by gating +// the entrypoint instead). +const invokedDirectly = process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href; +if (invokedDirectly) { + main().catch((err) => die(err.message)); +} diff --git a/scripts/tests/deploy-canary.test.mjs b/scripts/tests/deploy-canary.test.mjs new file mode 100644 index 000000000..a0bb3f538 --- /dev/null +++ b/scripts/tests/deploy-canary.test.mjs @@ -0,0 +1,203 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; + +import { + hostFromUrl, + addAuthorizedDomain, + removeAuthorizedDomain, + firstNonEmptyLine, + extractUrl, + parseArgs, + versionTrafficShare, +} from '../deploy-canary.mjs'; + +describe('hostFromUrl', () => { + it('strips scheme and trailing slash', () => { + assert.equal(hostFromUrl('https://foo.appspot.com/'), 'foo.appspot.com'); + }); + + it('accepts http as well as https', () => { + assert.equal(hostFromUrl('http://foo.appspot.com'), 'foo.appspot.com'); + }); + + it('strips a path and query', () => { + assert.equal( + hostFromUrl('https://20260627t123456-dot-myproj.uc.r.appspot.com/some/path?q=1'), + '20260627t123456-dot-myproj.uc.r.appspot.com', + ); + }); + + it('drops the port (authorizedDomains entries are bare hostnames)', () => { + assert.equal(hostFromUrl('http://localhost:5000/'), 'localhost'); + }); + + it('accepts a bare host with no scheme', () => { + assert.equal(hostFromUrl('foo-dot-bar.appspot.com'), 'foo-dot-bar.appspot.com'); + }); + + it('trims surrounding whitespace', () => { + assert.equal(hostFromUrl(' https://foo.appspot.com/ '), 'foo.appspot.com'); + }); + + it('throws on empty / non-string input', () => { + assert.throws(() => hostFromUrl('')); + assert.throws(() => hostFromUrl(undefined)); + }); +}); + +describe('addAuthorizedDomain', () => { + it('appends a new host', () => { + assert.deepEqual(addAuthorizedDomain(['localhost', 'app.simlin.com'], 'v1-dot-p.appspot.com'), [ + 'localhost', + 'app.simlin.com', + 'v1-dot-p.appspot.com', + ]); + }); + + it('is a no-op when the host is already present (idempotent)', () => { + const domains = ['localhost', 'app.simlin.com']; + assert.deepEqual(addAuthorizedDomain(domains, 'app.simlin.com'), ['localhost', 'app.simlin.com']); + }); + + it('dedups pre-existing duplicates while preserving order', () => { + assert.deepEqual(addAuthorizedDomain(['a', 'a', 'b'], 'c'), ['a', 'b', 'c']); + }); + + it('does not mutate the input list', () => { + const domains = ['localhost']; + addAuthorizedDomain(domains, 'new.example.com'); + assert.deepEqual(domains, ['localhost']); + }); + + it('tolerates an undefined/empty current list', () => { + assert.deepEqual(addAuthorizedDomain(undefined, 'x.appspot.com'), ['x.appspot.com']); + assert.deepEqual(addAuthorizedDomain([], 'x.appspot.com'), ['x.appspot.com']); + }); +}); + +describe('removeAuthorizedDomain', () => { + it('removes the host', () => { + assert.deepEqual(removeAuthorizedDomain(['localhost', 'v1-dot-p.appspot.com'], 'v1-dot-p.appspot.com'), [ + 'localhost', + ]); + }); + + it('is a no-op when the host is absent (idempotent)', () => { + assert.deepEqual(removeAuthorizedDomain(['localhost'], 'nope.appspot.com'), ['localhost']); + }); + + it('removes every occurrence if duplicated', () => { + assert.deepEqual(removeAuthorizedDomain(['a', 'b', 'a'], 'a'), ['b']); + }); + + it('does not mutate the input list', () => { + const domains = ['localhost', 'x.appspot.com']; + removeAuthorizedDomain(domains, 'x.appspot.com'); + assert.deepEqual(domains, ['localhost', 'x.appspot.com']); + }); + + it('tolerates an undefined/empty current list', () => { + assert.deepEqual(removeAuthorizedDomain(undefined, 'x'), []); + assert.deepEqual(removeAuthorizedDomain([], 'x'), []); + }); +}); + +describe('firstNonEmptyLine', () => { + it('returns the first non-blank trimmed line', () => { + assert.equal(firstNonEmptyLine('\n \n 20260627t1\n more\n'), '20260627t1'); + }); + + it('trims a single value', () => { + assert.equal(firstNonEmptyLine(' myproject \n'), 'myproject'); + }); + + it('returns empty string for blank / non-string input', () => { + assert.equal(firstNonEmptyLine(' \n '), ''); + assert.equal(firstNonEmptyLine(undefined), ''); + }); +}); + +describe('extractUrl', () => { + it('pulls the first http(s) URL out of noisy output', () => { + const out = + 'Did not detect your browser. Go to this link to view your app:\n' + + 'https://20260627t1-dot-myproj.uc.r.appspot.com\n'; + assert.equal(extractUrl(out), 'https://20260627t1-dot-myproj.uc.r.appspot.com'); + }); + + it('handles a plain URL', () => { + assert.equal(extractUrl('https://foo.appspot.com/'), 'https://foo.appspot.com/'); + }); + + it('returns undefined when there is no URL', () => { + assert.equal(extractUrl('no url here'), undefined); + assert.equal(extractUrl(undefined), undefined); + }); +}); + +describe('parseArgs', () => { + it('defaults to deploy mode', () => { + assert.deepEqual(parseArgs([]), { mode: 'deploy', project: undefined, version: undefined }); + }); + + it('parses --project with a separate value', () => { + assert.equal(parseArgs(['--project', 'myproj']).project, 'myproj'); + }); + + it('parses --project=value inline', () => { + assert.equal(parseArgs(['--project=myproj']).project, 'myproj'); + }); + + it('parses --cleanup into cleanup mode', () => { + const args = parseArgs(['--cleanup', '20260627t1']); + assert.equal(args.mode, 'cleanup'); + assert.equal(args.version, '20260627t1'); + }); + + it('parses --cleanup= inline', () => { + assert.equal(parseArgs(['--cleanup=20260627t1']).version, '20260627t1'); + }); + + it('combines --cleanup and --project', () => { + const args = parseArgs(['--cleanup', '20260627t1', '--project', 'p']); + assert.equal(args.mode, 'cleanup'); + assert.equal(args.version, '20260627t1'); + assert.equal(args.project, 'p'); + }); + + it('maps --help/-h to help mode', () => { + assert.equal(parseArgs(['--help']).mode, 'help'); + assert.equal(parseArgs(['-h']).mode, 'help'); + }); + + it('throws when --cleanup has no version', () => { + assert.throws(() => parseArgs(['--cleanup']), /requires a version/); + assert.throws(() => parseArgs(['--cleanup', '--project', 'p']), /requires a version/); + }); + + it('throws on an unknown argument', () => { + assert.throws(() => parseArgs(['--bogus']), /unknown argument/); + }); +}); + +describe('versionTrafficShare', () => { + it('returns the fraction allocated to the version', () => { + assert.equal(versionTrafficShare({ '20260627t1': 1 }, '20260627t1'), 1); + assert.equal(versionTrafficShare({ a: 0.3, b: 0.7 }, 'b'), 0.7); + }); + + it('returns 0 when the version is absent (safe to stop)', () => { + assert.equal(versionTrafficShare({ other: 1 }, '20260627t1'), 0); + }); + + it('returns 0 for a missing/empty allocations map', () => { + assert.equal(versionTrafficShare(undefined, 'v'), 0); + assert.equal(versionTrafficShare(null, 'v'), 0); + assert.equal(versionTrafficShare({}, 'v'), 0); + }); + + it('treats a non-numeric allocation as 0 rather than truthy', () => { + assert.equal(versionTrafficShare({ v: '1' }, 'v'), 0); + assert.equal(versionTrafficShare({ v: NaN }, 'v'), 0); + }); +}); From e790df7c087b3536f7689f08d386e03410d2651b Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 22:15:36 -0700 Subject: [PATCH 05/11] deploy: address canary cleanup review feedback Two fixes from the codex review of the canary script: - Cleanup now deletes the idle canary version instead of stopping it. `gcloud app versions stop` only works for manual/basic scaling, but prod uses automatic_scaling (app.yaml), so the stop would error out and leave the version (and its slot toward the GAE version cap) behind after the Firebase de-authorization had already run. `delete` works for any scaling and frees the slot; the existing zero-traffic guard already ensures we never delete a serving (promoted) version. - parseArgs now rejects --project with a missing or blank value. Because the script deploys and mutates production Firebase auth config, `--project $EMPTY` or `--project --cleanup v` must fail loudly rather than silently fall back to the gcloud default and operate on the wrong project. Omitting --project entirely still uses the env var / gcloud default as before. --- docs/dev/deploy.md | 4 +- scripts/deploy-canary.mjs | 79 ++++++++++++++++------------ scripts/tests/deploy-canary.test.mjs | 10 ++++ 3 files changed, 58 insertions(+), 35 deletions(-) diff --git a/docs/dev/deploy.md b/docs/dev/deploy.md index c588357e3..439ee399f 100644 --- a/docs/dev/deploy.md +++ b/docs/dev/deploy.md @@ -78,10 +78,10 @@ The `--no-promote` flow above lets you `curl` the canary, but you cannot actuall After testing, clean up: ```bash -pnpm deploy:canary --cleanup # de-authorize the host + stop the version +pnpm deploy:canary --cleanup # de-authorize the host + delete the version ``` -Cleanup is the inverse: it removes the host from `authorizedDomains` (same read-modify-write) and stops the version (it also prints the `gcloud app versions delete` command if you want to free the slot). +Cleanup is the inverse: it removes the host from `authorizedDomains` (same read-modify-write) and deletes the version (freeing its slot toward the GAE version cap). It deletes rather than `stop`s because production uses `automatic_scaling`, for which `gcloud app versions stop` is rejected. The delete is guarded by the version's current traffic share: if you have already promoted the canary it is now production, so cleanup refuses to delete it (de-authorizing the host only) and tells you to delete the previous, now-idle version instead -- cleanup can never cause an outage. This deliberately uses **your own** credentials (`gcloud auth print-access-token`), not the CI deploy service account: mutating Firebase auth config needs `roles/firebaseauth.admin`, an operator-level grant intentionally kept off the CI SA. The deploy/authorize and the traffic promote are separate, explicit steps so traffic is never switched implicitly. Keep the canary host authorized only while you are testing it -- leaving stale appspot hosts in `authorizedDomains` needlessly widens the OAuth surface. diff --git a/scripts/deploy-canary.mjs b/scripts/deploy-canary.mjs index 37e33522d..524f4b0b6 100644 --- a/scripts/deploy-canary.mjs +++ b/scripts/deploy-canary.mjs @@ -130,9 +130,9 @@ export function extractUrl(text) { * map is missing/malformed, and treats a non-finite-number allocation as 0 * (never truthy) so the caller's serving check can't be fooled by a string. * - * Cleanup uses this to REFUSE to stop a version that is serving traffic: once - * the operator promotes the canary, it IS production, and stopping it is a full - * outage. De-authorizing the host is always safe; only the stop is dangerous. + * Cleanup uses this to REFUSE to delete a version that is serving traffic: once + * the operator promotes the canary, it IS production, and deleting it is a full + * outage. De-authorizing the host is always safe; only the delete is dangerous. */ export function versionTrafficShare(allocations, versionId) { if (!allocations || typeof allocations !== 'object') return 0; @@ -166,9 +166,19 @@ export function parseArgs(argv) { args.mode = 'cleanup'; args.version = takeVal(); break; - case '--project': - args.project = takeVal(); + case '--project': { + // Fail loud on a missing/blank value: this tool deploys and mutates + // PROD Firebase config, so `--project $EMPTY` (or `--project --cleanup`) + // must error rather than silently fall back to the gcloud default and + // operate on the wrong project. Omitting --project entirely is still + // fine -- that intentionally uses the env var / gcloud default. + const value = takeVal(); + if (value === undefined || value.trim() === '') { + throw new Error('--project requires a value, e.g. --project my-gcp-project'); + } + args.project = value; break; + } case '--help': case '-h': args.mode = 'help'; @@ -291,14 +301,22 @@ function versionUrlAndHost(project, id) { return { url, host: hostFromUrl(url) }; } -/** Stop a version's instances (cleanup). Delete is mentioned as an option. */ -function stopVersion(project, id) { - run('gcloud', ['app', 'versions', 'stop', id, '--service=default', `--project=${project}`]); +/** + * Delete an idle canary version (cleanup). We delete rather than `stop` because + * `gcloud app versions stop` only works for manual/basic scaling, and prod uses + * automatic_scaling (app.yaml) -- a stop would error out, leaving the version + * (and its slot toward the GAE version cap) behind. delete works for any scaling + * and reclaims the slot. The caller guarantees this version serves no traffic + * (gcloud also refuses to delete a version that is receiving traffic). gcloud + * prompts for confirmation; stdio is inherited so the operator confirms. + */ +function deleteVersion(project, id) { + run('gcloud', ['app', 'versions', 'delete', id, '--service=default', `--project=${project}`]); } /** * The `default` service's current traffic split as a { versionId: fraction } - * map. Read from `gcloud app services describe` so cleanup can avoid stopping a + * map. Read from `gcloud app services describe` so cleanup can avoid deleting a * version that is serving traffic. Returns {} if the service has no split yet. */ function serviceTrafficAllocations(project) { @@ -387,8 +405,8 @@ function printHelp() { ' Firebase authorizedDomains so you can log in and smoke-test it.', '', ' node scripts/deploy-canary.mjs --cleanup [--project ]', - ' Remove that version host from authorizedDomains, then stop the version', - ' UNLESS it is serving traffic (a promoted canary is production; the stop', + ' Remove that version host from authorizedDomains, then delete the version', + ' UNLESS it is serving traffic (a promoted canary is production; the delete', ' is refused so cleanup can never cause an outage).', '', 'Project defaults to `gcloud config get-value project`; override with', @@ -455,15 +473,15 @@ function printPostDeploy(project, versionId, url) { ` gcloud app services set-traffic default --splits=${versionId}=1 ${projectFlag}`, '', 'Cleanup -- two cases:', - ` (a) ABANDONING the canary (did NOT promote): de-authorizes the host AND stops`, + ` (a) ABANDONING the canary (did NOT promote): de-authorizes the host AND deletes`, ' the version, which is safe because it serves no traffic:', ` node scripts/deploy-canary.mjs --cleanup ${versionId} ${projectFlag}`, ' (b) AFTER PROMOTING: this version is now production. The same cleanup command', - ' is safe to run -- it will de-authorize the host but REFUSE to stop a', - ' serving version. To reclaim resources, stop the PREVIOUS (now-idle)', + ' is safe to run -- it will de-authorize the host but REFUSE to delete a', + ' serving version. To reclaim resources, delete the PREVIOUS (now-idle)', ' version instead, not this one:', ` gcloud app versions list --service=default ${projectFlag}`, - ` gcloud app versions stop --service=default ${projectFlag}`, + ` gcloud app versions delete --service=default ${projectFlag}`, '', 'NOTE: keep the canary host authorized only as long as you are testing it;', 'leaving stale appspot hosts in authorizedDomains widens the OAuth surface.', @@ -473,7 +491,7 @@ function printPostDeploy(project, versionId, url) { async function runCleanupMode(project, token, versionId) { // Derive the host the same way the deploy path did (browse is region-aware). - // Do this BEFORE stopping the version so browse can still resolve it. + // Do this BEFORE deleting the version so browse can still resolve it. const { host } = versionUrlAndHost(project, versionId); console.log(`==> Cleaning up canary version ${versionId} (host ${host})`); @@ -489,37 +507,32 @@ async function runCleanupMode(project, token, versionId) { printDomains('after', after); } - // Guard the stop: if the operator promoted this canary, it is now serving - // production traffic and stopping it is a full-site outage. De-authorizing the - // host above is always safe; only the stop is dangerous, so we refuse it here - // rather than blindly running it. + // Guard the delete: if the operator promoted this canary, it is now serving + // production traffic and deleting it is a full-site outage. De-authorizing the + // host above is always safe; only the delete is dangerous, so we refuse it + // here rather than blindly running it. const share = versionTrafficShare(serviceTrafficAllocations(project), versionId); if (share > 0) { const pct = (share * 100).toFixed(share < 0.01 ? 2 : 0); console.log( [ '', - `REFUSING TO STOP version ${versionId}: it is serving ${pct}% of default-service traffic.`, + `REFUSING TO DELETE version ${versionId}: it is serving ${pct}% of default-service traffic.`, 'De-authorized the host only. If you promoted the canary, it IS production now --', - 'stopping it would take the site down. To reclaim resources, stop the PREVIOUS', + 'deleting it would take the site down. To reclaim resources, delete the PREVIOUS', '(now-idle) version instead:', ` gcloud app versions list --service=default --project=${project}`, - ` gcloud app versions stop --service=default --project=${project}`, + ` gcloud app versions delete --service=default --project=${project}`, ].join('\n'), ); return; } - console.log('\n==> Stopping the canary version (it is serving no traffic)'); - stopVersion(project, versionId); - console.log( - [ - '', - `Done. Version ${versionId} is stopped and its host is de-authorized.`, - 'To remove the version entirely (frees the slot toward the GAE version cap):', - ` gcloud app versions delete ${versionId} --service=default --project=${project}`, - ].join('\n'), - ); + // delete (not stop): prod uses automatic_scaling, where `gcloud app versions + // stop` is rejected; delete works and frees the slot toward the version cap. + console.log('\n==> Deleting the canary version (it is serving no traffic)'); + deleteVersion(project, versionId); + console.log(`\nDone. Version ${versionId} is deleted and its host is de-authorized.`); } async function main() { diff --git a/scripts/tests/deploy-canary.test.mjs b/scripts/tests/deploy-canary.test.mjs index a0bb3f538..ea9950c76 100644 --- a/scripts/tests/deploy-canary.test.mjs +++ b/scripts/tests/deploy-canary.test.mjs @@ -178,6 +178,16 @@ describe('parseArgs', () => { it('throws on an unknown argument', () => { assert.throws(() => parseArgs(['--bogus']), /unknown argument/); }); + + it('throws when --project has no value', () => { + // A prod-mutating tool must not silently fall back to the default project + // when an override was clearly intended (e.g. `--project $EMPTY`). + assert.throws(() => parseArgs(['--project']), /--project requires a value/); + assert.throws(() => parseArgs(['--project', '--cleanup', 'v']), /--project requires a value/); + assert.throws(() => parseArgs(['--project', '']), /--project requires a value/); + assert.throws(() => parseArgs(['--project=']), /--project requires a value/); + assert.throws(() => parseArgs(['--project= ']), /--project requires a value/); + }); }); describe('versionTrafficShare', () => { From 2c11ac13edc118a7986125437bd49acd9ff2bee5 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 23:01:24 -0700 Subject: [PATCH 06/11] diagram: preserve module compat when duplicating a model handleDuplicateModelForModule hand-built the upsertModule payload for the re-pointed module with only name/modelName/references/units/documentation, silently dropping compat (canBeModuleInput/isPublic/dataSource) -- the same full-replace data-loss class the rest of this change fixes. Its sibling handleCreateModelForModule had already been corrected, so the two had drifted. Extract the shared, pure buildModuleReferencePayload helper into module-wiring.ts (based on moduleToJson, overriding only name/modelName) and route BOTH handlers through it so they cannot diverge again, and unit-test the helper (compat preserved, bare fallback for no/non-module variable). Found in PR review. --- src/diagram/Editor.tsx | 40 ++++--------- src/diagram/module-wiring.ts | 26 ++++++++- src/diagram/tests/module-wiring.test.ts | 76 ++++++++++++++++++++++++- 3 files changed, 110 insertions(+), 32 deletions(-) diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index de53197aa..1313ca961 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -14,13 +14,7 @@ import Button from './components/Button'; import { canonicalize } from '@simlin/core/canonicalize'; import { Project as EngineProject } from '@simlin/engine'; -import type { - JsonProjectPatch, - JsonModelOperation, - JsonSimSpecs, - JsonArrayedEquation, - JsonModule, -} from '@simlin/engine'; +import type { JsonProjectPatch, JsonModelOperation, JsonSimSpecs, JsonArrayedEquation } from '@simlin/engine'; import { stockFlowViewToJson } from './view-conversion'; import { Project, @@ -69,6 +63,7 @@ import { applyGroupMovement } from './group-movement'; import { detectUndoRedo, isEditableElement } from './keyboard-shortcuts'; import { isStdlibModel } from './module-navigation'; import { countModelInstances } from './module-details-utils'; +import { buildModuleReferencePayload } from './module-wiring'; import { BreadcrumbBar } from './BreadcrumbBar'; import { ProjectController, type ProjectSnapshot, type EngineApi } from './project-controller'; @@ -1832,13 +1827,11 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac } // Look up existing module to preserve metadata (including compat) through the - // model reference change; moduleToJson carries every field forward. + // model reference change; the shared helper carries every field forward + // (incl. compat) and keeps this in lockstep with the duplicate-model path. const model = getModel(); const existingModule = model?.variables.get(moduleIdent); - const modulePayload: JsonModule = - existingModule && existingModule.type === 'module' - ? { ...moduleToJson(existingModule), name: moduleIdent, modelName: newModelName } - : { name: moduleIdent, modelName: newModelName }; + const modulePayload = buildModuleReferencePayload(existingModule, moduleIdent, newModelName); const patch: JsonProjectPatch = { projectOps: [{ type: 'addModel', payload: { name: newModelName } }], @@ -1897,26 +1890,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }); } - // Preserve existing module metadata through the model reference change + // Preserve ALL existing module fields (incl. compat: canBeModuleInput / + // isPublic / dataSource) through the model reference change. The hand-built + // payload here previously dropped compat -- the same full-replace data-loss + // trap fixed elsewhere; use the shared helper so it matches its sibling. const currentModel = getModel(); const existingModule = currentModel?.variables.get(moduleIdent); - const dupModulePayload: { - name: string; - modelName: string; - references?: { src: string; dst: string }[]; - units?: string; - documentation?: string; - } = { - name: moduleIdent, - modelName: newModelName, - }; - if (existingModule && existingModule.type === 'module') { - if (existingModule.references.length > 0) { - dupModulePayload.references = existingModule.references.map((ref) => ({ src: ref.src, dst: ref.dst })); - } - if (existingModule.units) dupModulePayload.units = existingModule.units; - if (existingModule.documentation) dupModulePayload.documentation = existingModule.documentation; - } + const dupModulePayload = buildModuleReferencePayload(existingModule, moduleIdent, newModelName); // Combined patch: create model, copy contents, update module reference. // Engine processes projectOps before model ops (patch.rs). diff --git a/src/diagram/module-wiring.ts b/src/diagram/module-wiring.ts index e99f3b4f4..649837933 100644 --- a/src/diagram/module-wiring.ts +++ b/src/diagram/module-wiring.ts @@ -4,7 +4,31 @@ // pattern: Functional Core -- pure functions for immutable reference array manipulation -import type { ModuleReference } from '@simlin/core/datamodel'; +import { type ModuleReference, type Variable, moduleToJson } from '@simlin/core/datamodel'; +import type { JsonModule } from '@simlin/engine'; + +/** + * Build the upsertModule payload that re-points an existing module variable at + * `newModelName` while preserving every other field. upsertModule is a + * full-replace by UID, so hand-listing fields silently drops anything omitted -- + * notably compat (canBeModuleInput / isPublic / dataSource), the same + * silent-data-loss trap fixed elsewhere in this change. Basing the payload on + * moduleToJson(existing) and overriding only name/modelName carries every field + * forward. Falls back to a bare {name, modelName} when there is no existing + * module (or the variable at that ident is not a module). Shared by the + * create-model-for-module and duplicate-model-for-module handlers so the two + * cannot drift apart again. + */ +export function buildModuleReferencePayload( + existing: Variable | undefined, + moduleIdent: string, + newModelName: string, +): JsonModule { + if (existing && existing.type === 'module') { + return { ...moduleToJson(existing), name: moduleIdent, modelName: newModelName }; + } + return { name: moduleIdent, modelName: newModelName }; +} // A module reference `dst` is stored in the canonical module-qualified form // `{moduleIdent}·{port}`: the engine's `build_module_inputs` strips that prefix diff --git a/src/diagram/tests/module-wiring.test.ts b/src/diagram/tests/module-wiring.test.ts index 12a491891..af904157d 100644 --- a/src/diagram/tests/module-wiring.test.ts +++ b/src/diagram/tests/module-wiring.test.ts @@ -6,7 +6,7 @@ * @jest-environment node */ -import type { ModuleReference } from '@simlin/core/datamodel'; +import type { ModuleReference, Module, Aux } from '@simlin/core/datamodel'; import { isDuplicateReference, @@ -17,8 +17,28 @@ import { getAvailableSrcVariables, qualifyDst, unqualifyDst, + buildModuleReferencePayload, } from '../module-wiring'; +function makeModule(overrides: Partial = {}): Module { + return { + type: 'module', + ident: 'hares_inst', + modelName: 'hares', + documentation: 'doc', + units: 'widgets', + references: [{ src: 'predators', dst: 'hares_inst·input_food' }], + canBeModuleInput: true, + isPublic: true, + dataSource: undefined, + data: undefined, + errors: undefined, + unitErrors: undefined, + uid: 7, + ...overrides, + }; +} + // -- Tests -- describe('qualifyDst', () => { @@ -254,3 +274,57 @@ describe('getAvailableSrcVariables', () => { expect(result).toEqual([]); }); }); + +describe('buildModuleReferencePayload', () => { + it('preserves compat (canBeModuleInput/isPublic/dataSource) while re-pointing the model', () => { + // upsertModule is a full-replace by UID, so re-pointing a module at a new + // model must carry every field forward -- compat is the data-loss trap. + const existing = makeModule({ + canBeModuleInput: true, + isPublic: true, + dataSource: { kind: 'data', file: 'd.csv', tabOrDelimiter: ',', rowOrCol: 'A', cell: 'B2' }, + }); + const payload = buildModuleReferencePayload(existing, 'hares_inst', 'hares_copy'); + expect(payload.name).toBe('hares_inst'); + expect(payload.modelName).toBe('hares_copy'); + expect(payload.compat?.canBeModuleInput).toBe(true); + expect(payload.compat?.isPublic).toBe(true); + expect(payload.compat?.dataSource).toEqual({ + kind: 'data', + file: 'd.csv', + tabOrDelimiter: ',', + rowOrCol: 'A', + cell: 'B2', + }); + // existing references/units/documentation also carry forward + expect(payload.references).toEqual([{ src: 'predators', dst: 'hares_inst·input_food' }]); + expect(payload.units).toBe('widgets'); + expect(payload.documentation).toBe('doc'); + }); + + it('returns a bare {name, modelName} when there is no existing module', () => { + const payload = buildModuleReferencePayload(undefined, 'new_inst', 'new_model'); + expect(payload).toEqual({ name: 'new_inst', modelName: 'new_model' }); + }); + + it('returns a bare payload when the existing variable is not a module', () => { + const aux: Aux = { + type: 'aux', + ident: 'not_a_module', + equation: { type: 'scalar', equation: '1' }, + documentation: '', + units: '', + gf: undefined, + canBeModuleInput: false, + isPublic: false, + activeInitial: undefined, + dataSource: undefined, + data: undefined, + errors: undefined, + unitErrors: undefined, + uid: 3, + }; + const payload = buildModuleReferencePayload(aux, 'x', 'y'); + expect(payload).toEqual({ name: 'x', modelName: 'y' }); + }); +}); From 3c9ac02a75c00a6e8fefcd87e9b0c028b3cfb55d Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 23:09:18 -0700 Subject: [PATCH 07/11] deploy: thread the selected project into the canary deploy step When an operator targets a non-default project via --project or SIMLIN_CANARY_PROJECT, deployStagedNoPromote invoked deploy-web-staged.sh with only --no-promote, so its final `gcloud app deploy` used the ambient gcloud project while the version lookup, traffic check, version delete, and Firebase authorized-domain mutation all used the selected project. That split-brain could deploy HEAD to an unintended project and authorize/operate on a version in a different one. Pass --project= through the deploy (deploy-web- staged.sh forwards "$@" to gcloud app deploy); resolveProject always yields a concrete id, so this is correct and harmlessly explicit in the default case too. Found in PR review. --- scripts/deploy-canary.mjs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/scripts/deploy-canary.mjs b/scripts/deploy-canary.mjs index 524f4b0b6..1e77dee48 100644 --- a/scripts/deploy-canary.mjs +++ b/scripts/deploy-canary.mjs @@ -247,11 +247,18 @@ function accessToken() { } /** Build + deploy the staged server with --no-promote (traffic stays put). */ -function deployStagedNoPromote() { +function deployStagedNoPromote(project) { console.log('\n==> Building and deploying the staged server with --no-promote (no traffic switch)\n'); // Reuse the proven staged build/deploy orchestration; do NOT pass --version, - // so gcloud auto-generates the version id we then discover below. - run('bash', [path.join(REPO_ROOT, 'scripts/deploy-web-staged.sh'), '--no-promote']); + // so gcloud auto-generates the version id we then discover below. Pass the + // resolved --project through (deploy-web-staged.sh forwards "$@" to + // `gcloud app deploy`): otherwise an operator who selected a non-default + // project via --project/SIMLIN_CANARY_PROJECT would deploy HEAD to the + // AMBIENT gcloud project while the version lookup and Firebase mutation below + // target the selected one -- a split-brain that could deploy to the wrong + // project. resolveProject() always yields a concrete id, so this is also + // correct (and harmlessly explicit) in the default case. + run('bash', [path.join(REPO_ROOT, 'scripts/deploy-web-staged.sh'), '--no-promote', `--project=${project}`]); } /** @@ -425,7 +432,7 @@ async function runDeployMode(project, token) { const preflight = await getIdentityConfig(project, token); printDomains('current authorizedDomains', preflight.authorizedDomains ?? []); - deployStagedNoPromote(); + deployStagedNoPromote(project); const versionId = latestVersionId(project); const { url, host } = versionUrlAndHost(project, versionId); From 8dbb1b05f6ff18bd156dcd8927ee4c3e17386650 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 23:24:20 -0700 Subject: [PATCH 08/11] core: keep datamodel tests runnable without a built engine The new datamodel-roundtrip-e2e test drives the real WASM engine, so it statically imported @simlin/engine and read libsimlin.wasm. On a clean checkout (or after pnpm clean) before pnpm build, `pnpm --filter @simlin/core test` then failed at module resolution (Could not locate module @simlin/engine), breaking core's documented standalone test script. Make the @simlin/engine value import dynamic (inside loadWasm) and gate the suite on the presence of the built libsimlin.wasm: skip with a clear note when the engine is not built, run fully otherwise. CI and the pre-commit hook build before testing, so the integration coverage is unchanged there; the pure datamodel.test.ts already covers every field without the engine. Found in PR review. --- .../tests/datamodel-roundtrip-e2e.test.ts | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/src/core/tests/datamodel-roundtrip-e2e.test.ts b/src/core/tests/datamodel-roundtrip-e2e.test.ts index 614df86f4..aee079bd0 100644 --- a/src/core/tests/datamodel-roundtrip-e2e.test.ts +++ b/src/core/tests/datamodel-roundtrip-e2e.test.ts @@ -19,17 +19,36 @@ import * as fs from 'fs'; import * as path from 'path'; -import { Project, configureWasm, ready, resetWasm } from '@simlin/engine'; import type { JsonProjectPatch } from '@simlin/engine'; import { projectFromJson, projectToJson, auxToJson, stockToJson, moduleToJson } from '../datamodel'; +// This e2e drives the REAL WASM engine, so it needs `pnpm build` to have +// produced the engine package (lib/) and libsimlin.wasm. Keep `pnpm --filter +// @simlin/core test` runnable standalone on a clean checkout by SKIPPING (not +// failing) when the built engine is absent; CI and the pre-commit hook build +// first, so this integration coverage always runs there. The @simlin/engine +// VALUE import is dynamic (inside loadWasm) so a missing build does not break +// module resolution for the skipped suite -- the pure datamodel.test.ts still +// covers every field without the engine. +const wasmPath = path.join(__dirname, '..', '..', 'engine', 'core', 'libsimlin.wasm'); +const engineBuilt = fs.existsSync(wasmPath); +const describeIfEngine = engineBuilt ? describe : describe.skip; +if (!engineBuilt) { + console.warn( + `[datamodel-roundtrip-e2e] skipping engine-backed checks: ${wasmPath} not found; run \`pnpm build\` to exercise them.`, + ); +} + +type EngineModule = typeof import('@simlin/engine'); +let engine: EngineModule; + async function loadWasm(): Promise { - const wasmPath = path.join(__dirname, '..', '..', 'engine', 'core', 'libsimlin.wasm'); + engine = await import('@simlin/engine'); const wasmBuffer = fs.readFileSync(wasmPath); - await resetWasm(); - configureWasm({ source: wasmBuffer }); - await ready(); + await engine.resetWasm(); + engine.configureWasm({ source: wasmBuffer }); + await engine.ready(); } // A project whose variables carry every field that datamodel.ts previously @@ -147,7 +166,7 @@ interface ParsedProject { } async function serializeProject(json: string): Promise { - const project = await Project.openJson(json); + const project = await engine.Project.openJson(json); try { return JSON.parse(await project.serializeJson()) as ParsedProject; } finally { @@ -195,7 +214,7 @@ function assertAllFieldsPresent(parsed: ParsedProject): void { }); } -describe('datamodel round-trip through the real engine serializer', () => { +describeIfEngine('datamodel round-trip through the real engine serializer', () => { beforeAll(async () => { await loadWasm(); }); @@ -218,7 +237,7 @@ describe('datamodel round-trip through the real engine serializer', () => { }); it('survives a full upsert replace (the literal data-loss bug)', async () => { - const project = await Project.openJson(BASE_PROJECT); + const project = await engine.Project.openJson(BASE_PROJECT); try { const datamodel = projectFromJson(JSON.parse(await project.serializeJson()) as never); const model = datamodel.models.get('main'); From b0316e574af07d0baa25b5d230a76d0d5509d1e6 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 23:42:16 -0700 Subject: [PATCH 09/11] core: route zero-element arrayed equations as arrayed The engine distinguishes ApplyToAll from Arrayed by the PRESENCE of the JSON `elements` field, not by it being non-empty: json.rs omits `elements` for ApplyToAll and always emits it for Arrayed (even as []). datamodel.ts routed an `elements: []` payload to applyToAll, which is not equivalent when the arrayed equation carries a default and hasExceptDefault:false (an EXCEPT excluding every subscript): the engine leaves every missing element at 0 (compiler/mod.rs uses the default only when apply_default_for_missing is true), but applyToAll makes every element use the default -- a silent behavior change the next editor upsert would persist. Route on `elements` presence (`!== undefined`) in both the stock and the aux/flow paths so a zero-element arrayed equation stays arrayed and round-trips hasExceptDefault. The hasExceptDefault:true case stays equivalent. Found in PR review. --- src/core/datamodel.ts | 27 +++++++++------- src/core/tests/datamodel.test.ts | 55 ++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index 4859af789..41e963c4b 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -293,12 +293,15 @@ function stockEquationFromJson( arrayedEquation: JsonArrayedEquation | undefined, ): Equation { if (arrayedEquation) { - // Per-element equations imply an 'arrayed' equation; otherwise it's applyToAll. - // Known inert divergence: a payload with an EXCEPT default but zero elements - // routes to applyToAll here (the engine routes Some([]) to Arrayed), dropping - // hasExceptDefault. This is semantically equivalent (a default with no excepted - // subscripts applies to all elements either way) and real models never emit it. - if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { + // The engine distinguishes ApplyToAll from Arrayed by the PRESENCE of the + // `elements` field, not by it being non-empty (json.rs: ApplyToAll omits + // `elements`; Arrayed always emits it, even as []). An Arrayed with no + // explicit elements + an EXCEPT default + hasExceptDefault:false means every + // element is missing and evaluates to 0, NOT the default (compiler/mod.rs: + // a missing element uses the default only when apply_default_for_missing is + // true). Collapsing that to applyToAll would silently change behavior, so + // route on presence: `elements` present (even []) => arrayed. + if (arrayedEquation.elements !== undefined) { return arrayedEquationFromJson(arrayedEquation); } else { return { @@ -322,12 +325,12 @@ function auxEquationFromJson( } if (arrayedEquation) { - // Per-element equations imply an 'arrayed' equation; otherwise it's applyToAll. - // Known inert divergence: a payload with an EXCEPT default but zero elements - // routes to applyToAll here (the engine routes Some([]) to Arrayed), dropping - // hasExceptDefault. This is semantically equivalent (a default with no excepted - // subscripts applies to all elements either way) and real models never emit it. - if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { + // Route on the PRESENCE of `elements`, not on it being non-empty -- see the + // matching note in stockEquationFromJson. The engine emits `elements` for + // every Arrayed (even []) and omits it for ApplyToAll; an Arrayed with no + // explicit elements + hasExceptDefault:false has every element evaluate to 0, + // which collapsing to applyToAll would silently change. + if (arrayedEquation.elements !== undefined) { return { equation: arrayedEquationFromJson(arrayedEquation), graphicalFunction }; } else { return { diff --git a/src/core/tests/datamodel.test.ts b/src/core/tests/datamodel.test.ts index f31498ae4..4ff1bef34 100644 --- a/src/core/tests/datamodel.test.ts +++ b/src/core/tests/datamodel.test.ts @@ -1159,6 +1159,61 @@ describe('Arrayed Equations', () => { expect(eq.elements.get('north')?.equation).toBe('50'); expect(eq.elements.get('south')?.equation).toBe('75'); }); + + // The engine serializes ApplyToAll with NO `elements` field and Arrayed with + // `elements` PRESENT (even []). An Arrayed with elements:[] + a default + + // hasExceptDefault:false means EVERY element is missing and evaluates to 0 (a + // missing element uses the default only when apply_default_for_missing/ + // hasExceptDefault is true -- compiler/mod.rs -- else 0.0). Collapsing such a + // payload to applyToAll would make every element use the default instead: a + // real behavior change the next editor upsert would persist. So the read path + // must route on `elements` PRESENCE, not on it being non-empty. + it('keeps a zero-element arrayed aux arrayed (EXCEPT-excludes-all), not applyToAll', () => { + const restored = auxFromJson({ + name: 'demand', + arrayedEquation: { + dimensions: ['regions'], + equation: '99', + elements: [], + hasExceptDefault: false, + }, + }); + expect(restored.equation.type).toBe('arrayed'); + const eq = restored.equation as ArrayedEquation; + expect(eq.dimensionNames).toEqual(['regions']); + expect(eq.elements.size).toBe(0); + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(false); + + // Round-trips back to elements:[] + equation:'99' + hasExceptDefault:false, + // which the engine reads as Arrayed(.., [], Some('99'), false). + const out = auxToJson(restored); + expect(out.arrayedEquation).toBeDefined(); + expect(out.arrayedEquation!.elements).toEqual([]); + expect(out.arrayedEquation!.equation).toBe('99'); + expect(out.arrayedEquation!.hasExceptDefault).toBe(false); + }); + + it('keeps a zero-element arrayed stock arrayed (EXCEPT-excludes-all), not applyToAll', () => { + const restored = stockFromJson({ + name: 'level', + initialEquation: '', + inflows: [], + outflows: [], + arrayedEquation: { + dimensions: ['regions'], + equation: '99', + elements: [], + hasExceptDefault: false, + }, + }); + expect(restored.equation.type).toBe('arrayed'); + const eq = restored.equation as ArrayedEquation; + expect(eq.dimensionNames).toEqual(['regions']); + expect(eq.elements.size).toBe(0); + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(false); + }); }); describe('LinkViewElement multiPoint', () => { From 85a502934de64f1c48c6df99c14facd032aa613a Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 27 Jun 2026 23:58:41 -0700 Subject: [PATCH 10/11] core: merge legacy top-level module visibility flags on import The engine's JSON reader (json.rs) OR-merges the legacy top-level canBeModuleInput / isPublic fields with compat for stock, flow, aux, AND module (read from old JSON, never written). datamodel.ts only OR-merged the legacy nonNegative flag (stock/flow) and read canBeModuleInput/isPublic from compat alone, so a project serialized in the old Go `sd` JSON schema -- visibility flags at the top level rather than under compat -- would import them as false and silently drop module-input / public visibility on the next edit+upsert. Mirror the engine reader: OR-merge the legacy top-level flags in all four readers, and declare the deprecated fields on the four Json* types. This is defense-in-depth for the current editor (the engine serializer normalizes to compat before datamodel.ts sees the JSON), but it removes a real divergence between the two JSON readers. Found in PR review. --- src/core/datamodel.ts | 26 ++++++++++++++------ src/core/tests/datamodel.test.ts | 42 ++++++++++++++++++++++++++++++++ src/engine/src/json-types.ts | 16 ++++++++++++ 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index 41e963c4b..4db6213b5 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -503,9 +503,12 @@ export function stockFromJson(json: JsonStock): Stock { outflows: json.outflows ?? [], // OR-merge: old code never writes compat booleans and new code never // writes top-level booleans, so both cannot be meaningfully set at once. + // Mirrors the engine's JSON reader (json.rs), which OR-merges all three + // legacy top-level flags so a project saved in the old schema is not + // silently stripped on the next edit. nonNegative: json.compat?.nonNegative || json.nonNegative || false, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, activeInitial: json.compat?.activeInitial, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, @@ -585,9 +588,12 @@ export function flowFromJson(json: JsonFlow): Flow { gf: graphicalFunction, // OR-merge: old code never writes compat booleans and new code never // writes top-level booleans, so both cannot be meaningfully set at once. + // Mirrors the engine's JSON reader (json.rs), which OR-merges all three + // legacy top-level flags so a project saved in the old schema is not + // silently stripped on the next edit. nonNegative: json.compat?.nonNegative || json.nonNegative || false, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, activeInitial: json.compat?.activeInitial, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, @@ -666,8 +672,10 @@ export function auxFromJson(json: JsonAuxiliary): Aux { documentation: json.documentation ?? '', units: json.units ?? '', gf: graphicalFunction, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + // OR-merge legacy top-level flags with compat, mirroring the engine's JSON + // reader (json.rs); old JSON wrote them at top level, new JSON under compat. + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, activeInitial: json.compat?.activeInitial, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, @@ -735,8 +743,10 @@ export function moduleFromJson(json: JsonModule): Module { documentation: json.documentation ?? '', units: json.units ?? '', references: (json.references ?? []).map((ref: JsonModuleReference) => moduleReferenceFromJson(ref)), - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + // OR-merge legacy top-level flags with compat, mirroring the engine's JSON + // reader (json.rs); old JSON wrote them at top level, new JSON under compat. + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, diff --git a/src/core/tests/datamodel.test.ts b/src/core/tests/datamodel.test.ts index 4ff1bef34..9b998bf07 100644 --- a/src/core/tests/datamodel.test.ts +++ b/src/core/tests/datamodel.test.ts @@ -197,6 +197,48 @@ describe('Stock', () => { }); }); +describe('legacy top-level canBeModuleInput / isPublic', () => { + // The engine's JSON reader (json.rs) OR-merges legacy top-level + // canBeModuleInput/isPublic with compat for stock/flow/aux/module (read from + // old JSON, never written). datamodel.ts must mirror it so a project saved in + // the old Go `sd` JSON schema -- flags at the top level rather than under + // compat -- does not silently lose module-input / public visibility the next + // time the variable is edited and re-upserted. + it('stockFromJson reads legacy top-level flags', () => { + const stock = stockFromJson({ name: 'level', inflows: [], outflows: [], canBeModuleInput: true, isPublic: true }); + expect(stock.canBeModuleInput).toBe(true); + expect(stock.isPublic).toBe(true); + }); + + it('flowFromJson reads legacy top-level flags', () => { + const flow = flowFromJson({ name: 'rate', canBeModuleInput: true, isPublic: true }); + expect(flow.canBeModuleInput).toBe(true); + expect(flow.isPublic).toBe(true); + }); + + it('auxFromJson reads legacy top-level flags', () => { + const aux = auxFromJson({ name: 'x', canBeModuleInput: true, isPublic: true }); + expect(aux.canBeModuleInput).toBe(true); + expect(aux.isPublic).toBe(true); + }); + + it('moduleFromJson reads legacy top-level flags', () => { + const mod = moduleFromJson({ name: 'sub_inst', modelName: 'sub', canBeModuleInput: true, isPublic: true }); + expect(mod.canBeModuleInput).toBe(true); + expect(mod.isPublic).toBe(true); + }); + + it('moduleFromJson still reads the new compat format', () => { + const mod = moduleFromJson({ + name: 'sub_inst', + modelName: 'sub', + compat: { canBeModuleInput: true, isPublic: true }, + }); + expect(mod.canBeModuleInput).toBe(true); + expect(mod.isPublic).toBe(true); + }); +}); + describe('Flow', () => { it('should roundtrip correctly', () => { const flow: Flow = { diff --git a/src/engine/src/json-types.ts b/src/engine/src/json-types.ts index 41885c713..240e3e536 100644 --- a/src/engine/src/json-types.ts +++ b/src/engine/src/json-types.ts @@ -107,6 +107,10 @@ export interface JsonStock { compat?: JsonCompat; /** @deprecated Legacy field; read from compat.nonNegative in new format. */ nonNegative?: boolean; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -123,6 +127,10 @@ export interface JsonFlow { compat?: JsonCompat; /** @deprecated Legacy field; read from compat.nonNegative in new format. */ nonNegative?: boolean; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -137,6 +145,10 @@ export interface JsonAuxiliary { documentation?: string; arrayedEquation?: JsonArrayedEquation; compat?: JsonCompat; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -150,6 +162,10 @@ export interface JsonModule { documentation?: string; references?: JsonModuleReference[]; compat?: JsonCompat; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** From a93d3858569e01fb4b54fb79fe8fd770b895c6cc Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sun, 28 Jun 2026 01:11:40 -0700 Subject: [PATCH 11/11] core: read ACTIVE INITIAL from arrayedEquation.compat for flow/aux The engine's JSON reader (json.rs) reads a flow/aux ACTIVE INITIAL from the top-level compat first and falls back to arrayedEquation.compat.active_initial, a legacy/native JSON shape. datamodel.ts read only the top-level compat, so an arrayed flow/aux that stored ACTIVE INITIAL on its arrayed equation imported it as undefined and a later full upsert via flowToJson/auxToJson dropped the initialization. Mirror the engine: top-level compat wins, else fall back to the arrayed equation's compat. Stocks have no such fallback in the engine reader (and it never writes one), so they are intentionally left unchanged. Found in PR review. --- src/core/datamodel.ts | 8 ++++++-- src/core/tests/datamodel.test.ts | 33 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index 4db6213b5..9b0a855ed 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -594,7 +594,9 @@ export function flowFromJson(json: JsonFlow): Flow { nonNegative: json.compat?.nonNegative || json.nonNegative || false, canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, isPublic: json.compat?.isPublic || json.isPublic || false, - activeInitial: json.compat?.activeInitial, + // ACTIVE INITIAL: top-level compat wins, else fall back to the arrayed + // equation's compat (a legacy/native JSON shape the engine reader accepts). + activeInitial: json.compat?.activeInitial || json.arrayedEquation?.compat?.activeInitial, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, @@ -676,7 +678,9 @@ export function auxFromJson(json: JsonAuxiliary): Aux { // reader (json.rs); old JSON wrote them at top level, new JSON under compat. canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, isPublic: json.compat?.isPublic || json.isPublic || false, - activeInitial: json.compat?.activeInitial, + // ACTIVE INITIAL: top-level compat wins, else fall back to the arrayed + // equation's compat (a legacy/native JSON shape the engine reader accepts). + activeInitial: json.compat?.activeInitial || json.arrayedEquation?.compat?.activeInitial, dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, diff --git a/src/core/tests/datamodel.test.ts b/src/core/tests/datamodel.test.ts index 9b998bf07..de631714d 100644 --- a/src/core/tests/datamodel.test.ts +++ b/src/core/tests/datamodel.test.ts @@ -239,6 +239,39 @@ describe('legacy top-level canBeModuleInput / isPublic', () => { }); }); +describe('ACTIVE INITIAL under arrayedEquation.compat', () => { + // The engine's JSON reader (json.rs) reads a flow/aux ACTIVE INITIAL from the + // top-level compat first, then falls back to arrayedEquation.compat (a legacy/ + // native JSON shape). datamodel.ts must mirror it so an arrayed flow/aux that + // stored ACTIVE INITIAL on the arrayed equation does not lose it on the next + // edit+upsert. (Stocks have no such fallback in the engine reader, so we do + // not add one here.) + it('flowFromJson falls back to arrayedEquation.compat.activeInitial', () => { + const flow = flowFromJson({ + name: 'rate', + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: '7' } }, + }); + expect(flow.activeInitial).toBe('7'); + }); + + it('auxFromJson falls back to arrayedEquation.compat.activeInitial', () => { + const aux = auxFromJson({ + name: 'x', + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: '9' } }, + }); + expect(aux.activeInitial).toBe('9'); + }); + + it('top-level compat.activeInitial wins over the arrayed fallback', () => { + const aux = auxFromJson({ + name: 'x', + compat: { activeInitial: 'top' }, + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: 'arrayed' } }, + }); + expect(aux.activeInitial).toBe('top'); + }); +}); + describe('Flow', () => { it('should roundtrip correctly', () => { const flow: Flow = {