diff --git a/R/CustomSearch.R b/R/CustomSearch.R index 188f5df4b..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) { @@ -59,8 +59,8 @@ EdgeListSearch <- function (edgeList, dataset, } hits <- 0L unimprovedSince <- 0L - - for (iter in 1:maxIter) { + + for (iter in seq_len(maxIter)) { candidateLists <- RearrangeEdges(edgeList[[1]], edgeList[[2]], dataset = dataset, TreeScorer = TreeScorer, @@ -107,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 "") + # `for` leaves the loop variable NULL when maxIter < 1 + if (maxIter > 0L) iter else 0L, + " rearrangements.", if (verbosity > 1L) "\n" else "") } #nocov end edgeList[3:4] <- c(bestScore, hits) 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 a5c1a5b0c..151140e4e 100644 --- a/dev/red-team/focus-areas.md +++ b/dev/red-team/focus-areas.md @@ -142,46 +142,41 @@ 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 — sonnet, UNMEASURED / no inherited maturity.** 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. - **`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. +- **15 Legacy pure-R search API — MEASURED 2026-08-05 at BOTH sonnet and opus; yielding + 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. + **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 + 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 6d8901573..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 @@ -60,6 +56,40 @@ 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 + +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 +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: | `. + +**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 +are frozen, not retired. + +--- + 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 +1406,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..fd2ed3511 100644 --- a/tests/testthat/test-CustomSearch.R +++ b/tests/testthat/test-CustomSearch.R @@ -108,3 +108,25 @@ 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 + + # 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") +})