red-team area 15: two paired passes over the legacy pure-R search API, plus two inline fixes

[ms609-agent]

Two red-team passes over area 15 (legacy pure-R search API), its first review since the row was created. This PR carries two small inline fixes and the round records; the 31 findings themselves are filed as issues and are not fixed here.

No Fixes #N lines: nothing filed is fixed by this branch. The two defects that were fixed were fixed inline and deliberately not filed, per the skill's rule against filing an issue and closing it the same round.

Code changes (2 files, 8 lines)

R/CustomSearch.R — EdgeListSearch() looped for (iter in 1:maxIter). 1:0 is c(1, 0), so maxIter = 0 silently performed two rearrangement iterations instead of none; because RearrangeEdges() accepts any candidate scoring <= scoreToBeat, a caller asking for zero rearrangements could get a different tree back. maxIter is user-facing (default 100) and reaches this loop from Bootstrap(), Jackknife() and Ratchet(), so maxIter = 0 is a reachable "score without searching" idiom.

Now seq_len(maxIter), with iter <- 0L pre-initialised — that part is load-bearing, since iter is read after the loop at :111.

R/SuccessiveApproximations.R — the "Stability not reached" branch emitted its message regardless of verbosity while the "converged" branch was gated, so a default verbosity = 0 call was noisy. Both branches now sit inside one gate. Nothing in tests/, vignettes/, man/ or R/ referenced the string.

Test

tests/testthat/test-CustomSearch.R gains a regression test pinning the maxIter = 0 contract. EdgeListSearch() is exported, so it uses mocks that error if called — no dataset, no C++, instant. Verified to discriminate: post-fix the loop body is never entered; with the pre-fix loop bound restored in an isolated copy of the function, it is.

Findings filed

Issue	Severity	Summary
#136	high	Ratchet(stopAtScore=) returns the input tree carrying the improved score, plus two more early-exit bookkeeping failures
#137	high	Ratchet() never forwards TreeScorer to its Bootstrapper, so the two search phases optimise different objectives
#138	high	TreeSearch()'s default swapper holds the root fixed, confining a mid-rooted start to ~5% of tree space — needs a design decision
#139	high	Fractional character weights truncate to zero before bootstrap resampling, silently degrading BootstrapTree()/JackknifeTree() to a random walk
#143	med	The pure-R samplers waste up to 40% of draws on identity moves and cannot reach part of the neighbourhood they document
#144	med	Exported swappers' argument validation is unreachable or absent — TBRSwap() hangs on a trifurcating root
#125	med	The documented edgeToBreak = -1 contract is broken at 4 of 6 exported entry points, one silently
#126	low	SuccessiveApproximations() runs without the sectorial/fuse/pool machinery and never says so
#131	low	Eight contract/documentation drifts, grouped for a single sweep

31 candidates were grouped into 9 issues by shared root cause rather than filed one-per-finding — the tracker is at 72 open with 3 claimed, and 31 tickets would have made triage worse, not better.

Why two passes

These were the two arms of a deliberate tier-economics experiment: is it cheaper to let sonnet find what it can and have opus find only the remainder, than to send opus first? Area 15 was the only never-visited area, so a first pass measured the cheap tier honestly.

	sonnet	opus (given sonnet's entire yield as off-limits)
Candidates	5	26
sev:high	0	4
Confirmed / refuted	5 / 0	26 / 0
Finder tokens	137k	180k

The cheap pass removed no work from the expensive one — opus still read all 2,185 loc and found five times as much — so sonnet-first was an added pass, not a substituted one. The severity split is the substantive result: sonnet found broken documented contracts, opus found silent wrong answers. Both log.md entries record this, and focus-areas.md now says not to re-run it.

focus-areas.md row 15 keeps start_tier: sonnet — the maintainer's recorded choice on #42 — but the rationale beside it now notes that the reasoning behind it is falsified, and that the field is inert for a visited area anyway. One word to change if you want it.

Verification notes worth reading

Verification corrected the finder three times, so the issues do not simply restate what the finders reported:

• One high-severity candidate was downgraded to med (a loud error, not a silent one).
• A15-06: TreeSearch()'s default swapper holds the root fixed, confining a mid-rooted start to ~5% of tree space (needs a design decision) #138's mechanism proved worse than claimed (the root split is a hard invariant, not a per-step bias — a 150k-step walk reached 45 of 945 topologies) while its motivating example was wrong: NJTree() is tip-rooted, so this package's own ?TreeSearch example is in the benign regime.
• Three sampler findings came back worse: the NNISwap self-hit rate is 2/(nTips-2), not 1/(nTips-2) — which changes the fix, since both root-adjacent edges must be excluded; NNI(-1) is missing an entire pair of true neighbours rather than merely padded with duplicates; and TBRSwap() on a trifurcating root hangs indefinitely rather than misbehaving.

Both peer verifiers built independent unrooted-neighbourhood enumerators and validated them against the closed forms 2(n-3) and 2(n-3)(2n-7) before using them as ground truth, which is what let them contradict the finders' numbers rather than defer to them. One recorded a trap for whoever rebuilds it: tied n/2|n/2 splits need lexicographic, not size-based, tie-breaking.

Checks

agent-check.yml green on ubuntu-arm64 and windows (runs 31014552218, 31037706585). spelling::spell_check_package(vignettes = TRUE) clean. No roxygen or C++ signature changed, so check_man() and compile-attrs.R were not triggered; vignettes/custom.Rmd documents this API but uses maxIter = 50L throughout, so the degenerate-input fix changes nothing it describes.

🤖 Generated with Claude Code