Skip to content

fix(engine): retain Sakashima's own abilities when it copies a creature (#6009)#6058

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
nghetienhiep:fix/issue-6009
Jul 17, 2026
Merged

fix(engine): retain Sakashima's own abilities when it copies a creature (#6009)#6058
matthewevans merged 3 commits into
phase-rs:mainfrom
nghetienhiep:fix/issue-6009

Conversation

@nghetienhiep

Copy link
Copy Markdown
Contributor

fix(engine): retain Sakashima's own abilities when it copies a creature (#6009)

Sakashima of a Thousand Faces reads "except it has Sakashima's other
abilities" — a CR 707.9a exception that keeps ALL of Sakashima's own
printed abilities (the legend-rule exemption static, Partner) on the
copy, not just a single indexed ability. The existing "except it has
this ability" handling only retains the one ability containing the
BecomeCopy effect, so Sakashima's legend-rule exemption and Partner
were silently dropped whenever it copied a creature.

Added RetainAllOtherAbilitiesFromSource, an unbounded sibling of
RetainPrintedTriggerFromSource/RetainPrintedAbilityFromSource that
pulls the source's entire activated/triggered/static/keyword surface
into the copy's Layer 1 copiable values instead of a single index.

Closes #6009

…re (phase-rs#6009)

Sakashima of a Thousand Faces reads "except it has Sakashima's other
abilities" — a CR 707.9a exception that keeps ALL of Sakashima's own
printed abilities (the legend-rule exemption static, Partner) on the
copy, not just a single indexed ability. The existing "except it has
this ability" handling only retains the one ability containing the
BecomeCopy effect, so Sakashima's legend-rule exemption and Partner
were silently dropped whenever it copied a creature.

Added RetainAllOtherAbilitiesFromSource, an unbounded sibling of
RetainPrintedTriggerFromSource/RetainPrintedAbilityFromSource that
pulls the source's entire activated/triggered/static/keyword surface
into the copy's Layer 1 copiable values instead of a single index.

Closes phase-rs#6009

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements support for ContinuousModification::RetainAllOtherAbilitiesFromSource to correctly handle CR 707.9a (e.g., Sakashima of a Thousand Faces), ensuring that the entire ability surface of the source is retained on the copy. Feedback on the changes suggests avoiding cloning the retained_other_abilities tuple inside the hot path loop over affected_ids in layers.rs to prevent unnecessary allocations, recommending borrowing the tuple instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +5736 to +5760
if let Some((abilities, triggers, statics, keywords)) =
retained_other_abilities.clone()
{
let obj_abilities = Arc::make_mut(&mut obj.abilities);
for ability in abilities.iter() {
if !obj_abilities.contains(ability) {
obj_abilities.push(ability.clone());
}
}
for trigger in triggers.iter() {
if !obj.trigger_definitions.iter_all().any(|t| t == trigger) {
obj.trigger_definitions.push(trigger.clone());
}
}
for static_def in statics.iter() {
if !obj.static_definitions.iter_all().any(|s| s == static_def) {
obj.static_definitions.push(static_def.clone());
}
}
for keyword in keywords {
if !obj.keywords.contains(&keyword) {
obj.keywords.push(keyword);
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Avoid cloning the entire retained_other_abilities tuple of vectors inside the loop over affected_ids. Since layer evaluation is a hot path, cloning these vectors for every affected object introduces unnecessary allocations. Instead, borrow the tuple and clone the individual elements only when they are actually pushed.

                if let Some((abilities, triggers, statics, keywords)) = &retained_other_abilities {
                    let obj_abilities = Arc::make_mut(&mut obj.abilities);
                    for ability in abilities.iter() {
                        if !obj_abilities.contains(ability) {
                            obj_abilities.push(ability.clone());
                        }
                    }
                    for trigger in triggers.iter() {
                        if !obj.trigger_definitions.iter_all().any(|t| t == trigger) {
                            obj.trigger_definitions.push(trigger.clone());
                        }
                    }
                    for static_def in statics.iter() {
                        if !obj.static_definitions.iter_all().any(|s| s == static_def) {
                            obj.static_definitions.push(static_def.clone());
                        }
                    }
                    for keyword in keywords {
                        if !obj.keywords.contains(keyword) {
                            obj.keywords.push(keyword.clone());
                        }
                    }
                }

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans matthewevans self-assigned this Jul 17, 2026
Co-authored-by: Hịp <13849419+nghetienhiep@users.noreply.github.com>
@matthewevans

Copy link
Copy Markdown
Member

Maintainer fixup applied; hold for one production regression.

I pushed dee203eabe to complete the new ContinuousModification match in phase-ai (the previous head did not compile) and to remove the per-affected-object clone in the Layer 1 loop identified in the review thread.

Before this can be approved, please add a regression that drives Sakashima's actual parsed as-enters replacement through the entry/choice pipeline and proves that selecting a copy preserves both Partner and the controller-scoped legend-rule exemption. The current tests validate parser shape and a hand-built ResolvedAbility, but do not exercise the production replacement-enter path.

Recommendation: add that pipeline regression, then request re-review on the resulting head.

@matthewevans matthewevans added the bug Bug fix label Jul 17, 2026
@matthewevans matthewevans removed their assignment Jul 17, 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.

Request changes — the current implementation still lacks a production-path regression.

The maintainer fixup restores the exhaustive AI match and removes the Layer 1 clone, but the PR still only proves constructed ability data. Please add a scenario using parsed Sakashima Oracle text that drives the replacement-enter/copy pipeline and verifies all retained source abilities through Layer 1. Until that test reaches the production path, this class remains unproven.

…lities (phase-rs#6009)

Addresses maintainer feedback on PR phase-rs#6058: the existing coverage only
validated parser shape and a hand-built ResolvedAbility. Adds Sakashima of a
Thousand Faces to the parser-backed MTGJSON test fixture (alongside
Witherbloom Apprentice) so its real Oracle text is parsed by the production
parser, then drives the actual cast/replacement-choice/copy-target-choice
pipeline to prove Layer 1's RetainAllOtherAbilitiesFromSource retains Partner
and the controller-scoped legend-rule exemption on the copy — and that the
exemption functionally suppresses CR 704.5j's legend-rule prompt for another
of the controller's legendary permanents.
@matthewevans matthewevans self-assigned this Jul 17, 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.

Approved: the current head exercises the parsed replacement-enter pipeline, retains the relevant source ability surface at Layer 1, and preserves that behavior in copiable values.

@matthewevans matthewevans added enhancement New feature or request and removed bug Bug fix labels Jul 17, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 17, 2026
@matthewevans matthewevans removed their assignment Jul 17, 2026
Merged via the queue into phase-rs:main with commit 311a0b4 Jul 17, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sakashima of a Thousand Faces bug — When Sakashima copies a creature, it doesn't retain its other abilities.

2 participants