Repository navigation
engine: harden the MDL writer (re-parse + semantic-loss + lossiness contract) - #864
Merged
Merged
Conversation
Adds two corpus-wide regression ratchets for the Vensim MDL writer plus a coverage floor, and retires three stale exclusions. Groundwork for the MDL writer hardening work (GH #851): later fixes must visibly shrink these allowlists. corpus_reparse_ratchet walks every .mdl fixture under test/ in-process (no CLI spawn, 200 KB cap skips only C-LEARN), and for each fixture whose source parses, asserts project_to_mdl output re-parses -- unless the path is in EXPECTED_REPARSE_FAILURES. The assertion runs both directions: an unlisted failure is a regression, and a listed entry that now re-parses is a stale allowlist entry the fix forgot to remove. This makes a padded allowlist structurally impossible while green. The 27 current entries are the wildcard-subscript ('*') and PI() collision classes. writer_output_idempotence_ratchet applies the same two-way contract to write(parse(write(parse(x)))) == write(parse(x)) over the re-parseable set. Its 83 entries document previously-uncatalogued writer non-idempotence: sketch UID/coordinate instability, arrayed element-order drift, CRLF free-text accretion, SIZE(x) spacing, unary-minus parenthesization, and control-variable value substitution. Both gates assert a MIN_CORPUS_FIXTURES floor so a reader/source-side regression that silently unparsed the corpus fails loudly rather than passing vacuously (the ratchets only make allowlisted breakage loud). The walk matches .mdl case-insensitively, which brought the uppercase teacup-upper.MDL fixture under test. xidz_zidz, lookups, and lookups_inline now survive the semantic MDL->MDL roundtrip and move into TEST_MDL_MODELS.
Introduces a WriterContext (the model's variable-ident set + a canonical ident -> declared-dimensions map) threaded through equation_to_mdl / expr0_to_mdl / MdlPrintVisitor, closing five MDL-writer defects whose common root cause was a context-free printer. Empties the corpus re-parse ratchet's allowlist (EXPECTED_REPARSE_FAILURES 27 -> 0). - #847 wildcard subscripts: a `*` at subscript position i now recovers the referenced variable's declared dimension at i and emits Vensim's bang form `Dim!` (was `*`, which the reader rejects). A `*:Dim` star-range emits `Dim!` from its own stored ident, preserving a subdimension name rather than widening to the parent. No declared dimension for the position falls back to a bare `*` (no panic). - #850 pi: a genuine `pi` builtin emits an f64-precision numeric literal (Vensim has no PI builtin and rejects `PI()`). - #853 reserved-name shadowing: at a zero-arg App, a declared model variable wins over the builtin-keyword rewrite, so a user aux named "Time Step" or "PI" stays its own identifier instead of silently rebinding to the simulation dt / the pi literal. - #852 INITIAL arity: `init` dispatches by argument count -- 1-arg -> INITIAL, 2-arg -> ACTIVE INITIAL (was unconditional ACTIVE INITIAL). 2-arg `normal(mean, sd)` synthesizes the 5-arg RANDOM NORMAL Vensim requires. safediv/ZIDZ/XIDZ was already arity-correct and is untouched. - #846 quoted-ident interiors: format_mdl_ident detects an already quoted ident up front and preserves it verbatim, so `"rate_of_change!"` keeps its underscores and `"a_b c"` keeps its quotes. expr0_to_mdl keeps a back-compat wrapper (default empty context) so existing callers are unchanged; production emission builds a real model-scoped context. The writer remains a pure function of the datamodel (the context maps are lookup-only, never iterated for output order). 20 formerly-unparseable fixtures now re-parse but are not fixpoints for pre-existing reasons (CRLF free-text accretion #849, sketch instability); they move into EXPECTED_NON_IDEMPOTENT. The importer's cross-process subscript-classification non-determinism surfaced by this work is tracked separately (#859); rendering both wildcard and star-range as `Dim!` keeps re-parse stable regardless.
Adds a single sanitize_free_text choke point in the MDL writer and routes every modeler-authored free-text sink through it: variable units/documentation, group name/doc banners, and 22: unit-equivalence tokens. Fixes GH #849 and lets the idempotence ratchet's allowlist shrink (EXPECTED_NON_IDEMPOTENT 103 -> 80); the re-parse allowlist stays empty. The reader unescapes every free-text field, so any character it reads as structure corrupts the file. The confirmed repro: a documentation string `cost is 5 | maybe 6 ~ per year` exported and re-parsed as FOUR variables (the `|` terminates the entry early and the remainder re-parses as phantom auxes) with no error. The policy, grounded in a read of reader.rs / lexer.rs / settings.rs and documented on the function: - line endings normalized losslessly to LF (the file-level LF->CRLF pass re-adds clean CRLF), which is what stops a comment carrying a `\r` from accumulating a carriage return every round trip -- the dominant idempotence-failure class; - `|` -> `/` (a non-whitespace substitute, so a field-final `|` cannot become a trim-sensitive trailing space and break idempotence); - the section-terminator runs and caller-specified field separators (`~` in units, `,` in a 22: token) -> a space; - substitute-and-preserve, never error (project_to_mdl keeps returning Result<String>; the richer warnings channel is a separate change). The lexer accepts both the canonical three-backslash section terminator and a two-backslash variant, and the units field is lexer-tokenized, so the pre-pass neutralizes all four runs -- three-backslash first, since a canonical run contains the short run as a substring. `~` is deliberately preserved in comments (the reader's :SUP flag heuristic is a feature). View titles keep their separate sanitize_view_title_for_mdl (sketch records use `|` non-structurally). Group-doc/name semantic loss on re-read is pre-existing and out of scope.
Fixes three silent-loss bugs in the MDL writer and adds the lossiness contract that lets callers learn an export was degraded. Warnings channel (#856): project_to_mdl_with_warnings returns (String, Vec<ExportWarning>) and is the primary entry point; project_to_mdl stays a thin wrapper that discards warnings so existing callers are unchanged. compat gains to_mdl_with_warnings. simlin-cli's `convert --to mdl` prints each warning to stderr (never stdout, which may be the output file). Warnings cover the known lossy cases: non_negative dropped, discrete/unrepresentable-extrapolate graphical functions, one-to-many element mappings collapsed, group name/doc truncated-on- reread, and an EXCEPT default that cannot be reconstructed. Exact- duplicate warnings are collapsed (first-seen order preserved). #857: write_stock_variable now honors stock.compat, so a stock whose INTEG initial is ACTIVE INITIAL(...) or GET DIRECT survives export instead of collapsing to the bare initial; wrap_active_initial is also applied in the Arrayed aux/flow/stock arm (was Scalar/ApplyToAll only). #854: a graphical function's Extrapolate kind is preserved by rewriting its LOOKUP call sites to TABXL(table, x) -- the same call-site marking the importer reads -- so an extrapolating table round-trips instead of being silently clamped to continuous. Discrete kinds and arrayed/embedded extrapolate that cannot be TABXL-marked emit a warning and clamp. #858: an Arrayed equation with has_except_default now materializes the declared-but-missing elements with the default equation as explicit entries (declared cartesian minus explicit, via project.dimensions), mirroring the engine's fill semantics, so elements covered only by the default keep their value instead of dropping to 0. This reopens the loss introduced by the #350 element-only fix. The reconstruction is a re-import fixpoint; it declines with a warning when dimension membership is unavailable or the default references the variable's own dimensions. Verified: except.mdl now emits s[A1]=14/s[A2]=14/s[A3]=13 (A1/A2 were dropped before) with identical simulation output, and re-parses. The corpus ratchets stay green (re-parse 0 failures; idempotence unchanged). The lossiness tests are split into writer_lossiness_tests.rs to keep writer_tests.rs under the per-file line cap (GH #645).
Closes the property-based-testing and documentation gaps of the MDL-writer hardening work. writer_proptest.rs adds four properties, each mounted in its own file to respect the per-file line cap: - doc free text never corrupts the variable set: an adversarial doc built from the structural characters and all four section-terminator runs must re-parse to exactly the original variable (guards #849, the confirmed |-in-doc phantom-variable corruption). - units free text is structurally safe: the same adversarial text in the typed units field never spawns or drops a variable (a hard unit-grammar parse failure is accepted, since units is an expression, not prose). - equation write is a re-parse fixpoint: an Expr0 grammar aimed at the printer fixes (wildcard subscript recovery, the pi literal, INITIAL vs ACTIVE INITIAL arity, precedence) must re-parse as MDL and reach a byte-equal second-vs-third render fixpoint. A prop_filter restricts the grammar to ASTs whose print_eqn output re-parses, excluding the separately-tracked print_eqn<->parser asymmetry (#863). - whole-model write is idempotent on the equations section. The properties surfaced two non-writer issues, both filed rather than worked around silently: a garbage-units hard-reimport gap (#862) and the stacked-unary print_eqn asymmetry (#863). Neither is a regression. mdl/CLAUDE.md gains a Writing section documenting the context-aware printer, the sanitization choke point, the warnings channel, TABXL preservation, and EXCEPT reconstruction, and a Known Gaps update (wildcard subscripts are no longer a gap; the remaining round-trip idempotence classes are enumerated).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
+ Coverage 90.88% 91.00% +0.12%
==========================================
Files 225 226 +1
Lines 145049 145803 +754
==========================================
+ Hits 131822 132683 +861
+ Misses 13227 13120 -107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No blocking correctness issues found on review. Verified the eight risk axes:
Overall correctness verdict: correct. The PR is well-scoped: the writer hardening is bounded (only 🤖 Generated with Claude Code |
![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: f1b0269b02
ℹ️ 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".
| match gf.kind { | ||
| Discrete => warnings.push(discrete_gf_warning(name)), | ||
| Extrapolate if !is_standalone => warnings.push(embedded_extrapolate_gf_warning(name)), | ||
| Continuous | Extrapolate => {} |
There was a problem hiding this comment.
When a standalone Extrapolate lookup has no simple LOOKUP(table, ...) call that the printer rewrites to TABXL—for example an unused table—the emitted MDL contains only the lookup definition, and the importer only restores GraphicalFunctionKind::Extrapolate from an explicit definition flag or a TABXL use. This branch suppresses the warning anyway, so the table silently re-imports as Continuous; please warn unless a preserving TABXL call was actually emitted, or preserve the kind another way.
Useful? React with 👍 / 👎.
An Extrapolate graphical function on a standalone lookup-only table is preserved on export by rewriting its LOOKUP call sites to TABXL (MDL has no definition-level extrapolate flag; the importer only infers the kind from a TABXL use). That preservation only happens when a call site exists. A standalone extrapolating table with no LOOKUP reference -- a dead or not-yet-wired table -- had no TABXL emitted, so its kind silently clamped to Continuous on re-import while the writer suppressed the warning on the assumption a preserving TABXL was written. That is a silent loss, contrary to the export lossiness contract (#856). WriterContext now scans the model's equations for the LOOKUP call sites the printer rewrites (only when the model has any extrapolating lookup, so the common no-lookup model pays nothing) and records which extrapolating tables are actually referenced. warn_unrepresentable_gf_kinds warns for a standalone extrapolating table absent from that set instead of suppressing it. A referenced table still round-trips silently (pinned by extrapolate_lookup_roundtrips_via_tabxl), so the change adds no false positives. Reported by Codex on #864.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a90b9a3fc
ℹ️ 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".
| // rebind to the simulation dt / lose its definition | ||
| // (#853, #850). | ||
| if self.ctx.is_variable(&crate::common::canonicalize(func)) { | ||
| return format_mdl_ident(func); |
There was a problem hiding this comment.
When the declared variable is one of the time-control names (time, time_step, initial_time, etc.), format_mdl_ident emits an unquoted bare identifier such as time step. On re-import the MDL converter treats those names case-insensitively as control variables (xmile_compat::format_name maps time step to DT, and convert::is_variable_equation skips TIME STEP definitions), so a model that actually has a user aux named time_step still loses/rebinds it despite this shadowing branch. Quote these reserved identifiers when they are variables.
Useful? React with 👍 / 👎.
|
Reviewed the writer changes end-to-end (sanitization, Overall correctness: correctA couple of very minor doc/edge-case notes that are not worth blocking on:
The corpus re-parse ratchet dropping to an empty allowlist plus the pinned property tests are a strong safety net for future changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The MDL writer (
project_to_mdl, the export path behindsimlin-cli convert --to mdl) was the subject of a pre-ship adversarial audit. That audit found two silent-corruption classes and several semantic-loss bugs: a context-free expression printer that emitted unparseable subscripts, raw free text that could terminate a construct early and invent phantom variables on re-import, and a handful of constructs (graphical-function kind, EXCEPT defaults, stock/arrayed initials) that were silently dropped. About 12% of the model corpus exported to files that would not re-parse.This PR hardens the writer to the point where the corpus re-parses cleanly and the known semantic-loss bugs are fixed, and it adds the test scaffolding (a two-way corpus ratchet plus property tests) that keeps those wins from regressing. The only production caller is the CLI converter, so these are export-quality fixes, not user-file-destruction fixes.
What, in five reviewed steps
Each step was implemented by a fresh sub-agent under strict TDD and then adversarially reviewed by a different fresh sub-agent; findings were folded in before commit.
1. Corpus round-trip ratchet gates (
tests/integration/mdl_roundtrip.rs). Two-way allowlists over every.mdlfixture:corpus_reparse_ratchet(writer output must re-parse) andwriter_output_idempotence_ratchet(write is a fixpoint). An unlisted failure is a regression; a listed-but-now-passing entry is also a failure, so the allowlists can only shrink. Coverage-floor guards prevent a vacuous pass. This landed first (green) so every subsequent step had an objective, non-regressing scoreboard.2. Context-aware expression printer. A
WriterContext(built per model) threads the variable-ident set, each arrayed variable's declared dimensions, the standalone-extrapolating-lookup idents, and each named dimension's element list into the printer. This resolves what a context-free printer cannot: wildcard subscript recoverya[*]->a[DimA!](#847), thepiliteral (#850), keyword-shadow resolution for user variables named like builtins (#853), INITIAL vs ACTIVE INITIAL arity at the call site (#852), and quoted-ident preservation (#846). Emptied the re-parse-failure allowlist from 27 to 0.3. Free-text sanitization choke point (
sanitize_free_text). Every modeler-authored free-text sink (units, documentation, group name/doc,22:tokens) routes through one function. Policy:|->/(a non-whitespace substitute so a field-final|cannot become an idempotence-breaking trailing space), the four section-terminator runs -> space, per-field separators -> space, line endings normalized losslessly to LF. Fixes the confirmed|-in-doc phantom-variable corruption (#849) and cut the idempotence allowlist from 103 to 80.4. Semantic-loss fixes + a lossiness contract.
project_to_mdl_with_warningsreturns(String, Vec<ExportWarning>)and is the new primary entry point;project_to_mdlstays a thin wrapper that discards warnings so existing callers are unchanged. The CLI prints warnings to stderr. Warnings never change the emitted text, so they do not affect the ratchets (#856). On top of the channel: stock and arrayedactive_initial/ GET DIRECT initials are preserved (#857); a graphical function's Extrapolate kind is preserved by rewriting itsLOOKUPcall sites toTABXL-- the same marking the importer reads (#854); and anEquation::Arrayedwith an EXCEPT default materializes the declared-but-missing elements with the default value instead of silently dropping them to 0 (#858, reopening the loss the #350 element-only fix introduced). The EXCEPT reconstruction was verified with a simulation-equivalence check (except.mdlnow yieldss[A1]=14/s[A2]=14/s[A3]=13, identical to the original run).5. Property tests + docs. Four properties (
writer_proptest.rs): free text never corrupts the variable set, the same text in the typed units field is structurally safe, equation write is a byte-equal re-parse fixpoint (a grammar aimed at the printer fixes), and whole-model write is idempotent. The review confirmed each is non-vacuous and non-tautological (a reverted fix trips the property).mdl/CLAUDE.mdgains a Writing section and an updated Known Gaps list.Decisions worth calling out
project_to_mdl -> Result<String>as a wrapper avoided churning ~40 test callsites while still giving the one production caller (the CLI) a way to surface degradation.TABXLcall. So the faithful inverse is to rewrite the call, which is what this does. An inlineWITH LOOKUPExtrapolate GF (no call site to mark) warns instead.:EXCEPT:syntax. The reader materializes EXCEPT coverage into explicit elements, so re-emitting the sugar is non-idempotent (the explicit set grows each write). Materializing the default-covered elements is both value-correct and a re-import fixpoint. It declines with a warning when dimension membership is unavailable or the default references its own dimensions.Follow-ups filed (not in this PR)
print_eqnemits--xfor stacked unary negatives, which the equation parser rejects (a shared AST/parser asymmetry, unreachable from real models).Closes
Fixes #846
Fixes #847
Fixes #849
Fixes #850
Fixes #852
Fixes #853
Fixes #854
Fixes #856
Fixes #857
Fixes #858
Substantially addresses #851 (the all-corpus re-parse gate, the idempotence ratchet, and the property tests all land here); the general simulation-equivalence oracle it also proposes remains open. #848's free-text non-idempotence class is fixed; its other non-idempotence classes (time-unit drift,
TIME BASErewriting, control-variable substitution) are enumerated in the writer's Known Gaps and remain in the idempotence allowlist.🤖 Generated with Claude Code