Skip to content

feat(parser): compound bare-color cost-mod subject "<color> spells and <color> spells cost less" (Prophecy Familiar cycle)#5138

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_familiar_colors
Jul 5, 2026
Merged

feat(parser): compound bare-color cost-mod subject "<color> spells and <color> spells cost less" (Prophecy Familiar cycle)#5138
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_familiar_colors

Conversation

@minion1227

@minion1227 minion1227 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds parser support for compound bare-color cost-modification subjects — "<color>[ spells] and <color>[ spells] cost {N} less to cast" — used by the Prophecy Familiar cycle (Nightscape, Stormscape, Sunscape, Thornscape, Thunderscape Familiar). The two-bare-color compound subject fell through both the single-bare-color path and parse_type_phrase (which only decomposes compounds whose operands carry a type noun), so the subject resolved to None and the color restriction was silently dropped — cheapening every spell instead of only the two colors. This is a rules bug, not just a coverage miss. Decomposes the subject into the same Or of HasColor typed filters the noun-bearing compounds already produce. No new variant, no new runtime.

Files changed

  • crates/engine/src/parser/oracle_static/static_helpers.rs
  • crates/engine/src/parser/oracle_static/tests.rs

CR references

  • CR 105.1 — the five colors (the bare-color operands)
  • CR 601.2f — cost modification applied while casting the affected spells

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high

Verification

Card-level before/after (full parse_oracle_text pipeline, Nightscape Familiar, run on both refs):

ref ModifyCost.spell_filter effect
baseline main 8d46028 None reduces every spell you cast (color restriction dropped — the bug)
this head c95971e Some(Or{ Typed{Card, HasColor Blue}, Typed{Card, HasColor Red} }) reduces only blue/red spells you cast (correct)

Note: the coverage-parse-diff sticky reports "No card-parse changes" here — a false negative. Its per-card signature captures the ModifyCost mode + affected filter but not the contents of spell_filter, the field this PR populates. The card data genuinely changes (table above); the diff tool just can't see inside spell_filter. See the pinned comment for the full parse dumps.

CI on this head — all green:

  • Rust lint (fmt, clippy, parser gate) — ✅ SUCCESS (fmt, clippy -D warnings, parser-combinator gate)
  • Rust tests (shard 1/2 + 2/2) — ✅ SUCCESS; includes static_compound_bare_color_spells_cost_less_narrows_to_or_of_colors asserting the Or{HasColor Blue, HasColor Red} spell_filter
  • Card data (generate, validate, coverage) — ✅ SUCCESS
  • WASM / Tauri / Frontend / Lobby worker — ✅ SUCCESS

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

@minion1227 minion1227 requested a review from matthewevans as a code owner July 5, 2026 11:47

@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 introduces support for parsing compound bare-color cost-modification subjects (e.g., 'Blue spells and red spells') in static abilities, mapping them to an Or of HasColor filters. This change correctly handles the Prophecy Familiar cycle and prevents color restrictions from being silently dropped. A unit test has been added to verify this behavior. I have no feedback to provide as there are no review comments and the implementation is clean and compliant with the repository's guidelines.

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.

@matthewevans

Copy link
Copy Markdown
Member

Initial code pass did not turn up a blocker in the touched parser seam, but I am holding approval/enqueue until the parser/card-level evidence exists for this head. This PR changes parser source, and right now the parse-diff sticky is absent while card-data is still running, so I cannot verify the card-level blast radius yet.

Please also fill in the PR proof sections so the summary, files changed, track, LLM usage, and verification are visible from the PR itself.

…d <color> spells cost less" (Prophecy Familiar cycle)

The Familiar cycle — Nightscape ("Blue spells and red spells you cast cost {1}
less to cast"), Stormscape, Sunscape, Thornscape, Thunderscape Familiar — used a
two-BARE-color compound subject that fell through both the single-bare-color path
(parse_named_color) and parse_type_phrase (which only decomposes compounds whose
operands carry a type noun). The subject resolved to None, silently DROPPING the
color restriction so EVERY spell was cheapened, not just the two colors.

Add parse_cost_mod_compound_color_subject in oracle_static/static_helpers.rs:
decompose "<color>[ spells] and <color>[ spells]" (>=2 colors, full consumption)
into the same Or of HasColor typed filters the noun-bearing compounds already
produce. A lone bare color and noun-bearing operands both decline and fall
through unchanged. No new variant, no new runtime.

CR 105.1 (colors) + CR 601.2f (cost modification).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@minion1227 minion1227 force-pushed the minion_familiar_colors branch from ea8dcd0 to c95971e Compare July 5, 2026 11:52
@minion1227

Copy link
Copy Markdown
Contributor Author

Filled in the PR proof sections (Summary / Files changed / CR references / Track / LLM / Verification / Scope Expansion / Validation Failures / CI Failures) on this head (c95971e2c).

On the parser/card-level evidence: this was previously behind on main, so I rebased onto 8d46028b9 and CI is re-running. Rust lint (fmt, clippy, parser gate) and the WASM/Tauri/frontend/lobby checks are already ✅ green; Card data (generate, validate, coverage) is still running, and its coverage-parse-diff sticky will auto-post the card-level blast radius when it completes — expected to be the 5 Prophecy Familiar-cycle cards (Nightscape / Stormscape / Sunscape / Thornscape / Thunderscape Familiar) narrowing their cost-reduction from all-spells to their two colors (the Or{HasColor,HasColor} fix). Ready for another look once card-data lands.

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans

Copy link
Copy Markdown
Member

The current head is green, but I am holding this rather than approving because the card-level evidence now contradicts the PR's stated impact.

The PR says this should narrow the five Prophecy Familiar-cycle cards, and the author follow-up expected those five card parses to change. The current coverage-parse-diff sticky for head c95971e2cfe99b5ce844d3579742791b9b23aaf4 says: No card-parse changes detected. That means the required card-level blast-radius evidence does not show the claimed fix affecting real card data.

Please either explain why the real card parse output is unchanged despite the bug claim, or adjust the PR to make the intended card-level change visible. A parser unit test alone is not enough for approval when the claimed production card impact is absent from the parse-diff evidence.

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

[MED] The PR's claimed card-level fix is not present in the parse-diff evidence. Evidence: the current coverage-parse-diff sticky for head c95971e2cfe99b5ce844d3579742791b9b23aaf4 says No card-parse changes detected, while the PR body/follow-up says the five Prophecy Familiar-cycle cards should narrow from all spells to their two colors. Why it matters: for parser-source changes, the card-level diff is required review evidence; a unit test alone does not prove the claimed production card impact. Suggested fix: either make the intended real card parse change visible in parse-diff, or explain/retitle the PR as a regression-test-only change with evidence for why current production card data is already correct.

@minion1227

Copy link
Copy Markdown
Contributor Author

The real card parse output is changed — the coverage-parse-diff "No card-parse changes detected" is a false negative for this fix. Its per-card signature captures the ModifyCost mode + affected filter but not the contents of ModifyCost.spell_filter, which is exactly the field this PR populates. So a spell_filter: None → Or{...} change is invisible to the signature even though the parsed card genuinely changes.

I verified the actual card-level blast radius by running the full parse_oracle_text pipeline on Nightscape Familiar ("Blue spells and red spells you cast cost {1} less to cast.") against both refs:

Baseline main (8d46028) — buggy:

ModifyCost { mode: Reduce, amount: {1}, spell_filter: None }
affected: Typed { type_filters: [Card], controller: You }

spell_filter: None ⇒ the reduction applies to every spell the controller casts — the printed "Blue spells and red spells" restriction is silently dropped.

This PR head (c95971e) — fixed:

ModifyCost { mode: Reduce, amount: {1}, spell_filter: Some(Or {
    Typed { type_filters: [Card], properties: [HasColor { Blue }] },
    Typed { type_filters: [Card], properties: [HasColor { Red }] },
}) }
affected: Typed { type_filters: [Card], controller: You }

spell_filter now narrows to blue OR red spells only. The same delta applies to the other four cards in the cycle (Stormscape = white/black, Sunscape = green/blue, Thornscape = red/white, Thunderscape = black/green).

So the fix does change real card data; the parse-diff simply can't see inside spell_filter. Reproducible by parsing the line through parse_oracle_text on each ref, or via the unit test static_compound_bare_color_spells_cost_less_narrows_to_or_of_colors (asserts the Or{HasColor Blue, HasColor Red} spell_filter). Happy to add a card-level regression assertion if that's the preferred form of evidence.

@matthewevans matthewevans self-assigned this Jul 5, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the follow-up against head c95971e2cfe99b5ce844d3579742791b9b23aaf4.

The earlier block is resolved: the coverage parse-diff sticky is blind to StaticMode::ModifyCost.spell_filter, and the PR's code/test prove the real parser output changes from spell_filter: None to an Or of HasColor filters for the Prophecy Familiar bare-color compound subject. The change is local to the existing cost-mod parser seam, uses existing typed filters and nom parsing, CI is green, and the added parser-pipeline assertion would fail if the color restriction were still dropped.

@matthewevans matthewevans added the bug Bug fix label Jul 5, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 5, 2026
@matthewevans matthewevans removed their assignment Jul 5, 2026
Merged via the queue into phase-rs:main with commit f1cc915 Jul 5, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants