fix(engine): offer TurnFaceUp/PlayFaceDown actions and reveal own face-down identity (#4381)#4538
fix(engine): offer TurnFaceUp/PlayFaceDown actions and reveal own face-down identity (#4381)#4538carlos4s wants to merge 9 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the identity_revealed flag to the game engine and frontend to properly render face-down cards (such as morphs or manifested cards) to authorized viewers (like their controller) while keeping them hidden from others. It also adds AI candidate generation for playing cards face down and turning them face up. Feedback highlights that the AI candidate generation overly restricts face-down casts to the hand and sorcery-speed only, violating CR 702.37a. Additionally, the identity_revealed property in TypeScript should be typed as non-optional to match the backend serialization and avoid redundant defensive checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| * rendering on `!face_down || identity_revealed` — never on `!face_down` | ||
| * alone, which would hide even the controller's own face-down permanents. | ||
| */ | ||
| identity_revealed?: boolean; |
There was a problem hiding this comment.
[MEDIUM] Property identity_revealed is typed as optional but is always serialized by the backend. The Rust GameObject struct defines identity_revealed as a non-optional bool with #[serde(default)] and no skip_serializing_if attribute, meaning it is guaranteed to be present on every serialized object. Typing it as optional in TypeScript forces unnecessary defensive checks or optional chaining.
| identity_revealed?: boolean; | |
| identity_revealed: boolean; |
References
- Do not add redundant defensive guards (such as optional chaining or nullish coalescing) to properties that are typed as non-optional and guaranteed to be initialized by the backend.
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for taking this on. The visibility-side direction is reasonable, but I can't approve the action candidate changes as written.
The new PlayFaceDown and TurnFaceUp candidates expose actions that the runtime still performs without paying the required costs. The PlayFaceDown candidate path explicitly has no affordability/payment gate, and morph::turn_face_up currently restores the card without routing morph/disguise/manifest turn-up costs through payment. Before these actions are surfaced through legal_actions, the candidate must either route through the same cost/payment authority that will actually charge the action, or stay unsurfaced until that payment path exists.
This is not just an AI issue: candidate generation is the engine's advertised legal-action surface. Exposing these as zero-cost actions would make ordinary morph/disguise/manifest play rules-wrong even though the UI symptom is fixed.
|
@matthewevans pls review again |
matthewevans
added
defer-fe
# Conflicts: # client/src/components/board/PermanentCard.tsx
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Art-crop battlefield mode still hides face-down permanents even when the engine reveals their identity to this viewer. Evidence: client/src/components/board/PermanentCard.tsx:1083 passes the revealed object into ArtCropCard, but client/src/components/card/ArtCropCard.tsx:38,43,66,68,101,111 still gate every lookup/render path on raw obj.face_down instead of obj.face_down && !obj.identity_revealed. Why it matters: a player using art-crop battlefield display still cannot see their own morph/manifest identity, so the core issue remains unfixed in that supported render mode. Suggested fix: thread the same renderAsFaceDown/identityVisible logic into ArtCropCard (or pass a faceDown override) and add a focused art-crop test for a face-down object with identity_revealed=true.
|
This PR's current head still has unresolved requested changes. Please push the requested fixes or leave a substantive update within 7 days (by 2026-07-22); otherwise the sweep will close it as stale. A new commit or meaningful follow-up will return it to review. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# Conflicts: # client/src/components/card/ArtCropCard.tsx # client/src/components/hand/__tests__/OpponentHand.test.tsx # client/src/game/dispatch.ts # client/src/hooks/useKeyboardShortcuts.ts # client/src/viewmodel/__tests__/battlefieldGrouping.test.ts
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (2)
📝 WalkthroughWalkthroughThe PR adds authoritative engine snapshots and viewer-scoped face-down identity metadata, supports AI-generated ChangesEngine visibility and action generation
Snapshot contracts and adapters
Authoritative store and persistence
Revealed face-down rendering
Recovery, debug, and display state selection
TurnFaceUp choice labels
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant WasmAdapter
participant GameStore
participant CardUI
Engine->>WasmAdapter: return viewer state, authoritative state, and legalResult
WasmAdapter->>GameStore: commit both state projections
GameStore->>CardUI: provide viewer state and identity_revealed
CardUI->>CardUI: render revealed or face-down card identity
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
client/src/viewmodel/costLabel.ts (1)
453-457: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLabel always shows "(X = N)", even for morph costs with no
{X}component.
candidates.rsalways emitsx: 0for a plain-cost morph (e.g.{3}with no X shard), so a normal morph's turn-face-up option will read "Turn face up (X = 0)" — confusing for a card that was never announced with X. Consider suppressing the X suffix when the object's morph cost has no X shard (checkable viaobject.keywords+manaCostToShards, already used elsewhere in this file for Sneak/Ninjutsu).🤖 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 `@client/src/viewmodel/costLabel.ts` around lines 453 - 457, Update the TurnFaceUp label handling in the action-label logic to append the X value only when the object's morph cost contains an X shard. Reuse the existing object.keywords and manaCostToShards pattern used for Sneak/Ninjutsu, while preserving the current fallback label and X display for morph costs that actually include X.client/src/adapter/wasm-adapter.ts (1)
935-959: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSame redundant viewer-state computation as the worker path.
Mirrors the worker's
getSnapshothandler:wasm.get_viewer_snapshot_js(0)andwasm.get_filtered_game_state(0)both independently compute the viewer-filtered projection. See consolidated comment.🤖 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 `@client/src/adapter/wasm-adapter.ts` around lines 935 - 959, Update the client getSnapshot implementation to avoid independently recomputing the viewer-filtered projection through both wasm.get_viewer_snapshot_js(0) and wasm.get_filtered_game_state(0). Reuse the single viewer-filtered state produced by the snapshot API while preserving the existing GameState wrapper, authoritativeState, and legalResult fields.crates/engine/src/ai_support/candidates.rs (1)
3809-3856: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding the X-announcement enumeration for
TurnFaceUp.
for x in 0..=max_xemits one candidate per affordable X value with no upper cap, unlike other combinatorial enumerators in this same file (e.g.PayableResource::LoopCollapseis deliberately capped to the default N=1 to avoid search-tree explosion, per thepay_amount_loop_collapse_offers_only_default_onetest). A face-down morph with an{X}cost in a high-mana game (ramp/ritual decks) could add a correspondingly large number ofTurnFaceUpcandidates to every priority decision, widening the AI's branching factor on the hot path.🤖 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/ai_support/candidates.rs` around lines 3809 - 3856, Bound the X-announcement loop in the TurnFaceUp candidate generation around turn_face_up_prepare and max_x to the established default X value, matching the capped enumeration pattern used by PayableResource::LoopCollapse. Preserve affordability checks and candidate creation for the retained values, while preventing high-mana states from generating an unbounded number of TurnFaceUp actions.client/src/adapter/engine-worker.ts (1)
330-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
getSnapshotcomputes the viewer-filtered projection twice per call.
get_viewer_snapshot_js(0)already produces a viewer-filteredstateinternally (seeViewerSnapshot.state: GameState), but this handler discards it and makes a second call toget_filtered_game_state(0)purely to get theClientGameState-wrapped/derived-data shape. That means the engine's visibility-filtering pass over the wholeGameStateruns twice on everygetSnapshotcall — andgetSnapshotis invoked after every player action (dispatch.ts::processAction). Worth confirming whetherget_viewer_snapshot_jscould return the samederived-wrapped shape so the extra filtering pass isn't needed. See consolidated comment (mirrors the identical pattern inwasm-adapter.ts'screateMainThreadFallback().getSnapshot()).🤖 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 `@client/src/adapter/engine-worker.ts` around lines 330 - 357, Update the getSnapshot handler to avoid performing viewer filtering twice: reuse the viewerSnapshot.state produced by get_viewer_snapshot_js(0) while preserving the existing ClientGameState derived-data wrapper and result shape. Adjust the snapshot getter contract or returned structure as needed so state, authoritativeState, and legalResult remain unchanged for callers, and apply the equivalent fix in createMainThreadFallback().getSnapshot().
🤖 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/board/PermanentCard.tsx`:
- Around line 455-458: Centralize viewer-scoped face-down rendering and identity
decisions in the engine, removing frontend derivations of face_down and
identity_revealed. Update client/src/components/board/PermanentCard.tsx at lines
455-458 and 1159-1161, client/src/components/board/AttachmentFan.tsx at line
315, client/src/components/card/ArtCropCard.tsx at lines 48-61,
client/src/components/card/GameCardPreview.tsx at lines 45-62, and
client/src/viewmodel/cardProps.ts at line 48 to consume the engine-provided
render mode, preview identity metadata, and public name; ensure all affected
surfaces render only those values.
In `@client/src/game/dispatch.ts`:
- Around line 447-450: Update the checkpoint construction in the TurnStarted
handling flow to append the pre-action authoritativeGameState ?? gameState value
captured at the earlier action boundary, rather than rereading
prev.authoritativeGameState after asynchronous engine calls. Preserve the
existing turnCheckpoints history limit and ensure the captured value is used
consistently to avoid async races.
- Around line 981-985: Update the loop containing the snapshot fetch to declare
an outer variable that stores the latest snapshot, assign each fetched snapshot
to it inside the loop, and use that variable’s authoritativeState in the final
saveAuthoritativeGame call after the loop. Preserve the existing newState
fallback.
In `@client/src/hooks/useKeyboardShortcuts.ts`:
- Around line 190-200: The keyboard shortcut export/copy flow and engine-loss
export can expose hidden authoritative state in user diagnostics. In
client/src/hooks/useKeyboardShortcuts.ts lines 190-200, use gameState whenever
the explicit AI-only mode is not active before calling exportGameStateDebugZip
or copyGameStateDebugSnapshot; in
client/src/components/modal/EngineLostModal.tsx lines 65-72, apply the same
AI-only gate or sanitize the snapshot before export. Preserve
authoritativeGameState only for explicitly permitted AI-only diagnostics.
---
Nitpick comments:
In `@client/src/adapter/engine-worker.ts`:
- Around line 330-357: Update the getSnapshot handler to avoid performing viewer
filtering twice: reuse the viewerSnapshot.state produced by
get_viewer_snapshot_js(0) while preserving the existing ClientGameState
derived-data wrapper and result shape. Adjust the snapshot getter contract or
returned structure as needed so state, authoritativeState, and legalResult
remain unchanged for callers, and apply the equivalent fix in
createMainThreadFallback().getSnapshot().
In `@client/src/adapter/wasm-adapter.ts`:
- Around line 935-959: Update the client getSnapshot implementation to avoid
independently recomputing the viewer-filtered projection through both
wasm.get_viewer_snapshot_js(0) and wasm.get_filtered_game_state(0). Reuse the
single viewer-filtered state produced by the snapshot API while preserving the
existing GameState wrapper, authoritativeState, and legalResult fields.
In `@client/src/viewmodel/costLabel.ts`:
- Around line 453-457: Update the TurnFaceUp label handling in the action-label
logic to append the X value only when the object's morph cost contains an X
shard. Reuse the existing object.keywords and manaCostToShards pattern used for
Sneak/Ninjutsu, while preserving the current fallback label and X display for
morph costs that actually include X.
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 3809-3856: Bound the X-announcement loop in the TurnFaceUp
candidate generation around turn_face_up_prepare and max_x to the established
default X value, matching the capped enumeration pattern used by
PayableResource::LoopCollapse. Preserve affordability checks and candidate
creation for the retained values, while preventing high-mana states from
generating an unbounded number of TurnFaceUp actions.
🪄 Autofix (Beta)
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: 535dce98-5605-48d1-8175-1ff7af514a67
📒 Files selected for processing (37)
client/src/adapter/engine-worker-client.tsclient/src/adapter/engine-worker.tsclient/src/adapter/types.tsclient/src/adapter/wasm-adapter.tsclient/src/components/board/AttachmentFan.tsxclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/card/ArtCropCard.tsxclient/src/components/card/GameCardPreview.tsxclient/src/components/card/__tests__/ArtCropCard.test.tsxclient/src/components/card/__tests__/GameCardPreview.test.tsxclient/src/components/chrome/DebugPanel.tsxclient/src/components/hand/OpponentHand.tsxclient/src/components/hand/__tests__/OpponentHand.test.tsxclient/src/components/help/HelpSheet.tsxclient/src/components/modal/EngineLostModal.tsxclient/src/game/__tests__/engineRecovery.test.tsclient/src/game/dispatch.tsclient/src/game/engineRecovery.tsclient/src/hooks/useKeyboardShortcuts.tsclient/src/i18n/locales/en/game.jsonclient/src/pages/GamePage.tsxclient/src/providers/GameProvider.tsxclient/src/stores/__tests__/gameStore.test.tsclient/src/stores/gameStore.tsclient/src/stores/multiplayerDraftStore.tsclient/src/stores/replayStore.tsclient/src/test/factories/gameObjectFactory.tsclient/src/viewmodel/__tests__/battlefieldGrouping.test.tsclient/src/viewmodel/__tests__/cardProps.test.tsclient/src/viewmodel/__tests__/costLabel.test.tsclient/src/viewmodel/cardProps.tsclient/src/viewmodel/costLabel.tsclient/src/viewmodel/gameStateView.tscrates/engine/src/ai_support/candidates.rscrates/engine/src/game/game_object.rscrates/engine/src/game/visibility.rs
| // CR 708.5: render the card back only for face-down objects whose identity | ||
| // has NOT been revealed to this viewer by the engine's visibility layer. The | ||
| // controller (and look-permitted viewers) see the real card image instead. | ||
| const renderAsFaceDown = obj.face_down && !obj.identity_revealed; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Centralize viewer-scoped face-down rendering state in the engine.
The frontend now reimplements face_down && !identity_revealed across battlefield cards, exile ghosts, art crops, previews, and public names. Replace these client-side calculations with one viewer-scoped engine-provided render/name value so all surfaces preserve the same hidden-information policy.
client/src/components/board/PermanentCard.tsx#L455-L458: consume the engine-provided mode for battlefield cards.client/src/components/board/AttachmentFan.tsx#L315-L315: consume the same mode for fan cards.client/src/components/board/PermanentCard.tsx#L1159-L1161: consume the same mode for exile ghosts.client/src/components/card/ArtCropCard.tsx#L48-L61: consume engine-provided name/render metadata.client/src/components/card/GameCardPreview.tsx#L45-L62: consume engine-provided preview identity metadata.client/src/viewmodel/cardProps.ts#L48-L48: consume the engine-provided public name.
As per path instructions, client/src/** must only render engine-provided state and must not calculate or derive game data; the coding guidelines impose the same frontend boundary.
📍 Affects 5 files
client/src/components/board/PermanentCard.tsx#L455-L458(this comment)client/src/components/board/AttachmentFan.tsx#L315-L315client/src/components/board/PermanentCard.tsx#L1159-L1161client/src/components/card/ArtCropCard.tsx#L48-L61client/src/components/card/GameCardPreview.tsx#L45-L62client/src/viewmodel/cardProps.ts#L48-L48
🤖 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 `@client/src/components/board/PermanentCard.tsx` around lines 455 - 458,
Centralize viewer-scoped face-down rendering and identity decisions in the
engine, removing frontend derivations of face_down and identity_revealed. Update
client/src/components/board/PermanentCard.tsx at lines 455-458 and 1159-1161,
client/src/components/board/AttachmentFan.tsx at line 315,
client/src/components/card/ArtCropCard.tsx at lines 48-61,
client/src/components/card/GameCardPreview.tsx at lines 45-62, and
client/src/viewmodel/cardProps.ts at line 48 to consume the engine-provided
render mode, preview identity metadata, and public name; ensure all affected
surfaces render only those values.
Sources: Coding guidelines, Path instructions
| await saveAuthoritativeGame( | ||
| gameId, | ||
| adapter, | ||
| snapshot.authoritativeState ?? newState, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
nl -ba client/src/game/dispatch.ts | sed -n '912,986p'Repository: phase-rs/phase
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path('client/src/game/dispatch.ts')
lines = p.read_text().splitlines()
for start, end in [(930, 990), (1, 120)]:
print(f'লines {start}-{end}')
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
print()
PYRepository: phase-rs/phase
Length of output: 8443
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path('client/src/game/dispatch.ts')
lines = p.read_text().splitlines()
for start, end in [(930, 990), (1, 120)]:
print(f"lines {start}-{end}")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
print()
PYRepository: phase-rs/phase
Length of output: 8443
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('client/src/game/dispatch.ts')
lines = p.read_text().splitlines()
for start, end in [(930, 990)]:
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: phase-rs/phase
Length of output: 3244
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path('client/src/game/dispatch.ts')
text = p.read_text().splitlines()
for i,line in enumerate(text,1):
if 'snapshot' in line:
print(f'{i:4d}: {line}')
PYRepository: phase-rs/phase
Length of output: 2381
Keep the final snapshot outside the loop. snapshot is block-scoped inside the for body, so the save call here has nothing in scope to read. Store the last fetched snapshot in an outer variable and use its authoritativeState after the loop.
🤖 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 `@client/src/game/dispatch.ts` around lines 981 - 985, Update the loop
containing the snapshot fetch to declare an outer variable that stores the
latest snapshot, assign each fetched snapshot to it inside the loop, and use
that variable’s authoritativeState in the final saveAuthoritativeGame call after
the loop. Preserve the existing newState fallback.
Source: Path instructions
| const debugState = authoritativeGameState ?? gameState; | ||
| if (debugState) { | ||
| exportGameStateDebugZip(debugState) | ||
| .then((filename) => console.log(`[Debug] Game state exported to ${filename}`)) | ||
| .catch((err) => console.error("[Debug] Failed to export:", err)); | ||
| } | ||
| } else if (!e.ctrlKey && !e.metaKey) { | ||
| e.preventDefault(); | ||
| if (gameState) { | ||
| copyGameStateDebugSnapshot(gameState) | ||
| const debugState = authoritativeGameState ?? gameState; | ||
| if (debugState) { | ||
| copyGameStateDebugSnapshot(debugState) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent authoritative hidden state from entering user-exported diagnostics.
authoritativeGameState can contain opponent-hand and face-down identities. Both changed sites select it without the AI-only guard, while the export helpers serialize the provided state verbatim.
client/src/hooks/useKeyboardShortcuts.ts#L190-L200: usegameStateoutside the explicit AI-only mode before export or clipboard copy.client/src/components/modal/EngineLostModal.tsx#L65-L72: apply the same gate or sanitize the snapshot before engine-loss export.
📍 Affects 2 files
client/src/hooks/useKeyboardShortcuts.ts#L190-L200(this comment)client/src/components/modal/EngineLostModal.tsx#L65-L72
🤖 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 `@client/src/hooks/useKeyboardShortcuts.ts` around lines 190 - 200, The
keyboard shortcut export/copy flow and engine-loss export can expose hidden
authoritative state in user diagnostics. In
client/src/hooks/useKeyboardShortcuts.ts lines 190-200, use gameState whenever
the explicit AI-only mode is not active before calling exportGameStateDebugZip
or copyGameStateDebugSnapshot; in
client/src/components/modal/EngineLostModal.tsx lines 65-72, apply the same
AI-only gate or sanitize the snapshot before export. Preserve
authoritativeGameState only for explicitly permitted AI-only diagnostics.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The newly surfaced TurnFaceUp legal action cannot complete a paused replacement-aware mana payment. Evidence: candidates.rs:3823-3847 treats the affordability probe as sufficient, while casting.rs:13351-13356 deliberately reports a paused mana-source cost as payable; engine.rs:7517-7524 then calls the compatibility pay_special_action_mana_cost wrapper, which converts SpecialActionManaPayment::Paused into an error at casting.rs:14622-14631. There is no TurnFaceUp ManaAbilityResume route to finish the special action. Why it matters: a player can be offered this action, then a mana source whose cost pauses for a replacement choice leaves the turn-up payment unable to resume. Suggested fix: give TurnFaceUp the same typed, cost-snapshotted resume path as other special actions and add an end-to-end paused-mana-source test, or keep it off legal_actions until that path exists.
|
Closing as stale: the requested-changes warning from July 15 expired on July 22, and the current head |
3 participants
Summary
Closes #4381
Players could neither look at face-down permanents they controlled nor turn morph/megamorph/manifest/manifest-dread permanents face up. The engine already implemented both actions (
morph::turn_face_up,morph::play_face_down) and revealed face-down identity to the controller (CR 708.5), butpriority_actionsnever offered the actions and the frontend gated face-down display on!face_down, hiding even the controller's own cards. This emits the missing candidates and adds an engine-ownedidentity_revealedflag so the frontend renders the real card to the controller without inferring it.Implementation method (required)
/engine-implementerpipeline (plan → review-plan → implement → review-impl → commit)/engine-implementer— explain why belowIf you did not use
/engine-implementer, state why:CR references
CR 702.37a(Morph) /CR 702.121a(Disguise) — face-down cast from hand, sorcery-speed window for the candidateCR 702.37c/CR 116.2b— turning a face-down permanent face up is a special action available any time its controller has priority (not suppressed by split second, CR 702.61a)CR 708.5— controller may look at their own face-down permanents; drives theidentity_revealedflagCR 708.7/CR 116.2b—CantBeTurnedFaceUpstatic (Karlov Watchdog) gates the candidate viais_blocked_by_cant_be_turned_face_upCR 701.40a— manifested creatures are turn-up eligible (no morph cost)Verification
cargo fmt --all --check— cleancargo clippy -p engine --lib --tests— no warningscargo test -p engine --lib— new tests pass; morph (15), visibility (36), candidates (43) suites green, including CR 708.5 look-permission statics (lumbering_laundry,found_footage) and split-second suppressionpnpm run type-check(protocol:check+tsc -b --noEmit) — cleaneslinton touched files — cleanviteston touched components + boundary guardrail — 35 passedNew tests:
priority_actions_offer_turn_face_up_for_controlled_face_down_morphpriority_actions_skip_turn_face_up_for_opponent_face_down_morphpriority_actions_offer_play_face_down_for_morph_card_in_handpriority_actions_skip_play_face_down_off_sorcery_windowbattlefield_face_down_identity_revealed_flag_split_by_viewerSummary by CodeRabbit
New Features
Bug Fixes
Tests