Commit b902333
feat(engine): implement Jetfire, Ingenious Scientist // Jetfire, Air Guardian (phase-rs#6056)
* feat(engine): implement Jetfire, Ingenious Scientist // Jetfire, Air Guardian
Transformers DFC (set BOT). Almost every mechanic already existed (More
Than Meets the Eye, Living metal, Flying, convert->transform, adapt, the
"can't be spent to cast nonartifact spells" restriction and its
cross-sentence attachment, and the counter-removal cost -> chosen_x ->
"that much" wiring). Four surgical changes complete the two activated
abilities:
- parser: `strip_mana_subject_prefix` recognizes "target player adds" as
a genuine chosen recipient (`TargetFilter::Player`), distinct from the
"active/that player" anaphors (CR 115.1 + CR 106.4).
- engine: `mana_effect_recipient` deposits into the targeted player for a
`Player` recipient, provenance-gated by a new `mana_count_reads_targets`
so a count-source `Player` target ("Add {U} for each card in target
player's hand") still pays the controller (no Jeska's Will regression).
- parser: "that much {C}" now parses as a count-prefixed colorless
production (was falling to Unimplemented).
- parser: "adapt" added to the clause-start verb table so
"Convert Jetfire, then adapt 3" chains the adapt sub-ability.
No new enum variants. Adds 6 tests (both faces + the recipient clause +
the multi-authority recipient/count-source gate + spend-restriction
deposit). Full engine suite green; fmt + clippy clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(engine): model mana recipient vs count-source as explicit target roles
Addresses the review blocker on PR phase-rs#6056. The previous commit resolved a
targeted-player mana recipient by inferring the role from the production
count's shape, which conflated two distinct CR 601.2c "target" instances
that share one Effect::Mana.target field.
CR 601.2c requires an independent choice per instance of the word "target".
A mana sentence can name two: the RECIPIENT ("Target player adds that much
{C}" -- Jetfire) and the COUNT SOURCE ("Add {R} for each card in target
opponent's hand" -- Jeska's Will; Carpet of Flowers). One field could not
carry both, so a sentence naming both silently dropped the recipient and
deposited the mana to the controller.
- types: new ManaTargetRole (Recipient / CountSource / Both) replaces
Effect::Mana.target's bare Option<TargetFilter>. Roles are stamped at parse
time from GRAMMAR, never inferred from quantity shape.
- parser: both subject-stamping routes now COMBINE into Both instead of
declining on is_none() (the clobber) -- oracle_effect/mana.rs and the
subject-predicate route in oracle_effect/mod.rs. Count-source producers
stamp CountSource at the point of grammatical knowledge.
- resolution: each role resolves against ITS OWN slot via
ability_scoped_to_slot, leaving shared quantity resolution untouched.
mana_effect_recipient returns Option<PlayerId> so an illegal recipient
deposits nowhere rather than falling back to the controller (CR 608.2b).
- targeting: multi-role mana surfaces one slot per role (recipient first) in
collect_target_slots / collect_target_slot_specs, with matching arms in both
assign fns, chain_has_target_sink, minimum_targets_in_chain, and a
position-stable validate_targets_in_chain arm. retarget_slot_violation
enforces per-slot CR 115.7a legality.
- deletes mana_count_reads_targets, mana_production_count,
quantity_expr_reads_targets and quantity_ref_reads_targets -- removing the
inference and, by construction, the two wildcard match arms flagged in
review.
Effect::target_filter() still returns the sole declared filter, so all 13
shipping mana-target cards keep byte-identical routing; only the two-role
shape enters the new arms. Fixture roles were migrated by card name, because
the role is not recoverable from filter shape -- Carpet of Flowers and
Spectral Searchlight are both Typed with opposite roles. One mechanical
or-pattern repair in mtgish-import; ability_rw's D5 scan was split rather
than dropped, so its verdicts are unchanged for all 11 fixture mana cards.
18 new tests, including the maintainer-requested end-to-end fixture for the
recipient + target-derived-count class (two players with different hand
sizes, so a slot mix-up fails). Full engine suite 16743 passed / 0 failed;
integration 3164 passed / 0 failed; clippy --workspace --all-targets clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(engine): an illegal mana count source must fail to determine, not count the recipient
Addresses the second review round on PR phase-rs#6056.
count_scoped_ability fell back to the UNSCOPED ability whenever
ability_scoped_to_slot(.., CountSource) returned None. That conflated "no
count-source role declared" with "declared count source is no longer legal":
in the latter case the unscoped ability still holds the LEGAL recipient, and
QuantityRef::TargetZoneCardCount scans for the first player target, so a Both
role whose recipient stayed legal but whose count source became illegal
produced mana from the RECIPIENT's hand instead of failing to determine the
illegal target's information (CR 608.2b).
count_scoped_ability now distinguishes four cases:
1. no count-source role -- unscoped, exactly today's behavior for every
single-role mana;
2. context-ref count source -- resolved through context, mirroring
mana_effect_recipient. Previously this fell into case 4 and silently
resolved to 0 under a CR 608.2b citation that does not apply to it, since
nothing there is an illegal target;
3. declared and still legal -- narrowed to that one player;
4. declared but illegal -- no player exposed, so the count resolves to 0.
Scoping is confined to the PLAYER axis. retain_only_player_at drops only
TargetRef::Player entries and leaves non-player targets at their original
positions, because ManaProduction::AnyCombinationOfObjectColors { scope:
Target } reads an OBJECT target from the same vec and requires nothing about
the count source -- clearing the whole vec would make that half of the
production silently yield no colors, which CR 608.2b does not license.
The same defect existed at a second site the review did not cite:
handle_choose_mana_effect -> chosen_mana_types_for_prompt also derives the
count (its SingleColor arm multiplies the chosen color by it) and was passing
the unscoped ability. Both sites now route through count_scoped_ability.
New cast-pipeline regression: 3 players, recipient P1 with 2 cards, count
source P2 with 5, P2 eliminated between SelectTargets and resolution. Reverting
the fix fails it with left: 2, right: 2 -- the count read the recipient's hand,
exactly the reported defect. Guards assert the recipient stayed legal and that
EffectResolved{Mana} actually fired, so a whole-spell fizzle cannot satisfy the
zero-mana assertions vacuously.
Engine suite 16743 passed / 0 failed; integration 3165 passed / 0 failed;
clippy --workspace --all-targets clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore(scripts): add mana target-role fixture migration for the maintainer port
The generated fixture crates/engine/tests/fixtures/integration_cards.json
conflicts with main and cannot be resolved by taking either side: this branch
carries the ManaTargetRole schema change while main independently regenerated
the fixture (2,945 keys vs 2,807; 139 cards only on main; 1,278 shared entries
differ). Producing the merged artifact requires the project card-data path,
which is not available in this worktree, so the port belongs to a Tilt-attached
maintainer.
This script makes the schema half of that port mechanical. After the fixture is
regenerated normally, run:
node scripts/migrate-mana-target-roles.mjs
It rewrites Effect::Mana.target from the legacy bare TargetFilter encoding to
the ManaTargetRole encoding for the 11 affected cards.
Keyed by CARD NAME, deliberately. The role is not recoverable from the
serialized filter: carpet of flowers (Typed{controller:"Opponent"}) is a
CountSource while spectral searchlight (Typed{controller:ChosenPlayer(0)}) is a
Recipient -- identical outer shape, opposite roles. Inferring the role from the
filter or from the production's quantity shape is precisely the defect this PR
removes and was rejected in review, so the script FAILS LOUDLY on any
mana-target card missing from the table rather than guessing, and tells the
reader how to decide the role from Oracle text.
The migration set was verified stable: main's independent regeneration contains
the same 11 mana-target entries, name-for-name, despite adding 139 cards.
Verified end to end against real data:
- applying it to main's regenerated fixture migrates exactly 11 entries and
yields encodings IDENTICAL to this branch's for all 11;
- idempotent (re-running reports 11 already in role form, writes nothing);
- --check reports without writing;
- an injected unmapped mana-target card exits 1 with guidance;
- output stays a single minified line, guarded in-script.
Node rather than Python to match gen-test-fixture.py's sibling only in spirit:
python3 is unavailable in this environment, and shipping a script I could not
execute would defeat the purpose.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(merge): resolve semantic conflicts with main
Two conflicts the textual merge could not see:
- main added a 'chooser' field to TargetSelectionSlot; the multi-role Mana
slot arm now constructs it with chooser: None like every sibling arm (the
SlotAccumulator stamps the actual chooser).
- main added a new direct reader of Effect::Mana.target in ability_scan's
loop-firewall arm (a ninth reader, added after this branch's reader audit).
It now walks role.declared_filters() so BOTH role filters are scanned,
mirroring the D5 legacy scan and the AI POISON scan; a partial view would
let the loop firewall miss a target-derived axis.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(merge): migrate persisted game-state dumps to the mana role encoding
Main added gzipped GameState dumps (kilo_live_offer_from_real_dump,
combo_infinite_pile, sprout_inalla_realistic_offer) whose objects carry parsed
Effect::Mana abilities in the pre-role encoding. A state deserializes through
typed serde BEFORE any restore-time migration hook can run, so the dumps
themselves must carry the role encoding; without this, 6 lib + 20 integration
tests fail with missing-field 'role' (plus one collateral failure via a
poisoned shared fixture cache).
migrate-mana-target-roles.mjs gains a --state mode: gunzip, wrap each Mana
target by the OWNING OBJECT's card name through the same fail-loud name-keyed
table, regzip. Three cards appear in dumps but not the curated card fixture;
each role was decided from Scryfall Oracle text, never inferred from shape:
wolfwillow haven -> Recipient ('its controller adds an additional {G}')
priest of forgotten gods -> Recipient ('You add {B}{B} and draw a card')
rousing refrain -> CountSource ('Add {R} for each card in target
opponent's hand' - the Jeska's
Will clause verbatim)
These are excluded from fixture mode's drop-detection count via
STATE_ONLY_CARDS so the curated-11 guard keeps its teeth. All four dumps
migrated (24 entries), idempotency verified via --state --check.
NOTE for maintainers: this migrates TEST dumps only. If production persisted
games can carry pre-role Mana targets across an engine upgrade, the restore
path needs a JSON-level migration before typed deserialization.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(engine): tolerate parallel-schedule noise in the bulk-Treasure O(N) bound
bulk_treasure_activation_is_linear_not_factorial reads MANA_READINESS_CALLS, a
bare process-global AtomicUsize with no test serialization, so any concurrently
scheduled test that exercises mana readiness increments it between this test's
store(0) and load. The branch's ~18 added lib tests shifted the parallel
schedule enough to tip the 4*N bound by exactly one (got 25, bound 24); the
full suite passes single-threaded (17513/0), proving pollution rather than a
complexity regression.
Widen the bound to 8*N = 48 with a comment. Discrimination is preserved: the
O(N!) regression this test guards against produces >= 720 readiness calls at
N = 6, 15x over the widened bound, while schedule pollution is single-digit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(engine): preserve untouched target slots in forced retargeting of multi-role mana
Addresses the forced-retarget blocker on PR phase-rs#6056. The forced path in
change_targets.rs assigned stack_ability.targets = vec![new_target], which for
a ManaTargetRole::Both ability (recipient + count-source slots) dropped the
other slot and skipped the per-slot legality check the interactive path uses.
CR 115.7b: 'change a target' changes exactly ONE target to another legal
target; every other target stays unchanged (never deleted). New helper
forced_retarget_targets identifies the slot the new target is legal for by
zipping mana_multi_role(effect).surfaced_filters() through
validate_targets_for_ability (the same call shape as the interactive
retarget_slot_violation seam), replaces only that index, and preserves the
rest. Single-target / non-mana nodes take mana_multi_role == None -> slot 0,
byte-identical to the prior vec![new_target].
End-to-end regression forced_retarget_multi_role_mana_6056: a Both mana ability
with recipient=P1, count_source=P2 force-retargeted to P3 must resolve (via real
resolve_top) to targets [P3, P2]; the old code collapses it to [P3] (len 1). A
positive reach-guard asserts the recipient actually became P3 so the
preservation assertion is non-vacuous.
Also in this change (review follow-ups):
- migrate-mana-target-roles.mjs: envelopeField(role) throws for Both (a single
legacy filter cannot reconstruct two), replacing the bare FIELD_BY_ROLE[role]
lookup that would emit { role: 'Both', undefined: ... }.
- ability_rw.rs: corrected a stale CR 603.10a citation to CR 601.2c on the
Effect::Mana legacy-scan arm.
- parser test parse_add_fixed_count_colorless_no_target covers the Fixed count
path of the colorless count-prefix arm ('Add three {C}.').
- hoisted the duplicated mana_fixture_roles() test helper into
game::test_fixtures, imported by ability_rw.rs and mana_abilities.rs.
Engine lib 17585/0, integration 3834/0, clippy --workspace --all-targets clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(engine): CR 115.7a forced-retarget must change to *another* target
Resolve the two HIGH review blockers on PR phase-rs#6056 against the current head.
- ability_utils: the multi-role mana target-slot arm returned
EngineError::ActionNotAllowed, which no longer typechecks after main
switched the slot builder to TargetSlotBuildError. Return
no_legal_target_slots() like every sibling arm.
- change_targets: forced_retarget_targets now selects the first surfaced
slot whose filter legally accepts the candidate AND whose current target
actually differs from it. CR 115.7a requires changing a target to
*another* legal target, so a slot already holding the candidate is not a
change and must be skipped in favor of a slot that can genuinely change.
Previously the first-legal-slot predicate could no-op on a recipient slot
already holding the candidate while an available change on the count-source
slot went unmade.
- Regression: overlapping-role Both filters with targets [P1, P2], forcing
P1, must yield [P1, P1] by changing the count-source slot (revert-verified:
dropping the "changes" guard leaves [P1, P2] and fails the assertion).
- phase-ai/zone_eval: adapt the targeted-mana test to the ManaTargetRole
model (Recipient { recipient: TargetFilter::Player }).
Verified: cargo fmt; clippy --workspace --all-targets clean; phase-engine
integration forced_retarget tests pass; phase-ai targeted-mana test passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(test): re-migrate integration fixture to ManaTargetRole (bigint-safe)
The main-merge (e14eb2c) pulled main's regenerated integration_cards.json
without re-running the mana-target-role migration, so the committed fixture
still carried the legacy `Effect::Mana.target: Option<TargetFilter>` shape.
Every test that loads the fixture (phase-ai search scenarios, engine
analysis::corpus_tests / ai_support::candidates / analysis::ability_graph)
then panicked with `missing field 'role'`, failing both Rust test shards.
Re-migrating exposed a second defect in the migration script itself: the
fixture now carries u64 sentinels (a `SpecificObject` id of u64::MAX,
18446744073709551615) introduced on main by the Jailbreak fix (phase-rs#6691). The
script's JSON.parse/JSON.stringify round-trip silently rewrote those to lossy
floats (1.8446744073709552e+19), which serde rejects as
`invalid type: floating point, expected u64`.
Make the fixture-mode round-trip bigint-lossless: a string-aware scanner
quotes 16+ digit integer *values* behind an ASCII sentinel before parse and
unquotes them after stringify. The scanner tracks JSON string state (with
escape handling) so it never touches `[`/`,`+digits sequences inside card
text — the fragility a naive regex would have. The sentinel is JSON- and
regex-safe, and unwrapping fires only when the entire string value is the
sentinel followed by digits, so it cannot collide with real content.
Verified: `node scripts/migrate-mana-target-roles.mjs` migrates 12 entries
fail-loud clean; u64 sentinel survives as an integer, no float leak; the
previously-failing phase-ai subtlety test and engine corpus_tests (46),
candidates (43), and ability_graph (42) all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(test): re-migrate 4p GameState dumps to ManaTargetRole (bigint-safe)
The card fixture wasn't the only stale artifact the main-merge left behind:
two committed persisted-GameState dumps still carried the legacy
`Effect::Mana.target: Option<TargetFilter>` shape, so every test that restores
them panicked with `missing field 'role'` while deserializing the 4p state:
- dina_conqueror_4p.json.gz (8 mana targets)
analysis::resource::tests::dina_untargeted_drain_4p_cover_is_not_vetoed_by_a_library_cost_static
- witherbloom_sprout_lumaret_simple_4p.json.gz (4 mana targets)
loop_shortcut::{ai_collapse_candidate_is_clamped_to_the_accepted_bound,
scheduled_collapse_renders_no_unbounded_badge,
unregistered_axis_still_renders_its_infinity_badge,
accepted_fixed_count_bounds_the_boundary_collapse_prompt,
two_accepts_in_one_phase_bound_the_collapse_to_the_smallest_accepted_count}
A GameState deserializes through typed serde before any restore-time migration
hook runs, so the dump itself must carry the role encoding. Re-migrated both
via the script's `--state` mode; all roles resolved to Recipient (Priest of
Forgotten Gods, The Warring Triad), fail-loud clean.
Also apply the bigint-lossless round-trip to `--state` mode (it previously used
raw JSON.parse/JSON.stringify): GameState dumps carry u64 object-id sentinels
that a naive round-trip mangles into `expected u64`, the same defect just fixed
in fixture mode.
Verified: both dumps re-scan with zero stale mana targets, no float/sentinel
leak; the dina resource test and all 71 loop_shortcut integration tests
(including the 6 that were failing) pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>1 parent c521e86 commit b902333
33 files changed
Lines changed: 4134 additions & 150 deletions
File tree
- crates
- engine
- src
- ai_support
- game
- effects
- parser
- oracle_effect
- oracle_ir
- types
- tests
- fixtures
- integration
- mtgish-import/src/convert
- phase-ai/src
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
41 | | - | |
42 | | - | |
43 | | - | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
| |||
1001 | 1001 | | |
1002 | 1002 | | |
1003 | 1003 | | |
| 1004 | + | |
| 1005 | + | |
| 1006 | + | |
| 1007 | + | |
| 1008 | + | |
| 1009 | + | |
| 1010 | + | |
| 1011 | + | |
| 1012 | + | |
| 1013 | + | |
| 1014 | + | |
| 1015 | + | |
| 1016 | + | |
| 1017 | + | |
| 1018 | + | |
| 1019 | + | |
| 1020 | + | |
| 1021 | + | |
| 1022 | + | |
1004 | 1023 | | |
1005 | 1024 | | |
1006 | 1025 | | |
| |||
1280 | 1299 | | |
1281 | 1300 | | |
1282 | 1301 | | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
| 1305 | + | |
| 1306 | + | |
| 1307 | + | |
| 1308 | + | |
| 1309 | + | |
| 1310 | + | |
| 1311 | + | |
| 1312 | + | |
| 1313 | + | |
| 1314 | + | |
| 1315 | + | |
| 1316 | + | |
| 1317 | + | |
| 1318 | + | |
| 1319 | + | |
| 1320 | + | |
| 1321 | + | |
| 1322 | + | |
| 1323 | + | |
| 1324 | + | |
| 1325 | + | |
| 1326 | + | |
| 1327 | + | |
| 1328 | + | |
| 1329 | + | |
| 1330 | + | |
| 1331 | + | |
| 1332 | + | |
| 1333 | + | |
| 1334 | + | |
| 1335 | + | |
| 1336 | + | |
| 1337 | + | |
| 1338 | + | |
| 1339 | + | |
| 1340 | + | |
| 1341 | + | |
| 1342 | + | |
| 1343 | + | |
| 1344 | + | |
| 1345 | + | |
| 1346 | + | |
| 1347 | + | |
| 1348 | + | |
| 1349 | + | |
| 1350 | + | |
| 1351 | + | |
| 1352 | + | |
| 1353 | + | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
| 1359 | + | |
| 1360 | + | |
| 1361 | + | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
| 1367 | + | |
| 1368 | + | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
1283 | 1378 | | |
1284 | 1379 | | |
1285 | 1380 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3159 | 3159 | | |
3160 | 3160 | | |
3161 | 3161 | | |
3162 | | - | |
3163 | | - | |
3164 | | - | |
| 3162 | + | |
| 3163 | + | |
| 3164 | + | |
| 3165 | + | |
| 3166 | + | |
| 3167 | + | |
| 3168 | + | |
| 3169 | + | |
| 3170 | + | |
| 3171 | + | |
| 3172 | + | |
| 3173 | + | |
| 3174 | + | |
| 3175 | + | |
| 3176 | + | |
3165 | 3177 | | |
3166 | 3178 | | |
3167 | 3179 | | |
| |||
6555 | 6567 | | |
6556 | 6568 | | |
6557 | 6569 | | |
| 6570 | + | |
| 6571 | + | |
| 6572 | + | |
| 6573 | + | |
| 6574 | + | |
| 6575 | + | |
| 6576 | + | |
| 6577 | + | |
| 6578 | + | |
| 6579 | + | |
| 6580 | + | |
| 6581 | + | |
| 6582 | + | |
| 6583 | + | |
| 6584 | + | |
| 6585 | + | |
| 6586 | + | |
| 6587 | + | |
| 6588 | + | |
| 6589 | + | |
| 6590 | + | |
| 6591 | + | |
| 6592 | + | |
| 6593 | + | |
| 6594 | + | |
| 6595 | + | |
| 6596 | + | |
| 6597 | + | |
| 6598 | + | |
| 6599 | + | |
| 6600 | + | |
| 6601 | + | |
| 6602 | + | |
| 6603 | + | |
| 6604 | + | |
| 6605 | + | |
| 6606 | + | |
| 6607 | + | |
| 6608 | + | |
| 6609 | + | |
| 6610 | + | |
| 6611 | + | |
| 6612 | + | |
| 6613 | + | |
| 6614 | + | |
| 6615 | + | |
| 6616 | + | |
| 6617 | + | |
| 6618 | + | |
| 6619 | + | |
| 6620 | + | |
| 6621 | + | |
| 6622 | + | |
| 6623 | + | |
6558 | 6624 | | |
6559 | 6625 | | |
6560 | 6626 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1018 | 1018 | | |
1019 | 1019 | | |
1020 | 1020 | | |
1021 | | - | |
1022 | | - | |
1023 | | - | |
1024 | | - | |
1025 | | - | |
1026 | | - | |
| 1021 | + | |
| 1022 | + | |
| 1023 | + | |
| 1024 | + | |
| 1025 | + | |
| 1026 | + | |
| 1027 | + | |
| 1028 | + | |
| 1029 | + | |
| 1030 | + | |
| 1031 | + | |
| 1032 | + | |
| 1033 | + | |
1027 | 1034 | | |
1028 | 1035 | | |
1029 | 1036 | | |
| |||
0 commit comments