A15-11: second sweep of legacy-API guard, documentation and dead-code defects (incl. an opaque SPR() crash on an explicit edgeToBreak = 1)

[ms609-agent]

Severity: sev:low · Area: 15 (Legacy pure-R search API)

Covers A15-11, -16, -17, -20, -21, -22, -26, -30, -31. A second sweep of small defects in the legacy API, companion to #131 and #144. All confirmed against commit eb6e26ce1; two candidates from the same round were refuted and are recorded at the end so they are not re-hunted.

---

1. SPR() crashes with an opaque internal error when edge 1's root-sibling is a tip (A15-11)

SPR(TreeTools::Preorder(ape::read.tree(text = "((((t1,t4),t2),t3),t5);")), edgeToBreak = 1)
#> Error in child[[mergeEdge]] : attempt to select less than one element in get1index

mergeEdge <- which(...) at R/SPR.R:231 returns integer(0) — every edge is a descendant of edge 1, the sister edge, or the .NonDuplicateRoot()-excluded edge — and :242/:244 then index with it.

The mechanism in the original report is wrong, and following it would fix nothing. The finding blamed the edgeToBreak != 1 clause of the guard at :203. But TreeTools::DescendantEdges includes the query edge, so for this repro edgesCutAdrift = T T T T T T T F and all(edgesCutAdrift[-1]) is already FALSE. Deleting that clause would not make the guard fire. The real defect is that all(edgesCutAdrift[-1]) only recognises the mirror-image layout — the [-1] hard-codes "the tip sibling is edge 1".

Exact condition, better than the finding's: breaking edge 1 fails iff the other root child is a tip. Exhaustive over all 1,155 rooted binary 6-tip trees: 525 errors, precisely the 525 whose edge-1 sibling is a tip — zero false positives, zero misses.

The count 138 does not reproduce. An all-rootings sweep of every edgeToBreak for n = 5, 6, 7 gives 60 / 525 / 5,670 = 6,255 failing (tree, edge) pairs, and every one is edgeToBreak == 1 — no other edge index ever fails.

Not reachable from any search — hence low, not med. In all 6,255 failures .NonDuplicateRoot()[[1]] is FALSE, so SampleOne(which(notDuplicateRoot)) at :182 can never pick edge 1. Analytically, edge 1 is samplable only when it subtends 1 or 3 edges, and a tip sibling forces nEdgeRight == nEdge - 1, impossible for nEdge >= 5. Confirmed by 108,600 random-path SPRSwap() calls over the n = 5–7 rooted-tree set: 0 errors. SPR() at :109 is the only caller forwarding an explicit edgeToBreak; the search machinery always uses the NULL path.

So: a latent trap in an exported, documented argument, surfacing as an opaque internal error.

2. Guard defects

A15-21 — range checks run after the index is used. R/NNI.R:123 tests !samplable[edgeToBreak] before any range check, so edgeToBreak = 99 gives missing value where TRUE/FALSE needed. R/SPR.R:424 tests !breakable[edgeToBreak] before the > nEdge / < 1 checks at :428-432, making those two SPRWarning() branches unreachable dead code. Fix is to reorder.

A15-22 — RootedNNISwap() lacks the guard its unrooted twin has. R/NNI.R:212-238 omits if (!any(samplable)) stop("Not enough edges to allow NNI rearrangement"), present in NNISwap() at :119, so SampleOne(integer(0)) raises invalid first argument on Preorder(BalancedTree(4)).

A15-26 — MultiRatchet() accepts ratchHits and ignores it. R/Ratchet.R:278 declares ratchHits = 10; :284 hard-codes ratchHits = 0L in the Ratchet() call. The documented argument is inert.

A15-30 — deprecated shims drop or fail on documented arguments. R/morphy-deprecated.R:73-77: MorphyLength() accepts nTaxa but :76 calls EdgeListScore() without it, contradicting the @param at :21 ("Passed to the replacement function"). :88-91: MorphyBootstrap() forwards to BootstrapTree(), whose maxIter/maxHits have no defaults (R/Bootstrap.R:16-17), so a historic no-argument call now errors.

3. Documentation defects

A15-16 — an override documented on TreeSearch() but implemented only in Ratchet(). R/CustomSearch.R:142-149 (rendered at man/TreeSearch.Rd:76-83) promises attr(FunctionName, "stopAtPeak") <- TRUE will override. Only Ratchet()'s Argument() helper (R/Ratchet.R:156) reads swapper attributes; TreeSearch() passes the caller's values straight to EdgeListSearch() without reading any. (A verifier initially marked this REFUTED on the grounds that Ratchet() does implement it — which refutes a claim the finding never made. Its own evidence confirms the finding as stated.)

A15-17 — \value{} names an attribute the function does not set, and leaks a mangled roxygen line. man/TreeSearch.Rd:103 promises attribute pscore; R/CustomSearch.R:236 sets score. And R/CustomSearch.R:171 reads #' #" Note that the parsimony score will be inherited from the tree"s — a doubled comment marker plus smart-quote damage — which renders verbatim into man/TreeSearch.Rd:105.

A15-20 — the docs direct users to a function that does not exist. R/SPR.R:64 → man/SPR.Rd:80: "RootIrrelevantSPR will search tree space more efficiently in these cases." No such object exists in R/, src/, NAMESPACE or elsewhere in man/. Because it is prose rather than \link{}, R CMD check will not catch it.

4. Dead code (A15-31)

Each verified individually — a single wrong entry here would send a fixer to delete live code:

Location	Symbol
R/SPR.R:102-107	unreachable after the unconditional stop() at :101 (see #125 — remove as part of that decision, not independently)
R/NNI.R:117	rootNode assigned in NNISwap(), never read
R/NNI.R:174	nEdge assigned in DoubleNNI(), never read
R/TBR.R:161	edgesRemaining assigned, never read
R/Ratchet.R:95	hits <- 0L assigned, never read
R/Ratchet.R:131	nullForest assigned to forest, then never referenced

Refuted — do not re-hunt

PrepareDataSA() is not dead code. R/SuccessiveApproximations.R:193-218 was reported as reachable only via the broken SuccessiveWeights(). It is also exported in NAMESPACE, so users can call it directly. It remains entirely untested, which is a different and lesser claim.

Ratchet(ratchIter = 0) does not reference an undefined i. Reported as interpolating an unbound i at R/Ratchet.R:209 under verbosity > 0, resolving to a global or erroring. R binds an empty-for loop variable locally to NULL, so it neither errors nor reads a global — verified by setting a global i <- "BANANA" and observing it is not printed. The only residue is a blank where the iteration count belongs. Not worth filing on its own.

Verification

Confirmed against eb6e26ce1 by two independent verifiers — peer-tier for the crash and Ratchet-semantics claims, cheap-tier for the documentation and dead-code claims, each item checked individually. Where the verifier's mechanism, count or severity differed from the finder's, the verifier's is recorded above: §1's mechanism, condition and count are all corrections, and its severity was downgraded from med.

Found by /red-team area 15, 2026-08-06, fable (Fable 5).