Phase 2.6: MOOSE state-variable wiring + canonical_arguments

[petlenz]

Summary

Phase 2.6 (issue #77) — wire state variables through the MOOSE backend so an evolution recipe emits a compilable, correct MOOSE Material. The backend was provably broken for such recipes (3-arg compute call into a 7-param function; <sv>_old never declared); this fixes it and flips the KNOWN_GAP guard test to positive checks.

The root cause + the fix

The MOOSE call-site rebuilt the argument list (parameters()→inputs()→outputs()) independently of the generated signature (symbols()→outputs()→newton-out), so it silently omitted state variables. Rather than patch the MOOSE order to match by hand (re-introducing the drift), this PR introduces canonical_arguments(model) — one ordered ArgSpec list that BOTH render_compute_function (signature) and every backend call-site iterate. One source of truth; the two can't diverge again. (Done as a verified pure refactor first — Step A, generated code byte-identical, all tests green — then the MOOSE wiring built on top.)

MOOSE wiring (Step B)

• Members: StateVariableCurrent → MaterialProperty<Real> & _<sv> (declareProperty), StateVariableOld → const MaterialProperty<Real> & _<sv>_old (getMaterialPropertyOld of the SAME property name — MOOSE versions it automatically).
• dt → MOOSE's framework _dt — excluded from validParams + the constructor (it's not an input-file param), passed as _dt in the compute call. The old code wrongly emitted getParam<Real>("dt").
• initQpStatefulProperties seeds each state variable to its initial value (emitted through the scalar pipeline, so non-constant initials work too).
• computeQpProperties call built by iterating canonical_arguments → exact arity match.

Example (Newton hardening) — the emitted .C now compiles:

Hardening::Hardening(const InputParameters & parameters)
  : Material(parameters),
    _K(getParam<Real>("K")),
    _alpha(declareProperty<Real>("Hardening_alpha")),
    _alpha_old(getMaterialPropertyOld<Real>("Hardening_alpha")),
    _sigma_y(declareProperty<Real>("Hardening_sigma_y")) {}

void Hardening::initQpStatefulProperties() { _alpha[_qp] = 0.0; }

void Hardening::computeQpProperties() {
  Hardening_compute(_K, _alpha_old[_qp], _dt, _sigma_y[_qp], _alpha[_qp]);
}

Tests

• KNOWN_GAP guard → 4 positive Phase-2.6 tests: declares state-var properties, maps dt→_dt (no getParam("dt")/addParam("dt")), compute-call arity threads the state args, state initialised in initQpStatefulProperties.
• Step A refactor verified pure: 182 tests unchanged after canonical_arguments extraction.
• 185/185 pass locally.

Closes #60

The Category-aware render branch is now complete across both layers: StateVariableCurrent → out-param (Newton path) / declared-property (MOOSE); StateVariableOld → getMaterialPropertyOld. Closes #60.

Out of scope

• Tensor state variables (scalar only, matching Phase 3a-2).
• Abaqus / ANSYS state-var wiring (the canonical_arguments helper makes them mechanical — they just need a role→argument mapping like MOOSE's).
• Consistent tangent / _Jacobian_mult wiring — Phase 3b (Phase 3b — Algorithmic tangent + Kuhn-Tucker lowering #35) / Phase 5 (Phase 5 — MOOSE + Standalone state-variable + algorithmic-tangent wiring #37).
• The emitted MOOSE source is structurally correct but not compiled in CI (no MOOSE headers); the compile-check harness covers the generic compute function numerically.

Closes #77, closes #60.

[petlenz]

Three-lens critical review (architect / cpp-pro / code-reviewer) on Phase 2.6. The canonical_arguments single-source-of-truth is the right architecture and the MOOSE happy-path is correct — but the review found 2 CRITICAL + 5 MAJOR, several convergent across lenses. Two I verified directly against the build:

• add_output has no name guard (confirmed) — a state var alpha + output alpha both declareProperty("Model_alpha") → MOOSE duplicate-property error + duplicate _alpha member.
• Non-constant initial values emit bare symbol names (confirmed) — add_scalar_state_variable("c", K*0.5) generates _c[_qp] = 0.5 * K; in initQpStatefulProperties, but the MOOSE member is _K — K is undefined. Silent broken MOOSE.

The recurring theme: the scope boundary (scalar-only, constant-init) is documented in comments but not enforced — tensor state vars, tensor params, and non-constant initials all mis-emit silently instead of failing loudly. Most fixes are small guards.

---

[CRITICAL] add_output does not check name availability (out-of-diff: recipe.h add_output, unchanged in this PR but exposed by it). Confirmed: add_output skips the assert_symbol_name_available guard that add_scalar_state_variable / the input+param adders call. A recipe with a state variable alpha and an output alpha both emit declareProperty<Real>("Model_alpha") + a _alpha member → MOOSE duplicate-property error + C++ redefinition. Phase 2.6 makes this reachable (state vars now declare properties). Fix: call assert_symbol_name_available(name) at the top of both add_output overloads + add a death test for the state-var/output name clash.

[petlenz]

Round-2 review (architect + code-reviewer). All 7 round-1 fixups verified held. 1 new CRITICAL found and fixed in d4ee408; 1 MINOR deferred to a follow-up.

[CRITICAL — fixed] Reverse-order output↔state-var name collision bypassed the round-1 guard. Round-1 made add_output check both m_symbols + m_outputs, but add_scalar_state_variable / inputs / params call assert_symbol_name_available, which only scanned m_symbols. So add_output("alpha") then add_scalar_state_variable("alpha") slipped through (confirmed: NO THROW) → duplicate declareProperty("M_alpha") + duplicate _alpha member = the same uncompilable MOOSE bug as the round-1 CRITICAL, just in the other declaration order. Fix: assert_symbol_name_available now also scans m_outputs — symmetric with assert_output_name_available. Test OutputStateVarNameClashThrowsEitherOrder exercises both orders + a param/output reverse clash.

[MINOR — deferred → #79] Derived names not reserved. A user output literally named alpha_residual could collide with the synthesised residual for state var alpha (and similar for _old/_out/_jacobian). Low-likelihood, not a current-recipe hazard; tracked in #79.

Everything else verified clean:

• is_time_step reuse path is self-consistent (a user-declared dt param drives both the residual division and MOOSE getParam; no hijack).
• Tensor state vars throw at emit (canonical_arguments + emit_init_stateful_body); the .h/.C are built in one return {...} so no partial-file is observable when emit_source throws.
• Constant-only init restriction has no false positives (scalar_constant/leafless exprs pass).
• count_compute_call_args is correct for these recipes (no nested parens in the call); the multi-SV arity test asserts against canonical_arguments().size(), not a magic number.
• No regression: standalone signature still emits double const dt via the TimeStep role; numerical compile-checks unaffected.

190/190 tests. Recommend landing once CI is green.

[petlenz]

Round-3 review (cpp-pro + code-reviewer). Both lenses: LAND IT.

cpp-pro — clean. Verified the round-2 symmetric name-guard is complete + correct: all five add-paths (params/inputs/state-vars) check against both m_symbols and m_outputs; both <sv> and <sv>_old are guarded before any push (no partial-state on throw); ordering reads the correct container; the new test fails without the fix.

code-reviewer — land, 2 MINOR + 1 NIT. I applied the one with real correctness flavour now (it matches this PR's fail-loud posture):

[MINOR — fixed f1c…] Non-Newton MOOSE evolution recipe emitted valid-but-DEAD code — residual/Jacobian as properties nothing solves + a current-state property never written. MooseMaterialTarget now throws on evolution_equations && !local_newton_enabled(), pointing the user to enable_local_newton() (or the standalone target for external-driver mode). Test MooseRejectsEvolutionRecipeWithoutLocalNewton (+ asserts standalone still accepts the same recipe).

[MINOR — fixed] Docs — workflow.md §5 2.6 row now states the local-newton requirement + constant-init-only limit.

[NIT — already covered] Standalone evolution-recipe signature — StandaloneEmitsAllPhaseOutputsConsistently + LocalNewtonTest already pin it; no new test needed.

Verified clean (no new issues): is_time_step reuse self-consistent, empty/never-evolved state var handled sanely, tensor + non-constant-init throw with no partial-file, PR scope coherent (canonical_arguments is the spine; everything else follows from it).

Deferred to fast-follow #79: reserve derived names (_residual/_jacobian/_old/_out) — low-likelihood, not a current-recipe hazard.

191/191 tests. Three review rounds converged (3 CRITICAL + 5 MAJOR found & fixed across rounds 1-2; round 3 = 1 MINOR fixed + clean). Ready to land.

[petlenz]

Round-4 review (calibrated, convergent-cycle pass). LAND IT — nothing actionable.

Checked every fresh angle:

• State var WITHOUT an evolution equation in MOOSE (the guard's blind spot candidate): not a gap — gets StateCurrentRead/StateOld + initQpStatefulProperties seed, carried forward unchanged = a valid degenerate constant-in-time state, identical to standalone semantics. The guard correctly targets only the genuinely-broken case (R/J emitted with nothing solving them).
• Existing tests/examples broken by the new throw: none — no registry recipe or example uses evolution equations; every MOOSE evolution recipe calls enable_local_newton(). 191/191 green.
• count_compute_call_args false-pass: sound — the MOOSE call emits only bare member refs (no embedded )), so the first-) scan never truncates; tensor-input + state-var shapes parse correctly.
• canonical_arguments tensor-throw vs partial emit: the throw fires while building an in-memory ostringstream, before any EmittedFile is returned — no partial output.

Converged: rounds 1-2 (3 CRITICAL + 5 MAJOR) → round 3 (1 MINOR) → round 4 (nothing). 191/191 tests. Ready to merge.

[petlenz]

Round 5 review — security lens (codegen injection surface)

Rounds 1–4 converged on correctness (3 CRITICAL + 5 MAJOR + 1 MINOR, all fixed; 191/191 green). Round 5 switched to an adversarial codegen-injection angle: every user-supplied string that flows verbatim into emitted C++ is an injection surface. Three real findings, all fixed in 922fa62.

[CRITICAL] Model name was never validated

The model name flows raw into the generated function name (<name>_compute), the MOOSE class header (class <name> : public Material), registerMooseObject("<name>"), and every declared-property literal (declareProperty<Real>("<name>_...")). Nothing checked it.

Verified pre-fix: ConstitutiveModel("Foo; struct Evil {};") emitted inline void Foo; struct Evil {};_compute( — a name carrying a statement separator injects a top-level declaration into the generated TU, with no throw.

Fix: the ConstitutiveModel constructor now runs SymbolValidationPass::is_valid_cxx_identifier(m_name) and throws on any non-identifier / keyword name. All existing recipes use valid identifiers — no regression.

[MAJOR] Parameter doc strings were emitted unescaped

validParams() streams each parameter's doc into a C++ string literal. An embedded " terminates the literal early; pre-fix, doc bad " doc emitted addParam<Real>("K", 1, "bad " doc"); (ill-formed, and the tail forms arbitrary tokens).

Fix: doc is routed through the existing escape_for_cpp_literal helper (escapes " \ \n \r \0).

[MAJOR] Non-finite parameter defaults streamed as nan/inf

A NaN/inf default streamed as the literal text nan/inf, which is not a valid C++ floating literal — the generated source would fail to compile downstream with a confusing error.

Fix: the validParams loop now guards with std::isfinite and throws a clear recipe-time error; defaults are formatted via std::format for round-trip precision.

Verification

• Build green; full suite 194/194 (191 prior + 3 new security regressions):
  • Integration.ModelNameMustBeValidIdentifier — separator-injection / leading-digit / space / empty / keyword names all throw; a valid name still constructs.
  • Integration.MooseEscapesParameterDocString — embedded ", \, and newline are escaped; no raw newline leaks into the literal.
  • Integration.MooseRejectsNonFiniteParameterDefault — NaN and inf defaults both throw.

Verdict

Round 5 complete. With the injection surface closed, I consider this PR converged and ready to merge. Deferred follow-up filed as #79 (reserve derived names _residual/_jacobian/_old/_out).

[petlenz]

Round 6 review — fresh lenses: adversarial test-quality + standards/generated-code conformance

Rounds 1–5 converged on correctness and security. Round 6 deliberately took two angles not yet exercised, run as independent parallel lenses, each told not to re-flag prior rounds.

Lens A — C++ standards conformance + generated-code correctness: clean

No new CRITICAL/MAJOR. Notable verifications:

• std::format("{}", double) is correct here — C++23 default float format is the shortest round-tripping representation (to_chars-equivalent) and is locale-independent. Strictly better than the old os << default_value (6-sig-fig, lossy). 1.0→1 is harmless (target is Real).
• No read/write aliasing in the call-site role mapping: the Newton-owned current var (NewtonStateOut → _alpha[_qp], write) and StateOld (_alpha_old[_qp], read) are distinct members of the same MOOSE-versioned property — correct.
• Include hygiene (<cmath>/<format>/<limits>), exception-safety (guards run before mutation), and RecipeView{model} lifetime all check out.

Lens B — adversarial test-quality: 1 genuine test defect + coverage notes (fixed e0d44ad)

• Fixed — misleading "arity" test gave false confidence. MooseComputeCallArityMatchesGeneratedSignature claimed to prove the historical 3-vs-7 arity bug was gone, but only asserted three substrings were present somewhere — a reordered or mis-wired call would still pass green. Renamed to MooseComputeCallThreadsStateAndDtInCanonicalOrder and now pins the relative call-site order (alpha_old < dt < alpha). (Actual arg-count remains covered by the separate MooseComputeCallArgCountEqualsCanonical.)
• Fixed — StateCurrentRead role was unexercised. canonical_arguments classifies a current state var on a non-local-Newton recipe as StateCurrentRead; only the Newton-owned case had a test. Added CanonicalArgumentsClassifiesNonNewtonCurrentStateAsRead.
• Lens A's wrong-reason-throw audit (does an EXPECT_THROW catch the intended guard?) came back clean — each negative test was traced by execution to the specific guard message.

Tracked / recommended follow-ups (not fixed in this PR — out of its introduced scope)

• Existing MOOSE backend: add runtime parity test against mock RankTwoTensor #12 already tracks the one MAJOR-ish gap: the MOOSE wiring is string-matched, never compile-checked. The numerical Newton writeback is covered, but via the standalone-shaped _compute signature (compile_check_driver), not the MOOSE call-site / property pairing / initQpStatefulProperties. A mock-MaterialProperty compile fixture would close it. No dupe filed.
• Recommended new follow-up (precision): initQpStatefulProperties seeds the state variable from its constant initial via the scalar emitter (scalar_code_emit.h), which renders constants through a default-precision ostringstream (6 sig-figs) — silently lossy for a non-zero initial like 0.123456789. Pre-existing, but Phase 2.6: MOOSE state-variable wiring + canonical_arguments #78 newly reaches it. The emitter should move to std::format/to_chars shortest-round-trip, matching the validParams path Phase 2.6: MOOSE state-variable wiring + canonical_arguments #78 already uses. The tested 0.0 case is exact, so current tests don't catch it.

Verification

Build green; full suite 195/195 (was 194; +1 StateCurrentRead, arity test renamed not added).

Verdict

Round 6 found no new functional or security defect — one real test-quality defect (false-confidence arity test) and one coverage hole, both fixed. The implementation itself stands. I consider PR #78 converged and ready to merge; the two follow-ups above (#12 compile coverage; scalar-emit precision) are out-of-scope improvements, not merge blockers.