Skip to content

Fix Ratchet() early-exit bookkeeping (A15-04) - #175

Merged
ms609 merged 1 commit into
cpp-searchfrom
claude/issue-136-assignment-0c7689
Aug 7, 2026
Merged

Fix Ratchet() early-exit bookkeeping (A15-04)#175
ms609 merged 1 commit into
cpp-searchfrom
claude/issue-136-assignment-0c7689

Conversation

@ms609

@ms609 ms609 commented Aug 7, 2026

Copy link
Copy Markdown

Summary

Ratchet(stopAtScore = )'s early-exit paths skipped the bookkeeping its return value depends on:

  • Already-met on entry: returned a bare phylo with no "score" attribute, and always a phylo even under returnAll = TRUE (documented to return multiPhylo). Broke MultiRatchet()'s vapply(trees, function(x) attr(x, "score"), double(1)).
  • Met mid-search: bestScore was updated but the edgeList <- candidate assignment (and, under returnAll, the forest append) lived below the if (BREAK) break — so the input tree could be returned carrying the improved score (silent wrong answer), and returnAll = TRUE could throw "No trees!? Is suboptimal set to a sensible (positive) value?" on the success path.

Both paths now perform the same bookkeeping the normal return does, before exiting.

Test plan

  • New regression tests in tests/testthat/test-Ratchet.R assert the property that failed: a returned tree's independently-recomputed TreeLength() must equal its own "score" attribute, on every exit path (already-met entry, mid-search hit, both returnAll values, and MultiRatchet()).
  • Confirmed empirically that all new assertions fail against the pre-fix code with the exact symptoms in the issue (stashed the fix, rebuilt, reran), and pass with the fix.
  • Full local run of test-Ratchet.R + test-CustomSearch.R + test-NativeSearch.R (the files exercising Ratchet()/MultiRatchet()): 44/44 pass, 0 failures.
  • Independent external-reviewer pass — verdict "ship after addressing nits"; addressed: seeded the mid-search test's RNG, tightened a length assertion, canonicalized the early-return edge matrix for parity with the normal return path, test-file style nits.
  • GHA agent-check.yml green on both ubuntu-arm64 and windows (run 31156683684).
  • spelling::spell_check_test() clean.

Known related edge case, out of scope here

Ratchet(returnAll = TRUE, ratchIter = 0) still throws the same misdirecting "No trees!?" error — but this is a different mechanism (zero iterations means forest never gets populated at all, regardless of the stopAtScore bookkeeping-skip bug this PR fixes), not covered by the issue's suggested fix either. Flagging for a possible follow-up issue rather than expanding this PR's scope.

Note on PR authorship

ms609-agent is still suspended (gh api user → 403 as of this PR), so this opened under the human maintainer's gh auth rather than the agent identity. cpp-search is not branch-protected and prior PRs (#73#134) merged the same way while the suspension has been in effect.

Fixes #136

Ratchet()'s stopAtScore= early-exit paths skipped the bookkeeping its
return value depends on:

- The already-met early return attached no "score" attribute and always
  returned a phylo, breaking MultiRatchet()'s vapply() and the documented
  returnAll = TRUE contract.
- The mid-search BREAK path set bestScore before the edgeList <- candidate
  assignment (and, under returnAll, the forest append) that live below the
  break -- so the input tree could be returned carrying the improved score,
  and returnAll = TRUE could throw "No trees!?" on the success path.

Both paths now perform the same bookkeeping the normal return does before
exiting. Regression test asserts the property that failed: a returned
tree's independently-recomputed TreeLength() must equal its own "score"
attribute, on every exit path.

Fixes #136

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ms609
ms609 merged commit 6b6b004 into cpp-search Aug 7, 2026
14 checks passed
@ms609
ms609 deleted the claude/issue-136-assignment-0c7689 branch August 7, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A15-04: Ratchet(stopAtScore=) returns the INPUT tree carrying the IMPROVED score — silent wrong answer, plus two more early-exit bookkeeping failures

1 participant