feat(engine): gate continuous statics on the top card of the library#5692
Conversation
CR 401.1 + CR 401.5: add `StaticCondition::TopOfLibraryMatches { filter }`, a
continuous-static gate that matches the top card of the source controller's
library (`library[0]`) against a characteristic `TargetFilter`. Covers the whole
"As long as the top card of your library is <X>" class: Vampire Nocturnus
(black), Mul Daya Channelers (creature/land card), Conspicuous Snoop (Goblin
card), Crown of Convergence and Chittering Illuminator (creature card), Skill
Borrower (artifact or creature card), and Oura, the Imitator (Faerie or instant
card).
- Parser: `parse_top_of_library_condition` composes the existing
`parse_bare_predicate_disjunction` seam (shared with the recipient/attached
"is a <filter>" paths), so bare colors, the informational " card" suffix, and
"X or Y" disjunctions fold with no bespoke type parsing; wired into
`parse_state_presence_conditions` in `oracle_nom/condition.rs`.
- Runtime: `layers::evaluate_condition_with_context` resolves the controller's
library top and matches its printed characteristics (a library card matches on
characteristics alone — no zone constraint on the filter). The `ability_rw`
profile reads `StateKind::HandLibrary`, so a draw/scry/surveil/mill/shuffle
re-evaluates the gate.
- Exhaustive `StaticCondition` classifiers updated in lockstep
(object-population, entry-perturb, legacy, rw-profile, scan, coverage feature +
formatter, unspent-mana) plus the ability- and trigger-condition bridges (no
effect-resolution / intervening-if equivalent → `None`).
Tests: parser (color/type/subtype/disjunction), runtime (library-top read,
discriminating on filter, player scope, and empty library), and end-to-end
(Vampire Nocturnus, Mul Daya Channelers).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements the TopOfLibraryMatches static condition to support cards that check the characteristics of the top card of a player's library (such as Vampire Nocturnus). The changes span the parser, static condition evaluation, and coverage tracking, along with comprehensive unit tests. The review feedback highlights several violations of the repository's style guide regarding the format of Magic Comprehensive Rules (CR) annotations, specifically pointing out that compound annotations (e.g., CR 401.1 + CR 401.5 or CR 401/402) do not match the required single-reference regex format.
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.
Parse changes introduced by this PR · 8 card(s), 6 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Request changes: the parser and evaluator are correctly scoped, but library-top mutations do not invalidate layers, leaving these continuous effects stale.
🔴 Blocker
-
crates/engine/src/game/layers.rs:1343-1361evaluatesTopOfLibraryMatchesfromlibrary.front(), whilecrates/engine/src/game/effects/shuffle.rs:68-77andcrates/engine/src/game/effects/change_zone.rs:153-157shuffle that vector without callingmark_layers_full.crates/engine/src/game/zones.rs:827-846only dirties hand/battlefield and active graveyard-membership changes;zones.rs:1094-1184also places a card at library index0without invalidation. Vampire Nocturnus reads: “As long as the top card of your library is black, this creature and other Vampire creatures you control get +2/+1 and have flying.” After a shuffle, mill, or put-on-top event, that truth must be recomputed under CR 611.3a.Add a shared active-library-top-static dependency query at the layers/zone seam, use it for every potentially top-changing library move and shuffle helper, and call
mark_layers_fullwhen it is live.
🟡 Non-blocking
crates/engine/src/game/layers.rs:12164-12258directly tests predicate truth after manual library edits, whilecrates/engine/src/parser/oracle_static/tests.rs:7006-7055tests lowering only. Add production-path regressions with an active continuous static: initial matching top applies, then real shuffle, Library → Graveyard, and top-placement each flip the resulting characteristics afterflush_layers.
✅ Clean
crates/engine/src/parser/oracle_nom/condition.rs:1415-1451uses composable nom parsing, a terminal-boundary guard, and the existing filter vocabulary; the parse-diff measures the intended eight cards/six signatures.crates/engine/src/game/layers.rs:1349-1361reads hidden library information only inside engine evaluation and exposes only rules-mandated derived permanent characteristics.
Recommendation: request changes—wire library-top invalidation through the existing full-layer flush boundary and add the real mutation-path regressions before re-review.
…yMatches statics Address review on phase-rs#5692 (CR 611.3a blocker). The TopOfLibraryMatches static is evaluated in the layer system from `library.front()`, but library-top mutations did not force a layer recompute, so a continuous static gated on the top card (Vampire Nocturnus: "as long as the top card of your library is black, ... get +2/+1 and have flying") went stale after a shuffle, mill, or put-on-top. CR 611.3a: a continuous effect from a static ability isn't locked in — it applies to whatever its text indicates at any moment, so the top changing must re-evaluate it. Mirror the existing graveyard-membership invalidation pattern: - `any_active_static_reads_top_of_library` — shared dependency query (recursing through And/Or/Not) for a live TopOfLibraryMatches static. - `mark_layers_full_if_top_of_library_static_live` — single authority that self-gates on that query and marks layers full, so routine library churn stays cheap when no such static exists. Wired into every top-changing library seam: - `zones::move_to_zone` — new library branch (covers draw off the top, mill into a graveyard, and any other library entry/exit); the prior hand/battlefield and graveyard marks did not cover library moves. - `zones::move_to_library_at_index` — the put-on-top path that bypasses move_to_zone. - `effects::shuffle` and `effects::change_zone::shuffle_library` — after the reorder. Production-path regressions (an active Flying-granting static, real mutations, flush_layers): initial black top grants Flying; then mill-to-graveyard and put-on-top each strip it, and a real shuffle both marks layers Full and leaves Flying matching the recomputed top. Full engine lib suite green (16249 passed); CI-exact clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — wired the library-top invalidation through the full-layer flush boundary and added production-path regressions (commit b700705). 🔴 Blocker — library-top mutations now invalidate layers (CR 611.3a). Mirrored the existing
Called at every top-changing seam you flagged:
🟡 Production-path regressions (
Full engine lib suite green (16249 passed, 0 failed); CI-exact clippy ( |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes: library-top statics still remain stale after Scry, Surveil, and library-returning Dig reorder the top card outside the new invalidation seam.
🔴 Blocker
-
crates/engine/src/game/engine_resolution_choices.rs:580-587handles Scry by directly retaining and inserting library cards;:1894-1903does the same for Dig withkept_destination == Library; and:5064-5067is the shared Surveil keep-on-top helper. None callsmark_layers_full_if_top_of_library_static_live.finalize_rules_stateonly flushes an already-dirty layer cache, so after any of these reorder paths a clean cache remains clean and aTopOfLibraryMatchesstatic continues to use the pre-choice top card. Please make direct library reordering use one shared invalidating primitive, or mark through the helper after all three paths, and add production-path regressions with an active top-gated static.
🟡 Non-blocking
- None.
✅ Clean
crates/engine/src/game/layers.rs:2278-2324correctly detects nestedAnd/Or/Nottop-of-library gates and self-gates the full invalidation.crates/engine/src/game/zones.rs:856-857plus both shuffle helpers cover ordinary library zone moves and shuffle paths.- The parse-diff artifact measures the intended eight cards across six signatures; the follow-up head changes only runtime invalidation files.
Recommendation: request changes—cover the remaining Scry, Surveil, and Dig reordering paths with the same shared invalidation authority, then re-review.
Second review round on phase-rs#5692. The first fix covered the zone-move and shuffle seams, but Scry, Surveil, and library-returning Dig reorder the top card by editing `player.library` directly (retain + insert), bypassing `move_to_zone`. `finalize_rules_state` only flushes an already-dirty layer cache, so a clean cache survived these reorders and a `TopOfLibraryMatches` static kept using the pre-choice top card (CR 611.3a — the effect isn't locked in). Route all three direct-reorder paths through the same shared invalidation authority added in the previous commit: - Scry keep-on-top (`ScryChoice` resolution). - Dig with `kept_destination == Library` (kept cards placed on top). - `surveil_keep_on_top` — the shared helper backing every Surveil caller. Production-path regressions drive the real resolution (`handle_resolution_choice` for Scry/Dig, the shared helper for Surveil) with an active Flying-granting top-gated static: an initial black top grants Flying, then keeping a white card on top via each path strips it after `flush_layers`. Each card stays in the library (no zone change), so the new invalidation call is the only thing that can force the recompute — the tests fail without it. Full engine lib suite green (16252 passed); CI-exact clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for catching the direct-reorder paths — covered all three (commit 237cbbc), routing them through the same shared invalidation authority from the first round. 🔴 Scry / Surveil / Dig now invalidate (CR 611.3a):
Production-path regressions (drive the real resolution —
Each keeps a white card on top and asserts Flying is recomputed away after Full engine lib suite green (16252 passed, 0 failed); CI-exact clippy ( |
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the eight-card, six-signature top-of-library static class is implemented at the parser/layer seam, with all live library-top mutation paths invalidating the cache.
🔴 Blocker
- None.
🟡 Non-blocking
- The remaining Gemini threads request replacing compound CR citations. Refuted: this repository explicitly permits compound
CR X + CR YandCR X / CR Yannotations; the cited rules are present indocs/MagicCompRules.txt(401.5covers a changing library top and611.3amakes static continuous effects live rather than locked in). The comments are resolved without a code change.
✅ Clean
oracle_nom/condition.rs:1415composes the existing bare-predicate parser into a typedTopOfLibraryMatchescondition; the current parse artifact is exactly eight cards across six signatures.layers.rs:2278provides the shared recursive dependency query andmark_layers_full_if_top_of_library_static_live; ordinary zone moves, both shuffle seams, and direct Scry/Surveil/Dig reorders route through it.engine_resolution_choices.rs:5190,:5217, and:5233exercise real resolution paths with an active static and prove the effect is removed afterflush_layerswhen the top card becomes white.
Recommendation: approve and enqueue as enhancement; all current-head blockers are resolved.
Summary
Adds engine support for the top-of-library static condition — a new
StaticCondition::TopOfLibraryMatches { filter }that gates a continuous static on the top card of the source controller's library (library[0], CR 401.1) matching a characteristicTargetFilter. Covers the whole "As long as the top card of your library isX" class: Vampire Nocturnus (black), Mul Daya Channelers (creature/land card), Conspicuous Snoop (Goblin card), Crown of Convergence and Chittering Illuminator (creature card), Skill Borrower (artifact or creature card), and Oura, the Imitator (Faerie or instant card).Files changed
crates/engine/src/types/ability.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/game/layers.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/quantity.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_static/tests.rsCR references
library[0].Note on variant choice (not a sibling to parameterize)
TopOfLibraryMatchessits alongside the existing filter-scoped conditionsIsPresent,SourceMatchesFilter,RecipientMatchesFilter, andDefendingPlayerControls— the engine already models each object-scope as its own variant rather than one parameterizedMatchesFilter { scope }. This one is a distinct resolution context, not a leaf parameterization ofSourceMatchesFilter: it resolves a library-position card (CR 401 zone access,library[0]) rather than a battlefield object (source_id/recipient_id), so it cannot ride an object-scope axis. It reuses the runtime match (matches_target_filter) and the parser vocabulary (parse_bare_predicate_disjunction) rather than re-implementing them.Implementation method (required)
Method: not-applicable — authored directly against the
oracle-parserskill and theadd-engine-variantdiscipline (existence + parameterization + categorical-boundary checks). Pure AST + runtime change; no card-data regeneration required.Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Verification
031d5f641).Results on head
031d5f641:cargo test -p engine top_of_library— ok. 39 passed; 0 failed (4 parser + 1 runtime + 2 end-to-end new tests; 32 pre-existing top-of-library tests unaffected).cargo clippy -p engine --all-targets --features proptest -- -D warnings— Finished, 0 warnings.cargo fmt --all— clean.Anchored on
crates/engine/src/parser/oracle_nom/condition.rs(parse_recipient_is_filter_condition) — the analogous "itis a<filter>" condition that composes the sameparse_bare_predicate_disjunctionseam (bare colors, informational " card" suffix, "X or Y" disjunction).crates/engine/src/game/layers.rs(SourceMatchesFilterarm inevaluate_condition_with_context) — the analogous "object matches aTargetFilter" runtime usingmatches_target_filter+FilterContext::from_source.Final review
Self-review against the review-impl lenses on head
031d5f641. Verified:.front()on the libraryim::Vectorislibrary[0](the top, per the engine convention ingame/zones.rs); a zonelessTargetFilter::Typedmatches a library card on printed characteristics (no implicit battlefield requirement infilter_inner_for_object); theability_rwprofile readsStateKind::HandLibrary, which draw/scry/mill/shuffle write, so the gate re-evaluates when the top changes; the two population classifiers correctly place the variant in thefalsegroup (a battlefield entry never changes the library); and the ability/trigger bridges returnNone(a top-of-library gate has no effect-resolution or intervening-if equivalent). The passing runtime test discriminates on filter, player scope, and empty library. One fix applied pre-commit: a disjunction-fold comment mis-cited CR 608.2c (which governs instruction ordering, not filter disjunction) — citation removed.Claimed parse impact
As long as the top card of your library is black, ...→TopOfLibraryMatches { HasColor(Black) }plus the anthem body).... is a creature card, this creature gets +3/+3.).The condition building block recognizes the whole "top card of your library is
X" family; the CIcoverage-parse-diffsticky enumerates the full set of affected cards for the current head.Validation Failures
None.
CI Failures
None.