Skip to content

Fix Approach of the Second Sun: filtered SpellsCastThisGame + correct gating - #398

Merged
matthewevans merged 9 commits into
phase-rs:mainfrom
Erckdd:wip/spells-cast-this-game-refactor
May 15, 2026
Merged

Fix Approach of the Second Sun: filtered SpellsCastThisGame + correct gating#398
matthewevans merged 9 commits into
phase-rs:mainfrom
Erckdd:wip/spells-cast-this-game-refactor

Conversation

@Erckdd

@Erckdd Erckdd commented May 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Approach of the Second Sun was handing the player a win on the very first cast. The card prints:

If this spell was cast from your hand and you've cast another spell named Approach of the Second Sun this game, you win the game. Otherwise, put Approach of the Second Sun into its owner's library seventh from the top and you gain 7 life.

Three failures stacked up:

  1. The parser couldn't parse the compound "this spell was cast from your hand and you've cast another spell named X this game" condition, so WinTheGame came out with condition: null and fired unconditionally.
  2. There was no game-scope name-filtered history to evaluate the second half of the gate against — only an unfiltered per-player count.
  3. The chunk splitter peeled "…and you gain 7 life" off the Otherwise body as a sibling clause, so even after chore: update coverage stats and badges #1 the gain-life half mis-attached to the win effect.

This PR lifts and lands all three.

What changed

Engine variant: QuantityRef::SpellsCastThisGame lifted to { scope, filter }

Mirrors the existing SpellsCastThisTurn shape (same parameterization axes within CR 117.1 — passed the add-engine-variant gate). filter: None keeps the fast O(1) state.spells_cast_this_game path used by Establishing Shot's "this is the first spell you've cast this game". filter: Some(_) scans the new game-scope history.

Game-scope history + name capture

  • New SpellCastRecord.name: String (with #[serde(default)] for backwards-compat snapshots).
  • New GameState.spells_cast_this_game_by_player: HashMap<PlayerId, Vec<SpellCastRecord>>, populated alongside the per-turn list in record_spell_cast_from_zone. Not cleared between turns.
  • FilterProp::Named { name } now matches against the cast-time snapshot (case-insensitive), unblocking name-filtered queries on spell history.

Parser: compound condition recognition

  • parse_youve_cast_another_named_this_game_condition recognises you've cast another spell named {LITERAL} this gameQuantityCheck(SpellsCastThisGame{filter=Named} >= 2). The >= 2 comparator (not 1) matches parse_another_spell_cast_this_turn's minimum: 2 convention — at resolution time the currently-resolving spell is already in the cast history, so "another" means at least 2 total.
  • The existing \"this spell was cast from X\" handler now accepts an optional \" and {recognised condition}\" tail and returns AbilityCondition::And. Partial recognition (zone-half OK, second-half unknown) is rejected on purpose — running with only half the gate is unsafe.

Splitter guard: Otherwise, X and Y stays together

Bare-and split suppressed when the chunk starts with otherwise, / otherwise (mirroring starts_prefix_clause). Without it, "Otherwise, put ~ 7th from top and you gain 7 life" splits at the and, the gain-life chunk attaches as a sibling sub_ability of the win effect, and the Otherwise body loses half its content.

Result

Approach now parses to:

WinTheGame
  condition: And { CastFromZone(Hand), SpellsCastThisGame{filter=Named \"approach of the second sun\"} >= 2 }
  else_ability: PutAtLibraryPosition(7th) -> GainLife(7)
  • First cast — condition false (only the resolving cast is in history). Otherwise fires: library 7th + gain 7 life.
  • Second cast — condition true (2 named-Approaches in history). Win the game.

Test plan

  • cargo test -p engine --lib (7146 tests, all green)
  • cargo clippy -p engine --all-targets -- -D warnings (clean)
  • cargo fmt --check
  • Parser regression test (approach_of_the_second_sun_parses_compound_condition_and_otherwise_branch) asserts the full parsed shape and the literal FilterProp::Named { name: \"approach of the second sun\" } value — fed un-tilde'd "named X" text the way normalize_card_name_refs' "named ~" → "named CardName" restore produces in production.
  • Resolver test (resolve_quantity_spells_cast_this_game_filtered_by_name) exercises the runtime against spells_cast_this_game_by_player.
  • End-to-end pipeline test (approach_of_the_second_sun_round_trips_through_record_spell_cast) runs the full path: record_spell_castSpellCastRecord.name populated → resolve_quantity finds the named-filter count, plus controller-scope isolation against an opponent's same-named casts.
  • Card-data regeneration confirms Approach's emitted condition is And { CastFromZone(Hand), SpellsCastThisGame{filter=Named(\"approach of the second sun\")} >= 2 }.

🤖 Generated with Claude Code

Erckdd and others added 4 commits May 13, 2026 21:00
…ilter}

Parks in-progress refactor off main so it doesn't block other work.
Lifts the unit variant `QuantityRef::SpellsCastThisGame` to a struct
variant `{ scope: CountScope, filter: Option<TargetFilter> }`, mirroring
the parameterization shape of `SpellsCastThisTurn`. Updates the
definition, helpers, and all known call sites (resolver, parser,
condition matcher, coverage report, restrictions, triggers).

Snapshot of the working tree at the time of the move; not verified end
to end. Resume by running clippy + tests and reconciling whatever the
agent driving the refactor has in mind for the open ends.
…ion tests

The lifted `SpellsCastThisGame { filter }` quantity is correct, but the
parser was emitting `>= 1` for "you've cast another spell named ~ this
game". At resolution time the currently-resolving Approach is already in
`spells_cast_this_game_by_player`, so the printed "another" semantic maps
to total count `>= 2` (mirroring `parse_another_spell_cast_this_turn`'s
`minimum: 2` convention).

Without this, the first Approach cast satisfies the gate and hands the
player the game on its very first resolution. With the fix:

  * First cast — count = 1, gate false → Otherwise branch fires
    (library 7th, gain 7 life).
  * Second cast — count = 2, gate true → win the game.

Also rewrites the `"and "` prefix probe inside the compound condition
recogniser to use a nom `tag` call instead of `strip_prefix`, satisfying
the parser-combinator gate (CLAUDE.md / PATTERNS.md Pattern 1).

Tests:
  * `approach_of_the_second_sun_parses_compound_condition_and_otherwise_branch`
    locks in the full parse: top-level `WinTheGame`, `And` condition with
    `CastFromZone(Hand)` + `QuantityCheck(SpellsCastThisGame{filter=Named}
    >= 2)`, and the Otherwise branch chained as
    `PutAtLibraryPosition -> GainLife(7)` on `else_ability` with no
    spurious sub_ability on the win effect.
  * `resolve_quantity_spells_cast_this_game_filtered_by_name` exercises
    the runtime resolver against `spells_cast_this_game_by_player`: name
    filter matches only the prior Approach record (not Lightning Bolt),
    and a second Approach cast brings the count to 2 so the `>= 2` gate
    holds.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…leanups, end-to-end test

Six fixes pulled from /review-impl on the Approach commit:

1. CR 104.2a -> 104.2b in the parser-regression doc comment. 104.2a is the
   "all opponents have left" win condition; Approach is a 104.2b "an
   effect states a player wins the game" card.

2. Parser regression test now feeds the *literal* card name in the
   "named X" slot (matching what `normalize_card_name_refs`' "named ~"
   -> "named CardName" restore at `oracle_util.rs:1553` produces in
   production) and asserts `FilterProp::Named { name }` equals
   "approach of the second sun". The pre-fix shape-only assertion would
   pass even if the restore step silently regressed and emitted
   `FilterProp::Named { name: "~" }`.

3. New cast-pipeline integration test
   `approach_of_the_second_sun_round_trips_through_record_spell_cast`
   exercises the full path: `record_spell_cast_from_zone` ->
   `SpellCastRecord.name` populated on the game-scope history ->
   `resolve_quantity` against `QuantityRef::SpellsCastThisGame` with the
   parser-shaped name filter. The earlier resolver test hand-populated
   `spells_cast_this_game_by_player` and bypassed the pipeline; this one
   fails if any future alt-cast path forks the recording flow without
   re-invoking `record_spell_cast_from_zone`, or if the `name` capture
   regresses. Also asserts controller-scope isolation against an
   opponent's same-named casts.

4. `parse_youve_cast_another_named_this_game_condition` now consumes the
   trailing " this game" anchor via `(take_until(" this game"),
   tag(" this game"))` instead of `rest.find(anchor)`. The combinator
   gate had let the `.find()` through because `anchor` is a variable —
   matches PATTERNS.md Pattern 5.

5. `inside_otherwise_body` in `split_clause_sequence` tightened from
   `tag("otherwise")` to `alt((tag("otherwise, "), tag("otherwise ")))`,
   mirroring the otherwise-prefix shapes in `starts_prefix_clause`. The
   looser form would have suppressed splitting on any chunk whose first
   word merely shared the letters "otherwise".

6. `trimmed.starts_with(['.', ','])` fallback in
   `try_nom_condition_as_ability_condition` carries an
   `allow-noncombinator` annotation explaining that the char-class
   check is a structural punctuation guard on already-tokenized text,
   not parsing dispatch (PATTERNS.md section 9).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@matthewevans

Copy link
Copy Markdown
Member

/gemini review

@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 game-scope spell cast history tracking and name-filtered counting, primarily to support cards like Approach of the Second Sun. It introduces a name field to SpellCastRecord, adds spells_cast_this_game_by_player to the GameState, and extends the parser to handle "you've cast another spell named {LITERAL} this game" conditions. My feedback focuses on several architectural violations: the use of .starts_with() for parsing dispatch (violating R1), the use of standard Vec instead of im::Vector for a hot GameState field (violating R7), and two instances where SpellCastRecord is initialized with an empty name in casting logic, which would break name-filtered restrictions.

Comment thread crates/engine/src/game/casting.rs Outdated
Comment thread crates/engine/src/game/casting.rs Outdated
Comment thread crates/engine/src/parser/oracle_effect/conditions.rs Outdated
Comment thread crates/engine/src/types/game_state.rs Outdated
Comment thread crates/engine/src/game/restrictions.rs Outdated
Comment thread crates/engine/src/game/restrictions.rs Outdated
@matthewevans
matthewevans enabled auto-merge (squash) May 15, 2026 02:03
@matthewevans
matthewevans merged commit 130dc27 into phase-rs:main May 15, 2026
3 checks passed
matthewevans added a commit that referenced this pull request May 15, 2026
Post-merge review of PR #398. Three surgical fixes; no behavior change.

* Doc comment at the `and [second condition]` branch was two clauses
  mashed into one ungrammatical sentence — rewritten to clearly state
  that partial recognition (when `and ` parses but the second condition
  fails) deliberately returns None rather than binding bare CastFromZone.
* `parse_youve_cast_another_named_this_game_condition` was using a raw
  parser tuple with `.ok().map()` to project out the first slot — that
  is exactly what `nom::sequence::terminated` does. Replaced with
  `terminated(take_until(...), tag(...))` to match the idiom used
  throughout the rest of the parser.
* Dropped the unused `_after_anchor` binding for the same reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants