fix: preserve revealed card knowledge - #7094
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThe PR adds durable viewer-specific card knowledge across the engine, client, and AI determinization. It separates delayed-trigger provenance by condition type, migrates card fixtures to deterministic gzip files, and adds Earthbend and Gitaxian Probe regression coverage. ChangesDurable Card Knowledge
Delayed-Trigger Provenance
Compressed Card Fixtures
Earthbend Regression
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GameAction
participant GameState
participant VisibilityProjection
participant OpponentHand
GameAction->>GameState: disclose or inspect card identities
GameState->>GameState: record viewer-specific knowledge
GameState->>VisibilityProjection: project known card IDs
VisibilityProjection->>OpponentHand: provide viewer_known_card_ids
OpponentHand->>OpponentHand: render known card faces
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
36a04fc to
a4d7fc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
crates/engine/src/game/engine.rs (1)
5570-5578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd CR citations to the new knowledge-capture blocks.
Both new blocks record durable card knowledge but carry no
CR NNNcitation, unlike most of the surrounding code in this file. Add a short comment naming the CR section that grounds each block (for example, the general reveal/visibility rules for the public-reveal block, and CR 701.22a for the scry-disclosure block), so the rule basis is verifiable at a glance.As per path instructions, rules-touching code without a verified
CR <number>: <description>annotation is a finding forcrates/engine/**.Also applies to: 6776-6790
🤖 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/game/engine.rs` around lines 5570 - 5578, Add verified CR citations to both durable knowledge-capture blocks: annotate the public-reveal block around state.remember_card_identities with the applicable general reveal/visibility rule, and annotate the scry-disclosure block around lines 6776-6790 with CR 701.22a. Use concise comments in the required “CR <number>: <description>” format without changing behavior.Source: Path instructions
crates/engine/src/game/effects/scoped_library_search.rs (1)
555-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a CR citation for this knowledge-capture block.
This block records durable card identities for the search's learned audience. Other new knowledge-capture code in this PR (for example
remember_public_revealsinengine.rs) documents its rules basis in prose. Add a short comment here that states which CR section grounds the audience/visibility rule this block implements (for example CR 701.23a and CR 400.7), so a future reader can verify the code against the rule.As per path instructions, "rules-touching code with no verified
CR <number>: <description>annotation" is a finding forcrates/engine/**.🤖 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/game/effects/scoped_library_search.rs` around lines 555 - 562, Add a concise rules-basis comment immediately above the knowledge-capture block in the active search flow, identifying the applicable CR sections (such as CR 701.23a and CR 400.7) and describing the audience/visibility rule it implements. Keep the existing looked_at collection, remember_card_identities call, and active search insertion unchanged.Source: Path instructions
crates/engine/src/database/card_db.rs (1)
80-83: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuffer the reader inside
from_export_readerto avoid single-byte reads through the decompression stream.
serde_json::from_readerdecodes throughRead::bytes(). That iterator callsself.inner.read()once per byte, regardless of whetherRis buffered. The encoding used for all stdio inputs and outputs. Default: 'buffer'. is unrelated here, but the general point stands from Rust's ownstd::io::Read::bytes()contract: it does not batch reads on your behalf.Every gzip-backed fixture loader in this PR (
test_support.rs,the_fourteenth_doctor_graveyard_copy.rs, and four call sites insearch.rs) passes a bareGzDecoder<BufReader<File>>into this function. The outerGzDecoderdoes not implementBufRead, so every decompressed byte triggers a separate call into the decompression state machine.Wrap
readerin an internalBufReaderso every current and future caller gets buffered reads automatically, without changing the public signature or touching any call site.⚡ Proposed fix
pub fn from_export_reader<R: Read>(reader: R) -> Result<Self, Box<dyn std::error::Error>> { - let entries: HashMap<String, CardExportEntry> = serde_json::from_reader(reader)?; + let entries: HashMap<String, CardExportEntry> = serde_json::from_reader(BufReader::new(reader))?; Ok(Self::from_export_entries(entries)) }Since I can't execute Rust in this sandbox, please confirm that
serde_json::from_readerstill buffers this way in the pinnedserde_jsonversion and that double-wrapping an already-bufferedBufReader<File>(fromfrom_export) causes no behavior change.🤖 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/database/card_db.rs` around lines 80 - 83, Update from_export_reader to wrap the generic reader in an internal std::io::BufReader before passing it to serde_json::from_reader. Keep the public signature and existing call sites unchanged, preserving behavior for already-buffered readers and gzip-backed readers.crates/phase-ai/src/search.rs (1)
4709-4717: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting a shared fixture-loading helper for this test module.
This block, and the three identical blocks at lines 7242-7248, 7437-7443, and 7600-7606, all open the same
.json.gzpath and repeat the sameBufReader/GzDecoder/from_export_readersequence. Extract one private helper function in this module and call it from all four tests to remove the duplication.🤖 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/phase-ai/src/search.rs` around lines 4709 - 4717, The four tests duplicate loading the integration_cards.json.gz fixture. Add a private fixture-loading helper in the test module that performs the Path, File, BufReader, GzDecoder, and CardDatabase::from_export_reader sequence, then replace the duplicated setup in prospective_fetch_choice_survives_to_the_real_search_prompt and the three corresponding tests with calls to that helper.
🤖 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.
Inline comments:
In `@client/src/components/hand/OpponentHand.tsx`:
- Line 83: Remove the client-side viewerKnownCardIds visibility check from
OpponentHand and render the engine-projected display visibility property
instead. Add the projection in the engine-to-client adapter, preserve it through
the reverse adapter, and add round-trip coverage confirming the property
survives conversion.
In `@client/src/viewmodel/gameStateView.ts`:
- Line 159: Remove the viewer_known_card_ids-based visibility/reportability
checks from the gameStateView logic, including the branches near the referenced
condition and lines 235-239. Use the engine-projected per-object visibility
result as the sole authority for rendering, without deriving visibility from
card identities in the frontend.
In `@crates/engine/src/game/effects/delayed_trigger.rs`:
- Around line 397-412: Add a verified “CR <number>: <description>” annotation to
the documentation for condition_uses_creation_time_provenance, citing the rule
that phase-delayed conditions retain creation-time object context while
event-delayed conditions resolve provenance from the firing event. Keep the
classifier behavior unchanged.
- Around line 1372-1413: Add a regression test alongside
only_phase_delayed_conditions_use_creation_time_provenance that creates a
WhenNextEvent delayed trigger during event A, then fires it via event B through
the production trigger and ability-resolution pipeline. Assert the resolved
effect uses B’s TriggeringSource and does not retain A’s destination-zone
origin, exercising the prevented failure path rather than calling
condition_uses_creation_time_provenance directly.
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Line 1567: Update the direct library reorder paths in SurveilChoice’s
reorder_within_library flow and DigChoice when kept_destination is
Some(Zone::Library) to call state.advance_library_knowledge_epoch(player) after
cards are placed. Preserve the existing behavior for other destinations and
ensure each direct keep-on-top mutation invalidates prior library ordering
knowledge.
In `@crates/engine/src/game/engine.rs`:
- Around line 5570-5578: Update the public-reveal knowledge update at the event
boundary to pass unpublished to state.remember_card_identities instead of the
full card_ids collection. Preserve the existing audience construction and ensure
controller-only occurrences excluded by resolve_and_apply_information remain
hidden from other players’ durable product knowledge.
In `@crates/engine/src/types/game_state.rs`:
- Around line 6296-6304: Update GameState::advance_library_knowledge_epoch to
invalidate or canonicalize library product facts when advancing the epoch, so
stale knowledge cannot affect loop-state equality. At the GameState::eq
comparison site, compare only current product knowledge or apply the same
normalization while preserving existing loop-state equality semantics; both
referenced locations in crates/engine/src/types/game_state.rs require
coordinated handling.
In `@crates/engine/tests/integration/integration_bending.rs`:
- Around line 164-174: Update the activation selection around sacrifice_ability
to stop matching AbilityCost internals; use the available typed test helper or a
stable printed ability identity for Yawgmoth’s intended sacrifice ability, while
preserving the assertion that the expected ability is present.
In `@crates/phase-ai/src/determinize.rs`:
- Around line 148-154: Extend the determinization regression tests around
remember_card_identities and viewer_knows_card_identity to exercise the failure
path: record identities for the AI viewer, verify known opposing cards remain
pinned while unknown cards resample, and verify library knowledge expires after
a reorder. Ensure the test explicitly invokes the relevant knowledge APIs and
fails without the durable pinning fix.
In `@scripts/migrate-mana-target-roles.mjs`:
- Around line 92-112: Update writeCanonicalGzip to keep gzipCommand’s stdout as
a raw Buffer when writing the compressed fixture, removing the UTF-8 output
encoding from that invocation. Preserve the existing UTF-8 decoding exclusively
in readGzipUtf8, where decompressed text is consumed.
---
Nitpick comments:
In `@crates/engine/src/database/card_db.rs`:
- Around line 80-83: Update from_export_reader to wrap the generic reader in an
internal std::io::BufReader before passing it to serde_json::from_reader. Keep
the public signature and existing call sites unchanged, preserving behavior for
already-buffered readers and gzip-backed readers.
In `@crates/engine/src/game/effects/scoped_library_search.rs`:
- Around line 555-562: Add a concise rules-basis comment immediately above the
knowledge-capture block in the active search flow, identifying the applicable CR
sections (such as CR 701.23a and CR 400.7) and describing the
audience/visibility rule it implements. Keep the existing looked_at collection,
remember_card_identities call, and active search insertion unchanged.
In `@crates/engine/src/game/engine.rs`:
- Around line 5570-5578: Add verified CR citations to both durable
knowledge-capture blocks: annotate the public-reveal block around
state.remember_card_identities with the applicable general reveal/visibility
rule, and annotate the scry-disclosure block around lines 6776-6790 with CR
701.22a. Use concise comments in the required “CR <number>: <description>”
format without changing behavior.
In `@crates/phase-ai/src/search.rs`:
- Around line 4709-4717: The four tests duplicate loading the
integration_cards.json.gz fixture. Add a private fixture-loading helper in the
test module that performs the Path, File, BufReader, GzDecoder, and
CardDatabase::from_export_reader sequence, then replace the duplicated setup in
prospective_fetch_choice_survives_to_the_real_search_prompt and the three
corresponding tests with calls to that helper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3928fec5-1da0-41b4-b78c-b10987ff642d
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/integration_cards.json.gzis excluded by!**/*.gz
📒 Files selected for processing (35)
client/src/adapter/types.tsclient/src/components/hand/OpponentHand.tsxclient/src/viewmodel/gameStateView.tscrates/engine/src/database/card_db.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/dig.rscrates/engine/src/game/effects/manifest_dread.rscrates/engine/src/game/effects/reveal_hand.rscrates/engine/src/game/effects/scoped_library_search.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_exile_return_tests.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/library.rscrates/engine/src/game/visibility.rscrates/engine/src/game/zones.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/test_support.rscrates/engine/src/types/game_state.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/bards_company_recruit.rscrates/engine/tests/integration/heist_production_path_handoff.rscrates/engine/tests/integration/integration_bending.rscrates/engine/tests/integration/issue_3263_gitaxian_probe.rscrates/engine/tests/integration/issue_6691_enters_under_their_control.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rscrates/engine/tests/integration/support.rscrates/engine/tests/integration/the_fourteenth_doctor_graveyard_copy.rscrates/engine/tests/integration/token_zone_change_index.rscrates/phase-ai/src/determinize.rscrates/phase-ai/src/search.rsscripts/check-test-card-data-load.shscripts/gen-test-fixture.pyscripts/migrate-mana-target-roles.mjs
8ad9011 to
5bc43b5
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes