fix(engine): Gift recipient selection for Peerless Recycling (#5981)#6570
fix(engine): Gift recipient selection for Peerless Recycling (#5981)#6570claytonlin1110 wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
🚧 Files skipped from review as they are similar to previous changes (37)
📝 WalkthroughWalkthroughGift costs now carry Gift-specific metadata, prompt for a recipient when needed, persist that choice through casting, and deliver effects to the selected opponent. The client, AI, interaction protocols, compatibility layer, scenario drivers, and integration tests support the new waiting state and action. ChangesGift recipient flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant GiftRecipientModal
participant GameEngine
participant CastingCosts
participant GiftDelivery
Player->>GiftRecipientModal: Select opponent
GiftRecipientModal->>GameEngine: ChooseGiftRecipient(opponent)
GiftRecipientModal->>GameEngine: DecideOptionalCost(pay)
GameEngine->>CastingCosts: Validate and resume pending cast
CastingCosts->>GiftDelivery: Preserve selected gift_recipient
GiftDelivery-->>Player: Deliver Gift effect to selected opponent
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: 3
🧹 Nitpick comments (2)
crates/engine/src/game/effects/gift_delivery.rs (1)
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the object-stamped fallback and the no-recipient/eliminated-recipient no-op.
resolve_gift_recipienthas two branches that no test exercises: (1) falling back toobj.gift_recipientwhenability.context.gift_recipientisNone(the path a permanent's post-ETB triggered Gift ability would take), and (2) the no-op when the candidate isn't a current, non-eliminated player.make_gift_abilityalways populatesability.context.gift_recipientdirectly, so a regression in either branch (e.g. the object stamp never being read, or delivering to an eliminated player) wouldn't be caught.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/gift_delivery.rs` around lines 99 - 113, Add tests covering resolve_gift_recipient: verify it falls back to the source object’s gift_recipient when ability.context.gift_recipient is None, and verify delivery is a no-op when no recipient exists or the candidate player is eliminated. Avoid relying on make_gift_ability for the fallback case; construct the ability and object state so each branch is exercised.crates/engine/tests/integration/peerless_recycling_gift_recipient.rs (1)
28-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen reach-guards for the decline and 2-player auto-select paths.
Both tests drive through the fluent
SpellCastbuilder, which hides intermediateWaitingFortransitions:
- Test 1 (
peerless_recycling_decline_returns_one_and_no_gift_draw) never independently provesOptionalCostChoice{origin: Gift}was actually raised before being declined — if Gift parsing silently broke, this test would still pass.- Test 2 (
peerless_recycling_promise_returns_two_and_opponent_draws) never proves the 2-player game skipsChooseGiftRecipient— the fluent driver (seescenario.rs) auto-answers that prompt too, so a regression that wrongly raises it in 2p wouldn't be caught here.Switching these two to the manual
waiting_formatch loop used by tests 3/4 (asserting theorigin/absence ofChooseGiftRecipient) would close both gaps.As per path instructions, "For every negative assertion... require a paired positive reach-guard proving the input actually reached the code under test."
🤖 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/tests/integration/peerless_recycling_gift_recipient.rs` around lines 28 - 94, The tests peerless_recycling_decline_returns_one_and_no_gift_draw and peerless_recycling_promise_returns_two_and_opponent_draws do not verify the expected WaitingFor transitions because SpellCast auto-answers them. Replace their fluent resolution with the manual waiting_for match flow used by the later tests: assert the decline path reaches OptionalCostChoice with origin Gift before declining, and assert the two-player promise path does not raise ChooseGiftRecipient before continuing. Preserve the existing zone and draw assertions.
🤖 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/modal/GiftRecipientModal.tsx`:
- Around line 28-32: Remove the client-side sorting and seatOrder lookup from
GiftRecipientModal, rendering waitingFor.data.candidates in the order received.
Update the engine logic that produces the candidates so it emits them in
authoritative seat order, including a defined placement for candidates absent
from seatOrder, and preserve that ordering through the client without further
derivation.
In `@client/src/i18n/locales/it/game.json`:
- Around line 1713-1730: In client/src/i18n/locales/it/game.json lines
1713-1730, client/src/i18n/locales/pl/game.json lines 1713-1730, and
client/src/i18n/locales/pt/game.json lines 1713-1730, remove the duplicated `},
"clashOpponent": {` insertion so each file has one valid `clashOpponent` key,
then validate all three files as JSON. Also translate the `giftRecipient` and
`optionalCost.gift` values into each locale.
In `@crates/phase-ai/src/search.rs`:
- Around line 1218-1222: The ChooseGiftRecipient fallback in the
action-selection flow is unreachable because has_pending_cast() handles this
state first. Update the pending-cast guard and related ChooseGiftRecipient
handling so gift-promised spells reach the candidates.first() fallback and
choose the first recipient, while preserving existing CancelCast behavior for
other pending-cast states.
---
Nitpick comments:
In `@crates/engine/src/game/effects/gift_delivery.rs`:
- Around line 99-113: Add tests covering resolve_gift_recipient: verify it falls
back to the source object’s gift_recipient when ability.context.gift_recipient
is None, and verify delivery is a no-op when no recipient exists or the
candidate player is eliminated. Avoid relying on make_gift_ability for the
fallback case; construct the ability and object state so each branch is
exercised.
In `@crates/engine/tests/integration/peerless_recycling_gift_recipient.rs`:
- Around line 28-94: The tests
peerless_recycling_decline_returns_one_and_no_gift_draw and
peerless_recycling_promise_returns_two_and_opponent_draws do not verify the
expected WaitingFor transitions because SpellCast auto-answers them. Replace
their fluent resolution with the manual waiting_for match flow used by the later
tests: assert the decline path reaches OptionalCostChoice with origin Gift
before declining, and assert the two-player promise path does not raise
ChooseGiftRecipient before continuing. Preserve the existing zone and draw
assertions.
🪄 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: c54f0d84-af35-4870-a7e1-f42e91f7820a
📒 Files selected for processing (38)
client/src/adapter/types.tsclient/src/components/modal/GiftRecipientModal.tsxclient/src/components/modal/OptionalCostModal.tsxclient/src/components/modal/__tests__/OptionalCostModal.test.tsxclient/src/game/waitingForRegistry.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/pages/GamePage.tsxcrates/engine/src/ai_support/candidates.rscrates/engine/src/database/synthesis.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/effects/gift_delivery.rscrates/engine/src/game/engine.rscrates/engine/src/game/game_object.rscrates/engine/src/game/interaction.rscrates/engine/src/game/scenario.rscrates/engine/src/game/stack.rscrates/engine/src/types/ability.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/engine/tests/integration/gift_delivery_draw_sequence_migration.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/peerless_recycling_gift_recipient.rscrates/engine/tests/integration/s07_coiling_rebirth_full_cast.rscrates/engine/tests/integration/s07_malamet_castpath.rscrates/manabrew-compat/src/lib.rscrates/phase-ai/src/decision_kind.rscrates/phase-ai/src/search.rscrates/server-core/src/game_action_payload_guard.rs
| const candidates = [...waitingFor.data.candidates].sort((a, b) => { | ||
| const aIdx = seatOrder?.indexOf(a) ?? a; | ||
| const bIdx = seatOrder?.indexOf(b) ?? b; | ||
| return aIdx - bIdx; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Move recipient ordering into the engine.
This sorts engine-provided candidates in the client, violating the display-only boundary; a candidate absent from seatOrder also receives -1 and is promoted. Emit candidates in authoritative seat order from the engine and render that order unchanged.
As per path instructions, client/src/** “must not calculate, filter, derive, or reinterpret game data.”
🤖 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/modal/GiftRecipientModal.tsx` around lines 28 - 32,
Remove the client-side sorting and seatOrder lookup from GiftRecipientModal,
rendering waitingFor.data.candidates in the order received. Update the engine
logic that produces the candidates so it emits them in authoritative seat order,
including a defined placement for candidates absent from seatOrder, and preserve
that ordering through the client without further derivation.
Sources: Coding guidelines, Path instructions
| // CR 702.174a: Gift recipient — fall back to the first candidate. | ||
| WaitingFor::ChooseGiftRecipient { candidates, .. } => candidates | ||
| .first() | ||
| .map(|&opponent| GameAction::ChooseGiftRecipient { opponent }), | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'fn has_pending_cast' -A 60 crates/engine/src/types/game_state.rs
rg -n 'ChooseGiftRecipient|ChooseAnnouncingOpponent|ClashChooseOpponent' crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 8332
🏁 Script executed:
#!/bin/bash
sed -n '1210,1240p' crates/phase-ai/src/search.rs
printf '\n---\n'
sed -n '9768,9818p' crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 4514
ChooseGiftRecipient fallback is unreachable
ChooseGiftRecipient is already treated as a pending-cast state, so the earlier has_pending_cast() bailout returns CancelCast before this branch. Gift-promised spells get dropped instead of choosing the first recipient; remove this arm or exempt this variant from the cast guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/phase-ai/src/search.rs` around lines 1218 - 1222, The
ChooseGiftRecipient fallback in the action-selection flow is unreachable because
has_pending_cast() handles this state first. Update the pending-cast guard and
related ChooseGiftRecipient handling so gift-promised spells reach the
candidates.first() fallback and choose the first recipient, while preserving
existing CancelCast behavior for other pending-cast states.
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Tip For best results, initiate chat on the files or code changes.
You’re right on both points concerning my earlier findings:
The remaining engineering follow-ups you identify—rendering engine-provided candidate order as-is, completing non-English translations, and adding an end-to-end permanent Gift ETB coverage path—are well scoped. The proof package and coverage parse-diff should be completed before another review round, while the architecture-scope label is correctly left to maintainers. 🐇✅ ✏️ Learnings added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/game_object.rs (1)
663-667: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not keep
gift_recipienton reusableGameObjectstate.
reset_for_battlefield_exitleaves this latch intact, so after CR 400.7 a laterGiftDeliverycan read a stale recipient from a prior incarnation. Capture it in the ability context/LKI and clear the object field on battlefield exit.🤖 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/game_object.rs` around lines 663 - 667, The gift recipient must not persist on reusable GameObject state across battlefield exits. Remove or relocate GameObject::gift_recipient into the ability context/LKI used by GiftDelivery, and update consumers to read that captured value; ensure reset_for_battlefield_exit clears any object-level recipient state so later incarnations cannot reuse a stale recipient.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/engine/src/game/game_object.rs`:
- Around line 663-667: The gift recipient must not persist on reusable
GameObject state across battlefield exits. Remove or relocate
GameObject::gift_recipient into the ability context/LKI used by GiftDelivery,
and update consumers to read that captured value; ensure
reset_for_battlefield_exit clears any object-level recipient state so later
incarnations cannot reuse a stale recipient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e853649a-263f-4965-a228-a274e7b401b7
📒 Files selected for processing (9)
client/src/components/modal/GiftRecipientModal.tsxclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsoncrates/engine/src/game/casting_costs.rscrates/engine/src/game/game_object.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- client/src/i18n/locales/es/game.json
- client/src/i18n/locales/pt/game.json
- client/src/i18n/locales/de/game.json
- client/src/i18n/locales/pl/game.json
- client/src/i18n/locales/it/game.json
- crates/engine/src/game/casting_costs.rs
|
Re-reviewed at 🔴
|
| Item | Status |
|---|---|
| Gift cost paid on one seat path, dropped on the other | Resolved — both branches drop uniformly and converge on finish_pending_cost_or_cast, with a CR-grounded comment on why the zero sentinel isn't a payment (casting_costs.rs:636-643, 645-650) |
Hardcoded SpellCostSource::Other |
Resolved, with a caveat below |
| Frontend sorts engine data | Resolved — sort, seatOrder prop and store subscription all removed, with a comment recording that order is engine-owned. I confirmed the engine genuinely emits seat order: players::opponents iterates state.seat_order (players.rs:160-167) |
gift_kind_for_object last-Gift vs single latch |
Resolved — doc comment records that printed cards carry at most one Gift and the per-ordinal queue is for uniformity |
| i18n English in six locales | Resolved — real translations in de/es/fr/it/pl/pt (de: "Geschenk-Empfänger wählen"; fr: "Choisir le destinataire du cadeau"), and the }, "clashOpponent" formatting glitch went with it |
| Proof package | Filled in — template sections, Gate A and Final review-impl both stamped head=637bd013d, method /engine-implementer, 3 anchors |
| Permanent Gift ETB (CR 702.174b) unexercised | Not addressed — carried forward below |
The delta bc3114a5..637bd013d is linear (no force-push): one polish commit plus a merge of main that picked up #6568/#6574. Contributor-touched files are casting_costs.rs, GiftRecipientModal.tsx, and the six locale files. No regressions in the delta.
On the cost-drop specifically — the removed call routed AbilityCost::Mana{zero} through continue_after_declared_mana_split (casting_costs.rs:6298-6304). The new path is the one the ≥2-opponent branch already used, so this converges the 2-player path onto the tested route rather than away from it, and peerless_recycling_promise_returns_two_and_opponent_draws covers it. The promise record is unaffected — record_additional_cost_instance_payment happens upstream in handle_decide_additional_cost, before updated_pending is built.
🔴 Blocking on evidence, not code
The engine tests have not run on this head. Rust tests (shard 1/2), Rust tests (shard 2/2), Rust lint (fmt, clippy, parser gate), WASM compile check and Frontend (lint, type-check, test) are all still queued. Your Test-plan boxes 1 and 2 are honestly marked _pending — local host has no MSVC SDK_, which means CI is the only channel that has ever executed this code, and it hasn't yet. Boxes 3 and 4 are checked with real vitest output, so this isn't a diligence problem — it's a toolchain one.
The parse-diff sticky is absent, so claimed-vs-measured card impact can't be confronted. That's timing rather than omission: .github/workflows/coverage-parse-diff-comment.yml:18-21 fires on workflow_run after CI completes.
The branch is behind main by one commit. Bringing it current re-triggers CI and regenerates the sticky — one action clears all three.
One reason I want the full suite here specifically, rather than as routine box-ticking: casting.rs widens the pre-target-declaration gate beyond Gift. requires_additional_cost_declaration_before_targets (:12227-12245) now walks the whole sub_ability chain instead of level 1, plus a new ability_chain_has_gift_delivery disjunct — and the comment this PR deleted explicitly documented that the old level-1 check "already returns false for Gift." So any card with AdditionalCostPaidInstead at depth ≥ 2 and a non-empty effective queue now defers targets where it previously didn't. The same widening is mirrored into legal_target_slots_for_castable_spell_in_flushed_state (:12533-12545), the AI/preview path — so an AI-visible legal-action change rides along, which is why both AI gates matter here too. I couldn't construct a counterexample and I believe the widening is CR-correct, but that isn't something reading can settle, and the three existing Gift cast-path tests were amended (only adding a ChooseGiftRecipient arm) rather than extended to a non-Gift nested case.
🟡 Non-blocking
resolve_gift_recipient hand-rolls players::is_alive. effects/gift_delivery.rs:107-111 is a byte-identical predicate to players.rs:11-16, and the PR removed the players import to write it inline. The candidate list is built by players::opponents using is_alive, so the delivery-time check can now drift from the offer-time one. players::is_alive(state, candidate).then_some(candidate) restores the single authority.
cost_source is threaded in only to be discarded — casting_costs.rs:628 adds the param, :643 does let _ = (cost_to_pay, cost_source);. Dead plumbing whose only effect is suppressing unused_variables. Since it was added in direct response to my last round, keeping the comment and dropping the param is equally fine — your call.
Empty-opponents path errors mid-cast rather than never offering. casting_costs.rs:635-639 returns EngineError::InvalidAction after the player has already accepted the promise, with state.pending_cast published and no matching WaitingFor. Requires every opponent to be eliminated between the prompt being raised and the decision applying, so SBAs would normally have ended the game first — but gating offerability on a non-empty opponents list is strictly better than erroring after the fact.
Permanent Gift ETB reader still untested (CR 702.174b). The .or_else() branch reading GameObject.gift_recipient at gift_delivery.rs:101-106 is unexercised; the unit helper pre-stamps ability.context.gift_recipient (:202-205) so unit tests structurally can't discriminate a dropped cast-path stamp; and the None arm returns Ok(()) silently (:39-42). A dropped stamp therefore fails silently — that combination is why I'm raising it a second time.
✅ Verified — please don't re-litigate these
- Oracle text re-verified against the live Scryfall API, not the PR description: Peerless Recycling,
{1}{G}instant, Bloomburrow, keywords['Gift'], text matching thePEERLESS_RECYCLINGtest constant verbatim. - Every CR in the diff grep-verified against
docs/MagicCompRules.txt: 702.174a (:5285), 702.174b (:5287), 702.174e (:5293), 702.174j (:5303), 702.174m (:5307), 601.2b (:2459), 601.2c (:2461), 702.194c (:5433), 800.4b (:6414), 609.3 (:2847), 800.4 (:6406), 115.1 (:838). All resolve and all describe their code. Notably CR 702.174a states the Gift ability is "As an additional cost to cast this spell, you may choose an opponent" — so modelling recipient choice as an additional-cost step is CR-exact rather than an approximation. - Sequencing nuance, not a defect: strictly the promise is 601.2b and choosing the opponent is 601.2f–h, i.e. after targets at 601.2c. The PR collapses both into 601.2b. Unobservable — no player acts in between and the recipient doesn't affect target legality — and collapsing is what lets 702.174m target-gating be answered in one pass. Worth a one-line comment if you want the deliberate collapse on record.
originmislabel onOptionalCostChoice: refuted.make_optional_cost_choicederivesoriginfrom the queue head (casting_costs.rs:329-333), andfinish_pending_cost_or_castdrains the queue before reaching the Repeatable/Kicker branches, so a Kicker prompt can't inherit Gift copy.- Double Gift prompt from the legacy
face.additional_cost: refuted. Thesynthesis.rs:1420-1426extraction into a sharedgift_additional_cost()is what letsobj_additional_matches_instanceshape-dedup the legacy face copy against the queue instance. - No new multiplayer leak.
visibility.rsredacts only hidden-info variants;ChooseGiftRecipientcarries publiccandidates+pending_cast, same shape as the unredactedChooseAnnouncingOpponent.handle_choose_gift_recipientvalidatescandidates.contains(&opponent)(casting_costs.rs:667-671). - CodeRabbit: both live findings settled — the frontend re-sort is gone, and it withdrew the locale-JSON and dead-arm objections in its 23:02 reply.
📋 Gate status, for the record
The architecture-scope gate is triggered and issue #5981 still lacks accepted. Repo policy mode is review, not enforce — so that is recorded for maintainer attention and is explicitly not grounds to decline or close. This PR gets reviewed on its merits, which is what I've done.
Recommendation: hold enqueue, we'll bring the branch current. Once all Rust/frontend/WASM checks are green, both AI gates are green, and the parse-diff sticky posts matching the claimed single-card scope, this is approve-and-enqueue on the merits. The four non-blocking items above are polish and none should hold it — though players::is_alive is a one-liner worth folding in while you wait.
|
Closing this PR, along with the other open PRs from this account, as a maintainer decision about contribution process rather than about any particular line of code here. The pattern across these submissions has been automated generation with our CI and review feedback acting as the correction loop — changes pushed before they are understood, then iterated against whatever comes back red. The PR histories show it directly: reverting a fix and pushing a That model doesn't work for this repository. Review capacity here is the scarce resource, and it is spent on rules-correctness questions that require someone to have reasoned about the Comprehensive Rules and the engine's layer architecture before pushing. When the correction loop is outsourced to maintainer review, each round consumes that capacity without transferring understanding, and the same class of defect returns on the next PR. To be clear about what this is not: it is not a verdict that the code in this PR is wrong. Some of this work was genuinely useful and several of these changes landed. Nor is it a judgement about using AI tooling — plenty of good contributions here are AI-assisted. The distinction is whether the author has verified and understood the change before it reaches review. If you want to contribute here again, the bar is a change you can explain end to end: which Comprehensive Rules section it implements, why that seam is the correct one, and what a test would have to do to fail if the change were reverted. Open an issue describing that first and we'll talk before any code is written. Thank you for the work that did land. |
github-actions
Bot
added
the
needs-maintainer
|
Reopening this. The closure was a maintainer error, and the reasoning I posted was wrong on the facts. I closed this PR yesterday as part of a batch action against this account. That was my call and it was not correct. Here is the specific thing I got wrong, because you are owed the detail rather than a vague retraction. The trigger was
So the model use was compliant with the rules as published at the time, and I applied a rule retroactively that did not exist when you wrote the code. The grandfather clause now in I also want to separate two things I ran together in the closure comment. The behavioural concerns I raised — reverting a fix to push a diagnostic commit so CI prints a value, and removing a passing assertion under a commit message that did not mention it — are real observations and I stand behind raising them. They are review findings about specific changes. They were not grounds for a batch closure of unrelated PRs, and treating them as evidence of bad faith rather than as things to fix in review was the error. What happens now: this PR is reopened at its original head with its review history intact. It gets processed on its merits like any other. Where it already had an approval, that approval stands and it goes back toward the queue; where it had open findings, those findings are still open and unchanged. Two things worth knowing going forward:
Thanks for the work, and sorry for the disruption — you had a PR approved and in the merge queue when I closed it. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
This one doesn't compile yet, so it can't be correctness-reviewed until it's green. The blocker is small and local. This is an ordinary technical finding, unrelated to account standing.
The failing check
Rust lint (fmt, clippy, parser gate) fails to build phase-ai, which also reds both test shards:
error[E0063]: missing fields `gift_kind` and `origin` in initializer of `engine::types::WaitingFor`
--> crates/phase-ai/src/search.rs:5667:29
This PR extends WaitingFor with gift_kind and origin for gift-recipient selection, but the construction site at crates/phase-ai/src/search.rs:5667 still builds the old shape. When you add a field to a shared type, every literal that constructs it has to be updated — this one was missed, so the whole phase-ai crate fails to compile.
What's needed
- Rebase onto current
mainfirst (the branch isBEHIND). - Complete the
WaitingFor { .. }literal atsearch.rs:5667with the correctgift_kind/originfor that search context. (AgrepforWaitingFor {across the workspace will surface any other call sites the same extension touched — worth double-checking there isn't a second one.) - Push; CI + review will pick it back up.
Promise Gift as AdditionalCostOrigin::Gift with optional ChooseGiftRecipient when multiple opponents exist, stamp gift_recipient through cast finalize, and fix Peerless nested Instead target slots via set_context_recursive. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a non-Peerless 3p Gift delivery test that chooses P2 (not seat-next), and route Gift optional-cost chrome through t() using optionalCost.gift keys. Co-authored-by: Cursor <cursoragent@cursor.com>
…e-rs#5981) Add giftRecipient/optionalCost.gift keys to de/fr/es/it/pl/pt (en fallback) and assert Gift OptionalCostChoice copy via OptionalCostModal test. Co-authored-by: Cursor <cursoragent@cursor.com>
…5981) CI failed E0063 on the GameObject struct literal and partition destructure after adding the Gift recipient stamp field. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the zero Gift sentinel uniformly on both recipient paths, render engine candidate order without client re-sort, and translate Gift UI strings in all locales. Co-authored-by: Cursor <cursoragent@cursor.com>
Scenario oracle inference cannot FromStr a "Gift a card (reminder…)" line. Pass the MTGJSON-style "Gift" hint so the non-Peerless 3p delivery test actually exercises ChooseGiftRecipient + latched recipient draw. Co-authored-by: Cursor <cursoragent@cursor.com>
…-rs#5981) The WaitingFor::OptionalCostChoice literal in the multikicker search fixture still used the pre-Gift shape; add Kicker origin and gift_kind: None so phase-ai compiles against the extended variant. Co-authored-by: Cursor <cursoragent@cursor.com>
1892231 to
fd4f133
Compare
2 participants
Summary
Latch the Gift additional-cost recipient on
SpellContext/GameObject(AdditionalCostOrigin::Gift) instead of hardcodingplayers::next_player. PromptChooseGiftRecipientwhen ≥2 opponents promise a gift; auto-assign the sole opponent in 2-player. Declare Gift before targets (queue + chain-walk +set_context_recursive) so Peerless Recycling’s gift-promised branch offers two graveyard targets. Frontend: Gift promise copy onOptionalCostChoice+GiftRecipientModal; AI/scenario/session wiring included.Fixes #5981
Files changed
crates/engine/src/types/ability.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/gift_delivery.rscrates/engine/src/game/engine.rscrates/engine/src/game/game_object.rscrates/engine/src/game/interaction.rscrates/engine/src/game/scenario.rscrates/engine/src/game/stack.rscrates/engine/src/database/synthesis.rscrates/engine/src/ai_support/candidates.rscrates/engine/tests/integration/peerless_recycling_gift_recipient.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/s07_coiling_rebirth_full_cast.rscrates/engine/tests/integration/s07_malamet_castpath.rscrates/engine/tests/integration/gift_delivery_draw_sequence_migration.rscrates/phase-ai/src/decision_kind.rscrates/phase-ai/src/search.rscrates/server-core/src/game_action_payload_guard.rscrates/manabrew-compat/src/lib.rsclient/src/adapter/types.tsclient/src/components/modal/GiftRecipientModal.tsxclient/src/components/modal/OptionalCostModal.tsxclient/src/components/modal/__tests__/OptionalCostModal.test.tsxclient/src/game/waitingForRegistry.tsclient/src/pages/GamePage.tsxclient/src/i18n/locales/{en,de,fr,es,it,pl,pt}/game.jsonCR references
ChooseAnnouncingOpponent)Implementation method (required)
Method: /engine-implementer
Track
Developer — MSVC Build Tools installed mid-run; engine + frontend tests executed locally on head
d34e6db59.LLM
Model: composer-2
Thinking: high
Tier: Standard
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
./scripts/check-parser-combinators.sh upstream/main— Gate A PASS (see ## Gate A)cargo test -p engine --test integration peerless_recycling_gift_recipient -- --nocapture—ok. 4 passedcargo test -p engine --test integration -- --nocapture s07_coiling_rebirth_full_cast—ok. 1 passedcargo test -p engine --test integration -- --nocapture s07_malamet_castpath—ok. 7 passedcargo test -p engine --test integration -- --nocapture gift_delivery_draw_sequence_migration—ok. 2 passedpnpm exec vitest run …/OptionalCostModal.test.tsx …/AnnouncingOpponentModal.test.tsx …/ClashOpponentModal.test.tsx—Test Files 3 passed; Tests 7 passedGate A
Anchored on
crates/engine/src/game/casting_costs.rs:1489—WaitingFor::ChooseAnnouncingOpponentraise after ≥2 opponents (same interactive cast-time opponent-choice seam)crates/engine/src/game/casting_costs.rs:7242/:7300—effective_teamwork_additional_cost_instances/effective_bargain_additional_cost_instancesone-origin-per-keyword queue pattern mirrored byeffective_gift_additional_cost_instancescrates/engine/src/types/game_state.rs:8693— existingChooseAnnouncingOpponentWaitingFor shape thatChooseGiftRecipientmirrorsFinal review-impl
Final review-impl PASS head=d34e6db5975e34f909838fd6b01ef04c9191fd57
Maintainer review at
bc3114a5found engineering CLEAN (request-changes on proof package + yellow polish). Follow-up on this head:cost_sourcethreading comment incontinue_after_gift_promisedGiftRecipientModalrenders engine candidate order (no clientseatOrderre-sort)gift_kind_for_objectgiftRecipient/optionalCost.gifttranslations in de/fr/es/it/pl/ptGiftkeyword hint so the test reaches production Gift synthesisIn-session
/review-implof that polish + fixture fix: no new findings (subagent API limited; reviewed in-parent against review-impl skill).Claimed parse impact
Test plan
peerless_recycling_gift_recipient— decline → 1 return / no draw; promise 2p → 2 returns + opponent draws; 3p → ChooseGiftRecipient delivers to chosen seat (not seat-next)Existing Gift cast paths (
s07_coiling_rebirth_full_cast,s07_malamet_castpath,gift_delivery_draw_sequence_migration) still resolve after ChooseGiftRecipient auto-answerBargain/Teamwork pre-target and ChooseAnnouncingOpponent unchanged
ChooseAnnouncingOpponent,effective_bargain_*,effective_teamwork_*).Frontend: Gift optional cost shows “Promise a gift?”; multiplayer recipient modal dispatches
ChooseGiftRecipientGiftRecipientModalrenderswaitingFor.data.candidatesin engine order (no client re-sort) and dispatchesChooseGiftRecipient.Validation Failures
None.
CI Failures
None known on this head at PR-body update time (CI re-queued after push).
Summary by CodeRabbit