Reject extra compass letters on PBN deals

[tameware]

Summary

• convert_from_pbn now rejects PBN deal strings that put N/E/S/W on any hand after the first; later hands must follow clockwise with no seat prefixes.
• Extra prefixes used to be ignored, so labeled deals could silently assign cards to the wrong seats.
• Docs and examples use a valid 52-card first-hand-only deal.

Test plan

• bazelisk test //library/tests:pbn_test
• bazelisk test //python:type_conversions_test
• bazelisk test //library/tests:dtest_nothing_makes_test

Made with Cursor

[Copilot]

Pull request overview

This PR tightens DDS PBN deal-string parsing by making convert_from_pbn reject compass letters (N/E/S/W) on any hand after the first, preventing silently mis-seated deals. It also updates Python bindings/docs and adds targeted tests to lock in the new validation behavior.

Changes:

• Enforce “first hand only” seat prefix rule in convert_from_pbn and document the constraint across C/C++/Python surfaces.
• Add C++ and Python tests that ensure extra seat letters on later hands are rejected.
• Update examples/docs to use valid first-hand-only PBN deal strings.

Reviewed changes

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

Show a summary per file

File	Description
python/tests/test_type_conversions.py	Adds Python tests asserting extra seat letters are rejected.
python/tests/README.md	Documents the new “extra compass letters rejected” behavior in test coverage notes.
python/src/bindings.cpp	Updates Python binding docstrings to describe first-hand-only compass letter rule.
library/tests/README.md	Documents PBN input format for dtest as first-hand-only seat prefix.
library/tests/pbn_test.cpp	Adds C++ unit tests for accepting valid PBN and rejecting later-hand seat letters.
library/tests/BUILD.bazel	Registers new pbn_test target in Bazel.
library/src/pbn.hpp	Documents the stricter PBN parsing rule.
library/src/pbn.cpp	Implements rejection of compass letters on later hands during parsing.
library/src/api/PBN.h	Updates API docs to specify first-hand-only compass letter rule.
library/src/api/dll.h	Clarifies DealPBN remainCards string format in public header docs.
docs/python_interface.md	Updates Python interface documentation and examples to match the stricter PBN rule.
doc/dll-description.md	Updates DLL documentation example/description to reflect first-hand-only compass letter rule.

Suppressed comments (1)

library/src/pbn.cpp:30

• convert_from_pbn can read past the end of the input buffer when the PBN string is empty or shorter than 3 characters. The initial scan increments bp up to 3 without stopping at \0, so inputs like "" (covered by python/tests/test_type_conversions.py::test_pbn_empty_string) can trigger out-of-bounds reads / UB. Consider also guarding dealBuff == nullptr since this is a public API that otherwise would crash on null input.

  int bp = 0;
  while ((bp < 3) && !is_compass_letter(dealBuff[bp]))
    bp++;

  if (bp >= 3)
    return 0;

---

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/src/pbn.cpp:23

• convert_from_pbn now returns early on dealBuff == nullptr before clearing remainCards. Since some callers may ignore the return code, this can leave stale output data. Consider validating remainCards for null and always zeroing the output buffer before returning on error (including null dealBuff).

{
  if (dealBuff == nullptr)
    return 0;

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/src/pbn.cpp:115

• convert_from_pbn() stops parsing at bp==80 and then returns RETURN_NO_FAULT even if the input wasn’t NUL-terminated within the 80-byte DealPBN.remainCards buffer (or if the caller passed a longer string). That can silently accept truncated/garbage-tailed deals and can bypass the new “reject extra compass letters” check if a later seat letter appears after the 80-byte cutoff. Consider failing when the loop exits due to hitting the 80-byte limit.

    else if (is_compass_letter(dealBuff[bp]))
      return 0;
    bp++;
  }
  return RETURN_NO_FAULT;

[tameware]

Addressed the 80-byte buffer comment in 82f03eb: convert_from_pbn now returns failure when parsing reaches index 80 without a NUL terminator, so overlong/truncated deals cannot succeed or bypass later-hand seat-letter checks. Covered by RejectsInputLongerThanRemainCardsBuffer in pbn_test.cpp.

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/tests/pbn_test.cpp:132

• The remain-cards buffer length checks are only tested for “< 80” accepted and “> 80” rejected. Since convert_from_pbn() now explicitly rejects bp >= 80, it would be good to add an edge-case test that an input of exactly 80 characters is rejected, to pin down the boundary behavior and prevent regressions.

TEST(ConvertFromPbn, RejectsInputLongerThanRemainCardsBuffer)
{
  const std::string pbn =
      std::string(kNorthFirst) + " E:873.J97.AT764.Q4";
  ASSERT_GT(pbn.size(), 80u);
  EXPECT_EQ(convert(pbn.c_str()), 0);
}

TEST(ConvertFromPbn, AcceptsInputThatFitsRemainCardsBuffer)
{
  ASSERT_LT(std::char_traits<char>::length(kNorthFirst), 80u);
  EXPECT_EQ(convert(kNorthFirst), RETURN_NO_FAULT);
}

[tameware]

Addressed the 80-character boundary test comment: added RejectsInputExactlyAtRemainCardsBufferLimit to pin rejection when the input is exactly 80 bytes without an early NUL.

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/tests/pbn_test.cpp:125

• RejectsInputLongerThanRemainCardsBuffer currently appends an extra hand separator/seat (" E:") after kNorthFirst, so the parse can fail early due to the extra hand/space rather than because the input exceeds the 80-byte remainCards buffer. This makes the test less effective at guarding the length limit behavior.

Consider appending only non-special filler characters (not space/dot/card/compass letters) to keep the deal otherwise parseable until the length check trips.

  const std::string pbn =
      std::string(kNorthFirst) + " E:873.J97.AT764.Q4";
  ASSERT_GT(pbn.size(), 80u);
  EXPECT_EQ(convert(pbn.c_str()), 0);

[tameware]

Addressed the overlong-test comment: RejectsInputLongerThanRemainCardsBuffer now pads with card characters (A) so the only reason it fails is the 80-byte length limit, not the compass letter or space.

[Copilot]

Pull request overview

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

Suppressed comments (3)

library/tests/pbn_test.cpp:133

• Avoid hard-coding 80 here as well; compute the capacity from DealPBN::remainCards so this boundary test tracks the public ABI definition.

  std::string pbn = "N:";
  pbn.append(78, 'A');
  ASSERT_EQ(pbn.size(), 80u);
  EXPECT_EQ(convert(pbn.c_str()), 0);

library/tests/pbn_test.cpp:138

• Same here: use the ABI-defined buffer size instead of hard-coding 80 so this test remains correct if DealPBN::remainCards changes.

  ASSERT_LT(std::char_traits<char>::length(kNorthFirst), 80u);

library/tests/pbn_test.cpp:125

• These tests hard-code the DealPBN remainCards buffer size (80). Derive it from the API struct instead so the tests stay correct if the buffer size changes in dll.h.

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

• line 130
• line 138

  std::string pbn = "N:";
  pbn.append(80, 'A');
  ASSERT_GT(pbn.size(), 80u);
  EXPECT_EQ(convert(pbn.c_str()), 0);

[tameware]

Addressed the hard-coded buffer size comments: both convert_from_pbn and the length-boundary tests now derive the limit from sizeof(DealPBN::remainCards).

[Copilot]

Pull request overview

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

Suppressed comments (2)

library/src/pbn.cpp:122

• convert_from_pbn() rejects extra spaces (too many hands) but still accepts inputs with fewer than 4 hands (e.g., only the first hand, or only two hands). That can treat truncated/partial deal strings as valid and silently produce a remainCards array with missing hands, which can lead to incorrect solver behavior.

  if (bp >= PbnBufferSize)
    return 0;

  return RETURN_NO_FAULT;

library/tests/pbn_test.cpp:118

• Tests cover too many hands via multiple spaces, but there is no coverage that truncated deal strings (fewer than 4 hands) are rejected. Since the parser is being tightened, adding a failing test for 1-hand and 2-hand inputs helps prevent regressions and aligns with the expected 4-hand format.

TEST(ConvertFromPbn, RejectsTooManyHands)
{
  unsigned int remain[DDS_HANDS][DDS_SUITS]{};
  EXPECT_EQ(convert_from_pbn("N:AK.K.K.K    A", remain), 0);
}

[tameware]

Addressed: convert_from_pbn now rejects truncated deals with fewer than 4 hands. Added RejectsTruncatedDealWithFewerThanFourHands test.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[zzcgumn]

Good catch.

[krtschil]

In python/test/README.md I have a question regarding the wording:

• Extra compass letters on later hands rejected

Does this mean that the validation continues or does the validation stop as failed?

Otherwise fine.