A15-01: the documented `edgeToBreak = -1` "enumerate all one-step trees" contract is broken at 4 of 6 exported entry points, one of them silently

[ms609]

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

Covers red-team candidates A15-1, A15-2, A15-3, A15-5. Filed as one issue because they share a single documented contract and a single design decision resolves all of them; the individual edits differ per file.

The contract

man/NNI.Rd and man/TBR.Rd both document, for the shared edgeToBreak parameter:

Alternatively, set to -1 to return a complete list of all trees one step from the input tree.

Six exported functions inherit that promise. One honours it. Four fail, in three different ways, and no test anywhere passes edgeToBreak = -1 to any of them.

Function	-1 handling	Behaviour
NNI()	real branch, R/NNI.R:65	✅ works — returns a 12-tree multiPhylo on an 8-tip tree
RootedNNI()	branch present but malformed, R/NNI.R:185/:196	❌ errors unused arguments (...)
SPR()	branch present, unconditional stop(), R/SPR.R:98/:101	⚠️ refuses deliberately; :102–107 unreachable
RootedSPR(), RootedSPRSwap()	branch → AllSPR(), R/SPR.R:377	❌ errors object 'tree' not found
TBR(), RootedTBR()	no -1 branch at all, R/TBR.R:132, :346	❌ silently warns and returns the input tree unchanged

The three defects

A15-1 — RootedNNI(tree, edgeToBreak = -1) always errors

R/NNI.R:196:

newTrees <- lapply(newEdges, function (edges) `[[<-`, tree, "edge", edge)

The anonymous function's body is the bare symbol `[[<-` — it ignores its edges argument and returns the replacement function itself. The trailing tree, "edge", edge are then passed by lapply through ... to a closure that has one formal and no ..., so every call errors.

Two lines above, NNI()'s own branch is written correctly (function (edges) {tree[["edge"]] <- edges; tree}) and is exercised by an unconditional @examples block — which is presumably why only the Rooted sibling drifted.

RootedNNI(BalancedTree(8), edgeToBreak = -1)
#> Error in FUN(X[[i]], ...) : unused arguments (...)
NNI(BalancedTree(8), edgeToBreak = -1)        # works — 12 trees

A15-2 — RootedSPR() / RootedSPRSwap() crash on an undefined tree

AllSPR()'s final expression (R/SPR.R:299–347) is

lapply(newEdges, function (newEdge) {tree[["edge"]] <- newEdge; tree})

but AllSPR's formals are only parent, child, nEdge, notDuplicateRoot, edgeToBreak. tree is never defined locally and does not exist in the package namespace, so the call errors object 'tree' not found.

AllSPR's own roxygen says it "returns a list of edge matrices", so the intended body is almost certainly just newEdges — which are already RenumberTree()-built edge matrices.

Second defect at the caller. Even with AllSPR fixed, RootedSPR() (R/SPR.R:353–362) has no -1 special case: it always falls through to tree[["edge"]] <- cbind(newEdge[[1]], newEdge[[2]]), which is meaningless when newEdge is a list of many candidate edge matrices. SPR() returns early from its own branch; RootedSPR() never does.

Repro caveat for whoever confirms this. A global variable named tree in the calling session masks the bug — lexical scoping finds it from AllSPR's namespace parent. A local tree in the caller does not. Verify in a clean environment; the first verification pass produced a false negative this way.

A15-3 — TBR() / RootedTBR() silently no-op

No code in TBRSwap()/RootedTBRSwap() tests for -1. The value falls into the ordinary bounds check (R/TBR.R:132, :346):

if (edgeToBreak < 1) return(TBRWarning(parent, child, "edgeToBreak < 1"))

so the call emits warning("No TBR operation performed.\n > edgeToBreak < 1") and returns the unchanged input tree — identical() to the input, verified as such rather than by inspection.

This is the worst of the three, because it is the only one that fails quietly. A caller relying on the documented enumeration gets one unchanged tree back and, unless it is checking warnings, no indication that anything went wrong.

Suggested resolution — one decision, not four fixes

R/SPR.R:101 already records a maintainer decision for this feature:

stop("Negative edgeToBreak not yet supported; please request on GitHub")

If that decision still stands, the cheap and consistent fix is to refuse uniformly at the other five entry points and remove the -1 sentence from the inherited documentation — turning three accidental failure modes into one deliberate, informative error. That is a handful of lines and it eliminates the silent-no-op class entirely.

If instead -1 should work everywhere, then all four sites need implementing and testing; NNI()'s branch is the working reference, and AllSPR() is one line from correct.

Either way the doc must move with the code — see below.

A15-5 — the documentation is already inconsistent

R/SPR.R:70–72 declares @inheritParams TBR and then immediately re-declares @param edgeToBreak the index of an edge to bisect, generated randomly if not specified. The local declaration wins, so the generated man/SPR.Rd:44 carries no mention of -1 at all, while man/TBR.Rd:42–45 does.

Given A15-2 this is accidentally the less wrong state — SPR()'s -1 genuinely is unsupported — but it means the promise is visible for TBR/NNI and invisible for SPR, with no relationship to which functions actually implement it. Whichever resolution is chosen above, this file's @param block needs to end up consistent with it.

Verification

All four confirmed REAL by an independent peer-tier verifier (sonnet), reproduced against trunk tip e0629be61, with function bodies checked byte-for-byte against the installed namespace. Zero candidates from this round were refuted.

Found by /red-team area 15, 2026-08-05, sonnet (Sonnet 5) — the first review this area has ever had.