Changes needed for doenet integration - #84
Open
siefkenj wants to merge 94 commits into
Open
Conversation
…ale comment This PR adds a `rust-lint` CI job running `cargo fmt --all -- --check`, and the PR's own code failed it in three files. Formatted those three and no others — the repo is deliberately not fmt-clean overall, so a repo-wide run would bury this in noise. `numeric.rs`'s `#[allow(clippy::too_many_arguments)]` carried its rationale as a trailing comment that rustfmt wanted to reindent oddly; moved it above the attribute instead. `cargo fmt --all -- --check` and `cargo clippy --workspace --all-targets -- -D warnings` are both clean now. `ops::preserve_order::is_non_finite` matched only a bare `Expr::Const`, but this pass deliberately preserves the `Neg` wrappers the canonical path peels off. So `0·(−∞)` annihilated to `0` instead of `NaN` — a silently wrong *number* on the `skip_ordering` path DoenetML's equality checking, `MathOperators` and `Parabola` all use, which is the exact failure shape `constructors::annihilate` was written to prevent for `0/0`. Peel negations before the match, with a regression test. The bare `Const` restriction is otherwise deliberate and unchanged: an unprovable factor such as `1/x` still falls through to `0`, matching legacy. `normalize/simplify.rs`'s "documented divergences we do NOT emulate" block still listed `0/0 → 0`, `0·∞ → 0` and `0^0 → 1`. All three now fold to `NaN` — `constructors::annihilate` changed that within this same PR — so the comment described the code as it was before, in the one place a reader goes to understand the ∞/NaN cluster. Rewritten to point at `annihilate` and at `tests/signed_zero.rs`, which pins the current behavior. Also: `math-expressions-js-compat/README.md` linked `./DOENET_INTEGRATION.md`; the file is at `active-plans/DOENET_INTEGRATION.md`. `cargo test --workspace`: 837 passed, 0 failed. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… wasm boundary
`polynomials/compat/groebner.rs`'s auxiliary elimination variable was `_t`, the
name the JS engine used. But `poly_lcm` eliminates by rejecting the basis
elements led by that variable, and a `Poly::Rec` is led by its *least* variable
under the default order — which compares symbol names byte-wise, so `"A" < "_t"`.
For any variable sorting before `_`, i.e. every uppercase name, `poly_gcd` and
`reduce_rational_expression` returned a basis element still carrying the
auxiliary variable: `reduce_rational_expression` on `(A²−1)/(A²+2A+1)` answered
`1/1` instead of `(A−1)/(A+1)`, silently, on an API exported to JS. Use a leading
control character, which sorts below every character a parser accepts in a name,
and pin it with a regression test over four spellings.
`ops::preserve_order::is_non_finite` — the previous cycle peeled `Neg` and
stopped there, and its commit message claimed the bare-`Const` restriction was
otherwise deliberate. It was not: this pass keeps trees unflattened *and*
un-peeled, so `0·∞²`, `0·(∞/2)`, `0·(∞+1)` and `0·(0^(-1))` all still annihilated
to `0` where `simplify` gives `NaN` — the same wrong-number-on-a-grading-path
failure the earlier fix was written to close. Recurse the wrappers the pass
preserves, mirroring `constructors::is_infinite_factor` for the `Pow` case, and
extend the test.
`poly_op("reduce_ith", …)` passed an unvalidated index into a direct slice
index. `reduce_ith(0, [])` panicked, and this crate is built `panic = "abort"`,
so a JS caller could trap the module for the whole page. Bounds-check at the
boundary and document the precondition on `reduce_ith`.
`polynomial_pow` had no resource limit while `constructors::pow` is capped by
`max_expand_power`. Raising a multi-term polynomial is a multinomial expansion
and `polynomial_mul` is quadratic in term count, so `(x+1)^400` takes two
seconds through `expression_to_polynomial` — reachable from an answer box. Hold
it to the same budget, exempting monomials so `x^1000` still works.
`polynomial_mul` returned a one-term `Poly::Rec` where `polynomial_add` collapses
to a `Poly::Coeff`, breaking the invariant its own module doc states — and this
API's contract is structural comparison of the returned AST, so `["polynomial",
"x", [[0,12]]]` is not `12`. Extracted `finish` and used it on all four paths.
`Context._assumptionsHandleCache` was not dropped on `setWasmModule`, although
`_wasm.ts` states that contract in as many words; the same applied to
`element_of_sets.ts`'s `emptyCache`. After a swap every `equals`/`solve_linear`
would fail with "expected instance of Expression". Registered both listeners.
`set_to_default` also leaked the handle it replaced — `clear_assumptions`
delegates to it, so a long-lived worker leaked one `Assumptions` per clear.
`js_match.rs`'s `orient_relations` spelled two relation heads `supseteq` /
`notsupseteq`, which are LaTeX commands; the JS AST names are `superseteq` /
`notsuperseteq`. Both arms were unreachable, so `A ⊇ B` alone failed to orient
under `allow_permutations`.
`from_number` built a `Num(Float(NaN))` for non-finite input while the JSON path
builds `Expr::Const`, and the ∞/NaN folds match only the `Const` form — so
`me.fromAst(NaN)` simplified differently depending on which path it took. Its
doc claimed the two agreed.
`ast-to-guppy.ts` dispatched `"~"` (and any unported function symbol) into
`operators[operator](...)` with no entry in the table, raising a `TypeError`
instead of falling through to the parenthesized default. Look the emitter up
once and fall through when there is none.
`ops::vector_matrix` argued against `unreachable!()` in one function's comment
and used it in two others. Made all three degrade instead.
Doc corrections: `simplify.rs`'s ∞/NaN comment claimed `0^0 → NaN` "matches JS"
(JS gives `1`, and this is policy-dependent on `pow_strict`) and named a test
file that pinned none of the four forms — corrected, and the test added.
`trees/basic.ts` called the `_skipped` splice unreachable when `flatten.ts` sets
it, described a pre-splice fold the port does not do, and promised reference
identity for leaf replacement. `active-plans/DOENET_INTEGRATION.md` had five
links into an `upstream_requests/` directory that does not exist, a heading
contradicting its own response section, and stale per-cluster counts. The compat
README and `JS_TEST_COVERAGE_AUDIT.md` still listed polynomial/Groebner as
unported, which this PR ports.
`cargo test --workspace`: 839 passed, 0 failed. `cargo fmt --all -- --check` and
`cargo clippy --workspace --all-targets -- -D warnings` clean. JS compat suite:
6316 passed, 1 failed (the documented assumptions-soundness divergence).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`substitute` had become a left-to-right pass, its doc asserting that legacy was
one too and that DoenetML relied on a substituted `<math>` code expanding into
further codes. Checked against the published `math-expressions@2.0.0-alpha95`
build: legacy walks the tree once, and `{c1: "c2", c2: 5}` leaves `c2` standing
there as well. What the sequential pass actually did was capture — no pair of
bindings could be swapped, and DoenetML substitutes a line's declared variable
names into `a·x + b·y + c` (`Line.js`), so `<line variables="y x">` put both
coefficients on one variable. Now simultaneous, differing from `substitute_all`
only in that it coerces each binding, so a string is parsed as legacy parsed it.
`variables(include_subscripts)` dropped its argument, reporting `x_1 + y` as
`["x", "y"]` in both modes; `Line.js` calls it with `true` six times to decide
whether a coefficient mentions the line's own variables.
`Expression#f()` threw `Invalid ast` on any tree holding `±Infinity` or `NaN`:
the compile path parsed `tree_json()` without decoding the `{"$":"Inf"}` /
`{"$":"NaN"}` wire tags, handing the mathjs converter a plain object where it
has a branch for each of the three as numbers. That is the path everything
plotted or searched for extrema goes through, and this branch deliberately folds
`0/0` to `NaN`, so it was reachable by design. `f()` had no spec coverage.
`critical_points` and `evaluate_many` passed their variable argument straight
into a `&str` binding, where wasm-bindgen reads a length off whatever it is
given; every other variable-taking method already went through `varName`, and
this crate is built `panic = "abort"`.
Also corrects `evaluate_to_constant`'s claim that answering `null` for a free
variable is what legacy did — legacy answered `NaN`, and the divergence is
deliberate but was described as continuity. Its `nan_for_non_numeric` option is
still not read; that is now said rather than implied.
Compat suite: 6320 passing, 1 failing (the documented assumptions-soundness
divergence), 10 skipped, 2 todo.
`is_single_delimited_group` (`print/latex.rs`) stepped one *byte* at a time over anything that was not a `\left`/`\right` token, so a multi-byte character inside the group put the next `&s[i..]` off a UTF-8 boundary. This crate is built `panic = "abort"`, so that is not an error a caller can catch — it takes the module down. `＿` (U+FF3F) is three bytes and is exactly what DoenetML leaves in a slot the student has not filled: `to_latex` of `(_, _) and x` aborted. The text-printer twin `is_single_paren_group` already walks `char_indices`, which is why only one of the two had it. `0^0` folded to `1` in `evaluate_numbers_preserve_order`, then disappeared into whatever it multiplied — `x·0^0` read as `x`. It is the one indeterminate form with no infinity in it, so the annihilation guard beside it, which looks for a non-finite factor, never saw it. `simplify` answers `NaN` under the default policy, and this is the `skip_ordering` path DoenetML's equality checking runs along, so the two were giving one expression two answers. The neighbouring powers are unaffected: `0^2` is `0`, `2^0` is `1`, and `0^(-1)` is not an integer power this pass folds at all, so the pole stays written out where `is_non_finite` already recognizes it. Both pinned by tests. `cargo test --workspace` 841 passing, 0 failing; fmt and clippy clean; the JS compat suite is unchanged at 6320 passing with the one documented assumptions-soundness failure.
Each was reproduced against the public API before the change and re-checked
after it; each has a regression test in `tests/doenet_review_fixes.rs`.
`default_order` was not a normal form. `append_unit` stringifies sort-key
index 1, which for a Seq/Array/Interval holds the operand count rather than a
kind name, and `cmp_key` compared that `Str` with a plain container's `Num`
numerically — `"2_%".parse::<f64>()` is `NaN`, so a unit-annotated container
tied with *every* plain one while the plain ones still ordered by length.
`(z,z) + (y,y)% + (x,x,x)` produced three different trees from its six input
orderings, and `default_order` is what `simplify="normalizeOrder"` runs, so
one answer graded differently depending on how it was written. `cmp_key` now
stratifies its domain — numbers and booleans, then strings, then arrays — which
also makes it a strict weak ordering, so the `sort_by` calls (including
`canonicalize`'s equals path) cannot trip Rust's order-violation panic. The
kind-name keys units were designed for compare string-to-string and are
unaffected.
Odd-root sign extraction treated *unknown* realness as real. `is_real` answers
`None` for `i·x` and `x + i` because it cannot rule out an imaginary `x`, and
the rule declined only on a provable `Some(false)`, so `cbrt(-i·x)` became
`-cbrt(i·x)` — a different number, as this crate's own `equals` reports. The
check is now on the spelling (via `constant_policy::is_i`, so a document
declaring `i` an ordinary variable keeps the rewrite); a bare symbol still
counts as real, which is what `cbrt(-x) → -cbrt(x)` rests on.
`∞ − ∞` written as poles cancelled arithmetically. `add` collects like terms,
and the additive-inverse identity does not hold for an infinite term:
`1/0 − 1/0` answered `0`, `1/0 + 2 − 1/0` answered `2`, `2/0 − 1/0` answered
`∞`. `simplify::is_infnan_constant` already counted a zero-pole, but
`constructors` runs first (it is canonicalization) and won. A group of like
terms with a non-finite top-level factor is now indeterminate unless every
coefficient pulls the same way, so `1/0 + 1/0` is still `∞` and a pole inside a
finite subexpression (`1/(1 + 1/0)`) still cancels.
The printers panicked on 104 of the trees `expr/serde.rs`'s catch-all accepts.
It builds `OtherOp(name, args)` for any unknown head with no arity check, so
`me.fromAst(["pm"])`, `["binom","x"]`, `["unit","x"]` and `["d"]` are all
constructible from JS, and both printers indexed `args[0]`/`args[1]` without
looking. A sweep of 22 heads x 4 arities x 4 wrappers x 6 operations found
every one of them in `to_text`/`to_latex` and none in the transform layers.
A shared `other_op_min_arity` now drops an under-supplied head to the generic
`name(args…)` form, which is what `render_angle` already did by hand.
`Number::neg` overflowed on `i64::MIN`, which is an abort under
`overflow-checks` and a silently wrong number without them. Both reachable
sites are typeable: `simplify("-9223372036854775808(1-x)")` and
`simplify("2^(-9223372036854775808/1)")`. Both integer tiers now widen.
`rule_gaussian`'s exponent cap was per node while `rewrite` runs bottom-up, so
nesting multiplied it: `(((2+i)^64+1)^64+1)^64` is about thirty typeable
characters and did not finish in twenty seconds. The result size is now bounded
too, by a new `ResourceLimits::max_gaussian_pow_bits` — far below
`max_pow_bits` because an integer power is one shift while `(a+bi)^k` is `k`
big-rational multiplies.
`log_b(a) → log(a)/log(b)` is invalid at `b = 1`, where `log b` is `0`:
`log_1(5)` answered `∞` and `log_1(1)` answered `1`. Both are `NaN` now.
Also here, without behavior change: the LaTeX printer spells a non-finite
`Num(Float)` as `\infty` rather than the text word `Infinity`, which reparses
as a product of letters; `present_mul` accumulates numeric factors instead of
overwriting the coefficient, an undocumented precondition on a display pass;
the `sqrt`/`cbrt`/`nthroot` dispatch duplicated between the two radical passes
is now one `as_root_call`; and the byte-identical `render_add` and
`is_shorthand_angle` in the two printers are now one copy each in `print/mod`.
Doc corrections: `simplify`'s module rule list named seven clusters where
`rewrite` runs nine, and named a `rule_distribute_neg_over_sum` that no longer
exists; the radical cluster claimed `sqrt(-2) → i·sqrt(2)` where `sqrt(-2)` is
in fact unchanged and `sqrt(-18) → 3·sqrt(-2)`; the trigonometric-Pythagorean
banner sat above the whole Q(i) cluster rather than above its own rule;
`print/mod`'s sign-splitting rationale claimed the parsers never emit a `Mul`
with a negative leading factor, but `parse_text("(-3)b")` does; and
`sort_key`'s doc claimed nested keys never propagate `ignore_negatives`, where
the `Pow`, `Apply` and unit branches do.
`cargo test --workspace` 848 passing, 0 failing; fmt and clippy clean. The JS
compat suite is unchanged at 6320 passing, with the one documented
assumptions-soundness failure.
Both were found by triaging DoenetML CI (run 31804350344), which is the first
full-suite signal this pair of PRs has had since the branches diverged — a
merge conflict had been suppressing `pull_request` workflows.
**A float-valued one was not the multiplicative identity.** `Number::is_one`
recognized only `Int(1)`, where its sibling `is_zero` had always accepted
`Float(0.0)`. That predicate is what drops the identity factor in
`normalize::mul` and the identity exponent in `normalize::pow`, so a coefficient
that folded to one *through a float* stayed written down.
Only the JSON path could reach it: the text and LaTeX parsers turn a decimal
literal into an exact rational (`Number::from_decimal_str`), while
`expr::serde::try_from_js` hands `0.5` to `Number::from_f64` and gets a `Float`.
So `fromAst(["*",0.5,2,"x"])` simplified to `1·x` while `fromText("0.5*2*x")`
gave `x` — the same mathematics, two spellings, decided by which door the
expression came in through, and DoenetML comes in through `fromAst`.
It reached grading. `<math simplify expand>` of `0.5(2x-2)(x+1)` produced
`1·x² − 1` where the same answer typed `1/2(2x-2)(x+1)` produced `x² − 1`, so a
correct response failed a `symbolicEquality` comparison against `x² − 1`
(DoenetML's `factoringOldAlgorithm` suite).
**The unordered term re-match fired when its allowance was never spent.**
`fuzzy::unordered_eq` exists because forgiving ε in a number may have moved the
term holding it — term order is derived from the very values being fuzzed. Sound
for that case, but it was gated only on "a tolerance is set", so it also forgave
a reordering the sort never caused. A permutation that needs no ε is not sort
drift; it is the two expressions being written in different orders, and
`equals_syntactic` is a *form* check.
It now declines when the terms match exactly, which is the same test the
rationale implies. DoenetML's `<answer symbolicEquality
allowedErrorInNumbers="0.001">` is documented and tested as refusing a reordered
response, and `e·25.602348230 + 2.15234262π` against `2.15234262π +
e·25.602348230` — every number identical — was grading correct.
Regressions for both, in `tests/doenet_review_fixes.rs` and `tests/equality.rs`,
including a case that pins the *original* motivating reordering as still
forgiven. `cargo test --workspace` and `cargo fmt`/`clippy -D warnings` are
green, and the JS compat suite is unchanged at 6333 tests — 6320 passed, 10
skipped, 2 todo, and the one documented `slow_assumptions.spec.ts` divergence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…ee doc fixes - **`mml-to-latex.ts` mistranslated `;`.** The entity table carried a bare `";"` key — the remains of a `ς` key that lost its head — under a comment asserting it was unreachable because "text content never equals `;` alone after the surrounding markup is stripped". It does: `content()` matches `/^([^<]*)/`, so `<mo>;</mo>` yields exactly `";"`, and MathJax emits `<mo>;</mo>` for every semicolon separator. `f(x; y)` converted to `f ( x \sigma y )`. Spelled as the entity it was meant to be, with a spec. - **`astToJson` was open-coded twice.** `expression/simplify.ts` and `expression/equality/discrete_infinite_set.ts` each wrote `JSON.stringify(tree, (_k, v) => tagNonFinite(v))`, which is character for character the helper both files were already importing from — and `converters/ast-json.ts` opens by saying it exists so that "the converters and `Expression.fromAst` tag them identically rather than one of them forgetting". - **`element_of_sets.ts`'s default-source branch was described wrongly.** `Context.assumptions` is the *facade*, not the live handle the comment claimed, and being a lazily built object it is never nullish, so the `?? empty()` beside it could not run. Behaviour is unchanged — the facade mirrors the predicate methods, verified both ways — but the two adjacent branches now say what they actually return. - **`equals_with_options` is not called anywhere in `lib/`.** The comment above `EQ_OPTION_KEYS` named it as the consumer of the camelCase mapping; the real consumers are `Assumptions#equals_expressions` and `Expression#structural_equality_with_options`. - The compat `README.md` still listed the MathML converters as unported, in the PR that ports them. Replaced with what is actually still missing there: `Context.fromMml` is `notImplemented`, and `me.from` does not try MathML as its third fallback the way legacy's `create_from_multiple` did. Suite unchanged: 6333 tests — 6320 passed, 10 skipped, 2 todo, and the one documented `slow_assumptions.spec.ts` divergence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…ntract `math-expressions@3.x` on npm is the agreed path off DoenetML#1622's temporary submodule bridge, so this pass took the package as far as an actual publish and fixed what that turned up. Verified by packing the tarball, installing it into a throwaway project outside the workspace, and driving it from there. Publishability: - `npm install` of the tarball failed outright: `math-expressions-rs-wasm` was a `dependency` and is not on the registry. Vite inlines it — the published `dist/` never imported it by name — so it belongs in `devDependencies`, and moving it there is what makes the package installable at all. - No `prepack`, so `dist/` (git-ignored, and what `main`/`exports` point at) shipped only if somebody remembered to build first. `prepack` now runs the new `build:package`. - `exports["."]` declared no `types`. The legacy library's hand-written declarations come back as `types/math-expressions.d.ts`, plus the v3 additions (`dopri`, `setWasmModule`, the default export). `npm run typecheck` compiles `types/usage.ts` against them so they cannot rot silently. - The tarball carried only the *nodejs*-target wasm, so a browser host still needed cargo. `build-wasm.sh` now builds both targets and `vendor/wasm-web/` ships under a `./wasm-web/*` export. `scripts/consumer/web-path.mjs` proves a worker-shaped host can consume the published package with no Rust toolchain. - math.js is external rather than inlined: it was 95% of the bundle (1,002 kB against 48 kB), it is already in `dependencies`, and a private copy is a second `math.create(math.all)`. `npm run verify:package` does the whole pack/install/consume cycle, and a new `package publishability` CI job gates on it. It is the only check that sees this package the way npm consumers will. `spec/build_esm.spec.ts` and `spec/build_umd.spec.ts` — the only two tests that exercise the built artifact — pointed at `../build/`, the legacy library's output directory. Vite writes `dist/`, so both had been reporting `todo` rather than running: they are the "2 todo" in the suite counts. Repointed, and the UMD one now supplies what a browser supplies (a `math` global for the external, and an injected wasm module), which is 16 tests that had never run. `src-js/wasm.ts` says it is the single source of truth for the wasm surface. It was ten members and one arity behind `lib/**` — `substitute_map`, `structural_equality_with_options`, `evaluate_numbers_to_floats`, `evaluate_numbers_evaluate_functions`, `normalize_applied_functions`, `normalize_negative_numbers`, `expand_relations`, `from_number`, `discrete_infinite_set`, `Assumptions#equals_expressions`, and `subscripts_to_strings(force)`. All declared now against the generated `.d.ts`. Nothing had caught it because `lib/**` was not type-checked at all, so this also clears the 25 remaining `lib/**` type errors (class fields assigned but never declared, two `never`-returning Proxy stubs, four casts, two signatures that were required where the code passes nothing) and adds `tsconfig.lib.json` + a gating `typecheck`. `spec/**` stays out of it, and the header now scopes the claim to what is enforced. README: the handle-lifetime section said there is no `FinalizationRegistry` and no disposal API, where wasm-bindgen registers one per class and `free()`/`dispose()`/`using` have existed for some time; `lib/wasm-types.ts` does not exist; the "known exclusion" spec is not excluded and passes; and the browser build is no longer "future work". Replaced with what the package actually does, plus a publishing section. No behavior change: the suite is 6,348 tests, 6,337 passing, 10 skipped, 1 failing (the documented `slow_assumptions.spec.ts` divergence) and 0 todo, against 6,333/6,320/10/2-todo before.
…e prose
`is_real(i*i)`, `is_real(i^2)`, `is_real((2i)^2)`, `is_integer(i*i)` and
`is_negative(i*i)` all answered false on the public assumptions API. `i^2` is
`-1`: a negative real integer, and the engine's own `simplify` folds the same
expression to `-1`, so it contradicted itself. Legacy JS answered true.
Measured through the public API (`is_real(&parse("i^2"), &Assumptions::default())`)
before and after. The chain: `queries::is_real` → `infer::facts` →
`constant_facts` → `eval_complex`, whose `Pow` arm fast-pathed to an exact
`powi` only when `base.im == 0.0`. An imaginary base fell through to `powc`,
which goes via exp/ln and returns `i^2 = -1 + 1.2246e-16i` and
`i^4 = 1 - 2.449e-16i`. `Facts::of_constant` then tests `z.im != 0.0` with
*exact* equality, calls the value genuinely complex, and `Facts::normalize`
forces integer/nonneg/positive/negative/nonpos to false behind it.
Fixed at the source rather than with a tolerance in `of_constant`: the integer-
exponent fast path now covers a base anywhere in the plane, using
`Complex64::powi` (exponentiation by squaring over exact complex
multiplication). An epsilon in `of_constant` would instead let a genuinely
tiny imaginary part claim to be real, which is the unsound direction; nothing
here answers `real = true` for a value off the real axis, and
`i^3`, `(1+i)^2`, `i*10^(-300)` are pinned to prove it. `sqrt(-1)^2` is fixed
by the same change. `(-8)^(1/3)` is not and is unrelated: its exponent is
fractional, its principal value really is `1 + 1.732i`, and only `simplify`'s
odd-real-root convention makes it `-2`.
cargo test --workspace: 850 passed / 0 failed before, 851 / 0 after (the new
regression test). `cargo fmt --all -- --check` and
`cargo clippy --workspace --all-targets -- -D warnings` clean. In js-compat,
after `bash build-wasm.sh`: `slow_assumptions` 843 passed / 1 failed /
1 skipped of 845 and `slow_simplify` 74/74, both unchanged by the fix.
Prose corrections, each verified first:
- COMPAT_TEST_FAILURE_SUMMARY pointed at `SLOW_ASSUMPTIONS_PLAN.md`, which is
in no commit on any branch. Replaced with the in-place writeup and
`tests/assumptions_sound_reasoning.rs`.
- Three places called the remaining `slow_assumptions` failure "a deliberate
soundness divergence", which reads as though *this* engine is the unsound
one. It is the reverse: legacy commits to answers that are mathematically
false (with `y` real, `y = 0` is a model, so `x*y` really can be
real/nonpositive/nonnegative) and we decline. Reworded; this engine is
incomplete on that test, never unsound.
- The failing test hides six failing assertions, not one — spec lines 7357,
7415, 7417, 7418, 7419, 7420, counted by temporarily converting that `it`'s
`expect` to `expect.soft` (scaffolding reverted) — from two unrelated root
causes. Recorded in the js-compat README and the summary.
- `Facts::and_meet`'s doc claimed to be "the one place this diverges". It is
one of two; `combine::mul` not carrying non-realness through a product is
the other.
- JS_TEST_COVERAGE_AUDIT recorded `slow_assumptions` at 44 cases. It runs 845
tests; 44 is the count of `it(` sites, and most tests are generated from
tables inside a loop.
- ASSUMPTIONS_ENGINE_PLAN's "target 0" is unreachable without adopting unsound
reasoning. Replaced with an Accepted-divergence section that also records the
`combine::add`/`combine::mul` non-realness propagation rules as a known gap
we decline: they turn `None` facts into `Some(false)`, `simplify`'s rewrites
are gated on those facts, and moving them moves grading.
- The summary's suite totals were a stale snapshot at `7082f8a`. Re-measured
at `67e99ee`: 1 failed / 6337 passed / 6348 total, 10 skipped, 0 todo, which
agrees with the README.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`simplify_root`'s odd-root sign extraction was the one rule in the crate
that treated an *unknown* realness fact as permission (`is_real(rest) !=
Some(false)`), and `combine::add`/`combine::mul` never carry non-realness
through an operator — so `is_real(sqrt(-2))` is `Some(false)` while
`is_real(x·sqrt(-2))` is `None`, and the residual read as real:
cbrt(-x*sqrt(-2)) -> -cbrt(x*sqrt(-2))
which is a different number. The engine contradicted itself about it: at
`x = 1` the residual is closed, the fact is `Some(false)`, and it correctly
declined. `equals` inherited the contradiction, answering `true` for the
symbolic pair and `false` for the `x = 1` instance — a wrong answer on
DoenetML's grading path, reachable with no assumptions declared at all.
`x·ln(-1)`, `x·arcsin(2)` and `x + sqrt(-2)` are the same shape, as are
`x + 1` and `x·y` under an `x ∉ R` assumption.
The guard is now per subexpression, so a residual declines when any part of
it is provably non-real. That over-declines — a non-real part does not make
the whole non-real — which only ever leaves an expression as written, and
it turns no `None` into `Some(false)`, so no other rewrite moves. That was
the stated reason for not closing this gap in `combine`, and it still
holds: the fix is on the consumer. A grep for `Some(false)` outside
`src/assumptions/` finds no other realness consumer, so that was the whole
risk surface.
The regression test asserts on the tree, deliberately: an `equals`-based
assertion passes with the defect in place, because `equals` is what the
defect breaks.
Also: the js-compat CI job can report a regression now. It ended in
`|| true`, from when the suite was hundreds of failures deep, which made it
incapable of failing for any reason. The suite has been at exactly one
failure for several passes, and the eighth established that that one cannot
be made green without asserting something untrue — so it is `it.skip`ped at
its site with the reason, and the `|| true` is gone. 6,348 tests: 6,337
passing, 11 skipped, 0 failing, exit 0. `cargo test --workspace` 852.
…macOS build
Nine passes wrote tests; none checked that those tests could fail. This one did,
by reverting each production hunk and watching for red. Three on this side could
not fail, and each repair was verified the same way it was diagnosed.
`signed_zero.rs::indeterminate_forms_do_not_annihilate` asserted through
`simplify`, but its commit's only behavioural change was
`ops::preserve_order::is_non_finite`, on the `skip_ordering` path `simplify`
never touches — reverting that function wholesale left the test green. The four
wrapped spellings the commit named are now asserted on
`evaluate_numbers_preserve_order`, with the `simplify` assertions kept beside
them as the statement that one expression must not get two answers. Reverted, it
now fails with `left: "0", right: "{\"$\":\"NaN\"}"`. Worth recording: the
behaviour was not actually uncovered — `a_wrapped_infinity_blocks_annihilation`
in `src/ops/preserve_order.rs` already pins all four forms and also goes red.
The `substitute` coercion spec held both before and after its fix, which was
*simultaneity*. It now drives the two together: `substitute({x: "y", y: "x"})`
with string bindings must swap. Reverted to the left-to-right loop it fails with
`expected 'x + x' to be 'y + x'`.
`nested_gaussian_powers_are_bounded` put its `assert!(elapsed < 10s)` after the
loop that hangs, so with the bound gone the call never returns (still running at
400 s) and the assertion is never reached — the regression could only ever
surface as a CI job timeout under another job's name. Measured: the shipped test
body under that revert runs past 100 s and reports nothing. It now asserts on the
shape the bound controls, under a tightened `max_gaussian_pow_bits` so the
assertion is about the bound rather than the machine, and keeps the nested inputs
on a worker thread joined with `recv_timeout` so a hang is this test failing
rather than the suite wedging. Reverted, it fails in 0.08 s.
`build-wasm.sh` could not run on the bash macOS ships. It runs under
`set -euo pipefail` and expands `"${FEATURES[@]}"`, which is empty on every
invocation that is not `--debug`; expanding an empty array under `set -u` is a
fatal "unbound variable" before bash 4.4, and 3.2 is what macOS ships as
/bin/bash. Verified in a bash:3.2 container with cargo/node/wasm-bindgen stubbed:
before, the script exits 1 without ever reaching `cargo build`; after, both the
default and `--debug` paths invoke it with the right arguments. This reaches
DoenetML too — `packages/math/scripts/build-wasm.mjs` shells out to this script,
so `npm run build` there could not work on a stock macOS. `js-compat`'s
`build-wasm.sh` has the same shape and is not affected: its `targets` array is
refilled before it is expanded, and `"$@"` is special-cased even in 3.2.
cargo test --workspace 852 passed / 0 failed; cargo fmt and clippy clean;
quick_doenet_grading_gaps.spec.ts 7 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…where The branch taken for (negative)^(1/odd) depended on whether the radicand was a perfect power: (-8)^(1/3) folded to the real -2 while (-2)^(1/3) evaluated to the principal 0.6300 + 1.0911i, so equals told the same number apart from itself and four DoenetML <answer> cases that scored 1 on the legacy engine scored 0. Fixed at the three sites the tenth pass's recipe named: - rule_radical's Pow arm pulls the sign out at simplify time ((-2)^(1/3) -> -2^(1/3)), the load-bearing site: evaluate_to_constant runs simplify_core and the certified tape before any evaluator, so with the base positive the tape needs no new Op. - eval_complex's Pow arm takes the real branch for a negative real base under an exact odd-denominator rational exponent, matching also the raw quotient-node shape because that walk is evaluate_many's per-point fallback. - CBRT::eval1 / NTHROOT::eval2 follow, so the root spellings sample like the power spelling. Even roots, decimal exponents like (-8)^0.3333 (= 3333/10000), complex bases and the i-literal realness pins stay put. Pinned in tests/odd_root_real_branch.rs and quick_doenet_grading_gaps.spec.ts, both verified to fail against the unfixed engine (5/8 and 3/11). cargo test --workspace: 860 passed / 0 failed; compat suite: 6352 tests, 6341 passed, 11 skipped, 0 failing; fmt and clippy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
PR Doenet#84's description had grown to ~7,700 words of layered review findings — unversioned, ungreppable and unreadable. The entries that still describe the code move here as active-plans/PR84_REVIEW_KNOWN_ISSUES.md: the open known issues (each re-verified at its named pin or anchored to a symbol checked to exist at 41b9cb4), the standing invariants (no module-scope wasm dereference, the one sound skip, aggregate parser spellings), and the odd-root fix's contract. The pass-by-pass history stays in the git log and the PR's edit history; the PR body now fits on a screen and links here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
ROOT_SIMPLIFICATION_SPEC.md claimed grading was never at stake because equals evaluated everything principal — true for the even-root rows it settled, and exactly wrong for odd roots of non-perfect-power radicands, which is the gap the eleventh pass closed. A dated addendum records the extension of the real-branch rule to the evaluators; the even-root convention is unchanged. COMPAT_TEST_FAILURE_SUMMARY.md's totals line moves to the current pin (6,341/6,352, 0 failed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
Moving the known-issues ledger out of Doenet#84's body cost it two things. `wasm.ts` was left as a bare filename under a header that says `.ts` paths are relative to `packages/math-expressions-js-compat/lib/`. There is no `lib/wasm.ts`; the file is `packages/math-expressions-rs-wasm/src-js/wasm.ts`, which is the one entry that sits outside the convention. That contradicted the file's own promise that every entry carries a symbol anchor checked to exist. The standing-invariants bullet — the most important entry in the file — was run through a formatter that collapsed its code spans into the surrounding bold run, ate the spaces after every closing backtick, unindented the continuation lines out of the list item, and left a stray backslash inside `Context._assumptionsHandle`. Rewritten so it renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`functionConversions` maps AST heads onto math.js names. math.js spells this one `nthRoot`, and the map did not carry it — so `nthroot` compiled to a `FunctionNode` over an undefined symbol, which is not a compile error. It throws `Undefined function nthroot` on the first `evaluate`, i.e. once per sample, from inside whatever was plotting. So this is not "wrong at negative inputs". `nthroot(x, n)` was unevaluable through `f()` at every input: `nthroot(x,3)` at `x = 8` threw, where legacy answered 2. `f()` is the plotting and root-finding entry point, so a DoenetML `<function>nthroot(x,3)</function>` — ordinary authored DoenetML — drew nothing at all on this engine and drew fine on legacy. No test named it because the compat suite exercises `nthroot` through `evaluate_to_constant`, which has always handled it. Mapping it also puts an odd root of a negative on the real branch (`nthRoot(-8, 3) === -2`), which is what the eleventh pass's odd-root fix and `cbrt` already do, and throws for an even root of a negative, where `f()`'s callers turn the throw into `NaN` — the right answer for a point with no real value to plot. Verified by mutation: with the mapping removed the new case fails with `Undefined function nthroot`. Full compat suite green, 6,342 passing / 11 skipped. Worth a follow-up sweep: any other AST head math.js spells differently, or does not have, fails this same silent way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`erf` is the mirror image of the twelfth pass's `nthroot`, and just as silent.
`ERF` in `special_functions/misc.rs` carried parser spellings and LaTeX
rendering but no `eval1`, so every engine-side numeric path answered nothing:
`evaluate_to_constant("erf(0.5)")` was `None` and `evaluate_many` sampled `NaN`
at every point. `f()` was right the whole time, because it compiles to math.js
and math.js *has* `erf` — so a DoenetML `<function>erf(x)</function>` drew a
correct curve whose `<number>$$f(0.5)</number>` read `NaN` and whose extrema
search (which samples `evaluate_many`) found nothing. Legacy evaluated `erf`
from all of those paths, so this was a regression, not a gap.
`eval1` is now a port of the same W. J. Cody rational-Chebyshev approximation
math.js uses, so the two paths agree to the last bit rather than to a
tolerance. `tests/erf.rs` pins the reference values across all three of Cody's
intervals and asserts `evaluate_to_constant`, `evaluate_fast_f64` and
`evaluate_many` agree; the compat suite gets the DoenetML-shaped case. Both
verified to fail with `eval1` removed. `functions_registry.rs` moves `erf` out
of its "deliberately NOT evaluable" list, which is where the omission was
codified.
Also done: the sibling sweep the twelfth pass asked for. Every spelling the
Rust registry can produce was diffed against `Object.keys(mathjs)` and against
`functionConversions`, and all 69 author-typable spellings were evaluated
through `f()`. `nthroot` was the only head broken that way; `rootof` is
unmapped but unreachable (in neither of DoenetML's applied lists, and the
`critical_points()` output that produces it never reaches `f()`).
One documentation correction: the odd-root entry claimed the real-branch
convention as a blanket "deliberate divergence from mathjs". It is not blanket.
`f()` compiles a `Pow` node straight to math.js, so `f()` of `x^(1/3)` at
`x = -8` is the principal `1 + i√3` while `cbrt` and `nthroot` are `-2` — the
engine's own evaluators and its math.js compile path disagree about the power
spelling and agree about the root spellings. Measured, and now stated where the
claim was.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
Three claims in `active-plans/PR84_REVIEW_KNOWN_ISSUES.md` no longer described
what is there, found by a cross-document audit against the tree:
- The header said "eleven review passes" and anchored every entry to commit
`41b9cb4`, while the file already carried a twelfth-pass entry and three
thirteenth-pass ones.
- "The one skipped-with-reason compat test" is eleven: 9 in
`quick_trees.spec.ts` and 2 in `slow_assumptions.spec.ts`. All but one carry
a `[wontfix: …]` tag; the exception is named so it can be tagged.
- `1/(0^0)`'s tree was quoted as `["/",1,{"$":"NaN"}]`. Measured at this pin it
is `["/", 1, NaN]` — the envelope reaching `.tree` was the separate defect
that is already recorded as fixed. The substantive claim, that it does not
fold, still reproduces.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`equals` says a determinant is not equal to its own value.
`\det\begin{pmatrix}1&2\\3&4\end{pmatrix}` simplifies to `-2` and
`evaluate_to_constant`s to `-2`; `equals` between it and `-2` is `false`. `trace`
and `5` behave the same way. There is no spec for `det` equality and no DoenetML
test grades a determinant, so nothing is red.
The cause is the third numeric path. The thirteenth pass's sibling sweep covered
`f()` and `evaluate_to_constant`; `equals` runs on neither, it samples through
`eval_numeric/complex.rs`, whose `head_evaluable` asks `special_functions::eval1`
whether a head is evaluable. `det` and `trace` have no scalar kernel, so
`is_opaque_atom` classifies the whole application as a fresh unknown and samples
it as one — it agrees with `-2` at no point. The reduction that produces the
right answer is one layer away, in `normalize/fold_apply.rs`'s `det`/`trace` arm
over `crate::matrix::{det, trace}`, which `simplify` reaches and the sampler
never consults. Filed open with that diagnosis and a fix sketch; it is the only
grading-reaching divergence the review has left open.
That path also fails in a different *shape*, which is why the sweep did not
notice: a missing head there is an equality that answers `false`, not a value
that reads `NaN`. And `tests/functions_registry.rs` cannot catch the class —
it asserts the evaluable list evaluates and the deny list does not, so it pins
whatever is true rather than testing against an outside authority. A head that
ought to be evaluable, is not, and is written into the deny list passes. That is
exactly how `erf` was codified, and how `det` still is.
Also, the `erf` port re-measured independently over 38,385 sample points (both
tails, both Cody interval boundaries, denormals, ±0, ±∞, NaN): 0 mismatches
against math.js on the shipped wasm build, because wasm32 Rust and V8 use the
same fdlibm `exp`. A native `cargo test` build differs at ≤2 ulp (max relative
3.6e-16, all inside `erfc2`) because it links glibc's `exp` — a property of the
two `exp`s, not of the port, and ~2,800× under the 1e-12 grading tolerance. The
"last bit" claim now says which build it is about.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
The fourteenth pass found `equals` saying a determinant differs from its own
value and left it filed open, unable to establish it as a regression. It is one:
the legacy JavaScript library answers `-2`, `5` and `true` to every case here,
measured against `math-expressions@2.0.0-alpha94` built from `upstream/main`.
The divergence is also wider than reported — `evaluate_to_constant` of
`\det\begin{pmatrix}1&2\\3&4\end{pmatrix}` is `None`, not `-2`; the `-2` the
fourteenth pass measured came through a `substitute`, which simplifies on the
way. So `<number>` reads it as nothing and `<answer>` grades it wrong.
`matrix::scalar_reduction` is now the one place that decides whether an
application of `det`/`trace` has a scalar value, and both layers that need to
agree call it: `normalize/fold_apply.rs`, which keeps its `Num`-only gate so
`simplify` is byte-unchanged, and `eval_numeric/complex.rs`, which does not stop
at a number — so `det([[x,2],[3,4]])` samples as `4x-6` and compares equal to it.
`head_evaluable` takes the argument list rather than its length so it can ask.
`free_symbols` needed nothing: an application that is no longer opaque already
descends into its arguments, and the `Expr::Matrix` arm already walks entries. A
matrix the reducers decline — non-square, or over `resource_limits` — still comes
back as the `OtherOp` residual and is still sampled as an unknown, so `det` of a
2×3 matrix stays unequal to `0`, as in legacy.
`DET` also gains the scalar identity kernel `TRACE` already had. That is mathjs's
`det(2) = 2` convention, which legacy implemented too (`det(x) == x` is `true`
there and was `false` here), and a `Matrix` argument never reaches it.
On the question the finding implies — which other heads become opaque atoms on
this path — the registry has exactly two definitions with no evaluation at all,
and after this one only `rootof` is left. That one is unreachable: `canonicalize`
rewrites `Apply(rootof, …)` back into the `Expr::RootOf` leaf, which
`eval_complex` evaluates, and every sampling entry point canonicalizes first.
Pinned in `tests/matrix.rs` (including the discriminating negatives and the
declined non-square matrix) and in the compat suite, both verified to fail
against the unfixed engine, twice — once for the sampler hook and once for the
`DET` kernel. `tests/functions_registry.rs` had `det` written into its
"deliberately NOT evaluable" list, which is where the omission was codified,
exactly as `erf`'s was; `det` moves to the evaluable list and the deny list now
says in the file that it pins a decision rather than an outside fact, and names
the compat suite as the check that has outside authority.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…of is reachable
**An eighth grading defect, the same shape as the seventh.** The fifteenth pass
made `matrix::scalar_reduction` the one place that decides whether `det`/`trace`
has a value, because two layers deciding independently had answered differently.
Asking what *else* those two layers could disagree about found `f((a, b))`.
Legacy's text parser wrote one tree for `mod(7,3)` and `mod((7,3))` — a head
applied to a tuple — so the extra parentheses cost nothing and both answered `1`.
This parser keeps the spellings apart, and only `normalize/fold_apply.rs` put
them back together. The equality sampler did not, and `known_function("mod", 1)`
is false, so it called the application an opaque atom and drew a random value for
it: `simplify(mod((7,3)))` was `1`, `equals(mod((7,3)), 1)` was `false`, and
`evaluate_to_constant` was `None`. `<number>` read nothing and `<answer>` graded
it wrong. `nPr` and `nCr` are the other two heads whose folder takes the arity the
spread produces.
The spread is now `normalize::spread_list_argument`, `pub(crate)` and consulted by
both layers. `head_evaluable` asks it for the effective arity, `eval_apply`
evaluates the spread list, and `free_symbols` needed nothing for the same reason
it did not for `det`. The arity check still happens downstream, so `abs((-3,5))`
stays symbolic on *both* layers rather than being forced into a two-argument
`abs`. Aggregates are unaffected: they already spread, and now also evaluate
(`evaluate_to_constant` of a `sum` over a tuple was `None`).
**Two claims that had to be corrected before the fix could be the right one.**
An earlier pass had this in the open ledger as "`is_variadic` tests 'has an exact
folder' rather than 'is an aggregate'". Narrowing `is_variadic` to the aggregates
is what that reads as, and it was tried: it makes `mod((7,3))` stop being `1`,
which measurement against `math-expressions@2.0.0-alpha94` shows is a
*regression*, not a fix. The spreading is legacy parity; the sampler was the half
that was wrong. The entry's example, `["apply","mod",["tuple",7,3]]`, also never
took the branch — the JS deserializer flattens a tuple argument into an argument
list first, so a DoenetML tree could not reach it and only the text parser could.
**And `rootof` is not unreachable.** The fifteenth pass's commit message swept the
registry for heads with no evaluation, found `rootof` the only one left, and said
it was unreachable because "`canonicalize` rewrites `Apply(rootof, …)` back into
the `Expr::RootOf` leaf". The sweep's arithmetic is right; the unreachability is
not. The rewrite runs only when `from_apply_args` accepts, which needs a *dense
canonical* univariate polynomial, so `rootof((x-1)(x-2), 0)` and
`rootof(2(x^2-2), 0)` stay applications and compare unequal to `1` and to
`rootof(x^2-3x+2, 0)`, which is the same number. Filed rather than fixed, with the
reason stated: the residue is self-consistent — it equals itself and neither layer
claims a value for it — so this is a cross-spelling inequality, not the
simplifies-to-a-number-it-then-denies shape, and closing it means teaching
`expr_to_upoly` to multiply polynomials under a degree guard.
Pinned in `tests/equality.rs`, `normalize/fold_apply.rs` and the compat suite,
verified to fail against the unfixed engine. `fold_apply`'s aggregate test also
gains a directly-built tuple, because the `run_js` cases it had never reached the
spread branch at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
The sixteenth pass filed this rather than fixing it, and asked the seventeenth to
decide whether that holds. It does not — but two of the filing's own details had
to be corrected by measurement before the fix could take the right shape.
**`canon_apply` rewrites `rootof(p, k)` into the `Expr::RootOf` leaf only when
`from_apply_args` can read `p`, and `expr_to_upoly` read only a sum of
monomials.** Canonicalization does not expand products, so a *factored* spelling
was declined and stayed an application of a head with no evaluation at all — an
opaque atom. Two spellings of the same number therefore compared unequal:
`rootof((x-1)(x-2), 0)` was neither `1` nor `rootof(x^2-3x+2, 0)`, and
`rootof(2(x^2-2), 0)` was not `-sqrt(2)`. `expr_to_upoly` now multiplies and adds
polynomials.
**It is not "dense canonical" input that was required.** Sparse reads fine
(`x^2-2`), and so does a *scaled* one: `make_rootof` normalizes to primitive
integer coefficients with a positive leading coefficient, so
`rootof(2x^2-6x+4, 0)` and `rootof(x^2/2-3x/2+1, 0)` already equalled `1`, and
`-x^2+3x-2` did too. The one shape that failed was an unexpanded product —
`(x-1)(x-2)`, `2(x^2-2)`, `x(x-1)`. The ledger said "a factored or scaled one",
which is half right and points at the wrong half.
**And the obvious degree guard is a narrowing.** A first draft capped every arm
at `max_rootof_degree`; that makes `rootof(x^70 - x^69, 0)` stop being a leaf,
which the old reading accepted because `make_rootof` takes the squarefree radical
(degree 70 → `t^2 - t`) — measured, not reasoned about. The cap belongs on
products alone, which are the one operation here that costs more than the tree it
came from: multiplying many-term polynomials grows the coefficients as well as
the degree, and `(x^2+x+1)^200` spent ten seconds under a looser cap building
integers that were then thrown away. A monomial sum grows nothing, so it keeps
its old uncapped arm — which also makes the change a strict widening, with every
tree the old reading accepted reaching the same coefficients.
**Why fixed rather than filed.** The question the sixteenth pass left open was
whether a cross-spelling inequality on a grading path is shippable. The premise
turns out not to hold: `rootof` appears in neither of DoenetML's
`appliedFunctionSymbols` lists (`utils/math.ts` has no occurrence of the name),
so it cannot be typed into a `<math>` or an `<answer>`, and the leaves
`critical_points()` produces are canonical by construction. Nor is it a
regression — legacy `math-expressions@2.x` has no `rootof` at all. What is left
is a self-inconsistency in *this* engine's own new surface, which ships as
`math-expressions@3.x` to npm, where a library caller reaches it through the
default text parser (any name followed by a parenthesized list) and through
`\operatorname{rootof}` in LaTeX. That is worth 40 lines and a bounded blast
radius: `expr_to_upoly` has exactly one caller.
Pinned in `tests/rootof_adversarial.rs` — the widening, the refusals it must
keep (two variables, a non-polynomial factor, an oversized expansion), and the
monomial parity — with the widening verified to fail against the unfixed reading.
The unbounded dense allocation for a monomial of enormous degree
(`rootof(x^1000000000 - 1, 0)`) is pre-existing and moves to the open ledger
rather than being closed here, since capping it is the narrowing this change
deliberately avoids.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
The sixteenth pass widened `evaluate_to_constant` to `number | Complex | null`
after establishing that the engine really returns a math.js `Complex` for a
non-real constant — `fromText("i").evaluate_to_constant()` is `{re: 0, im: 1}`,
which the compat layer documents at the implementation. It widened DoenetML's
*vendored* copy of these declarations. It did not widen the original, which is
this package's published `types` entry, and whose header on the DoenetML side
says in so many words that "a change to either belongs in both".
So `math-expressions@3.x` would have shipped to npm declaring a return type that
cannot hold what it returns — the same defect, now in the artifact every future
consumer types against rather than in one snapshot of it. Both declarations
(`Expression` and the `Context`-level one) are widened here, and the two files
are byte-identical again on every declaration line.
`types/usage.ts` had codified the narrow contract: `const _constant: number |
null = fromText.evaluate_to_constant()`. That file exists to fail when a
declaration stops admitting a real call shape, and it was instead asserting the
shape that let a `{re, im}` object out of two DoenetML functions promising a
number. It now annotates what a consumer actually has to handle, so `npm run
typecheck` covers the widening rather than contradicting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…each
The sixteenth pass made `normalize::spread_list_argument` the one place that
decides whether `f((a, b))` is `f(a, b)`, and it closes the cases that reached
grading. Asking what the *other* heads do turned up the layer underneath, which
is not an equality question at all.
**`to_js` is not injective.** `f((x, y))` parses to `Apply(f, [Seq(Tuple,
[x, y])])` and `f(x, y)` to `Apply(f, [x, y])`; both serialize to
`["apply","f",["tuple","x","y"]]`, and `try_from_js` maps that JSON back to the
second. So the crate carries a distinction its own JS AST cannot express — and
that AST is the contract with every consumer. Measured through the built
package, `me.fromText("f((1,2))").tree` is byte-identical to
`me.fromText("f(1,2)").tree` while `x.equals(me.fromAst(x.tree))` is `false`:
an expression is not equal to itself after a round trip.
**Filed rather than fixed, and the reason is measured, not assumed.** It is not
a grading defect: DoenetML's `checkEquality` hands raw `.tree` values to
`check_equality`, which rebuilds both operands with `me.fromAst` one line before
`.equals()`, so an `<answer>` awards full credit for the extra-parenthesis
spelling on every head tried. What does survive is display — the same saved JSON
renders `\sin\left(\left( x, y \right)\right)` before a save/restore and
`\sin\left( x, y \right)` after, and `floor((x,y))` changes notation outright.
That is a regression against legacy, which had one tree for both spellings, but
it is a rendering one.
The fix belongs in the **parsers**, not in canonicalization — flatten a lone
`Tuple` argument at parse time, the way `try_from_js` already does — because the
printers read the raw tree, so a canonical-form fix would repair `equals` and
leave the display. That is a change with real reach into the round-trip suites,
which is why it is written up here in full rather than attempted at the end of a
pass. `spread_list_argument` still earns its place afterwards: `Seq(List, …)`
round-trips through JS intact and is reachable from a DoenetML tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
The seventeenth pass filed this in full rather than attempting it at the end of a pass. Taken on properly here. **`to_js` was not injective.** `f((x, y))` parsed to `Apply(f, [Seq(Tuple, [x, y])])` and `f(x, y)` to `Apply(f, [x, y])`; `to_js` writes `["apply","f",["tuple","x","y"]]` for *both*, and `try_from_js` reads that back as the second. The crate carried a distinction its own JS AST cannot express, and that AST is the contract with every consumer — so `x.equals(me.fromAst(x.tree))` was `false`, and the same saved JSON rendered `\sin\left(\left( x, y \right)\right)` before a save/restore and `\sin\left( x, y \right)` after. **The fix is in the parsers, not in canonicalization**, because the printers read the raw tree: a canonical-form fix would have repaired `equals` and left the display wrong. `parse::common::apply` flattens a lone `Tuple` argument exactly as `expr::serde::try_from_js` always has, and every `Expr::Apply` the two parsers build now goes through it — the call form, the simplified application, `|…|`, `⌊…⌋`, `⌈…⌉`, `√`, `∛`, `…!`, and the integral. Only a *lone* tuple flattens: in `f((x, y), z)` the inner tuple is one of two arguments, survives the round trip intact, and is left alone. Legacy had one tree for both spellings, so this is parity, not a new rule — verified against `math-expressions@2.0.0-alpha95` head by head. **Two things came out of doing it.** The corpus property that states the defect — *rendering is a function of the saved JSON* — turned up a second, unrelated instance while it was being written, and then ruled it out: the parsers keep raw associative grouping (`1+x+3` is `Add([Add([1,x]),3])`) while `to_js` flattens, but `to_text`/ `to_latex` flatten first too, so the rendering is stable. `tests/roundtrip.rs` calls the inner `text::convert` rather than the public `to_text`, which is why that suite never saw the difference; `tests/js_ast_image.rs` uses the public entry points. And the LaTeX printer's bracket notations were guarded on `args.len() == 1`, falling through to `head\left(…\right)` otherwise and spelling the head as a command that does not exist: `abs(x, y)` rendered as `\abs\left( x, y \right)` and `sqrt(x, y)` as `\sqrt\left( x, y \right)`, neither of which MathJax can render. That was reachable before this change from any stored tree, and the parser fix routes `|(x,y)|`, `floor((x,y))`, `sqrt((x,y))` and `(x,y)!` into it, so it is fixed here: `sole_argument` wraps the tuple, which is both what the JS AST says the argument is and exactly what legacy rendered. `nthroot` keeps its two-argument arm — its second argument is the index, not part of what the radical wraps. **`spread_list_argument` keeps its place, for the other list kinds.** `Tuple` no longer reaches it from either parser or from `try_from_js`, but `mod([7,3])` and `["apply","mod",["list",7,3]]` are still one sequence argument, so its tests move to the bracketed spelling — the one that can still fail if the branch is narrowed — and keep the parenthesized one alongside as a value check. Its doc, the sampler's comment in `eval_numeric/complex.rs`, and the compat spec no longer claim the parser keeps the two spellings apart. **Verified by revert-fail-restore in both directions**, since "this test cannot fail" is the live failure mode here: three of the four tests in `tests/js_ast_image.rs` fail against the unfixed parsers, and the fourth — the one asserting a tuple among *several* arguments is preserved — fails against the plausible over-flattening variant that spreads every tuple argument. The fixture files are machine-generated and none of them spells a lone tuple argument, so the corpus sweep carries six hand-written inputs; without them it passed with the fix reverted. `tests/formatter_fixes.rs`' new case fails against the old `args.len() == 1` guards. Full crate suite (840 tests), the compat suite (6347), `typecheck` and `verify:package` all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
…hat can rot
The eighteenth pass fixed the LaTeX bracket notations that fell through their
`args.len() == 1` guard. Verified here against `math-expressions@2.0.0-alpha95`
head by head — the parser flatten and the printer fix are both exactly legacy on
every case tried, including seventeen adversarial parses (nested applications, a
two-argument head receiving a tuple, `f((x,y),z)`, an integral with a
differential) — and three things the same guard was still hiding turned up.
**`nthroot` still fell through at any arity but two.** Its second argument is
the index, so the two-argument arm is right, but the fallthrough printed
`\operatorname{nthroot}\left(x\right)` where legacy prints `\sqrt{x}` — and
where `normalize::canonicalize` *already* rewrites `nthroot(x)` to `sqrt(x)`, so
the engine displayed as a function something it compares as a radical. At every
other arity there is no index to raise, so what is left is a plain radical over
the whole argument; that reproduces legacy for one, three, zero and non-tuple
arguments alike.
**`is_radical` was left behind by the eighteenth pass's own fix**, still testing
`args.len() == 1`. Once `sqrt`/`cbrt` learned to wrap a multi-argument list they
rendered as radicals that `is_radical` no longer recognized, so the parentheses
a radical gets under a power went missing: `\sqrt{\left( x, y \right)}^{3}`
where legacy writes `\left(\sqrt{\left( x, y \right)}\right)^{3}`. The
disagreement is silent — the radical still renders — so it needs the two to be
written to agree, which they now say they must.
**The text printer had the same guard on the same two notations** it shares with
LaTeX, so a multi-argument `abs`/`factorial` lost its notation entirely and
printed `abs(x, y)` where legacy wrote `|(x, y)|`. Only the LaTeX half was
fixed.
Twenty of twenty-one printer outputs now match legacy byte for byte; the
twenty-first is a pre-existing `list`-argument corner that legacy renders no
more faithfully. Each of the three tests was verified by reverting *its own*
source change alone.
**A compat expectation had been rewritten away from the JS output, and the
reason does not hold.** `quick_ast-to-latex.spec.ts` pinned
`\operatorname{nthroot}\left(2\right)^{3}` in place of the library's
`\left(\sqrt{2}\right)^{3}`, because `\sqrt{2}` "did not re-parse to the same
head". It does not — but the engine does not keep that head either, and
`canonicalize` makes the two one expression before anything compares them. Put
back; the header note now records the reversal rather than the claim.
**The ledger was stale in four places and nothing could catch it.**
`JS_RUST_TEST_DIVERGENCES.md` said 36 latex / 82 text divergences against a
snapshot holding 15 / 56, and 14 simplify known-failures against a fixture
holding 4 — because it *copied* the enumeration the snapshot owns, and each
divergence fixed over eighteen cycles had to be deleted from the prose by hand.
Twenty-one of its listed latex cases no longer diverge at all. It no longer
copies the cases: the snapshot is machine-generated and test-enforced, so the
ledger now carries the counts and a breakdown by cause and points at the file.
And `output_established.rs` now checks the counts the ledger does keep — both
section headers, both summary-table rows, and that each per-cause table accounts
for exactly the divergences measured. It `include_str!`s the ledger, so moving
or renaming it breaks the build rather than disabling the check. Verified to
fail on each of the three: a wrong header count, a wrong table row, and a
cause table that sums short.
Full crate suite 843 (was 840, +3), compat suite 6347, `cargo fmt --check`,
`clippy -D warnings`, `typecheck` and `verify:package` all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9QoEYrYzcLeJKxWoeheK8
`evaluate_to_constant` answered `null` for a free variable, a blank `＿` and a
placeholder hole (`0·_`, `_/_`), and `NaN` only for an indeterminate form. The
split was deliberate — "cannot be evaluated" really is a different thing from
"evaluates to NaN" — but `null` is the wrong way to carry it into JavaScript,
and it was carried into every consumer whether or not the consumer had asked.
`null` is *anti*-poisoning: `Number(null)` is `0`, `null + 5` is `5`,
`null <= 1` is `true`, `null >= -3` is `true`, `Number.isNaN(null)` is `false`.
So an expression with no value did not blow up, it behaved like zero, in
anything that had not been individually taught to test for it. Measured against
`math-expressions@2.0.0-alpha95`, legacy answers `NaN` for every one of these
inputs; the `nan_for_non_numeric: true` default was the whole contract. This
package's stated job is to be a drop-in replacement for that API, so returning
`null` where legacy returned `NaN` was a compatibility bug rather than a design
choice. The Rust core keeps `Option<f64>`, where it is right and where nothing
coerces.
The tail of `evaluate_to_constant` is now one `return NaN`, and `UNIT_NAMES`,
`treeHasBlank` and `treeHasBareBlank` go with it — they existed only to decide
which of the two answers to give. That also retires the known issue about those
helpers' comments describing the wrong trees: there are no such helpers now.
Two other things about the return type, since a NaN sentinel alone does not
tell a consumer what to narrow:
- The complex case stays. `fromText("i").evaluate_to_constant()` is a math.js
`Complex`, as legacy's was, so a caller passing the result into math.js gets
it intact. The published declarations now say `number | Complex` — dropping
`| null`, which was the lie — and say in as many words that
`typeof x === "number" && !Number.isNaN(x)` is the narrowing that works and
`x !== null` never was.
- `match` returns `false` when the pattern does not match, and always did, but
was declared `MatchResult | null`; a `!== null` test written against that
declaration was unconditionally true. Also declared its second parameter as
`allow_permutations?: boolean` when the implementation takes an options
object and routes it through `normalizeMatchOptions` — a bare `true` takes
the *no-options* path, so the declared call silently did nothing. Both fixed,
with a real `MatchOptions` interface.
`evaluate_to_complex` keeps answering `null`, and that is not the same
inversion: it has no legacy counterpart to be compatible with, and its range
already contains `Complex(NaN, NaN)` as a genuine value (`Infinity*i`), so
`NaN` cannot double as its marker. It is not part of the published declarations
either. `evaluate_many` already filled gaps with `NaN`. Documented in place.
`nan_for_non_numeric` is now `@deprecated` in the declarations: it is accepted
and ignored, and what it is ignored *in favor of* is legacy's own default.
Tests pin the new contract at the point where the old one fails —
revert-fail-restore verified, four tests across the two specs. The added one
checks the property that actually matters: that the marker survives being
computed with (`+`, `*`, `/`, `Number()`, `<=`, `>=`, `Number.isNaN`,
`Number.isFinite`), which is the whole reason `NaN` is the right marker and
`null` was not.
Suite: 6,359 tests, 6,348 passing, 11 skipped. The README's counts were stale
by ten before this change; corrected.
Two declaration-vs-implementation lies were found in the twentieth pass, both
while doing something else. That is not a search strategy, so this pass walked
`types/math-expressions.d.ts` member by member against `lib/`, running the built
package rather than reading the source.
**Nine more declared parameters the engine has no arity for.** `simplify`,
`simplify_logical`, `collect_like_terms_factors`, `simplify_ratios` and `expand`
are all declared with options and take none — a legacy `simplify(assumptions)`
compiles and drops them. `derivative`'s `story` array is never written.
`equalsViaReal`/`equalsViaComplex` ignore their `EqualsOptions`, so their
tolerances have no effect. `isAnalytic` is declared to accept a `string[]`,
which the implementation reads as an options object, so every flag comes out
`false` and the call takes the strictest path — `match(true)` a second time,
exactly. And `Context.toString(expr)` answers `"[object Object]"`: the
expression-first mirror deliberately skips `Object.prototype` members so
`String(me)` keeps working, and the declaration promised it anyway.
All nine now say so — `@deprecated`, "accepted and ignored", the way
`nan_for_non_numeric` already did — the `string[]` arm is gone, and
`Context.toString` is no longer declared. That is honesty and not a fix: the
engine should either honor these or they should be dropped. Filed, with six
further divergences that need a decision about which side is wrong
(`Context.assumptions`, `get_assumptions`, `solve_linear`'s
`ABSENT_EXPRESSION`, `Context.from`, `create_discrete_infinite_set`,
`Context.class`, and `Expression.match` dropping `allow_extended_match` where
the free `utils.match` honors it).
**Two were failures rather than divergences, and are fixed.**
- `add_unit` passed its argument straight to a wasm entry point typed `&str`,
and wasm-bindgen reads a non-string argument as a pointer/length pair into
linear memory. So `add_unit(me.fromText("%"))` — the call the declaration
invites, and legacy's own signature — gave `RuntimeError: memory access out
of bounds`, and an array tree gave `arg.charCodeAt is not a function`. The
fix is the `varName` coercion `critical_points` already used against the
identical hazard, in a comment one method away. The rest of the string-taking
entry points were swept; all were already guarded. Pinned in
`quick_doenet_open_items.spec.ts`, revert-fail-restore verified.
- `evaluate()` marshals its bindings through a `Float64Array`, so a `Complex`
binding — which `Bindings` declares as legal — silently became `NaN`, and the
declared `Complex` *return* never occurs. Now declared
`NumericBindings -> number`, with `f()` named as the path that handles both.
**The suite state in `PR84_REVIEW_KNOWN_ISSUES.md` said "at this head" and had
not been re-derived since the thirteenth pass** — 862 / 6,354 / 6,343 against a
real 876 / 6,363 / 6,352. The twentieth pass corrected the identical numbers in
the sibling README and left this one, which is the file the PR body names as a
reviewer's entry point. Corrected, with a note telling the next editor to re-run
them rather than carry them.
Every run behind these numbers was redirected to a file with the vitest or cargo
process's own exit status checked, never a pipeline's: `cargo test --workspace`
876 passed / 0 failed, compat suite 6,363 tests / 6,352 passing / 11 skipped / 0
failing, `typecheck`, `cargo fmt --all -- --check` and
`clippy -D warnings` all clean.
**The twenty-first pass's `add_unit` fix reported sweeping "the rest of the
string-taking entry points; all were already guarded". It had missed the two
biggest.** `parse_text` and `parse_latex` — `me.fromText` and `me.fromLatex` —
take `&str`, and wasm-bindgen reads a non-string as a pointer/length pair, so
`fromText(5)`, `fromText(anExpression)` and `fromText({})` were all
`RuntimeError: memory access out of bounds`, and an array tree
`arg.charCodeAt is not a function`. The module recovers, so this is a bad error
rather than a corrupted heap — but it is an engine-internal one, and DoenetML
renders the parser's complaint into `<mathInput showPreview>`.
They now throw a `TypeError` naming the argument type. A *throw* where
`add_unit` took a coercion, because `add_unit`'s declaration invites an
`Expression | Tree` and a unit is a symbol, while `fromText` is declared to take
a string and nothing else has a faithful reading — so nothing that used to
succeed changed. `String` objects still parse, as wasm-bindgen always read them.
Twelve cases in `quick_doenet_open_items.spec.ts`, revert-fail-restore verified.
The rest of that sweep was re-run at runtime rather than re-read: every
string-taking wasm entry reachable from the published surface was probed with an
`Expression`, an array tree and a number, and each is guarded.
**The two decisions the twenty-first pass deferred, decided.**
*The 48 declared-but-absent `Expression` members: narrowed, not documented.* They
were `undefined` at runtime and mirrored on `Context`, so TypeScript accepted
`expr.sin()` and it threw. A declaration whose job is to describe a drop-in earns
nothing by promising members that are not there: keeping them made the failure a
compile-time *success* and a runtime `TypeError`, which is the worse of the two
places to find out, and removing them moves the report to `tsc` and names the
member. All 96 are gone, plus `Context`'s own `ZmodN` and `parser_parameters` —
Context-only properties, so outside the `Expression` audit that measured the 48.
Every member either interface still declares — 66 on `Expression`, 87 on
`Context` — was checked present at runtime on the built package. The gap is
unchanged and is still the ask upstream: the names are enumerated in a comment at
the end of `Expression`, and one goes back the moment `lib/` implements it.
*The six unresolved divergences: one verdict each, all measured first.* Five were
the declaration being wrong about a deliberate implementation and now say what
the code does — `Context.assumptions` is the object of methods it is;
`get_assumptions` takes the three query shapes that work (a name, a *nested*
`[["x","y"]]` list, an expression) and returns `Tree | undefined`, the bare
`["x","y"]` it used to declare being the one shape that answers `undefined`, and
legacy's own suite queries `[["x"]]`; `Context.from` and
`create_discrete_infinite_set` are declared `| undefined`; `Context.class` takes
a wasm handle, declared `never` so `new me.class(tree)` is a compile error rather
than an object whose every method fails. `solve_linear`'s `ABSENT_EXPRESSION` is
**accepted** and says so: legacy handed back an `Expression` to read `.tree` off,
so `| undefined` would break the callers the stand-in exists for.
The sixth was **fixed**. `Expression.match` dropped `allow_extended_match` while
the free `utils.match` honored it — the option is handled outside the Rust
matcher, and `Expression.match` called that matcher directly while its comment
claimed the two entry points could not drift. It delegates to the shared
implementation now, which is what makes the claim true; `MatchOptions` declares
the option because it works from both; and the no-options path is still gated on
`hasOptions`, so the legacy default where every string leaf binds is unchanged.
`x+y+z` against `a+b` bound `b` to `y+z` here and to `y` with `_skipped: ["z"]`
there; both answer the second now. Revert-fail-restore verified.
The nine "accepted and ignored" parameters were re-verified against the built
package and the verdict stands: each is a parameter legacy honored and this
engine has no arity for, so the alternatives are engine work or breaking a legacy
call that compiles today.
Every run behind this was redirected to a file with the process's own exit status
checked: compat suite 6,383 tests / 6,372 passing / 11 skipped / 0 failing,
`cargo test --workspace` 876 passed / 0 failed, `typecheck`, `verify:package`,
`cargo fmt --all -- --check` and `clippy -D warnings` all clean. The suite state
in `PR84_REVIEW_KNOWN_ISSUES.md` and the sibling README is re-derived, not
carried.
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
2 participants
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.
Upstream half of Doenet/DoenetML#1622, which pins this branch as its
vendor/math-expressionssubmodule. It completes the Rust core (packages/math-expressions-rs) and itsmath-expressionsdrop-in (packages/math-expressions-js-compat) far enough that DoenetML runs its whole suite on this engine. Mostly hand-written Rust plus compat-layer additions; the only fixture changes are deletions of known-failure entries.Status: CI green at the head (
46f4c49), including thejs-compat testandpackage publishabilityjobs, and that is the commit #1622's submodule pin names.package publishabilitypacks the tarball, installs it into a throwaway project outside the workspace and drives it through the--target webwasm export, sonpm publishfrompackages/math-expressions-js-compatproduces a working package.What it adds for DoenetML —
fromAstaccepting anExpressionat any depth,Expression#f(), the context-level operation family,setWasmModuleinjection,dopri, a realdefault_order,solve_linear, and indeterminate forms folding toNaN— is pinned inspec/quick_doenet_compat_pr84.spec.ts; the grading-path fixes are inspec/quick_doenet_grading_gaps.spec.tsand the review's own fixes inspec/quick_doenet_open_items.spec.ts. Two contract changes are worth knowing before reading them:evaluate_to_constantanswersNaN, nevernull, for an expression with no numeric value (legacy's answer —nullcoerces to0in JavaScript, and that inversion was the root of most of the grading defects the review found), and its declared return type isnumber | Complex.Reading order for review:
active-plans/PR84_REVIEW_KNOWN_ISSUES.mdfirst — the surviving known issues, the standing invariants, and what was fixed during review; none of the open items block #1622. Thenlib/math-expressions.ts(the compat surface; note the "no module-scopewasmdereference" invariant atContext._assumptionsHandle), thennormalize/simplify.rsandeval_numeric/for the engine's branch conventions. Engine-vs-JS output divergences are counted and broken down by cause inactive-plans/JS_RUST_TEST_DIVERGENCES.md, whose numberstests/output_established.rsenforces against the machine-generated snapshot.One branch convention, and it is not uniform. On the engine's own numeric paths an odd root of a negative real is real —
x^(1/3)at-8is-2, not mathjs's principal1 + i√3(pinned intests/odd_root_real_branch.rs). That does not extend tof(), which hands aPownode to math.js and keeps mathjs's principal branch, whilecbrtandnthrootare real in both. The gap is unchanged from the legacy library, so it is a standing difference rather than a regression; closing it means mapping the odd-rootPowshape ontonthRootintree-to-mathjs.ts.Residual risk: there is no differential grading harness on either side of the seam, so the ordinary suites are all that guard semantic divergence in grading. The DoenetML review found eighteen wrong-answer-on-grading defects that no existing test named; all are fixed, several of them here, and the confidence argument is structural — the recurring cause was fixed at its source — rather than exhaustive. The per-finding detail is in
active-plans/PR84_REVIEW_KNOWN_ISSUES.mdand, downstream, inMATH_EXPRESSIONS_ENGINE_NOTES.mdandMATH_EXPRESSIONS_UPSTREAM_REQUESTS.md. The pass-by-pass history is in the git log (Review cycle N:commits) and this description's edit history (GitHub web UI only).🤖 This description was written by an agent during review; the findings ledger lives in
active-plans/PR84_REVIEW_KNOWN_ISSUES.md.