Pre-deploy hardening: fix silent data-loss + undo regression, pin pnpm, add canary deploy#810
Conversation
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.
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.
The staged GAE deploy generates a self-contained server package.json that pins packageManager: pnpm@<version>. 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.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
==========================================
- Coverage 90.87% 90.87% -0.01%
==========================================
Files 224 224
Lines 143133 143133
==========================================
- Hits 130072 130070 -2
- Misses 13061 13063 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review summaryI reviewed the four work streams in this PR (datamodel round-trip, undo recording, engines.pnpm pin, canary deploy script) against the engine's What I cross-checked
Overall correctness: correct. Each of the four work streams has end-to-end test coverage (the e2e test for the datamodel fix even drives the real WASM engine to catch wrong JSON keys), and the patch does not regress existing behavior in places I inspected. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b65dad47a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** 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}`]); |
There was a problem hiding this comment.
When cleaning up an unpromoted canary for this app, this command will fail because the production config is expected to use automatic_scaling (app.yaml:6-7), while the Google Cloud SDK docs for gcloud app versions stop state that it only works when the service scaling is manual. That means pnpm deploy:canary --cleanup <version> de-authorizes Firebase but then aborts before reclaiming the App Engine version/slot; after the existing zero-traffic guard, use gcloud app versions delete or another cleanup path that works for automatic-scaling versions.
Useful? React with 👍 / 👎.
| case '--project': | ||
| args.project = takeVal(); |
There was a problem hiding this comment.
If an operator passes --project without a value (for example --project $PROJECT when the shell variable is empty, or --project --cleanup v), takeVal() returns undefined and resolveProject() silently falls back to SIMLIN_CANARY_PROJECT or the current gcloud default. Because this script deploys and mutates Firebase auth config, a missing override should fail loudly like the --cleanup value check rather than potentially operating on the wrong project.
Useful? React with 👍 / 👎.
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.
|
@codex review |
Review Findings[P2] handleDuplicateModelForModule still drops module compat fieldsFile: src/diagram/Editor.tsx:1903-1919 The PR upgrades handleCreateModelForModule (line 1838-1841) to use moduleToJson(existingModule) so the module reference preserves canBeModuleInput / isPublic / dataSource, but the very next handler handleDuplicateModelForModule still hand-constructs dupModulePayload with only name/modelName/references/units/documentation. When a user picks "Duplicate model" for an existing module variable whose compat carries canBeModuleInput, isPublic, or a Vensim dataSource, the upsert is a full-replace by UID and silently drops those fields - the same data-loss class the PR set out to fix. The fix is the same pattern used in the sibling handler: spread moduleToJson(existingModule) and override name and modelName (with an empty payload in the no-existing-module branch). Overall correctness verdict: correctThe datamodel round-trip additions (activeInitial / dataSource / EXCEPT default / per-element graphical functions / per-element activeInitial / module compat) match the engine json.rs field names and Compat shape, the inert ApplyToAll-vs-Arrayed divergence is documented and semantically equivalent, the updateView/recordHistory rewiring records exactly one entry per discrete edit (the post-content-patch snapshot captures both halves, no double-recording) while the viewport-only stream still goes through queueViewUpdate and records nothing, the deploy-staging manifest correctly derives the bare pnpm version from a +sha512-suffixed packageManager spec and merges it onto inherited engines, and the canary script read-modify-write against the full current authorizedDomains list plus the traffic-share guard on cleanup look safe. The handleDuplicateModelForModule miss above is the only finding and is a non-blocking quality issue. |
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c11ac13ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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']); |
There was a problem hiding this comment.
When an operator uses --project or SIMLIN_CANARY_PROJECT to target a project other than the active gcloud default, this call still invokes deploy-web-staged.sh with only --no-promote; I checked that script and its final gcloud app deploy only receives its forwarded argv, so the deploy goes to the ambient gcloud project while the subsequent version lookup and Firebase authorized-domain mutation use the selected project. That can authorize/promote the wrong version and deploy HEAD to an unintended production project; pass --project=${project} through this deploy call (and thread project into this helper).
Useful? React with 👍 / 👎.
|
Reviewed: The JSON key names in No actionable bugs found. Overall correctness: correct. |
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=<resolved> 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.
|
@codex review |
Review verdict: correctI reviewed each commit end-to-end and found no blocking issues.
Small nit, non-blocking: the PR description says cleanup "stops the version", but the script (and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c9ac02a75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
|
|
||
| import { Project, configureWasm, ready, resetWasm } from '@simlin/engine'; |
There was a problem hiding this comment.
When the core package test is run from a clean checkout or after pnpm clean before pnpm build, this new runtime import is resolved by src/core/jest.config.js to src/engine/lib/index.js, which does not exist until the engine package has been built. I reproduced pnpm --dir src/core exec jest --runInBand failing with Could not locate module @simlin/engine; either map the e2e test to engine sources or skip/build-gate it so the package's documented test script remains runnable independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was addressed in 8dbb1b0 (before this review ran). Line 22 is now a type-only import type { JsonProjectPatch } (erased at runtime); the @simlin/engine value import is dynamic inside loadWasm, and the suite is gated on the built libsimlin.wasm via describeIfEngine. I reproduced your exact command with the engine fully unbuilt (both src/engine/lib and the wasm removed):
$ pnpm --dir src/core exec jest --runInBand
Test Suites: 1 skipped, 2 passed, 2 of 3 total
Tests: 3 skipped, 68 passed, 71 total
No "Could not locate module @simlin/engine" -- the e2e skips cleanly and the package's standalone test script passes.
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.
|
@codex review |
|
Reviewed the three fixes plus the canary script. Traced the data round-trip via key names against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dbb1b05f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // hasExceptDefault. This is semantically equivalent (a default with no excepted | ||
| // subscripts applies to all elements either way) and real models never emit it. |
There was a problem hiding this comment.
This fallback is not equivalent when the serialized arrayed equation has elements: [], a default equation, and hasExceptDefault: false (for example an EXCEPT that excludes every subscript). The engine represents that as Arrayed with apply_default_for_missing=false, so omitted elements evaluate as 0; converting it to applyToAll makes every element use the default equation and the next editor upsert persists that changed behavior. Please treat a present elements array as arrayed even when it is empty, and apply the same fix in the aux/flow path below.
Useful? React with 👍 / 👎.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0316e574a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| canBeModuleInput: json.compat?.canBeModuleInput ?? false, | ||
| isPublic: json.compat?.isPublic ?? false, |
There was a problem hiding this comment.
When the editor opens a project that still uses the legacy top-level module fields canBeModuleInput / isPublic, these newly added fields are initialized to false because only json.compat is read. The Rust JSON layer still accepts those legacy fields for modules, and stock/flow already OR-merge their legacy top-level flags, so a subsequent module edit/upsert will silently drop module input/public visibility for those projects. Please OR these with the legacy top-level values as well.
Useful? React with 👍 / 👎.
|
Reviewed the PR end-to-end (datamodel round-trip, undo recording, manifest pin, canary script). Most of the change is well-tested and the defensive coverage is strong; I have one small follow-up. [P3]
|
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review SummaryI reviewed all 10 commits in this PR. The changes are well-scoped, well-tested, and well-documented. Each of the three main concerns is appropriately addressed:
No findings to flagI do not see any concrete, blocking bugs introduced by this PR. A handful of minor edge cases exist (e.g. the canary cleanup will exit early if VerdictCorrect. The patch is free of blocking issues; existing tests pass and the new tests appropriately cover the changed behavior. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85a502934d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isPublic: json.compat?.isPublic ?? false, | ||
| canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, | ||
| isPublic: json.compat?.isPublic || json.isPublic || false, | ||
| activeInitial: json.compat?.activeInitial, |
There was a problem hiding this comment.
When importing legacy/native JSON where an arrayed flow or auxiliary carries ACTIVE INITIAL under arrayedEquation.compat (the Rust reader still supports this fallback in src/simlin-engine/src/json.rs), this line leaves activeInitial undefined unless top-level compat also has it. A later full upsert via flowToJson/auxToJson will then omit the ACTIVE INITIAL and drop that initialization metadata; use the same top-level-then-arrayed fallback here and on the aux path.
Useful? React with 👍 / 👎.
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.
|
@codex review |
ReviewI reviewed the four areas: the datamodel round-trip fix, the undo history fix, the engines.pnpm pin, and the canary deploy script. Spot-checked the new TS round-trip code against the Rust engine's
For the undo fix: the For For the canary script: the read-modify-write pattern is correct and the preflight GET fails fast before the long deploy; Overall correctness: correctNo blocking bugs found. The PR is comprehensively tested (pure round-trip tests + real-engine e2e + manifest tests + controller history tests + canary script pure-core tests) and the comments document non-obvious decisions (the |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Why
A pre-deploy audit of HEAD against the version currently running in production (
ae68caa4, from 2022) found that the code is broadly safe to ship except for one silent data-loss bug and one undo regression, plus a deploy-mechanics gap. This PR fixes those and adds tooling to safely canary the deploy. Each commit passed the full pre-commit gate (Rust fmt/clippy/test, WASM build, TS lint/build/tsc/test, pysimlin).What
1.
core: round-trip variable compat to avoid silent data loss (blocker).Model content is persisted as engine-serialized protobuf, and edits apply as full-replace upsert ops.
datamodel.tsdropped several wire fields on the JSON round-trip, so editing any field of an affected variable silently discarded them on save:ACTIVE INITIAL(separate initialization equation), the VensimEXCEPTdefault equation, per-element graphical functions, external-data references (GET DIRECT DATA/CONSTANTS/LOOKUPS), and module compat (canBeModuleInput/isPublic/dataSource). TheACTIVE INITIALcase silently corrupted real 2022 models' numerics on a routine edit. All are now round-tripped for Stock/Flow/Aux/Module; JSON key names were verified against the engine'sjson.rs, and an end-to-end test drives the real WASM engine through an upsert round-trip.2.
diagram: record undo history for canvas content and layout edits (high).Create, delete, move, label-move, and flow/link attach were not undoable (only equation/table/module/rename/sim-spec edits recorded history), making accidental deletes unrecoverable. These six discrete edits now record exactly one history entry each via a
recordHistoryoption onupdateView; the per-frame viewport stream (pan/zoom/momentum) still records nothing, so a momentum flick can't evict real edits from the small undo buffer.3.
deploy: pinengines.pnpmin the staged server manifest.The staged GAE deploy (PR #809) has never run against real
gcloud. App Engine's Node buildpack readsengines.pnpmwith precedence overpackageManager/corepack, and an exact version short-circuits its registry resolution; without it the buildpack can fall back to its bundled pnpm and reject the frozen lockfile. The pin is derived from the samepackageManagerspec (single source of truth).4.
deploy: add a canary deploy script (pnpm deploy:canary).Automates a
--no-promotestaged deploy and temporarily authorizes the canary's*.appspot.comhost in FirebaseauthorizedDomains(read-modify-write PATCH that can't wipe the list) so the operator can exercise the real product, including Google login, before switching traffic. A--cleanupmode de-authorizes the host and deletes the version, but refuses to delete a version that is serving traffic (so cleanup after promotion can't take the site down). Uses the operator's own credentials, not the CI deploy SA, and never promotes traffic.Testing
TDD throughout; each item was implemented by one subagent and independently adversarially reviewed by another before commit. Pure logic is unit-tested (datamodel round-trips, history recording, manifest generation, canary host/domain/traffic helpers), plus a real-engine end-to-end round-trip test for the data-loss fix. The canary script's gcloud/Firebase shell cannot be integration-tested without prod credentials and was reviewed by reading.
Residual findings (tracked separately, not in this PR)
The audit surfaced additional, lower-severity items being filed as issues: view-element fidelity dropped on view edits, a dangling link/flow reference making the editor uneditable, a stale chart shown as current after a failed re-sim, no SIGTERM/graceful-shutdown handler, the in-process render worker being an unbounded CPU/mem sink, CI deploy-gate gaps (node 22 vs prod node 24, no
engines.nodepin, non-frozen lockfile, staged path not exercised), and.app.prod.yamlmissing the static-handler CSP/HSTS headers.