fix(engine): resolve triaged card interaction bugs#5353
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request transitions graveyard-to-hand returns from Effect::Bounce to Effect::ChangeZone, implements permission pruning for same-source exile grants, adds support for Last Known Information (LKI) in shared quality filters, and introduces parsing for possessive P/T comparisons and same-source exile invalidation riders. The review feedback primarily highlights style guide violations, specifically missing mandatory CR annotations (Rule R6) across several new engine functions and the use of verbatim string matching (Rule R1) in the new exile invalidation parser, which should be refactored to use modular nom combinators.
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.
Comment on lines
+263
to
+267
| fn prune_replaced_play_from_exile_permissions( | ||
| state: &mut GameState, | ||
| target_id: crate::types::identifiers::ObjectId, | ||
| granted: &CastingPermission, | ||
| ) { |
Contributor
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for game-rule implementation.
Evidence: crates/engine/src/game/effects/grant_permission.rs:263.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment per repository style guide rule R6.
Suggested fix: Add a comment citing CR 611.2a above the function.
// CR 611.2a: Furious Rise-class permissions last until a source-specific invalidation event.
fn prune_replaced_play_from_exile_permissions(
state: &mut GameState,
target_id: crate::types::identifiers::ObjectId,
granted: &CastingPermission,
) {References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| let values = | ||
| object_shared_quality_values(reference_obj, quality, &state.all_creature_types); | ||
| .any(|reference_id| { | ||
| let values = state |
Contributor
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for LKI lookup.
Evidence: crates/engine/src/game/filter.rs:5025.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment per repository style guide rule R6.
Suggested fix: Add a comment citing CR 608.2h for Last Known Information (LKI) lookup.
Suggested change
| let values = state | |
| // CR 608.2h: If an object is no longer in the expected zone, the effect uses its last known information (LKI). | |
| let values = state |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Comment on lines
+943
to
+949
| let (input, _) = alt(( | ||
| tag::<_, _, OracleError<'_>>("~"), | ||
| tag("this enchantment"), | ||
| tag("this artifact"), | ||
| tag("this creature"), | ||
| tag("this permanent"), | ||
| )) |
Contributor
There was a problem hiding this comment.
[MEDIUM] Avoid verbatim string equality for parsing Oracle phrases like 'this '.
Why it matters: Verbatim string equality bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., 'this' followed by a card type) and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
Suggested fix: Decompose the phrase by parsing 'this ' followed by a modular card type parser.
let (input, _) = alt((
tag::<_, _, OracleError<'_>>("~"),
preceded(tag("this "), card_type),
))References
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
Comment on lines
+1648
to
+1650
| fn lift_shared_quality_parent_target_to_triggering_source_in_ability( | ||
| ability: &mut AbilityDefinition, | ||
| ) { |
Contributor
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for trigger-source target lifting.
Evidence: crates/engine/src/parser/oracle_trigger.rs:1648.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment per repository style guide rule R6.
Suggested fix: Add a comment citing CR 608.2c above the function.
Suggested change
| fn lift_shared_quality_parent_target_to_triggering_source_in_ability( | |
| ability: &mut AbilityDefinition, | |
| ) { | |
| // CR 608.2c: Lift shared quality parent target references to the triggering source when no fresh target is introduced. | |
| fn lift_shared_quality_parent_target_to_triggering_source_in_ability( | |
| ability: &mut AbilityDefinition, | |
| ) { |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
| }) | ||
| } | ||
|
|
||
| fn parse_cost_paid_object_possessive_pt_comparison(lower: &str) -> Option<AbilityCondition> { |
Contributor
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for cost-paid object P/T comparison parser.
Evidence: crates/engine/src/parser/oracle_effect/conditions.rs:5713.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment per repository style guide rule R6.
Suggested fix: Add a comment citing CR 608.2h above the function.
Suggested change
| fn parse_cost_paid_object_possessive_pt_comparison(lower: &str) -> Option<AbilityCondition> { | |
| // CR 608.2h: Parse comparisons against the last known power/toughness of a cost-paid object. | |
| fn parse_cost_paid_object_possessive_pt_comparison(lower: &str) -> Option<AbilityCondition> { |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR · 642 card(s), 455 signature(s) (baseline: main
|
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
1 participant
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.
No description provided.