Skip to content

feat(engine): implement CR 612 text-changing effects (word replacement)#5866

Open
real-venus wants to merge 18 commits into
phase-rs:mainfrom
real-venus:feat/text-changing-effects
Open

feat(engine): implement CR 612 text-changing effects (word replacement)#5866
real-venus wants to merge 18 commits into
phase-rs:mainfrom
real-venus:feat/text-changing-effects

Conversation

@real-venus

@real-venus real-venus commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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 SetChosenName name-change, extending the same layer path
rather 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 no
representation.

What's implemented

  • Effect::ChangeTextWords — interactive effect. At resolution the controller chooses a
    category (from the card's allowed set), a from word present on the target (CR 612.2), and
    a to word of that category. Drives a WaitingFor / GameAction round-trip.
  • ContinuousModification::ReplaceTextWord { category, from, to } — the Layer-3
    modification. 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 over
    every 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 existing target_filter(), so the
    walker reaches words inside an ability's effect target filter (e.g. {T}: Destroy target red creature).
  • Interactive plumbing: AI legal-action enumeration, multiplayer routing, and a render-only
    choice UI (all option labels are engine-computed).

CR compliance

Every rule-bearing arm is annotated and verified against the Comprehensive Rules:

  • CR 612.1 / 612.2 / 613.1c — text-changing effect, "used in the correct way," Layer-3
    application.
  • CR 612.2 exclusions are structural — the walker never descends into name / base_name
    (a card named "Whitemane Lion" is untouched by a white→blue change), the Layer-5 color
    characteristic, or mana cost / pips. Mana symbols ({R}), color-set-size predicates, and
    chosen-attribute refs are explicit no-ops; the color word "red" is a carrier, the {R} pip
    is not.
  • Duration (until end of turn vs. 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_to rider),
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_filter accessors.

Testing

12 discriminating runtime tests, each driving the real parse → cast → resolve → choose → flush
pipeline with revert-failing assertions:

  • per-category substitution (color in a keyword, basic land type on a type line + landwalk,
    creature type on a type line);
  • duration — until-end-of-turn expiry vs. indefinite persistence across cleanup;
  • filter-position recursion — a color word inside a granted static's affected filter, and
    inside an ability's effect target filter;
  • CR 612.2 name exclusion (with a positive reach-guard proving the object was actually
    processed);
  • multi-authority timestamp ordering; the no-op case (no qualifying word); serde round-trip;
    and a 10-card parser snapshot.

Verification

cargo fmt clean · cargo clippy -p engine --all-targets -- -D warnings clean · full
cargo test -p engine green (0 failed) · phase-ai and engine-wasm compile clean ·
parser-combinator and CR-annotation gates pass.

Summary by CodeRabbit

  • New Features
    • Added support for effects that replace words in card text (color words, basic land types, and creature types), including optional durations and “can’t be” exclusions.
    • Added a dedicated Change Text prompt where players choose from valid replacement options.
    • Localized the new prompt across supported languages.
  • Bug Fixes
    • Improved end-to-end reliability of the text-replacement flow, including AI candidate generation and a deadlock-safe default when a choice is needed.

@real-venus
real-venus requested a review from matthewevans as a code owner July 15, 2026 15:10

@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 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"

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

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.

Suggested change
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
  1. Mobile / touch: touch targets ≥ 44pt; :hover-only state breaks on mobile and needs a touch-equivalent. (link)

Comment on lines +29 to +57
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);

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 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
real-venus force-pushed the feat/text-changing-effects branch from 72d7f28 to d122a11 Compare July 15, 2026 16:19
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 10 card(s), 8 signature(s) (baseline: main 7ed59c04d41b)

🟢 Added (6 signatures)

  • 4 cards · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[ColorWord], target=spell on stack or permanent)
    • Affected (first 3): Alter Reality, Glamerdye, Sleight of Mind (+1 more)
  • 2 cards · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[BasicLandType], target=spell on stack or permanent)
    • Affected (first 3): Magical Hack, Spectral Shift
  • 2 cards · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[ColorWord, BasicLandType], duration=until end of turn, target=permanent)
    • Affected (first 3): Trait Doctoring, Whim of Volrath
  • 1 card · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[ColorWord, BasicLandType], duration=until end of turn, target=spell on stack or permanent)
    • Affected (first 3): Crystal Spray
  • 1 card · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[ColorWord, BasicLandType], target=permanent)
    • Affected (first 3): Mind Bend
  • 1 card · ➕ ability/ChangeTextWords · added: ChangeTextWords (categories=[CreatureType], excluded to=Wall, target=spell on stack or permanent)
    • Affected (first 3): Artificial Evolution

🔴 Removed (2 signatures)

  • 7 cards · ➖ ability/change · removed: change
    • Affected (first 3): Alter Reality, Artificial Evolution, Glamerdye (+4 more)
  • 3 cards · ➖ ability/change · removed: change (duration=until end of turn)
    • Affected (first 3): Crystal Spray, Trait Doctoring, Whim of Volrath

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

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
real-venus force-pushed the feat/text-changing-effects branch from d122a11 to b36db57 Compare July 15, 2026 17:24
@real-venus

Copy link
Copy Markdown
Contributor Author

@matthewevans
Please kindly review this.

@matthewevans

Copy link
Copy Markdown
Member

Closed without implementation-diff review. This PR enters protected architecture scope because it touches crates/engine/src/game/mod.rs, crates/server-core/src/game_action_payload_guard.rs and spans engine, frontend, and server. AI-contributor PRs may enter this scope only after an explicit prior maintainer appointment or when the PR closes an issue labeled accepted. Tier, contributor standing, the quality label, prior praise, and frontend permission do not waive this gate. Open a fresh PR from current main only after one of those authorizations exists, and rerun /engine-implementer, the final review-impl, and Gate A against its committed head.

@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.

Verdict: Request changes

🔴 Blocker

  • [HIGH] The word walker explicitly omits AbilityDefinition.cost (and AbilityCondition), so this is not rules-complete for the class it parses. Evidence: crates/engine/src/game/text_substitution.rs:157-161 says 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 from AbilityDefinition into cost, so it leaves that rules text unchanged while presenting this as a complete class implementation. Please add a recursive AbilityCost traversal (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.

@matthewevans matthewevans added the enhancement New feature or request label Jul 15, 2026
@real-venus
real-venus force-pushed the feat/text-changing-effects branch from 6a437f1 to b3c5198 Compare July 15, 2026 21:22
@real-venus

Copy link
Copy Markdown
Contributor Author

@matthewevans
Please review this

@matthewevans matthewevans self-assigned this Jul 15, 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.

Verdict: Request changes

🔴 Blocker

  • [HIGH] The replacement traversal still deliberately skips TriggerDefinition.condition / TriggerConstraint and PlayerFilter, 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-177 documents those roots as excluded, and walk_trigger_definition at lines 1158-1194 only visits event-shape filters, execution, and unless_pay; meanwhile TriggerCondition::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-bearing TriggerCondition / TriggerConstraint / PlayerFilter branches (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 AbilityCost structure but never attempts the activated ability with an Elf versus a Goblin sacrifice candidate. Evidence: crates/engine/tests/integration/text_changing_effects.rs:469-537 uses the local sacrifice_cost_subtypes helper 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.

@matthewevans matthewevans removed their assignment Jul 15, 2026
real-venus added a commit to real-venus/phase that referenced this pull request Jul 16, 2026
… rules-text roots

Addresses maintainer review on phase-rs#5866. Extends the Layer-3 word-substitution
walker to reach every color/land/creature-type word carrier in a permanent's
rules text, so a text-changing effect (Artificial Evolution, Glamerdye, Magical
Hack, …) rewrites the word everywhere it is used as that kind (CR 612.1/612.2).

Now traversed (previously silently skipped):
- Ability costs and conditions (Sacrifice/Discard/Exile filters, unless-pay,
  AbilityCondition, cost-reduction, activation restrictions).
- Trigger intervening-if: TriggerCondition (incl. AttackersDeclaredCount),
  TriggerConstraint, and PlayerFilter roots.
- Replacement effects (Root 6): ReplacementCondition + replacement mode/decline.
- Static modes (the entire StaticDefinition.mode field, 119 variants).
- Durations (ForAsLongAs), and QuantityRef/QuantityExpr filter carriers.
- Cross-reference wrappers that embed a walked carrier: Effect::Counter
  source-rider, Effect::AddTargetReplacement, ContinuousModification::
  AddStaticMode, CreateDelayedTrigger/ExiledSpellRider conditions, Vote objects.

A mechanical type-graph sweep enumerates every carrier-type field across the AST
so the traversal is exhaustive rather than ad hoc; the module doc honestly lists
the remaining excluded surfaces (mana pips, core types, names, card-level cast
options), each genuinely wordless or unreachable from a battlefield object.

Tests: real activation-pipeline regression for Goblin Chirurgeon (Elf sacrifice
accepted, Goblin rejected), a trigger intervening-if firing regression, and
revert-failing cast-pipeline tests for the higher-risk carriers. Full engine
suite green; clippy --workspace --all-targets clean.
@real-venus
real-venus force-pushed the feat/text-changing-effects branch from b3c5198 to 5d01eda Compare July 16, 2026 15:11

@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.

Verdict: Request changes — the shared CR 612 walker still misses a rules-text root that creates typed tokens.

🔴 Blocker

  • [HIGH] Artificial Evolution-like effects do not rewrite creature-type words in token creation. Evidence: crates/engine/src/game/text_substitution.rs:2528-2534 visits only Effect::Token.static_abilities, while the same file explicitly excludes token-creation subtypes at :229-234; the created-token name and types are stored in Effect::Token at crates/engine/src/types/ability.rs:9714-9722 and copied unchanged into token resolution at crates/engine/src/game/effects/token.rs:479-499. Why it matters: after changing Goblin to Zombie in a permanent's rules text, an effect such as "create a 1/1 Goblin creature token" still produces a Goblin token, contrary to CR 612.2. Suggested fix: make the shared walk_effect token arm visit the creature-type-bearing name and types fields under the appropriate category, then add a production-pipeline regression that applies the text change and proves both the created token's display name and subtype changed; include an unaffected color/land-type sibling assertion.

🟡 Non-blocking

  • The documented exclusion also leaves mass-population filters unwalked. Keep those forms unsupported rather than silently claiming them covered; audit them as a separate class after the confirmed token root is fixed.

Recommendation: request changes for the token-root traversal plus a discriminating pipeline regression.

… rules-text roots

Addresses maintainer review on phase-rs#5866. Extends the Layer-3 word-substitution
walker to reach every color/land/creature-type word carrier in a permanent's
rules text, so a text-changing effect (Artificial Evolution, Glamerdye, Magical
Hack, …) rewrites the word everywhere it is used as that kind (CR 612.1/612.2).

Now traversed (previously silently skipped):
- Ability costs and conditions (Sacrifice/Discard/Exile filters, unless-pay,
  AbilityCondition, cost-reduction, activation restrictions).
- Trigger intervening-if: TriggerCondition (incl. AttackersDeclaredCount),
  TriggerConstraint, and PlayerFilter roots.
- Replacement effects (Root 6): ReplacementCondition + replacement mode/decline.
- Static modes (the entire StaticDefinition.mode field, 119 variants).
- Durations (ForAsLongAs), and QuantityRef/QuantityExpr filter carriers.
- Cross-reference wrappers that embed a walked carrier: Effect::Counter
  source-rider, Effect::AddTargetReplacement, ContinuousModification::
  AddStaticMode, CreateDelayedTrigger/ExiledSpellRider conditions, Vote objects.

A mechanical type-graph sweep enumerates every carrier-type field across the AST
so the traversal is exhaustive rather than ad hoc; the module doc honestly lists
the remaining excluded surfaces (mana pips, core types, names, card-level cast
options), each genuinely wordless or unreachable from a battlefield object.

Tests: real activation-pipeline regression for Goblin Chirurgeon (Elf sacrifice
accepted, Goblin rejected), a trigger intervening-if firing regression, and
revert-failing cast-pipeline tests for the higher-risk carriers. Full engine
suite green; clippy --workspace --all-targets clean.
@real-venus
real-venus force-pushed the feat/text-changing-effects branch from 5d01eda to 663739d Compare July 16, 2026 20:20
@matthewevans matthewevans self-assigned this Jul 16, 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.

Verdict: Request changes — token creation remains outside the CR 612 traversal.

🔴 Blocker

  • [HIGH] Effect::Token still walks only static_abilities; it leaves the token’s typed name and types unchanged, while the same walker documents token creation specs as deliberately red. Evidence: crates/engine/src/game/text_substitution.rs:254-259 and :2620-2625; crates/engine/src/types/ability.rs:9714-9723 stores the printed token name/types; crates/engine/src/game/effects/token.rs:479-499 resolves those fields into the produced token. CR 612.1 changes an object’s text, and CR 612.2 permits creature-type words used as creature types. Thus after Artificial Evolution changes “Goblin” to “Elf” on “create a Goblin creature token,” resolution still creates a Goblin token. Extend the shared walk_effect Effect::Token arm to walk the creature-type-bearing token fields (and only the valid category), then add a production-pipeline regression that resolves the token creation and proves the output token’s subtype/display name changes, with an unaffected color/land-type sibling assertion.

✅ Clean

  • The current head now traverses activation costs and trigger conditions, and its integration tests exercise the activation payment and intervening-if pipelines; those resolve the earlier blockers at the intended shared walker seam.

Recommendation: fix the token-spec traversal and runtime regression, then request a current-head re-review.

@matthewevans matthewevans removed their assignment Jul 16, 2026
@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.

Verdict: Request changes — token creation remains outside the CR 612 traversal.

🔴 Blocker

[HIGH] Rewrite token creation text through the same substitution authority. Evidence: crates/engine/src/game/text_substitution.rs:2620-2626 handles Effect::Token by walking only static_abilities; its typed name and types fields are untouched. CR 612.2a states that a text-changing effect affecting a token-creating ability “can change these words because they’re being used as creature types.” Why it matters: an Artificial Evolution-style creature-type substitution leaves the created token’s printed creature type unchanged, so the PR does not implement the stated CR 612 class and the current tests do not exercise the real token-resolution case. Suggested fix: extend the existing walk_effect Effect::Token arm to visit the typed token name/type carriers under the category rules, then add a runtime parse → cast → resolve test that proves a changed token type and fails when that traversal is reverted.

🟡 Verification

Current-head required Rust CI is terminal red (lint/tests/card-data aggregate); the current head differs from the previously reviewed one only by a merge from main, not a contributor fix for this blocker.

Recommendation: request changes; keep this PR out of the queue until the token-resolution runtime path and failing CI are resolved.

@matthewevans matthewevans removed their assignment Jul 17, 2026
@matthewevans matthewevans self-assigned this Jul 21, 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.

Verdict: Request changes — token creation is still outside the CR 612 traversal on the current head.

🔴 Blocker

  • [HIGH] Effect::Token only walks static_abilities; it does not walk the typed token name or types used to construct the created token. Evidence: crates/engine/src/game/text_substitution.rs:2665-2671; the same module expressly classifies token creation specs as deliberately red at :230-246, while crates/engine/src/game/effects/token.rs:485-556 copies Effect::Token.name and .types into the token-resolution path. The official Comprehensive Rules say: “A text-changing effect that affects such a spell or an object with such an ability can change these words because they’re being used as creature types” (CR 612.2a, June 19 2026 rules) [and CR 612.1/612.2 establish the general text-changing rule]. Why it matters: after Artificial Evolution changes Goblin to Elf on an ability that creates a Goblin token, this implementation still creates a Goblin token, so the advertised CR 612 class is rules-incorrect. Suggested fix: extend the shared walk_effect token arm to visit the creature-type-bearing token name/types under the existing category discipline, then add a parse → cast → resolve → choose runtime regression that proves the resulting token’s subtype and display name change, with an unaffected color/land-type sibling assertion.

🟡 Non-blocking

  • The current CodeRabbit comments are individually valid but subordinate to the blocker: parse_text_change_excluded_to accepts a parsed prefix while discarding trailing rider text (crates/engine/src/parser/oracle_effect/sequence.rs:7133-7166); the new localized UI strings remain English in non-English catalogs; coverage details omit target and excluded_to; and the serde test does not exercise the empty excluded_to default path. Address them with the blocker or in a follow-up after the rules-correct token path lands.

✅ Clean

  • The parser parse-diff is current for this head and matches the stated ten-card scope: its only additions are the ChangeTextWords signatures for the named Mind Bend/Sleight/Glamerdye/Alter Reality/Magical Hack/Spectral Shift/Artificial Evolution/Crystal Spray/Trait Doctoring/Whim cards. Earlier cost and trigger-condition blockers are now traversed on the shared walker path.

Recommendation: keep the PR out of the queue until the token-creation traversal and discriminating runtime regression are complete; then re-run the current-head implementation review.

@matthewevans matthewevans removed their assignment Jul 21, 2026
…effects

# Conflicts:
#	crates/engine/src/parser/oracle_effect/mod.rs
Addresses the remaining review blocker. `Effect::Token` previously walked only
`static_abilities`, so after Artificial Evolution changed Goblin to Elf, an
ability that creates a Goblin token still created a Goblin token.

CR 612.2a is an explicit carve-out from the CR 612.2 name rule: spells and
abilities that create creature tokens use creature types to define both the
token's types AND its name, and a text-changing effect changes those words
"because they're being used as creature types, even though they're also being
used as names." The walker now honors that:

- `Effect::Token` walks `name` + `types`, and also `colors` (a color word used
  as a color word, CR 612.2), `keywords`, `count` and `enter_with_counters`.
  The name rewrite is deliberately narrower than the type rewrite: only for the
  creature-type category, and only for name words the spec also declares in
  `types` (snapshotted before substitution), so a predefined token name that is
  not backed by a declared creature type (Treasure, Food, a Role) is untouched.
- The same discipline applies to the replacement-side `TokenSpec`
  (`additional_token_spec` / `ensure_token_specs`), to `FaceDownProfile.subtypes`
  (CR 708.2a) on Manifest / TurnFaceDown / ChangeZone / ChangeZoneAll, and to
  `CopyTokenOf`'s printed carriers. `CopyTokenOf`/`CreateTokenCopyFromPool` take
  their name and types from the copied object (CR 707.2) and print none of their
  own, so CR 612.2a has nothing to reach there.

Seven runtime regressions, each verified by neutering its seam and observing the
failure: the created token's subtype and display name both change; a land-type
change leaves the token's creature type and name alone; a color-word change
rewrites only the color axis; and the face-down profile, replacement token spec
and copy-token carriers each have their own test.

Also addresses the review's non-blocking notes: the serde round trip now covers
the empty `excluded_to` skip path, the "The new X can't be Y" rider is anchored
so a truncated rider no longer parses, coverage detail reports target/
excluded_to/duration, the parser re-pairs via TextPair::split_at instead of raw
byte offsets, and the six non-English locales carry real translations.
@matthewevans

Copy link
Copy Markdown
Member

Current-head implementation review completed. The shared walker now reaches Effect::Token through WordCursor::token_spec, rewrites the CR 612.2a creature-type/name pair only when the printed spec declares that creature type, and keeps color and basic-land-type changes isolated. The added integration coverage drives the actual token-resolution path and checks both token name and subtype; the previous blocker is addressed at the shared traversal seam.

No approval or enqueue yet: required Rust, AI/performance, and frontend checks are still running on this head. I am holding the PR for those external gates.

@coderabbitai coderabbitai 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.

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/engine_resolution_choices.rs (1)

5722-5724: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Route this arm through finish_with_continuation. This branch stops after effects::drain_pending_continuation; the shared helper also runs resume_pending_continuation_if_priority, so an inline copy can leave a parked continuation or resolution frame behind.

🤖 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/engine_resolution_choices.rs` around lines 5722 -
5724, Update the affected resolution-choice arm to return through
finish_with_continuation instead of directly constructing
ResolutionChoiceOutcome::WaitingFor after drain_pending_continuation. Preserve
the existing set_priority and continuation-draining inputs while ensuring the
shared helper performs resume_pending_continuation_if_priority and completes any
parked continuation or resolution frame.
🤖 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/engine_resolution_choices.rs`:
- Around line 5722-5724: Update the affected resolution-choice arm to return
through finish_with_continuation instead of directly constructing
ResolutionChoiceOutcome::WaitingFor after drain_pending_continuation. Preserve
the existing set_priority and continuation-draining inputs while ensuring the
shared helper performs resume_pending_continuation_if_priority and completes any
parked continuation or resolution frame.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e8a0fad-d434-41d2-81c9-1b6087f16adb

📥 Commits

Reviewing files that changed from the base of the PR and between b83bb2f and 7cf1a37.

📒 Files selected for processing (13)
  • client/src/i18n/locales/de/game.json
  • client/src/i18n/locales/es/game.json
  • client/src/i18n/locales/fr/game.json
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pl/game.json
  • client/src/i18n/locales/pt/game.json
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/text_substitution.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/sequence.rs
  • crates/engine/tests/integration/main.rs
  • crates/engine/tests/integration/text_changing_effects.rs
🚧 Files skipped from review as they are similar to previous changes (11)
  • crates/engine/tests/integration/main.rs
  • client/src/i18n/locales/pt/game.json
  • client/src/i18n/locales/es/game.json
  • crates/engine/src/parser/oracle_effect/mod.rs
  • client/src/i18n/locales/fr/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/coverage.rs
  • crates/engine/src/parser/oracle_effect/sequence.rs
  • crates/engine/src/game/text_substitution.rs

@matthewevans matthewevans self-assigned this Jul 22, 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.

Verdict: Request changes — the current head still bypasses the shared resolution-completion boundary.

🔴 Blocker

[HIGH] Route ChooseTextWordReplacement through finish_with_continuation. Evidence: crates/engine/src/game/engine_resolution_choices.rs:5722-5724 calls drain_pending_continuation and returns the cloned waiting state directly, while finish_with_continuation at :6786-6795 additionally invokes resume_pending_continuation_if_priority, which resumes remaining typed resolution frames. Why it matters: a text-word choice can finish its immediate continuation while a higher-level resolution frame remains parked, leaving the resolution incomplete despite returning priority. Suggested fix: replace the local set_priority/drain_pending_continuation/clone sequence with ResolutionChoiceOutcome::WaitingFor(finish_with_continuation(state, player, events)), and add a regression that exercises a text-word choice beneath such a parked frame.

✅ Clean

The shared token-spec traversal and the Artificial Evolution → Ib Halfheart runtime path are present on this head; the parse-diff sticky comment is current and matches the intended ten-card scope.

Recommendation: make this local completion-boundary fix and add its discriminating regression, then rerun the current-head review.

@matthewevans matthewevans removed their assignment Jul 22, 2026
…uation

The `ChooseTextWordReplacement` arm re-implemented the completion boundary
inline (`set_priority` + `drain_pending_continuation` + returning the cloned
waiting state) instead of calling the shared helper every other resolution
choice uses. `finish_with_continuation` additionally runs
`resume_pending_continuation_if_priority`, which resumes parked typed
resolution frames (BatchDelivery, PerPlayerZoneChoice, CopyToken, MultiDraw,
...) and drains eligible cost-move roots — none of which the inline copy
reached. Routing through the shared authority removes the divergence.

Everything before the completion boundary is unchanged: index validation, the
Layer-3 ReplaceTextWord transient continuous effect keyed to the target, and
its duration default.

Also classifies two new upstream shapes the walker's exhaustive matches
require, both explicit no-ops: `Effect::Cloak.enters_under` (a controller ref,
CR 110.2a - no printed word, mirroring Manifest) and
`StaticMode::MustAttackAwayFromSource` (nullary combat requirement,
CR 508.1d + CR 701.15b).

No regression test accompanies this: the behaviour is currently unobservable
from the shipped card class. `drain_pending_continuation` already drains every
frame kind these ten cards can park (including RepeatFor at effects/mod.rs:773
and merged AbilityContinuation chains), and the frame kinds it cannot reach
require ChangeTextWords to execute inside a batch / per-player / replacement
body, which no printed card in the class does. Details in the PR discussion.
@real-venus

Copy link
Copy Markdown
Contributor Author

Routed ChooseTextWordReplacement through finish_with_continuation as requested — the arm no longer re-implements the completion boundary, so it now inherits resume_pending_continuation_if_priority (typed resolution-frame resume + cost-move drain) like the ~40 sibling arms. Everything before the boundary (index validation, the Layer-3 ReplaceTextWord TCE install, its duration default) is unchanged.

On the requested regression: I could not write one that fails on revert, and I'd rather say so with evidence than ship a test that passes either way.

  1. Nested AbilityContinuation frames don't stack. With a ChooseOneOf outer tail plus a branch tail, dumping resolution_stack at the text prompt shows one frame, not two — the engine merges the branch tail into the parked outer chain (Draw -> GainLife 5), so a single drain_pending_continuation pass covers both.
  2. A parked RepeatFor frame passes with the fix reverted. I built that fixture, reverted the arm to the old inline sequence, and re-ran — the test still passed, because drain_pending_continuation already drains repeat-for at crates/engine/src/game/effects/mod.rs:773. I deleted the non-discriminating test.
  3. The frame kinds the drain can't reach aren't reachable from this class. BatchDelivery, CopyToken, EachPlayerCopyChosen, PerPlayerZoneChoice (early-returns at :692), PerCategoryZoneChoice, MultiDraw, ConniveReentry, LifeTotalAssignment, PostReplacement only become the active frame during a multi-object zone batch, a per-player/per-category sweep, a copy-token proposal, a multi-card draw, or a replacement dispatch. Per active_frame_requires_ability_continuation_parent (effects/mod.rs:1513), a continuation parked under one of those is inserted below it, so ChangeTextWords would have to execute inside such a body. All ten in-scope cards parse to a single top-level ChangeTextWords on an instant/sorcery, optionally followed by "Draw a card" (pinned by parser_snapshots_for_text_changing_class); Spectral Shift's modes chain via build_chained_resolved, not a resolution-time ChooseOneOf frame.

So the change is a refactor onto the single shared authority — correct and defense-in-depth for future frame kinds, but behaviour-neutral for the shipped class today. If you'd still like a guard, I'm happy to add a #[cfg(test)] assertion at the handle_resolution_choice level that the arm completes via the shared helper — just say the word.

Also merged upstream/main and classified the two new shapes the walker's exhaustive matches require, both explicit no-ops with CR annotations: Effect::Cloak.enters_under (controller ref, CR 110.2a) and StaticMode::MustAttackAwayFromSource (nullary, CR 508.1d + CR 701.15b). cargo clippy --workspace --all-targets -D warnings clean; cargo test -p engine 21327 passed / 0 failed.

@coderabbitai coderabbitai 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/engine/src/game/ability_scan.rs (1)

407-409: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Scan the ChangeTextWords target filter.

Effect::ChangeTextWords still carries a target: TargetFilter, but this arm returns Axes::NONE, so event/sibling/projected reads inside that filter are ignored. A dynamic target can therefore be misclassified as inert by the loop/read analyzers. Use the already-added SnapshotOrEvent context:

Proposed fix
-        Effect::ChangeTextWords { .. } => Axes::NONE,
+        Effect::ChangeTextWords { target, .. } => {
+            scan_target_filter(target, target_ctx, mode)
+        }
🤖 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/ability_scan.rs` around lines 407 - 409, Update the
Effect::ChangeTextWords arm in the axis scan to inspect its target TargetFilter
using the existing SnapshotOrEvent context, rather than always returning
Axes::NONE. Propagate any event, sibling, or projected axes read by that filter
while preserving NONE when the target filter is inert.
client/src/i18n/locales/en/game.json (1)

1749-1752: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add the translation key consumed by the text-word replacement modal.

This change adds cardChoice.zoneOpponentChooser, while TextWordReplacementModal calls cardChoice.textWordReplacement.title and .subtitle. The new prompt will therefore render missing-key fallbacks unless that key exists elsewhere; add the expected textWordReplacement object and mirror it across the locale files.

🤖 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/i18n/locales/en/game.json` around lines 1749 - 1752, Add the
missing cardChoice.textWordReplacement object with title and subtitle
translations matching TextWordReplacementModal’s lookups, and mirror the new
keys across every locale file that defines cardChoice. Keep the existing
zoneOpponentChooser translations unchanged.
🤖 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 `@client/src/i18n/locales/en/game.json`:
- Around line 1749-1752: Add the missing cardChoice.textWordReplacement object
with title and subtitle translations matching TextWordReplacementModal’s
lookups, and mirror the new keys across every locale file that defines
cardChoice. Keep the existing zoneOpponentChooser translations unchanged.

In `@crates/engine/src/game/ability_scan.rs`:
- Around line 407-409: Update the Effect::ChangeTextWords arm in the axis scan
to inspect its target TargetFilter using the existing SnapshotOrEvent context,
rather than always returning Axes::NONE. Propagate any event, sibling, or
projected axes read by that filter while preserving NONE when the target filter
is inert.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e0f96534-830c-4f9f-8ae8-a5dffa8a5a07

📥 Commits

Reviewing files that changed from the base of the PR and between 7cf1a37 and 0d5b04c.

📒 Files selected for processing (30)
  • client/src/adapter/types.ts
  • client/src/game/waitingForRegistry.ts
  • client/src/i18n/locales/de/game.json
  • client/src/i18n/locales/en/game.json
  • client/src/i18n/locales/es/game.json
  • client/src/i18n/locales/fr/game.json
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pl/game.json
  • client/src/i18n/locales/pt/game.json
  • crates/engine/src/ai_support/candidates.rs
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/game/scenario.rs
  • crates/engine/src/game/text_substitution.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_ir/ast.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/action_stable_order.rs
  • crates/engine/src/types/actions.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/tests/integration/main.rs
  • crates/manabrew-compat/src/lib.rs
  • crates/phase-ai/src/decision_kind.rs
  • crates/phase-ai/src/search.rs
  • crates/server-core/src/game_action_payload_guard.rs
🚧 Files skipped from review as they are similar to previous changes (27)
  • crates/engine/tests/integration/main.rs
  • crates/phase-ai/src/search.rs
  • crates/engine/src/game/effects/mod.rs
  • client/src/game/waitingForRegistry.ts
  • client/src/i18n/locales/pt/game.json
  • crates/manabrew-compat/src/lib.rs
  • crates/engine/src/game/layers.rs
  • crates/server-core/src/game_action_payload_guard.rs
  • client/src/i18n/locales/es/game.json
  • crates/engine/src/types/action_stable_order.rs
  • crates/engine/src/types/actions.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/game/scenario.rs
  • client/src/adapter/types.ts
  • client/src/i18n/locales/fr/game.json
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pl/game.json
  • crates/engine/src/parser/oracle_ir/ast.rs
  • client/src/i18n/locales/de/game.json
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/ai_support/candidates.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/game/text_substitution.rs

@matthewevans

Copy link
Copy Markdown
Member

Blocked on a compile failure in this PR's own code — I'm not running a full implementation review until it builds, because none of the other nine red jobs mean anything until it does.

Reviewed at head 0d5b04c0.

🔴 Blocker — six E0004 non-exhaustive match errors

Rust lint (fmt, clippy, parser gate) fails to compile. Three new variants were added without updating every match site that covers them:

error[E0004]: non-exhaustive patterns: `&WaitingFor::TextWordReplacement { .. }` not covered            (×3)
error[E0004]: non-exhaustive patterns: `&GameAction::ChooseTextWordReplacement { .. }` not covered      (×2)
error[E0004]: non-exhaustive patterns: `&ContinuousModification::ReplaceTextWord { .. }` not covered    (×1)

This is why nine jobs are red at once — Rust lint, both Rust tests shards, Card data, WASM compile check, Decision-cost perf gate, Paired-seed AI gate, and the required aggregator. They are all downstream of the same compile failure, not eight independent problems. The one red job that is not yours is Tauri compile check: that was our stale client/src-tauri/Cargo.lock after the v0.35.2 release, fixed on main in #6568 and permanently prevented in #6574. It will clear when you rebase.

The compiler is the complete census here — cargo check -p engine locally will enumerate every remaining site, and it will be exhaustive in a way that grepping for variant names is not. Adding a new WaitingFor or GameAction variant means every exhaustive match over those enums needs an arm, including the ones in the AI and server crates that are easy to miss from inside the engine.

On scope

Setting the compile failure aside: at +7,765 / −136 across 45 files, this is very large for a single review. CR 612 is a broad section, and a PR that lands text-changing effects, a new WaitingFor state, a new GameAction, and a new ContinuousModification in one change is difficult to review with the care that a layer-3 continuous-effect feature deserves. I'm not asking you to split it as a precondition — but if there is a natural seam (for example, the ContinuousModification::ReplaceTextWord layer plumbing landing ahead of the interactive word-choice flow), separating them would get you faster and more useful review on each half.

When you push the compile fix, I'd also like the PR body to state which CR 612 subrules are implemented versus deliberately deferred. Specifically: 612.1's restriction to text changes only, the layer placement relative to type-changing effects, 612.5 on a changed land subtype altering its intrinsic mana ability, and 612.6's scoping of "instances of a word". A PR claiming a whole CR section is reviewed against that section — knowing up front what's in scope prevents me from filing deferred items as defects.


Recommendation: fix the six E0004 sites, confirm green, and flag it here — I'll pick up the full implementation review at that point. The existing CHANGES_REQUESTED stands until then. Nothing in this note is a judgment on the approach; I simply can't evaluate it in this state.

@matthewevans

Copy link
Copy Markdown
Member

Correcting my previous comment — I got two things wrong, and both were unfair to you.

The compile failure is ours, not yours. We'll port it.

I said you "added three new variants without updating every match site." That is false, and I should have checked the timestamps before writing it.

Your head 0d5b04c0 was committed at 2026-07-22T13:23:59Z. The match sites that fail to compile were added to main after that:

Neither file is among your 45. You could not have covered match arms in a file that did not exist. This is maintainer-caused staleness: our churn broke your branch, so we do the port, regardless of size — that is our standing rule and I misapplied it. Please don't rebase or chase these on our account; if you already started, stop and let us take it.

Tauri compile check was separately ours (stale client/src-tauri/Cargo.lock from the v0.35.2 release; fixed in #6568, prevented in #6574).

My CR references were wrong in three places

I asked you to address "612.2 layer placement", "612.5's land subtype mana ability", and "612.6's instances-of-a-word scoping". All three are misattributions on my part — grep-verified against docs/MagicCompRules.txt:

What I wrote What the rule actually is
612.2 = layer placement CR 613.1c (:2964) — "Layer 3: Text-changing effects are applied." CR 612.2 (:2936) is the "used in the correct way" restriction
612.5 = land subtype → intrinsic mana ability CR 305.6 / 305.7 (:1705, :1707). CR 612.5 (:2944) is Exchange of Words, a text-box swap
612.6 = instances-of-a-word scoping CR 612.6 (:2946) is Volrath's Shapeshifter "full text". The instance scoping is CR 612.1 / 612.2

Ignore that list. I'm holding contributors to grep-verifying every CR number this week; the same standard applies to me, and I didn't meet it here.

On the substance — this reads as a genuine CR 612 implementation

Reviewing against what the rules actually say rather than my mangled version of them:

  • CR 612.1 (text only) is structural in walk_object_words (crates/engine/src/game/text_substitution.rs:364-409): six rules-text roots, never descending into name/base_name, the Layer-5 color characteristic, or mana cost. Treating mana symbols as no-ops while the color word is a carrier is exactly the CR 612.2 distinction.
  • CR 613.1c ordering holds — types/layers.rs:19-22 places Text (3) before Type (4), and ReplaceTextWord maps to Layer::Text.
  • CR 305.6/305.7 falls out for free, which is the strongest evidence this is a class implementation rather than card work: Root 1 rewrites live type-line subtypes in Layer 3, and apply_intrinsic_basic_land_mana_abilities (game/layers.rs:6835-6854) reads live subtypes after Layer 4 — so Magical Hack on a Mountain yields an Island tapping for {U} with zero card-specific code.
  • The _-free exhaustive matches over every word-bearing enum mean a future variant fails to compile until it's classified. That's the right way to make this hold up.

Standing non-blocking items

These are real and worth addressing whenever you next push — none is urgent given we're taking the rebase:

  • The completion-boundary fix has no discriminating regression. tests/integration/text_changing_effects.rs:1225 exercises only Crystal Spray's immediate draw, which the previous code drained too — reverting the finish_with_continuation change leaves the suite green.
  • No fixture targets a spell on the stack, though seven of the ten cards read "target spell or permanent". That path branches differently: collect_scan_zones routes SpecificObject into Zone::Stack (game/layers.rs:5710-5720), but the Step-1 re-seed iterates battlefield ids only (:1836-1840), and the invariant at :5614-5617 is keywords-only for Stack while ReplaceTextWord writes abilities, subtypes, and static/replacement definitions.
  • No land fixture anywhere in the suite, so the CR 305.6/305.7 path above — arguably Magical Hack's signature use — is correct by construction but unproven. A Mountain → Island test asserting {U} would lock in the layer ordering.
  • game/ability_scan.rs:409 returns Axes::NONE and never consumes the target_ctx this PR adds at :5424-5428. Inert today, but dead as written.

CodeRabbit's i18n finding is refuted — cardChoice.textWordReplacement.* exists in en and all seven locales with real translations; it anchored to unrelated zoneOpponentChooser churn.


Revised recommendation: we take the rebase and the two exhaustive-match ports. The three test fixtures above are the remaining ask, and they can land on top. My earlier CHANGES_REQUESTED was based on a compile failure I wrongly attributed to you — treat this comment as superseding it.

matthewevans added a commit that referenced this pull request Jul 23, 2026
… timestamps

Addresses @coderabbitai review on #6576.

The finding is correct: `.commit.committer.date` is the head commit's author/
committer metadata, not push time, so a rebase, an amend, or a delayed push all
skew a date comparison — and a squash-merged main commit carries a date
unrelated to when its content actually landed.

The suggested remedy (read push time from PR timeline/update events) would fix
the skew but still answers the wrong question. What the check actually needs to
know is not "which is newer" but "did the branch ever contain the failing
code?" — which `git merge-base --is-ancestor` answers directly and is immune to
the entire class of timestamp problems, including ones PR timeline events also
get wrong.

Replaces the three date greps with:
  1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all
  2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit
     that introduced the match site in the branch's history
  3. the unchanged PR file-list check

Also adds a non-vacuity probe, because a check that returns "not an ancestor"
for every input is broken rather than exonerating: a commit known to be in the
branch's history (`git merge-base origin/main <headOid>`) must report ancestor.

Verified verbatim against the case that motivated the section (#5866): step 1
reports interaction.rs absent at the head, step 2 reports the shared.rs match
site not in the branch's history, and the probe reports the real merge-base
df2ab2d as an ancestor — so the test discriminates rather than always
failing closed.

Timestamps stay as human-readable colour in the review comment, relabelled
"committed at" rather than "pushed at".
Main added the game/interaction.rs multiplayer subsystem, whose exhaustive
WaitingFor / GameAction / ContinuousModification matches did not cover this
PR's variants, so the merged tree did not compile. Classifies them:

- WaitingFor::TextWordReplacement is a single-selection index choice with
  engine-computed public options, bound exactly like DamageSourceChoice across
  human_response_model / classify_waiting_for / selection_projection.
- GameAction::ChooseTextWordReplacement carries index: usize, so in
  project_action_payload it joins the OptionIndex group (ChooseBranch /
  ChooseReplacement), not the ObjectId-carrying ChooseDamageSource; adds the
  InteractionActionCode::ChooseTextWordReplacement literal and regenerates the
  client TS binding.
- ContinuousModification::ReplaceTextWord carries no QuantityExpr, so it joins
  the None group in continuous_modification_dynamic_quantity_mut.

check-interaction-bindings.sh and check-parser-combinators.sh pass; workspace
clippy -D warnings clean; cargo test -p engine 3914 passed / 0 failed.
matthewevans added a commit that referenced this pull request Jul 24, 2026
…7-23 sweep (#6576)

* docs(pr-review-loop): encode review-sweep defects found in the 2026-07-23 sweep

Four failure modes from a live authorized-maintainer sweep, each of which
produced a defective posted review:

- Dispatching Review Agents (new): a subagent's final assistant text is not
  delivered to the lead — findings arrive only via SendMessage, and charters
  must say so verbatim. Idle means resumable, not dead. Do not substitute a
  lead-authored review for a slow agent; three of the sweep's four bad reviews
  came from doing exactly that, and each was overturned by the agent's report
  minutes later.

- Verifying the Review's Own Evidence (new): the CR grep-verification bar binds
  the reviewer as hard as the contributor, since the contributor acts on what
  the review says. Check repo convention before proposing a citation, and read
  the rule rather than trusting a number in roughly the right section —
  adjacent subrules routinely cover unrelated single cards (612.5 is Exchange
  of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a
  different section from the effect it orders (613.1c, not 612.x).

- Judging the seam (new, under Review Bar): "reuses existing machinery" is not
  "correct seam". Every proliferation heuristic in this file returned clean on
  a PR that reused the instance-blind checker while the instance-aware
  authority twenty lines away already cited the rule and dropped half of it.
  Grep for the rule, not the symbol; two mechanisms for one rule is the defect.

- Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's
  own new variants look like contributor sloppiness and are frequently ours —
  main adds a new match site after the contributor's last push. Timestamp the
  failing site against the head before writing that finding.

- Review Freshness: stale approval plus armed auto-merge is a safety defect,
  not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting
  APPROVED after a new commit lands, so an unreviewed head can sit armed to
  merge. Worse when the post-approval commit is maintainer-authored.

* docs(pr-review-loop): test compile-error attribution by ancestry, not timestamps

Addresses @coderabbitai review on #6576.

The finding is correct: `.commit.committer.date` is the head commit's author/
committer metadata, not push time, so a rebase, an amend, or a delayed push all
skew a date comparison — and a squash-merged main commit carries a date
unrelated to when its content actually landed.

The suggested remedy (read push time from PR timeline/update events) would fix
the skew but still answers the wrong question. What the check actually needs to
know is not "which is newer" but "did the branch ever contain the failing
code?" — which `git merge-base --is-ancestor` answers directly and is immune to
the entire class of timestamp problems, including ones PR timeline events also
get wrong.

Replaces the three date greps with:
  1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all
  2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit
     that introduced the match site in the branch's history
  3. the unchanged PR file-list check

Also adds a non-vacuity probe, because a check that returns "not an ancestor"
for every input is broken rather than exonerating: a commit known to be in the
branch's history (`git merge-base origin/main <headOid>`) must report ancestor.

Verified verbatim against the case that motivated the section (#5866): step 1
reports interaction.rs absent at the head, step 2 reports the shared.rs match
site not in the branch's history, and the probe reports the real merge-base
df2ab2d as an ancestor — so the test discriminates rather than always
failing closed.

Timestamps stay as human-readable colour in the review comment, relabelled
"committed at" rather than "pushed at".

---------

Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🤖 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 `@crates/engine/src/game/interaction.rs`:
- Line 266: Update the WaitingFor::TextWordReplacement handling and its
corresponding action-submission path so clients receive candidate replacement
text and type information rather than only OptionIndex values. Add or reuse a
text-replacement candidate projection, and convert the selected candidate to an
index only when materializing the submitted action.
- Around line 3968-3970: Update the comment immediately above
GameAction::ChooseTextWordReplacement { index } to remove the inaccurate “CR
612.1” citation and describe this as protocol projection: the answer is a single
index into the engine-computed option list. Do not add a rules citation unless
it refers to a verified rule governing the text-changing effect.

In `@crates/engine/src/types/interaction.rs`:
- Line 338: Update the ChooseTextWordReplacement enum variant’s serde
serialization discriminator to emit exactly "ChooseTextWordReplacement" despite
the surrounding camelCase convention, preserving the client contract.
🪄 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: 2755c544-6531-4250-8338-d3141883882d

📥 Commits

Reviewing files that changed from the base of the PR and between 0d5b04c and 7da1a6d.

⛔ Files ignored due to path filters (1)
  • client/src/adapter/generated/interaction/index.ts is excluded by !**/generated/**
📒 Files selected for processing (26)
  • client/src/adapter/types.ts
  • client/src/i18n/locales/de/game.json
  • client/src/i18n/locales/en/game.json
  • client/src/i18n/locales/es/game.json
  • client/src/i18n/locales/fr/game.json
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pl/game.json
  • client/src/i18n/locales/pt/game.json
  • crates/engine/src/ai_support/candidates.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/game/interaction.rs
  • crates/engine/src/game/mod.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/game/scenario.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/sequence.rs
  • crates/engine/src/parser/oracle_static/shared.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/action_stable_order.rs
  • crates/engine/src/types/actions.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/types/interaction.rs
  • crates/engine/tests/integration/main.rs
  • crates/manabrew-compat/src/lib.rs
  • crates/phase-ai/src/decision_kind.rs
🚧 Files skipped from review as they are similar to previous changes (22)
  • crates/engine/src/game/scenario.rs
  • crates/manabrew-compat/src/lib.rs
  • crates/engine/tests/integration/main.rs
  • crates/engine/src/game/mod.rs
  • client/src/i18n/locales/fr/game.json
  • crates/phase-ai/src/decision_kind.rs
  • crates/engine/src/game/effects/mod.rs
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pt/game.json
  • client/src/i18n/locales/en/game.json
  • client/src/i18n/locales/es/game.json
  • crates/engine/src/parser/oracle_effect/mod.rs
  • client/src/i18n/locales/de/game.json
  • crates/engine/src/types/game_state.rs
  • client/src/adapter/types.ts
  • crates/engine/src/game/quantity.rs
  • client/src/i18n/locales/pl/game.json
  • crates/engine/src/game/engine_resolution_choices.rs
  • crates/engine/src/ai_support/candidates.rs
  • crates/engine/src/types/actions.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/parser/oracle_effect/sequence.rs

| WaitingFor::OpponentGuess { .. }
| WaitingFor::SpellbookDraft { .. }
| WaitingFor::DamageSourceChoice { .. }
| WaitingFor::TextWordReplacement { .. }

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.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Expose replacement choices, not only their indices.

Routing TextWordReplacement through ExactCandidates and projecting only OptionIndex leaves clients with choices such as 0, 1, and 2, but no replacement text/type to render. Add a text-replacement projection or candidate surfaces containing the actual options; materialize the index only when submitting the action.

Also applies to: 3967-3971

🤖 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/interaction.rs` at line 266, Update the
WaitingFor::TextWordReplacement handling and its corresponding action-submission
path so clients receive candidate replacement text and type information rather
than only OptionIndex values. Add or reuse a text-replacement candidate
projection, and convert the selected candidate to an index only when
materializing the submitted action.

Comment thread crates/engine/src/game/interaction.rs Outdated
ChooseBranch,
SubmitLifeRedistribution,
ChooseDamageSource,
ChooseTextWordReplacement,

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Fix the serialized action discriminator mismatch.

With rename_all = "camelCase", this variant serializes as chooseTextWordReplacement, but the client contract expects "ChooseTextWordReplacement". The client will therefore reject or misclassify this action payload.

Use an explicit serde rename for this variant, or update both sides of the contract consistently.

🤖 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/types/interaction.rs` at line 338, Update the
ChooseTextWordReplacement enum variant’s serde serialization discriminator to
emit exactly "ChooseTextWordReplacement" despite the surrounding camelCase
convention, preserving the client contract.

@matthewevans

Copy link
Copy Markdown
Member

You did the port we owed you, correctly, after we told you to stop — thank you, and I'm sorry you had to. Reviewed at 7da1a6d0c. Two HIGH blockers, both in the untested paths I flagged last round; one of them makes the flagship use case of this card class inert.

First, the port

7682b5f7d8 merged main through 21a53d50cb, and 7da1a6d0ce bound text-word replacement into the interaction subsystem — 4 files, +15/−2. Verified by ancestry: a2db3e306b and 70e4f2d77e are both now in your history (non-vacuity probe passed against merge-base 21a53d50c), and crates/engine/src/game/interaction.rs exists at your head.

The port is minimal and you classified each variant by its shape rather than guessing. GameAction::ChooseTextWordReplacement correctly joined the OptionIndex group at interaction.rs:3964-3970 rather than sitting with the ObjectId-carrying ChooseDamageSource — that's the distinction that actually matters, since it carries index: usize, and it's the one a hurried port gets wrong. WaitingFor::TextWordReplacement bound like DamageSourceChoice across human_response_model (:266), classify_waiting_for (:494) and selection_projection (:3208); ContinuousModification::ReplaceTextWord into the None group at shared.rs:930; InteractionActionCode literal added and the TS binding regenerated.

There are no red jobs on this head — 11 checks queued, 3 success, 3 skipped, zero failures. The nine reds I misattributed were all on 0d5b04c09.

🔴 Blocker 1 — text-changing a static ability rewrites the AST but never changes what the ability does

This is the one I most want you to see, because it's an architectural trap this repo has hit before and documented, and it's invisible from the test you wrote.

The Layer-3 rewrite of obj.static_definitions can't influence the continuous effect that static generates. I traced every link:

  1. layers.rs:1840seed_live_characteristics_from_base(obj) re-seeds live characteristics from base, so static_definitions holds printed words at the top of each pass.
  2. layers.rs:1953let mut effects_by_layer = gather_active_continuous_effects(state);. The gather reads source.static_definitions.as_slice() (layers.rs:4057) — printed words.
  3. layers.rs:1957for (layer, layer_bucket) in &effects_by_layer consumes that frozen set. Your Layer-3 ReplaceTextWord rewrite at layers.rs:6149 mutates obj.static_definitions inside this loop, after the capture.
  4. Next pass returns to step 1 and the re-seed discards the rewrite.

I checked the obvious falsifier — is there a re-gather after Layer::Text? There isn't. effects_by_layer is assigned once (:1953), appended once for stickers (:1954), consumed at :1957. gather_active_effects_for_layer is called only for Layer::Copy (:1920, :3783, :6909).

The codebase already names this exact hazard and solved it for a sibling. layers.rs:4162-4165, verbatim:

"without this gather-time expansion, the layer-6 push onto obj.static_definitions would not appear in effects_by_layer (which is captured before layer 6 applies) and the inner static would be inert for a full pass."

That's GrantStaticAbility getting a bespoke gather-time expansion (expand_granted_static_effects, :4167) for precisely this reason. ReplaceTextWord needs an equivalent.

What it breaks are the headline plays: Magical Hack on Blood Moon (SetBasicLandType { Mountain }) — the AST says Island, the effect keeps making Mountains. Artificial Evolution on a tribal lord — the AST says Goblin, Elves keep getting the buff. These are the reasons these cards see play.

Roots 1–4 and 6 are live-read and genuinely work — your trigger test at text_changing_effects.rs:917 and sacrifice-cost test at :990 prove that behaviourally. static_definitions is the one broken root.

Why no test caught it: text_changing_effects.rs:317 (color_word_in_static_affected_filter_is_replaced) asserts collect_present_words before and after — it observes the field the fix writes, never the behaviour that field is supposed to drive. A test that settles it in one run: Bad Moon + Sleight of Mind (black→red), one black creature and one red creature on the battlefield, assert which one is 2/2.

Confidence: high. Pure code trace, four line-level confirmations, plus the repo's own comment naming the mechanism. Not executed — Tilt owns cargo here.

🔴 Blocker 2 — stack targets have no per-pass base re-seed, and the layer code says not to land this

layers.rs:1896-1901 re-seeds stack objects with obj.keywords = obj.base_keywords.clone() only. The comment directly above it (:1893-1895) reads verbatim:

"Extend the relevant reset authority before landing a static that modifies another stack characteristic."

walk_object_words (text_substitution.rs:364-409) mutates six roots: card_types.subtypes (:370), keywords (:375), abilities (:379), trigger_definitions (:386), static_definitions (:392), replacement_definitions (:404). Five of those six have no stack reset. sync_missing_base_characteristics (game_object.rs:1895-1898) only populates base when empty and early-returns once initialized — it does not revert live→base.

Your own layer comment states the missing precondition: layers.rs:6144-6148 says the swap "relies on the per-pass base re-seed + mark_full() re-derivation so each layer pass starts from printed values." True on the battlefield; not true on the stack.

7 of the 10 shipped cards template "target spell or permanent." A Duration::UntilEndOfTurn change (Crystal Spray) on a stack target has nothing reverting subtypes/abilities/definitions when the effect is pruned. All 37 casts in the test file target battlefield permanents.

Either extend the reset authority at :1896 to cover the roots walk_object_words touches, or explain why it's unnecessary — and add a stack-spell fixture either way.

🟡 Non-blocking

protection from [creature type] is silently unreachable. text_substitution.rs:649-655 puts ProtectionTarget::CardType(..) in the wordless no-op group, but keywords.rs:1531-1532 documents that the parser stores a protection subtype exactly there. So Artificial Evolution can't rewrite "protection from Goblins" — a creature type used as a creature type, which CR 612.2 (docs/MagicCompRules.txt:2936) says must change: "A text-changing effect changes only those words that are used in the correct way (for example, … a creature type word used as a creature type)." Only ProtectionTarget::Color is walked (:647) and only that is tested. Note the stored form is lowercase/plural, so a naive cursor.subtype call won't match.

Basic land dual-tap — one question decides whether this is a bug or a missing test. synthesize_basic_land_mana (synthesis.rs:447-476) pushes {T}: Add {R} into face.abilities for any face with subtype Mountain, called unconditionally from synthesize_all (:9019). Layer 3 rewrites the subtype Mountain→Island; Effect::Mana is correctly a no-op in the walker (text_substitution.rs:3523) per CR 612.2. Then apply_intrinsic_basic_land_mana_abilities (layers.rs:6847-6866) only adds, so {T}: Add {U} joins a surviving {T}: Add {R}. CR 305.6 (:1705) says an Island's intrinsic ability is {U} — only.

What makes this look like a real gap rather than an oversight: the sibling set_land_subtype_replacing (layers.rs:6834-6842) does clear obj.abilities, under CR 305.7 (:1707): "If an effect sets a land's subtype to one or more of the basic land types, the land no longer has its old land type. It loses all abilities generated from its rules text, its old land types…" ReplaceTextWord has no equivalent.

The question: does a Mountain loaded from real card data actually carry {T}: Add {R} in abilities, or is it purely intrinsic? A second read of this suggested the explicit ability is baked only by the GameScenario::add_basic_land harness. That single fact decides it — please answer it with a fixture built from real card data, not add_basic_land, which would give a misleading result either way. Affects the 6 BasicLandType cards: Mind Bend, Magical Hack, Spectral Shift, Trait Doctoring, Whim of Volrath, Crystal Spray.

~600-option choice list, and the doc claims otherwise. effects/text_change.rs:104-117 builds |present words| × |all_creature_types| options, each with an allocated label. state.all_creature_types is the full printed vocabulary — card_db.rs:52-59 says "every printed creature type, not just the subset present in the loaded decks". A two-type creature under Artificial Evolution yields ~600. game_state.rs:9243 claims "even for the creature-type category the product stays small in practice", which the code it documents doesn't support. Downstream: AI branching ~600 (candidates.rs:1574-1583), ~600 heap Strings per search node and per publish, and CardChoiceModal.tsx renders one motion.button per option at delay: 0.05 + index * 0.03 — the last button animates in around 18 seconds, with no filtering or virtualization. The flat list is right for colors (5) and land types (5); it doesn't survive creature types.

ability_scan.rs:409 returns Axes::NONE for ChangeTextWords without scanning its target filter, though the effect has a target and target_ctx is computed at :406 and left dead. Siblings Effect::Destroy (:491-497) and Effect::Bounce (:763-769) scan theirs; the stack-targeting sibling Effect::Counter (:509) is Axes::CONSERVATIVE. ChangeTextWords targets the stack like Counter but takes the least conservative value — scan it, or fail closed.

excluded_to has zero runtime coverage — deleting || excluded_to.contains(to) at effects/text_change.rs:107 fails nothing. That's Artificial Evolution's entire "can't be Wall" restriction.

"All instances" is never asserted. CR 612.1 (:2934) is about replacing instances of a word; every fixture buries exactly one word and clamps all_creature_types to 2–3 entries across 20 sites, so only one from is ever legal. No test puts the same word in two roots.

Vacuity guard: partial_excluded_to_rider_does_not_parse (:2546, :2558) asserts .all() over a vec that would be true if empty. Non-vacuous today; one line (assert_eq!(snapshots.len(), 1)) makes it stay that way.

Drop the CR 612.1 annotation at interaction.rs:3966 — CR 612.1 describes text-changing effects, not payload projection, and CLAUDE.md says not to annotate serialization plumbing.

TextWord::label() (types/ability.rs:1099-1106) returns hardcoded English ("White", "Blue"), so every option label is untranslatable even though you added the modal title/subtitle keys to all 7 locales.

✅ Credit

The CR discipline here is the best I've seen on this repo from a contributor, and I say that having got three citations wrong myself last round. The ones I re-checked all resolve and describe their code — including CR 613.1c (:2964), "Layer 3: Text-changing effects are applied", which matches the existing sibling SetChosenName at types/layers.rs:88-91. My corrected citation was right and yours agreed with it.

WordCursor / walk_object_words is a genuine building block rather than a special case — a visitor over six word-carrying roots with 39 walk functions, correctly category-gated so a color change doesn't retype a token, and honouring CR 612.2a's creature-type/name carve-out only when the printed spec declares that type. The tests drive the real production path (runner.act()apply_as_current, real cast/stack/activate/pay) with explicit reach guards.

And your answer on finish_with_continuation was the right one — I'm withdrawing that ask. You routed it through the shared authority (engine_resolution_choices.rs:5836) and then posted a negative result: nested AbilityContinuation frames merge into one, your parked RepeatFor fixture passed with the fix reverted, so you deleted it. I independently confirmed :1225 is non-discriminating — it pre-dates the fix byte-identically and both assertions pass either way. Deleting a test that passes either way, and saying so, is exactly the behaviour I want to see. I'll stop presenting :1225 as its guard.

CodeRabbit — 2 of 3 refuted

  • "Expose replacement choices, not only their indices" — refuted. Labels do reach the client: WaitingFor::TextWordReplacement carries options: Vec<TextWordReplacementOption> with an engine-computed label (game_state.rs:9250-9265), mirrored at adapter/types.ts:1605-1611,1676 and rendered verbatim by TextWordReplacementModal. It reasoned only about the interaction projection, which deliberately doesn't enumerate option payloads for any of its ~45 label-bearing siblings.
  • "Serialized action discriminator mismatch" — refuted. InteractionActionCode carries both #[serde(rename_all = "camelCase")] and ts(rename_all = "camelCase") (types/interaction.rs:283-286), so the two can't disagree; the generated TS has "chooseTextWordReplacement" and zero of the PascalCase form, and check-interaction-bindings.sh regenerates and diffs in CI.
  • "Remove the incorrect CR citation" — confirmed, listed above.

Recommendation: request changes on the two HIGH blockers. Blocker 1 is the substantive one and I'd fix it first — a gather-time expansion for ReplaceTextWord mirroring expand_granted_static_effects, plus the Bad Moon / Sleight of Mind test. Blocker 2 needs the reset authority extended or a reasoned explanation, plus a stack-spell fixture. The basic-land question needs one answer from you before I'd call it a defect at all.

Please regenerate the parse-diff on the current head when you push — the sticky is from 2026-07-15, two pushes ago, though its claimed-equals-measured 10 cards still matches.

… comment

Per CodeRabbit review: the comment above the ChooseTextWordReplacement action
projection described protocol shaping (the answer is an index into the
engine-computed option list), not a rules implementation, so the CR 612.1
citation didn't belong there.
@matthewevans

Copy link
Copy Markdown
Member

One of the 🟡 items is fixed and the parse-diff ask is now closed. Blockers 1 and 2 are still the only gate — neither has been attempted. Re-reviewed at head 495e89dc49.

You pushed while I was writing this, so I re-ran everything against the new head rather than carrying the earlier verdict forward.

✅ The CR 612.1 removal is right

495e89dc49 replaces the // CR 612.1: line above GameAction::ChooseTextWordReplacement { index } (interaction.rs:3966-3970) with a comment that explicitly disclaims itself as a rules citation. Grep-verified: MagicCompRules.txt:2934 reads "Some continuous effects change an object's text… Such an effect is a text-changing effect." That describes text-changing effects, not a push_value_surface payload projection — and CLAUDE.md says not to annotate serialization plumbing at all.

Removing it rather than relocating it was the correct call. A citation that doesn't describe its code is worse than no citation, because it creates false confidence that the surface was checked against a rule it has nothing to do with. That 🟡 is closed.

✅ The parse-diff ask is closed — verified at this head

Heads-up before you read the sticky on this PR: it's stale. It still shows baseline main 6a3d080aee3a from 02:00:20Z because the sticky workflow hasn't reposted for the new head. Don't read that as current.

I pulled the artifact from the head CI run directly (run 30062614329, artifact 8585189737) and diffed it against the previous one:

  • The clusters are byte-identical to the previous head — same 8 signatures, same per-cluster card lists, same counts.
  • 10 distinct cards == your claimed 10: Alter Reality, Artificial Evolution, Crystal Spray, Glamerdye, Magical Hack, Mind Bend, Sleight of Mind, Spectral Shift, Trait Doctoring, Whim of Volrath. Verified against Scryfall Oracle text — categories, duration, target scope, and Artificial Evolution's excluded to=Wall all match.
  • added_cards: [], removed_cards: [].
  • Zero blast radius. All six deliberately-deferred cards (New Blood, Balduvian Shaman, Deceptive Divination, Magical Hacker, Overload, March of Progress) are absent, so coverage stays honestly red exactly as your PR body says.
  • Spectral Shift's double-clustering (ColorWord + BasicLandType) is legitimate — the card is genuinely modal, so two modes means two signatures and one card. Not a defect.

The only field that moved anywhere in the artifact is oracle_changed: 1 → 2, and that's baseline drift — the baseline advanced from 6a3d080aee3a to 7a95408fe0a1 and one unrelated card picked up an errata in between. Your delta touches no parser code, so it can't have caused it.

🔴 Blockers 1 and 2 — re-traced at this head, byte-identical

I re-ran the greps rather than inferring from the diff.

Blocker 1 — the Layer-3 rewrite is inert for a full pass. The call sites in layers.rs are unchanged line-for-line: 1920, 1953, 3775 (defn), 3783, 3847 (defn), 6921. gather_active_continuous_effects is still called exactly once, at :1953; gather_active_effects_for_layer is still reached only from Layer::Copy sites. No re-gather after Layer::Text was added, and no gather-time expansion for ReplaceTextWord. :1684 still re-seeds obj.static_definitions from base_static_definitions every pass, and ReplaceTextWord still mutates it at :6149 — after the capture at :1953.

The concrete consequence is unchanged: Magical Hack on Blood Moon and Artificial Evolution on a tribal lord still do nothing. Your codebase already names this exact hazard at layers.rs:4162-4165 for GrantStaticAbility and solves it with expand_granted_static_effects at :4167 — that's the pattern to mirror.

Blocker 2 — stack objects reset one of six mutated roots. layers.rs:1896-1901 still does sync_missing_base_characteristics() then obj.keywords = obj.base_keywords.clone() and nothing else, against the six roots walk_object_words mutates (text_substitution.rs:370/375/379/386/392/404). revert_layered_characteristics_to_base() still has exactly three call sites — zones.rs:451, zones.rs:490, stickers.rs:106none in the stack reset loop, so the Root-6 comment at text_substitution.rs:402 claiming a per-pass re-seed for stack objects is still wrong. The instruction at layers.rs:1893-1895 ("Extend the relevant reset authority before landing a static that modifies another stack characteristic") is still unmet.

Test gap, still measurable: rg -c "Zone::Stack|on_stack" over tests/integration/text_changing_effects.rs returns zero across 37 cast sites, while 7 of your 10 cards template "target spell or permanent."

🟡 Low, unchanged

tests/integration/text_changing_effects.rs:1425-1428 asserts spectral.len() == 2 with the message "must lower to two ChangeTextWords modes", but collect_change_text (:1276-1294) flattens across ChooseOneOf { branches }, sub_ability, else_ability and mode_abilities. So len() == 2 is satisfied identically by one modal ability with two branches and by two independent always-applying abilities — and the latter would apply both text changes unconditionally (CR 700.2) with this test still green.

This is a test-adequacy point, not a bug report: modal parsing is pre-existing infrastructure your diff doesn't touch, so the modal shape is almost certainly what's produced. Asserting the shape — one top-level ability whose effect is ChooseOneOf with two branches — would make it discriminating.

📋 Operational

Your branch is now BEHIND main and will need an update before it can merge, independent of the review. Say the word and I'll run it — the baseline moved under you, so that's ours, not yours.


Recommendation: changes requested, on Blockers 1 and 2 only. Everything else in this PR — the parse impact, the deferrals, the CR annotations, the AI and multiplayer wiring — I've now verified clean across two heads. The two blockers are both about the same thing: the layer pass captures its effect set before your text change has been applied, and the stack reset doesn't undo five of the six roots you mutate. Fixing those two is the whole remaining distance.

# Conflicts:
#	crates/engine/src/parser/oracle_ir/doc.rs
…e CR 612 walker

Main advanced under the branch again. The walker's exhaustive matches require:
- Effect::ChoosePermanent { filter } — the Metamorphic Alteration "choose a
  creature" filter can name a creature type used as a creature type (CR 612.2),
  so walk_effect rewrites it; target_filter_mut returns None to stay paired with
  target_filter (the choice is an as-enters replacement, not a stack target).
- ContinuousModification::CopyChosen — a runtime copy of the chosen object's
  values (CR 707.2), no printed word, no-op.
Plus the additive doc.rs merge (both sides added a no-op printed-slot arm).
…nt (carrier)

New upstream variant. A granted replacement effect (CR 613.1f layer-6
ability-adding) carries the same word-bearing condition/event/replaced-effect
filters as any other ReplacementDefinition (e.g. a subtype-scoped check-land
condition), so the CR 612 walker routes it through walk_replacement_definition,
mirroring GrantStaticAbility. Also merges latest main.
jsdevninja pushed a commit to jsdevninja/phase that referenced this pull request Jul 24, 2026
…7-23 sweep (phase-rs#6576)

* docs(pr-review-loop): encode review-sweep defects found in the 2026-07-23 sweep

Four failure modes from a live authorized-maintainer sweep, each of which
produced a defective posted review:

- Dispatching Review Agents (new): a subagent's final assistant text is not
  delivered to the lead — findings arrive only via SendMessage, and charters
  must say so verbatim. Idle means resumable, not dead. Do not substitute a
  lead-authored review for a slow agent; three of the sweep's four bad reviews
  came from doing exactly that, and each was overturned by the agent's report
  minutes later.

- Verifying the Review's Own Evidence (new): the CR grep-verification bar binds
  the reviewer as hard as the contributor, since the contributor acts on what
  the review says. Check repo convention before proposing a citation, and read
  the rule rather than trusting a number in roughly the right section —
  adjacent subrules routinely cover unrelated single cards (612.5 is Exchange
  of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a
  different section from the effect it orders (613.1c, not 612.x).

- Judging the seam (new, under Review Bar): "reuses existing machinery" is not
  "correct seam". Every proliferation heuristic in this file returned clean on
  a PR that reused the instance-blind checker while the instance-aware
  authority twenty lines away already cited the rule and dropped half of it.
  Grep for the rule, not the symbol; two mechanisms for one rule is the defect.

- Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's
  own new variants look like contributor sloppiness and are frequently ours —
  main adds a new match site after the contributor's last push. Timestamp the
  failing site against the head before writing that finding.

- Review Freshness: stale approval plus armed auto-merge is a safety defect,
  not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting
  APPROVED after a new commit lands, so an unreviewed head can sit armed to
  merge. Worse when the post-approval commit is maintainer-authored.

* docs(pr-review-loop): test compile-error attribution by ancestry, not timestamps

Addresses @coderabbitai review on phase-rs#6576.

The finding is correct: `.commit.committer.date` is the head commit's author/
committer metadata, not push time, so a rebase, an amend, or a delayed push all
skew a date comparison — and a squash-merged main commit carries a date
unrelated to when its content actually landed.

The suggested remedy (read push time from PR timeline/update events) would fix
the skew but still answers the wrong question. What the check actually needs to
know is not "which is newer" but "did the branch ever contain the failing
code?" — which `git merge-base --is-ancestor` answers directly and is immune to
the entire class of timestamp problems, including ones PR timeline events also
get wrong.

Replaces the three date greps with:
  1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all
  2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit
     that introduced the match site in the branch's history
  3. the unchanged PR file-list check

Also adds a non-vacuity probe, because a check that returns "not an ancestor"
for every input is broken rather than exonerating: a commit known to be in the
branch's history (`git merge-base origin/main <headOid>`) must report ancestor.

Verified verbatim against the case that motivated the section (phase-rs#5866): step 1
reports interaction.rs absent at the head, step 2 reports the shared.rs match
site not in the branch's history, and the probe reports the real merge-base
df2ab2d as an ancestor — so the test discriminates rather than always
failing closed.

Timestamps stay as human-readable colour in the review comment, relabelled
"committed at" rather than "pushed at".

---------

Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>

@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.

Changes requested — the text-change authority is not yet behaviorally complete.

🔴 Blocker

[HIGH] Rebuild rewritten static effects at the layer gather seam. Evidence: crates/engine/src/game/layers.rs:1962-1966 gathers effects before static_definitions are read at :4062-4068, while ReplaceTextWord mutates them only at :6161-6171. A rewritten static ability is therefore inert until the next reset. Add behavioral coverage for a static-ability case.

[HIGH] Reset every stack characteristic that the text walker changes. Evidence: layers.rs:1888-1910 resets only stack keywords, whereas game/text_substitution.rs:364-408 also mutates subtypes, abilities, triggers, static definitions, and replacement definitions. A stack text change can persist or accumulate after its duration. Extend the reset authority (or disallow stack targets) and add a duration-expiry regression.

Recommendation: request changes; re-run the static and stack behavioral cases on the revised head.

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.

2 participants