From 826d332b01f0c9f8e6b9a8fc4e8e939bee0a15eb Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:18:04 +0100 Subject: [PATCH 1/8] red-team: area 15 (legacy pure-R search API), first-ever review Fixes the one defect small enough to fix inline, and records the round. `EdgeListSearch()` looped `for (iter in 1:maxIter)`. `1:0` is `c(1, 0)`, so `maxIter = 0` silently performed two rearrangement iterations instead of none -- and since `RearrangeEdges()` accepts any candidate scoring `<= scoreToBeat`, a caller asking for zero rearrangements could get a different tree back. `maxIter` is user-facing and reaches this loop from `Bootstrap()`, `Jackknife()` and `Ratchet()`, so `maxIter = 0` is a reachable "score without searching" idiom. Switched to `seq_len(maxIter)`, pre-initialising `iter <- 0L` because `iter` is read after the loop. Pinned by a regression test using mocks that error if called; `EdgeListSearch()` is exported, so it needs no dataset and no C++. The round's other findings are filed as agent-issues/TreeSearch#125 (the documented `edgeToBreak = -1` contract is broken at 4 of 6 exported entry points, one of them silently) and #126 (`SuccessiveApproximations()` runs without the sectorial/fuse/pool machinery, undocumented). Co-Authored-By: Claude Opus 5 --- R/CustomSearch.R | 5 +++-- dev/red-team/focus-areas.md | 30 ++++++++++++++++++++---------- dev/red-team/log.md | 25 ++++++++++++++++++++++++- tests/testthat/test-CustomSearch.R | 15 +++++++++++++++ 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/R/CustomSearch.R b/R/CustomSearch.R index 188f5df4b..859e91142 100644 --- a/R/CustomSearch.R +++ b/R/CustomSearch.R @@ -59,8 +59,9 @@ EdgeListSearch <- function (edgeList, dataset, } hits <- 0L unimprovedSince <- 0L - - for (iter in 1:maxIter) { + iter <- 0L + + for (iter in seq_len(maxIter)) { candidateLists <- RearrangeEdges(edgeList[[1]], edgeList[[2]], dataset = dataset, TreeScorer = TreeScorer, diff --git a/dev/red-team/focus-areas.md b/dev/red-team/focus-areas.md index 4868adbf1..1c4f10444 100644 --- a/dev/red-team/focus-areas.md +++ b/dev/red-team/focus-areas.md @@ -166,14 +166,24 @@ top of `log.md`; seams that a version bump has made re-eligible are queued in (`test-MaddisonSlatkin.R`, `test-Concordance.R`, `test-ParsSim.R`, `test-Consistency.R`, `test-ScoreSpectrum.R`, `test-QuartetResolution.R`, `test-TaxonInfluence.R`, `test-WideSample.R`, `test-pp-*.R`) is a useful first read. -- **15 Legacy pure-R search API — sonnet, UNMEASURED / no inherited maturity.** Added +- **15 Legacy pure-R search API — MEASURED 2026-08-05 at sonnet, still yielding.** Added 2026-08-05 from #42's scope-coverage diff: 2,183 lines across 9 files backing the - still-shipped pre-C++-engine search functions, owned by no area. **Higher urgency than its - size suggests:** #16 (`sev:high`) names `EdgeListScore()` as *"the default `TreeScorer` for - `TreeSearch()`/`Ratchet()`/`Jackknife()`"* and one of four confirmed-vulnerable entry points, - so this family is a second, wholly unreviewed exposure surface for an already-confirmed bug — - take that question first. #42 offered "review once as frozen legacy, then deprioritise"; - **the maintainer chose a full rotation area instead (2026-08-05): keep revisiting until the - seam stops yielding.** Legacy is not the same as clean, and this code is still shipped and - still the documented entry point for users who have not moved to the C++ engine. Treat "it - isn't growing" as a reason the seam should *exhaust* quickly, not as a reason to stop early. + still-shipped pre-C++-engine search functions, owned by no area. #42 offered "review once as + frozen legacy, then deprioritise"; **the maintainer chose a full rotation area instead + (2026-08-05): keep revisiting until the seam stops yielding.** Legacy is not the same as + clean, and this code is still shipped and still the documented entry point for users who have + not moved to the C++ engine. Treat "it isn't growing" as a reason the seam should *exhaust* + quickly, not as a reason to stop early. + **The `#16`/`EdgeListScore()` urgency that originally justified this row is now discharged** + — #16 is closed, its guard landed in PR #50, and the first round measured the pure-R layer + above it as *doubly* guarded (`R/CustomSearch.R:209` and `R/Ratchet.R:96` both reject + non-bifurcating input independently of the C++ fix; a 600-iteration fuzz of the default + rearrangement path produced no non-binary trees). Do not spend a second round re-asking it; + reopen only if the C++ guard is relaxed or a new caller bypasses those entry checks. + `start_tier` remains `sonnet` but is now inert — the seam yielded, so routing keeps the next + visit at the tier the log's experiment paragraph specifies. **Next visit starts here:** the + round's own leads — `SuccessiveWeights()` (`R/SuccessiveApproximations.R:175-189`) scores via + `CharacterLength()` rather than the kernel and was never checked for agreement with it, which + is exactly the shape of the already-confirmed #83 divergence; and the `-1` contract class of + #125, whose fix is one maintainer decision (refuse uniformly, per the existing deliberate + `stop()` at `R/SPR.R:101`) rather than four separate repairs. diff --git a/dev/red-team/log.md b/dev/red-team/log.md index 6d8901573..e9e8aa129 100644 --- a/dev/red-team/log.md +++ b/dev/red-team/log.md @@ -60,6 +60,29 @@ persistently-dry reputation leans on pre-tier rounds (areas 3 and 10 both do) ha --- +area: 15 (Legacy pure-R search API) — first-ever review +reviewed_by: sonnet finder afbe83c1d183868a3 + sonnet peer verifier abd8f225317a23c4c (3 med, R-semantics verdicts) + haiku verifier af328ab30afd622f0 (2 low, artifact-tracing verdicts) +date: 2026-08-05 +tier: sonnet (Sonnet 5) +yield: **3 confirmed findings, 0 refuted of 5 candidates.** Filed: #125 (A15-01, sev:med — groups candidates 1/2/3/5), #126 (A15-02, sev:low — candidate 4). Fixed inline, logged not filed: A15-03. **Zero sev:high.** +notes: **THIS ROUND IS THE SONNET ARM OF A DELIBERATE TIER-ECONOMICS EXPERIMENT — DO NOT DISPATCH THE OPUS ARM WITHOUT READING THIS PARAGRAPH.** The question under test: is it cheaper to let sonnet find what sonnet can find and then have opus find only the remainder, than to send opus first? Area 15 was chosen because it was the only never-visited area — nothing here had been pre-harvested, so a first pass measures the cheap tier honestly. **The opus arm must run over the identical scope with a fresh agent, with #125/#126 AND the A15-03 inline fix in the "do not re-investigate" block**, and its yield recorded here for comparison. Anything else measures a different question. Prior to this round the record was: sonnet 6 rounds / **0** sev:high ever; opus 24 rounds / 11 producing at least one. This round did not break that pattern. + +**The scope row's headline question was stale and would have burned the round.** The row still cited #16 (`TreeState::init_from_edge` OOB on multifurcating trees) as live, naming `EdgeListScore()` as a vulnerable entry point. **#16 is CLOSED/COMPLETED** — the boundary guard landed in PR #50 — so the brief reframed it before dispatch: the C++ boundary now *rejects* non-binary trees, so the live question is whether the pure-R layer above hands it inputs it will now reject. **Answer: no, and the seam is doubly guarded.** `TreeSearch()`/`Ratchet()` both already `stop("tree must be bifurcating")` on entry (`R/CustomSearch.R:209`, `R/Ratchet.R:96`), predating and independent of the C++ fix, and `EdgeListScore()` itself carries its own `tabulate(parent)` binary check (`R/tree_length.R:611-618`) that looks to have been added in response to #16's family. A 600-iteration chained fuzz of `RootedNNI`/`RootedSPR`/`RootedTBR` on the default random-move path, with full structural-sanity checks, found zero corruption — **the default heuristic-search rearrangement path does not spontaneously build non-binary trees.** Recorded as measured, not assumed: reopen if the C++ guard is ever relaxed, or if a new caller bypasses those two entry checks. + +**The actual find is a documented contract that is broken almost everywhere it is documented.** `man/NNI.Rd`/`man/TBR.Rd` promise that `edgeToBreak = -1` returns "a complete list of all trees one step from the input tree". Six exported functions inherit that promise; **one honours it.** `NNI()` works; `RootedNNI()` errors *unused arguments* (`R/NNI.R:196` — the anonymous function's body is the bare symbol `` `[[<-` ``, so `lapply`'s `...` hits a one-formal closure); `RootedSPR()`/`RootedSPRSwap()` error *object 'tree' not found* (`AllSPR()` references a `tree` that is not among its formals); `TBR()`/`RootedTBR()` have **no `-1` branch at all** and fall into the ordinary `edgeToBreak < 1` bounds check, so they warn and return the input tree **unchanged** — the only silent failure of the four, and the dangerous one. `SPR()` alone refuses deliberately, via an unconditional `stop("Negative edgeToBreak not yet supported; please request on GitHub")` at `R/SPR.R:101` that makes `:102-107` dead code. **That `stop()` is the key to the fix and reframes the finding**: this is one maintainer decision, not four bugs — refusing uniformly at the other five sites (and dropping the `-1` sentence from the docs) is a handful of lines and eliminates the silent class outright. No test anywhere passes `edgeToBreak = -1` to any of the six. + +**A15-03, fixed inline, not filed:** `EdgeListSearch()` looped `for (iter in 1:maxIter)` (`R/CustomSearch.R:62`). `1:0` is `c(1, 0)`, so `maxIter = 0` silently performed **two** rearrangement iterations instead of none — and since `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 propagates from `Bootstrap()`, `Jackknife()` and `Ratchet()`, so `maxIter = 0` is reachable as a plausible "score without searching" idiom. Fixed to `seq_len(maxIter)` with `iter <- 0L` pre-initialised, which is load-bearing: `iter` is read after the loop at `:111`. **Pinned by a regression test** (`test-CustomSearch.R`) using mocks that error if called — `EdgeListSearch()` is exported, so this needs no data and no C++. Verified pre/post by rebuilding the pre-fix loop bound in an isolated copy of the function: post-fix the loop body is never entered, pre-fix it is. + +**Verifier routing note worth keeping.** Candidates 1/2/3 were `sev:med`, which the severity rule alone would send to haiku — but all three turn on **R language/roxygen semantics** rather than on tracing project code, so they went peer-tier under the "route by what decides the verdict" rule. That paid: on candidate 2 the peer verifier found that **a global variable named `tree` masks the bug entirely** (lexical scoping reaches it from `AllSPR`'s namespace parent; a *local* `tree` in the caller does not), and it initially produced a false negative that way before re-running in a clean environment. A cheap verifier would plausibly have refuted a real finding. That is now the third time this project has been bitten by cheap-verifying a library/language-fact verdict. + +**Not chased, for the next reviewer:** (a) `SuccessiveWeights()` (`R/SuccessiveApproximations.R:175-189`) calls `CharacterLength()` rather than the kernel — whether its scoring agrees with the C++ kernel for the same tree and weights is untraced, and #83 already shows `TreeLength()`/`EdgeListScore()` disagreeing by default on extended IW, so this looks live; (b) `RearrangeEdges()` (`R/tree_rearrangement.R:36-94`) calls `TreeScorer()` **twice per iteration** when `scoreToBeat` is not supplied — once for the default argument, once for `candidateScore` — a perf observation only, not investigated for correctness, and arguably a `/profile` item rather than a red-team one; (c) `MultiRatchet()`/`Ratchet()`'s `swappers=` list combined with a caller passing `edgeToBreak = -1` through `...` would reach the broken paths — no such call site exists in this codebase, but the docs promise it, so a downstream user script plausibly could. + +**Re-verified, not re-filed:** #119's `R/Bootstrap.R` `sample()` length-1 trap. `deindexedChars` can have length 1 only when the entire dataset compresses to a single unique pattern of weight exactly 1 — genuinely degenerate, so #119's "unreachable with normal weights" characterisation holds. **Not re-filed:** #83, confirmed still present; its specific two-path shape does **not** extend to `SuccessiveApproximations()`, which builds `contrast`/`tip_data`/`weight` itself and calls the kernel directly, so it has only one path. + +Seam status: **still yielding** (5 candidates, 5 confirmed, 0 refuted, on a first pass). **NEXT VISIT: the opus arm of the experiment above — same scope, fresh agent, #125/#126/A15-03 in the do-not-re-investigate block.** This is not an escalation on the usual trigger (the seam did not run dry); it is a deliberate paired measurement. + +--- + area: 14 (Statistics & support-metrics cluster) — first-ever review, area enacted this round reviewed_by: opus finder a01961467a9937203 + opus verifier a13226f4cad264aea (7 high-sev/memory-safety candidates) + haiku verifier a02207b4e4057b65e (30 low/med batch) + orchestrator direct code-read (A14-11, A14-12 — omitted by the haiku batch's return) date: 2026-08-05 @@ -1376,4 +1399,4 @@ tier: n/a (directed single-finding fix) yield: 1 filed-and-fixed same session (T-366, P3) notes: Handed a pre-verified finding for `expand_and_reinsert` (`ts_prune_reinsert.cpp:396`): it scored the rebuilt backbone with `score_tree()`, which on `has_inapplicable` data falls through to `fitch_na_score` and writes NA-regime `prelim`, while the insertion loop's `wagner_incremental_rescore` (`ts_wagner.cpp:131-166`) only maintains standard-Fitch `prelim` with no NA branch — `compute_insertion_edge_sets` then reads this mixed-regime array to choose reinsertion edges. The two sibling backbone-scoring call sites (`ts_wagner.cpp:449`, `ts_sector.cpp:917`) both already use the EW-proxy `fitch_score`, so this one call site reads as an oversight. **Fix applied:** swapped to `fitch_score(tree, ds)`. **Verification performed this session:** built clean; ran an NA repro (`Vinther2008`, then `Dikow2009` for a stronger test) with `pruneReinsertCycles` forced nonzero (default is `0L`, fully inert otherwise) — confirmed the path was actually exercised via `prune_reinsert_ms` timing (0ms before forcing the params right, ~1.3s after). Direct A/B (temporarily reverted the fix, rebuilt, re-ran identical seeds): on `Dikow2009` with 6 fixed RNG seeds, 5/6 gave byte-identical final score AND topology (`write.tree` hash) before vs. after; seed 4 diverged (1614 before → 1616 after) — confirms the fix changes search trajectory on this now-live path, exactly as the finding predicted, with no crash and no corrupted score in either arm. Existing `test-ts-prune-reinsert.R` (52 tests) and `test-ts-sector.R` (52 tests) both still pass. **Not done, flagged as a separate follow-up (do not conflate with this fix):** `fitch_na_score`'s `local_cost` is only written on its non-NA branch, which independently corrupts `wagner_incremental_rescore`'s `old_cost` subtraction for NA blocks — this changes placement further and needs its own A/B before landing. **This entry was not independently re-verified by a second reviewer** (no red-team-verifier pass) — the A/B above is empirical evidence, not a peer confirmation; a future round should sanity-check the reasoning, not just re-trust this note. This was a directed fix task, not a rotation round, so `last_focus` is left untouched. -last_focus: 14 +last_focus: 15 diff --git a/tests/testthat/test-CustomSearch.R b/tests/testthat/test-CustomSearch.R index 4abcd037b..f8f352e4b 100644 --- a/tests/testthat/test-CustomSearch.R +++ b/tests/testthat/test-CustomSearch.R @@ -108,3 +108,18 @@ test_that("Profile parsimony works in tree search", { test_that("Ratchet fails gracefully", { expect_error(Ratchet(unrooted11, data11)) }) + +test_that("EdgeListSearch() performs no rearrangements when maxIter = 0", { + # `1:maxIter` evaluates to c(1, 0) when maxIter is zero, so the loop silently + # ran two rearrangement iterations; seq_len() is empty, as intended. + edge <- PectinateTree(letters[1:6])[["edge"]] + MustNotRun <- function (...) { + stop("No rearrangement should be attempted when maxIter = 0") + } + searched <- EdgeListSearch(list(edge[, 1], edge[, 2], 99), dataset = NULL, + TreeScorer = MustNotRun, + EdgeSwapper = MustNotRun, + maxIter = 0, verbosity = 0L) + expect_equal(searched[[3]], 99) # Starting score returned unchanged + expect_equal(searched[[4]], 0L) # No hits recorded +}) From eb6e26ce11b71c1ee01f8ab7a78ef584eaebcf9c Mon Sep 17 00:00:00 2001 From: R script <1695515+ms609@users.noreply.github.com> Date: Wed, 5 Aug 2026 20:03:59 +0100 Subject: [PATCH 2/8] red-team: area 15 opus pass, and the tier-economics result Second pass over the same scope with the sonnet arm's entire yield in the do-not-re-investigate block. 26 candidates, 26 confirmed, 0 refuted -- four of them sev:high, against the sonnet arm's zero. Filed as seven grouped issues: #136, #137, #138, #139 (high), #143, #144 (med), #131 (low). The paired result answers the question the two passes were run to settle: a cheap first pass did not reduce the expensive pass's work, so sonnet-first is an added pass rather than a substituted one. Recorded in log.md; the rationale in focus-areas.md now says not to re-run it. Fixes one defect inline: the "Stability not reached" branch of `SuccessiveApproximations()` 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. Co-Authored-By: Claude Opus 5 --- R/SuccessiveApproximations.R | 14 ++++++++------ dev/red-team/focus-areas.md | 27 +++++++++++++++++++-------- dev/red-team/log.md | 25 ++++++++++++++++++++++++- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/R/SuccessiveApproximations.R b/R/SuccessiveApproximations.R index 52361f9c5..bc9c18fc6 100644 --- a/R/SuccessiveApproximations.R +++ b/R/SuccessiveApproximations.R @@ -108,12 +108,14 @@ SuccessiveApproximations <- function (tree, dataset, outgroup = NULL, k = 3, c(searchArgs, .KernelConstraintArgs(consArgs), profileArgs)) - if (result$converged && verbosity > 0) { - message("Successive approximations converged after ", - result$sa_iterations, " iteration(s).") - } else if (!result$converged) { - message("Stability not reached after ", result$sa_iterations, - " iteration(s).") + if (verbosity > 0) { + if (result$converged) { + message("Successive approximations converged after ", + result$sa_iterations, " iteration(s).") + } else { + message("Stability not reached after ", result$sa_iterations, + " iteration(s).") + } } # Reconstruct phylo from C++ edge matrix diff --git a/dev/red-team/focus-areas.md b/dev/red-team/focus-areas.md index 1c4f10444..b811a50b1 100644 --- a/dev/red-team/focus-areas.md +++ b/dev/red-team/focus-areas.md @@ -166,7 +166,10 @@ top of `log.md`; seams that a version bump has made re-eligible are queued in (`test-MaddisonSlatkin.R`, `test-Concordance.R`, `test-ParsSim.R`, `test-Consistency.R`, `test-ScoreSpectrum.R`, `test-QuartetResolution.R`, `test-TaxonInfluence.R`, `test-WideSample.R`, `test-pp-*.R`) is a useful first read. -- **15 Legacy pure-R search API — MEASURED 2026-08-05 at sonnet, still yielding.** Added +- **15 Legacy pure-R search API — MEASURED 2026-08-05 at BOTH sonnet and opus; yielding + heavily. The `start_tier` column still reads `sonnet` — the maintainer's recorded choice on + #42 — but the experiment below falsifies the reasoning behind it. Left unchanged pending the + maintainer's call; it is inert either way, since routing governs a visited area.** Added 2026-08-05 from #42's scope-coverage diff: 2,183 lines across 9 files backing the still-shipped pre-C++-engine search functions, owned by no area. #42 offered "review once as frozen legacy, then deprioritise"; **the maintainer chose a full rotation area instead @@ -180,10 +183,18 @@ top of `log.md`; seams that a version bump has made re-eligible are queued in non-bifurcating input independently of the C++ fix; a 600-iteration fuzz of the default rearrangement path produced no non-binary trees). Do not spend a second round re-asking it; reopen only if the C++ guard is relaxed or a new caller bypasses those entry checks. - `start_tier` remains `sonnet` but is now inert — the seam yielded, so routing keeps the next - visit at the tier the log's experiment paragraph specifies. **Next visit starts here:** the - round's own leads — `SuccessiveWeights()` (`R/SuccessiveApproximations.R:175-189`) scores via - `CharacterLength()` rather than the kernel and was never checked for agreement with it, which - is exactly the shape of the already-confirmed #83 divergence; and the `-1` contract class of - #125, whose fix is one maintainer decision (refuse uniformly, per the existing deliberate - `stop()` at `R/SPR.R:101`) rather than four separate repairs. + **This row carried the tier-economics experiment, and it settled the tier question for good.** + Paired passes over the identical scope on 2026-08-05: `sonnet` returned 5 candidates and **0 + sev:high**; `opus`, handed sonnet's entire yield as off-limits, returned **26 candidates and 4 + sev:high**, all 26 confirmed. The cheap pass removed no work from the expensive one. Sonnet + found broken *documented contracts*; opus found *silent wrong answers* — different classes, + not different amounts. See the log's two 2026-08-05 entries; **do not re-run this experiment.** + **Next visit starts here** (stay at `opus`, fresh agent, and prefer a targeted shape over + another general finder): the **decayed custom-criterion façade** — #137 (`Ratchet()` never + forwards `TreeScorer`), #126 (`SuccessiveApproximations()`'s undocumented capability gap) and + the dead `SuccessiveWeights()` are one story, and the useful work is auditing *which advertised + custom-criterion entry points work end to end*, each with a test whose scorer is + distinguishable from `EdgeListScore` (a test using an equivalent scorer cannot see #137); + and **porting the validated neighbourhood enumerator to the C++ `all_spr`/`all_tbr` paths**, + which were never audited — the R-side harness (validated against 2(n−3) and 2(n−3)(2n−7)) + found four distinct sampler defects in one pass and should generalise. diff --git a/dev/red-team/log.md b/dev/red-team/log.md index e9e8aa129..6bdf70274 100644 --- a/dev/red-team/log.md +++ b/dev/red-team/log.md @@ -60,7 +60,30 @@ persistently-dry reputation leans on pre-tier rounds (areas 3 and 10 both do) ha --- -area: 15 (Legacy pure-R search API) — first-ever review +area: 15 (Legacy pure-R search API) — second pass, opus arm of the tier-economics experiment +reviewed_by: opus finder afdeee5c3ba129361 + opus peer verifier af6672385451a1c75 (4 high + the weight corruption) + sonnet verifier a2466e9113439999b (sampler coverage + guard ordering) + haiku verifier ab75625e2caf8410b (13 code-tracing rows) +date: 2026-08-05 +tier: opus (Opus 5) +yield: **26 candidates, 26 confirmed, 0 refuted.** Filed as 7 grouped issues: #136 (high), #137 (high), #138 (high), #139 (high), #143 (med), #144 (med), #131 (low). One further defect fixed inline, logged not filed. +notes: **THE TIER-ECONOMICS EXPERIMENT IS ANSWERED, AND THE ANSWER IS "SEND OPUS FIRST". DO NOT RE-RUN THIS.** The hypothesis under test was that it is cheaper to let sonnet find what sonnet can find and have opus find only the remainder, than to send opus first. **Refuted empirically.** This arm ran over the *identical* scope with the sonnet arm's entire yield in its do-not-re-investigate block, and still returned **26 candidates including 4 sev:high** against the sonnet arm's 5 candidates and **0 sev:high** — after which every one of the 26 survived independent verification. The cheap pass did not reduce the expensive pass's work: opus still read all 2,185 loc and found five times as much, so `C_sonnet + C_opus > C_opus` held exactly as the cost argument predicted. Finder tokens were 137k (sonnet) vs 180k (opus) — the cheap rung was not even much cheaper on this scope. **The severity split is the real result:** sonnet found broken *documented contracts*; opus found *silent wrong answers*. Those are different classes, not different amounts, which is the capability cliff made concrete. Standing conclusion: **`start_tier: sonnet` on a never-visited area is not a saving.** The genuinely cheap lever this round exposed is verifier routing (the haiku batch cost 41k against the finder's 180k and confirmed 13 rows), not finder tier. + +**The four sev:high, all silent-failure class:** (a) **#136** — `Ratchet(stopAtScore=)` returns the **input** tree carrying the **improved** score: `edgeList <- candidate` sits at `R/Ratchet.R:190`, *below* the `break` at `:178`. Measured on Lobo: returned tree RF-0 from the input, `attr(,"score")` 223, independent `TreeLength()` 230. Two more symptoms share the cause (target-meeting tree never added to `forest` → the success path errors "No trees!?"; the already-met early return omits the score attribute and kills `MultiRatchet()`). (b) **#137** — `Ratchet()` never forwards `TreeScorer` to its `Bootstrapper`, and **`...` cannot rescue it because `TreeScorer` is a named formal** (`R/Ratchet.R:82`), so it is consumed by argument matching; instrumented run showed the custom scorer called **0 times** during both bootstrap phases against 391 overall. The perturbation and search phases optimise different objectives, in the one function whose reason to exist is a custom criterion. `Jackknife()` forwards it correctly. (c) **#138** — `TreeSearch()`'s default `RootedTBRSwap` holds the root fixed; the root split is a **hard invariant** (one distinct root bipartition over 20,000 calls), so a 150k-step walk reached **45 of 945** 7-tip topologies. A mid-rooted start confines the whole search to ~4.8% of tree space, permanently. `EdgeListScore` verified root-invariant (230 across rootings), so the loss is pure. (d) **#139** — `original_weight` is filled by `as.integer()` **truncation** (`R/PrepareData.R:76`) while scoring uses `.ScaleWeight()`; uniform fractional weights floor to all-zero, every character resamples to weight 0, every tree ties at 0, and `R/CustomSearch.R:74` accepts equal scores, so `BootstrapTree()`/`JackknifeTree()` degenerate into a random walk — **with zero errors, warnings or messages**, confirmed via `withCallingHandlers`. + +**VERIFICATION CORRECTED THE FINDER THREE TIMES — DO NOT TREAT A FINDER'S NUMBERS AS THE RECORD.** (i) A15-O-03 **downgraded** high→med: it is a loud immediate error, not a silent one. (ii) **#138's mechanism is worse but its motivating example was wrong** — the finder wrote "`NJTree()`/`rtree()`/`read.tree()` all yield mid-rooted trees"; **`NJTree()` is tip-rooted** (`RootTree(tree, names(dataset)[[1]])`, measured `1|47` on Lobo), so the package's own `?TreeSearch` example is in the benign 78-81% regime and real exposure is narrower than first stated. `ape::rtree(30)` (`24|6`), `read.tree` of a balanced Newick (`4|4`) and `BalancedTree(20)` (`10|10`) are the genuinely confined cases. (iii) **Three findings came back WORSE:** the `NNISwap` self-hit rate is `2/(nTips-2)` not `1/(nTips-2)` (40% at 7 tips, not 19.8%) because a root with two internal children makes **both** root-adjacent edges self-hitting — **the fix must exclude both, which the finder's numbers would not have told a reader**; `NNI(-1)` is not merely padded with duplicates but is **missing one internal edge's entire pair of true neighbours** (6 of 8 reachable, not 8 of 8); and `TBRSwap()` on a trifurcating root does not merely misbehave, it **HANGS INDEFINITELY** (killed after 2+ min), which is why #144 is `med` not `low`. + +**Verifier methodology worth reusing, not re-deriving.** Both peer verifiers built independent unrooted-neighbourhood enumerators (bisect every edge, suppress both cut nodes, reconnect at every edge pair, canonicalise by split set) and **validated them against the closed forms 2(n-3) for NNI and 2(n-3)(2n-7) for SPR before using them as ground truth** — *n* = 5-10, several tree shapes. That validation is what let them contradict the finder's numbers with confidence rather than deferring to them. One recorded a canonicalisation trap for whoever rebuilds it: **tied n/2|n/2 splits need lexicographic, not size-based, tie-breaking.** This harness would generalise to the C++ `all_spr`/`all_tbr` enumerators, which were **not** audited this round. + +**Fixed inline, not filed:** `R/SuccessiveApproximations.R:111-119` — the "Stability not reached" branch emitted its message unconditionally while the "converged" branch was gated on `verbosity`, so the default `verbosity = 0` call was noisy. Both branches now sit inside one gate. Nothing in `tests/`, `vignettes/`, `man/` or `R/` referenced the string. **Note an identical ungated `message()` survives at `R/Ratchet.R:291` in `MultiRatchet()`** — left for #131's sweep because that file has other pending changes. + +**Ruled out, do not redo:** `morphy-deprecated.R` is **clean** — all eight shims delegate to native replacements, none reaches a removed MorphyLib path, none loads a DLL, `UnloadMorphy()` correctly returns `invisible(0L)`, and `.GapHandler()`'s `pmatch` still accepts the legacy `"inapp"` abbreviation. Only cosmetic residue: `MorphyLength()`'s `nTaxa` and `PhyDat2Morphy()`'s `gap` are accepted and not forwarded, contradicting the `@param ... Passed to the replacement function` roxygen. Also ruled out: the `1:n` idiom fixed in the sonnet arm survives **nowhere else in scope** (grep clean); `RearrangeEdges()`'s double `TreeScorer()` call is **correctness-safe for deterministic scorers** (`parent`/`child` are not mutated between the two evaluations) and is a hazard only for a stochastic custom scorer, because the swapper's RNG draws are interleaved — a documentation sentence, not a fix; `Ratchet()`'s accept criterion **is** monotone on the normal exit path (`R/Ratchet.R:188-196`), the failures being confined to the `stopAtScore` short-circuit and the epsilon mismatch; `.NonDuplicateRoot()` (`R/SPR.R:42-54`) **is correct** including its asymmetric branching — do not "simplify" it; and `TBRSwap()` is the only sampler here with **full** neighbourhood coverage (64/64, 106/106). + +**Not chased, for the next reviewer:** `TBRSwap()`'s residual identity rate (1.7-2.65% under `BalancedTree`'s own rooting) is measured but its mechanism is unidentified — it is *not* the `brokenRootDaughters` bug and *not* a warning path; whether `SPRSwap()` can reach `nCandidates == 0` and die on `parent[[integer(0)]]` (`R/SPR.R:231-248`, guarded only by a commented-out assertion the author clearly worried about); `PrepareDataSA()` (`R/SuccessiveApproximations.R:191-216`) is reachable only through the broken `SuccessiveWeights()` and so is entirely untested — its taxon-major fill was read and looks correct, but nothing has exercised it; and `NNISwap()`/`RootedNNISwap()` derive `rootNode <- nTips + 1L` rather than reading it from the edge list (`R/NNI.R:117`, `:214`) — holds for every current caller, would silently mis-classify a hand-built `phylo`. + +Seam status: **still yielding, abundantly — nowhere near dry.** Next visit stays **opus** with a fresh agent. Highest-value next work-shape is **not** another general finder: the finder itself proposed, and the evidence supports, a targeted audit of *which advertised custom-criterion entry points actually work end to end* (#137 + #126 + the dead `SuccessiveWeights()` are one decayed-façade story), plus porting the validated neighbourhood enumerator to the C++ `all_spr`/`all_tbr` paths. + +--- + +area: 15 (Legacy pure-R search API) — first-ever review, sonnet arm of the tier-economics experiment reviewed_by: sonnet finder afbe83c1d183868a3 + sonnet peer verifier abd8f225317a23c4c (3 med, R-semantics verdicts) + haiku verifier af328ab30afd622f0 (2 low, artifact-tracing verdicts) date: 2026-08-05 tier: sonnet (Sonnet 5) From e060c30572d8a1aec462bb46b5e4be4b88907839 Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Thu, 6 Aug 2026 09:13:40 +0100 Subject: [PATCH 3/8] red-team: move area 15's round records to Discussions The round record has no business waiting on a code review. This file sits on a protected branch, and four completed rounds with 56 filed findings once sat stranded on an unmerged PR while `last_focus:` here still named a stale area -- so the next dispatch would have re-swept an area already reviewed twice that day. Area 15's three rounds now live as one Discussion post each, under that area's category. Removes the two entries added earlier on this branch and replaces them with a pointer. `last_focus:` stays live and stays current: the new scheme picks the stalest category rather than following a pointer, but that ordering cannot be computed until every area has a discussion, and the backfill has to post oldest-first so createdAt reproduces true staleness. Only area 15 is migrated so far. The historical entries below the pointer stay put -- eleven in-repo files and the /red-team skill cite this path, and the T-nnn ids are frozen. Co-Authored-By: Claude Opus 5 --- dev/red-team/log.md | 71 ++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 43 deletions(-) diff --git a/dev/red-team/log.md b/dev/red-team/log.md index 6bdf70274..6459e0c6d 100644 --- a/dev/red-team/log.md +++ b/dev/red-team/log.md @@ -60,49 +60,34 @@ persistently-dry reputation leans on pre-tier rounds (areas 3 and 10 both do) ha --- -area: 15 (Legacy pure-R search API) — second pass, opus arm of the tier-economics experiment -reviewed_by: opus finder afdeee5c3ba129361 + opus peer verifier af6672385451a1c75 (4 high + the weight corruption) + sonnet verifier a2466e9113439999b (sampler coverage + guard ordering) + haiku verifier ab75625e2caf8410b (13 code-tracing rows) -date: 2026-08-05 -tier: opus (Opus 5) -yield: **26 candidates, 26 confirmed, 0 refuted.** Filed as 7 grouped issues: #136 (high), #137 (high), #138 (high), #139 (high), #143 (med), #144 (med), #131 (low). One further defect fixed inline, logged not filed. -notes: **THE TIER-ECONOMICS EXPERIMENT IS ANSWERED, AND THE ANSWER IS "SEND OPUS FIRST". DO NOT RE-RUN THIS.** The hypothesis under test was that it is cheaper to let sonnet find what sonnet can find and have opus find only the remainder, than to send opus first. **Refuted empirically.** This arm ran over the *identical* scope with the sonnet arm's entire yield in its do-not-re-investigate block, and still returned **26 candidates including 4 sev:high** against the sonnet arm's 5 candidates and **0 sev:high** — after which every one of the 26 survived independent verification. The cheap pass did not reduce the expensive pass's work: opus still read all 2,185 loc and found five times as much, so `C_sonnet + C_opus > C_opus` held exactly as the cost argument predicted. Finder tokens were 137k (sonnet) vs 180k (opus) — the cheap rung was not even much cheaper on this scope. **The severity split is the real result:** sonnet found broken *documented contracts*; opus found *silent wrong answers*. Those are different classes, not different amounts, which is the capability cliff made concrete. Standing conclusion: **`start_tier: sonnet` on a never-visited area is not a saving.** The genuinely cheap lever this round exposed is verifier routing (the haiku batch cost 41k against the finder's 180k and confirmed 13 rows), not finder tier. - -**The four sev:high, all silent-failure class:** (a) **#136** — `Ratchet(stopAtScore=)` returns the **input** tree carrying the **improved** score: `edgeList <- candidate` sits at `R/Ratchet.R:190`, *below* the `break` at `:178`. Measured on Lobo: returned tree RF-0 from the input, `attr(,"score")` 223, independent `TreeLength()` 230. Two more symptoms share the cause (target-meeting tree never added to `forest` → the success path errors "No trees!?"; the already-met early return omits the score attribute and kills `MultiRatchet()`). (b) **#137** — `Ratchet()` never forwards `TreeScorer` to its `Bootstrapper`, and **`...` cannot rescue it because `TreeScorer` is a named formal** (`R/Ratchet.R:82`), so it is consumed by argument matching; instrumented run showed the custom scorer called **0 times** during both bootstrap phases against 391 overall. The perturbation and search phases optimise different objectives, in the one function whose reason to exist is a custom criterion. `Jackknife()` forwards it correctly. (c) **#138** — `TreeSearch()`'s default `RootedTBRSwap` holds the root fixed; the root split is a **hard invariant** (one distinct root bipartition over 20,000 calls), so a 150k-step walk reached **45 of 945** 7-tip topologies. A mid-rooted start confines the whole search to ~4.8% of tree space, permanently. `EdgeListScore` verified root-invariant (230 across rootings), so the loss is pure. (d) **#139** — `original_weight` is filled by `as.integer()` **truncation** (`R/PrepareData.R:76`) while scoring uses `.ScaleWeight()`; uniform fractional weights floor to all-zero, every character resamples to weight 0, every tree ties at 0, and `R/CustomSearch.R:74` accepts equal scores, so `BootstrapTree()`/`JackknifeTree()` degenerate into a random walk — **with zero errors, warnings or messages**, confirmed via `withCallingHandlers`. - -**VERIFICATION CORRECTED THE FINDER THREE TIMES — DO NOT TREAT A FINDER'S NUMBERS AS THE RECORD.** (i) A15-O-03 **downgraded** high→med: it is a loud immediate error, not a silent one. (ii) **#138's mechanism is worse but its motivating example was wrong** — the finder wrote "`NJTree()`/`rtree()`/`read.tree()` all yield mid-rooted trees"; **`NJTree()` is tip-rooted** (`RootTree(tree, names(dataset)[[1]])`, measured `1|47` on Lobo), so the package's own `?TreeSearch` example is in the benign 78-81% regime and real exposure is narrower than first stated. `ape::rtree(30)` (`24|6`), `read.tree` of a balanced Newick (`4|4`) and `BalancedTree(20)` (`10|10`) are the genuinely confined cases. (iii) **Three findings came back WORSE:** the `NNISwap` self-hit rate is `2/(nTips-2)` not `1/(nTips-2)` (40% at 7 tips, not 19.8%) because a root with two internal children makes **both** root-adjacent edges self-hitting — **the fix must exclude both, which the finder's numbers would not have told a reader**; `NNI(-1)` is not merely padded with duplicates but is **missing one internal edge's entire pair of true neighbours** (6 of 8 reachable, not 8 of 8); and `TBRSwap()` on a trifurcating root does not merely misbehave, it **HANGS INDEFINITELY** (killed after 2+ min), which is why #144 is `med` not `low`. - -**Verifier methodology worth reusing, not re-deriving.** Both peer verifiers built independent unrooted-neighbourhood enumerators (bisect every edge, suppress both cut nodes, reconnect at every edge pair, canonicalise by split set) and **validated them against the closed forms 2(n-3) for NNI and 2(n-3)(2n-7) for SPR before using them as ground truth** — *n* = 5-10, several tree shapes. That validation is what let them contradict the finder's numbers with confidence rather than deferring to them. One recorded a canonicalisation trap for whoever rebuilds it: **tied n/2|n/2 splits need lexicographic, not size-based, tie-breaking.** This harness would generalise to the C++ `all_spr`/`all_tbr` enumerators, which were **not** audited this round. - -**Fixed inline, not filed:** `R/SuccessiveApproximations.R:111-119` — the "Stability not reached" branch emitted its message unconditionally while the "converged" branch was gated on `verbosity`, so the default `verbosity = 0` call was noisy. Both branches now sit inside one gate. Nothing in `tests/`, `vignettes/`, `man/` or `R/` referenced the string. **Note an identical ungated `message()` survives at `R/Ratchet.R:291` in `MultiRatchet()`** — left for #131's sweep because that file has other pending changes. - -**Ruled out, do not redo:** `morphy-deprecated.R` is **clean** — all eight shims delegate to native replacements, none reaches a removed MorphyLib path, none loads a DLL, `UnloadMorphy()` correctly returns `invisible(0L)`, and `.GapHandler()`'s `pmatch` still accepts the legacy `"inapp"` abbreviation. Only cosmetic residue: `MorphyLength()`'s `nTaxa` and `PhyDat2Morphy()`'s `gap` are accepted and not forwarded, contradicting the `@param ... Passed to the replacement function` roxygen. Also ruled out: the `1:n` idiom fixed in the sonnet arm survives **nowhere else in scope** (grep clean); `RearrangeEdges()`'s double `TreeScorer()` call is **correctness-safe for deterministic scorers** (`parent`/`child` are not mutated between the two evaluations) and is a hazard only for a stochastic custom scorer, because the swapper's RNG draws are interleaved — a documentation sentence, not a fix; `Ratchet()`'s accept criterion **is** monotone on the normal exit path (`R/Ratchet.R:188-196`), the failures being confined to the `stopAtScore` short-circuit and the epsilon mismatch; `.NonDuplicateRoot()` (`R/SPR.R:42-54`) **is correct** including its asymmetric branching — do not "simplify" it; and `TBRSwap()` is the only sampler here with **full** neighbourhood coverage (64/64, 106/106). - -**Not chased, for the next reviewer:** `TBRSwap()`'s residual identity rate (1.7-2.65% under `BalancedTree`'s own rooting) is measured but its mechanism is unidentified — it is *not* the `brokenRootDaughters` bug and *not* a warning path; whether `SPRSwap()` can reach `nCandidates == 0` and die on `parent[[integer(0)]]` (`R/SPR.R:231-248`, guarded only by a commented-out assertion the author clearly worried about); `PrepareDataSA()` (`R/SuccessiveApproximations.R:191-216`) is reachable only through the broken `SuccessiveWeights()` and so is entirely untested — its taxon-major fill was read and looks correct, but nothing has exercised it; and `NNISwap()`/`RootedNNISwap()` derive `rootNode <- nTips + 1L` rather than reading it from the edge list (`R/NNI.R:117`, `:214`) — holds for every current caller, would silently mis-classify a hand-built `phylo`. - -Seam status: **still yielding, abundantly — nowhere near dry.** Next visit stays **opus** with a fresh agent. Highest-value next work-shape is **not** another general finder: the finder itself proposed, and the evidence supports, a targeted audit of *which advertised custom-criterion entry points actually work end to end* (#137 + #126 + the dead `SuccessiveWeights()` are one decayed-façade story), plus porting the validated neighbourhood enumerator to the C++ `all_spr`/`all_tbr` paths. - ---- - -area: 15 (Legacy pure-R search API) — first-ever review, sonnet arm of the tier-economics experiment -reviewed_by: sonnet finder afbe83c1d183868a3 + sonnet peer verifier abd8f225317a23c4c (3 med, R-semantics verdicts) + haiku verifier af328ab30afd622f0 (2 low, artifact-tracing verdicts) -date: 2026-08-05 -tier: sonnet (Sonnet 5) -yield: **3 confirmed findings, 0 refuted of 5 candidates.** Filed: #125 (A15-01, sev:med — groups candidates 1/2/3/5), #126 (A15-02, sev:low — candidate 4). Fixed inline, logged not filed: A15-03. **Zero sev:high.** -notes: **THIS ROUND IS THE SONNET ARM OF A DELIBERATE TIER-ECONOMICS EXPERIMENT — DO NOT DISPATCH THE OPUS ARM WITHOUT READING THIS PARAGRAPH.** The question under test: is it cheaper to let sonnet find what sonnet can find and then have opus find only the remainder, than to send opus first? Area 15 was chosen because it was the only never-visited area — nothing here had been pre-harvested, so a first pass measures the cheap tier honestly. **The opus arm must run over the identical scope with a fresh agent, with #125/#126 AND the A15-03 inline fix in the "do not re-investigate" block**, and its yield recorded here for comparison. Anything else measures a different question. Prior to this round the record was: sonnet 6 rounds / **0** sev:high ever; opus 24 rounds / 11 producing at least one. This round did not break that pattern. - -**The scope row's headline question was stale and would have burned the round.** The row still cited #16 (`TreeState::init_from_edge` OOB on multifurcating trees) as live, naming `EdgeListScore()` as a vulnerable entry point. **#16 is CLOSED/COMPLETED** — the boundary guard landed in PR #50 — so the brief reframed it before dispatch: the C++ boundary now *rejects* non-binary trees, so the live question is whether the pure-R layer above hands it inputs it will now reject. **Answer: no, and the seam is doubly guarded.** `TreeSearch()`/`Ratchet()` both already `stop("tree must be bifurcating")` on entry (`R/CustomSearch.R:209`, `R/Ratchet.R:96`), predating and independent of the C++ fix, and `EdgeListScore()` itself carries its own `tabulate(parent)` binary check (`R/tree_length.R:611-618`) that looks to have been added in response to #16's family. A 600-iteration chained fuzz of `RootedNNI`/`RootedSPR`/`RootedTBR` on the default random-move path, with full structural-sanity checks, found zero corruption — **the default heuristic-search rearrangement path does not spontaneously build non-binary trees.** Recorded as measured, not assumed: reopen if the C++ guard is ever relaxed, or if a new caller bypasses those two entry checks. - -**The actual find is a documented contract that is broken almost everywhere it is documented.** `man/NNI.Rd`/`man/TBR.Rd` promise that `edgeToBreak = -1` returns "a complete list of all trees one step from the input tree". Six exported functions inherit that promise; **one honours it.** `NNI()` works; `RootedNNI()` errors *unused arguments* (`R/NNI.R:196` — the anonymous function's body is the bare symbol `` `[[<-` ``, so `lapply`'s `...` hits a one-formal closure); `RootedSPR()`/`RootedSPRSwap()` error *object 'tree' not found* (`AllSPR()` references a `tree` that is not among its formals); `TBR()`/`RootedTBR()` have **no `-1` branch at all** and fall into the ordinary `edgeToBreak < 1` bounds check, so they warn and return the input tree **unchanged** — the only silent failure of the four, and the dangerous one. `SPR()` alone refuses deliberately, via an unconditional `stop("Negative edgeToBreak not yet supported; please request on GitHub")` at `R/SPR.R:101` that makes `:102-107` dead code. **That `stop()` is the key to the fix and reframes the finding**: this is one maintainer decision, not four bugs — refusing uniformly at the other five sites (and dropping the `-1` sentence from the docs) is a handful of lines and eliminates the silent class outright. No test anywhere passes `edgeToBreak = -1` to any of the six. - -**A15-03, fixed inline, not filed:** `EdgeListSearch()` looped `for (iter in 1:maxIter)` (`R/CustomSearch.R:62`). `1:0` is `c(1, 0)`, so `maxIter = 0` silently performed **two** rearrangement iterations instead of none — and since `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 propagates from `Bootstrap()`, `Jackknife()` and `Ratchet()`, so `maxIter = 0` is reachable as a plausible "score without searching" idiom. Fixed to `seq_len(maxIter)` with `iter <- 0L` pre-initialised, which is load-bearing: `iter` is read after the loop at `:111`. **Pinned by a regression test** (`test-CustomSearch.R`) using mocks that error if called — `EdgeListSearch()` is exported, so this needs no data and no C++. Verified pre/post by rebuilding the pre-fix loop bound in an isolated copy of the function: post-fix the loop body is never entered, pre-fix it is. - -**Verifier routing note worth keeping.** Candidates 1/2/3 were `sev:med`, which the severity rule alone would send to haiku — but all three turn on **R language/roxygen semantics** rather than on tracing project code, so they went peer-tier under the "route by what decides the verdict" rule. That paid: on candidate 2 the peer verifier found that **a global variable named `tree` masks the bug entirely** (lexical scoping reaches it from `AllSPR`'s namespace parent; a *local* `tree` in the caller does not), and it initially produced a false negative that way before re-running in a clean environment. A cheap verifier would plausibly have refuted a real finding. That is now the third time this project has been bitten by cheap-verifying a library/language-fact verdict. - -**Not chased, for the next reviewer:** (a) `SuccessiveWeights()` (`R/SuccessiveApproximations.R:175-189`) calls `CharacterLength()` rather than the kernel — whether its scoring agrees with the C++ kernel for the same tree and weights is untraced, and #83 already shows `TreeLength()`/`EdgeListScore()` disagreeing by default on extended IW, so this looks live; (b) `RearrangeEdges()` (`R/tree_rearrangement.R:36-94`) calls `TreeScorer()` **twice per iteration** when `scoreToBeat` is not supplied — once for the default argument, once for `candidateScore` — a perf observation only, not investigated for correctness, and arguably a `/profile` item rather than a red-team one; (c) `MultiRatchet()`/`Ratchet()`'s `swappers=` list combined with a caller passing `edgeToBreak = -1` through `...` would reach the broken paths — no such call site exists in this codebase, but the docs promise it, so a downstream user script plausibly could. - -**Re-verified, not re-filed:** #119's `R/Bootstrap.R` `sample()` length-1 trap. `deindexedChars` can have length 1 only when the entire dataset compresses to a single unique pattern of weight exactly 1 — genuinely degenerate, so #119's "unreachable with normal weights" characterisation holds. **Not re-filed:** #83, confirmed still present; its specific two-path shape does **not** extend to `SuccessiveApproximations()`, which builds `contrast`/`tip_data`/`weight` itself and calls the kernel directly, so it has only one path. - -Seam status: **still yielding** (5 candidates, 5 confirmed, 0 refuted, on a first pass). **NEXT VISIT: the opus arm of the experiment above — same scope, fresh agent, #125/#126/A15-03 in the do-not-re-investigate block.** This is not an escalation on the usual trigger (the seam did not run dry); it is a deliberate paired measurement. +## ⚠ Round records have moved to GitHub Discussions — this file is closed to new entries + +**Area 15's three rounds of 2026-08-05/06 are deliberately NOT below.** They are the first +records written under the new scheme, and they live in the repository's Discussions, one post +per round under that area's category: + + + +Why the move: this file sits on a protected branch, so a finished round's record was hostage to +a code review it has nothing to do with. Four completed rounds and 56 filed findings once sat +stranded on an unmerged PR while this file still named a stale `last_focus:` — so the next +dispatch would have re-swept an area that had already been reviewed twice that day. Discussions +decouple the record from the merge. + +**Do not add new round entries here.** Post to the area's Discussions category instead. Title +format `RT - area - () - yield `; first line of the body +` () | effort: `. + +**`last_focus:` at the bottom is still live and still governs rotation.** Under the new scheme +the next area is the *stalest* category rather than a pointer — but that ordering cannot be +computed until every area has at least one discussion, and the backfill must post each area's +most recent round **oldest-first** so `createdAt` reproduces true staleness. Only area 15 has +been migrated. Until the rest are, `last_focus:` remains the rotation mechanism and must be +kept current. + +Everything below this line is the frozen historical record, newest first. **It stays**: eleven +in-repo files and the `/red-team` skill cite `log.md` by path, and the `T-nnn` ids it carries +are frozen, not retired. --- From 8e5d5472352ed80ff686feaf812439224191afb7 Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Sat, 22 Aug 2026 07:34:25 +0100 Subject: [PATCH 4/8] docs(red-team): retire last_focus now the backfill is complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 15 areas have a Discussion record, so rotation reads staleness from createdAt and the pointer is dead. Records the invariant createdAt relies on — creation order equals review-recency order — which the backfill broke and discussion #184 restored. Co-Authored-By: Claude Opus 5 --- dev/red-team/log.md | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/dev/red-team/log.md b/dev/red-team/log.md index 6459e0c6d..34e82fa17 100644 --- a/dev/red-team/log.md +++ b/dev/red-team/log.md @@ -62,11 +62,9 @@ persistently-dry reputation leans on pre-tier rounds (areas 3 and 10 both do) ha ## ⚠ Round records have moved to GitHub Discussions — this file is closed to new entries -**Area 15's three rounds of 2026-08-05/06 are deliberately NOT below.** They are the first -records written under the new scheme, and they live in the repository's Discussions, one post -per round under that area's category: +Every round is now one post in its area's Discussions category, `NN-`: - + Why the move: this file sits on a protected branch, so a finished round's record was hostage to a code review it has nothing to do with. Four completed rounds and 56 filed findings once sat @@ -76,14 +74,19 @@ decouple the record from the merge. **Do not add new round entries here.** Post to the area's Discussions category instead. Title format `RT - area - () - yield `; first line of the body -` () | effort: `. - -**`last_focus:` at the bottom is still live and still governs rotation.** Under the new scheme -the next area is the *stalest* category rather than a pointer — but that ordering cannot be -computed until every area has at least one discussion, and the backfill must post each area's -most recent round **oldest-first** so `createdAt` reproduces true staleness. Only area 15 has -been migrated. Until the rest are, `last_focus:` remains the rotation mechanism and must be -kept current. +` () | effort: | `. + +**Migration complete as of 2026-08-06.** All 15 areas have a record: area 15's three rounds +(#152-154), plus each other area's most recent round backfilled verbatim from this file +(#158-171). + +**`last_focus:` is retired.** The next area is the one whose most recent Discussion has the +oldest `createdAt`. That rests on an invariant — **creation order equals review-recency +order** — which the backfill broke and discussion #184 restored: area 15's three records were +posted before the other fourteen areas were backfilled, so by creation order the most recently +reviewed area looked like the stalest one. Any future backfill or out-of-order re-post must +restore the invariant the same way, with a marker record. The value below is left as a +historical marker and is **not** to be updated. Everything below this line is the frozen historical record, newest first. **It stays**: eleven in-repo files and the `/red-team` skill cite `log.md` by path, and the `T-nnn` ids it carries From 228177e9baeeefb1cda6cc86c34a8139cc8828bb Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Sat, 22 Aug 2026 07:38:08 +0100 Subject: [PATCH 5/8] docs(red-team): the log.md header still described last_focus rotation The header claimed /red-team appends an entry and updates last_focus, which the same file now says is retired. Replaced with what the file actually is. Co-Authored-By: Claude Opus 5 --- dev/red-team/log.md | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/dev/red-team/log.md b/dev/red-team/log.md index 34e82fa17..7d82ad6dc 100644 --- a/dev/red-team/log.md +++ b/dev/red-team/log.md @@ -1,13 +1,9 @@ # Red-team round log — TreeSearch -Append-only record of every red-team round. **Newest first.** Each invocation of -`/red-team` adds one entry and updates `last_focus:` at the **bottom** of this file. The -next area is `(last_focus mod N) + 1`, where `N` is the current row count in -`focus-areas.md` (**15 as of 2026-08-05**, when areas 14 and 15 were added to close #42's -scope-coverage gap — previously 13 as of 2026-07-03, and **not** the stale `10` this line -said until then, which made areas 11-13 mathematically unreachable by normal rotation; see -RT12-01, 2026-07-03 area-12 round below). **Count the rows; do not trust this number.** -Recompute `N` whenever a row is added. +**Closed to new entries.** A round's record is a GitHub Discussion, one post per round in +that area's category, and rotation reads staleness from those posts — see the pointer block +below. What this file still carries: the model-version legend, the `T-nnn` ids that shipped +source comments cite, and the frozen pre-2026-08 history, **newest first**. **Entry format** (per round): `area`, `reviewed_by`, `date`, `tier` — **which now records the model *version* that ran, not just the rung** (`tier: opus (Opus 4.8)`) — `yield` (count of From 0768b4e3a9d4c67b5c25ffa419c8ca7aaa25a5d1 Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Sat, 22 Aug 2026 07:44:35 +0100 Subject: [PATCH 6/8] fix(search): report 0 rearrangements when maxIter = 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `iter <- 0L` added alongside the seq_len() fix was dead: `for` binds its variable unconditionally, so a zero-length sequence leaves `iter` NULL rather than falling through to the earlier assignment. The verbosity report then printed "after rearrangements." with a blank. Handle it where it is read, and cover it — the existing maxIter = 0 test runs at verbosity 0, so it could not see this. Co-Authored-By: Claude Opus 5 --- R/CustomSearch.R | 5 +++-- tests/testthat/test-CustomSearch.R | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/R/CustomSearch.R b/R/CustomSearch.R index 859e91142..56d91fa60 100644 --- a/R/CustomSearch.R +++ b/R/CustomSearch.R @@ -59,7 +59,6 @@ EdgeListSearch <- function (edgeList, dataset, } hits <- 0L unimprovedSince <- 0L - iter <- 0L for (iter in seq_len(maxIter)) { candidateLists <- RearrangeEdges(edgeList[[1]], edgeList[[2]], @@ -108,7 +107,9 @@ EdgeListSearch <- function (edgeList, dataset, } if (verbosity > 0L) { #nocov start message(" - Final score ", bestScore, " found ", hits, " times after ", - iter, " rearrangements.", if (verbosity > 1L) "\n" else "") + # A zero-length loop leaves `iter` NULL rather than unset + if (is.null(iter)) 0L else iter, + " rearrangements.", if (verbosity > 1L) "\n" else "") } #nocov end edgeList[3:4] <- c(bestScore, hits) diff --git a/tests/testthat/test-CustomSearch.R b/tests/testthat/test-CustomSearch.R index f8f352e4b..fd2ed3511 100644 --- a/tests/testthat/test-CustomSearch.R +++ b/tests/testthat/test-CustomSearch.R @@ -122,4 +122,11 @@ test_that("EdgeListSearch() performs no rearrangements when maxIter = 0", { maxIter = 0, verbosity = 0L) expect_equal(searched[[3]], 99) # Starting score returned unchanged expect_equal(searched[[4]], 0L) # No hits recorded + + # A zero-length `for` leaves the loop variable NULL, not 0 + expect_message(EdgeListSearch(list(edge[, 1], edge[, 2], 99), dataset = NULL, + TreeScorer = MustNotRun, + EdgeSwapper = MustNotRun, + maxIter = 0, verbosity = 1L), + "after 0 rearrangements") }) From 232164c31fb6243a525934176ae854bf52cb24da Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Sat, 22 Aug 2026 07:46:52 +0100 Subject: [PATCH 7/8] docs(red-team): cut the archaeology from the area 14 and 15 scope rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These rows argued with superseded versions of themselves — why a start_tier was chosen and why it no longer binds, which #42 urgency justified the row and why it is discharged. A scope row briefs the next round; it is not a record of how it came to say what it says. Keeps the operative content: scope, seam verdict, the standing prohibition on re-running the tier experiment, the transferable lesson, and the next-visit leads. Co-Authored-By: Claude Opus 5 --- dev/red-team/focus-areas.md | 74 ++++++++++++------------------------- 1 file changed, 24 insertions(+), 50 deletions(-) diff --git a/dev/red-team/focus-areas.md b/dev/red-team/focus-areas.md index c962b1bef..151140e4e 100644 --- a/dev/red-team/focus-areas.md +++ b/dev/red-team/focus-areas.md @@ -142,61 +142,35 @@ top of `log.md`; seams that a version bump has made re-eligible are queued in reading the backlog row that holds the actual ask (item 7 explains this at length). Whoever takes area 13 next must decide explicitly: harness first, or #18/#19 first — both are live, and the harness plan predates the two findings. -- **14 Statistics & support metrics — MEASURED 2026-08-05, still yielding heavily.** Added - 2026-08-05 from #42's scope-coverage diff: 5,553 lines across 14 files that were owned by no - area and therefore never reviewed at any tier. **The gap has already cost a finding** — the - arm64 `probe_slot()` hang in `src/MaddisonSlatkin.cpp` (fixed, PR #272, - cf. [[maddisonslatkin-arm64-profile-hang]]) was found incidentally, not by rotation. The code - is numerically dense — recursive DP, factorial caches, log-space arithmetic, Monte Carlo - fallbacks — the profile the tier doctrine normally reserves for `opus`, and #42 recommended - `opus` on that basis. **Deliberately starting at `sonnet` anyway** (maintainer decision, - 2026-08-05): density is a prediction about where bugs *hide*, not evidence that cheap sweeps - are exhausted, and this area has no measured yield at all. **Overtaken by events:** the - first-ever review had already run at `opus` on 2026-08-05, before this row merged, and returned - **36 findings, 4 sev:high — the highest yield on record for this rotation** (see `log.md`). - `start_tier` is left at `sonnet` as decided, but it is now inert: the seam is measured and - yielding, so the routing rules keep the next visit at **opus** with a fresh agent. - **Next visit starts here** (the round's own leads, and the reason it stays opus): the - **array-dimension-drop pattern** — four independent instances in one round (`ConcordanceTable`, - `ClusteringConcordance`, `Consistency`, `ClusterStrings`, all missing `drop = FALSE`), so treat - it as a class and sweep for it rather than re-finding instances; and the **not-yet-examined - `R/PresentContra.R` forest/reference-tip-mismatch angle** — read but never exercised against a - forest whose trees have tips absent from the reference (it calls `KeepTip` first, which *should* - be safe, but that is unproven). Its own test convention +- **14 Statistics & support metrics — MEASURED 2026-08-05, still yielding heavily.** 5,553 + lines across 14 files. The code is numerically dense — recursive DP, factorial caches, + log-space arithmetic, Monte Carlo fallbacks — so brief for that: the first review returned + **36 findings, 4 sev:high, the highest yield on record for this rotation**. Next visit stays + at **opus** with a fresh agent. + **Next visit starts here:** the **array-dimension-drop pattern** — four independent instances + in one round (`ConcordanceTable`, `ClusteringConcordance`, `Consistency`, `ClusterStrings`, + all missing `drop = FALSE`), so sweep for it as a class rather than re-finding instances; and + the **not-yet-examined `R/PresentContra.R` forest/reference-tip-mismatch angle** — read but + never exercised against a forest whose trees have tips absent from the reference (it calls + `KeepTip` first, which *should* be safe, but that is unproven). Its own test convention (`test-MaddisonSlatkin.R`, `test-Concordance.R`, `test-ParsSim.R`, `test-Consistency.R`, `test-ScoreSpectrum.R`, `test-QuartetResolution.R`, `test-TaxonInfluence.R`, `test-WideSample.R`, `test-pp-*.R`) is a useful first read. - **15 Legacy pure-R search API — MEASURED 2026-08-05 at BOTH sonnet and opus; yielding - heavily. The `start_tier` column still reads `sonnet` — the maintainer's recorded choice on - #42 — but the experiment below falsifies the reasoning behind it. Left unchanged pending the - maintainer's call; it is inert either way, since routing governs a visited area.** Added - 2026-08-05 from #42's scope-coverage diff: 2,183 lines across 9 files backing the - still-shipped pre-C++-engine search functions, owned by no area. #42 offered "review once as - frozen legacy, then deprioritise"; **the maintainer chose a full rotation area instead - (2026-08-05): keep revisiting until the seam stops yielding.** Legacy is not the same as - clean, and this code is still shipped and still the documented entry point for users who have - not moved to the C++ engine. Treat "it isn't growing" as a reason the seam should *exhaust* + heavily.** 2,183 lines across 9 files backing the still-shipped pre-C++-engine search + functions. Keep revisiting until the seam stops yielding: legacy is not the same as clean, + and this code is still shipped and still the documented entry point for users who have not + moved to the C++ engine. Treat "it isn't growing" as a reason the seam should *exhaust* quickly, not as a reason to stop early. - **The `#16`/`EdgeListScore()` urgency that originally justified this row is now discharged** - — #16 is closed, its guard landed in PR #50, and the first round measured the pure-R layer - above it as *doubly* guarded (`R/CustomSearch.R:209` and `R/Ratchet.R:96` both reject - non-bifurcating input independently of the C++ fix; a 600-iteration fuzz of the default - rearrangement path produced no non-binary trees). Do not spend a second round re-asking it; - reopen only if the C++ guard is relaxed or a new caller bypasses those entry checks. - **`src/rearrange.cpp` added 2026-08-06** while closing #147: `all_tbr()` had never broken the - root edge, so `TBRMoves()` returned a strict subset of `SPRMoves()` for six years. The bug is - instructive twice over. It was an **off-by-one propagated by copy**: `all_spr()` was created - in 2020 as a copy of `all_tbr()`, inherited its `break_seq` starting at edge 3, and had that - corrected in PR #65 (2021) — the parent never was. And two `dev/benchmarks/` scripts had - already *characterised* the omission and routed around it by enumerating at two rootings, - without anyone filing it. **A documented workaround for a package deficiency is a finding - that was never written down** — grep `dev/` for such comments when auditing a new file. - **This row carried the tier-economics experiment, and it settled the tier question for good.** - Paired passes over the identical scope on 2026-08-05: `sonnet` returned 5 candidates and **0 - sev:high**; `opus`, handed sonnet's entire yield as off-limits, returned **26 candidates and 4 - sev:high**, all 26 confirmed. The cheap pass removed no work from the expensive one. Sonnet - found broken *documented contracts*; opus found *silent wrong answers* — different classes, - not different amounts. See the log's two 2026-08-05 entries; **do not re-run this experiment.** + **Do not re-run the tier experiment.** Paired passes over identical scope, 2026-08-05: + `sonnet` returned 5 candidates and **0 sev:high**; `opus`, handed sonnet's entire yield as + off-limits, returned **26 candidates and 4 sev:high**, all confirmed. The cheap pass removed + no work from the expensive one. Sonnet found broken *documented contracts*, opus found + *silent wrong answers* — different classes, not different amounts. + **A documented workaround for a package deficiency is a finding that was never written + down** — grep `dev/` for such comments when auditing a new file. Two `dev/benchmarks/` + scripts had characterised `all_tbr()`'s missing root-edge break and routed around it by + enumerating at two rootings, and nobody filed it for six years (#147, `src/rearrange.cpp`). **Next visit starts here** (stay at `opus`, fresh agent, and prefer a targeted shape over another general finder): the **decayed custom-criterion façade** — #137 (`Ratchet()` never forwards `TreeScorer`), #126 (`SuccessiveApproximations()`'s undocumented capability gap) and From 63b7e70ac449c8f613abf38740af94def4c7fb70 Mon Sep 17 00:00:00 2001 From: ms609-agent <313734811+ms609-agent@users.noreply.github.com> Date: Sat, 22 Aug 2026 07:57:13 +0100 Subject: [PATCH 8/8] fix(search): don't announce a tree search that will not happen "Performing tree search" was emitted before maxIter was consulted, so maxIter = 0 claimed a search had started and only the closing summary contradicted it. Gate the claim instead, and ask maxIter directly rather than inferring it from the loop variable being left NULL. Co-Authored-By: Claude Opus 5 --- R/CustomSearch.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/CustomSearch.R b/R/CustomSearch.R index 56d91fa60..1f0bdce1a 100644 --- a/R/CustomSearch.R +++ b/R/CustomSearch.R @@ -46,7 +46,7 @@ EdgeListSearch <- function (edgeList, dataset, bestScore <- edgeList[[3]] } } - if (verbosity > 0L) { + if (verbosity > 0L && maxIter > 0L) { message(" - Performing tree search. Initial score: ", bestScore) #nocov } if (!is.null(stopAtScore) && bestScore < stopAtScore + epsilon) { @@ -107,8 +107,8 @@ EdgeListSearch <- function (edgeList, dataset, } if (verbosity > 0L) { #nocov start message(" - Final score ", bestScore, " found ", hits, " times after ", - # A zero-length loop leaves `iter` NULL rather than unset - if (is.null(iter)) 0L else iter, + # `for` leaves the loop variable NULL when maxIter < 1 + if (maxIter > 0L) iter else 0L, " rearrangements.", if (verbosity > 1L) "\n" else "") } #nocov end