fix: validate presence and shape at the HSJ/XFORM bridge - #49
Merged
Conversation
Three guard clauses at the HSJ/XFORM scoring bridge: - unpack_hsj() (T-398/#14): a present-but-NULL hsjTipLabels passed the containsElementNamed() check but was then skipped, leaving scoring_mode = HSJ with an empty ds.tip_labels -- segfaulting score_hierarchy_block() later. Now requires hsjTipLabels non-NULL whenever hsjConfig is supplied, and (found during review) that it covers every hierarchy block's primary/secondary character index. - unpack_xform() (T-397/#13): ported ts_sankoff_test()'s cost-matrix dimension check and added three more guards for holes that function didn't have either: combo_grid row count, tip_sec_known dimensions (found during review), and out-of-range tip_states. Previously a mis-shaped-but-same-length matrix read garbage with no warning at all (Rcpp's Matrix::operator() bounds-checks only the linear offset). - ts_collapsed.cpp collapse guards (T-408/#22): keyed on scoring_mode alone, disabling branch collapse for an HSJ/XFORM config with no actual hierarchy data -- a case collapse is provably safe for. Now gates on hierarchy-data presence, matching DataSet::topology_independent()'s predicate. Verified the loosening is strictly confined to the no-data case: every config with real hierarchy_blocks/sankoff_n_chars still gets all-zero flags exactly as before, so the sibling #17 chip's reliance on this guard's current behaviour for non-degenerate configs is untouched. Reviewed by an independent external-reviewer pass, which confirmed the three primary guards and surfaced two adjacent gaps in the same functions (hsjTipLabels shape, tip_sec_known shape), now closed alongside them. Fixes #14 Fixes #13 Fixes #22 Pre-fix failures (confirmed by reverting the src/ changes and rebuilding): - T-398 standalone segfault: exit code 139 (SIGSEGV) running the compat wrapper with hierarchyBlocks set and hsjTipLabels left at its default. - test-ts-hsj-xform-guards.R (T-397/T-408 portion, T-398 test excluded since a segfault kills the testthat process): FAIL 4 | PASS 2 -- three xform guards did not error, and the empty-hierarchy HSJ config collapsed to 10 edges instead of 8 (matching the plain-EW baseline). Post-fix: full guard suite plus related HSJ/XFORM/collapse/resample suites all pass (509/509), verified via a temp-library tarball install per AGENTS.md, not devtools::load_all().
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
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.
Fixes #14
Fixes #13
Fixes #22
Three guard clauses at the HSJ/XFORM scoring bridge (
src/ts_rcpp.cpp) and the collapse gate (src/ts_collapsed.cpp).#14 (T-398) —
unpack_hsj()segfault on present-but-NULLhsjTipLabelshsjConfig$hsjTipLabels == NULL(element present, value NULL) passedcontainsElementNamed()but was then skipped by!Rf_isNull(), leavingscoring_mode = HSJwith an emptyds.tip_labels—score_hierarchy_block()then reads it unconditionally and segfaults. Fixed with an explicit presence/non-NULL check thatRcpp::stop()s. Review also surfaced a second entrance to the same crash class — a non-NULL but too-narrowhsjTipLabelsthat doesn't cover every hierarchy block's primary/secondary character index — closed with the same guard.Pre-fix standalone reproduction (segfault, cannot be asserted from inside testthat since it kills the process):
#13 (T-397) —
unpack_xform()had no dimension validationPorted
ts_sankoff_test()'s cost-matrix dimension check (guarded since0856748f) and added guards for holes that function doesn't have either:combo_gridrow count againstn_states - 1,tip_sec_knowndimensions (found in review), and out-of-rangetip_states. Previously a mis-shaped-but-same-length cost matrix read garbage with no warning at all — Rcpp'sMatrix::operator()only bounds-checks the linear offset, not(row, col)— and an out-of-rangetip_statesvalue corrupted the score via the1e18pool sentinel, which passesis.finite().#22 (T-408) — collapse guard keyed on
scoring_modealonets_collapsed.cpp's two collapse guards (compute_collapsed_flags,compute_collapsed_flags_aggressive) disabled branch collapse for any HSJ/XFORM config, even one carrying no actual hierarchy data (hierarchy_blocksempty andsankoff_n_chars == 0) — a case collapse is provably safe for. Fixed to gate on hierarchy-data presence, matching the predicateDataSet::topology_independent()already uses (src/ts_data.h:292).Verified the loosening is strictly confined to the no-data case: every config with real
hierarchy_blocks/sankoff_n_charsstill gets all-zero collapse flags exactly as before — the sibling #17 chip's reliance on this guard's current behaviour for non-degenerate configs is untouched. Confirmed by an independentexternal-reviewerpass and by the existing T-330 regression suite (test-ts-t330-collapse-hsj-xform.R), which still passes unchanged.Pre-fix failures (confirmed by reverting
src/changes and rebuilding via the tarball recipe)(The T-398 test is excluded from this run since its pre-fix behaviour is a segfault that kills the process — see above.)
Post-fix
Full guard suite plus related HSJ/XFORM/collapse/resample suites:
509/509pass, verified via a temp-library tarball install (R CMD build+R CMD INSTALL --library=.agent-hierarchy-guards) per AGENTS.md, notdevtools::load_all().Reviewed by an independent
external-revieweragent, scoped to correctness; it confirmed the three primary guards and the two adjacent gaps noted above, both now closed in this branch.CI status
ubuntu-arm64(fullR CMD check+ tests) — passed.windows— fails at the "Set up R dependencies" step, but this is confirmed pre-existing, unrelated infra breakage: it reproduces identically oncpp-searchtip itself right now (two directcpp-searchdispatches failed with the same signature in the last 20 minutes),DESCRIPTIONis untouched by this branch, and it is root-caused and already tracked/fixed at #43 (MaxMinpak-resolution asymmetry between Linux/Windows pandoc auto-detect), verified working onfeature/ci-maxmin-windows. Not duplicated here to avoid conflicting with that in-flight fix.Mandatory checks
Rscript .claude/tools/compile-attrs.R— arg counts match (no new exports).Rscript check_init.R— arg counts match.spelling::spell_check_test(vignettes = TRUE, ...)(exacttests/spelling.Rinvocation) — clean.vignettes/search-algorithm.Rmdnot touched (collapse behaviour on a degenerate config only, not the search itself).NEWS.mdupdated with the one user-visible surface (Resample()replicates that drop every hierarchy block from a unit now collapse like ordinary Fitch data).