ship/sandbox deck sync - #7123
Conversation
|
Warning Review limit reached
Next review available in: 25 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 (2)
📝 WalkthroughWalkthroughThe PR adds bounded batch creation for debug cards and tokens, persisted debug-card resolution frames, engine-authored Scry outcome reporting, cloud deck merging, and protocol version updates. ChangesCore debug creation and persistence
Scry outcome reporting
Cloud conflict merging
Protocol compatibility
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/server-core/src/protocol.rs (1)
2259-2283: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale test-name reference after the rename.
protocol_version_is_24was renamed toprotocol_version_is_25at line 2260, but the doc comment onfull_game_floor_is_current_only_not_a_rollout_window(line 2271) still refers toprotocol_version_is_24. Update the reference so the cross-reference stays accurate for future readers.📝 Proposed fix
/// REVERT-PROBE: relax to `PROTOCOL_VERSION - 1` — the exact regression - /// this guards — and this test reds while `protocol_version_is_24` stays + /// this guards — and this test reds while `protocol_version_is_25` stays /// green, which is why the two are separate assertions.🤖 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/server-core/src/protocol.rs` around lines 2259 - 2283, Update the REVERT-PROBE documentation in full_game_floor_is_current_only_not_a_rollout_window to reference protocol_version_is_25 instead of the renamed protocol_version_is_24 test, leaving the test logic unchanged.crates/engine/src/game/engine_resolution_choices.rs (1)
1573-1579: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a serialization round-trip test for
PlayerPerformedAction { Scry }.The new
scry_top_countfield is present in the engine event and client adapter type, but the transport-layer serialization coverage does not lock inscry_top_count,scry_bottom_count, andlook_countfor all-top, all-bottom, mixed, and zero-count scry outcomes.🤖 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_resolution_choices.rs` around lines 1573 - 1579, Add a transport serialization round-trip test for the Scry variant of PlayerPerformedAction, covering all-top, all-bottom, mixed, and zero-count outcomes. Assert that look_count, scry_bottom_count, and scry_top_count survive serialization and deserialization with their expected values.Source: Path instructions
🧹 Nitpick comments (3)
client/src/components/chrome/DebugCreateActions.tsx (1)
235-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the debug create forms at the engine limit.
MAX_DEBUG_CREATE_COUNTis100, and the engine rejects larger counts as an error. The four debug create forms currently pass onlymin={0}toNumberInput, so values above100can still be submitted. Add a shared source for this limit, export it to the client, and pass it asmaxon each copy count input; avoid re-deriving the constant in the UI.🤖 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/chrome/DebugCreateActions.tsx` around lines 235 - 237, Use the existing MAX_DEBUG_CREATE_COUNT definition as the shared source, export it for client use, and update all four debug create forms in DebugCreateActions to pass max={MAX_DEBUG_CREATE_COUNT} on their copy-count NumberInput components while retaining min={0}; do not duplicate or re-derive the limit in the UI.crates/server-core/tests/game_action_payload_guard.rs (1)
299-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd ceiling-rejection tests for
CreateCard.countandCreateToken.count.
rejects_debug_create_count_above_engine_ceilingonly exercisesDebugAction::CreateTokenCopy. The sameMAX_DEBUG_CREATE_COUNTcheck was added toCreateCardandCreateTokeningame_action_payload_guard.rs, but neither has an over-ceiling test here.🤖 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/server-core/tests/game_action_payload_guard.rs` around lines 299 - 310, Add tests alongside rejects_debug_create_count_above_engine_ceiling covering DebugAction::CreateCard and DebugAction::CreateToken with count set to MAX_DEBUG_CREATE_COUNT + 1. Assert guard_game_action_payload returns an error containing the respective count field path, matching the existing CreateTokenCopy test.crates/server-core/src/game_action_payload_guard.rs (1)
365-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
MAX_DEBUG_CREATE_COUNTcheck into one helper.
CreateCard,CreateToken, andCreateTokenCopyeach repeat the same bound-then-ceiling-check block, differing only in the label string. Extract a helper to remove the duplication and keep the three checks from drifting apart later.♻️ Proposed refactor to remove duplication
+fn bound_debug_create_count(label: &str, count: u32) -> Result<(), String> { + bound_batch_count(label, count)?; + if count > MAX_DEBUG_CREATE_COUNT { + return Err(format!( + "{label} {count} exceeds the maximum {MAX_DEBUG_CREATE_COUNT}" + )); + } + Ok(()) +} + fn guard_debug_action_payload(action: &DebugAction) -> Result<(), String> { match action { DebugAction::CreateCard { card_name, count, .. } => { bound_string("Debug.CreateCard.card_name", card_name)?; - bound_batch_count("Debug.CreateCard.count", *count)?; - if *count > MAX_DEBUG_CREATE_COUNT { - return Err(format!( - "Debug.CreateCard.count {count} exceeds the maximum {MAX_DEBUG_CREATE_COUNT}" - )); - } + bound_debug_create_count("Debug.CreateCard.count", *count)?; } DebugAction::AddMana { mana, .. } => { bound_list("Debug.AddMana.mana", mana.len())?; } DebugAction::CreateToken { request, count, .. } => { - bound_batch_count("Debug.CreateToken.count", *count)?; - if *count > MAX_DEBUG_CREATE_COUNT { - return Err(format!( - "Debug.CreateToken.count {count} exceeds the maximum {MAX_DEBUG_CREATE_COUNT}" - )); - } + bound_debug_create_count("Debug.CreateToken.count", *count)?; guard_debug_token_request_payload(request)?; } DebugAction::CreateTokenCopy { count, .. } => { - bound_batch_count("Debug.CreateTokenCopy.count", *count)?; - if *count > MAX_DEBUG_CREATE_COUNT { - return Err(format!( - "Debug.CreateTokenCopy.count {count} exceeds the maximum {MAX_DEBUG_CREATE_COUNT}" - )); - } + bound_debug_create_count("Debug.CreateTokenCopy.count", *count)?; }🤖 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/server-core/src/game_action_payload_guard.rs` around lines 365 - 395, Extract the repeated batch-count validation into a shared helper near the existing guard functions, accepting the label and count and applying both bound_batch_count and the MAX_DEBUG_CREATE_COUNT ceiling error. Replace the duplicate validation blocks in the CreateCard, CreateToken, and CreateTokenCopy branches with calls to that helper, preserving their distinct labels and existing error behavior.
🤖 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/adapter/types.ts`:
- Around line 2469-2478: Replace the PlayerPerformedAction data.action string
type with a closed string-literal union matching all PlayerActionKind variants
emitted by the engine, including Scry, Surveil, Investigate, CollectEvidence,
SearchedLibrary, ShuffledLibrary, and Proliferate. Keep look_count,
scry_bottom_count, and scry_top_count optional, and ensure non-scry/non-surveil
variants remain valid when those counts are absent.
In `@client/src/components/chrome/DebugCardContextMenu.tsx`:
- Around line 253-259: Keep engine counter identifiers raw at both affected
sites: in client/src/components/chrome/DebugCardContextMenu.tsx lines 253-259,
pass counterType directly as the CounterRow label instead of formatCounterType;
in client/src/components/chrome/DebugObjectActions.tsx lines 227-232, pass the
raw counterType value to SelectInput instead of formatting it.
In `@client/src/components/chrome/DebugObjectActions.tsx`:
- Around line 155-157: Update the “Copies” label in the DebugObjectActions
component to use the existing t() localization function, and add the
corresponding locale entries for this key in the supported translation
resources.
In `@client/src/game/diceContest.ts`:
- Around line 92-105: Invoke flashCompletedScry from the production
event-delivery path in client/src/game/dispatch.ts alongside flashInGameRolls,
ensuring completed Scry events update scryOutcome before ScryOutcomeOverlay
renders. Keep the existing flashCompletedScry behavior and test usage unchanged.
In `@client/src/services/backup.ts`:
- Around line 111-127: Update parseRecord and parseFolders to validate every
parsed nested value against the required DeckMeta and DeckFolder shapes before
casting and returning typed collections. Return null for any invalid entry,
including null values, so callers treat the payload as unmergeable and retain
the local raw value; preserve the existing empty defaults for null raw input.
In `@client/src/stores/cloudSyncStore.ts`:
- Line 39: Add a merge action to the conflict controls in PreferencesModal,
alongside the existing cloud and local choices, calling resolveConflict("merge")
and displaying the t("sync.keepBothDecks") label so the ConflictChoice merge
flow is reachable.
In `@client/src/stores/uiStore.ts`:
- Line 130: Update clearPromptOverlayState to call resetScryOutcome alongside
resetDiceRoll, ensuring session cleanup clears scryOutcome and cancels any
pending scryOutcomeTimer before the next game session.
In `@crates/engine-wasm/src/lib.rs`:
- Around line 1470-1508: Make engine::check_debug_action_access public, then
call it once at the beginning of the WASM debug-action function before branching
on count or accessing CARD_DB. Remove the duplicated debug_mode and
debug_permitted checks from both branches, while preserving owner validation and
the existing zero-count result behavior after centralized authorization.
- Around line 1348-1361: Update the replay handling around record_replay_action
so zero-count debug creates skip recording entirely rather than being passed as
a non-debug action that appends to REPLAY_LOG; preserve the existing clearing
behavior for other debug actions and appending behavior for normal actions.
Strengthen the sibling zero-count replay test to assert the recorded action
count or exported replay JSON excludes the debug action, rather than only
checking has_replay_recording().
In `@crates/engine/src/game/engine_debug.rs`:
- Around line 609-612: Update apply_debug_action’s debug-create count conversion
to propagate an EngineError when i32::try_from(count) fails instead of calling
expect and panicking. Preserve the existing successful QuantityExpr::Fixed
construction and use the function’s Result return path for the conversion
failure.
- Around line 941-974: Ensure drain_debug_card_entries preserves or explicitly
rejects pending batches when called while state.waiting_for is not
WaitingFor::Priority; currently the loop is skipped and pending is discarded.
Update the entry handling around drain_debug_card_entries and create_debug_cards
so true battlefield entries with run_etb enabled either park the active
debug-card frame for later processing or return an explicit error, while
preserving synchronous creation behavior.
---
Outside diff comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 1573-1579: Add a transport serialization round-trip test for the
Scry variant of PlayerPerformedAction, covering all-top, all-bottom, mixed, and
zero-count outcomes. Assert that look_count, scry_bottom_count, and
scry_top_count survive serialization and deserialization with their expected
values.
In `@crates/server-core/src/protocol.rs`:
- Around line 2259-2283: Update the REVERT-PROBE documentation in
full_game_floor_is_current_only_not_a_rollout_window to reference
protocol_version_is_25 instead of the renamed protocol_version_is_24 test,
leaving the test logic unchanged.
---
Nitpick comments:
In `@client/src/components/chrome/DebugCreateActions.tsx`:
- Around line 235-237: Use the existing MAX_DEBUG_CREATE_COUNT definition as the
shared source, export it for client use, and update all four debug create forms
in DebugCreateActions to pass max={MAX_DEBUG_CREATE_COUNT} on their copy-count
NumberInput components while retaining min={0}; do not duplicate or re-derive
the limit in the UI.
In `@crates/server-core/src/game_action_payload_guard.rs`:
- Around line 365-395: Extract the repeated batch-count validation into a shared
helper near the existing guard functions, accepting the label and count and
applying both bound_batch_count and the MAX_DEBUG_CREATE_COUNT ceiling error.
Replace the duplicate validation blocks in the CreateCard, CreateToken, and
CreateTokenCopy branches with calls to that helper, preserving their distinct
labels and existing error behavior.
In `@crates/server-core/tests/game_action_payload_guard.rs`:
- Around line 299-310: Add tests alongside
rejects_debug_create_count_above_engine_ceiling covering DebugAction::CreateCard
and DebugAction::CreateToken with count set to MAX_DEBUG_CREATE_COUNT + 1.
Assert guard_game_action_payload returns an error containing the respective
count field path, matching the existing CreateTokenCopy test.
🪄 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: 9d499d84-7dd2-4180-a5b5-a157326d8e8b
📒 Files selected for processing (70)
client/src/adapter/types.tsclient/src/adapter/ws-adapter.tsclient/src/components/animation/ScryOutcomeOverlay.tsxclient/src/components/animation/__tests__/ScryOutcomeOverlay.test.tsxclient/src/components/chrome/DebugCardContextMenu.tsxclient/src/components/chrome/DebugCreateActions.tsxclient/src/components/chrome/DebugObjectActions.tsxclient/src/components/chrome/__tests__/DebugCreateActions.test.tsclient/src/components/chrome/__tests__/DebugPanel.sandboxCapability.test.tsxclient/src/components/chrome/debugFields.tsxclient/src/game/__tests__/diceContest.test.tsclient/src/game/diceContest.tsclient/src/i18n/locales/de/common.jsonclient/src/i18n/locales/de/settings.jsonclient/src/i18n/locales/en/common.jsonclient/src/i18n/locales/en/settings.jsonclient/src/i18n/locales/es/common.jsonclient/src/i18n/locales/es/settings.jsonclient/src/i18n/locales/fr/common.jsonclient/src/i18n/locales/fr/settings.jsonclient/src/i18n/locales/it/common.jsonclient/src/i18n/locales/it/settings.jsonclient/src/i18n/locales/pl/common.jsonclient/src/i18n/locales/pl/settings.jsonclient/src/i18n/locales/pt/common.jsonclient/src/i18n/locales/pt/settings.jsonclient/src/network/__tests__/protocol.test.tsclient/src/network/protocol.tsclient/src/pages/GamePage.tsxclient/src/services/__tests__/backup.test.tsclient/src/services/backup.tsclient/src/stores/__tests__/cloudSyncStore.test.tsclient/src/stores/cloudSyncStore.tsclient/src/stores/uiStore.tscrates/engine-wasm/src/lib.rscrates/engine/src/analysis/sim.rscrates/engine/src/game/effects/collect_evidence.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/investigate.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/scoped_library_search.rscrates/engine/src/game/effects/scry.rscrates/engine/src/game/effects/search_library.rscrates/engine/src/game/effects/surveil.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/game_object.rscrates/engine/src/game/library.rscrates/engine/src/game/log.rscrates/engine/src/game/mod.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/events.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/cr733_resolved_trigger_collection.rscrates/lobby-broker/src/protocol.rscrates/phase-server/src/main.rscrates/server-core/src/game_action_payload_guard.rscrates/server-core/src/protocol.rscrates/server-core/src/session.rscrates/server-core/tests/game_action_payload_guard.rsscripts/check-protocol-version.mjs
| | { | ||
| type: "PlayerPerformedAction"; | ||
| data: { | ||
| player_id: PlayerId; | ||
| action: string; | ||
| look_count?: number; | ||
| scry_bottom_count?: number; | ||
| scry_top_count?: number; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List PlayerActionKind variants and every emission site.
set -euo pipefail
fd -e rs . crates/engine/src --exec rg -nP -C2 '\benum\s+PlayerActionKind\b' {} +
rg -nP --type=rust -C2 '\bPlayerActionKind::[A-Za-z]+' crates
# Confirm how the client currently narrows the field.
rg -nP --type=ts -C3 'PlayerPerformedAction' client/srcRepository: phase-rs/phase
Length of output: 24702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Locate relevant files ----"
git ls-files | rg '(^|/)types\.rs$|client/src/adapter/types\.ts$|engine\.rs$' | sed -n '1,120p'
echo
echo "---- PlayerActionKind enum ----"
rg -n -C 10 'pub enum PlayerActionKind|enum PlayerActionKind' .
echo
echo "---- PlayerPerformedAction emissions ----"
rg -n -C 4 'PlayerPerformedAction|ActionKind|player_action|scry_top_count|scry_bottom_count|look_count|type: PlayerActionKind' crates || true
echo
echo "---- Client types occurrence ----"
rg -n -C 8 'type: "PlayerPerformedAction"|PlayerPerformedAction' client/src || true
echo
echo "---- Client narrow usages ----"
python3 - <<'PY'
from pathlib import Path
import re
root=Path('client/src')
for p in root.rglob('*'):
if p.suffix not in {'.ts','.tsx'}:
continue
text=p.read_text(errors='ignore')
if 'PlayerPerformedAction' in text:
print(f'--- {p} ---')
lines=text.splitlines()
for i,l in enumerate(lines,1):
if 'PlayerPerformedAction' in l:
start=max(1,i-6); end=min(len(lines),i+18)
for j in range(start,end+1):
print(f'{j:5}: {lines[j-1]}')
PYRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Engine PlayerPerformedAction/PlayerActionKind occurrences (focused) ----"
rg -n -C 3 'GameEvent::PlayerPerformedAction|player_actions_this_way|player_actions_this_turn|PlayerActionKind::|PlayerActionKind\|' crates/engine/src crates/engine/tests --glob '*.rs' | \
python3 - <<'PY'
import sys, subprocess, re
text = subprocess.check_output(['grep','-n','-C3','GameEvent::PlayerPerformedAction|player_actions_this_way|player_actions_this_turn|PlayerActionKind::|PlayerActionKind\\|','crates/engine/src','crates/engine/tests','-R','--include','*.rs','--color=never'], text=True)
lines = text.splitlines()
print('\n'.join(lines[:400]))
PY
echo
echo "---- Client TypeScript PlayerPerformedAction occurrences with line numbers ----"
rg -n -C 6 'PlayerPerformedAction|type: "PlayerPerformedAction"|action: string' client/src --glob '*.ts' --glob '*.tsx' || true
echo
echo "---- TypeScript union mirror names nearby ----"
rg -n -C 8 'type .* = .*\\|;|enum .* \\{|union|DungeonId|PlanarDieFace|GameEvent' client/src/adapter/types.ts | sed -n '1,260p'Repository: phase-rs/phase
Length of output: 44114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract only PlayerActionKind declarations and GameEvent construction sites for PlayerPerformedAction.
python3 - <<'PY'
import pathlib,time
for path in pathlib.Path('crates/engine/src').rglob('*.rs') + pathlib.Path('crates/engine/tests').rglob('*.rs'):
try:
s=path.read_text(errors='ignore')
except Exception:
continue
if 'pub enum PlayerActionKind' in s or 'enum PlayerActionKind' in s:
print(f'--- {path} enum ---')
for i,l in enumerate(s.splitlines(),1):
if 'PlayerActionKind' in l: print(f'{i}: {l}')
if 'GameEvent::PlayerPerformedAction' in s:
print(f'--- {path} constructions ---')
lines=s.splitlines()
for i,l in enumerate(lines,1):
if 'GameEvent::PlayerPerformedAction' in l:
start=max(1,i-4); end=min(len(lines),i+10)
for j in range(start,end+1):
print(f'{j}: {lines[j-1]}')
PY
echo
python3 - <<'PY'
import pathlib
for path in pathlib.Path('client/src').rglob('*'):
if path.suffix not in {'.ts','.tsx'}:
continue
text=path.read_text(errors='ignore')
if 'PlayerPerformedAction' in text:
print(f'--- {path} ---')
lines=text.splitlines()
for i,l in enumerate(lines,1):
if 'PlayerPerformedAction' in l:
start=max(1,i-8); end=min(len(lines),i+12)
for j in range(start,end+1):
print(f'{j}: {lines[j-1]}')
PYRepository: phase-rs/phase
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
rs = pathlib.PurePosixPath('crates/engine/src/**/*.rs')
tests = pathlib.PurePosixPath('crates/engine/tests/**/*.rs')
for path in list(pathlib.Path('crates/engine/src').rglob('*.rs')) + list(pathlib.Path('crates/engine/tests').rglob('*.rs')):
try:
s = path.read_text(errors='ignore')
except Exception:
continue
if 'pub enum PlayerActionKind' in s or 'enum PlayerActionKind' in s:
print(f'--- {path} enum ---')
for i, l in enumerate(s.splitlines(), 1):
if 'PlayerActionKind' in l:
print(f'{i}: {l}')
if 'GameEvent::PlayerPerformedAction' in s:
lines = s.splitlines()
print(f'--- {path} constructions ---')
for i, l in enumerate(lines, 1):
if 'GameEvent::PlayerPerformedAction' in l:
start = max(1, i - 4)
end = min(len(lines), i + 10)
for j in range(start, end + 1):
print(f'{j}: {lines[j-1]}')
PY
echo
python3 - <<'PY'
import pathlib
for path in pathlib.Path('client/src').rglob('*'):
if path.suffix not in {'.ts', '.tsx'}:
continue
text = path.read_text(errors='ignore')
if 'PlayerPerformedAction' in text:
print(f'--- {path} ---')
lines = text.splitlines()
for i, l in enumerate(lines, 1):
if 'PlayerPerformedAction' in l:
start = max(1, i - 8)
end = min(len(lines), i + 12)
for j in range(start, end + 1):
print(f'{j}: {lines[j-1]}')
PYRepository: phase-rs/phase
Length of output: 50370
Mirror PlayerActionKind with a closed string-literal union.
GameEvent::PlayerPerformedAction is a Rust PlayerActionKind; its variants are emitted by the engine (Scry, Surveil, Investigate, CollectEvidence, SearchedLibrary, ShuffledLibrary, Proliferate, etc.). In client/src/adapter/types.ts, widening it to string makes client narrowing incomplete and weakens the transport contract against future variant changes. Keep the existing optional count fields, but make non-scry/non-surveil actions handle missing counts.
🤖 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/types.ts` around lines 2469 - 2478, Replace the
PlayerPerformedAction data.action string type with a closed string-literal union
matching all PlayerActionKind variants emitted by the engine, including Scry,
Surveil, Investigate, CollectEvidence, SearchedLibrary, ShuffledLibrary, and
Proliferate. Keep look_count, scry_bottom_count, and scry_top_count optional,
and ensure non-scry/non-surveil variants remain valid when those counts are
absent.
| <CounterRow | ||
| key={counterType} | ||
| label={formatCounterType(counterType)} | ||
| objectId={objectId} | ||
| counterType={counterType} | ||
| current={current} | ||
| onDispatch={dispatchDebugKeepOpen} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep engine counter identifiers raw.
formatCounterType changes serialized counter identifiers such as "P1P1" before display. Pass the engine value unchanged.
client/src/components/chrome/DebugCardContextMenu.tsx#L253-L259: usecounterTypeas the row label.client/src/components/chrome/DebugObjectActions.tsx#L227-L232: do not passformatCounterTypetoSelectInput.
Based on learnings, engine-provided enum strings must stay raw.
📍 Affects 2 files
client/src/components/chrome/DebugCardContextMenu.tsx#L253-L259(this comment)client/src/components/chrome/DebugObjectActions.tsx#L227-L232
🤖 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/chrome/DebugCardContextMenu.tsx` around lines 253 -
259, Keep engine counter identifiers raw at both affected sites: in
client/src/components/chrome/DebugCardContextMenu.tsx lines 253-259, pass
counterType directly as the CounterRow label instead of formatCounterType; in
client/src/components/chrome/DebugObjectActions.tsx lines 227-232, pass the raw
counterType value to SelectInput instead of formatting it.
Sources: Path instructions, Learnings
| <FieldRow label="Copies"> | ||
| <NumberInput value={count} onChange={setCount} min={0} /> | ||
| </FieldRow> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localize the new "Copies" label.
This is frontend-authored user-facing text. Route it through t() and add the locale entries.
🤖 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/chrome/DebugObjectActions.tsx` around lines 155 - 157,
Update the “Copies” label in the DebugObjectActions component to use the
existing t() localization function, and add the corresponding locale entries for
this key in the supported translation resources.
Source: Path instructions
| export function flashCompletedScry(events: GameEvent[]): void { | ||
| const scry = events.find( | ||
| (event): event is CompletedScryEvent => | ||
| event.type === "PlayerPerformedAction" && | ||
| event.data.action === "Scry" && | ||
| event.data.scry_top_count !== undefined && | ||
| event.data.scry_bottom_count !== undefined, | ||
| ); | ||
| if (!scry) return; | ||
| useUiStore.getState().flashScryOutcome({ | ||
| playerId: scry.data.player_id, | ||
| topCount: scry.data.scry_top_count, | ||
| bottomCount: scry.data.scry_bottom_count, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect at least one non-test invocation in the production client.
rg -nP --glob '*.{ts,tsx}' \
-g '!**/__tests__/**' -g '!**/*.test.*' \
'^(?!\s*export\s+function\s+flashCompletedScry\b).*?\bflashCompletedScry\s*\(' \
client/src || true
# Trace the store write and event-consumption sites.
rg -n -C 4 --glob '*.{ts,tsx}' \
'\bflashScryOutcome\b|\bscryOutcome\b|\bPlayerPerformedAction\b' \
client/srcRepository: phase-rs/phase
Length of output: 12862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files invoking ScryOutcomeOverlay (production/tests):\n'
rg -n --glob '*.{ts,tsx}' 'ScryOutcomeOverlay|flashCompletedScry' client/src || true
printf '\nclient/src/game/diceContest.ts outline and relevant content:\n'
wc -l client/src/game/diceContest.ts
sed -n '1,130p' client/src/game/diceContest.ts
printf '\nSearch events/event-handler call sites in production src:\n'
rg -n --glob '*.{ts,tsx}' \
'processEvent|handle.*Event|onGameEvent|GameEvent|useGameEvents|useEvents|EventProvider|events' \
client/src || trueRepository: phase-rs/phase
Length of output: 50371
No production caller invokes flashCompletedScry.
ScryOutcomeOverlay reads scryOutcome, but client/src/pages/GamePage.tsx only mounts the overlay. In client/src/game/dispatch.ts, flashInGameRolls(events) runs after action resolution, while flashCompletedScry(events) remains unused outside tests. Call it through the same event-delivery path so completed Scry outcomes populate the UI store.
🤖 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/diceContest.ts` around lines 92 - 105, Invoke
flashCompletedScry from the production event-delivery path in
client/src/game/dispatch.ts alongside flashInGameRolls, ensuring completed Scry
events update scryOutcome before ScryOutcomeOverlay renders. Keep the existing
flashCompletedScry behavior and test usage unchanged.
| function parseRecord<T>(raw: string | null): Record<string, T> | null { | ||
| if (raw == null) return {}; | ||
| try { | ||
| const value: unknown = JSON.parse(raw); | ||
| return value !== null && typeof value === "object" && !Array.isArray(value) | ||
| ? (value as Record<string, T>) | ||
| : null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| function parseFolders(raw: string | null | undefined): DeckFolder[] | null { | ||
| if (raw == null) return []; | ||
| try { | ||
| const value: unknown = JSON.parse(raw); | ||
| return Array.isArray(value) ? (value as DeckFolder[]) : null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate parsed record values and folder entries.
The top-level checks accept invalid nested values. For example, {"Shared": null} passes parseRecord<DeckMeta>() and then meta.folderId throws at Line 178. Similarly, [null] passes parseFolders() and then folder.id throws at Line 146.
Validate each DeckMeta and DeckFolder value before returning typed data. Treat an invalid payload as an unmergeable collection and retain the local raw value.
🤖 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/services/backup.ts` around lines 111 - 127, Update parseRecord and
parseFolders to validate every parsed nested value against the required DeckMeta
and DeckFolder shapes before casting and returning typed collections. Return
null for any invalid entry, including null values, so callers treat the payload
as unmergeable and retain the local raw value; preserve the existing empty
defaults for null raw input.
| // payload; `diceRollQueue` holds the pending ones. Distinct from the board-event | ||
| // step queue (animationStore) — that coordinates spatial per-object effects. | ||
| let diceAdvanceTimer: ReturnType<typeof setTimeout> | null = null; | ||
| let scryOutcomeTimer: ReturnType<typeof setTimeout> | null = null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 10 '\b(resetScryOutcome|flashScryOutcome)\b' client/src || true
rg -n -C 12 'ScryOutcomeOverlay|useEffect\(' \
client/src/pages/GamePage.tsx \
client/src/components/animation/ScryOutcomeOverlay.tsx || trueRepository: phase-rs/phase
Length of output: 31417
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '--- DiceRollOverlay cleanup ---\n'
fd -a 'DiceRollOverlay.tsx' client/src -x sh -c 'echo "FILE: $1"; nl -ba "$1" | sed -n "1,220p"' sh {}
printf '\n--- DiceRollOverlay tests ---\n'
fd -a 'DiceRollOverlay.test.tsx|diceRollOverlay.*test.*' client/src -x sh -c 'echo "FILE: $1"; nl -ba "$1" | sed -n "1,240p"' sh {}
printf '\n--- GamePage overlay cleanup effects search ---\n'
rg -n -C 6 'clear(Initial|Prompt|Selected|Reroll|DiceRoll|Scry|Game)|Timer|setTimeout|clearTimeout|useEffect\(\\(\\) =>|resetDiceRoll|resetScryOutcome' client/src/pages/GamePage.tsx client/src/hooks | sed -n '1,240p' || true
printf '\n--- Global resetScryOutcome call sites ---\n'
rg -n -C 4 'resetScryOutcome' client/src || trueRepository: phase-rs/phase
Length of output: 21142
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "--- DiceRollOverlay.tsx ---"
FILE="$(fd 'DiceRollOverlay.tsx' client/src | head -n1)"
if [ -n "${FILE:-}" ]; then
sed -n '1,220p' "$FILE"
fi
echo
echo "--- DiceRollOverlay tests ---"
sed -n '1,240p' "$(fd 'DiceRollOverlay.test.tsx' client/src | head -n1)"
echo
echo "--- clearPromptOverlayState definition and usages ---"
rg -n -C 8 'function clearPromptOverlayState|const clearPromptOverlayState|export .*clearPromptOverlayState|clearPromptOverlayState\(' client/src/game client/src/pages client/src/hooks || true
echo
echo "--- uiStore timer-related lines ---"
sed -n '1,40p' client/src/stores/uiStore.ts
sed -n '260,715p' client/src/stores/uiStore.ts | sed -n '1,85p'
sed -n '630,710p' client/src/stores/uiStore.tsRepository: phase-rs/phase
Length of output: 35211
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "--- sessionCleanup.ts ---"
sed -n '1,180p' client/src/game/sessionCleanup.ts
echo
echo "--- DiceRollOverlay cleanup tests ---"
sed -n '1,320p' "$(fd 'DiceRollOverlay.test.tsx' client/src | head -n1)"
echo
echo "--- read-only summary: all clearPromptOverlayState calls outside tests ---"
python3 - <<'PY'
from pathlib import Path
for p in Path('client/src').rglob('*'):
if p.suffix not in {'.ts','.tsx','.js','.jsx'} or 'node_modules' in str(p) or '__tests__' in str(p):
continue
txt = p.read_text(errors='replace')
if 'clearPromptOverlayState' in txt:
for i,line in enumerate(txt.splitlines(),1):
if 'clearPromptOverlayState()' in line:
print(f"{p}:{i}:{line.strip()}")
PYRepository: phase-rs/phase
Length of output: 3105
Add resetScryOutcome() to clearPromptOverlayState.
scryOutcomeTimer and scryOutcome are cleared by resetScryOutcome, but session cleanup does not call it. Add it alongside resetDiceRoll() so a pending timer cannot persist into the next game session.
🤖 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/stores/uiStore.ts` at line 130, Update clearPromptOverlayState to
call resetScryOutcome alongside resetDiceRoll, ensuring session cleanup clears
scryOutcome and cancels any pending scryOutcomeTimer before the next game
session.
Source: Path instructions
| if count == 0 { | ||
| return with_state(|state| { | ||
| if !state.debug_mode { | ||
| return Err("Engine error: Debug actions require debug_mode to be enabled"); | ||
| } | ||
| if !state.debug_permitted.is_empty() && !state.debug_permitted.contains(&actor) { | ||
| return Err("Engine error: Debug actions require debug permission"); | ||
| } | ||
| if !state.players.iter().any(|player| player.id == owner) { | ||
| return Err("Engine error: Debug: invalid owner player id"); | ||
| } | ||
| Ok(engine::types::game_state::ActionResult { | ||
| events: vec![], | ||
| waiting_for: state.waiting_for.clone(), | ||
| log_entries: vec![], | ||
| }) | ||
| }) | ||
| .unwrap_or(Err(NOT_INITIALIZED_ERR)); | ||
| } | ||
| let source = CARD_DB.with(|cell| { | ||
| let db = cell.borrow(); | ||
| let Some(db) = db.as_ref() else { | ||
| return Err("Engine error: card database not loaded"); | ||
| }; | ||
| match db.get_face_by_name(card_name) { | ||
| Some(face) => Ok(face.clone()), | ||
| Some(face) => Ok(engine::game::debug_card_entry_source(db, face)), | ||
| None => Err("Engine error: card not found in database"), | ||
| } | ||
| })?; | ||
| with_state_mut(|state| { | ||
| if !state.debug_mode { | ||
| return Err("Engine error: Debug actions require debug_mode to be enabled"); | ||
| } | ||
| if !state.debug_permitted.is_empty() && !state.debug_permitted.contains(&actor) { | ||
| return Err("Engine error: Debug actions require debug permission"); | ||
| } | ||
| if !state.players.iter().any(|p| p.id == owner) { | ||
| return Err("Engine error: Debug: invalid owner player id"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Route both branches through one authorization authority, and check authorization before the card lookup.
Two problems in this function.
-
The
debug_modeanddebug_permittedgate is written twice here (lines 1472-1477 and 1500-1505) with independently-worded error strings, and a third copy is the engine's owncheck_debug_action_access(crates/engine/src/game/engine.rsline 11584). That helper's doc comment states it exists so "transports cannot use a no-op payload to probe or bypass debug authorization". Re-implementing it in the adapter is the drift the helper was added to prevent. Export the engine helper and call it once at the top of this function, before thecount == 0branch. -
For
count > 0theCARD_DBlookup at lines 1489-1498 runs before the authorization check at line 1500. An unauthorized actor receives "card not found in database" or a success-shaped lookup outcome before authorization is evaluated, which discloses database membership. The outer gate at line 1312 covers this in multiplayer today, so it is not currently exploitable; the ordering still must be inverted so the guarantee does not depend on that outer gate remaining.
🛡️ Proposed restructure
if count > engine::types::actions::MAX_DEBUG_CREATE_COUNT {
return Err("Engine error: debug create count exceeds the maximum");
}
+ // One authority for the sandbox capability gate, evaluated before any
+ // card-database lookup can disclose membership.
+ with_state(|state| {
+ engine::game::check_debug_action_access(state, actor)
+ .map_err(|_| "Engine error: Debug actions require debug permission")?;
+ if !state.players.iter().any(|player| player.id == owner) {
+ return Err("Engine error: Debug: invalid owner player id");
+ }
+ Ok(())
+ })
+ .unwrap_or(Err(NOT_INITIALIZED_ERR))?;
if count == 0 {
return with_state(|state| {
- if !state.debug_mode { /* … */ }
- if !state.debug_permitted.is_empty() && !state.debug_permitted.contains(&actor) { /* … */ }
- if !state.players.iter().any(|player| player.id == owner) { /* … */ }
Ok(engine::types::game_state::ActionResult {
events: vec![],
waiting_for: state.waiting_for.clone(),
log_entries: vec![],
})
})
.unwrap_or(Err(NOT_INITIALIZED_ERR));
}Note that check_debug_action_access is currently a private fn in engine.rs. Making it pub is part of this change.
As per path instructions, the engine owns all game logic and "adapters (WASM, WebSocket, Tauri, P2P) are thin serialization boundaries with zero rules".
🤖 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-wasm/src/lib.rs` around lines 1470 - 1508, Make
engine::check_debug_action_access public, then call it once at the beginning of
the WASM debug-action function before branching on count or accessing CARD_DB.
Remove the duplicated debug_mode and debug_permitted checks from both branches,
while preserving owner validation and the existing zero-count result behavior
after centralized authorization.
Source: Path instructions
| fn drain_debug_card_entries( | ||
| state: &mut GameState, | ||
| mut pending: PendingDebugCardEntries, | ||
| events: &mut Vec<GameEvent>, | ||
| ) { | ||
| while pending.remaining > 0 && matches!(state.waiting_for, WaitingFor::Priority { .. }) { | ||
| let child_stack_start = state.resolution_stack.len(); | ||
| let object_id = materialize_debug_card( | ||
| state, | ||
| &pending.source, | ||
| pending.owner, | ||
| pending.attach_to, | ||
| pending.nonlegendary, | ||
| Zone::Hand, | ||
| ); | ||
| pending.remaining -= 1; | ||
| let entry = route_debug_create_to_battlefield(state, object_id, true); | ||
| events.extend(entry.events); | ||
| state.waiting_for = entry.waiting_for; | ||
|
|
||
| if !matches!(state.waiting_for, WaitingFor::Priority { .. }) | ||
| || state.resolution_stack.len() > child_stack_start | ||
| { | ||
| if state.resolution_stack.len() > child_stack_start { | ||
| state | ||
| .insert_debug_card_entries_parent_at_child_boundary(pending, child_stack_start) | ||
| .expect("debug-card parent must sit below the entry child stack"); | ||
| } else { | ||
| state.push_debug_card_entries(pending); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace the debug-card frame lifecycle: producers, parkers, resumers.
set -euo pipefail
rg -nP --type=rust -C4 'drain_pending_debug_card_entries|push_debug_card_entries|take_active_debug_card_entries|insert_debug_card_entries_parent_at_child_boundary|active_debug_card_entries'
# Every caller of the batch entry point and its waiting_for preconditions.
rg -nP --type=rust -C6 '\bcreate_debug_cards\s*\('
# Confirm DebugCardEntries frames are drained alongside the other resolution frames.
rg -nP --type=rust -C3 'ResolutionFrame::DebugCardEntries'Repository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Rust files with relevant identifiers =="
rg -n --type=rust -C 4 'drain_pending_debug_card_entries|push_debug_card_entries|take_active_debug_card_entries|insert_debug_card_entries_parent_at_child_boundary|active_debug_card_entries|pending_debug_card_entries|PendingDebugCardEntries' . || true
echo
echo "== create_debug_cards call sites =="
rg -n --type=rust -C 6 '\\bcreate_debug_cards\\s*\\(' . || true
echo
echo "== ResolutionFrame entries =="
rg -n --type=rust -C 3 'ResolutionFrame::DebugCardEntries|enum ResolutionFrame|Resume|Resolution' crates/engine/src/game crates/engine/src 2>/dev/null | head -n 220 || true
echo
echo "== debug create card and debugging/action routing =="
rg -n --type=rust -C 5 'handle_debug_create_card_inner|create_debug_cards|bypass WaitingFor|DebugCard|Debug.*WaitingFor' crates/engine/src crates -g '*.rs' 2>/dev/null | head -n 260 || trueRepository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== engine_debug functions and definitions around debug card logic =="
wc -l crates/engine/src/game/engine_debug.rs
sed -n '800,1050p' crates/engine/src/game/engine_debug.rs
sed -n '1,120p' crates/engine/src/game/engine_debug.rsRepository: phase-rs/phase
Length of output: 14185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WASM debug create handler =="
sed -n '1390,1575p' crates/engine-wasm/src/lib.rs
echo
echo "== DebugAction CreateCard validation =="
sed -n '870,925p' crates/engine/src/types/actions.rs
echo
echo "== engine_debug tests around batch behavior =="
sed -n '1115,1230p' crates/engine/src/game/engine_debug.rs
echo
echo "== deterministic body skip probe from source text =="
python3 - <<'PY'
from pathlib import Path
src = Path('crates/engine/src/game/engine_debug.rs').read_text()
start = src.index('fn drain_debug_card_entries(')
end = src.index('fn materialize_debug_card(')
snippet = src[start:end]
print('while_condition:')
print(snippet.split('drain_debug_card_entries', 2)[2].split('fn materialize_debug_card')[0].split('while', 1)[1].split('fn materialize_debug_card')[0].strip())
print('park_statements_present_in_body:')
for name in ['insert_debug_card_entries_parent_at_child_boundary', 'push_debug_card_entries', 'return;']:
print(f'{name}: {snippet.index(name) != -1}, index={snippet.index(name) if snippet.index(name) != -1 else "-"}')
print('no_waiting_for_priority_guard_before_while:', 'matches!(state.waiting_for, WaitingFor::Priority { .. })' not in snippet.split('fn drain_debug_card_entries', 2)[1].split('while', 1)[0])
PYRepository: phase-rs/phase
Length of output: 15741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
src = Path('crates/engine/src/game/engine_debug.rs').read_text()
start = src.index('fn drain_debug_card_entries(\n')
end = src.index('fn materialize_debug_card(\n')
snippet = src[start:end]
print(snippet[:900])
print('while_condition:', 'while pending.remaining > 0 && matches!(state.waiting_for, WaitingFor::Priority { .. })' in snippet)
print('first_non_empty_line:', [row for row in snippet.splitlines() if row.strip()][0])
print('park_before_while:', 'state.push_debug_card_entries(pending);' in snippet.split('fn drain_debug_card_entries(\n',1)[1].split('while',1)[0])
print('park_inside_loop_entries:', [(name, snippet.split("while pending.remaining > 0 && matches!(state.waiting_for, WaitingFor::Priority { .. })")[1].find(name)) for name in ['state.push_debug_card_entries(pending);', 'insert_debug_card_entries_parent_at_child_boundary', 'return;']])
PYRepository: phase-rs/phase
Length of output: 1308
A real-entry debug card batch requested while waiting_for is not Priority is silently discarded.
Only the synchronous create path has no precondition. drain_debug_card_entries starts processing only inside the while pending.remaining > 0 && matches!(state.waiting_for, WaitingFor::Priority { .. }) loop, and the only preservation that parks pending happens inside that loop body. If drain_debug_card_entries is called while another prompt is open, the loop never runs, pending drops, and create_debug_cards returns success with no cards created and no pending batch. This is reachable for true battlefield entries with run_etb == true; non-battlefield/raw-placement branches take the synchronous loop. Either park the active debug-card frame before returning or reject the request with an explicit error.
🤖 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_debug.rs` around lines 941 - 974, Ensure
drain_debug_card_entries preserves or explicitly rejects pending batches when
called while state.waiting_for is not WaitingFor::Priority; currently the loop
is skipped and pending is discarded. Update the entry handling around
drain_debug_card_entries and create_debug_cards so true battlefield entries with
run_etb enabled either park the active debug-card frame for later processing or
return an explicit error, while preserving synchronous creation behavior.
a83ce5d to
4d67f5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/game/engine_debug.rs (1)
1542-1565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a zero-count case for
create_debug_cards.
debug_create_zero_is_authorized_noop_without_finalizationcoversDebugAction::CreateTokenwithcount: 0. The early return increate_debug_cardsat lines 872-878 has no test. That path is the card equivalent and returns before any materialization or frame push, so a regression there would not be caught.♻️ Proposed additional test
#[test] fn debug_create_zero_cards_is_a_noop() { let mut state = sandbox_state(); let result = create_debug_cards( &mut state, DebugCardCreateRequest { source: DebugCardEntrySource { face: CardFace { name: "Unrequested Debug Card".into(), ..Default::default() }, back_face: None, }, owner: PlayerId(0), zone: Zone::Battlefield, count: 0, attach_to: None, run_etb: true, nonlegendary: false, }, ); assert!(result.events.is_empty()); assert!(state.objects.is_empty()); assert!(state.active_debug_card_entries().is_none()); }🤖 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_debug.rs` around lines 1542 - 1565, Add a unit test alongside the existing debug creation tests for `create_debug_cards` with `count: 0`, using a minimal `DebugCardCreateRequest`. Assert that the operation produces no events, creates no objects, and leaves `state.active_debug_card_entries()` as `None`, confirming the early return occurs without materialization or frame setup.
🤖 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 `@crates/engine/src/game/engine_debug.rs`:
- Around line 931-939: Update drain_pending_debug_card_entries to handle the Err
result from take_active_debug_card_entries without calling expect or panicking;
return immediately for UnexpectedTop while preserving the existing None return
and draining behavior for valid pending entries.
---
Nitpick comments:
In `@crates/engine/src/game/engine_debug.rs`:
- Around line 1542-1565: Add a unit test alongside the existing debug creation
tests for `create_debug_cards` with `count: 0`, using a minimal
`DebugCardCreateRequest`. Assert that the operation produces no events, creates
no objects, and leaves `state.active_debug_card_entries()` as `None`, confirming
the early return occurs without materialization or frame setup.
🪄 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: bd470c91-7c6b-4eaa-ac19-809e047635b2
📒 Files selected for processing (7)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/log.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/types/events.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/log.rs
- crates/engine/src/types/events.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/game/targeting.rs
- crates/engine/src/game/trigger_matchers.rs
| pub(crate) fn drain_pending_debug_card_entries(state: &mut GameState, events: &mut Vec<GameEvent>) { | ||
| let Some(pending) = state | ||
| .take_active_debug_card_entries() | ||
| .expect("debug-card resumer may consume only its active frame") | ||
| else { | ||
| return; | ||
| }; | ||
| drain_debug_card_entries(state, pending, events); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=rust -C 20 'fn take_active_debug_card_entries' crates/engine/src
rg -nP --type=rust -C 6 'fn insert_debug_card_entries_parent_at_child_boundary|fn push_debug_card_entries' crates/engine/srcRepository: phase-rs/phase
Length of output: 10333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## engine_debug snippet"
sed -n '910,945p' crates/engine/src/game/engine_debug.rs
echo
echo "## take_active_debug_card_entries call sites"
rg -n --type=rust -C 8 'take_active_debug_card_entries\(' crates/engine/src
echo
echo "## debug-card resumer usages"
rg -n --type=rust -C 10 'drain_pending_debug_card_entries|pending_debug_card_entries|DEBUG_CARD|debug-card' crates/engine/src crates/engine/tests crates/wasm || trueRepository: phase-rs/phase
Length of output: 19101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## WASM/global state panic handling"
fd -i 'wasm|state|panic|resume|debug' . -t f | sed -n '1,200p'
echo
echo "## panic hooks / set_hook usages"
rg -n --type=rust -C 8 'set_hook|panic_hook|tracing_panic|print_backtrace|tracing::set' crates || true
echo
echo "## resolution stack errors"
rg -n --type=rust -C 10 'enum ResolutionStackError|UnexpectedTop|ResolutionStackError' crates/engine/srcRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## engine-wasm panic handling and panic result return"
sed -n '480,680p' crates/engine-wasm/src/lib.rs
echo
echo "## call stack around last_drain and panic result"
rg -n --type=rust -C 12 'last_drain|drain_last|take_panicked|PANIC|RuntimeError|catch_unwind|last_panic|panic_result|panic_hook' crates/engine-wasm/src crates/server-core crates/wasm || trueRepository: phase-rs/phase
Length of output: 14173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## top wasm module panic wiring"
rg -n --type=rust -C 5 'catch_unwind|set_hook|debug!|with_state_with_panic|NOT_INITIALIZED_ERR|state_cell\.set' crates/engine-wasm/src/lib.rs | sed -n '1,240p'
echo
echo "## imports/panics modules"
rg -n --type=rust -C 3 'std::panic::|panic_hook|catch_unwind|try_unwind|debug!' crates/engine-wasm/src crates/engine/src/game/engine_debug.rs crates/engine/src/types/resolution.rs crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 5402
Handle UnexpectedTop instead of panicking in the debug-card resumer.
take_active_debug_card_entries() returns Error::UnexpectedTop when a non-debug-card frame is on top, and expect() turns that result into a panic. In WASM that traps with the state already removed from GAME_STATE, leaving later calls with NOT_INITIALIZED_ERR. Since there is no error channel and ExpectedDebugCardEntries is a structural precondition rather than user-facing validation, return instead of panicking.
🤖 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_debug.rs` around lines 931 - 939, Update
drain_pending_debug_card_entries to handle the Err result from
take_active_debug_card_entries without calling expect or panicking; return
immediately for UnexpectedTop while preserving the existing None return and
draining behavior for valid pending entries.
Source: Path instructions
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Localization