fix: OOB read in HSJ token_states/CanonOrder lookup

[ms609]

Fixes #51

Root cause — one defect, one site

fitch_label_char() in src/ts_hsj.cpp labels each secondary character over a
traversal rooted canonically at tip 0. build_canon_order() stores that
traversal's children CSR-style: co.kids flattened, with co.kidOff[n] /
co.kidNum[n] delimiting node n's slice.

kidOff[n] is written when the DFS pops n:

co.kidOff[n] = static_cast<int>(co.kids.size());   // ts_hsj.cpp:99

For a childless node that is simply "wherever kids had got to". Every node
except the canonical root becomes some node's child exactly once, so kids
ends at n_node - 1 entries — and the last node the DFS pops necessarily
has no unseen neighbours (or it would have pushed them), so its kidOff
equals the final kids.size().

The uppass loop then did:

for (int i = static_cast<int>(co.post.size()) - 1; i >= 0; --i) {
  int node = co.post[i];
  int nk = co.kidNum[node];
  const int* kid = &co.kids[co.kidOff[node]];   // ts_hsj.cpp:302 (pre-fix)

co arrives as const CanonOrder&, so that is
std::vector<int>::operator[](size()) const — dereferencing one past the end,
which is exactly the reported signature (_Tp = int, const reference).
The downpass (:189) and the tie-break accumulation (:256) both already
continue on nk == 0; this third loop did not.

The fix adds the same guard. kid is only ever read by the immediately
following for (int k = 0; k < nk; ++k), whose body cannot execute when
nk == 0, so skipping the iteration is observationally identical.

The two reproducers are the same defect, not two

The issue's second reproducer (test-ts-xform.R, aborting after exactly five
assertions) was reported as possibly not involving inapplicable = "hsj".
It does. Counting the file in order:

test	expectations
Xform prefers single gain + losses over multiple gains	3
Xform penalizes secondary variation on present branches	2
HSJ and xform agree on optimal tree for simple gain scenario	aborts before its first expectation

The third test's first statement is
MaximizeParsimony(ds, hierarchy = h, inapplicable = "hsj", ...). Five
assertions then an abort is precisely that. Both reproducers enter
score_hierarchy_block() → fitch_label_char() and die at :302.

I separately ruled out the other lead named in the issue: ts_hsj_score()
validates tip_labels against contrast.nrow()/contrast.ncol() before
make_dataset() runs, and build_dataset() assigns
ds.token_states = token_states (sized n_tokens = contrast.nrow()) and
ds.n_levels = n_states verbatim, explicitly un-remapped for HSJ
(src/ts_data.cpp:56-59). The pre-construction bounds are therefore exactly
the post-construction ones; that path is sound. No OOB was found on the
pure-XFORM/Sankoff path either.

Before / after under -D_GLIBCXX_ASSERTIONS

Both libraries built from a tarball into a private library per AGENTS.md, with
the flag in PKG_CPPFLAGS (~/.R/Makevars.win zeroes PKG_CXXFLAGS).
grep -c _GLIBCXX_ASSERTIONS on each build log: 34, both.

# "before" = origin/cpp-search @ 2c59965e6 in a detached worktree
TMPBUILD=$(mktemp -d)
(cd "$TMPBUILD" && R CMD build --no-build-vignettes --no-manual --no-resave-data <src>)
PKG_CPPFLAGS="-D_GLIBCXX_ASSERTIONS" \
  R CMD INSTALL --library=<lib> --preclean "$TMPBUILD"/TreeSearch_*.tar.gz

BEFORE (trunk, 2c59965e6)

$ NOT_CRAN=true Rscript -e "library(TreeSearch, lib.loc='<lib-pre>');
    testthat::test_file('tests/testthat/test-tree_length.R', reporter='summary')"
tree_length: ...................................! No informative characters in `dataset`.
.! No informative characters in `dataset`.
......................C:/rtools45/.../include/c++/bits/stl_vector.h:1149:
std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::operator[](size_type) const
[with _Tp = int; _Alloc = std::allocator<int>; const_reference = const int&;
size_type = long long unsigned int]: Assertion '__n < this->size()' failed.

$ NOT_CRAN=true Rscript -e "library(TreeSearch, lib.loc='<lib-pre>');
    testthat::test_file('tests/testthat/test-ts-xform.R', reporter='summary')"
ts-xform: .....C:/rtools45/.../include/c++/bits/stl_vector.h:1149:
std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::operator[](size_type) const
[with _Tp = int; ...]: Assertion '__n < this->size()' failed.

AFTER (this branch)

$ NOT_CRAN=true Rscript -e "library(TreeSearch, lib.loc='<lib-post>');
    testthat::test_file('tests/testthat/test-ts-xform.R', reporter='summary')"
ts-xform: ....................................................................[134 dots]

══ DONE ════════════════════════════════════════════════════════════════════════

Matched sweep of every hierarchy-touching test file, same two libraries:

test file	before	after
test-tree_length.R	abort	77 passed, 0 failed
test-ts-xform.R	abort	138 passed, 0 failed
test-ts-hsj.R	abort	147 passed, 0 failed
test-ts-resample-hierarchy.R	abort	74 passed, 0 failed
test-ts-hsj-xform-guards.R	9 passed	9 passed
test-recode-hierarchy.R	53 passed	53 passed
test-ts-t330-collapse-hsj-xform.R	8 passed	8 passed
test-CharacterHierarchy.R	34 passed, 5 error	34 passed, 5 error

The five test-CharacterHierarchy.R errors are identical either side and are
an artefact of my invocation, not of the package: that file calls internals
(.BuildTipLabels, .HierarchyToBlocks, .NonHierarchyWeights) unqualified,
which library() + test_file() cannot see but R CMD check's namespace
environment can.

No scored value moves

The bug is a bad reference, never a bad read — kid is unreachable when
nk == 0 — so the fix should be score-neutral, and is. 900 lengths from
matched plain (non-assertions) builds of trunk and this branch:

• 648 random matrices × trees: 4/5/6/8/11/16 tips, 12 matrices each, 3 random
  rooted trees each, hsj_alpha ∈ {0, 0.5, 1}, plus the XFORM length of the
  same tree;
• the issue's own 4-taxon matrix on balanced, pectinate and 13 random rootings,
  HSJ and XFORM;
• three seeded end-to-end MaximizeParsimony() searches (HSJ and XFORM) on the
  6-taxon matrix from test-ts-xform.R.

identical(pre, post) on the whole result frame is TRUE: 900/900 equal,
max absolute difference 0 over the 897 finite scores (range 1–25), and the 3
Inf entries match as Inf on both sides.

Regression test

None added, deliberately. The defect is a one-past-the-end reference that is
never dereferenced, so it has no observable effect in an ordinary build — a
normal-build test would pass identically before and after the fix and would be
tautological. The existing HSJ suites are the regression test; what was
missing was a build that checks bounds. ASan.yml is that gate, and per #51 it
is red on trunk for this defect alone; it is dispatched on this branch and its
result is the real proof. No permanent -D_GLIBCXX_ASSERTIONS CI job is added
here — that is an infra decision outside this issue.

Checks

• spelling::spell_check_package() and tests/spelling.R — clean; no
  inst/WORDLIST addition needed.
• check_init.R — 50 init.c entries, 48 RcppExports entries, all shared arg
  counts match (unchanged; the 2 manual entries are pre-existing).
• .claude/tools/compile-attrs.R not run and devtools::check_man()
  not run: neither trigger fires. fitch_label_char() is a file-static
  C++ function with no Rcpp export and an unchanged signature; no roxygen
  block, R signature, or documentation prose changed.
• src/ts_rcpp.cpp and src/TreeSearch-init.c untouched, so the append-only
  rule is not engaged. No src/Makevars.win created or left behind.
• Search behaviour unchanged, so vignettes/search-algorithm.Rmd needs no
  update.

Diff is 7 lines of src/ts_hsj.cpp (1 statement + 6 comment) and a NEWS entry.

CI

• agent-check — 30973482216 — success (ubuntu-arm64, windows), run on this branch's final commit.
• ASan — 30971403379 — success, all three legs (tests/examples/vignettes). This is the workflow HSJ scoring reads past the end of a std::vector<int>, caught by -D_GLIBCXX_ASSERTIONS #51's third comment reported red on trunk for this exact defect (stl_vector.h:1282, identical _Tp = int const-operator[] signature) — it now passes.

Reviewed

Three independent external-reviewer passes (load-bearing / scoring-semantics / conventions), all recommending ship. Two follow-ups were filed rather than folded in, since both are out of this issue's scope:

• HSJ tip_labels row count is unvalidated at both Rcpp bridges #58 — a second, unguarded tip_labels row-count bound at the same two Rcpp bridges, same assertion signature, confirmed independently against a build already carrying this fix. Not reachable from the public API.
• No CI leg builds with libstdc++ assertions, so container-bounds bugs are undetectable pre-merge #60 — no CI leg builds with -D_GLIBCXX_ASSERTIONS, so this defect class (visible to neither ordinary builds, Valgrind, nor ASan's own instrumentation — only to libstdc++'s own precondition checks) has no automated gate short of the gcc-ASAN container incidentally carrying the hardened library.

Review also corrected the PR's own mechanism description (a run of childless nodes carries the stale end-offset on every HSJ call, not "the last node" alone — see dev/red-team/reviews/feature-hsj-oob-read/repro-02-canon-order-invariant.R) and its ASan phrasing (the job dies on the libstdc++ assertion, not on ASan's own instrumentation, which does not watch address arithmetic).