Add belief_evaluation module: types, injection contracts, and renumbering

[zzcgumn]

Summary

Stands up library/src/belief_evaluation/ — the module, its public types, the
three declarer/defender/layout-source injection contracts, and the
renumbering scheme — with no evaluator behind any of it yet. This is the
foundation the recursive belief-space evaluator described in
docs/replenished_belief_evaluation/algorithm.md
will be built on.

What's included

• library/src/belief_evaluation/ — new Bazel component (cc_library +
  cc_test)
• Types: Card, StrategyId, StateKey, ObservationState,
  BeliefView/BeliefEntry, RankMap, NodeSearchInfo
• The three injection contracts, as headers: DeclarerStrategy,
  DefenderStrategy/DefenderQuery/WeightedCard, LayoutSource
• renumber() — exact gap-removal bijection over a suit's outstanding cards
• RankMap — absolute/relative rank conversion over dds's existing lookup
  tables, exhaustively round-trip tested
• A canonical Position type and an isomorphism test proving renumbering
  preserves legal moves, trick winners, and trick counts
• layout_key() — an exact, node-local 64-bit layout identity
• Validation helpers for declarer/defender callback returns (a user-supplied
  callback is an input, not an internal — rejected with a distinct error,
  never asserted)
• A Kahan compensated-summation accumulator
• specs/replenished-belief-evaluation.md

Testing

36 tests, all green: bazel test //library/src/belief_evaluation/...

• RankMap's round trip is exhaustive over every outstanding rank in every
  aggregate in [1, 8192), not sampled
• The renumbering isomorphism is exhaustively walked for two, three, and five
  cards per hand, the last with a trump suit and an uneven split that forces
  a void (and so a ruff)
• Direct correctness tests for trick-winner/legal-play logic, independent of
  the isomorphism property (see note below)
• Full coverage of the validation helpers and the Kahan accumulator

Strict TDD throughout — red confirmed before every implementation.

Worth a second look

The renumbering isomorphism test is, by construction, insensitive to which
order-based rule wins a trick: an order-preserving bijection is equally
consistent with "highest card wins" or "lowest card wins". Confirmed this by
deliberately breaking the comparison and watching the isomorphism tests stay
green; added independent tests against actual bridge rules to close the gap.
Worth keeping in mind for anything else proven primarily via an
invariance/isomorphism argument.

Not in this PR

No evaluator, no recursion, no sampling or replenishment, no early cuts, and
no dds_c_* shim entry — all deliberately out of scope here, listed as
non-goals in the new spec.

Constraints respected

• No existing public type modified — dll.h, dds_api.hpp and
  dds_c_api.h are untouched.
• specs/dds-public-api.md's claim that the shim "covers [the modern
  layer's] full surface" was checked against this change and still holds, so
  it's left as-is — no public entry point is added yet.

[Copilot]

🟡 Changes recommended

The new public utilities contain confirmed undefined-behavior risks (missing bounds checks / out-of-range indexing) and a validation implementation that currently does not enforce its documented legality contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces the initial library/src/belief_evaluation/ module as a foundation for a future replenished belief-space evaluator, including the public vocabulary types, injection contracts (declarer/defender/layout source), rank/renumbering utilities, a node-local layout identity, and supporting validation + numerical-accumulation helpers, along with comprehensive unit tests.

Changes:

• Added belief_evaluation Bazel component with public headers for strategies/types and internal test-only Position mechanics to state/test the renumbering isomorphism.
• Implemented core utilities (renumber, RankMap, layout_key, validation helpers, Kahan accumulator) and added focused/exhaustive tests for correctness and invariants.
• Added a capability spec document describing module-wide contracts/invariants and known non-goals.

File summaries

File	Description
specs/replenished-belief-evaluation.md	Capability-level spec for belief evaluation contracts/invariants and scope.
library/src/belief_evaluation/BUILD.bazel	New Bazel targets: public belief_evaluation, private position, and test suite.
library/src/belief_evaluation/types.hpp	Public vocabulary types: Card, RankMap, ObservationState, belief-view structs, etc.
library/src/belief_evaluation/declarer_strategy.hpp	Declarer injection contract (DeclarerStrategy) and purity/state-key rules.
library/src/belief_evaluation/defender_strategy.hpp	Defender injection contract (DefenderStrategy, DefenderQuery, WeightedCard).
library/src/belief_evaluation/layout_source.hpp	Layout enumeration injection contract (LayoutSource).
library/src/belief_evaluation/renumber.hpp	Public renumber() declaration and bit-convention contract.
library/src/belief_evaluation/renumber.cpp	renumber() implementation (gap-removal bijection over a pool).
library/src/belief_evaluation/renumber_test.cpp	Unit tests for renumbering behavior and boundary conditions.
library/src/belief_evaluation/rank_map.hpp	make_rank_map() declaration and RankMap mapping overview.
library/src/belief_evaluation/rank_map.cpp	RankMap methods using lookup tables + make_rank_map() implementation.
library/src/belief_evaluation/rank_map_test.cpp	Exhaustive round-trip tests for RankMap plus make_rank_map() test.
library/src/belief_evaluation/layout_key.hpp	Public layout_key() declaration and node-local uniqueness contract.
library/src/belief_evaluation/layout_key.cpp	layout_key() implementation packing defender holding into 64 bits.
library/src/belief_evaluation/layout_key_test.cpp	Tests for layout-key uniqueness/stability and suit bit ranges.
library/src/belief_evaluation/validation.hpp	Validation API + ValidationError enum for callback-return checking.
library/src/belief_evaluation/validation.cpp	Validation implementations for declarer card and defender distribution.
library/src/belief_evaluation/validation_test.cpp	Validation behavior tests for both declarer and defender checks.
library/src/belief_evaluation/kahan.hpp	KahanAccumulator declaration for compensated summation.
library/src/belief_evaluation/kahan.cpp	KahanAccumulator implementation.
library/src/belief_evaluation/kahan_test.cpp	Tests demonstrating improved precision vs naive summation.
library/src/belief_evaluation/position.hpp	Test-only minimal Position model + trick-play helpers.
library/src/belief_evaluation/position.cpp	Implementations of legal_plays, trick_winner, and play_card.
library/src/belief_evaluation/position_test.cpp	Isomorphism walk tests + direct bridge-rule correctness tests.
library/src/belief_evaluation/aggr_invariance_test.cpp	Test asserting RankMap pool (aggr) invariance across defender splits.
library/src/belief_evaluation/smoke_test.cpp	Minimal link/run canary test for the module.

Review details

• Files reviewed: 26/26 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Two public helpers (layout_key() and make_rank_map()) can exhibit undefined behavior or violate documented bit-width contracts when given malformed/uninitialized Deal data, and should be hardened with simple bounds/masking fixes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

library/src/belief_evaluation/rank_map.cpp:47

• make_rank_map() shifts remainCards down by 2 but does not mask to 13 bits. If a Deal contains any out-of-range rank bits (e.g., uninitialized memory or malformed input), aggr[suit] can exceed 8191 and later indexing into lookup tables (e.g., rel_rank[aggr]) becomes out-of-bounds/UB. Masking keeps the function bounds-safe while preserving the intended 13-rank aggregate encoding.

            pool |= deal.remainCards[hand][suit];
        }
        // remainCards sets bit r for absolute rank r; aggr and the lookup
        // tables use the compacted convention, bit r-2 for absolute rank r.
        map.aggr[suit] = pool >> 2;

• Files reviewed: 26/26 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are confirmed correctness/UB issues in the new module (unmasked RankMap aggregates and non-finite defender probabilities slipping through validation) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

library/src/belief_evaluation/validation.cpp:90

• validate_defender_distribution() does not handle NaN/+Inf probabilities: entry.probability <= 0.0 is false for NaN, and std::abs(total - 1.0) > tol is also false when total becomes NaN, so the function can incorrectly return ValidationError::None. Treat non-finite probabilities as invalid.

        if (entry.probability <= 0.0)
        {
            return ValidationError::ProbabilityNonPositive;
        }
        total += entry.probability;

library/src/belief_evaluation/validation.cpp:75

• validate_defender_distribution() only reports an out-of-range seat via the per-entry is_held() guard; for an empty distribution an invalid seat currently falls through to ProbabilitiesDoNotSumToOne, which is misleading and inconsistent with how validate_declarer_card() handles invalid seats. Add an explicit seat bounds check up front.

auto validate_defender_distribution(
    Deal const& layout,
    int seat,
    std::vector<WeightedCard> const& distribution) -> ValidationError
{

• Files reviewed: 26/26 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The current diffs include a correctness hole in defender distribution validation (NaN/Inf acceptance) and an isomorphism test setup/termination issue that can allow tests to pass without actually playing out the full deal.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

library/src/belief_evaluation/validation.cpp:96

• validate_defender_distribution() will accept NaN/Inf probabilities: entry.probability <= 0.0 is false for NaN, and the final std::abs(total - 1.0) > tol check also fails when total becomes NaN, so the function can incorrectly return None for an invalid distribution. Guard for finite probabilities (and a finite total) before accepting the distribution.

        if (entry.probability <= 0.0)
        {
            return ValidationError::ProbabilityNonPositive;
        }
        total += entry.probability;

library/src/belief_evaluation/position_test.cpp:118

• The isomorphism walk treats the deal as complete when the current hand has no cards at a trick boundary. If a test accidentally constructs hands with unequal card counts, the walk can terminate early (or silently dead-end) without checking remaining cards in other hands, making the isomorphism assertions vacuously pass. Consider checking that all hands are empty before declaring completion.

This issue also appears on line 328 of the same file.

            if (count_in_trick == 0 && total_holding(orig, hand) == 0)
            {
                // Deal complete: every hand's trick count must correspond.
                for (int h = 0; h < DDS_HANDS; ++h)
                {

library/src/belief_evaluation/position_test.cpp:332

• This test is named FiveCardsPerHand... but the South hand is currently initialized with 7 cards (4 spades + 1 heart + 2 diamonds). That can cause the isomorphism walk to stop after the other hands run out, leaving cards unplayed and reducing the coverage of the property being asserted. Adjust the holdings so each hand has 5 cards (and update the descriptive comment accordingly).

    position.trump = 0;  // spades
    position.leader = 0;

    position.holding[0][0] = 0b0000011;        // N spades: bits 0,1
    position.holding[1][0] = 0b0001100;        // E spades: bits 2,3

• Files reviewed: 26/26 changed files
• Comments generated: 0 new
• Review effort level: Lite