fix(crew): pilot tokens crew for adjusted power, not raw or doubled#5154
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces index-aligned contributions to WaitingFor::CrewVehicle to ensure the UI gates vehicle crewing on engine-validated adjusted power (e.g., for Pilot tokens or Giant Ox) rather than raw printed power. It also fixes a bug where catalog presets duplicated abilities already granted by a token's creating effect. Feedback highlights that SaddleMount is missing a parallel contributions field to support its own power modifiers, and that skipping catalog injection entirely when static_definitions is populated will break predefined tokens (like Treasure or Food) when they are created with additional abilities (e.g., haste).
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.
| /// power were N greater" (Pilot tokens) and "using its toughness" (Giant | ||
| /// Ox) mean the contribution differs from the creature's printed power, so | ||
| /// the UI MUST sum these values, not raw power, when gating the selection. | ||
| contributions: Vec<i32>, |
There was a problem hiding this comment.
[HIGH] SaddleMount is missing parallel contributions field. Evidence:
crates/engine/src/types/game_state.rs:3262.\n> Why it matters: The direct siblingSaddleMount(CR 702.171a) also supports power contribution modifiers (e.g., Aetherdrift Pilot's "saddles Mounts ... as though its power were 2 greater"), but lacks thecontributionsfield, which will cause the UI to incorrectly gate saddle selection on raw power.\n> Suggested fix: Addcontributions: Vec<i32>toWaitingFor::SaddleMountand wire it through the engine and frontend similarly toCrewVehicle.
References
- Universal Review Lenses -> L2. Sibling coverage: If a fix landed in one site of a class, check if the siblings need the same fix. (link)
| if !obj.static_definitions.is_empty() { | ||
| if obj.token_rules_text.is_none() { | ||
| obj.token_rules_text = Some(rules_text.to_string()); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
[HIGH] Skipping catalog injection when static_definitions is not empty breaks predefined tokens with additional abilities. Evidence:
crates/engine/src/game/effects/token.rs:2541.\n> Why it matters: If a predefined token (such as a Treasure, Food, or Clue) is created with an additional granted ability (e.g., "Create a Treasure token with haste"),static_definitionswill not be empty, causing the engine to skip injecting the token's defining catalog abilities (like the Treasure's mana ability).\n> Suggested fix: Only skip functional injection if the token is not a predefined token (e.g., by checking against a list of predefined token names like "Treasure", "Food", "Clue", "Blood", "Map", "Powerstone", "Junk", "Incubator"), or by checking if the preset itself is marked as predefined.
let is_predefined = matches!(
obj.name.as_str(),
"Treasure" | "Food" | "Clue" | "Blood" | "Map" | "Powerstone" | "Junk" | "Incubator"
);
if !is_predefined && !obj.static_definitions.is_empty() {
if obj.token_rules_text.is_none() {
obj.token_rules_text = Some(rules_text.to_string());
}
return;
}References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the PR. I found a few blockers before this can move forward.
crates/phase-ai/src/policies/payment_selection.rsstill constructsWaitingFor::CrewVehiclewithout the new requiredcontributionsfield, so the all-targets/test build path will fail before the behavior can be validated. Please thread contributions into that test helper, ideally from the same contribution calculation used by the engine.- The engine now computes saddle contribution through
object_crew_power_contribution(..., CrewAction::Saddle), butclient/src/viewmodel/gameStateView.tsstill sends no contribution map forWaitingFor::SaddleMount. That leaves the frontend confirm gate on raw power for saddle even when the engine accepts adjusted Pilot-token saddle contribution. WaitingFor::CrewVehicle.contributionsis a new serialized field without a serde default. Saved/debug-exported games paused in aCrewVehicleprompt before this PR would fail to deserialize on restore. Please add a safe default/normalization path for old waiting states.- The token ability comment in
crates/engine/src/game/effects/token.rscitesCR 111.4, but that rule is about token names/subtypes, not the token abilities/characteristics being implemented here. Please verify and cite the correct CR rule before landing.
Because this PR also changes frontend/view-model files, frontend acceptance is deferred to Matt under the current sweep policy; I’m not approving or enqueueing it from the automation sweep.
Parse changes introduced by this PR · 9 card(s), 9 signature(s) (baseline: main
|
b85f92e to
aaf7c4a
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the update. The phase-ai constructor issue is resolved, but the current head still has blockers.
SaddleMountstill gates the frontend on raw power. The engine computes saddle eligibility withobject_crew_power_contribution(..., CrewAction::Saddle), butclient/src/viewmodel/gameStateView.tsmapsSaddleMountwithout contributions. A Pilot-style saddle modifier can be accepted by the engine while the frontend blocks confirm. Please add index-aligned saddle contributions toWaitingFor::SaddleMountand wire them through the view model the same way as crew.CrewVehicle.contributionsis still a required serialized field without a serde default. Older saved/debug-exported games paused in aCrewVehicleprompt can fail to deserialize. Please add a default and normalize/recompute when needed.- The token ability comments still cite
CR 111.4, which is about token names/subtypes. The ability/characteristics authority isCR 111.3; please update the annotation.
Frontend acceptance remains deferred to Matt under the current sweep policy.
A Pilot token that "crews Vehicles as though its power were 2 greater"
(Shorikai, Genesis Engine / Kotori, Pilot Prodigy) crewed for the wrong
amount through two independent bugs:
- Double-grant: the token carried the crew static twice -- once from the
creating effect's `with "..."` clause and again from the catalog art
preset's rules_text -- so a 1/1 Pilot contributed 5 (1+2+2). When the
1/1 body matched a different printing's art (e.g. the Aetherdrift
"saddles Mounts and crews Vehicles" Pilot), the two statics had
different keyword actions and slipped past an exact-match de-dupe. Per
CR 111.4 the creating effect's grant is authoritative, so the catalog
`rules_text` fallback is now display-only and injects no functional
abilities when the token already carries granted statics.
- Frontend re-derived crew power: the crew-selection UI summed each
creature's raw printed power (1) instead of the engine's adjusted crew
contribution, gating the lone Pilot at 1 < N and blocking the crew
("crews for just 1"). The engine now sends per-creature contributions
on the CrewVehicle choice and the view-model gates on those.
Result: a 1/1 Pilot crews for 3; a 2/2 (with a +1/+1 counter) for 4.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
aaf7c4a to
c9d5300
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the update. The phase-ai constructor and predefined-token injection path look addressed on current head c9d5300c, but two blockers from the last review are still present.
-
SaddleMountstill does not carry index-aligned adjusted contributions. The engine computes saddle eligibility usingobject_crew_power_contribution(..., CrewAction::Saddle), butWaitingFor::SaddleMountstill only serializeseligible_creatures(crates/engine/src/types/game_state.rs) andhandle_saddle_activationstill returns no contribution array. The frontend saddle path still buildsselection: { type: "totalPowerAtLeast", power: waitingFor.data.saddle_power }, soboardChoiceSelectedPowerfalls back to raw object power. Any saddle modifier can still be accepted by the engine and blocked by the UI. -
CrewVehicle.contributionsis still a required serialized field. The new field onWaitingFor::CrewVehiclehas no serde default, so older saved/debug-exported states with an in-flight crew prompt and nocontributionsfield will fail to deserialize. Please make this field backward-tolerant and keep the frontend’s existing fallback behavior for absent or mismatched contribution data.
# Conflicts: # crates/engine/tests/integration/main.rs
matthewevans
left a comment
There was a problem hiding this comment.
Current head f90cf0b resolves the outstanding blockers: SaddleMount now carries engine-computed contributions through the serialized waiting state and frontend board-choice gate, CrewVehicle contributions are backward-tolerant for older pause-state payloads, and frontend fallback remains raw power when contribution data is absent or mismatched. CI is green on the current head after rerunning the canceled Rust shard.
|
Maintainer update: I pushed a small fixup on top of this branch () to correct the token ability CR citation and add a production-path Saddle regression proving adjusted saddle contribution reaches the engine waiting state and payment flow. Holding this for the refreshed CI on the new head before clearing the prior request-changes review. |
|
Correction: the maintainer fixup SHA is |
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head after maintainer fixup review: crew/saddle now use engine-computed contribution values through the production activation/payment paths, token catalog ability injection no longer double-grants functional statics, and there is runtime coverage for both Shorikai/Pilot crewing and adjusted Saddle contribution. CI can finish through the merge queue.
A Pilot token that "crews Vehicles as though its power were 2 greater"
(Shorikai, Genesis Engine / Kotori, Pilot Prodigy) crewed for the wrong
amount through two independent bugs:
Double-grant: the token carried the crew static twice -- once from the
creating effect's
with "..."clause and again from the catalog artpreset's rules_text -- so a 1/1 Pilot contributed 5 (1+2+2). When the
1/1 body matched a different printing's art (e.g. the Aetherdrift
"saddles Mounts and crews Vehicles" Pilot), the two statics had
different keyword actions and slipped past an exact-match de-dupe. Per
CR 111.4 the creating effect's grant is authoritative, so the catalog
rules_textfallback is now display-only and injects no functionalabilities when the token already carries granted statics.
Frontend re-derived crew power: the crew-selection UI summed each
creature's raw printed power (1) instead of the engine's adjusted crew
contribution, gating the lone Pilot at 1 < N and blocking the crew
("crews for just 1"). The engine now sends per-creature contributions
on the CrewVehicle choice and the view-model gates on those.
Result: a 1/1 Pilot crews for 3; a 2/2 (with a +1/+1 counter) for 4.
Co-Authored-By: Claude Opus 4.8 [email protected]