feat(engine): implement CR 612 text-changing effects (word replacement)#5866
feat(engine): implement CR 612 text-changing effects (word replacement)#5866real-venus wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements interactive text-changing effects (CR 612.1 and CR 612.2) across the engine, parser, AI, and frontend. It introduces the ChangeTextWords effect, the ReplaceTextWord continuous modification in Layer 3, and the TextWordReplacement waiting state, allowing players to choose word substitutions (colors, basic land types, or creature types) via a new frontend modal. The reviewer feedback suggests increasing the touch target height of the replacement buttons in the React modal to at least 44px to meet mobile accessibility guidelines, and optimizing the backend resolution logic in text_change.rs by passing references instead of cloning vectors to avoid unnecessary allocations.
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.
| {data.options.map((option, index) => ( | ||
| <motion.button | ||
| key={index} | ||
| className="rounded-lg border border-white/20 bg-white/10 px-4 py-2 text-sm text-white transition hover:bg-white/20" |
There was a problem hiding this comment.
The touch target height of the text-word replacement buttons is less than 44px (using py-2 with text-sm yields approximately 36px). To ensure mobile/touch friendliness, the touch target should be at least 44pt/px as specified in the repository style guide.
| className="rounded-lg border border-white/20 bg-white/10 px-4 py-2 text-sm text-white transition hover:bg-white/20" | |
| className="min-h-[44px] rounded-lg border border-white/20 bg-white/10 px-4 py-2 text-sm text-white transition hover:bg-white/20" |
References
- Mobile / touch: touch targets ≥ 44pt; :hover-only state breaks on mobile and needs a touch-equivalent. (link)
| let (allowed_categories, excluded_to, duration) = match &ability.effect { | ||
| Effect::ChangeTextWords { | ||
| allowed_categories, | ||
| excluded_to, | ||
| duration, | ||
| .. | ||
| } => ( | ||
| allowed_categories.clone(), | ||
| excluded_to.clone(), | ||
| duration.clone(), | ||
| ), | ||
| _ => { | ||
| return Err(EffectError::InvalidParam( | ||
| "expected ChangeTextWords effect".to_string(), | ||
| )) | ||
| } | ||
| }; | ||
|
|
||
| // CR 608.2b: the effect needs a legal object target still in its zone. | ||
| let target = ability.targets.iter().find_map(|t| match t { | ||
| TargetRef::Object(id) => Some(*id), | ||
| TargetRef::Player(_) => None, | ||
| }); | ||
| let Some(target) = target.filter(|id| state.objects.contains_key(id)) else { | ||
| events.push(resolved_event(ability)); | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let options = build_options(state, target, &allowed_categories, &excluded_to); |
There was a problem hiding this comment.
Avoid cloning allowed_categories and excluded_to vectors during resolution. Since build_options only requires references to these slices, and WaitingFor::TextWordReplacement does not store them, we can match them as references and only clone the duration option. This avoids two unnecessary vector allocations on every resolution pass.
let (allowed_categories, excluded_to, duration) = match &ability.effect {
Effect::ChangeTextWords {
allowed_categories,
excluded_to,
duration,
..
} => (
allowed_categories,
excluded_to,
duration.clone(),
),
_ => {
return Err(EffectError::InvalidParam(
"expected ChangeTextWords effect".to_string(),
))
}
};
// CR 608.2b: the effect needs a legal object target still in its zone.
let target = ability.targets.iter().find_map(|t| match t {
TargetRef::Object(id) => Some(*id),
TargetRef::Player(_) => None,
});
let Some(target) = target.filter(|id| state.objects.contains_key(id)) else {
events.push(resolved_event(ability));
return Ok(());
};
let options = build_options(state, target, allowed_categories, excluded_to);
real-venus
force-pushed
the
feat/text-changing-effects
branch
from
72d7f28 to
d122a11
Compare
Parse changes introduced by this PR · 10 card(s), 6 signature(s) (baseline: main
|
real-venus
force-pushed
the
feat/text-changing-effects
branch
from
d122a11 to
b36db57
Compare
|
@matthewevans |
|
Closed without implementation-diff review. This PR enters protected architecture scope because it touches |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Request changes
🔴 Blocker
- [HIGH] The word walker explicitly omits
AbilityDefinition.cost(andAbilityCondition), so this is not rules-complete for the class it parses. Evidence:crates/engine/src/game/text_substitution.rs:157-161says those roots are an intentional coverage gap, while CR 612.1 says a text-changing effect generally affects an object's rules text and/or type line, and CR 612.2 limits which words may change—not which rules-text fields may be skipped. A concrete supported pattern is Goblin Chirurgeon: its activation cost is “Sacrifice a Goblin”; after Artificial Evolution changes Goblin to Elf, that cost must require sacrificing an Elf. The current walker never descends fromAbilityDefinitionintocost, so it leaves that rules text unchanged while presenting this as a complete class implementation. Please add a recursiveAbilityCosttraversal (including nested target/filter-bearing costs and the relevant condition roots), and add an integration test that proves Artificial Evolution changes the legal sacrifice candidate for this activation cost. Do not silently parse cards whose affected text carriers remain unimplemented.
🟡 Non-blocking
- Gemini's two open notes are worthwhile cleanup: avoid cloning the category/exclusion vectors in
text_change::resolve, and make the new modal buttons meet the project's touch-target minimum.
✅ Clean
- The selected replacement is represented as a Layer-3 transient continuous effect with a concrete object identity and duration; that is the right seam for CR 612/613. The type-line traversal is also supported by CR 612.1.
Recommendation: Address the missing rules-text roots and add the activation-cost regression, then request re-review on the updated head.
Add a reusable Layer-3 token-substitution primitive that replaces one color word / basic land type / creature type with another on a target spell or permanent, mirroring the existing SetChosenName name-change. - New Effect::ChangeTextWords (interactive from/to choice at resolution) and ContinuousModification::ReplaceTextWord (latched operands), applied at Layer 3 per CR 612.1 / 612.2 / 613.1c. - New game/text_substitution.rs walker with an exhaustive, CR-612.2-correct carrier/no-op classification across every color/land/creature-type-bearing enum leaf (keywords, protection/hexproof, target filters, devotion, type lines, landwalk, and ability effect target filters). Mana symbols, color-set-size predicates, chosen-refs, card names, and the Layer-5 color characteristic are correctly never changed. - Interactive WaitingFor::TextWordReplacement + GameAction round-trip with AI legal-action enumeration, multiplayer routing, and render-only choice UI. - Effect::target_filter_mut accessor (paired with target_filter) so the walker reaches words inside ability effect target filters. Covers Mind Bend, Sleight of Mind, Glamerdye, Alter Reality, Magical Hack, Artificial Evolution (with the "can't be Wall" excluded_to rider), Crystal Spray, Trait Doctoring, Whim of Volrath, and Spectral Shift (modal). New Blood, Balduvian Shaman, Deceptive Divination, Magical Hacker, and mass-population / on-stack-instant text-changes remain honest coverage gaps. 12 discriminating runtime tests (duration expiry, filter-position color recursion, landwalk, multi-authority timestamp ordering, Crystal Spray continuation, CR-612.2 name exclusion, serde round-trip, effect-target-filter recursion) plus a 10-card parser snapshot.
real-venus
force-pushed
the
feat/text-changing-effects
branch
from
6a437f1 to
b3c5198
Compare
|
@matthewevans |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Request changes
🔴 Blocker
-
[HIGH] The replacement traversal still deliberately skips
TriggerDefinition.condition/TriggerConstraintandPlayerFilter, while this PR makes text-changing spells supported and lets them target any matching permanent or spell. Evidence:crates/engine/src/game/text_substitution.rs:166-177documents those roots as excluded, andwalk_trigger_definitionat lines 1158-1194 only visits event-shape filters, execution, andunless_pay; meanwhileTriggerCondition::ControlsType { filter },ControlCount { filter },EventObjectMatchesFilter { filter }, and similar variants carry creature-type/color/land-type words. Why it matters: Artificial Evolution and siblings must replace eligible words in an object's full rules text; a permanent whose relevant word is only in an intervening-if trigger condition will either offer no replacement or retain the old trigger predicate, producing silently wrong behavior. Suggested fix: add exhaustive traversal for the word-bearingTriggerCondition/TriggerConstraint/PlayerFilterbranches (or keep the affected-card class honestly unsupported), then add a production-pipeline regression that changes a trigger-condition filter and proves the trigger behavior changes. -
[MED] The new Goblin Chirurgeon regression inspects the rewritten
AbilityCoststructure but never attempts the activated ability with an Elf versus a Goblin sacrifice candidate. Evidence:crates/engine/tests/integration/text_changing_effects.rs:469-537uses the localsacrifice_cost_subtypeshelper after the spell resolves. Why it matters: it proves traversal mutation but not that activation's real legality/payment pipeline consumes the rewritten filter—the specific regression requested was legal-candidate behavior. Suggested fix: after Artificial Evolution resolves, drive the actual activation path with both candidate types and assert Elf is accepted while Goblin is rejected (with a reach guard showing the ability/action is available).
🟡 Non-blocking
- Gemini's outdated notes on the earlier head do not block this review; the current resolver already borrows the category/exclusion slices and the modal uses a 44px minimum height.
✅ Clean
- The updated head does descend through
AbilityDefinition.cost,AbilityCondition,unless_pay, and nested costs, so it addresses the previous cost-root finding at the intended traversal seam.
Recommendation: Complete the remaining rules-text carriers and replace the structural-only cost assertion with an activation-pipeline regression before re-review.
2 participants
Implement CR 612 text-changing effects (word replacement)
Summary
Adds a reusable, rules-correct primitive for text-changing effects (CR 612) — the
"replace all instances of one color word / basic land type / creature type with
another" family. The effect is applied as a Layer-3 continuous modification that
mirrors the engine's existing
SetChosenNamename-change, extending the same layer pathrather than introducing a parallel mechanism.
This is a class implementation, not a per-card one: a single interactive effect plus a
single substitution walker cover every card in the family, and the walker classifies
every word-bearing enum leaf in the engine, so future printings in this class are handled
with no additional parser work.
Motivation
Word-replacement text-changers (Mind Bend, Sleight of Mind, Glamerdye, …) were unsupported —
the parser dropped the clause and the cards fell through to
unimplemented. Name-changing(
SetChosenName) already existed at Layer 3, but word replacement (CR 612.1/612.2) had norepresentation.
What's implemented
Effect::ChangeTextWords— interactive effect. At resolution the controller chooses acategory (from the card's allowed set), a
fromword present on the target (CR 612.2), anda
toword of that category. Drives aWaitingFor/GameActionround-trip.ContinuousModification::ReplaceTextWord { category, from, to }— the Layer-3modification. Operands are latched at resolution (never re-read from a source), keyed to
the target via
TargetFilter::SpecificObject.game/text_substitution.rs— the substitution walker. Exhaustive,_-free matches overevery word-bearing enum (keywords, protection/hexproof, target filters, devotion, type lines,
landwalk, and ability effect target filters), so a future word-bearing variant fails to
compile until it's explicitly classified as carrier or no-op.
Effect::target_filter_mut— a mutable mirror of the existingtarget_filter(), so thewalker reaches words inside an ability's effect target filter (e.g.
{T}: Destroy target red creature).choice UI (all option labels are engine-computed).
CR compliance
Every rule-bearing arm is annotated and verified against the Comprehensive Rules:
application.
name/base_name(a card named "Whitemane Lion" is untouched by a white→blue change), the Layer-5
colorcharacteristic, or mana cost / pips. Mana symbols (
{R}), color-set-size predicates, andchosen-attribute refs are explicit no-ops; the color word "red" is a carrier, the
{R}pipis not.
until end of turnvs. indefinite) reuses the existing continuous-effect machinery;multi-authority composition follows CR 613.7 timestamp order.
Scope
In scope (10 cards): Mind Bend, Sleight of Mind, Glamerdye, Alter Reality, Magical Hack,
Artificial Evolution (with the "The new creature type can't be Wall"
excluded_torider),Crystal Spray, Trait Doctoring, Whim of Volrath, Spectral Shift (modal).
Deliberately out of scope (left
unimplemented, coverage stays honestly red): New Blood(gain-control + fixed target word), Balduvian Shaman (constrained target + granted upkeep),
Deceptive Divination (mass fixed replacement + chaos), Magical Hacker (
+/-symbol swap —different category), Overload / March of Progress (Overload keyword), and text-changes on an
instant/sorcery during its own resolution (the layer system doesn't process stack objects).
One documented walker boundary: words inside mass-population effect filters (
DestroyAll"all red creatures") are not reached — a benign no-op, never a wrong substitution. The correct
future fix is a separate effect-filter enumerator, not a divergence of the paired
target_filteraccessors.Testing
12 discriminating runtime tests, each driving the real
parse → cast → resolve → choose → flushpipeline with revert-failing assertions:
creature type on a type line);
affectedfilter, andinside an ability's effect target filter;
processed);
and a 10-card parser snapshot.
Verification
cargo fmtclean ·cargo clippy -p engine --all-targets -- -D warningsclean · fullcargo test -p enginegreen (0 failed) ·phase-aiandengine-wasmcompile clean ·parser-combinator and CR-annotation gates pass.