refactor: extract apply_move_to_track() and add unit tests

[wopdevries]

Factored out duplicated code to apply_move_to_track() and added unit tests.

• Extracted shared track-update logic from MakeSpecific, MakeNext, and MakeNextSimple into a single helper (128 lines → one function)
• Added 4 unit tests covering lead hand, follow suit, trump beats non-trump, and trick completion
• Zero performance regression (benchmark: solve equal, calc +0.6% noise)
• 41/41 library tests pass

[wopdevries]

All CI checks pass. Ready for review and merge when you have time.

[Copilot]

Pull request overview

Refactors move-tracking state updates by extracting the shared logic from Moves::MakeSpecific, Moves::MakeNext, and Moves::MakeNextSimple into a single helper (apply_move_to_track()), and adds unit tests to validate key scenarios.

Changes:

• Added Moves::apply_move_to_track() and rewired MakeSpecific / MakeNext / MakeNextSimple to use it.
• Added 2 unit tests for apply_move_to_track() covering lead-hand and follow-suit behavior.
• Reduced duplicated code in moves.cpp by centralizing per-card track updates.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
library/src/moves/moves.cpp	Extracts and uses apply_move_to_track() for track updates across move-selection paths.
library/src/moves/moves.hpp	Declares the new helper in the Moves interface.
library/tests/moves/moves_test.cpp	Adds unit tests for the new track-update helper.

[tameware]

I requested a Copilot review. I expect its second and third comments apply equally to the original code that you refactored. If so, you can address them in this PR to keep it as a pure refactor or add an issue to address them in the future - your choice.

The two phrases in Copilot's third comment seem equivalent, so I asked Cursor for an opinion:

Yes — Copilot is right.

The lead is Ace (rank = 14), the follow is King (rank = 13). Ace beats King, so the lead stays winning. The next lines already say that:

// King < Ace so high stays at 0 (lead hand wins)
EXPECT_EQ(moves->trackp->high[1], 0);

So // Follow with King of Spades — higher rank wins is wrong; // Follow with King of Spades — lower rank loses matches the test.

I won't have any other look at the PR myself until Copilot's comments are resolved.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

library/src/moves/moves.cpp:448

• apply_move_to_track() takes a trick index but relies on the caller to have already set trackp. That makes the API easy to misuse (trackp could point at a different trick than the provided trick), and in non-debug builds the assert won’t protect against null/incorrect trackp. Consider binding trackp from the trick argument inside this helper (and validating the trick index) so the precondition is enforced in one place.

auto Moves::apply_move_to_track(const MoveType &move, const int relHand,
                                const int trick) -> void {
  assert(trackp != nullptr && "apply_move_to_track: trackp must be set");
  assert(relHand >= 0 && relHand < DDS_HANDS);
  if (relHand == 3)

library/src/moves/moves.hpp:347

• The docstring for apply_move_to_track() says trick is in (1..12), but other code/tests index tricks with 0..12 (arrays are sized [13]). This comment should match the actual allowed range and call out the additional constraint only when relHand==3 (since the helper writes track[trick - 1]).

     * @brief Update TrackType state to reflect a played move.
     *
     * @param move Move to apply
     * @param relHand Relative hand index within the current trick (0..3)
     * @param trick Trick index (1..12); when relHand==3 updates next trick state
     */

library/tests/moves/moves_test.cpp:611

• The added unit tests cover leading and following suit, but they don’t exercise the two other important branches in apply_move_to_track(): (1) trump-played beats non-trump, and (2) relHand==3 completion updates track[trick - 1] (lead_hand and removed_ranks). Adding coverage for those branches would better lock in the refactor’s behavior and guard against regressions in MakeNext/MakeNextSimple.

TEST_F(MovesTest, ApplyMoveToTrackFollowSuit) {
  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

[tameware]

First two new Copilot comments could be either addressed or postponed. Third seems relevant to this PR.

[tameware]

I've asked for a new Copilot review. For now, I note that the PR description needs an update.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

library/src/moves/moves.hpp:347

• The doc for apply_move_to_track() says trick is 1..12, but the implementation/other APIs use 0..12 and only require trick > 0 when relHand == 3 (because it updates track[trick - 1]). This comment is currently misleading about valid values and the direction of the update.

     * @brief Update TrackType state to reflect a played move.
     *
     * @param move Move to apply
     * @param relHand Relative hand index within the current trick (0..3)
     * @param trick Trick index (1..12); when relHand==3 updates next trick state

library/tests/moves/moves_test.cpp:668

• ApplyMoveToTrackTrickCompletion currently checks removed_ranks[0] != 0, but removed_ranks is typically non-zero immediately after Init(), so this assertion can pass even if apply_move_to_track() never updates removed ranks. Resetting removed_ranks to a known value before the trick and asserting other suits remain unchanged would make this test actually validate the behavior.

  // removed_ranks should include all played cards
  EXPECT_NE(moves->track[4].removed_ranks[0], 0);

library/src/moves/moves.cpp:447

• apply_move_to_track() takes a trick parameter but still requires callers to pre-set trackp. Since all current callers set trackp = &track[trick], setting trackp inside the helper would remove a fragile precondition (and make the trick param consistently used for all relHand values).

auto Moves::apply_move_to_track(const MoveType &move, const int relHand,
                                const int trick) -> void {
  assert(trackp != nullptr && "apply_move_to_track: trackp must be set");
  assert(relHand >= 0 && relHand < DDS_HANDS);
  if (relHand == 3)
    assert(trick > 0 && "apply_move_to_track: trick must be > 0 when relHand==3");

library/tests/moves/moves_test.cpp:589

• The PR description says "Added 2 unit tests for apply_move_to_track()", but this diff adds four new tests (LeadHand, FollowSuit, TrumpBeatsNonTrump, TrickCompletion). Please update the PR description to match what was actually added so the summary stays accurate.

TEST_F(MovesTest, ApplyMoveToTrackLeadHand) {
  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);

[tameware]

Regarding Copilot's comment on the PR description, I think it should actually be something like "Factored out duplicated code to apply_move_to_track() and added unit tests." The number of tests is unimportant.

[tameware]

I see now! Copilot was referring to the detailed description, not the PR name itself.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

library/tests/moves/moves_test.cpp:620

• In ApplyMoveToTrackFollowSuit (and the similar trump test), multiple assignments are packed onto one line. Elsewhere in this test file initializations use one statement per line; splitting these improves readability and keeps formatting consistent.

  MoveType lead;
  lead.suit = 0;  lead.rank = 14;  lead.sequence = 0;
  moves->apply_move_to_track(lead, 0, 5);

  // Follow with King of Spades — lower rank loses

library/tests/moves/moves_test.cpp:605

• PR description says “Added 2 unit tests for apply_move_to_track()”, but this file adds 4 new TEST_F cases for apply_move_to_track. Please update the PR description (or consolidate the tests) so it accurately reflects what was changed.

TEST_F(MovesTest, ApplyMoveToTrackLeadHand) {
  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

  MoveType move;
  move.suit = 2;  // Diamonds
  move.rank = 10;
  move.sequence = 0;

  moves->apply_move_to_track(move, 0, 5);

  EXPECT_EQ(moves->trackp->move[0].suit, 2);
  EXPECT_EQ(moves->trackp->move[0].rank, 10);
  EXPECT_EQ(moves->trackp->high[0], 0);
  EXPECT_EQ(moves->trackp->lead_suit, 2);
  EXPECT_EQ(moves->trackp->play_suits[0], 2);
  EXPECT_EQ(moves->trackp->play_ranks[0], 10);
}

library/tests/moves/moves_test.cpp:674

• ApplyMoveToTrackTrickCompletion currently doesn’t strongly validate removed_ranks updates: getSampleRankInSuit() makes track[5].removed_ranks values non-deterministic for this scenario, and the test re-calls apply_move_to_track(relHand==3) after manually zeroing track[4].removed_ranks, which can pass even if the per-card OR logic is wrong. Making removed_ranks deterministic for the source track and asserting the expected bits are set after the first trick completion will make this test meaningful and stable.

  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];
  moves->track[5].lead_hand = 0;

  MoveType cards[4];

library/src/moves/moves.cpp:447

• apply_move_to_track() takes a trick index but still requires the caller to pre-set trackp. That makes the helper easy to misuse (null or pointing at the wrong track slot), and the debug-only assert won’t protect release builds. Consider making the helper self-contained by setting trackp from trick (and asserting trick is in-range) inside the function.

auto Moves::apply_move_to_track(const MoveType &move, const int relHand,
                                const int trick) -> void {
  assert(trackp != nullptr && "apply_move_to_track: trackp must be set");
  assert(relHand >= 0 && relHand < DDS_HANDS);
  if (relHand == 3)

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

library/src/moves/moves.cpp:447

• apply_move_to_track() relies on trackp being set by the caller, but Init() does not initialize trackp and the method signature already receives trick. If trackp is unset or points to a different track entry than track[trick], this will assert in debug and can dereference a null/incorrect pointer in release builds (asserts compiled out). Make the helper self-contained by selecting track[trick] internally (and optionally asserting trick bounds).

  assert(trackp != nullptr && "apply_move_to_track: trackp must be set");
  assert(relHand >= 0 && relHand < DDS_HANDS);
  if (relHand == 3)
    assert(trick > 0 && "apply_move_to_track: trick must be > 0 when relHand==3");

library/tests/moves/moves_test.cpp:680

• This test intends to verify that removed_ranks[0] contains the exact bits for A/K/Q/J, but the current assertion only checks for non-zero. That can pass even if the wrong bits are set. Compare against the expected bitmask so the test actually validates correctness.

  // removed_ranks[0] (spades) should have bits set for A K Q J
  EXPECT_NE(moves->track[4].removed_ranks[0], 0);

[tameware]

Looking good so far. Copilot's comments can seem never-ending. I've found they are worth addressing.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (5)

library/tests/moves/moves_test.cpp:608

• Use DDS_SUITS instead of a hard-coded [4] in the rankInSuit pointer type so the test stays consistent with the codebase constants and won’t break if suit count ever changes.

  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();

library/tests/moves/moves_test.cpp:633

• Use DDS_SUITS instead of a hard-coded [4] in the rankInSuit pointer type so the test stays consistent with the codebase constants and won’t break if suit count ever changes.

  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();

library/tests/moves/moves_test.cpp:658

• Use DDS_SUITS instead of a hard-coded [4] in the rankInSuit pointer type so the test stays consistent with the codebase constants and won’t break if suit count ever changes.

  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();

library/tests/moves/moves_test.cpp:588

• Use DDS_SUITS instead of a hard-coded [4] in the rankInSuit pointer type so the test stays consistent with the codebase constants and won’t break if suit count ever changes.

This issue also appears in the following locations of the same file:

• line 608
• line 633
• line 658

  const unsigned short (*rankInSuit)[4] = getSampleRankInSuit();

library/tests/moves/moves_test.cpp:669

• Avoid hard-coded hand count constants in tests. Using DDS_HANDS here keeps the test aligned with the production constants and makes intent clearer.

  MoveType cards[4];
  for (int h = 0; h < 4; h++) {

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (6)

library/tests/moves/moves_test.cpp:93

• This assertion/comment is incorrect: DDS_NOTRUMP is 4 (see constants.h), so trump == 3 corresponds to Clubs. Use the DDS_NOTRUMP constant to avoid silently testing the wrong strain.

  EXPECT_EQ(moves->trump, 3);  // 3 = notrump
  
  // Verify move lists are reset
  for (int h = 0; h < DDS_HANDS; h++) {

library/tests/moves/moves_test.cpp:153

• Init(..., 3, ...) sets trump to Clubs, not notrump. Use DDS_NOTRUMP to ensure this test is exercising the intended no-trump path.

  const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);

library/tests/moves/moves_test.cpp:590

• This test initializes trump with 3, which is Clubs. If the intent is notrump, use DDS_NOTRUMP (4) so the test doesn't accidentally pass under a suit contract.

  const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

library/tests/moves/moves_test.cpp:610

• This test initializes trump with 3, which is Clubs. If the intent is notrump, use DDS_NOTRUMP (4) to match the codebase’s strain encoding.

  const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

library/tests/moves/moves_test.cpp:660

• This test initializes trump with 3, which is Clubs. If the intent is notrump, use DDS_NOTRUMP (4) so removed-ranks behavior is validated under the correct strain.

  const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
  moves->trackp = &moves->track[5];

library/tests/moves/moves_test.cpp:86

• Init(..., 3, ...) sets trump to Clubs (3), not notrump. In this codebase, notrump is DDS_NOTRUMP (4), so this test is initializing the wrong contract strain.

This issue also appears in the following locations of the same file:

• line 90
• line 152
• line 588
• line 608
• line 658

  const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
  moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);

[tameware]

I'd be wary of the latest batch of Copilot comments. I've seen similar comments turn out to be incorrect when I ran them by Cursor in a context more familiar with our codebase. I'm not saying Copilot is wrong, just that it's worth verifying.

[zzcgumn]

Extracting what looks like copy-paste code into a re-usable method makes sense to me. I agree that copilot's last set of comments are dubious.

[zzcgumn]

Review from Claude Code:

Review note: apply_move_to_track extraction silently changes MakeNextSimple

What changed

The extraction of apply_move_to_track (moves.cpp:442) gives MakeNextSimple a trick-completion side effect it did not have before. Previously, on
the 4th card (relHand == 3) MakeNextSimple updated only track[trick-1].lead_hand. It now also runs the full removed_ranks propagation
(moves.cpp:480–485), copying the completing trick's removed_ranks into the next trick and OR-ing in the four played ranks — identical to
MakeNext.

Why it matters (this path is live)

removed_ranks is seeded once by Init for the current trick (moves.cpp:161–172) and thereafter propagated forward on each trick completion rather
than recomputed. MakeNext (the alpha-beta path) always did this; MakeNextSimple did not.

All make_next_simple call sites pass hand_rel_first = (48 - ini_depth) % 4 (solver_if.cpp:151). When a position is solved with three cards
already on the current trick, hand_rel_first == 3, so MakeNextSimple completes the trick. In the forbidden-move enumeration loop
(solver_if.cpp:576–599) this call is interleaved with a fresh alpha-beta search that then reads track[trick-1].removed_ranks. Under the old code
that value was left stale; under the new code it is correct.

Assessment

This looks like a latent-bug fix, not a regression: the old MakeNextSimple was inconsistent with MakeNext, and the only observable difference
(stale removed_ranks feeding the next trick) is a correctness improvement. Risk of the writes harming anything is negligible — in the
output-enumeration loops (lines 322, 399, 425, 502) the writes are dead, and in the interleaved loop they replace a stale value with the right
one.

The concern is purely that it's an unlabeled semantic change shipped inside a refactor, with no test over the relHand == 3 / hand_rel_first == 3
path.

Proposed regression test

Drives the change through the public MakeNextSimple entry point (not apply_move_to_track directly), so it locks in the behavior a caller actually
sees. It fails against the pre-refactor MakeNextSimple (which would leave track[4].removed_ranks at the stale 0xFFFF) and passes after.

  // Regression test for the apply_move_to_track extraction.
  // MakeNextSimple must propagate removed_ranks when it completes a trick
  // (relHand == 3), matching MakeNext. Before the extraction it updated only
  // lead_hand, leaving track[trick-1].removed_ranks stale. That path is reached
  // from the top-level solver when a deal is solved with three cards already
  // played to the current trick (solver_if.cpp: hand_rel_first == 3).
  TEST_F(MovesTest, MakeNextSimplePropagatesRemovedRanksOnTrickCompletion) {
    const unsigned short (*rankInSuit)[DDS_SUITS] = getSampleRankInSuit();
    moves->Init(5, 0, nullptr, nullptr, rankInSuit, 3, 0);
    moves->track[5].lead_hand = 0;

    // Zero the source so we can assert exactly which bits get set, and poison
    // the destination so a missing propagation is visible (old code leaves it).
    for (int s = 0; s < DDS_SUITS; s++) {
      moves->track[5].removed_ranks[s] = 0;
      moves->track[4].removed_ranks[s] = 0xFFFF;
    }

    // Simulate three cards already played to trick 5: hands 0..2 play A, K, Q of spades.
    for (int h = 0; h < 3; h++) {
      MoveType played;
      played.suit = 0;
      played.rank = 14 - h;
      played.sequence = 0;
      moves->apply_move_to_track(played, h, 5);
    }

    // The 4th card is delivered through MakeNextSimple's move list — the entry
    // point under test. Jack of spades.
    MovePlyType &list = moves->moveList[5][3];
    list.current = 0;
    list.last = 0;
    list.move[0].suit = 0;
    list.move[0].rank = 11;
    list.move[0].sequence = 0;

    MoveType const *mp = moves->MakeNextSimple(5, 3);
    ASSERT_NE(mp, nullptr);
    EXPECT_EQ(mp->rank, 11);

    // Trick complete: next trick's removed_ranks must now reflect A,K,Q,J of spades.
    // Old MakeNextSimple never wrote here, so track[4].removed_ranks[0] would stay 0xFFFF.
    EXPECT_EQ(moves->track[4].removed_ranks[0], 0x1E00);  // A|K|Q|J
    EXPECT_EQ(moves->track[4].removed_ranks[1], 0);
    EXPECT_EQ(moves->track[4].removed_ranks[2], 0);
    EXPECT_EQ(moves->track[4].removed_ranks[3], 0);

    // Winner (hand 0, Ace) leads the next trick.
    EXPECT_EQ(moves->track[4].lead_hand, 0);
  }

Notes for the reviewer

• Fields/access mirror the existing ApplyMoveToTrackTrickCompletion test (same fixture, same direct track/moveList access), so no new test
  scaffolding is needed.
• 0x1E00 matches this codebase's bit_map_rank convention (rank r → 1 << (r-2)), consistent with the existing trick-completion test.
• Setup for hands 0–2 uses apply_move_to_track purely to arrange state; the assertions are all on the MakeNextSimple call. If you'd prefer a
  "purer" version, all four hands can be pushed through MakeNextSimple by populating moveList[5][0..3] — behaviorally identical, just more
  boilerplate.
• Optional stronger coverage: a solver-level test that calls the public solve entry point on a deal with three cards already on the trick and
  solutions == 2, asserting the returned scores. That exercises the actual interleaved make_next_simple + alpha-beta path, but it's harder to make
  deterministic; the unit test above is the minimal guard.

[zzcgumn]

Solver test suggested by Claude

Note that it downgrades the problem from a possible trick counting bug to an inefficiency. This feels consistent with the empirical stability we have observed for the solver.

• Martin

Follow-up: solver-level test for the hand_rel_first == 3 path — with a caveat worth reading first

Before proposing an end-to-end test I traced where the propagated removed_ranks is actually read. All consumers (moves.cpp:232–243, 309–316,
363–370, and GetTopNumber at 409) use it for one thing: sequence-equivalence merging — collapsing card groups when the ranks between them have
all been played — and the related winner-counting. It does not restrict the legal move set; legal moves come from rank_in_suit (the real
holding).

The consequence: alpha-beta's min-max result is invariant to removed_ranks. A stale value makes the generator under-merge equivalent cards
(explore redundant-but-equivalent moves, and possibly emit different equals[] masks / representative cards under solutions ≥ 2), but it cannot
change the returned trick counts. So a solver-level test that asserts on score[] almost certainly cannot distinguish old MakeNextSimple from the
extracted version — both return the correct score.

That reframes finding #1: the omission in the old code was most likely an efficiency / equals-fidelity discrepancy on the 3-cards-on-trick path,
not a wrong-trick-count bug. The deterministic unit test in the previous comment is still the right guard (it pins the state write directly). The
solver-level test below is therefore an end-to-end invariant / smoke test for that code path, not a red/green oracle — and I've noted how to
empirically confirm whether it can go red.

Proposed test

  /// @file trick_four_solutions.cpp
  /// @brief End-to-end coverage for solving a position with three cards already
  ///        on the current trick (solver_if.cpp: hand_rel_first == 3), which
  ///        routes the 4th card through Moves::MakeNextSimple's trick-completion
  ///        branch. NOTE: asserts invariants only — see the caveat in the PR
  ///        discussion; this is not expected to fail against the pre-refactor
  ///        MakeNextSimple, because removed_ranks feeds sequence grouping, not
  ///        the min-max result.

  #include <algorithm>
  #include <gtest/gtest.h>
  #include <api/dds.h>

  namespace {

  // A card is (suit, rank); DDS encodes a holding as a bitmask with bit (1<<rank).
  bool east_holds(const Deal& dl, int suit, int rank) {
    return (dl.remainCards[1][suit] & (1 << rank)) != 0;  // hand 1 == East
  }

  int max_score(const FutureTricks& fut) {
    int m = 0;
    for (int i = 0; i < fut.cards; ++i) m = std::max(m, fut.score[i]);
    return m;
  }

  // Three cards already played to the current trick (S:Q, W:K, N:A of spades),
  // East to play the 4th card. East is void in spades and holds one card in each
  // of hearts/diamonds/clubs, so it has three non-equivalent legal moves — enough
  // to exercise the solutions>=2 enumeration loops at hand_rel_first == 3.
  //
  // Each hand held 3 cards at the start of this trick; all card ranks are unique
  // within their suit. Please sanity-run once to confirm legality (I can't execute
  // the solver here).
  Deal make_three_on_trick_deal() {
    return Deal{
      .trump = 4,                            // No Trump
      .first = 2,                            // South led the current trick
      .currentTrickSuit = {0, 0, 0},         // all spades
      .currentTrickRank = {12, 13, 14},      // South Q, West K, North A
      .remainCards = {
        {256,   0,   0,   4},   // North: S8, C2
        {0,   512, 512, 512},   // East : H9, D9, C9  (void in spades)
        {1024,  4,   0,   0},   // South: S10, H2
        {128,   0,   4,   0},   // West : S7,  D2
      }
    };
  }

  }  // namespace

  class TrickFourSolutions : public ::testing::Test {};

  TEST_F(TrickFourSolutions, SolvesWithThreeCardsOnTrick) {
    const Deal dl = make_three_on_trick_deal();
    const int thr = 0;

    FutureTricks best{};   // solutions = 1: single optimal card
    ASSERT_EQ(RETURN_NO_FAULT,
              SolveBoard(dl, /*target=*/-1, /*solutions=*/1, /*mode=*/0, &best, thr));
    ASSERT_GE(best.cards, 1);

    FutureTricks optimal{}; // solutions = 2: all optimal-scoring cards
    ASSERT_EQ(RETURN_NO_FAULT,
              SolveBoard(dl, /*target=*/-1, /*solutions=*/2, /*mode=*/0, &optimal, thr));

    FutureTricks all{};     // solutions = 3: every card, scored
    ASSERT_EQ(RETURN_NO_FAULT,
              SolveBoard(dl, /*target=*/0, /*solutions=*/3, /*mode=*/0, &all, thr));

    // Invariant 1: every returned card is a card East actually holds (no move was
    // dropped or fabricated on the hand_rel_first == 3 path).
    for (const FutureTricks* f : {&best, &optimal, &all}) {
      for (int i = 0; i < f->cards; ++i) {
        EXPECT_TRUE(east_holds(dl, f->suit[i], f->rank[i]))
            << "returned card S" << f->suit[i] << " R" << f->rank[i]
            << " not in East's holding";
      }
    }

    // Invariant 2: the three solution modes must agree on the achievable maximum.
    EXPECT_EQ(max_score(best), max_score(optimal));
    EXPECT_EQ(max_score(best), max_score(all));

    // Invariant 3: the best card (solutions=1) appears in the solutions=3 list
    // with that same maximum score.
    bool found = false;
    for (int i = 0; i < all.cards; ++i) {
      if (all.suit[i] == best.suit[0] && all.rank[i] == best.rank[0]) {
        EXPECT_EQ(all.score[i], best.score[0]);
        found = true;
      }
    }
    EXPECT_TRUE(found);
  }

For the reviewer

• Harness: mirrors library/tests/solve_board/trick_three_bug.cpp (same SolveBoard API, FutureTricks, RETURN_NO_FAULT); add the file to that
  directory's BUILD.bazel target.
• Two things I could not verify without running the solver, please confirm locally before merging:
  a. Deal legality — I built the layout by hand (M=3 cards per hand at trick start, unique ranks per suit, East void in the led suit). A quick
  run returning RETURN_NO_FAULT confirms it.
  b. Sensitivity — per the caveat above, I expect this to pass on both old and new MakeNextSimple. If you want a genuine red/green guard,
  temporarily revert MakeNextSimple's trick-completion branch to the old lead_hand-only form and re-run: if the test still passes (it likely will),
  that empirically confirms the change is not observable at the trick-count level, and the unit test from the previous comment is the appropriate
  regression guard instead.
• If we do want to pin the observable difference, the place to assert is equals[] (the sequence-equivalence masks) under solutions >= 2, not
  score[] — but that needs the concrete expected masks from a run, so I'd rather generate those from the fixed build than hardcode a guess.