C++ search#210
Draft
ms609 wants to merge 861 commits into
Draft
Conversation
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
Previously, attr(dat, "weight") <- c(0.5, 1.7) silently truncated to
c(0L, 1L) at the Rcpp boundary, dropping 50%/41% of the respective
characters' contributions without warning.
The C++ scoring engine still stores int weights internally; .ScaleWeight()
(R/fractional-weights.R) converts fractional inputs to integer with a
configurable precision (default 0.001 via getOption("TreeSearch.fractional.scale")).
Integer weights pass through unchanged.
Applied at the R-level chokepoints that read at$weight before calling
the C++ engine: tree_length.R (3 sites), MaximizeParsimony.R, Morphy.R,
AdditionTree.R, SuccessiveApproximations.R.
Motivated by AutoPart's centrality-weighted parsimony feasibility study
(dev/notes/centrality-weighting.R in auto-part), which derives per-character
weights from AMI-distance centrality on the [0.1, 1] interval.
Note: this is the R-level fix. A future patch could widen
ts_rcpp.cpp's IntegerVector weight signatures to NumericVector and
push the conversion (or true double-weight scoring) into the engine.
The hot Fitch path uses int multipliers throughout; that change is
substantially more invasive than this one.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
R CMD INSTALL refuses files that aren't listed in Collate; required for the fractional-weights patch to install cleanly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The resampling code in ts_resample.cpp expands each pattern weight[p]
times into a flat char_index vector, then casts its size to int. With
scale×1000 fractional weights on a large matrix this cast can overflow
to a negative value, producing an out-of-bounds array access (SIGSEGV).
Two complementary guards:
* R level (.ScaleWeight): after computing scaled integer weights, check
sum(as.double(scaled)) > .Machine$integer.max and stop() with an
actionable message pointing at options("TreeSearch.fractional.scale").
* C++ level (resample_search): accumulate sum(original_weights) in
size_t before the expansion loop and Rf_error() if it exceeds INT_MAX.
Also guard against negative individual weights.
Tests added for both paths (R error and C++ Rf_error paths).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Add withr to Suggests: tests use withr::with_options(). * Add Rcpp to inst/WORDLIST: spell check flagged it as unknown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Avoids a Suggests entry for withr just for two test helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
INT_MAX is defined in <climits>; the guard added in the previous commit failed to compile on stricter toolchains (GCC 14 on Windows runner). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rlang 1.2.0 is on CRAN, which restored compatibility with R-devel's PREXPR removal (the shim was promised to come out at rlang >= 1.1.8). Recent runs confirm the package builds cleanly in the gcc-asan container, so the temporary continue-on-error is no longer needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Both .Rmd files existed under vignettes/ but were missing from the articles section of inst/_pkgdown.yml, causing pkgdown::build_articles (invoked by memcheck/vignettes.R) to abort. Added under "Advanced use". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace phangorn::phyDat() with TreeTools::MatrixToPhyDat(), which already handles the "-" inapplicable token and is in Imports (so always installed during memcheck/ASan vignette builds). Matches the construction TreeSearch uses internally (e.g. AdditionTree.R:122). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Set rogueInstalled <- requireNamespace("Rogue", quietly = TRUE) in the
setup chunk, then wrap each Rogue::-using chunk with an if/else that
falls back to a message(). Lets the vignette render under memcheck/
ASan builds where Suggests aren't installed.
TreeTools::RoguePlot does not call into the Rogue package, so the
restore-wiwaxia chunk is unchanged.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wrap the minimax-linkage clustering block with requireNamespace(). Sets hSil <- -Inf and hCluster <- NULL as fallbacks so the downstream bestMethodId logic works unchanged when protoclust is absent (e.g. during CRAN checks where Suggests may not be installed). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Set rogueInstalled in the load-trees chunk. Wrap the entire exploring-consensus chunk body (Rogue feeds plottedRogue/consTrees/ tipCols which all feed RoguePlot, so they must be gated together). In cluster-consensus, gate only the two Rogue lines inside the loop and substitute character(0) / black tip colours as fallbacks so the consensus plotting continues without rogues. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Accept fractional per-character weights via attr(dataset, "weight")
In R CMD check, R runs as a non-interactive subprocess with captured stdout. R_FlushConsole() calls fflush() on that pipe; when the buffer fills the call blocks indefinitely, causing the 6 h GHA timeout seen on every ubuntu runner for PR #210. Gate the \r-overwrite progress line and the flush behind R_Interactive (FALSE in batch/check contexts). Interactive sessions are unchanged. At verbosity >= 2 in batch mode, emit plain \n-terminated lines so diagnostic logs still carry progress detail without the flush risk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…taset S-RED area-5 finding: ts_sector.cpp's build_reduced_dataset() did not copy flat_blocks or all_weight_one from the full DataSet. Since use_flat = ds.all_weight_one is checked at the top of every tbr_search() call, sector TBR always used the slower non-flat path even when all characters have weight=1. Fixing all_weight_one without flat_blocks would have triggered UB (flat functions dereference flat_blocks.data()); both fixed together. Also: - profiling round 1: Ratchet area (#2) PROFILED — 62% of search time via verbosity=2 phase data; VTune not available for per-function hotspot; T-300 (lazy rescore) is the actionable follow-up - focus-areas.md: NNI-perturb → AT-LIMIT (disabled via T-274), Ratchet → PROFILED with 2026-05-18 baseline (2.80 s/rep Zhu2013 thorough) - red-team.md last_focus → area 5 with full findings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…31/T-332 work # Conflicts: # src/ts_collapsed.cpp
…ecord Keeps the reusable rssPicks SearchControl knob (exposes existing SectorParams::rss_picks_per_round; default 0 = byte-identical) and the mission-gate harness+FINDINGS documenting the sectorial-escape thread's closure (mechanism resolved, wall-lever ruled out). See dev/benchmarks/missiongate/FINDINGS.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test-ViewChars.R and test-Distribution.R were silently broken since the app was modularised: they drove consensus inputs by pre-modularisation unqualified ids (keepNTips, consP, outgroup, concordance ...), so every consensus interaction no-oped and the committed baselines had drifted stale (still citing the long-removed MorphyLib refs). Nothing caught this because these tests are not run in CI. Also fixed mapLines, which is a top-level input (ui.R) that had been wrongly namespaced treespace-. - Namespace every input to its real id (consensus-*, mapLines top-level); regenerate and certify the ViewChars/Distribution value baselines (verified the app now reaches each intended state). - Port the legacy tests/shinytest/*.R download-content coverage: add savePlotZip script snapshots via expect_download() at the same states, with a shared normalize_download() transform (setup.R) that is CRLF-safe and scrubs dates/versions/system lines. Teeth verified: a meaningful script change is detected; a volatile version change is scrubbed. - SearchLog: the saveZip session log embeds non-portable search results (trees[[N]], allTrees[1:M], nThreads); assert deterministic log-generation markers instead of a brittle full-content snapshot, and check saveNwk/saveNex structurally. This ports v1 intent in a CI-portable form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The three legacy shinytest scripts (Distribution.R, SearchLog.R, ViewChars.R) plus their -expected snapshots and the shinytest.R runner are now fully superseded by their shinytest2 AppDriver counterparts under tests/testthat/ (interaction coverage, plus the download-content coverage ported in the previous commit). The v1 runner could no longer execute anyway: shinytest (v1) was dropped from DESCRIPTION Suggests some time ago (only shinytest2 remains), so `library(shinytest)` errored on the first line. codemeta.json still lists shinytest; that file is generated (see .github/workflows/codemeta.yml) and will drop the entry on its next regen from DESCRIPTION, so it is intentionally left untouched here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prepares the three integration tests to run under GitHub Actions.
- setup.R gains a shared new_app_driver() factory and a wait_stable() retry
wrapper. wait_stable() re-attempts app$wait_for_idle(), which can transiently
error ("An error occurred while waiting for Shiny to be stable") right after a
heavy dataset load; a first-try success incurs no delay.
- Disable expect_values() browser screenshots everywhere
(expect_values_screenshot_args = FALSE) and drop the committed .png baselines.
The screenshot is flaky even run-to-run on one machine (anti-aliasing / render
timing) and carried no reliable signal; the value (json) + download-content
snapshots carry the coverage.
- Route value snapshots through a thin expect_vals() wrapper (a seam for a
future normalising transform) and add the .md download-filename baselines that
the previous commit missed (they live at the _snaps root, not the per-test
subdir).
Note on portability: expect_values() hashes the app's rendered plot output, so
the value snapshots are only portable on the OS that generated them. The shiny
CI job therefore runs on Windows (the development platform) rather than Linux,
which keeps CI output consistent with the committed baselines.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restructure R-CMD-check.yml from a single 6-config matrix into fail-fast waves,
mirroring ../MaxMin:
sense-check (ubuntu-24.04-arm, release) -- cheapest leg, NO concurrency group
so a rapid follow-up push never cancels it; every commit gets a build signal.
core (needs sense-check) -- Windows (+codecov), R 4.1 (highs pin), R devel;
per-leg concurrency cancels superseded in-flight legs.
full (needs core) -- macOS x2 backstop; the ~10x-runner-minute legs only run
once core is green (in practice, about to merge).
All six original check legs are preserved, just reorganised into waves. There is
deliberately no workflow-level concurrency (see header comment).
New: a `shiny` job running the EasyTrees shinytest2 suite, gated on BOTH the
sense-check passing AND the app having changed (a `changes` job with
dorny/paths-filter feeds its `if:`, since workflow-level `paths` cannot combine
with `needs:`). It runs on windows-latest -- the development platform -- because
expect_values() hashes the app's rendered plot output, so the value snapshots
are only portable on the OS that generated the baselines. It is
`continue-on-error: true` (NON-BLOCKING) until its stability on CI is
established, and uploads any *.new.* snapshot diffs as an artifact for triage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EasyTrees shinytest2 repair/port/CI (local) + sectorial mission-gate (origin); disjoint file sets, automatic merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + Δ-probe expand_and_reinsert (T-266, 2026-03-27) used the union-of-finals fitch_indirect_length_bounded approximation that the June directional fix (#26/#27) replaced everywhere else — it was MISSED. Port it to the exact edge_set scorer, mirroring ts_wagner.cpp:487: - per-tip compute_insertion_edge_sets (prelim is current via wagner_incremental_rescore) + fitch_indirect_length_cached per edge. Also a gated, non-perturbing -DTS_SCOREAPPROX_PROBE oracle that tallied Δ = exact_cost(E_bounded) − min_E exact_cost(E) per placement. On Zanol (forced pruneReinsertCycles): bounded chose strictly-worse edges in ~62% of placements, mean ~6 steps, ~48% greedy-regret SHARE. After the port the probe reports Δ=0 at every placement (production == exact argmin). NOTE: this is greedy-regret SHARE, not realizable wall-clock — prune_reinsert auto-enables ONLY at nTip>=120 (`large` preset); NO mission dataset reaches 120t (max=88). So this path runs on zero default mission searches. Land + time-matched A/B (NNI-polish `large` preset, ≥120t) is COMPOSITION-gated (#40). Worktree-only; NOT for cpp-search until composition. Tests: prune-reinsert 44/0, drift 22/0, ratchet 17/0, tbr 28/0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-search data race Post-review hardening of the exact-directional scorer port. The -DTS_SCOREAPPROX_PROBE diagnostic block's function-local `static` counters (sa_placements/sa_delta_*/sa_*_exact_sum) are mutated with no synchronization, but expand_and_reinsert runs concurrently on parallel-search workers — an unsynchronised data race (adversarial review, CONFIRMED low). thread_local gives each worker its own tally (per-thread partials under multithreading; exact for the single-threaded diagnostic runs this probe is intended for). Production (probe undefined) is byte-identical and unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…w coverage gap) Adversarial review of the exact-directional scorer port found no prior test drove expand_and_reinsert on NA (inapplicable) or IW (implied-weights) data, nor asserted score self-consistency in any regime — yet prune_reinsert_search runs on both (its guard only early-returns for PROFILE/HSJ/XFORM). The port is a ranking-only construction heuristic; the accepted tree is gated by a regime-correct score_tree() (strict-improve + revert), so the REPORTED best_score must equal an independent length recompute of the RETURNED tree in every regime. Adds two tests (Vinther2008, 23t, forced pruneReinsertCycles=3): NA-EW and IW (concavity=3) — each asserts result$best_score == ts_fitch_score(returned tree). Both green. Empirically confirms the port cannot corrupt the accepted tree on NA/IW (mirrors the ts_wagner.cpp all-regime convention, not the ts_tbr.cpp guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sert (T-266) Lands 41b0d2373's exact edge-set insertion scorer in expand_and_reinsert (replacing the union-of-finals fitch_indirect_length_bounded approximation; mirrors the June directional fix at ts_wagner.cpp), plus branch post-review hardening: 4007f8e port (byte-identical to 41b0d2373); 0fb6de9 thread_local probe accumulators (parallel-search data-race fix, production byte-identical); 98122e5 NA + IW score-soundness regression tests (Vinther2008). The 'NOT for cpp-search until composition (#40)' note in 4007f8e's body is superseded: landing the correctness fix is decoupled from the #40 time-matched A/B. Dormant path (prune-reinsert auto-enables only at nTip>=120 / large preset; no default mission search reaches 120t) so nil current wall-clock risk. Verified: clean consistent rebuild + targeted tests 123 pass / 0 fail (incl new NA/IW soundness + wagner-quality). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the non-portable value snapshots from the ViewChars/Distribution AppDriver tests. app$expect_values() embeds rendered-plot output (a data-URI image hash plus float plot geometry) that differs machine to machine even on the same OS, so it could never green on GHA. Replace each expect_values() with wait_stable() (retrying wait_for_idle wrapper) and keep the portable coverage: the savePlotZip download-script snapshots (which encode app state), the interaction path itself, and SearchLog's log markers. Regenerate all download baselines; delete every _snaps/*/*.json. Rewrite test-app-smoke to load a bundled dataset (Sun2018) before asserting "N trees in memory" -- a fresh boot has data-dataSource="file" with nothing loaded, so the old assertion was checking a state the app never reaches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The testServer module tests drifted from the app code while the suite was CI-unwired: * test-mod-search.R carried a hand-copied SearchConfidenceText frozen at an older signature; global.R's real one grew stopReason/replicateScores (consensus/timeout stop notes + Chao1 coverage note). The module now passes those args, so every observeEvent(input$implied.weights) that calls DisplayTreeScores() errored with "unused arguments" and aborted before completing the T-165 run-stats reset -- cascading into the reset/cancel-file assertions (656-658, 519). Re-sync the stub verbatim to global.R and flag it KEEP-IN-SYNC. * TreeLength()/ScoreSpectrum() are TreeSearch exports but the file only attached shiny, so they were "could not find function". Attach TreeSearch. * The consensus stop-note assertion pinned "consensus stable"; the shipped text is "consensus tree unchanged across recent replicates". Align the assertion to the real output. (See note to maintainer: confirm this wording is intended, not an accidental UX regression.) * test-mod-data.R expected tipLabels() to be NULL with no trees, but mod_data.R deliberately guards `return(character(0L))`. Align the expectation. Verified: test-mod-search PASS=66 FAIL=0, test-mod-data PASS=13 FAIL=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ARNs - global.R: consensus stop-note now reads "Search stopped: consensus stable across recent replicates." (terse, descriptive); test-mod-search stub kept in sync + assertion pinned to "consensus stable". - test-mod-clustering.R: library(shinyjs) so the clThresh observer resolves runjs() (was 3 "could not find function" WARNs). - test-mod-consensus.R: suppressWarnings around two isolated-harness artefacts (transient non-numeric signif / length-zero if) irrelevant to the assertions. CI fails on WARN; whole EasyTrees suite now FAIL 0 | WARN 0 | PASS 192. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(constraint): reject impossible constraints, guard collapsed-split OOB (T-329) An impossible (non-laminar) constraint reached random_constrained_tree() unfiltered: .PrepareConstraint() only filtered splits by size, never checked pairwise compatibility. In random_constrained_tree(), a split that loses all its tips to tighter non-laminar splits collapses (split_root = -1); if it still has a strict-superset parent, the parent's child-split loop pushed that -1 as a phantom item, causing tree.parent[-1] and tree.left/right[n_tip-1] out-of-bounds writes. Primary fix: .PrepareConstraint() now validates pairwise split compatibility (disjoint or nested) and stops with a clear error before reaching the C++ builder. Defensive fix: guard split_root[j] >= 0 in random_constrained_tree()'s child-split collection, mirroring the existing root-level guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci(ASan): switch mem-check runner to ubuntu-latest (x86) The r-hub gcc-asan container image has no arm64 variant; on ubuntu-24.04-arm the container dies at init before any R work runs (every run since 2026-06-19 failed in ~30-40s). Cherry-picked from TreeSearch main's fix (PR #262) so the T-329 fix can be verified under a working ASan CI. * ci(asan): install phangorn for tests leg, exclude MaxMin from vignettes leg The tests leg only installs hard deps + Config/Needs/memcheck (Suggests are skipped for tests/examples), but tests/testthat/*.R make heavy unguarded use of phangorn::phyDat() etc. -> "there is no package called 'phangorn'". Add it to Config/Needs/memcheck so it installs regardless of leg. The vignettes leg installs all immediate Suggests via a single pak::pkg_install() batch, which resolves plain package names against CRAN/Bioconductor only - it doesn't see the local DESCRIPTION's Remotes mapping. MaxMin is GitHub-only (Remotes: ms609/MaxMin), so that batched solve fails outright with "Can't find package called MaxMin", taking every other Suggest down with it. Exclude MaxMin, mirroring the existing Rogue exclusion: its only use (WideSample()) is requireNamespace-guarded and no vignette/example exercises it. * fix: guard zero-length memcpy in TreeState::load_tip_states Datasets whose characters are all uninformative (e.g. every observed state is a singleton) collapse to zero blocks and a zero-length tip_states vector in build_dataset(). std::vector::data() on an empty vector may legally return nullptr, which memcpy's nonnull-parameter attribute forbids — flagged by UBSan (gcc-ASAN run 28662381835) as a null-pointer violation reached via ts_char_steps() -> init_from_edge() -> load_tip_states(). Skip the copy when there is nothing to copy. Covered by the existing "ConcordantInformation() works" regression test in test-Concordance.R, which already exercises an all-singleton dataset via ConcordantInformation() -> CharacterLength() -> ts_char_steps(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(constraint): use four-gamete split compatibility, not laminar-subset (T-329 follow-up) T-329 (19f4f97) added a laminarity gate to .PrepareConstraint() that tested each constraint character's "1"-side as a full bipartition vs everything-else and accepted only disjoint-or-nested pairs. This rejected SATISFIABLE constraints: it (1) folded wildcard "?" and unconstrained tips into the "0" side though they are free to plot either side, and (2) tested only three of the four split-compatibility cases, missing the case where the two "0" sides are disjoint (the "1" sides jointly cover the constrained tips). The gcc-ASAN CI (run 28662381835) surfaced this on the pre-existing Morphy test and the tree-search vignette "complex-constraints" chunk (ab|cef & abcd|ef, g free) -- both displayable on ((a,b),(d,(c,(e,f))))+g. T-329's own "impossible" example {t1,t2,t3}&{t3,t4,t5} was likewise satisfiable (coexist on ((t1,t2),t3,(t4,t5))). Fix: build a companion 0-group matrix and apply the four-gamete test on both groups (wildcards excluded) -- incompatible iff all four group intersections are non-empty. Valid constraints return to their working pre-T-329 path; genuinely-impossible ones still error. The C++ OOB guard (ts_wagner.cpp split_root[j] >= 0) is the real crash fix and is retained unchanged. Tests: corrected the first T-329 case to a genuinely four-gamete-incompatible constraint ({t1,t2,t3}&{t2,t3,t4}); added a positive regression asserting the non-laminar-but-compatible case is accepted. Verified locally: Morphy suite, vignette chunk, and 446 constraint tests all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(wagner): guard zero-Fitch-words start-tree build (all-hierarchy HSJ/xform) When every character belongs to a hierarchy block, the recoded equal-weights dataset has zero blocks, so DataSet::total_words == 0 and the per-word Fitch state vectors (ds.tip_states, edge_set) are empty. wagner_tree() unconditionally took the address of element 0 of those empty vectors (&ds.tip_states[tip * total_words] == &ds.tip_states[0], and likewise &edge_set[node * tw]) to feed the incremental insertion-cost machinery. That is undefined behaviour (address of a past-the-end element of an empty vector) and aborted under the hardened libstdc++ assertions in the gcc-ASAN CI (run 28662381835, ts-xform group; _GLIBCXX_ASSERTIONS operator[] __n < size()). With no Fitch characters every insertion cost is identically zero, so guard the per-word work behind have_words = ds.total_words > 0: skip the edge-set precompute and evaluate the indirect length as 0, while still running the DFS so constraints are honoured and the first legal edge is chosen. The search then optimises the tree from the Sankoff (xform) / hierarchy-DP (HSJ) term alone. The have_words == true path is behaviourally identical to before. Regression tests (test-ts-xform.R, test-ts-hsj.R) run an all-hierarchy dataset through MaximizeParsimony() so the hardened/ASAN CI covers the zero-words path. Verified locally with a -D_GLIBCXX_ASSERTIONS -O0 -g build: the whole ts-xform group (incl. SK-01, which previously aborted) and ts-hsj group now pass with no abort; a normal equal-weights search is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…harness Investigates whether the T-327 guard `build_postorder().size()==n_internal` (impose_one_pass, src/ts_constraint.cpp:735) is equivalent to structural validity, and whether a net-zero corruption can slip it to reach the advertised std::bad_alloc from MaximizeParsimony(constraint=). Adds dev/red-team/heavy-tests/impose_validity/ — a standalone C++ harness (no R/Rcpp/Morphy/SIMD) that enumerates all (2n-3)!! rooted binary trees for n_tip=4..8, runs the verbatim kernel functions (topology_spr, collect_edges_*, find_maximal_subtrees, compute_node_tips extracted from HEAD at build time) + the real build_postorder, and models the whole impose_constraint loop. Findings (T-333, P3): the guard is NOT a full validator — it admits reachable net-zero TYPE-1 left/right corruption (double-ref + orphan) that DOES reach the DFS helpers, so its comment is inaccurate. But the hypothesised P1 is REFUTED: 0 root-reachable left/right cycles AND 0 parent[] cycles across ~1.33M accepted-invalid trees (no std::bad_alloc on either vector, incl. the pre-verify tbr_search->reroot_at_tip path), and 0 corrupt final trees survive the callers' map_constraint_nodes verify-and-discard (no wrong answer). Fix = replace the size-check with a full left/right arborescence check (O(n)). Files: findings.md + to-do.md (T-333), log.md (area-13 round entry). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tor (T-333) The T-327 backstop in impose_one_pass's try_move — postorder.size() == n_internal (commit 6b60f23) — is not equivalent to structural validity. topology_spr's root-child degenerate case can emit a net-zero corruption (one internal node double-referenced +1, one orphaned -1) that lands on exactly n_internal and slips a size check, then feeds a malformed tree to the next reanchor DFS and to reroot_at_tip's parent walk. (The hypothesised std::bad_alloc P1 was refuted — no reachable cycle on either vector — and no malformed tree was ever returned; correctness rested entirely on the callers' verify-and-discard. This makes the guard self-sufficient.) Add structurally_valid() (O(n): child slots in range + left!=right; in-degree 1 for every non-root node / 0 for root; a root DFS visiting every node exactly once; parent[] the exact inverse of left/right). try_move now validates before rebuilding the postorder and reverts on any corruption. Performance: the check is confined to impose_one_pass/try_move, which runs ONLY when constraint= is passed to MaximizeParsimony. TreeState::build_postorder (97 hot-path call sites) is untouched and keeps its > n_internal cap as the bad_alloc backstop, so unconstrained search pays nothing; within a constrained run it is the same O(n) complexity class as the build_postorder already there. Verification: - Exhaustive harness (dev/red-team/heavy-tests/impose_validity/) re-pointed at the real, verbatim-extracted structurally_valid: probe() now asserts it equals the independent full_validity() oracle on every tree, with a g_guard_mismatch gate (exit 3 if they ever diverge — the durable CI signal against reverting to a size-only check). Result: 0 disagreements across all (2n-3)!! rooted binary trees n=4..8 (every net-zero witness the old guard admitted is now rejected). - R integration test added (test-ts-impose-constraint.R "T-333: structural guard survives root-child repair, honours constraint") — the stricter guard neither crashes nor over-rejects valid repairs through the linked engine. - Package builds clean (-Wall -pedantic); full test file green. findings.md row removed (resolved); to-do.md T-333 -> FIXED (pending CI); log.md area-13 round annotated. GHA CI (ASan + full check) pending on push. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fitch_na_score()'s first-uppass tip loop iterates every tip 0..n_tip-1 and dereferences tree.final_[tree.parent[tip] * total_words + ...]. That assumes every tip is attached, but a partial tree can legitimately be scored while some tips are detached: expand_and_reinsert() (prune-reinsert perturbation, T-266) maps the optimised reduced backbone into the full node space and calls score_tree() BEFORE the dropped tips are Wagner-reinserted. Those dropped tips still carry the parent == -1 sentinel from init_wagner_state(), so anc == -1 and the index becomes (-1) * total_words == -total_words, a wild negative subscript (size_t-wrapped). That is out-of-bounds on tree.final_ / tree.prelim. Only the NA path trips this: the EW path (fitch_uppass) and every other pass in fitch_na_score (Pass 1, Pass 2 internals, Pass 3) walk tree.postorder, which contains only attached nodes, so they already ignore detached tips. The NA first-uppass tip loop was the lone place reading tree.parent[] for all tips unconditionally. It aborted under the hardened libstdc++ assertions in the gcc-ASAN CI (run 28681669404, ts-prune-reinsert group; _GLIBCXX_ASSERTIONS operator[] __n < size(), long unsigned int == uint64_t on x86-64). Under a normal build the same read silently touches out-of-bounds heap. Fix: guard the tip loop with `if (anc < 0) continue;`, making the NA uppass honour the same attached-nodes-only contract the EW path already follows. The guard is inert for a complete tree (every tip's parent is >= n_tip), so a normal search is behaviourally identical. Detached tips get their prelim/final recomputed by wagner_incremental_rescore() when they are reinserted, and the accept-gate score_tree() runs on the completed tree, so scoring is unaffected. Regression coverage: the NA and IW score-self-consistency tests in test-ts-prune-reinsert.R (both drive prune-reinsert on the inapplicable Vinther2008 dataset; IW reuses it via fitch_score_ew -> fitch_na_score) exercise this exact path and previously aborted before their assertions ran. Verified locally with a -D_GLIBCXX_ASSERTIONS -O0 -g build: test-ts-prune-reinsert.R now completes 52/52 with no abort (was exit 134), and the ts-na-* / ts-driven groups still pass with no assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 638a6ce)
TS_NA_INCR_AUDIT is an opt-in dev env var (never set by any user-facing code path) that cross-checks the native-NA incremental dirty rescore against full_rescore per TBR candidate. The Rcpp::stop()-based mismatch check is still load-bearing -- it's the sole assertion behind test-ts-na-incremental.R's "TS_NA_INCR_AUDIT cross-check runs clean" test -- so that stays. The na_incr_n/na_incr_ok counters and the REprintf summary they fed existed only to print "N/N candidates byte-matched" once the sweep finished. Because na_incr_ok is only ever incremented immediately after the mismatch check, and any mismatch throws Rcpp::stop() before that increment runs, the ratio can never be anything but N/N -- the message is tautological and was never read by anything (not the test, not a caller). Pure vestige of the incremental dirty-rescore validation work, now that the optimisation is production default; same pattern as the DEBUG_RESCORE/DEBUG_NA_RESCORE/DEBUG_NNI_RESCORE cross-checks removed once validated per T-304. No behavioural change. Verified: clean -O2 rebuild, test-ts-na-incremental.R (91/91) and test-ts-tbr-search.R (28/28) pass.
…y (T-328, T-323) (#261) simplify_patterns() indexed token_states[tip_data-1] with no range check, so an out-of-range tip_data value from a hand-crafted matrix passed to the unexported ts_wagner_tree()/ts_random_wagner_tree()/ts_resample_search()/ ts_parallel_resample()/ts_successive_approx()/ts_simplify_diag() entry points read out of bounds (0 -> before the array; > n_tokens -> past the end, likely a segfault). Add a shared validate_tip_data_values() check at the single Rcpp boundary and its four duplicated call sites (Rcpp::stop on violation), and give ts_simplify_diag its first weight/levels length guards while there. Bundled with the sibling T-323 gap: ts_wagner_tree's addition_order was converted 1-based -> 0-based with no length/range/duplicate check, so a short vector read past its end (reproduced segfault) and an out-of-range or duplicated value corrupted the tree via OOB heap writes. Add validate_addition_order() mirroring the existing weight/levels/min_steps guards. Only reachable via TreeSearch:::, not the public API (AdditionTree()/ MaximizeParsimony() always build valid inputs from a checked phyDat).
…R#264) (#265) Any dataset that collapses to 0 Fitch blocks (total_words == 0) -- not just the all-hierarchy HSJ/xform case PR #264 fixed in wagner_tree/load_tip_states, but also plain equal-weights data where every character is constant or an autapomorphy -- leaves tip_states/prelim/edge_set genuinely empty std::vectors. Code that unconditionally took the address of element 0 of these empty vectors is undefined behaviour: it aborts under hardened libstdc++ assertions/ASan but is silent (out-of-bounds address, never dereferenced if the following block loop is also 0-trip) in a plain release build. Audited every zero-word-reachable site sharing this pattern and guarded each with the same have_words ternary wagner_tree already uses (never bare nullptr+0 arithmetic, which UBSan's pointer-overflow check would still flag): - ts_sector.cpp: compute_from_above_for_sector (called unconditionally on every build_reduced_dataset(), i.e. every sector reduction including the default rasStarts=1 -- the most severe of these, reachable with no non-default control), build_ras_sector (the RAS-restart start-tree builder, reachable via SearchControl(rasStarts>=2)), and build_reduced_dataset_collapsed (reachable via sectorCollapseTarget>0). - ts_prune_reinsert.cpp: build_reduced_dataset and expand_and_reinsert, reachable via pruneReinsertCycles>0 (auto-selected by the "large" preset at nTip>=120). - ts_temper.cpp: stochastic_tbr_phase (annealing), same "large"-preset reachability, needed its own top-level total_words==0 early return matching nni_search/tbr_search/ratchet_search/drift_search's existing style. - ts_tree.cpp: TreeState::save_node_state's prealloc fast path -- a shared primitive under TBR/ratchet/temper, guarded once at the source. - ts_wagner.cpp: wagner_goloboff_scores/wagner_entropy_scores (biased-Wagner tip scoring), also reachable via the "large" preset. Regression tests added to test-ts-sector.R (default control, rasStarts=3, sectorCollapseTarget=6, pruneReinsertCycles=1, annealCycles=1, all through MaximizeParsimony() on a 40-tip all-uninformative EW dataset) and test-ts-wagner.R (ts_wagner_bias_bench directly). Reachability of all 6 sites was confirmed empirically (temporary REprintf probes on each total_words==0 branch, fired under every new test, then removed). Verified the tests are non-tautological, not just reachability-proxy: a negative control reverting build_ras_sector's guard back to the unconditional &rd.data.tip_states[...] reproduces the exact predicted abort (Assertion '__n < this->size()' failed) under a local -D_GLIBCXX_ASSERTIONS -O0 -g build, with a gdb backtrace confirming build_ras_sector (ts_sector.cpp) -> search_sector -> xss_search -> run_single_replicate -> driven_search -> ts_driven_search, i.e. reachable from the public MaximizeParsimony() API. Restoring the guard reruns clean. Full targeted suite (sector, wagner, hsj, xform, anneal, driven, drift, ratchet, tbr, Concordance) passes under the same hardened build with no behavioural regression. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…OX4/NODIRTY) The IW/XPIWE x4 reroot-batch + extract_char_steps dirty-region opts (byte-identical to the scalar path per the 3725746 validation + the dirty-rescore testthat guard) were opt-in (TS_IW_X4/TS_IW_DIRTY, default-off). Flip to default-ON with kill-switches TS_IW_NOX4/TS_IW_NODIRTY, so production IW/XPIWE search runs the ~14-18% faster path by default. All correctness prerequisites already on cpp-search: iw_family gate (fires on XPIWE), the active_mask!=0 nx_cs underflow guard, and the byte-identity test. Test updated to toggle via the kill-switch (else the off-arm would be a no-op under the new default = tautological). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Manual testing underway; shiny app in particular has some usability issues.