fix(parser): keep granted-ability description case when stripping turn conditions#5603
Merged
Merged
Conversation
…t anthems Closes phase-rs#5599. CR 604.1: A granted ability's `description` is a serialized, user-visible field (`AbilityDefinition.description` → `card-data.json`, rendered in the client). Two static-parser paths stripped a trailing turn-window condition from the LOWERCASED predicate and then fed that lowercase buffer to the continuous- modification parser — so the granted ability's description leaked lowercased. Brightcap Badger ("Each Fungus and Saproling you control has \"{T}: Add {G}.\"") rendered its grant as "{t}: add {g}." instead of "{T}: Add {G}.". Fix: pass the ORIGINAL-case predicate to `strip_suffix_turn_condition` in both `parse_subject_continuous_static` (anthem.rs) and the self-static predicate arm (dispatch.rs), matching what the `. Otherwise` grammar path already does. The condition phrase ("during your turn" / "during turns other than yours") is lowercase in the original text too, so the suffix still matches and the retained predicate keeps its printed case. Single-subject anthems ("Tokens you control have ...") were already correct and are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 87 card(s), 122 signature(s) (baseline: main
|
matthewevans
approved these changes
Jul 11, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved — the right seam, and the pool-level evidence proves it costs nothing. This restores an existing contract rather than special-casing a card, which is why it fixes 87 cards instead of one.
✅ Clean
Right seam — you restored the TextPair contract instead of patching the symptom. Both callees lowercase internally:
let lower = text.to_lowercase();
let tp = TextPair::new(text, &lower); // parse_continuous_gets_has (anthem.rs:824)
// parse_continuous_modifications (keyword_grant.rs:1230)Their contract is therefore "hand me original-case text." Passing a pre-lowercased buffer did not merely duplicate work — it destroyed TextPair.original, and that is precisely the field the serialized, user-visible description is built from. That is the whole bug class, and it explains why the leak was invisible to the parse logic yet visible on the card. The third caller (grammar.rs:657) already passed original-case; this brings the two deviants (anthem.rs:312, dispatch.rs:1937) into line with the established idiom rather than inventing anything.
Blast radius verified against the pool, not asserted. The parse-diff sticky reports 87 cards / 122 signatures — larger than the ~40 the PR estimated. I checked its symmetry mechanically: after case-folding, added-only = 0 and removed-only = 0. Every one of the 88 card-changes is a pure case restoration; no ability signature was gained or lost anywhere in the pool. A parser regression would have surfaced as a removal with no matching addition, and there are none. That is the strongest evidence this change could produce.
Discriminating test. compound_subject_granted_ability_description_keeps_original_case asserts description == "{T}: Add {G}.", which fails on pre-fix code with "{t}: add {g}.". It pins the defect rather than restating behavior.
🟡 Non-blocking — a note on the CR citation
CR 604.1 grep-verifies as real ("Static abilities do something all the time rather than being activated or triggered. They are written as statements, and they're simply true"), and it is fine where it sits — it rides along on the pre-existing turn-condition-stripping comment, which is static-ability territory.
But it does not actually govern what this PR fixes. No CR rule mandates the character case of a serialized description — that is a display-fidelity concern, not a rules concern. The real justification is the TextPair original/lowercase contract, which is an engineering invariant of this codebase. The PR body over-claims slightly when it says CR 604.1 requires faithful rendering. Worth internalizing, because a correct-looking CR citation attached to the wrong proposition is how a misapplied rule survives review: grepping proves a rule exists, only tracing proves it governs.
Recommendation: approve and enqueue. Nothing blocking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5599
CR 604.1
Bug
A granted ability's
descriptionis a serialized, user-visible field (AbilityDefinition.description→card-data.json, rendered in the client). Two compound-subject / self static paths passed the lowercased predicate tostrip_suffix_turn_conditionand then fed the lowercase result toparse_continuous_modifications/parse_continuous_gets_has, so the granted ability's description leaked the lowercase parse buffer:Each Fungus and Saproling you control has "{T}: Add {G}."rendered its grant as{t}: add {g}.instead of{T}: Add {G}..Per the issue, the lowercase-mana-symbol leak spans ~40 cards across the pool; the compound-subject anthem path is the reproducing route.
Fix
Pass the original-case predicate to
strip_suffix_turn_conditionin both leaking callers:parse_subject_continuous_static(anthem.rs)dispatch.rs) — the now-redundantpredicate_loweris removed.The trailing condition phrase (
during your turn/during turns other than yours) is lowercase in the original text too, so the suffix still matches and the correctStaticConditionis still recovered (regression-tested) — the retained predicate simply keeps its printed case. The third caller (grammar.rs, Hunter's Blowgun. Otherwisepath) already passed original-case text and is unchanged; this brings the two leaking callers in line with it and with the sibling handlers, which build predicates from.original.Tests
compound_subject_granted_ability_description_keeps_original_case— Brightcap Badger's compound-subject grant →description == "{T}: Add {G}.".DuringYourTurn(the suffix match is case-insensitive to the mixed-case predicate because the phrase itself is lowercase), and single-subject anthems are unchanged.parser::oracle_static::tests(1050 passed, 0 failed).CR verification
description) must render faithfully — verified indocs/MagicCompRules.txt.AI-contributor notes
cargo fmt, CI-exactclippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static module are green.🤖 Generated with Claude Code