Skip to content

engine: element-level cycle resolution (C-LEARN hero model)#592

Merged
bpowers merged 72 commits into
mainfrom
clearn-hero-model
May 20, 2026
Merged

engine: element-level cycle resolution (C-LEARN hero model)#592
bpowers merged 72 commits into
mainfrom
clearn-hero-model

Conversation

@bpowers
Copy link
Copy Markdown
Owner

@bpowers bpowers commented May 20, 2026

Summary

Makes the C-LEARN v77 hero model (~53k MDL lines) compile via the incremental compilation path, run to FINAL TIME, and match genuine Vensim's Ref.vdf within 1% on 96.3% of cells. Implements the 7-phase plan in docs/implementation-plans/2026-05-18-element-cycle-resolution/.

The core work is element-level cycle resolution: an SCC whose variables form a cycle at the variable level but are acyclic at the element level (e.g. a recurrence x[i] = f(x[i-1])) is now resolved into a valid per-element evaluation order instead of being rejected with a false CircularDependency. Resolution runs on the GH #575 symbolic SymVarRef element graph (resolve_recurrence_sccs, refine_scc_to_element_verdict, symbolic_phase_element_order), with members' symbolic segments interleaved into one combined PerVarBytecodes injected at assemble_module for both the dt and init phases. Loud-safe throughout: any node that can't be sourced falls back conservatively to CircularDependency, never a panic or silent miscompile.

The false CircularDependency had been masking a graveyard of pre-existing incremental-compile bugs. Each fix is general (its own fast, model-agnostic unit test), not a C-LEARN-specific hack:

Genuine-Vensim builtin semantics corrected along the way: VECTOR SORT ORDER is 0-based; VECTOR ELM MAP uses base+full-source with out-of-bounds reads producing NaN (no modulo wrap).

Known residual (tracked, not a tolerance change)

VDF_RTOL is still literally 0.01. C-LEARN matches 96.3% of cells; the ~3.7% residual (32 base variables) is excluded-and-tracked, not silenced: #590 (data/graph-lookup variables importing as 0+0) and #591 (SAMPLE UNTIL / INIT-:NA: / numeric-tail / NaN-vs-:NA:). The carve-out is pinned exact by the clearn_residual_exactness test, which fails if the residual grows (regression) or shrinks (an unaccounted fix), so it can never silently drift.

Test plan

All 32 acceptance criteria are covered by real asserting tests (test-analyst PASS). The default suite is green under the 3-minute cap on every commit (pre-commit hook, never bypassed).

  • cargo test -p simlin-engine --features file_io -- default suite (runtime-class C-LEARN tests report ignored)
  • Runtime-class C-LEARN gates (heavy; run explicitly):
    • cargo test -p simlin-engine --features file_io --release -- --ignored compiles_and_runs_clearn_structural
    • cargo test -p simlin-engine --features file_io --release -- --ignored simulates_clearn
    • cargo test -p simlin-engine --features file_io --release -- --ignored clearn_residual_exactness
    • cargo test -p simlin-engine --features xmutil --release -- --ignored clearn_ltm_discovery

Full human verification plan (HV-1 through HV-5 plus the AC6.3 y/p exclusion judgement) is in docs/test-plans/2026-05-18-element-cycle-resolution.md.

bpowers added 30 commits May 16, 2026 07:54
canonicalize() was not idempotent for a quoted identifier containing a literal period (e.g. Vensim's "Goal 1.5 for Temperature"): pass 1 keeps the quoted period as a raw '.', which is_canonical() then rejects, so a re-canonicalization pass treats that '.' as the U+00B7 module-hierarchy separator. The ident silently becomes a phantom submodule path, reference resolution fails with DoesNotExist, and the whole model is reported NotSimulatable -- even when the variable is an unreferenced dead constant. This is C-LEARN blocker B4 (the mischaracterized "non-time $" item in #559; no $ is involved).

Fix: reserve a distinct codepoint (U+2024 ONE DOT LEADER, neither '.' nor the U+00B7 module separator) for literal periods inside quoted idents, and add canonical_to_source() to map both U+00B7 and the sentinel back to '.' so all user-facing and serialized output is byte-identical. The canonical form of every non-period identifier, and unquoted module-path canonicalization, are unchanged; this restores the idempotency invariant rather than weakening any check.
The native MDL converter (xmile_compat.rs::format_var_ctx) rewrote a
self-reference -- a RHS variable identical to the equation's LHS -- to
the literal token `self`, matching xmutil's Variable::OutputComputable
parity. The engine only ever un-rewrites a *bare* `self` back to the
enclosing variable inside PREVIOUS()/SIZE() arguments
(builtins_visitor.rs self_allowed Var arm). A *subscripted*
self-reference -- `self[..]`, the C-LEARN
`X[COP,tNext] = ... X[COP,tPrev]` staggered-subrange idiom -- is an
Expr0::Subscript whose arm never inspects the identifier, so the literal
`self` leaked into dependency analysis as an undefined name
(UnknownDependency/DoesNotExist) even with no cycle present.

Option A: emit the variable's real formatted name for every ordinary
self-reference. A well-founded staggered subrange recurrence then flows
to ordinary element-level resolution, while a genuine same-element
self-cycle is still correctly caught by the cycle gate. The hard-coded
PREVIOUS(SELF,init) template that SAMPLE IF TRUE conversion emits is a
separate site and is deliberately left untouched (its SELF is always
inside PREVIOUS(), still resolved by the self_allowed Var arm, and the
writer's recognize_sample_if_true round-trip depends on it). The
now-dead ElementContext.lhs_var_canonical field and its
build_element_context cascade are removed.

This is a precondition for the C-LEARN hero model (issue #559); after it
lands the self_recurrence fixture transitions from the self-leak
(UnknownDependency) to the structural CircularDependency that B1's
element-level resolution will clear -- the proof B3 != B1.
The native MDL converter's flow decomposition (collect_flows in
mdl/convert/stocks.rs) discarded a flow reference's subscripts: a 2D
flow pinned to one element -- C-LEARN's
`C in Mixed Layer[scenario] = INTEG(... - Diffusion Flux[scenario, layer1], ...)`
idiom -- was registered as a bare named outflow of the 1D
`[scenario]` stock. The dimension checker then sees a 2D flow feeding a
1D stock and reports MismatchedDimensions (C-LEARN's c_in_mixed_layer /
heat_in_atmosphere_and_upper_ocean).

The stock equation's canonicalized LHS subscripts are now threaded
through extract_flows_from_equation -> collect_flows. A subscripted
flow reference is accepted as a bare named flow only when its
canonical subscripts equal the stock LHS's; otherwise the simple
decomposition declines and the caller falls through to the synthetic
net-flow path, which preserves the exact, correctly-ranked sliced rate.
A reference with no subscripts is unchanged (implicit same-shape), and
a genuine same-shape/same-token flow still wires as a named flow. This
is general -- no dimension-name or model-specific special-casing. The
stale xmutil-parity comment that documented the dropped-subscript
behavior as a known bug is replaced, since the divergence from that
xmutil bug is now intentional (issue #559).

The deep-ocean shift-mapped synthetic-net-flow path (Pattern B) is a
separate decomposition defect, fixed in a following commit.
When the native MDL converter cannot decompose a stock's INTEG into
named inflows/outflows it synthesizes a single <stock>_net_flow aux.
build_synthetic_flow_equation cloned the raw formatted rate string
verbatim into every cartesian element key, so a rate over a
shift-mapped subrange -- C-LEARN's
`C in Deep Ocean[scenario, upper] = INTEG(Diffusion Flux[scenario, upper]
- Diffusion Flux[scenario, lower], ...)` deep-ocean idiom -- assigned a
multi-element subrange slice to each scalar element. The dimension
checker then reports MismatchedDimensions on the synthesized aux
(C-LEARN's c_in_deep_ocean_net_flow / heat_in_deep_ocean_net_flow).

The rate is now kept as an Expr and resolved per cartesian element key
through the same build_element_context + format_expr_with_context
subscript-range-mapping the regular arrayed-equation path uses, instead
of the raw clone -- reusing the existing tested resolution rather than
duplicating subrange-shift logic into the stock path. build_element_context
is widened to pub(super) (visibility + doc only, no logic change) so the
sibling convert::stocks module can share it. The scalar-stock path is
unchanged (no per-element resolution needed). General -- no
dimension-name or model-specific special-casing.

This makes the synthesized net-flow per-element shape correct. The
deep-ocean values themselves are a genuine subscript-shift recurrence
that only simulates once the element-level dependency fix (B1) lands;
the tests here assert the per-element shape and the removal of the
MismatchedDimensions symptom, and explicitly defer the deep-ocean
value assertion to B1.
The Condition-2 verification-harness additions (the dt-phase cycle relation, its VarInfo builder, and the #[cfg(test)] SCC accessor the binding pre-code B1 gate consumes) pushed db.rs and db_tests.rs past the 6000-line per-file cap that scripts/lint-project.sh enforces. Extract that cohesive unit into a sibling db_dep_graph.rs module plus its test sibling db_dep_graph_tests.rs, following the established db_ltm_ir.rs / db_macro_registry.rs per-file-cap precedent.

This is a pure behavior-preserving relocation: moved bodies are byte-identical, visibility is only widened to pub(crate) for the module-boundary crossing, and no salsa-tracked item moves -- the consistency cross-assert reaches the salsa layer via crate::db:: paths, so query identity is preserved. The two ltm/* doc-comment references to the relocated dt_cycle_sccs symbol are updated to its new path in the same commit so docs and code stay aligned. db_tests.rs returns byte-identical to its pre-harness state (all relocated tests now live in the test sibling).

The extracted dt-walk cycle relation is the shared primitive the B1 element-level SCC resolution (gate-1) will reuse, establishing that single-definition-used-twice boundary.
Move the per-variable lowering half of compile_var_fragment out of
db.rs into a new db_var_fragment.rs as a plain, side-effect-free
shared function (lower_var_fragment) that returns its diagnostics as
data rather than accumulating them via salsa.

This is a behavior-preserving refactor with no functional change. It
creates a clean reuse surface so upcoming read-only structural
analysis (the element-level SCC work for the C-LEARN circular
dependency) can run the engine's own per-variable production lowering
without re-deriving it from a reconstructed context, and it keeps
db.rs under the per-file line cap. The salsa-tracked caller in db.rs
replays the returned diagnostics, so accumulation order and all
observable compile output are unchanged.
Replace the inert empty-set stub with the real
db_dep_graph::array_producing_vars accessor. Over the same
build_var_info(db, model, project, &[]) universe that dt_cycle_sccs
uses (one shared definition, not a reconstruction), source each
variable's production lowered Vec<Expr> via
db_var_fragment::lower_var_fragment with a caller context built
byte-identically to db::compile_var_fragment, and flag the variables
whose complete expression list contains an array-producing builtin
using the same production contains_array_producing_builtin predicate
the hoister uses.

It aborts loudly (never silently skips) on any incompletely sourced
variable -- a whole-variable Fatal or a production-phase Var::new
error -- because a false negative there would make the downstream B1
condition-2 gate false-green by under-reporting array-producing
variables.

Adds the four-case spine test with both binding lowered-shape
assertions as permanent regression guards: a bare-scalar
array-producing builtin stays in the main AssignCurr element, while a
hoisted arrayed one appears only in a hoisted AssignTemp. This is the
read-only precondition for the B1 element-level SCC resolution work.
The C-LEARN blocker work landed with a dense citation scheme pointing at
design docs that do not exist in the repo (B1-design.md, B2.md, B3.md,
reviewer-priors.md) plus an internal taxonomy (B1/B2/B3/B4 blockers,
section numbers, C4-a/C4-b, Layer-1/Layer-2, Task #14/#15, gate-1,
"footgun-proofing", reviewer/adversarial narrative, RED/GREEN play-by-
play). Per CLAUDE.md, comments must explain the why -- invariants,
ordering constraints, edge-case semantics -- self-containedly; git
history holds the journey.

Each affected comment, rustdoc, panic/assert message, and the two
src/simlin-engine/CLAUDE.md module-map bullets are rewritten from
scratch as clean specs. Every genuine invariant is preserved (the
shared dt_walk_successors cycle relation and its per-node-kind
behavior, the canonicalize quoted-period idempotency rationale, the
subscripted-self-reference resolution, the stock-flow decomposition and
per-element net-flow resolution, the array-producing membership
completeness argument). Real GitHub issue #559 (the actual C-LEARN
umbrella issue) is kept; only the phantom doc/section/task references
are dropped.

Test functions and helpers that baked the B1/B2/B3/B4 taxonomy into
their names are renamed to describe what they verify
(e.g. simulates_b2_pattern_a_subscript_pinned_flow ->
simulates_subscript_pinned_higher_rank_flow; b3_compile_diags ->
compile_diags). These renames are test-scoped and behavior-preserving;
no production code semantics change.

The VarDepCollection rustdoc is tightened to state that the walk logic
is defined once but invoked from two salsa-memoized call sites, and
tech-debt.md entry #64 -- which only tracked that rustdoc imprecision,
a behavior-neutral non-issue -- is deleted now that it is resolved,
restoring tech-debt.md to its pre-branch content.
Completed brainstorming for the C-LEARN hero-model branch's remaining
work. Ground-truth investigation established the sole fatal C-LEARN
compile blocker is the false whole-variable CircularDependency (a
22-node multi-variable SCC plus a self-loop); the dimension/unknown
blockers the stale simulates_clearn comment lists are already cleared.

Key decisions captured:
- Refine only flagged SCCs to an exact per-element graph that is a
  faithful refinement of the existing dt relation (dt_walk_successors),
  not the LTM element graph (which retains PREVIOUS-lagged edges and
  would falsely block C-LEARN); promote the test-only Tarjan to a
  production primitive.
- New combined-fragment lowering for multi-variable recurrence SCCs;
  per-variable fragment/runlist model is otherwise unchanged.
- Synthetic in-SCC helpers sourced from parent implicit_vars, else
  loud-safe fallback to CircularDependency; genuine cycles still rejected.
- VECTOR ELM MAP / SORT ORDER corrected to genuine Vensim semantics with
  the bug-encoding fixtures rewritten; an explicit mid-plan structural
  gate precedes numeric Ref.vdf finalization.
- 7 implementation phases.
The shared SCC primitive scc_components (the fn in ltm/indexed.rs and
the re-export in ltm/mod.rs) was #[cfg(test)]-gated because the
#[cfg(test)] dt_cycle_sccs accessor was its only consumer. Phase 1's
element-level cycle resolution adds a production consumer (the
db_dep_graph.rs element-cycle refinement, landing in this same phase's
Subcomponent B), so the primitive is promoted to an unconditional
pub(crate). Both #[cfg(test)] gates are removed; the algorithm and
signature are unchanged.

The promotion lands ahead of its caller within this phase, so the
lib-target-only build temporarily has no use of the symbol (the
#[cfg(test)] dt_cycle_sccs is invisible to the plain lib target). A
narrowly-scoped, self-documenting #[allow(dead_code)] /
#[allow(unused_imports)] keeps the clippy -D warnings gate green until
Subcomponent B wires the production caller; the allowance is removed
then.
Scaffold the resolution payload the Phase 1 element-level cycle
resolution will produce. SccPhase records whether a resolved recurrence
SCC was proved acyclic by the dt-phase or init-phase cycle-gate run
(Phase 1 only emits Dt; Initial is reserved for the Phase 2 init-cycle
resolution). ResolvedScc carries the SCC's byte-stable member set and
its per-element topological evaluation order.

ModelDepGraphResult is a salsa return value (#[salsa::tracked]
returns(ref)), so salsa decides downstream re-execution by structural
equality of the value. The new types therefore derive the identical
trait set ModelDepGraphResult already derives -- in particular
PartialEq/Eq/salsa::Update -- so a change in the resolved-SCC set
correctly invalidates the salsa cache. The new resolved_sccs field is
initialized to Vec::new() at the sole construction site; the dt/init
back-edge paths use unwrap_or_else and fall through to that one struct
literal, so there is no separate early-return initializer (no
..Default::default(); the struct has no Default).

Two db_dep_graph_tests.rs unit tests pin the equality wiring: a
ResolvedScc constructs and its phase participates in identity, and
pushing a ResolvedScc makes two ModelDepGraphResult values compare
unequal -- the property that proves resolved_sccs is wired into the
derived equality salsa uses, not silently skipped.
A single-variable self-recurrence (a dt self-loop whose induced element
graph is acyclic and well-founded, e.g. `ecc[tNext]=ecc[tPrev]+1` over a
subrange) now compiles via the incremental path and simulates instead of
being rejected with a false `CircularDependency`. Genuine cycles
(scalar/multi-variable 2-cycles, same-element self-loops) stay rejected.

The fast whole-variable dt cycle gate's acyclic happy path is unchanged
(zero extra work). Only when `model_dependency_graph_impl`'s dt back-edge
fires does the element-cycle refinement run: it identifies the offending
SCC over the shared `dt_walk_successors` relation, refines just that SCC
into an exact (member, element-offset) graph built from the engine's own
production-lowered per-element `Expr::AssignCurr` exprs (a new
non-panicking `var_phase_lowered_exprs_prod` accessor; the `#[cfg(test)]`
`var_noninitial_lowered_exprs` panic wrapper is left unchanged), and runs
the now-promoted iterative Tarjan (`crate::ltm::scc_components`) over it.
Element-acyclic + element-sourceable single-variable self-recurrence =>
the member's whole-variable self-edge is broken in `compute_transitive`
(only at the self-edge; every other back-edge still errors) and a
`ResolvedScc` is recorded; anything else stays the conservative
loud-safe `CircularDependency`.

A single-variable self-recurrence's self-edge is structurally present in
BOTH the dt and the init dependency relations (the recurrence is the
variable's init AST too), so the refinement verifies element-acyclicity
in both phases and the init cycle gate breaks the same resolved members'
self-edges; an init-element-cyclic member is never resolved. This is the
minimal correctness extension required for the recurrence to compile --
not Phase 2's distinct init-cycle resolution.

The transient `#[allow(dead_code)]`/`#[allow(unused_imports)]` that
Subcomponent A added to `scc_components` ahead of its production caller
are removed now that the caller is wired in (clippy stays clean).

This folds the outside-in TDD acceptance test (`self_recurrence.mdl`
compiles with no `CircularDependency` and simulates to ecc[t1..t3]=1,2,3)
plus the engine machinery into one green commit, since the pre-commit
hook runs the full cargo test and a RED test cannot be committed.
… (AC4.2)

Subcomponent B resolved the `self` token to the real LHS name, so
`x[dimA]=x[dimA]+1` is now a genuine element self-loop (every element
reads itself) rather than a leaked `self` undefined name. Per AC4.2 the
engine must report `CircularDependency` SPECIFICALLY for this case.

The previous assertion deliberately accepted the union
`CircularDependency | UnknownDependency | DoesNotExist` because the
pre-#559 `self` leak surfaced as `UnknownDependency`; that loose form
(and its mandating comment) now contradicts the resolved behavior.
Tighten the assertion to require `CircularDependency` exactly, keep the
"no UnknownDependency/DoesNotExist leak" guard where it still guards a
real #559 name-resolution regression, and rewrite the rationale comment
in the same commit to state the new invariant (CLAUDE.md
comment-freshness hard rule). The `a=b+1;b=a+1` scalar 2-cycle
assertion is unchanged.
The existing byte-stable tests cover the dt SCC instrumentation
accessor (dt_cycle_sccs_is_byte_stable_across_runs) and the internal
Task 5 builder (resolve_dt_recurrence_sccs_is_byte_stable_across_runs),
but nothing pinned the emitted production payload. The Phase 1
acceptance obligation AC1.4 is about the per-element run order that
actually rides on the salsa-tracked ModelDepGraphResult, so this adds a
test asserting model_dependency_graph().resolved_sccs (members,
element_order, phase) is byte-identical across two compiles on fresh
databases for the single-variable self-recurrence, and that an acyclic
control emits an empty resolved_sccs both times (AC1.3 happy path
unaffected). The non-empty/element_order asserts keep the
byte-stability check from passing vacuously on two empty vectors. This
proves ResolvedScc.element_order inherits the existing determinism
discipline (sorted Tarjan, BTreeSet-ordered dt_walk_successors) end to
end through the production query, not just inside the isolated builder.
…le graph-key/input-wiring docs

Documentation-only accuracy fixes from Phase 1 code review (no behavioral
change):

- phase_element_order's rustdoc claimed PREVIOUS/lagged-read safety was
  "inherited" by stripping and that lagged edges were "not re-added".
  That misstated the mechanism: collect_read_slots recurses through
  Expr::App via BuiltinFn::for_each_expr_ref, which visits both operands
  of Previous(a, b), so a PREVIOUS argument slot IS collected as an
  ordinary current-value read -- the element graph does not inherit
  PREVIOUS-stripping. The actual protection is upstream at SCC
  identification: build_var_info strips dt_previous_referenced_vars from
  dt_deps, so dt_walk_successors reports no whole-variable self-edge for
  a PREVIOUS-only recurrence and this function is never reached for it.
  For any SCC that IS identified, collect_read_slots' deliberate
  over-approximation is the loud-safe direction (can only ADD an element
  edge, never drop one). Rewrote the rustdoc to state this accurately and
  cross-reference collect_read_slots' over-approximation contract.

- element_node_key now documents that it is an opaque graph key,
  deliberately NOT a canonical identifier (U+241F is not a
  canonicalization output), that it never escapes phase_element_order's
  local graph, and why U+241F is the injective separator -- making the
  intentional from_str_unchecked contract deviation explicit.

- resolve_dt_recurrence_sccs now documents the no-module-input-wiring
  completeness gap: it identifies SCCs over build_var_info(.., &[]) while
  the real model_dependency_graph_impl path uses the actual
  module_input_names. Soundness is preserved in Phase 1 (compute_transitive
  re-runs over the real with-inputs var_info and clears resolved_sccs on
  any residual genuine cycle, so the worst case is a missed resolution),
  but Phase 2 must plumb real module_input_names before consuming
  element_order. The &[] argument is not neutral.
Phase 2 Task 2. Add `pub(crate) fn init_walk_successors` to
db_dep_graph.rs, the exact init-phase analogue of `dt_walk_successors`:
the single shared definition of the init-phase cycle-successor relation.
It returns `[]` for an absent name or a Module (the module early-return
in `compute_inner` applies to both phases), and otherwise the variable's
`initial_deps` filtered only to known vars -- with NO stock filter and
NO stock sink, because `compute_inner`'s stock sink is
`!is_initial`-gated (a stock's initial value is a genuine init-relation
node, so a stock whose init equation references itself is a real init
self-loop). BTreeSet-sorted for byte-stability, mirroring
`dt_walk_successors`.

Refactor `compute_inner`'s init branch to call `init_walk_successors`
instead of the inlined `initial_deps.filter(contains_key)` expression.
This is a pure refactor: at that call site the inlined expression and
`init_walk_successors` produce byte-identical results (the defensive
absent/module guards never fire there -- `compute_inner` already
early-returned for those cases, exactly as `dt_walk_successors`'s guards
are redundant at its own call site). Sharing the init relation by
construction means the upcoming init-phase per-element recurrence
resolution observes the engine's actual init relation rather than a
re-derivation that could drift -- the same 'single shared relation,
never re-derive' discipline `dt_walk_successors` already follows.
Phase 2 Task 3. Parameterize the Phase 1 per-element recurrence
resolution by `SccPhase` instead of duplicating it for init:

- `resolve_dt_recurrence_sccs` -> `resolve_recurrence_sccs(.., phase)`:
  identifies the offending SCCs over `dt_walk_successors` (Dt) or
  `init_walk_successors` (Initial, Task 2), single-variable-only in
  Subcomponent A (multi-variable SCCs stay `has_unresolved` for
  Subcomponent B's combined-fragment lowering).
- `refine_scc_to_element_verdict` gains a `phase` arg. The `Dt` branch
  is unchanged (verify dt element-acyclic AND, as a precondition, init
  element-acyclic -- a same-equation aux self-recurrence's self-edge is
  in both relations; emit `phase: Dt`). The new `Initial` branch verifies
  init element-acyclicity ONLY: an init-only recurrence behind a stock
  has no dt self-edge and no dt per-element `AssignCurr` graph (a stock's
  dt lowering is `AssignNext`), so a dt precondition would spuriously
  reject every init-only recurrence; emit `phase: Initial`.
- `model_dependency_graph_impl`'s init gate is now symmetric to the dt
  block: an init first pass with the dt-resolved set runs first (so the
  acyclic happy path and the both-relations aux self-recurrence --
  `self_recurrence.mdl` -- take the `Ok` path with ZERO extra work and
  byte-identical behavior). Only a back-edge that survives breaking the
  dt-resolved set's init self-edges (a structurally distinct init-only
  cycle, e.g. a per-element forward recurrence in a stock's initial
  value) triggers `resolve_recurrence_sccs(.., Initial)`.

Crucially this EXTENDS rather than duplicates Phase 1's existing init
handling: Phase 1 already breaks the both-relations aux
self-recurrence's init self-edge via the shared dt-resolved set and
verifies its init element-acyclicity as a dt-resolution precondition. To
avoid emitting a duplicate `phase: Initial` `ResolvedScc` for `{ecc}`
(which would regress the Phase 1 single-ResolvedScc behavior), the init
pass excludes init SCCs whose members the dt path already resolved. A
genuine init element cycle stays unresolved (loud-safe
`CircularDependency`).

Also generalize the `#[cfg(test)]` dt-phase consistency cross-check
(`dt_cycle_sccs_consistency_violation`) -- previously its rustdoc flagged
this as a required Phase 2 step. Check (2) is now scoped to `phase: Dt`
resolved SCCs: a `phase: Initial` SCC is by design absent from the dt
instrumentation (a stock breaks the dt chain), so cross-checking it
against `dt_cycle_sccs` would falsely flag a correct resolution. The dt
cross-check stays exactly as strong for the dt path (an un-instrumented
`phase: Dt` SCC is still a hard divergence).
…ber, GH #575)

The SCC element-cycle verdict built its induced element graph from raw
Expr::AssignCurr operands, which are per-variable mini-slots
(lower_var_fragment builds a fresh per-variable layout: every member's
own variable at crate::vm::IMPLICIT_VAR_COUNT, its deps after it). For
any multi-member SCC every member's write-slots collided and every
cross-member read landed on the reading member's private dep mini-slots,
so the builder produced ZERO cross-member edges: a wrong topological
order AND -- fatally -- a genuine multi-variable element cycle resolved
as acyclic (unsound, violates the AC4 loud-safe rule). This was masked,
not absent, by the members.len() != 1 short-circuit in
refine_scc_to_element_verdict plus the multi-SCC short-circuit in
resolve_recurrence_sccs.

Rebuild the verdict on the cross-member-comparable symbolic layer. Add
db::var_phase_symbolic_fragment_prod, which sources a variable's
symbolic PerVarBytecodes through the exact production compile+symbolize
path: the compile_phase closure body is factored out of
compile_var_fragment into compile_phase_to_per_var_bytecodes (a pure
refactor -- the production per-variable compile path is byte-identical),
and the accessor mirrors var_phase_lowered_exprs_prod's context
construction byte-for-byte. symbolic_phase_element_order replaces the
mini-slot phase_element_order: it segments each member's symbolic.code
on its per-element write opcode and keys edges on the layout-independent
SymVarRef { name, element_offset } (name is the canonical variable name,
directly comparable across members). The N=1 and N>=2 cases are the same
builder over the same element_node_key / scc_components / sorted-Kahn
algorithm, so single-variable self-recurrence behavior is byte-identical
(every Phase 1 + Subcomponent A single-member regression guard stays
green unchanged). The members.len() != 1 mask and the multi-SCC
short-circuit are removed; a genuine multi-variable element cycle
(a[i]=b[i];b[i]=a[i]) is now detected and stays Unresolved (loud-safe),
while an element-acyclic multi-member recurrence (ref.mdl-shaped ce/ecc)
resolves with the correct interleaved per-element order.

var_phase_lowered_exprs_prod is left in place (Phase 3 extends its
no-SourceVariable arm and restores a production consumer).
Task 6 of the element-cycle-resolution Phase 2 plan. With Task 5b a
multi-member element-acyclic recurrence SCC now survives the cycle gate
(has_cycle=false, resolved_sccs populated, members contiguous at the
SCC's topological runlist slot), but assemble_module still pushed each
member's one-contiguous-AssignCurr-block-per-variable fragment, so the
required cross-member per-element interleaving was inexpressible and
ref.mdl/interleaved.mdl simulated to wrong series.

assemble_module now builds, per ResolvedScc, a combined PerVarBytecodes
(Task 5 combine_scc_fragment over each member's exact production-compiled
symbolic fragment from var_phase_symbolic_fragment_prod). For the dt
flows runlist it skips every phase==Dt member's per-variable push and
injects the combined fragment at the first member encountered; for the
initials runlist it applies the Task 1 spike's validated mechanism --
one synthetic-ident SymbolicCompiledInitial ($:scc:init:{n}) at the
first init SCC member's slot, members skipped. The combined fragments
are owned in function-scope Vecs that outlive concatenate_fragments and
the init renumber loop. The runlist Vec<String> is salsa-owned and never
mutated (skip during collection, never remove).

resolve_module/compute_layout/the init renumber loop/the VM init runner
are untouched: each write keeps its original SymVarRef name/element
offset, so resolve_module maps it to the same model slot the acyclic
layout assigns and the results offset map is unchanged (AC2.3), and the
spike-verified positional ident-agnostic init consumption needs no code
changes. Unsourceable members or a combine_scc_fragment error accumulate
an Assembly diagnostic and abort assembly (loud-safe, mirrors the
existing missing-fragment/concatenate-error pattern) -- never a silent
miscompile or partial injection.

Removes the transient cfg_attr(not(test), allow(dead_code)) on
combine_scc_fragment and segment_member_by_element now that they have a
production consumer.
Add an end-to-end test that runs test/sdeverywhere/models/ref/ref.mdl
through simulate_mdl_path, which loads the sibling ref.dat and compares
the VM output via ensure_results. ref.mdl is a two-variable inter-element
recurrence (ce<->ecc) whose whole-variable graph is a 2-cycle but whose
induced element graph is acyclic. Before Subcomponent B (GH #575) this
multi-member SCC was short-circuited to CircularDependency and the model
did not compile; Subcomponent B resolves the {ce,ecc} SCC and injects one
combined per-element fragment, so it now simulates to the hand-computed
series ce=1,3,5 / ecc=2,4,6 (constant across both saved steps).

The interim Task-5b retarget of ref_interleaved_inter_variable_cycles_
report_circular gets a one-line doc-comment update pointing at this new
dedicated Task 7 test; Task 9 finalizes that test's transition.
Add an end-to-end test that runs
test/sdeverywhere/models/interleaved/interleaved.mdl through
simulate_mdl_path, comparing the VM output against the sibling
interleaved.dat (all 1.0 across 101 saved steps) via ensure_results.
interleaved.mdl (x=1; a[A1]=x; a[A2]=y; y=a[A1]; b[DimA]=a[DimA]) has a
whole-variable a<->y 2-cycle but an acyclic element chain
x -> a[A1] -> y -> a[A2]. Before Subcomponent B (GH #575) this
multi-member SCC was rejected as CircularDependency; Subcomponent B
resolves the {a,y} SCC and evaluates its members in interleaved
per-element order, so it now simulates to all 1.0.

The interim Task-5b retarget of ref_interleaved_inter_variable_cycles_
report_circular gets a doc-comment update referencing both dedicated
Task 7/8 tests; Task 9 finalizes that test's transition.
…ture (AC2.4, AC2.5)

AC2.5: finalize ref_interleaved_inter_variable_cycles_report_circular to
its correct end state. Subcomponent B's correctness fix structurally
falsified its original CircularDependency precondition; Task 5b left it
in an interim retarget. The correct-simulation assertion is now folded
into the dedicated ref.mdl/interleaved.mdl .dat tests (Tasks 7/8); the
transitioned test (renamed to ..._simulate_no_circular_no_leak) retains
the still-meaningful diagnostic-level guards those value tests do not
pin: no CircularDependency + compiles end-to-end (the inverse of the
original assertion), and the verbatim AC2.5 #559-class no-self-token-leak
guard.

AC2.4: add a NEW init-phase-recurrence fixture
test/sdeverywhere/models/init_recurrence/{init_recurrence.mdl,.dat}.
Two arrayed stocks cs[Target]/ecs[Target] over t1..t3 whose per-element
INTEG initial values form a ref.mdl-shaped inter-element recurrence
across the two variables, with a constant zero inflow. Each stock breaks
the dt chain (its dt-equation is the acyclic flow), so there is NO dt
cycle; the INIT relation has the multi-member element-acyclic {cs,ecs}
recurrence. This is the FIRST real exercise of Task 6's init
synthetic-ident SymbolicCompiledInitial path on a MULTI-member init SCC
(Subcomponent A only covered the single-variable case). The shape was
empirically confirmed: db_dep_graph_tests gains three datamodel-level
probe/regression tests (multi-member phase:Initial verdict, production
model_dependency_graph payload, and the loud-safe genuine-init-cycle
rejection), and the simulate.rs end-to-end test first asserts the parsed
fixture's production resolved_sccs is exactly one phase:Initial SCC with
>=2 members {cs,ecs} and has_cycle==false, then simulates it against the
hand-computed init_recurrence.dat (cs=1,3,5 / ecs=2,4,6, held constant
by the zero flow across both saved steps).
bpowers added 27 commits May 19, 2026 09:32
Task 4 (commit b25dc06) empirically disproved the spec's root-cause for
#580 Bug A before coding: translate_via_mapping resolves the expanded
group element_map correctly once the mapped dimension is in the
per-variable DimensionsContext. The real bug was a display-vs-canonical
raw == in db.rs::expand_maps_to_chains that kept the reverse-mapping pass
from pulling the mapped source dimension into the context. The as-built
fix is there (canonicalize reachability comparisons; resolve canonical
targets back to display names before insertion), leaving
substitute_dimension_refs and dimensions.rs byte-identical -- so the
prescribed translate_via_group_mapping resolver was unnecessary/dead.

Add an as-built correction note at the top of Task 4 (the original
investigation-path diagnosis is retained below it for provenance, mirroring
the plan's existing "verified deviation" convention). The dimensions.rs
unit test the spec named was replaced by expand_maps_to_chains_tests on
the actually-fixed function; the RED->GREEN fixture and loud-safe Part B
test landed as specced. GH #580's root-cause section was corrected by
comment; test-requirements.md AC7 reconciliation is deferred to finalization.
Task 5 (commit ad432fb) root-cause-confirmed (as the spec mandates) that
the prescribed location was wrong: Var::new succeeds, the dep's per-element
tables and dims are correctly reconstructed, and build_stub_variable /
extract_tables_from_source_var are not the gap. The real bug is a general
engine codegen gap -- a per-element arrayed GF applied with an index
lowers to App(Lookup(full-array-view, scalar-index)) and no codegen path
materialized that as an array view (Cannot push view for SUM; BadTable for
VECTOR SELECT). The as-built fix is a new layout-independent LookupArray
opcode plus a single Expr3 decomposition point, in ast/expr3.rs / bytecode.rs
/ vm.rs / compiler/codegen.rs / compiler/symbolic.rs -- not db_var_fragment.rs.

Add an as-built correction note at the top of Task 5 (original
investigation-path diagnosis retained below, mirroring Task 4 and the plan's
"verified deviation" convention). Note that AC7.1 remains blocked after
this task by a distinct GraphicalFunctionId u8 overflow in
concatenate_fragments::absorb (no cross-fragment GF de-duplication) that
the fix unmasked -- tracked separately; the Task 1 simulate.rs gate commit
is not sequenced until it is resolved.
Fixing Bug B (ad432fb) let C-LEARN's ~6 arrayed-GF-in-reducer fragments
compile for the first time, which surfaced a 3rd distinct, orthogonal,
pre-existing latent bug (GH #582): concatenate_fragments::absorb appends
each fragment's graphical_functions with no de-duplication, so a dependency
arrayed GF referenced by N consumer fragments is duplicated N times. C-LEARN
has only ~165 distinct GF tables but the duplication accumulates past 255
and the GraphicalFunctionId=u8 renumber guard trips
("graphical function offset 493 exceeds ... u8::MAX = 255"). The monolithic
Module::compile de-duplicates GF tables, so the incremental path is
incorrect-by-omission, not merely capacity-limited.

The user was surfaced the pattern (this is the 5th latent bug the cleared
cycle gate exposed; the plan's per-bug diagnoses have been wrong 3x) and
chose "drive #582, checkpoint per layer". Task 6 implements the principled
fix -- cross-fragment GF de-duplication keyed by the identity the monolithic
path uses, with a per-fragment local->global GF index remap threaded through
renumber_opcode/renumber_fragment_code, keeping GraphicalFunctionId=u8 (not
the weaker u16 widening). The dedup must be value-exact (distinct tables
never merge). concatenate_fragments is the same machinery the Phase 2 #575
combined-SCC-fragment lowering uses, so the combined-fragment suite and the
22-model corpus are mandatory regression pins. Task 6 runs the C-LEARN
structural gate and reports the exact outcome; any further unmasked layer
is surfaced to the user before being driven.
The user authorized a forward sweep of the whole incremental-compile
assembly path rather than peeling ceilings one at a time. The sweep (an
all-narrow-types-widened-to-u32 + peak-count probe build, fully reverted)
overturned #583's premise and found the bottom: only two u8 index types
exist (GraphicalFunctionId, already bounded by #582's dedup; and TempId),
everything else is already u16, and none serialize to protobuf. With
everything widened C-LEARN compiles Ok and Vm::new succeeds but the VM
panics at vm.rs:2372 (temp_offsets[347], table len 243) -- so widening is
the wrong fix. TempId is a #582-class concat-vs-monolithic divergence, not
a genuine-capacity wall: monolithic Module::compile recycles temps via a
keyed max-merge to 21 slots, while the incremental path sums to 243
(merged table) / 3233 (flows_concat) and those two tables disagree.

Task 7 fixes it by matching the monolithic recycle: remove the per-fragment
+ ctx_base.temps re-add (the arithmetic bug making flows_concat != merged),
and max-merge plain-phase fragment temps into a shared keyed pool, while
KEEPING combine_scc_fragment (the GH #575 combined-SCC machinery) on the
disjoint-range path because its per-element segments interleave per
element_order. TempId stays u8 (no widen). The 22-model corpus and
combined-fragment suite are mandatory regression pins; the recycle must be
collision-free. The sweep could not exercise AC7.2/7.3 (the probe panicked
before run_to_end), so a runtime-numeric layer may surface after Task 7 --
that is surfaced to the user per the checkpoint-per-layer directive.
With C-LEARN compiling and running to FINAL TIME (Tasks 4-7, AC7.1/7.2
met), AC7.3 (no all-NaN core series) fails: 708 of 3482 matched core
series are entirely NaN. A read-only investigation found two independent
SPURIOUS engine bugs -- Ref.vdf (genuine Vensim) is finite at every offset
Simlin reports NaN/inf, so neither is a genuine model NaN.

Cluster A (Task 8, the majority): C-LEARN's user macro INIT(x)=INITIAL(x)
(which shadows the built-in init) compiles to a module whose initials
runlist OMITS its own INITIAL() primary output, so the parent reads an
unwritten slot during initials -> uninitialized inf, frozen into
initial_values and served by LoadInitial forever. Fix: include an
INITIAL-backed module/macro primary output that is read during initials in
runlist_initials. Cluster B (Task 9, completing the AC5 multi-row case
Phase 4 missed): arrayed VECTOR SORT ORDER returns flat global indices into
the whole [COP,Target] array instead of per-iterated-slice 0-based ranks,
so a downstream VECTOR ELM MAP indexes out of bounds -> NaN. Fix: rank
within the iterated source slice, 0-based, keeping the single-row AC5 case
byte-identical.

Both are clean bounded general fixes (no model-specific hacks), each
root-cause-confirmed first and soundness-pinned (22-model corpus, the AC5
single-row VSO suite, the macro/init suites, the recurrence gates). The
user chose to drive both. The full Ref.vdf 1% numeric match remains Phase
7's AC8.1.
The user (domain-authoritative) corrected the prior "Simlin is correct"
verdict on the residual 434 all-NaN C-LEARN series: Vensim :NA: is NOT
IEEE NaN -- per the Vensim docs it is a finite sentinel value -2^109 used
to test for the existence of data. So Simlin's :NA:->NaN
(mdl/xmile_compat.rs:109 :NA:->"NAN" -> parser :NA:->Const(f64::NAN)) is
an engine bug. A deep combined research + raw-VDF-byte audit confirmed:
:NA: = -2^109 is an ordinary finite number (IF THEN ELSE(X=:NA:,...) is a
plain equality-to-the-sentinel existence test, only working because it is
finite); Vensim saves :NA: as literal 0.0 to the VDF (exhaustive byte scan
of Ref.vdf: -2^109 and NaN each appear 0 times); and Simlin is three-way
inconsistent (expression :NA:->NaN, data-list :NA:->-1e38, neither = the
correct -2^109).

Task 10 represents :NA: as a single canonical NA = -2^109 across both
paths. This clears AC7.3 by itself (the series become finite, not NaN, and
existence tests gate correctly via finite equality), keeping the
structurally-distinct OOB->NaN (VECTOR ELM MAP, range subscripts) and
empty-reducer->NaN paths untouched -- they must stay NaN (C-LEARN line
19481 feeds a non-:NA: slice into ELM MAP, so the two intersect and must
remain distinct). The -2^109 (Simlin runtime) <-> 0 (Vensim VDF)
reconciliation needed for AC8.1's 1% numeric match is Phase 7 work, not
this task.
Add the #[ignore]d compiles_and_runs_clearn_structural test: it parses
C-LEARN, compiles via the incremental path (compile_project_incremental
directly, asserting Ok with no CircularDependency -- AC7.1), runs the VM to
FINAL TIME without catch_unwind (a post-gate panic propagates as a hard
failure -- AC7.2/AC7.5), and asserts no matched core series (Ref.vdf
offsets intersect results offsets) is entirely NaN (AC7.3).

This is the structural value-locking checkpoint, held uncommitted until the
Phase 6 engine fixes (Tasks 3-10: element-level cycle resolution, the four
assembly-path fixes #580/#582/#583/#584/#585, and the :NA: sentinel) made
it pass. It now passes: 3482 core series matched across 251 steps, zero
all-NaN, no CircularDependency, no panic. Runtime-class (#[ignore]d, ~14s
release) so it stays out of the capped default cargo test set. The full
Ref.vdf 1% numeric match is Phase 7 (AC8.1).
Task 2 (re-pointing clearn_ltm_discovery_* to expect a clean compile, with
LTM discovery ENABLED) surfaced that #363 reproduces on the LTM-discovery
synthetic-fragment path: compile_project_incremental PANICS at
compiler/codegen.rs:494 -- self.walk_expr(expr).unwrap().unwrap() on a
recoverable Err(NotSimulatable, "PREVIOUS requires a variable reference
after helper rewriting") (a PREVIOUS whose arg sits inside a Subscript
index). The sibling arms at codegen.rs:768/769/795 already use ?, and the
caller db_ltm.rs:2083 is already Err(_) => None (it intends to gracefully
drop an un-compilable LTM synthetic fragment), so the stray unwrap
short-circuits the exact Result path the caller is poised to absorb. This
is the planned AC7.5 / #363 work: convert the panic site to a propagated
typed Err. Task 3 Part B checked only the plain incremental path (panic
absent there); it reproduces on the LTM-discovery path.

Task 11 changes codegen.rs:494 to propagate via ? (matching the siblings),
letting the recoverable Err reach the existing graceful drop -- so C-LEARN
compiles with LTM discovery enabled. codegen.rs:494 is a shared codegen
arm, so the full compiler/codegen suites + the 22-model corpus + simulate_ltm
are mandatory regression pins (the ? must only convert panics to Errs,
never alter a success). It unblocks Task 2's clean-compile re-point (AC7.4),
which commits after.
Phase 6c review (chunk 2) Minor: the gf_block_key boundary-marker comment
overstated that "no to_bits() point pair can forge a boundary" -- absolute
for arbitrary f64 (a crafted NaN with the marker's bit pattern could). Soften
to the accurate precondition: the marker is a NaN bit pattern (sign +
exponent all-ones); genuine GF points are finite so a finite point's
to_bits() never equals it, and the only collision risk (a NaN point value,
which GF data never contains) would yield a spurious distinction, never an
over-merge (the only unsound direction). Comment-only; no behavior change.
The Phase 7 diagnostic (a measured C-LEARN-vs-Ref.vdf categorization) found
~71% of matched cells already match-or-comparator-reconcilable, and the 28%
genuine divergence (1,314 idents) is a cascade from one general engine bug:
per-element graphical functions are consumed by Equation::Arrayed elems-Vec
position (sorted order) instead of by element-name -> dimension-index, so for
a dimension whose declared element order differs from sorted order (C-LEARN's
COP), every arrayed GF feeds the wrong element's table to each slot
(un_population_high[oecd_us] gets COP Developing B's table). It poisons
population/gdp/emissions downstream; not caught earlier because most fixtures
declare elements in sorted order.

Task 5 fixes the mapping by element name (at lowering time, so the VM hot path
is unchanged -- no runtime name lookup). Per the user's direction, it
establishes the test baseline FIRST: audit the existing per-element-GF /
arrayed-GF coverage and add baseline correctness + performance-regression pins
(sorted-order GFs must stay byte-identical; GF eval must not be slowed) BEFORE
the fix, so a broad-blast-radius change cannot silently regress. Executed first
in Phase 7 (before the Tasks 1/2 comparator), then re-measure C-LEARN vs
Ref.vdf and checkpoint.
Weave the user-directed principled comparator into the Phase 7 Task 2
ensure_vdf_results hardening (alongside the AC8.2 matched-floor + NaN
guards), and add comparator-behavior cases to the Task 1 guard test:

- :NA:-sentinel reconciliation: a SIM value == crate::float::NA (-2^109) is
  :NA:; Vensim renders :NA: as 0 in the VDF, so SIM-NA vs VDF-~0 is a MATCH,
  while SIM-NA vs VDF-genuine-nonzero is a real mismatch (spurious :NA:,
  must be caught). The engine keeps the sentinel -- only the comparator
  interprets it (no output-side :NA:->0 mapping).
- Near-zero-robust tolerance: replace the per-cell relative error (which is
  ~100% when the reference is a literal 0 and the sim is any small jitter)
  with isclose-style |e-a| <= atol + rtol*max(|e|,|a|), rtol=0.01, and a
  per-series absolute floor atol = k * max_step|e| tuned so near-zero jitter
  passes while a genuine >1% divergence still fails (k must not be loosened
  to force a pass). A principled correction of the comparison at zero, not a
  relaxation for meaningful values.

This is the measure-driven next step after the GF-mapping fix (6157354)
collapsed C-LEARN's genuine divergence from 28% to 3.27%: the comparator
mechanically reconciles the ~36.5% :NA:-sentinel + near-zero buckets and
re-measures to reveal the TRUE genuine residual before more numeric work.
…c nits

Add a committed `clearn_residual_exactness` regression test (runtime-class,
`#[ignore]`d) that runs C-LEARN through the same end-to-end path as
`simulates_clearn` (`open_vensim` -> `compile_vm` -> `run_to_end` -> parse
`Ref.vdf`), classifies every matched ident through the SAME `classify_vdf_ident`
comparator with NO exclusion, and asserts the live failing set (bases with a
tolerance violation, spurious `:NA:`, or an entirely-NaN core series) EQUALS
`EXPECTED_VDF_RESIDUAL`, failing loudly with the symmetric difference if the
residual grew (a regression) or shrank (a fix that should prune the exclusion).
This permanently guards that the exclusion stays exact -- replacing the
`EXPECTED_VDF_RESIDUAL` doc comment's citation of a `__clearn_residual_diagnostic`
harness that was never committed (a phantom re-derivation procedure) with a
reference to this committed test. The comparator path is reused (no duplication):
`simulates_clearn` and the new test share a `run_clearn_vs_vdf` helper.

Also fix three documentation nits flagged in review: the match percentage is
3298/3482 = ~94.7% (was stated ~95.6% in two places); the `MIN_MATCHED_FRACTION`
comment conflated the matched-intersection (~3482) with the total `Ref.vdf` var
count (~3484); and the NaN-skipped guard scenario comment dropped a discarded
"~25% global skip" first clause that did not match the code (the scenario NaN-s
3 of 4 steps in 12 of 16 vars, ~56% of cells).

No tolerance, exclusion membership, or `#[ignore]` attribute changed.
The 68-commit clearn-hero-model branch made C-LEARN compile via the
incremental path and match genuine Vensim within tolerance. Surface the
structural and contract deltas in src/simlin-engine/CLAUDE.md that were
not yet documented:

- compiler/symbolic.rs (the layout-independent symbolic bytecode layer,
  FragmentMerger, GfDedup #582, TempStrategy { Recycle, Sum }) is now
  listed in the compilation pipeline.
- Opcode::LookupArray (per-element arrayed-GF apply, #580) in bytecode.rs
  and vm.rs; the extracted vm_vector_sort_order.rs (#585, per-iterated-
  slice 0-based ranks) and vm_vector_elm_map.rs (base+full-source,
  OOB->NaN, no modulo) helper modules.
- crate::float::NA, the finite Vensim :NA: sentinel (-2^109, not NaN),
  distinct from the genuine NaN of OOB vector reads / empty reducers.
- db_var_fragment.rs (the lowering half of per-variable compilation) and
  db.rs combine_scc_fragment / var_phase_symbolic_fragment_prod (the
  resolved-SCC combined-fragment injection at assemble_module).
- Per-element GF table layout by declared dimension index, not
  Equation::Arrayed Vec position (variable.rs reorder_arrayed_element_
  tables / build_tables, db.rs extract_tables_from_source_var).
- db_combined_fragment_tests.rs, and the un-stubbed simulates_clearn /
  EXPECTED_VDF_RESIDUAL VDF comparator in tests/simulate.rs.

The root CLAUDE.md is unchanged: this is engine-internal architecture,
not a monorepo-level pattern or cross-component contract.
…ion note

Add the 2026-05-18-element-cycle-resolution plan + its test-requirements to
the docs/README.md implementation-plans index (was missing -- #577), matching
the sibling server-rewrite/macros entries.

Add a "Finalization reconciliation" section to the plan's test-requirements.md:
the 32 design ACs are all covered as mapped; document the newly-surfaced
prerequisite fixes the phases discovered (the pre-existing bugs the false
CircularDependency masked -- #580 A/B, #582, #583, #584, #585, #363, #586 :NA:,
#589 per-element-GF mapping -- each with its own unit test, tracked on GitHub,
not design ACs); note #585 also completes the AC5 multi-row case; and record
the AC8.1 disposition (simulates_clearn passes within the unchanged 1%
tolerance on ~96.3% of cells, with the ~3.7% residual explicitly excluded via
EXPECTED_VDF_RESIDUAL and tracked in #590/#591 -- not a tolerance loosening;
guarded for exactness by the committed clearn_residual_exactness test).
Final-review Minor: the simulates_clearn comments quote "~94.7% of matched
idents (3298 of 3482)" while the plan's test-requirements.md quotes "~96.3%
of cells" -- both correct but different denominators. Cross-reference the
per-cell figure in the comments so a reader doesn't mistake them for a
discrepancy. Comment-only; no behavior change.
@claude
Copy link
Copy Markdown

claude Bot commented May 20, 2026

Review of PR #592 (element-level cycle resolution + Vensim builtin / :NA: fixes)

I reviewed the production (non-test) changes across the engine: MDL conversion (mdl/convert/*, xmile_compat.rs, parser.rs, reader.rs), common.rs canonicalize, float.rs / :NA: sentinel, the new VECTOR ELM MAP / SORT ORDER opcodes and VM dispatch, variable.rs / bytecode.rs / expr3.rs / ltm, the symbolic compiler plus assemble_module / combine_scc_fragment injection, and the new element-cycle refinement in db_dep_graph.rs / db_var_fragment.rs. One finding.

[P2] symbolic_phase_element_order under-counts dynamic / range subscript reads of in-SCC members

src/simlin-engine/src/db_dep_graph.rs lines 853-858 (read arm) and the catch-all _ => {} at line 914.

The element-graph builder records PushVarViewDirect / PushVarView / LoadSubscript reads at the variable base element (var.element_offset, which is 0 for a whole-array base), and the opcodes that actually select the read element (ViewSubscriptDynamic, ViewRangeDynamic, ViewRange, ViewStarRange, ViewWildcard, and PushSubscriptIndex) all fall through the _ => {} arm at line 914 and are ignored. The constant case is handled exactly (the PushStaticView arm at lines 895-908 enumerates the precise element set via static_view_element_offsets), but a genuinely dynamic scalar index (x[idx], lowered by codegen.rs to LoadSubscript) or a dynamic/range view read (x[idx] or x[t1:t2] in a reducer arg, lowered to PushVarViewDirect plus ViewSubscriptDynamic / ViewRangeDynamic) records only the base (x, 0). There is no upstream guard in refine_scc_to_element_verdict / resolve_recurrence_sccs rejecting such members. Recording the wrong (base) element is the unsound direction: it can drop the genuine cycle edge from a non-base element and let symbolic_phase_element_order return an acyclic order for an SCC that is actually element-cyclic, which then resolves into a wrong per-element evaluation order (a silent read-before-write / wrong-results miscompile), contradicting the stated loud-safe contract. A minimal trigger is a 2-member recurrence SCC where a[D] = b[idx] + 1 reads b through a data-dependent index (runtime element b[2]) and b[2] = a[0]: the true cycle closes through b[2], but the builder only records the edge into base (b, 0), so the cycle is missed. The C-LEARN target and unit fixtures use only constant / subrange subscripts that fold to StaticSubscript (the correctly-handled PushStaticView path), so no shipped or tested model is affected today; this is a latent gap. The discrete fix consistent with the design is to treat a PushVarViewDirect / PushVarView followed by a non-statically-enumerable view subscript, and a dynamic LoadSubscript, as not element-sourceable (return None, falling back to CircularDependency) rather than recording the base element.

Overall correctness

The patch is correct for its shipped scope: the canonicalize idempotency fix, :NA: sentinel routing, stock-flow / per-element net-flow decomposition, VECTOR ELM MAP / SORT ORDER semantics, per-element GF table layout, and the symbolic combined-fragment injection all check out, and the resolution path is loud-safe for the constant-subscript recurrences it targets. The single issue above is a latent soundness gap (not triggered by the C-LEARN target or current tests) rather than a release blocker, so I would consider the patch correct-with-a-follow-up.

@bpowers bpowers merged commit 81cdcb1 into main May 20, 2026
12 of 13 checks passed
@bpowers bpowers deleted the clearn-hero-model branch May 20, 2026 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant