fix: reject non-binary trees at the TreeState boundary

[ms609]

Fixes #16
Fixes #24

#16 (T-400) — the defect

TreeState::init_from_edge derives n_tip = (n_edge / 2) + 1, which identifies a tree only when it is binary. Both parities broke, differently:

• odd n_edge — n_node_derived == n_edge, one short: parent[c] = p writes one int past the end, and pi = p - n_tip reaches n_internal, making left[pi]/right[pi] out-of-bounds writes.
• even n_edge — indices stay in range, but n_tip_derived < n for every non-binary tree, so parent[n_tip] = n_tip roots the tree on a real tip. build_postorder then yields a one-element postorder and ts_fitch.cpp forms prelim.data() - total_words — the score is read from memory before the buffer.

The fix

Refused at the kernel boundary, not per caller. ts::edge_list_is_binary() checks the shape from the edge arrays alone, before anything is written: even edge count >= 2, every parent an internal index, every child in range and not the root, each child claimed once, each internal claiming at most two children. Since n_edge == 2 * n_internal by construction, "at most two" forces "exactly two".

init_from_edge throws std::invalid_argument carrying the message "tree must be binary", which Rcpp forwards as an R error at every export boundary — so all 24 kernel entry points, including the exports with no R caller, are fixed by one check.

Two call sites need more than that:

• build_topology_tree() (src/ts_rcpp.cpp:271) clones the same derivation for the least-squares path. The issue listed it as "do not touch" because R/LeastSquares.R:64 guards with ape::is.binary, but that guard is at the R level only and the C++ hole is reachable from TreeSearch:::ts_ls_fit. Its own header documents the identical rooted-binary convention, so it now uses the same predicate.
• startEdge ingestion (src/ts_rcpp.cpp) is screened on the main thread, because init_from_edge also runs on a std::thread worker where an uncaught throw would terminate the session rather than raise an R error.

R-level guards were added to TreeLength.list(), .CheckTreeCharLen(), TreeScore() and EdgeListScore() so users get the same message TreeLength.phylo() has always given. FastCharacterLength() is deliberately left unchecked — its documentation promises no checks — and the kernel now gives it the same message anyway.

The bundled Shiny app scores every tree it displays and filters the pool on those scores, so it now searches with collapse = FALSE. It was already failing today whenever the returned pool was heterogeneous ("Trees have different numbers of edges") and silently displaying wrong numbers when it was not.

Not implemented: polytomy scoring. Upstream ms609/TreeSearch#259 asks for that as an enhancement and asserts as background that the API already refuses; this is a bug inside today's contract, and ms609#259 stays open.

#24 (T-411) — comment only

reset_states' T-261 audit covered score_tree / fitch_na_score only. The collapse kernels are a third consumer: they compare whole rows by memcmp, spanning words no pass writes. The audit now names them, and names every path that zero-fills the state arrays — assign in init_from_edge, ts_sector.cpp and ts_constraint.cpp; resize on a fresh TreeState's empty vectors in ts_fuse.cpp — rather than only init_from_edge.

The T-382 one-sidedness comment is corrected, but not in the direction the issue proposed. The issue holds that the stale words are zero and so make equality easier. That is false for condition 3: load_tip_states memcpys every word for every tip regardless of active_mask, and perturb_zero never touches ds.tip_states, so a tip sibling always carries its real states while its parent holds zeros or stale values. Equality is made harder, exactly as the original comment said — its reasoning is restored. The "always zero, always equal" case is real only for conditions 4-5, where down2 / subtree_actives are never written for non-NA blocks; that is where it now lives.

Regression tests

tests/testthat/test-ts-t400-multifurcating-guard.R (Tier 2). Pre-fix 16 failures, post-fix 41 pass, 0 skip — verified against temp-library installs of origin/cpp-search and of this branch, not load_all.

Pre-fix output:

FAILURE ':42:5'  Expected TreeLength(structure(list(tr), class = "multiPhylo"), dat) to throw a error.   [odd parity]
FAILURE ':44:5'  Expected TreeLength(list(tr, tr), dat) to throw a error.
FAILURE ':42:5'  Expected TreeLength(structure(list(tr), class = "multiPhylo"), dat) to throw a error.   [even parity]
FAILURE ':44:5'  Expected TreeLength(list(tr, tr), dat) to throw a error.
FAILURE ':55:3'  Expected TreeLength(trees[1], dat) to throw a error.
FAILURE ':68:3'  Expected unique(outcomes) to have length 1.  Actual length: 2.
FAILURE ':73:3'  Expected CharacterLength(.OddPolytomy(), dat) to throw a error.
FAILURE ':74:3'  Expected CharacterLength(.EvenPolytomy(), dat) to throw a error.
FAILURE ':80:3'  Expected TreeScore(tr, dat) to throw a error.
FAILURE ':81:3'  Expected EdgeListScore(tr[["edge"]][, 1], tr[["edge"]][, 2], dat) to throw a error.
FAILURE ':84:3'  Expected TreeScore(ev, dat) to throw a error.
FAILURE ':85:3'  Expected EdgeListScore(ev[["edge"]][, 1], ev[["edge"]][, 2], dat) to throw a error.
FAILURE ':97:3'  Expected TreeSearch:::ts_fitch_score(...) to throw a error.
FAILURE ':124:3' Expected Score(Edge(5,1, 5,6, 6,2, 6,7, 7,3, 7,3)) to throw a error.   [duplicate child]
FAILURE ':126:3' Expected Score(Edge(5,1, 5,6, 6,2, 6,5, 7,3, 7,4)) to throw a error.   [root as child]
FAILURE ':128:3' Expected Score(Edge(5,1, 5,6, 6,2, 6,7, 1,3, 1,4)) to throw a error.   [tip as parent]

[ FAIL 16 | WARN 0 | SKIP 0 | PASS 20 ]

Line 68 is the determinism test — the symptom the issue reports as 12, 9, 12, 12, 12.

The startEdge test does not merely fail pre-fix, it takes the session down:

$ Rscript -e '... ts_driven_search(..., startEdge = polytomy[["edge"]])'   # origin/cpp-search
Segmentation fault
pre-fix exit code: 139

Post-fix the same call raises "tree must be binary", on both the serial and the nThreads = 2 path; a binary startEdge is still accepted on both.

Review

Three independent external-reviewer lenses (conventions, cold bug-scan, history/blame). Findings applied: the second derivation clone, the T-382 direction (above), the four zeroing sites, an over-claiming header (edge_list_is_binary proves a degree spectrum, not tree validity — a cycle unreachable from the root still passes, and build_postorder's T-327 backstop catches that), the Shiny app, NEWS scope, and the untested startEdge pre-check.

Refuted after measurement, so unchanged:

• "MaximizeParsimony(inapplicable = "xform") now errors on its own default output" (rated a blocker). compute_collapsed_flags returns all-zero flags for HSJ/XFORM (T-330), so those trees are never collapsed. Measured on Vinther2008: Nnode 22 on 23 tips — fully binary — score 79. The same reasoning covers the vignettes/search-algorithm.Rmd and ?MaximizeParsimony promises that TreeLength() reproduces the reported score, which are scoped to those two modes and remain true.
• Vignette breakage. The profile.Rmd search chunks were run against both builds: identical output, trees binary.
• Root-numbering over-rejection. ape::checkValidPhylo() also treats a root other than nTip + 1 as fatal, and pre-fix such a tree was mis-scored regardless.

Checks

• Rscript .claude/tools/compile-attrs.R and Rscript check_init.R — arg counts match; no new Rcpp export (edge_list_is_binary is plain C++), so TreeSearch-init.c is untouched.
• spelling::spell_check_package(vignettes = TRUE) — clean, using the invocation tests/spelling.R uses.
• No roxygen block or function signature changed, so man/ is untouched.
• No src/Makevars.win, .o or .dll left behind.
• No search-behaviour change, so vignettes/search-algorithm.Rmd is not triggered.

CI status

ubuntu-arm64 is green — full R CMD check, Status: OK, FAIL 0 | WARN 4 | SKIP 10 | PASS 11754 (run 30928649665). The 4 warnings are pre-existing.

windows fails in dependency setup — Could not solve package dependencies: MaxMin: Can't find package called MaxMin — before any code is compiled. This is not from this branch: cpp-search itself failed identically (run 30909722952), as did every other branch that afternoon. #45 fixes it. This branch should be re-checked once that merges.

Known gaps, deliberately not addressed here

• ?MaximizeParsimony's collapse parameter does not mention that collapsed trees cannot be scored. Adding it means a roxygen change and man/ regeneration; NEWS carries the guidance for now.
• ts_compute_splits (src/ts_rcpp.cpp:1131) hand-derives node counts from a caller-supplied n_tip and is likewise unguarded — a smaller instance of the same family, with no polytomy-reachable R caller.

Merge note

A sibling chip edits src/ts_rcpp.cpp:1829-1949 and src/ts_collapsed.cpp:24-25, 153-156. This branch touches different regions of both files; nothing outside its own hunks was reformatted or reordered.