Skip to content

Fix Orvar copying itself instead of targeted permanent (fixes #4830) - #4852

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
kiannidev:fix/4830-orvar-copy-target
Jul 2, 2026
Merged

Fix Orvar copying itself instead of targeted permanent (fixes #4830)#4852
matthewevans merged 3 commits into
phase-rs:mainfrom
kiannidev:fix/4830-orvar-copy-target

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

  • Resolve ParentTarget on spell-cast triggers from the triggering spell's committed object targets.
  • Fixes Orvar creating a copy of itself when the cast spell targeted another permanent you control (e.g. a land).

Test plan

  • cargo test -p engine --test integration orvar
  • New integration test: land-targeting instant creates a token copy of the land, not Orvar

Made with Cursor

Resolve ParentTarget on spell-cast triggers from the triggering spell's
committed object targets so copying targets a land (or other permanent)
instead of defaulting to Orvar itself.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev
kiannidev requested a review from matthewevans as a code owner July 2, 2026 02:16
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

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

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying the narrower Orvar fix. I’m requesting changes on the current head.

  1. The new regression test does not discriminate the bug it claims to cover. orvar_copies_targeted_land_not_itself only asserts that permanent count increased and the original Orvar object still exists. That would still pass if the trigger created a token copy of Orvar instead of the targeted land. Please assert the copied object identity directly, e.g. that there are two Forest/target-land copies or one additional land-copy token, and not a second Orvar.

  2. The Rust lint workflow is failing. The diff also adds a function-local use crate::types::events::GameEvent; in targeting.rs; imports need to live at module scope in this repo.

@matthewevans matthewevans added the bug Bug fix label Jul 2, 2026
kiannidev and others added 2 commits July 2, 2026 04:52
Move GameEvent import to module scope, alphabetize the integration test
module, and strengthen the regression test to assert a land token copy
rather than only a permanent count increase.

Co-authored-by: Cursor <cursoragent@cursor.com>
# Conflicts:
#	crates/engine/tests/integration/main.rs
@matthewevans matthewevans self-assigned this Jul 2, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updated head after the requested changes. The Orvar fix now binds ParentTarget from the triggering spell’s committed object targets, and the regression discriminates the bug by proving no Orvar token is created while exactly one land token copy is created. I also resolved the current main.rs integration-test mod-line conflict and verified formatting/parser-gate during the merge-forward commit; CI can finish under merge-when-ready.

@matthewevans
matthewevans enabled auto-merge July 2, 2026 03:04
@matthewevans matthewevans removed their assignment Jul 2, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 2, 2026
Merged via the queue into phase-rs:main with commit 8e2f7cf Jul 2, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants