Skip to content

cr733(p2): journal information-boundary commands#6524

Merged
matthewevans merged 1 commit into
mainfrom
ship/cr733-p2-information-boundary
Jul 23, 2026
Merged

cr733(p2): journal information-boundary commands#6524
matthewevans merged 1 commit into
mainfrom
ship/cr733-p2-information-boundary

Conversation

@matthewevans

@matthewevans matthewevans commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • New Features

    • Added journaled tracking for revealed-card information with audience and visibility lifetime, including deterministic replay support.
    • Public reveal bookkeeping is now command-driven to avoid duplicate/incorrect announcements.
  • Bug Fixes

    • Improved reveal handling for “reveal until” and choice flows, ensuring accurate reveal/hide state transitions.
    • Reveal information now clears correctly when cards exit zones or action boundaries.
  • Tests

    • Expanded coverage to verify that recorded reveal commands replay to the same final state and reject malformed data.

@matthewevans
matthewevans enabled auto-merge July 23, 2026 00:51
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 73870314-22ae-4c92-8365-e6f450ef2600

📥 Commits

Reviewing files that changed from the base of the PR and between b7988fd and ddb59c1.

📒 Files selected for processing (9)
  • crates/engine/src/game/effects/reveal_hand.rs
  • crates/engine/src/game/effects/reveal_until.rs
  • crates/engine/src/game/engine.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/engine_tests.rs
  • crates/engine/src/game/zones.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/types/resolved_commands.rs
  • crates/engine/tests/integration/cr733_resolved_commands_p2.rs
🚧 Files skipped from review as they are similar to previous changes (7)
  • crates/engine/src/game/zones.rs
  • crates/engine/src/game/engine_tests.rs
  • crates/engine/src/game/effects/reveal_hand.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/game/engine.rs
  • crates/engine/src/types/resolved_commands.rs
  • crates/engine/src/game/engine_resolution_choices.rs

📝 Walkthrough

Walkthrough

The PR adds journaled reveal-information commands with audience and lifetime semantics, routes reveal and cleanup paths through them, validates replay invariants, and clears zone-exit information by object incarnation.

Changes

Reveal Information Journaling

Layer / File(s) Summary
Information command model and validation
crates/engine/src/types/resolved_commands.rs
Adds information command types, journal recording, replay errors, serialization validation, and malformed-payload tests.
GameState information application
crates/engine/src/types/game_state.rs
Adds validated reveal/hide application, replay support, journaling, zone-exit cleanup, and replay tests.
Reveal effect transitions
crates/engine/src/game/effects/..., crates/engine/src/game/engine_resolution_choices.rs
Routes reveal and cleanup operations through controller/public information commands with action-boundary or zone-change lifetimes.
Public propagation and replay integration
crates/engine/src/game/engine.rs, crates/engine/src/game/zones.rs, crates/engine/src/game/engine_tests.rs, crates/engine/tests/integration/...
Filters already-journaled controller reveals, publishes remaining reveals, handles information commands during replay, and tests journal replay.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RevealEffect
  participant GameState
  participant ResolvedRulesJournal
  participant Engine
  RevealEffect->>GameState: Apply controller and public reveal information
  GameState->>ResolvedRulesJournal: Record Information commands
  Engine->>ResolvedRulesJournal: Inspect current action journal window
  Engine->>GameState: Apply unpublished public reveals
Loading

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: kiannidev, lgray

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: journaling information-boundary commands.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ship/cr733-p2-information-boundary

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

🧹 Nitpick comments (1)
crates/engine/src/types/resolved_commands.rs (1)

1464-1481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Centralize the audience/lifetime compatibility rule.

information_command_is_invalid re-encodes the exact (Controller, UntilActionBoundary) | (Public, UntilZoneChange) pairing that GameState::apply_information_edit (game_state.rs:15454-15463) already checks. Two independent copies of a CR-derived invariant can silently diverge. Extract a single predicate (e.g. an inherent ResolvedInformationAudience::permits(lifetime)) and call it from both sites.

As per coding guidelines: "Do not duplicate logic: centralize reveal/hide bookkeeping ... rather than re-implementing card-visibility rules."

♻️ Suggested shared predicate
impl ResolvedInformationAudience {
    pub fn permits(self, lifetime: ResolvedInformationLifetime) -> bool {
        matches!(
            (self, lifetime),
            (
                ResolvedInformationAudience::Controller(_),
                ResolvedInformationLifetime::UntilActionBoundary
            ) | (
                ResolvedInformationAudience::Public,
                ResolvedInformationLifetime::UntilZoneChange
            )
        )
    }
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/types/resolved_commands.rs` around lines 1464 - 1481,
Centralize the audience/lifetime compatibility rule by adding an inherent
predicate on ResolvedInformationAudience, such as permits, that accepts
ResolvedInformationLifetime and matches the two valid pairings. Replace the
duplicated matches! logic in information_command_is_invalid and
GameState::apply_information_edit with this shared predicate, preserving all
existing invalid-command and edit behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/engine/src/types/resolved_commands.rs`:
- Around line 1464-1481: Centralize the audience/lifetime compatibility rule by
adding an inherent predicate on ResolvedInformationAudience, such as permits,
that accepts ResolvedInformationLifetime and matches the two valid pairings.
Replace the duplicated matches! logic in information_command_is_invalid and
GameState::apply_information_edit with this shared predicate, preserving all
existing invalid-command and edit behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 96a879de-db77-4215-9142-7e42a6491c38

📥 Commits

Reviewing files that changed from the base of the PR and between 70e4f2d and b7988fd.

📒 Files selected for processing (8)
  • crates/engine/src/game/effects/reveal_hand.rs
  • crates/engine/src/game/effects/reveal_until.rs
  • crates/engine/src/game/engine.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/engine_tests.rs
  • crates/engine/src/game/zones.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/types/resolved_commands.rs

@matthewevans
matthewevans force-pushed the ship/cr733-p2-information-boundary branch from b7988fd to 9d65c15 Compare July 23, 2026 01:01
@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans
matthewevans force-pushed the ship/cr733-p2-information-boundary branch from 9d65c15 to 9d08741 Compare July 23, 2026 01:15
@matthewevans
matthewevans added this pull request to the merge queue Jul 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jul 23, 2026
@matthewevans
matthewevans force-pushed the ship/cr733-p2-information-boundary branch from 9d08741 to ddb59c1 Compare July 23, 2026 01:41
@matthewevans
matthewevans enabled auto-merge July 23, 2026 01:41
@matthewevans
matthewevans added this pull request to the merge queue Jul 23, 2026
Merged via the queue into main with commit d56400a Jul 23, 2026
15 checks passed
@matthewevans
matthewevans deleted the ship/cr733-p2-information-boundary branch July 23, 2026 02:13
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.

1 participant