fix(engine): implement the World rule state-based action (CR 704.5k) - #5088
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the Magic: The Gathering 'world rule' state-based action (CR 704.5k) in the game engine, ensuring that if multiple world permanents exist, only the newest one survives, or all are put into graveyards in the event of a tie. Feedback suggests optimizing the code by consuming the worlds vector idiomatically with into_iter() and adding a test case to verify that phased-out world permanents are correctly ignored.
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.
| let mut doomed: Vec<ObjectId> = worlds | ||
| .iter() | ||
| .filter(|(_, ts)| tied || *ts != newest) | ||
| .map(|(id, _)| *id) | ||
| .collect(); |
There was a problem hiding this comment.
[MEDIUM] Idiomatic consumption of
worldsvector. Evidence:crates/engine/src/game/sba.rs:1293-1297.
Why it matters: Consuming theworldsvector withinto_iter()instead of borrowing it withiter()is more idiomatic in Rust when the vector is not used afterward.
Suggested fix: Useinto_iter()to consume the vector.
| let mut doomed: Vec<ObjectId> = worlds | |
| .iter() | |
| .filter(|(_, ts)| tied || *ts != newest) | |
| .map(|(id, _)| *id) | |
| .collect(); | |
| let mut doomed: Vec<ObjectId> = worlds | |
| .into_iter() | |
| .filter(|(_, ts)| tied || *ts != newest) | |
| .map(|(id, _)| id) | |
| .collect(); |
| assert!(state.players[0].graveyard.is_empty()); | ||
| assert!(state.players[1].graveyard.is_empty()); | ||
| } | ||
|
|
There was a problem hiding this comment.
[MEDIUM] Missing test coverage for phased-out world permanents. Evidence:
crates/engine/src/game/sba.rs:5203.
Why it matters: Phased-out permanents must be ignored by the world rule (CR 702.26b), and adding a test prevents regressions where phased-out world permanents are incorrectly processed.
Suggested fix: Add a test case verifying that phased-out world permanents are ignored by the world rule.
}
#[test]
fn sba_world_rule_ignores_phased_out_worlds() {
// (h) CR 702.26b: phased-out permanents are treated as though they don't exist.
// A phased-out world permanent should not count toward the "two or more"
// world permanents check, nor should it be put into the graveyard.
let mut state = setup();
let older = add_world(&mut state, CardId(1), PlayerId(0), "The Abyss", 10);
let newer = add_world(&mut state, CardId(2), PlayerId(0), "Nether Void", 20);
phase_out_object(&mut state, older);
let mut events = Vec::new();
check_state_based_actions(&mut state, &mut events);
assert!(state.battlefield.contains(&older), "phased-out world remains on battlefield");
assert!(state.battlefield.contains(&newer), "lone active world survives");
assert!(state.players[0].graveyard.is_empty());
}
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The world-rule ordering uses the permanent's battlefield-entry timestamp, not the time it gained the World supertype. Evidence: crates/engine/src/game/sba.rs collects (ObjectId, obj.timestamp) for every current World permanent, but the engine already supports continuous effects that grant World after entry via ContinuousModification::AddSupertype { supertype: Supertype::World } (for example, All enchantments are world. is parsed in crates/engine/src/parser/oracle_static/tests.rs, and the layer application appends the supertype in crates/engine/src/game/layers.rs). Why it matters: CR 704.5k keeps the permanent that has had the World supertype for the shortest amount of time; an older permanent that becomes World later should be newer for this rule than a printed World permanent that entered afterward but had World longer. Suggested fix: track/derive the World-acquisition timestamp (or otherwise preserve the CR 704.5k ordering authority) and add a discriminating test where a non-World permanent gains World after a printed World permanent already has it.
Gemini's phased-out concern is not the blocker here: this implementation uses battlefield_phased_in_ids() and live_battlefield_object, so phased-out permanents are already excluded from the candidate set.
5d29160 to
8d69b47
Compare
done. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head after re-review. The previous World-acquisition timestamp blocker is fixed: printed World uses the object's entry timestamp, granted World uses the active continuous-effect timestamp authority, and the new runtime tests cover printed-vs-granted ordering plus phased-out World permanents. CI is still running, so auto-merge will wait for required checks.
Two or more permanents with the "world" supertype (Nether Void, The Abyss,
Living Plane, Concordant Crossroads — 30 cards) coexisted on the battlefield
forever: the CR 704.5k world rule was entirely unimplemented, while the sibling
legend rule (704.5j) and role uniqueness were fully built. Adds check_world_rule
as a state-based action.
- Global collection of all battlefield permanents with the world supertype (no
controller grouping — CR 704.5k has no controller qualifier, unlike the
per-player legend rule).
- The permanent that has HELD the world supertype for the shortest time survives;
all others go to their owners' graveyards. "Time held" is derived per
permanent by world_acquisition_timestamp: a printed-world permanent has held
world since it entered (CR 613.7d); a permanent granted world by a continuous
AddSupertype{World} effect has held it since the later of its entry and the
grant's start (the granting static's source-entry timestamp, CR 613.7a) — so a
permanent that gains world late correctly counts as newer than a printed-world
permanent that entered later but has held world longer. Grant matching mirrors
apply_continuous_effect_filtered (affected_filter + per-recipient condition).
- CR 704.5k tie twist: if two or more are tied for newest (shortest time held),
ALL world permanents are put into graveyards — the divergence from the
legend/role rules.
- Choiceless; moves via the shared graveyard pipeline with the CR 616.1
replacement-order bail and CR 603.10a/704.3 simultaneous-departure marking.
Tests: 11 runtime scenario tests through the real check_state_based_actions path —
newest-of-two/three survives; tie kills all (incl. the strictly-older non-tied
world); single/zero-world no-op; global-across-different-controllers; choiceless;
phased-out worlds ignored (CR 702.26b); and the acquisition-order guards: a
granted-world permanent is newer than a printed-world one that entered later, and
the survivor tracks max(recipient-entry, source-entry), not a naive read. The
tie=all-die, global-scope, and granted-vs-printed tests are revert-failing
(mutation-verified). Full cargo test -p engine --lib green (15015 pass / 0 fail);
clippy -D warnings clean.
CR 704.5k, CR 613.7a, CR 613.7d, CR 205.4b, CR 616.1, CR 603.10a, CR 704.3, CR 614.6, CR 702.26b.
Head branch was pushed to by a user without write access
8d69b47 to
9d6b0d8
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved refreshed head. The only delta since the prior approval is a test-helper clippy allowance plus the branch update to current main; the production World-rule implementation and discriminating tests remain unchanged. Auto-merge will wait for required checks.
Summary
Two or more permanents with the
worldsupertype (Nether Void, The Abyss, Living Plane, Concordant Crossroads — 30 cards) coexisted on the battlefield forever: the CR 704.5k world rule was entirely unimplemented, while the sibling legend rule (704.5j) and role uniqueness were fully built. Addscheck_world_ruleas a state-based action.Files changed
CR references
CR 704.5k, CR 613.7d, CR 616.1, CR 603.10a, CR 704.3, CR 614.6, CR 702.26b
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Verification
cargo fmt --all— cleancargo clippy -p engine --all-targets -- -D warnings— cleancargo test -p engine --lib— 15003 pass / 0 fail (114/114 in the sba module: 106 pre-existing + 8 new; purely additive)check_state_based_actionspath; the tie=all-die and global-across-controllers tests are revert-failing guards (mutation-verified: forcing no-tie / per-controller grouping makes them fail)gen-card-data/coverage/semantic-audit— N/A: engine-runtime SBA fix, no card parsing changed (30 real World cards confirmed to load withSupertype::World)Scope Expansion
None.
Validation Failures
None.
CI Failures
None.