Skip to content

fix(3p): resolve North tile from hand in handle_kita to prevent hand corruption - #180

Merged
smly merged 3 commits into
mainfrom
fix/3p-handle-kita
Mar 14, 2026
Merged

fix(3p): resolve North tile from hand in handle_kita to prevent hand corruption#180
smly merged 3 commits into
mainfrom
fix/3p-handle-kita

Conversation

@smly

@smly smly commented Mar 14, 2026

Copy link
Copy Markdown
Owner

Fixes #179.

handle_kita fell back to tile ID 0 (1m) when the incoming Action had tile = None, silently failing to remove the North tile from the player's hand. This corrupted subsequent game state: the hand retained an extra tile, causing legal_actions() to omit valid actions (reach, ankan, hora) and include spurious Kita entries.

Root cause

// Before (sanma.rs)
let tile = act.tile
    .unwrap_or_else(|| act.consume_tiles.first().copied().unwrap_or(0));

When act.tile is None and consume_tiles is empty (both true for a generic Kita action), tile resolved to 0 (1m). The subsequent hand.remove() found no matching tile, so nothing was removed. The rinshan draw then added an extra tile, leaving the hand one tile too large for the rest of the round.

Fix

handle_kita now validates act.tile and, when it is missing or not a North tile, looks up the actual North tile from the player's hand:

@smly smly self-assigned this Mar 14, 2026
@smly smly added bug Something isn't working core labels Mar 14, 2026
@smly smly added this to the v0.5.0 milestone Mar 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a sanma (3P) game-state corruption bug where handle_kita could default to tile ID 0 when Action.tile is None, failing to remove the North tile from the hand and cascading into incorrect legal_actions() behavior (Issue #179).

Changes:

  • Update GameState3P::handle_kita to resolve the actual North tile from the player’s hand when the incoming Kita action omits tile.
  • Add regression/unit tests covering Kita with tile=None and ensuring follow-up legal_actions() include expected actions (e.g., riichi/ankan/tsumo) and exclude spurious Kita when no North remains.

Reviewed changes

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

File Description
riichienv-core/src/state_3p/sanma.rs Fixes Kita tile resolution to prevent silent hand corruption when Action.tile is missing.
riichienv-core/src/tests.rs Adds sanma regression tests reproducing the tile=None Kita scenario and validating downstream legality/action masking.
Comments suppressed due to low confidence (1)

riichienv-core/src/state_3p/sanma.rs:33

  • handle_kita still has a fallback path that can resolve to a non-North tile (including 0) if no North is found in the hand, and the function then proceeds to push that tile into kita_tiles even if it was never removed from the hand. That can reintroduce hand corruption for invalid inputs/call sites. Consider failing fast here (e.g., return/trigger an error) when no North tile exists in the hand, and/or assert that the resolved tile is North and was actually removed.
        let tile = match act.tile {
            Some(t) if t / 4 == 30 => t,
            _ => self.players[p_idx]
                .hand
                .iter()
                .find(|&&t| t / 4 == 30)
                .copied()
                .unwrap_or_else(|| {
                    act.tile
                        .unwrap_or_else(|| act.consume_tiles.first().copied().unwrap_or(0))
                }),
        };

        // Remove North tile from hand
        if let Some(idx) = self.players[p_idx].hand.iter().position(|&t| t == tile) {
            self.players[p_idx].hand.remove(idx);
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread riichienv-core/src/tests.rs Outdated
Comment thread riichienv-core/src/tests.rs Outdated
Comment thread riichienv-core/src/tests.rs Outdated
Comment thread riichienv-core/src/tests.rs Outdated
Comment thread riichienv-core/src/tests.rs Outdated
Comment thread riichienv-core/src/tests.rs Outdated
@smly
smly merged commit 94ff862 into main Mar 14, 2026
7 checks passed
@smly
smly deleted the fix/3p-handle-kita branch March 14, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][3P] obs.select_action_from_mjai() returns None for valid actions / obs.legal_actions() is incorrect

2 participants