fix(engine): prompt Food/Treasure choice on Tireless Provisioner landfall

[jsdevninja]

Summary

Fixes #927: Tireless Provisioner's landfall trigger fires but the player was not prompted to choose Food or Treasure.

• Card-data loading: Accept legacy SourceIsRenowned in TriggerCondition via serde alias → IsRenowned { subject: Source }, so CardDatabase::from_export / WASM rehydrate works with current exports.
• ChooseOneOf runtime: Sync priority_player to the authorized chooser when pausing on ChooseOneOfBranch; fail loudly if no eligible chooser; derive token branch labels ("Create a Food token", etc.) when descriptions are absent.
• Frontend: Improve ChooseOneOfBranchModal labels (description capitalization, token-name fallback from branch wire data, binary-choice subtitle, focus styles).
• Tests: Six integration tests for parser, export shape, runtime prompt, both branches, and export-backed + rehydrate path; fixture updated for Tireless Provisioner.

Test plan

• cargo test -p engine choose_one_of
• cargo test -p engine --test integration issue_927
• cargo test -p engine legacy_source_is_renowned
• Manual: Tireless Provisioner on battlefield → play a land → resolve landfall → modal shows Food / Treasure before any token is created
• Manual: Choose Food → one Food token; choose Treasure → one Treasure token

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

Review: PR #2804 — Tireless Provisioner Food/Treasure landfall choice

Verdict: Approve with comments. The change correctly reuses the existing ChooseOneOf / WaitingFor::ChooseOneOfBranch / GameAction::ChooseBranch interactive primitive (not a one-off), covers the whole "create A or B token" class, and both branches are tested. But two findings below question whether the engine changes are actually load-bearing for the reported "no prompt" bug, plus a frontend display-only/i18n nit.

Correct seam / reuse (good)

The primitive (ChooseOneOf effect, ChooseOneOfBranch waiting state, ChooseBranch action, ChooseOneOfBranchModal) already existed and is reused, not re-implemented. The parser already produced ChooseOneOf for this text on main (no parser change needed), and shipped client/public/card-data.json already encodes Tireless Provisioner as ChooseOneOf. Generalizes to any "create X or Y token" card. Good.

Findings

[MED] The manual priority_player routing line is redundant with the centralized sync, and its CR citation is wrong. Evidence: crates/engine/src/game/effects/choose_one_of.rs:84-86 adds state.priority_player = turn_control::authorized_submitter_for_player(state, player); annotated CR 117.3d. But apply() already routes this centrally on every action: engine.rs:156 sync_waiting_for(state, &result.waiting_for) and engine.rs:169 finalize_public_state(state) both call sync_priority_player_from_waiting_for (public_state.rs:34-39), which maps waiting_for.acting_player() through the same authorized_submitter_for_player. WaitingFor::ChooseOneOfBranch { player, .. } is already an arm of acting_player() (game_state.rs:3945) on the base branch. So the line sets priority_player to the identical value the tail sync sets a moment later — it is idempotent dead duplication of the centralized routing authority. Separately, CR 117.3d is "a player passes priority → next player gets priority"; it does not describe routing an at-resolution choice to its chooser. The applicable rule is CR 608.2d (already cited at the top of the function). Why it matters: duplicating the central priority_player authority at one resolver invites drift and a wrong CR annotation creates false verification confidence. Suggested fix: delete the line and rely on the centralized sync; if kept for a reason I'm missing, re-cite to CR 608.2d and add a comment explaining the mid-apply read it guards.

[MED] The new engine integration tests largely re-assert behavior that already held on main, so they may not be discriminating for the reported "doesn't prompt" bug. Evidence: tests/integration/issue_927_tireless_provisioner.rs asserts waiting_for == ChooseOneOfBranch and zero tokens — but ChooseOneOf::resolve set ChooseOneOfBranch and acting_player()/central routing were already correct on base; the parse was already ChooseOneOf. The only assertion that is genuinely new behavior is branch_descriptions[i].contains("Food"/"Treasure") (the Token-labeling change at choose_one_of.rs:223-225); on base those labels were "Option 1"/"Option 2". Why it matters: the reported bug ("triggers but no prompt") is most plausibly stale shipped card-data or a frontend gating issue, and the PR's engine tests would (I believe) pass on main for everything except the label text — i.e. the tests prove the labels, not the prompt regression. Suggested fix: either confirm (and state in the PR) that these tests fail on origin/main pre-fix, or add the test that actually reproduces the user-visible failure (e.g. an assertion that the prompt is routed to / actionable by P0 via the FE-facing gate). Please verify against origin/main rather than the PR base.

[LOW] Frontend re-derives the branch label from raw effect data with a hardcoded English string — display-only + i18n violation (currently dead code). Evidence: client/src/components/modal/ChooseOneOfBranchModal.tsx:19-26,38-43 — tokenNameFromBranch() reads branch.effect.name and branchLabel() returns `Create a ${tokenName} token` not routed through t(). The engine now always populates branch_descriptions 1:1 with branches (choose_one_of.rs:73), so this fallback is unreachable in practice, but it (a) duplicates engine display logic in the frontend and (b) would render untranslated English if ever hit. Why it matters: the frontend is a display layer; per i18n/README.md frontend-authored text must use t(). Suggested fix: drop tokenNameFromBranch/the hardcoded string and fall back only to the existing t("chooseOneOfBranch.optionFallback"). Also the comment "Engine descriptions are lower-case oracle fragments" is inaccurate — the engine emits already-capitalized "Create a Food token".

Notes / refuted concerns

• i18n 7-locale parity: PASS. chooseOneOfBranch.subtitleBinary exists and is properly translated in all of de/en/es/fr/it/pl/pt — resources.test.ts parity gate is satisfied.
• IsRenowned / SourceIsRenowned serde alias (ability.rs:11293-11540): necessary companion, not gratuitous scope creep — the regenerated integration_cards.json contains the legacy SourceIsRenowned tag, which would fail to deserialize without #[serde(alias = ...)] + #[serde(default)]. CR 702.112 (Renown) verified. Worth a follow-up to stop the exporter emitting the legacy tag, but the alias is the right minimal fix here.
• CR annotations: 701.55a-b (villainous choice), 608.2d (choices at resolution), 702.112 (Renown) all verified against docs/MagicCompRules.txt. Only 117.3d is mis-applied (see MED finding above).
• empty-chooser guard (choose_one_of.rs:35-42): correct hardening (fail-loud over silent no-op), CR 608.2d justified, but note it does not reproduce the Tireless Provisioner - Not Prompting Selection — [[Tireless Provisioner]] triggers correctly, but doesn't prompt for se… #927 repro (Controller chooser is never empty in normal play).
• Both Food and Treasure branches have dedicated resolution tests. Good.

[natefinch]

Review (review-impl lenses) — second pass, building on @matthewevans.

The engine fix itself is at the correct seam and idiomatic: it reuses the existing ChooseOneOf / WaitingFor::ChooseOneOfBranch / GameAction::ChooseBranch primitive, routes priority through the established turn_control::authorized_submitter_for_player helper (same idiom as public_state.rs:36), and covers the whole "create A or B token" class, not just this card. Both branches are tested. But the generated fixture is stale, and that drift is the load-bearing problem.

Findings

[MED] crates/engine/tests/fixtures/integration_cards.json was regenerated from a stale card-data.json export, causing broad collateral drift. Evidence: vs origin/main (653 cards, 644 with metadata, renown encoded as the new IsRenowned tag), the PR head fixture has 654 cards but 0 with metadata (stripped from all 644) and re-encodes renown as the legacy SourceIsRenowned tag. Why it matters: (1) it strips metadata from 644 cards — collateral and unrelated to this fix; (2) it reintroduces the pre-#2782 SourceIsRenowned tag that main already migrated away from; (3) it is the source of mergeable: false (the single-line fixture conflicts with main). The shipped/canonical export emits IsRenowned + metadata (proven by main's fixture), so the author's local export predates both #2782 and the metadata addition. Suggested fix: rebase and re-run scripts/gen-test-fixture.py against the current client/public/card-data.json; the new fixture will carry metadata, emit IsRenowned, contain Tireless Provisioner, and resolve the merge conflict.

[MED] The #[serde(alias = "SourceIsRenowned")] + default_renown_subject_source shim (crates/engine/src/types/ability.rs) and its test (triggers.rs) exist only to make the stale fixture deserialize. Evidence: main's fixture already uses IsRenowned; the only consumer of the legacy tag is this PR's own regressed fixture. Why it matters: this is a band-aid at the wrong seam — hardening the deserializer to accept a stale artifact instead of regenerating the artifact. Regenerating the fixture (finding 1) removes the need for this scope entirely. Suggested fix: drop the alias/default/test and regenerate the fixture; only keep a back-compat alias if prod-cached card-data.json (R2) genuinely still ships the legacy tag, which main indicates it does not.

[MED] Wrong CR annotation on the priority line — confirmed. Evidence: choose_one_of.rs annotates state.priority_player = …authorized_submitter_for_player(…) with CR 117.3d, but docs/MagicCompRules.txt:954 defines 117.3d as "a player passes priority → next player in turn order receives priority" — it does not describe routing an at-resolution choice to its chooser. The applicable rule is CR 608.2d (already cited at the function top). Additionally, the line is at least partly redundant with the centralized sync: apply() → sync_waiting_for → sync_priority_player_from_waiting_for maps waiting_for.acting_player() (which has a ChooseOneOfBranch { player, .. } arm, game_state.rs:4050) through the same helper. Suggested fix: re-cite to CR 608.2d (or delete the line if the centralized post-apply sync already covers the mid-apply read; if kept, comment what mid-apply read it guards).

[LOW] Frontend re-derives the branch label with a hardcoded English string, bypassing t() (confirmed, currently dead code). Evidence: ChooseOneOfBranchModal.tsx tokenNameFromBranch() / branchLabel() return `Create a ${tokenName} token` not routed through t(). The engine now always populates branch_descriptions 1:1, so this fallback is unreachable, but it duplicates engine display logic in the display layer and would render untranslated English if hit. The comment "Engine descriptions are lower-case oracle fragments" is also inaccurate — the engine emits already-capitalized "Create a Food token". Suggested fix: drop the hardcoded path; fall back only to t("chooseOneOfBranch.optionFallback").

Notes / confirmed-and-refuted

• Empty-chooser guard: correct fail-loud hardening; CR 608.2d justified. Does not itself reproduce Tireless Provisioner - Not Prompting Selection — [[Tireless Provisioner]] triggers correctly, but doesn't prompt for se… #927 (Controller chooser is never empty in normal play).
• i18n parity: PASS — subtitleBinary present and translated in all 7 locales.
• Token-label branch_descriptions: this is the one genuinely new engine behavior the tests discriminate; the prompt/pause assertions would likely already pass on main (matthewevans' tests-not-discriminating point — confirmed for the prompt assertions, refuted for the label assertions).

VERDICT: request-changes

[matthewevans]

Maintainer note — I merged origin/main into this branch and pushed fixes resolving the review findings:

• a3c996ec8 (merge) — [MED] Stale fixture: merged current main and re-ran scripts/gen-test-fixture.py against the current canonical client/public/card-data.json. The regenerated integration_cards.json now carries metadata (650/659 cards), encodes renown exclusively as IsRenowned (0 legacy SourceIsRenowned tags), includes Tireless Provisioner, and resolves the merge conflict.
• 2da9f3b69 — [MED] Legacy alias shim: dropped #[serde(alias = "SourceIsRenowned")], the default_renown_subject_source default, and the companion test. The only consumer of the legacy tag was the stale fixture; the current export contains zero occurrences, so the deserializer hardening was dead scope at the wrong seam.
• f0a1b1599 — [MED] Wrong CR annotation / redundant priority line: deleted the manual state.priority_player = … assignment in prompt_next. Verified the centralized post-apply sync covers it: apply() always runs sync_waiting_for + finalize_public_state, both routing waiting_for.acting_player() (which has a ChooseOneOfBranch { player, .. } arm) through the same turn_control::authorized_submitter_for_player. The mis-cited CR 117.3d annotation is gone with it; the replacement comment documents the centralized ownership (CR 608.2d).
• 9e99598ca — [LOW] Frontend hardcoded English fallback: removed tokenNameFromBranch and the `Create a ${tokenName} token` string; the modal now falls back only to the translated chooseOneOfBranch.optionFallback. Corrected the inaccurate description-casing comment (first-letter capitalization kept as pure display formatting).
• 03624b3eb — [MED] Test discrimination: empirically confirmed @matthewevans' suspicion, and then some — all six issue_927 integration tests pass against the pre-fix origin/main engine (the parser already populated branch descriptions on base, so even the label assertions hold). They are now documented as regression pins. The discriminating coverage for the PR's actual engine deltas is in the choose_one_of.rs unit tests: empty_chooser_set_fails_loudly and token_branches_without_descriptions_get_create_labels both verified to FAIL against the base logic. Net effect: the engine changes in this PR are hardening + display labels; the Tireless Provisioner - Not Prompting Selection — [[Tireless Provisioner]] triggers correctly, but doesn't prompt for se… #927 "no prompt" symptom was not reproducible against current main + current card data, consistent with the stale-shipped-card-data hypothesis.

Verification: cargo fmt --all; cargo clippy -p engine --lib --tests clean; cargo test -p engine --lib choose_one_of (25 passed), --lib renowned (11 passed), --test integration issue_927 (6 passed), full --test integration suite green; pnpm type-check and pnpm lint (0 errors) for the client change.

[matthewevans]

Maintainer sign-off: ChooseOneOfBranch hardening + token-label fixes + fixture regeneration (659 cards, legacy serde shim removed); discriminating unit pair revert-verified; FE fallback i18n-correct; type-check/lint green. Caveat on record: #927's no-prompt symptom is not reproducible on current main + data (stale-shipped-card-data hypothesis documented on the PR) — if it recurs post-redeploy, reopen for FE/data investigation.

[matthewevans]

Maintainer note: rebased-by-merge to resolve the conflict with current main (15 PRs landed today). Resolution was confined to the crates/engine/tests/integration/main.rs mod list (kept both issue_924_offspring from main and this PR's issue_927_tireless_provisioner, alphabetized) and a regeneration of crates/engine/tests/fixtures/integration_cards.json via scripts/gen-test-fixture.py so the fixture is a superset covering both branches' tests (662 cards). Verified post-merge: cargo test -p engine --test integration issue_927 (6 passed), cargo test -p engine --lib choose_one_of (25 passed), and cargo check -p engine --all-targets clean; cargo fmt --all applied. Pushed as merge commit be2a50fd9 — no changes to the approved implementation itself, automerge can proceed once CI is green.

[jsdevninja]

@matthewevans Can this be merged?

[matthewevans]

@jsdevninja It would be helpful to include screenshots so we can see the before/after when it comes to frontend changes.