feat(engine,server): wire production trigger matchers, draft match spawn, and planechase scaffold - #2113
Conversation
…awn, and planechase scaffold
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Planechase planar deck scaffolding, implements several new trigger matchers (including Discover, Adapt, Forage, and Foretell), adds parsing support for the Specialize keyword, and wires up server-side draft match spawning. Feedback on these changes highlights a critical issue where a single missing deck submission can block all draft matches from spawning, and notes that match_pay_life incorrectly triggers on any life loss rather than specific life payments. Additionally, several style guide violations were identified, including the use of string prefix checks instead of nom combinators for parser dispatch (Rule R1), incorrect Comprehensive Rules (CR) references for Forage, and a missing CR annotation for Forage resolution (Rule R6). Finally, it is recommended to log errors when pairing generation fails during draft match spawning.
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 deck_payloads: Result<Vec<_>, String> = pairing | ||
| .players | ||
| .iter() | ||
| .map(|pid| { | ||
| let seat = pid.0 as usize; | ||
| let submission = session.session.submitted_decks.get(pid).ok_or_else(|| { | ||
| format!( | ||
| "seat {} has not submitted a deck for match {}", | ||
| seat, pairing.match_id | ||
| ) | ||
| })?; | ||
| deck_payload_from_submission(db, submission) | ||
| }) | ||
| .collect(); | ||
| let decks = deck_payloads?; |
There was a problem hiding this comment.
[HIGH] Gracefully handle missing deck submissions to avoid blocking other matches.
Why it matters: Using the ? operator on deck_payloads will abort the entire spawn_match_games_for_round function if any single player in any pairing has not yet submitted their deck, preventing all subsequent ready pairings from spawning their games.
Suggested fix: Log a warning and continue the loop for that pairing instead of returning early.
let deck_payloads: Result<Vec<_>, String> = pairing
.players
.iter()
.map(|pid| {
let seat = pid.0 as usize;
let submission = session.session.submitted_decks.get(pid).ok_or_else(|| {
format!(
"seat {} has not submitted a deck for match {}",
seat, pairing.match_id
)
})?;
deck_payload_from_submission(db, submission)
})
.collect();
let decks = match deck_payloads {
Ok(d) => d,
Err(e) => {
warn!(match_id = %pairing.match_id, error = %e, "cannot spawn match yet");
continue;
}
};| let GameEvent::LifeChanged { player_id, amount } = event else { | ||
| return false; | ||
| }; | ||
| if *amount >= 0 { | ||
| return false; | ||
| } | ||
| valid_player_matches(trigger, state, *player_id, source_id) | ||
| } |
There was a problem hiding this comment.
[HIGH] match_pay_life incorrectly triggers on any life loss.
Why it matters: Under CR 119.3 and CR 118.4, paying life is a specific action/cost. Triggering on any negative LifeChanged event will incorrectly fire "Whenever you pay life" triggers for regular damage or loss of life effects, violating strict CR fidelity (Pillar 2).
Suggested fix: Distinguish life payments from other life loss events, or add a TODO/guard to prevent false positives from damage/loss of life.
|
|
||
| // Digital-only Specialize: "specialize {cost}" — MTGJSON may omit the keyword | ||
| // when it appears as a standalone rules line; intercept before dispatch fallback. | ||
| if lower_starts_with(&lower, "specialize ") { |
There was a problem hiding this comment.
[MEDIUM] Avoid using lower_starts_with for parsing dispatch.
Why it matters: Rule R1 of the style guide strictly prohibits using full-string starts_with/contains checks for parsing dispatch in non-test parser code, requiring nom 8.0 combinators instead.
Suggested fix: Use a nom combinator to match the "specialize" prefix.
References
- Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Full-string starts_with/contains checks are prohibited. (link)
- 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.
| /// CR 702.808: A player foraged (exiled cards from graveyard for food). | ||
| Forage, |
There was a problem hiding this comment.
[MEDIUM] Correct the CR rule reference for Forage.
Why it matters: The Comprehensive Rules section for the keyword action Forage is CR 701.50, not CR 702.808 (which is a hallucinated rule number). This violates Rule R6 of the style guide.
Suggested fix: Update the comment to reference CR 701.50.
| /// CR 702.808: A player foraged (exiled cards from graveyard for food). | |
| Forage, | |
| /// CR 701.50: A player foraged (exiled cards from graveyard or sacrificed a Food). | |
| Forage, |
References
- Every rules-touching line of engine code must carry a verified CR comment. (link)
| /// CR 702.808: Forage — fires when a player forages. | ||
| pub(super) fn match_forage( |
There was a problem hiding this comment.
[MEDIUM] Correct the CR rule reference for Forage.
Why it matters: The Comprehensive Rules section for the keyword action Forage is CR 701.50, not CR 702.808. This violates Rule R6 of the style guide.
Suggested fix: Update the comment to reference CR 701.50.
| /// CR 702.808: Forage — fires when a player forages. | |
| pub(super) fn match_forage( | |
| /// CR 701.50: Forage — fires when a player forages. |
References
- Every rules-touching line of engine code must carry a verified CR comment. (link)
| Effect::Forage => { | ||
| // This keyword action is recognized by the parser but not yet implemented. | ||
| // It's a no-op at runtime but counts as supported for coverage. | ||
| events.push(GameEvent::PlayerPerformedAction { | ||
| player_id: ability.controller, | ||
| action: PlayerActionKind::Forage, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Add mandatory CR annotation for Forage resolution.
Why it matters: Every rules-touching line of engine code must carry a verified CR comment under Rule R6 of the style guide.
Suggested fix: Add a comment referencing CR 701.50.
| Effect::Forage => { | |
| // This keyword action is recognized by the parser but not yet implemented. | |
| // It's a no-op at runtime but counts as supported for coverage. | |
| events.push(GameEvent::PlayerPerformedAction { | |
| player_id: ability.controller, | |
| action: PlayerActionKind::Forage, | |
| }); | |
| Effect::Forage => { | |
| // CR 701.50: To forage is to exile three cards from your graveyard or sacrifice a Food. | |
| events.push(GameEvent::PlayerPerformedAction { | |
| player_id: ability.controller, | |
| action: PlayerActionKind::Forage, | |
| }); |
References
- Every rules-touching line of engine code must carry a verified CR comment. (link)
| if draft_mgr.ensure_pairings_generated(draft_code).is_err() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Log errors when generating pairings fails.
Why it matters: Silently returning on error makes debugging difficult when pairings fail to generate during match spawning.
Suggested fix: Log the error using warn!.
if let Err(e) = draft_mgr.ensure_pairings_generated(draft_code) {
warn!(draft = %draft_code, error = %e, "failed to generate pairings");
return;
}
🤖 Architecture Review (automated)Verdict: Seam: PASS — trigger matchers / index / events live in the engine; draft-spawn logic correctly sits in The trigger-matcher wiring is the high-value core and is mostly correct: I verified Findings
|
🏛️ Architecture & Idiom Deep-Dive (automated)Verdict: ♻️ Refactor recommended What's idiomatic here: The trigger-index rework is the right architectural move — Discover/Adapt/Exerted/Foretell graduate from the "unclassified, checked against every event" bucket into dedicated Idiom / architecture findings
The two REUSE items are the load-bearing ones: they add new matcher functions where the codebase already has the authoritative dispatcher for that event class, which is the exact "verbatim copy of a sibling matcher" smell. Reshaping now keeps the matcher table from accreting one-function-per-keyword duplication that future keywords will copy. |
🔁 Re-review (upgraded process)Prior verdict: Reconciled with existing reviews:
Missed or re-rated
|
…iggers-draft-planechase # Conflicts: # crates/engine/src/game/trigger_matchers.rs
…awn, and planechase scaffold
99aea3c to
051bbd6
Compare
…ers-draft-planechase' into feat/mega-features-triggers-draft-planechase # Conflicts: # crates/engine/src/game/effects/mod.rs # crates/engine/src/game/trigger_index.rs # crates/engine/src/game/trigger_matchers.rs # crates/engine/src/types/game_state.rs # crates/engine/src/types/triggers.rs # crates/phase-server/src/main.rs # crates/server-core/src/draft_session.rs
06b76fa to
ae0a6ae
Compare
…iggers-draft-planechase
ae0a6ae to
aa9e505
Compare
|
Cleanup pass pushed to the original fork branch. Changes I made:
Current state:
Assumption: “fake Forage” means this PR should not change Forage relative to |
2 participants
Summary
First implementation pass on the mega-features phased roadmap: not full plan completion, but the highest-leverage slices across Phases 1–4.
docs/phase1-coverage-inventory.md(86.89% corpus; cluster chore: update coverage stats and badges #1 =Effect:static_structure+Effect:unknown, 35 cards). Parser intercept for standalonespecialize {cost}→Keyword::Specialize(15-card pattern).GameEvent::Foretold,PlayerActionKind::Forage. Removed duplicate stub registration for modes that already had matchers intrigger_matcher().compute_modal_total_cost— no runtime change. BeginGame/Class/dungeon rooms largely pre-existing or still deferred (BG Wilderness rooms 7–8, 10–14 remainUnimplemented).PlanechaseState+GameState.planechasescaffold (planar_deck, active plane, controller). Draft:ensure_pairings_generated+spawn_match_games_for_round+maybe_spawn_draft_matchessendsDraftMatchStartwhen the pod enters match play.Out of scope (follow-up PRs): ~30 remaining stub
TriggerModes, full planechase die/planeswalk/chaos, bulkEffect:unknown(420 single-gap cards), mtgish-import.Test plan
cargo fmt --allcargo check -p engine -p server-core -p phase-servercargo test -p engine trigger_matchers::(194 passed)scripts/coverage-regression-check.shafter card-data regen if specialize unlock is measuredDraftMatchStart+ game session inactive_matchesFiles touched (high level)
trigger_matchers.rs,trigger_index.rs,types/triggers.rs,types/events.rscasting.rs,effects/mod.rs,log.rs,public_state.rsoracle.rsgame/planechase.rs,game_state.rsdraft_session.rs,phase-server/main.rsdocs/phase1-coverage-inventory.md