A15-09: exported swappers' argument validation is unreachable or absent — TBRSwap() HANGS on a trifurcating root

[ms609]

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

Covers red-team candidates A15-O-20, -15, -16, -17, -18. The exported swappers' argument validation is systematically unreachable or absent. Filed as med rather than low because one case hangs indefinitely.

---

1. TBRSwap() on a trifurcating root hangs indefinitely (A15-O-20)

R/TBR.R:136:

edgeToBreak <- which(parent == parent[[1]])[-1]

On a bifurcating root this yields length 1 and works. On a genuine trifurcating (unrooted) phylo it yields length 2 — verified, indices 1 2. Then brokenEdge <- seq_along(parent) == edgeToBreak recycles, and parent[edgeToBreak] returns two values.

The observed consequence is not a wrong answer but a hang. Calling TBRSwap(uparent, uchild, edgeToBreak = 1) on such a tree did not return; the verifier had to kill -9 the process after 2+ minutes.

TBR() guards against this via StopUnlessBifurcating(). TBRSwap() is exported with no such guard, so the hang is reachable from the public API. This is the reason the whole group is med — an unresponsive R session with no error is worse than a refused call, and EdgeSwapper arguments are precisely where a user is invited to pass these functions directly.

2. RootedNNI()/RootedSPR()/RootedTBR() omit the bifurcation guard their unrooted counterparts have (A15-O-15)

Function	Guard
NNI() R/NNI.R:64	✅ StopUnlessBifurcating()
RootedNNI() R/NNI.R:183	❌ absent
SPR() R/SPR.R:97	✅
RootedSPR() R/SPR.R:353	❌ absent
TBR() R/TBR.R:67	✅
RootedTBR() R/TBR.R:299	❌ absent

The shared help page states "All nodes in a tree must be bifurcating", so the Rooted* variants accept input their own documentation forbids.

Consequence: with a polytomy, nTips = (length(parent) / 2) + 1 becomes fractional — 7.5 for an 8-tip, 13-edge tree — so the child > nTips internal-edge test misclassifies the highest-numbered tip as internal. Over 50 draws on a polytomous 8-tip tree: RootedNNI produced 7/50 obscure errors, RootedSPR 4/50, RootedTBR 0/50. No structurally invalid tree was observed, but the misclassification is real and silent when it does not error.

3. mergeEdge length guard is unreachable (A15-O-16)

R/SPR.R:215-222 and :451-458 test if (mergeEdge > nEdge) before else if (length(mergeEdge) != 1), so a length-2 mergeEdge produces a length-2 condition:

Error: the condition has length > 1

instead of the intended SPRWarning("... must be NULL or a vector of length 1"). Reproduced on R 4.7.0-dev; note this was only a warning before R 4.2, so the guard has become an error as R tightened. Affects both SPRSwap() and RootedSPRSwap().

Fix: swap the two branches.

4. RootedSPRSwap()'s edgeToBreak range guards are unreachable (A15-O-17)

R/SPR.R:424-432 — if (!breakable[edgeToBreak]) runs before the > nEdge and < 1 checks, so the index is used before it is validated:

edgeToBreak = nEdge + 5  ->  Error: missing value where TRUE/FALSE needed
edgeToBreak = 0          ->  Error: argument is of length zero

SPRSwap() (:183-187) and RootedTBRSwap() (:345-349) order these correctly and emit the intended SPRWarning/TBRWarning for the same inputs — so this is an isolated slip, and the correct ordering already exists two functions away to copy from.

5. RootedTBRSwap() can silently break an edge other than the one requested (A15-O-18)

R/TBR.R:351-366 — after selectableEdges[edgeToBreak] passes, the repeat loop may still find the break unproductive, clear the flag and resample edgeToBreak at :365, with no warning. A caller passing an explicit edgeToBreak for reproducibility silently gets a different move and an extra RNG draw, which desynchronises any seeded sequence that follows.

Also unguarded: if every selectable edge fails, SampleOne(which(selectableEdges)) is called on integer(0). The author's own ###Assert(any(selectableEdges)) at :364 is commented out. Not observed in 250-tree fuzzing across n = 4…12, but nothing prevents it.

---

Suggested fix

These are all small and share a shape — validate before use, and validate at the exported boundary rather than only in the wrapper:

1. Add StopUnlessBifurcating() to RootedNNI, RootedSPR, RootedTBR and to the exported *Swap functions, which is what closes §1's hang.
2. Reorder the guards in §3 and §4 to test shape and range before indexing.
3. Either honour an explicit edgeToBreak in §5 or warn when overriding it; restore the commented-out assertion.

A single test asserting that every exported swapper refuses a non-bifurcating tree would cover §1 and §2 together.

Verification

All five confirmed REAL against commit 826d332b0. §1, §3 and §4 were reproduced by direct execution on R 4.7.0-dev with functions sourced live from the worktree; §1's hang was observed rather than inferred. §2 was confirmed by reading all six cited lines.

Found by /red-team area 15, 2026-08-05, opus (Opus 5). The finder rated §1 low on the strength of a code read; it is filed here at the group's med because execution showed it hangs.