fix(ai): stop proposing strategically vacuous loop-shortcut Shortens - #7101
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds conservative shortcut-efficacy analysis for priority actions. Shortcut responses now use separate possibility and efficacy stages, including grouped sacrifice-for-mana actions and precast-copy responses. Game objects retain parser diagnostics with visibility-aware redaction. ChangesShortcut efficacy evaluation
Parser diagnostic visibility
Supporting engine updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/ai_support/shortcut_efficacy.rs`:
- Around line 424-445: Update any_action_may_interfere and the CastSpell
classification path so a spell whose object_window_reach result is
OwnResourcesOnly solely because of Effect::Mana is treated as MayInterfere.
Preserve confined classification for non-mana self-contained effects, while
ensuring mana-producing casts are fail-closed and return true.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1bd4a8-7262-4e2a-b5ca-e3dcd0e47ce9
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/dina_noff_turn5_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
crates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/shortcut_efficacy.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/shorten_efficacy.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested
Reviewed e72a0c8f6a04e52dce55686cc267760abe312cdc.
[HIGH] Mana-producing responses are incorrectly classified as non-interfering in a real Shorten priority window. shortcut_efficacy.rs:171-183 classifies Effect::Mana as OwnResourcesOnly; :443-445 then lets a CastSpell with that reach produce a non-interference result. But engine.rs:5513-5522 gives the Shorten responder an actual Priority window. A ritual or mana spell can fund an otherwise-unaffordable response inside that window, so accepting the shortcut would wrongly surrender a live out.
The same unsound class includes actor-owned sacrifice-for-mana actions: stage two considers them, while Effect::Mana combined with an actor-owned cost collapses to OwnResourcesOnly. Handle these paths conservatively and fail closed. Add response-level regressions for (1) a cast-mana spell and (2) an owned sacrifice-for-mana action, each enabling an otherwise-unaffordable interaction during the Shorten window. Do not accept a residual-deficiency note in place of this behavior.
The current head also lacks a current-head parse-diff receipt and CI is still pending. Those are evidence holds, but the shortcut false-negative above independently blocks approval.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
e72a0c8 to
63cc3be
Compare
|
🤖 AI text below 🤖 Response to @matthewevans' [HIGH]The change you requested is implemented and both required regressions are written, and this is now pushed — current head Every SHA below is re-stamped to the post-rebase chain, with one deliberate exception. The rebase renamed all nine commits, so the navigational SHAs — per-commit table, head, base — now name objects you can actually Note what that costs you, since it is my problem and not yours to discover: The test module is still registered in Your evidence re-checked at the current base, and it holds. The Fail-closed classification. The This also closes @coderabbitai's inline finding at Both regressions are response-level, not classifier-unit. Each drives a real 4p game dump through
Both discriminate on this fix specifically. Restoring the deleted arm flips both rows to No deficiency note stands in for the behaviour. Per-commit on Added and removed are split rather than summed, because the commit that does the most here removes The table names the live chain.
Five behavioural commits, not one — the count has grown twice since this line first said An earlier revision of this section said "every other production-file touch in the chain is zero The CR 701.21a note that remains is on a different axis ( Evidence holds you named
The adjacent case in the same class — closed, not disclosedThe untapped fetch is fixed in this PR — in the local chain; the remote branch is still The same call was made three more times, on the same reasoning. A review found that
The flagship set was re-derived from the final predicate rather than patched, and it moved. What this changes at today's card pool. Two things make this number easy to get wrong, and I got
Re-measured at the enforcement surface — 22,717 top-level abilities. Every figure below is
Read the two right-hand columns together, because they are the whole argument. 2449 of 2600 Object-level, on top of the above. A review round found that A later round measured that "the three collections" was itself too narrow by eight fields: The instrument is a twin of Why "conservative" here is a structural guarantee and not a hope. Each edit either adds a Every gate is revert-probed, and each probe is reported with the assertion it landed on. "The
The last two are the pair worth reading. Cutting the split conjunct reds the split row and nothing Which conjuncts are inert at today's pool, stated rather than left for you to discover:
Four serialization details make a census here easy to get vacuously right, and I name them because 2. This does not manifest at the tip being pushed — the comparator there is 0/0/0/0 with both Already fixed upstream — recording it only so the earlier artifacts in this thread are readable. One process noteAn instruction on my side of this work asked that the existing private helper
Happy to do the relocation as a separate change if the reuse is wanted for future rows. Known and measured — disclosed rather than heldThese are open items I chose to surface instead of holding the branch for another internal round.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== checked-out commit =='
git rev-parse HEAD
git status --short
printf '%s\n' '== PR head =='
gh pr view 7101 --repo phase-rs/phase --json headRefOid --jq '.headRefOid'
efficacy_file="$(fd -a '^shortcut_efficacy\.rs$' crates/engine/src)"
integration_file="$(fd -a '^shorten_efficacy\.rs$' crates/engine/tests)"
printf '%s\n' '== classifier declarations and Mana handling =='
ast-grep outline "$efficacy_file" --items all --type function,enum --match 'WindowReach|effect_window_reach|any_action_may_interfere'
rg -n -C 5 'Effect::Mana|fn effect_window_reach|fn any_action_may_interfere|MayInterfere' "$efficacy_file"
printf '%s\n' '== named response-level regressions =='
rg -n -C 4 'v10a_a_cast_mana_spell_that_funds_an_unaffordable_answer_keeps_its_window|v10b_an_actor_owned_sacrifice_for_mana_seat_keeps_its_window|smart_shortcut_response|RespondToShortcut' "$integration_file"Length of output: 54345
I also confirmed both named integration tests exist and assert
The original finding is addressed. Thank you for the precise evidence and the separate regression paths. 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
`smart_shortcut_response` documented "a way to break the loop" but called
`has_meaningful_priority_action` — the auto-pass gate, a strictly weaker
predicate it was never meant to reuse. On a real 4p board a seat holding only a
basic-land fetch answered Shorten against a mandatory trigger cascade it could
not touch: rules-legal under CR 732.2b, strategically vacuous, and paid for with
a real priority window.
Adds a stage-2 efficacy classifier (`WindowReach`) as a fourth orthogonal axis:
controller-relative confinement. A seat whose only actions write inside its own
resources Accepts; a seat that can reach past them still Shortens. The wildcard
arm is deliberately `MayInterfere` — the opposite of `ability_scan`'s default —
because a wrong Accept can lose a game while a wrong Shorten costs a beat.
Stage 1 is behaviorally unchanged. No CR licence is claimed for stage 2:
CR 732.2b is unconditioned and CR 732.2c requires only a different choice. This
is AI policy, and the module doc says so.
Stage 2 must classify every action stage 1 counted as meaningful, and at first it
did not. Stage 1's second disjunct, `has_activatable_sacrifice_for_mana`, reads
the state rather than the action list, and sacrifice-for-mana activations never
reach that list — so a seat whose only meaningful action was one of them cleared
stage 1 and then had stage 2 fold over a list that did not contain it. A confined
remainder meant Accept where the shipped predicate returned Shorten. That is not
a wrong classification but an absent one, and a fail-closed wildcard cannot cover
a shape it never receives. `stage_two_action_set` is that union, built from the
same predicate stage 1 uses so the two cannot drift apart, and a row asserts the
membership independently of any verdict so a later reclassification cannot
silently retire the invariant.
`smart_shortcut_response` serves two prompt shapes, so stage 2 applies at the
precast-copy window as well as the ordinary one — the same question deserves the
same answer — and a row drives that window through production
`ai_support::candidate_actions` rather than leaving it inferred. The
`predicted_winner` read is structurally absent there: that variant carries no
`proposal` field.
The two shortcut windows and the tests' reach-guards share one probe recipe
(`ai_support::shortcut_probe`) rather than parallel copies. At base the recipe was
inlined at two production sites and no test-side guard existed; early revisions of
the new guards drifted, evaluating `has_meaningful_priority_action` against the
caller's prompt state, whose `has_activatable_sacrifice_for_mana` rung is gated on
`waiting_for == Priority`, so a guard could read false where production reads
true. Sharing the recipe removes the class. The second copy is NOT unified:
`game::engine::no_living_player_has_meaningful_priority_action` still inlines the
same seven statements. Only the shared probe documents the mirroring; that second
copy carries no back-reference, so a reader standing there has no pointer to it.
Routing it through the shared probe would touch the CR 732.5 mandatory-loop
firewall — both its production callers are mandatory-loop gates: the
`LoopDetectionMode::On` auto-win block, whose own comment calls it the "entire
soundness firewall", and `interactive_loop_bridge`'s `mandatory` binding. That is
outside this change's scope, so it stays a follow-up rather than being claimed as
done.
The confinement guarantee is scoped to what it can actually promise — never a
false `Accept` from a shape it does not recognize — and the doc now names the
residual it cannot cover. A parser-swallowed clause makes an ability look
strictly MORE confined, which is the direction that loses games. Measured on
`Invoke Justice`, whose "distribute four +1/+1 counters … target player
controls" clause is dropped with no `Unimplemented` marker, leaving a lone
graveyard-recursion effect that classifies as confined.
`SearchLibrary`'s arm gains a real fold-absorption witness (`Haunting Echoes`,
whose `SearchLibrary { target_player: None }` sub-ability classifies confined on
its own and is absorbed by its sibling graveyard-exile head). The two library
rows already present are both caught by `target_player` directly and never
exercised the fold they were cited for.
Acceptance rides the real dump (844846 B, sha256
9843d5165cbbf7dd7bca4171c7888c190b7eba7e52a2ed095b44ff76fadd7886) driven
through the production restore and `apply()`, not a synthetic scenario.
Assisted-by: ClaudeCode:claude-opus-4.8
…window
Stage two classified `Effect::Mana` as `WindowReach::OwnResourcesOnly`, so a
seat whose only answer was a ritual -- or an actor-owned sacrifice-for-mana
source -- was judged unable to interfere, and the AI accepted a loop shortcut
that surrendered a live out. `ShortcutResponse::Shorten` hands the responder a
real `Priority` window; inside it, mana is fungible reach.
The defect was a CR 106.4 quotation truncated before the clause that refutes it.
The arm quoted "that mana goes into a player's mana pool" and concluded
board-neutrality implies non-interference. The rule's next sentence is "From
there, it can be used to pay costs immediately". Board-neutrality and
non-interference are different propositions; a correct citation was attached to
the wrong one.
`Effect::Mana` now falls through to the fail-closed `_ => MayInterfere` wildcard.
The allowlist drops from four shapes to three. The *spending*-side
`AbilityCost::Mana` arm deliberately stays -- paying a cost consumes reach, it
does not create it -- and the arm now says so, so a future reader does not read
its survival as an oversight.
Regressions on the real 4p dump, both required by the maintainer:
v10a a cast mana spell (Dark Ritual) funding an otherwise-unaffordable Bolt
v10b an actor-owned sacrifice-for-mana seat (Lotus Petal, Composite[Tap,
Sacrifice{SelfRef}]) re-admitted through `stage_two_action_set`
Revert probe executed: restoring the deleted arm flips exactly 3 of 20 rows
(the unit row, v10a, v10b) and leaves 17 green -- including both ACCEPT arms,
the flagship, v9b, and the positive control -- then the files were restored and
verified byte-identical.
Two fixture facts are load-bearing and documented at their sizing sites: Lotus
Petal's capacity is exactly 1 (`AnyOneColor` yields `vec![type; amount]`),
leaving a 1-mana margin over P2's cheapest alternative; and any staged P2 source
contributing 2 or more unlocks Angel of the Ruins' plainscycling, which would
silently destroy v10b's attribution. Both assertions fail loudly instead.
`give_bolt_with_cost` seeds `base_mana_cost` as well as `mana_cost`:
`seed_live_characteristics_from_base` reseeds `mana_cost` from the base on every
layer pass, and every consumer reaches the object through `shortcut_probe`,
which flushes layers -- so setting only `mana_cost` would have made the
"otherwise-unaffordable" premise measure nothing.
Assisted-by: ClaudeCode:claude-opus-4.8
…as false Review-impl returned Semantic-Impact PASS and Maintainer-Simulation PASS on the parent commit; these are its three [LOW] and two [NIT] items. No classifier logic changes -- with comments stripped, the `shortcut_efficacy.rs` delta is a single assertion-message string. - `give_bolt_with_cost`'s rationale for setting `base_mana_cost` was false for the objects it stages. `reset_recipient_to_base` is reached only over `battlefield_phased_in_ids()`, and the incremental arm's recipient set is battlefield entrants plus attachment hosts -- neither reaches a hand object. `sync_missing_base_characteristics` back-fills base FROM live, the opposite direction. The code is kept because it is defensively correct if the staging zone ever changes; the comment now says that instead of asserting a mechanism that does not fire here. A false justification is worse than none: it gets trusted. - The CR 701.21a quotation began at the rule's second sentence -- the same half-quote shape this branch exists to repair. It now starts at sentence one, "its controller moves it from the battlefield directly to its OWNER's graveyard", which is what bounds the conclusion: `SelfRef` proves control, never ownership, so a controlled-but-not-owned source puts a card in another player's graveyard while the leg still answers `OwnResourcesOnly`. Not repairable at this seam -- the AST carries no ownership -- so the limit is named rather than papered over. - `give_ironworks` had become a byte-equivalent duplicate of `give_parsed_card`; field-by-field equivalence was verified, then it was collapsed to a one-line delegation. The divergence hazard had already fired in the parent commit, where `base_mana_cost` reached one construction path and not the other. - The `v1` NON-VACUITY note credited the wrong assertion: `stage_two_action_set` only appends, so `stage_two == flat` cannot catch a non-sacrifice hand or graveyard mana activation. The `non_pass.len() == 1` reach-guard above does. - `summoning_sick = false` was a proven no-op (`create_object` documents that it does not set the flag; `GameObject::new` defaults it false). Removed, with a comment recording why it is absent so it is not re-added. Assisted-by: ClaudeCode:claude-opus-4.8
…sions
Review-impl r2 passed all five of the maintainer's acceptance criteria and
returned no BLOCK; these are its three test-quality [LOW] items. Test-only --
no production seam is touched, and each new assertion has a mutation that was
RUN and observed to redden it, not merely derived.
- v10b's ordinary-mana-source control rested on two negatives (the Sol Ring
activation is absent from the flat list; stage two equals the flat list) whose
outcome was dominated by an upstream conjunct. `stage_two_action_set` chains
`meaningful_sacrifice_mana_actions` over `activatable_object_mana_actions`,
which for a probe at `Priority` is literally
`mana_sources::activatable_mana_actions_for_player` -- so a Sol Ring that was
never swept is filtered out for a reason the row could not distinguish from
the penalty filter it means to measure. MEASURED: with the new guard removed
and the Sol Ring tapped, BOTH pre-existing negatives still pass. They were
green for the wrong reason.
Two guards close it at two different levels. The sweep function is `pub`, so
the row now asserts definitionally that the Sol Ring IS in the list stage two
filters; and it asserts the consequence the row's own doc already stated as
fact but never checked -- that the Sol Ring's capacity of 2 unlocks Angel of
the Ruins' {2} plainscycling in the flat list. Swapping in a capacity-1 source
reddens only the second, so they are independently breakable rather than
redundant.
- v10a bounded only its ACCEPT arm. Its SHORTEN arm's discrimination rested on
a derived-but-unasserted fact, so a future fixture or capacity change that
added one MayInterfere action would over-determine the row silently instead
of reddening. It now carries the threshold sentinel its sibling already had.
MEASURED at 2 (the Ritual cast plus Terramorphic Expanse); staging one extra
two-capacity source makes it 3 and reddens the sentinel.
- v10a asserted only that the {B}{B}{B} Bolt is NOT castable, leaving the
funding half -- the half its own name claims -- as prose. The row now measures
it on the production instrument the way v10b's lemma does: a quarantined clone
casts the Ritual through `apply`, drives the stack, and re-probes. A zone
reach-guard pins that the Ritual actually resolved (CR 608.2n) before the
funding assertion runs. Printing the Bolt at {B}{B}{B}{B} reddens the funding
assertion alone; collapsing the drive bound reddens the zone guard alone.
The row's doc previously said "The row does NOT assert the post-resolution
board". That became false with this change, so it is corrected rather than
left to mislead.
CR 117.1d, CR 601.2g and CR 608.2n were each grepped from the rules text before
being written. The first draft of the last one said 608.2m from memory, which is
a different rule ("if it leaves the stack once it starts to resolve, it will
continue to resolve fully"); the verification step caught it.
Assisted-by: ClaudeCode:claude-opus-4.8
Two review findings, both cases of an assertion claiming more than it
measured.
The SHORTEN-arm sentinel bounded the action set's COUNT at 2 while its own
prose claimed its MEMBERSHIP ("the ACCEPT arm's single fetchland PLUS the
Ritual cast, and nothing else"). A fixture or capacity change that dropped
the fetchland and added some unrelated MayInterfere action satisfies both
`len() == 2` and the ritual reach-guard, so the row would have gone on
measuring the wrong pair silently. Partitioning on the Ritual and asserting
the remainder equals the ACCEPT arm's set closes that gap and pins the
Ritual leg in the same equality: an empty or doubled partition reddens.
The Angel guard matched on card name alone, but the fact it exists to
establish is specific — that the Sol Ring's two mana unlock the {2}
PLAINSCYCLING activation from HAND, which is what makes withholding a
verdict assertion below it correct rather than evasive. A bare name match
is also satisfied by some other Angel ability or an Angel in another zone.
Ability index 0 is measured off the fixture, not assumed: object 210
carries exactly one parsed ability, tagged Cycling with activation_zone
Hand.
Both tightenings are additive; no existing assertion was weakened. Both
were revert-probed, and the SHORTEN probe was chosen to be invisible to the
two pre-existing guards so the new assertion is provably the one catching
it.
Assisted-by: ClaudeCode:claude-opus-5
The mana-reach fix deleted the `Effect::Mana` allowlist arm because mana produced inside the priority window a Shorten hands the responder can fund an otherwise-unaffordable answer. The same mechanism was still live one arm over: `effect_window_reach` allowlisted ANY library-to-battlefield move as `OwnResourcesOnly`, so an untapped fetch — whose land taps for mana in that same window — read as confined. Measured on the production classifier, Crop Rotation and Nature's Lore both classified `OwnResourcesOnly`. `enter_tapped` was already destructured and ignored in that arm. The gate now requires the AST to PROVE a confined entry rather than merely suggest one (CR 110.5b: permanents enter untapped unless something says otherwise): `EtbTapState::Tapped`, with no conditional `enters_modified_if` rider that could change the arrival (CR 614.12 + CR 614.12a), not `enters_attacking` (CR 508.4), and `enters_under` absent or the actor. `Unspecified` and `Untapped` both fall out to `MayInterfere`. `SearchLibrary`'s `split` is the second door onto the battlefield — it moves its own found cards with no `ChangeZone` node — and is gated the same way. Both edits are conjunctive, so they can only move verdicts toward `MayInterfere`; neither can manufacture a false Accept. The gate is destination-scoped rather than tied to the anaphoric disjunct, because ownership does not stop an untapped land from producing mana. This DOES reclassify a live class, and an earlier revision of this message claimed otherwise. That claim came from a census that counted AST nodes over the whole card document; both halves were wrong. `printed_cards.rs` copies only `card_face.abilities` into `obj.abilities` and the classifier folds over `object.abilities` alone, so nodes in `triggers`/`static_abilities`/ `replacements` never reach it; and a verdict is a fold over a whole ability, so an ability already `MayInterfere` for an unrelated reason cannot flip whatever its `ChangeZone` node says. Re-measured at the enforcement surface — 22,717 top-level abilities in this candidate's own projection — this commit's two conjuncts move 195 of the 522 abilities that were still `OwnResourcesOnly` after the mana arm's deletion (37.4%), all in the `OwnResourcesOnly` -> `MayInterfere` direction, with 0 moving the other way. The `split` conjunct is inert at today's pool (9 of 9 battlefield-primary carriers already print tapped, none routes `rest_destination` to the battlefield); it is in because that door would otherwise stay open for the first card that walks through it. Adds `v10c`, a response-level regression driving the real 4p dump to a real `RespondToShortcut` and asserting on `smart_shortcut_response`, with a tapped control on the same board so the tap axis is what the row measures. Assisted-by: ClaudeCode:claude-opus-5
…adable
`object_window_reach` folded only `obj.abilities`. `printed_cards.rs` splits one
card face into four collections — `abilities`, `replacement_definitions`,
`static_definitions` and `base_trigger_definitions` — so a card whose entire
function lives in one of the other three was classified from the empty half.
Measured witness: Stunning Reversal projects `abilities[0] = ChangeZone{origin:
None, destination: Exile, target: SelfRef}`, which is confined on every conjunct
this module reads, while its function is `replacements[0] = {event: GameLoss,
mode: Mandatory}`. A seat holding it read `OwnResourcesOnly`, so the shortcut
window was Accepted for the one card that exists to survive it.
Both entry points now return `MayInterfere` on the presence of any of the three
collections. The activation path gets the gate for its own reason rather than by
symmetry: activating an ability is itself a game event, so a trigger on the same
object can fire off the activation (CR 603.2) or off the cost being paid. The
gate is presence, not content — this module has no classifier for those
definition types, and the named upgrade path is to classify them the way
`ability_window_reach` classifies `AbilityDefinition`.
Same review round, one door over: `destination: Zone::Hand` leaves the confined
set. A card put into hand is a castable card; after the spell that put it there
resolves the active player receives priority (CR 117.3b) and priority then passes
in turn order (CR 117.3d), so the responding seat gets it back still inside this
window and a hand arrival is not provably confined.
That hand gate went in on the `ChangeZone` arm only, and a review round found the
`SearchLibrary` split still routing cards to hand unchecked. Both doors now go
through one `landing_zone_is_confined` authority rather than two call sites
answering the same question — two answers is how they drifted apart. The split is
destructured `..`-free in the closure pattern for the same reason the `ChangeZone`
arm is: a future `rest_enter_tapped` must be a compile error here, not a silently
ignored arrival modifier.
All changes are strictly narrowing — early returns to the absorbing value and one
more conjunct each — so `May -> Own` is 0 by construction, not by census.
Corpus, on this candidate's own SHA-bound projection of 22,717 top-level
abilities: combined 2600 -> 151 confined (2449 flips, 94.2%, 0 widen); the entry
gate alone 522 -> 151 (371, 71.1%). The hand authority accounts for 176 of those,
split by door: 166 on `ChangeZone` across 154 cards, 10 on the split across 10
cards. All 12 split carriers route something to hand (9 via `rest_destination`,
3 via `primary_destination`), so Cultivate and its class leave the confined set.
At object level, 72 printed cards have a fully-confined ability list and 19 carry
an unreadable collection (7 static-only); 53 stay confined. That is a printed-card
census and a lower bound on runtime objects — `game/stickers.rs` pushes into all
three collections at runtime.
The gate's price, stated rather than buried: Diligent Farmhand fetches a basic
land TAPPED and flips anyway, on one static scoped `active_zones: ["Graveyard"]`
that cannot touch this window. It is the only one of the 19 provably inert by its
own zone scope; presence-gating cannot see that, and classifying the collections
is what buys it back.
Tests: `an_object_whose_rules_content_this_module_cannot_read_is_never_confined`
covers all three disjuncts — replacement, trigger (through the real
`materialize_base_trigger_definitions()` wiring, with a premise that the field the
gate reads is populated) and static — each with the other two cleared first, so
every verdict is attributable to one collection.
`a_destination_is_confined_only_when_the_seat_cannot_act_on_what_lands_there`
walks the destination axis on a real parsed node whose target is `SelfRef`, so
destination is the only free variable. The split row asserts Cultivate's real
`rest_destination == Hand` premise and mutates that one field as its control:
since all 12 carriers touch hand, no real card can serve as the tap-axis positive
control, and the test says so instead of implying live coverage.
Revert-probed: each gate and each conjunct cut alone from pristine source reds its
own row and leaves its neighbours green.
Assisted-by: ClaudeCode:claude-opus-4.8
…ontent gate Review findings against a15f074, all reproduced before being fixed. `landing_zone_is_confined` matched `Battlefield`/`Hand` and closed with `_ => true`. `Zone` is a closed seven-variant enum (CR 400.1) and that wildcard was this module's only fail-OPEN default: it silently absorbed `Zone::Stack`, a live `ChangeZone` destination, so a node landing a card on the stack read as confined. CR 405.1 puts a cast spell's card on the stack and CR 608.1 resolves it once all players pass, which is strictly stronger reach than the `Hand` case this same function closed last round. The match is now exhaustive, so adding a `Zone` variant breaks the build. `Zone::Command` joins hand and stack per CR 903.8 (a commander may be cast from the command zone) and CR 114.1 (emblems carry abilities there). The object gate's own doc claimed `printed_cards` splits a card face across four collections. Measured, it writes eight more rules-bearing fields, and `obj.keywords` carries printed Cascade — which never reaches `trigger_definitions` — so a Cascade spell whose printed abilities all read confined was provably `OwnResourcesOnly` while resolving it casts a free spell of arbitrary reach inside the window. Both entry points now route through one `carries_unreadable_rules_content` authority covering every unreadable rules-bearing field rather than a curated subset, since a subset is an allowlist someone has to remember to extend. That argument applies to the list itself, so the list is now compiler-enforced. A new test destructures `CardFace` `..`-free and sorts every field into folded / gated / not-rules-bearing with a reason on each. Writing it immediately found four more unreadable fields no re-reading had surfaced — `case_state`, `class_level`, `intensity` and `attraction_lights` — all now gated. `CardFace` rather than `GameObject` because `GameObject` has 149 fields, mostly runtime state, and destructuring it would be a churn magnet blanket-`..`'d back within a round; `CardFace` has 33 and is the source `printed_cards` actually reads. Scope limit stated in the test: it does not cover `GameObject` fields written outside `printed_cards` (`game::stickers` writes only the three definition collections, which are gated). Measured on this candidate's own projection: ability-level figures are unchanged (2600 -> 151), confirming the zone fix is latent — 0 `Stack` destinations, 1 `Command` (Hellkite Courser, in `triggers`, never folded). Object level moves 72/19/53 to 72/29/43: the widening adds 10 flips (8 keywords, 1 modal, 1 additional_cost). The four fields found by the guard are 0 among survivors and real document-wide (solve conditions 15, Class 38, Case 15, Attraction 35) — latent holes, closed for the same reason as the zone arm. The protected class is untouched — Terramorphic Expanse, Evolving Wilds and Rampant Growth carry none of these fields, asserted in the census as a reach-guard that fails rather than prints. Tests: the destination table now covers all seven zones with a compile-time exhaustiveness guard that is deliberately not a mirror of the production match; the object-gate test gains keyword and spellbook disjuncts with a cleared-field control between each, so no verdict can be a constant. Assisted-by: ClaudeCode:claude-opus-4.8
…able-content gate A review round measured three holes in the object-level gate this PR added, all of the same shape: a set that was curated by hand and therefore incomplete. Seven of the gate's disjuncts had neither a test nor a revert-probe -- `modal`, `additional_cost`, `strive_cost`, `cleave_variant`, `casting_restrictions`, `casting_options`, `back_face`. Each occurred exactly once in the file, in the gate itself, so deleting any of them left the whole suite green. Witness coverage was anti-correlated with liveness: `modal` and `additional_cost` are 2 of the 10 cards the widening flips, while all four disjuncts that did have witnesses are inert at today's pool -- witnesses had been written for the newest additions rather than derived from the gate. `every_gated_card_face_field_reaches_the_gate_through_printed_cards` closes that with 12 cases, each mutating a `CardFace` and running the real `apply_card_face_to_object`, which also turns the staleness guard's comment-only CardFace->GameObject mapping into a runnable assertion. CONTROL and witness are separate freshly-created objects because `printed_cards` seeds `class_level` only while `base_characteristics_initialized` is false (CR 716.2b); re-applying to one object would have skipped the field the Class case tests. `back_face` reuses `game::specialize::empty_back_face`, promoted to `pub(crate)`, rather than duplicating a 22-field literal that would go stale the moment `BackFaceData` grew. The staleness guard bound `metadata: _` -- the one field its own comment called "mixed", and the exact path `spellbook` had already used to get into this gate. It now destructures all 8 `CardMetadata` fields with a reason each. The guard's `card_type` bucket reason was measurably wrong: `printed_cards` derives four object-level rules fields from `subtypes` alone (`Class`, `Case`, `Room`, `Attraction`), and `room_unlocks` was the one with no disjunct -- so the bucket whose job is certifying "no gate needed" was certifying a real hole. CR 709.5 makes a shared type line two static abilities that remove each locked half's rules text, and CR 709.5e lets any player unlock a half at any priority, so which halves are unlocked decides what rules text the permanent has and `obj.abilities` cannot express it. Presence gate, strictly narrowing: 0 flips at today's pool (72/29/43 unchanged; of 60 Room faces only two clear every other gate and neither is confined). Assisted-by: ClaudeCode:claude-opus-5
63cc3be to
545c840
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== checked-out commit =='
git rev-parse HEAD
echo '== current PR head =='
gh pr view 7101 --repo phase-rs/phase --json headRefOid --jq '.headRefOid'
efficacy_file="$(fd -a '^shortcut_efficacy\.rs$' crates/engine/src | head -n1)"
integration_file="$(fd -a '^shorten_efficacy\.rs$' crates/engine/tests | head -n1)"
main_file="$(fd -a '^main\.rs$' crates/engine/tests/integration | head -n1)"
echo '== relevant classifier code =='
rg -n -C 6 'fn effect_window_reach|Effect::Mana|fn any_action_may_interfere|CastSpell|ActivateAbility|indexed_ability_window_reach|cost_window_reach' "$efficacy_file"
echo '== named integration tests =='
rg -n -C 5 'v10a_a_cast_mana_spell_that_funds_an_unaffordable_answer_keeps_its_window|v10b_an_actor_owned_sacrifice_for_mana_seat_keeps_its_window|smart_shortcut_response|RespondToShortcut' "$integration_file"
echo '== integration module registration =='
rg -n -C 2 'shorten_efficacy' "$main_file"Length of output: 38893
Both action paths use the updated classification:
Both response-level regressions exist and assert
The original inline finding is addressed. ✏️ Learnings added
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== checked-out commit =='
git rev-parse HEAD
echo '== PR head =='
gh pr view 7101 --repo phase-rs/phase --json headRefOid --jq '.headRefOid'
efficacy_file="$(fd -a '^shortcut_efficacy\.rs$' crates/engine/src | head -n1)"
integration_file="$(fd -a '^shorten_efficacy\.rs$' crates/engine/tests | head -n1)"
main_file="$(fd -a '^main\.rs$' crates/engine/tests/integration | head -n1)"
echo '== Effect::Mana matches (must have no explicit OwnResourcesOnly arm) =='
rg -n -C 3 'Effect::Mana|fn effect_window_reach' "$efficacy_file" || true
echo '== action classification paths =='
rg -n -C 4 'GameAction::CastSpell|GameAction::ActivateAbility|object_window_reach|indexed_ability_window_reach' "$efficacy_file"
echo '== named response-level regressions =='
rg -n -C 4 \
'v10a_a_cast_mana_spell_that_funds_an_unaffordable_answer_keeps_its_window|v10b_an_actor_owned_sacrifice_for_mana_seat_keeps_its_window|smart_shortcut_response|ShortcutResponse::Shorten' \
"$integration_file"
echo '== integration module registration =='
rg -n -C 2 'shorten_efficacy' "$main_file"Length of output: 30944
The two action paths use this classification:
Both response-level regressions exist and assert
The high-priority finding is addressed. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The two response-level regressions cover both required paths:
The updated classifier also keeps unknown or unreadable rules content conservative. This prevents the shortcut logic from treating incomplete classifier visibility as proof of Thank you for the detailed evidence and for distinguishing the measurement SHAs from the current pushed head. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head 545c8402ef59601668867f5c475d88d3f4838858 is blocked on two remaining false-Accept paths. The Mana finding from the prior review is resolved; these findings are new and are independently verified against the current head.
🔴 Blocker
[HIGH] A locally confined fetch can still trigger a live, cross-player response elsewhere on the board. Evidence: shortcut_efficacy.rs:766-779 classifies only the chosen action's source/effect; :396-420 therefore proves a self-targeted, tapped fetch OwnResourcesOnly. But the actual trigger authority collects matching battlefield triggers for the event, including other permanents, at triggers.rs:3530-3640. A controller's Hedron Crab makes a tapped Terramorphic-style landfall action non-vacuous: it creates a target-player mill trigger after the fetch resolves. The classifier never inspects that trigger source and can emit Accept, surrendering the only window that reaches it. The local card corpus records Hedron Crab's landfall as target-player milling at data/mtgish-cards.json:12623.
Suggested fix: make the efficacy decision use the event/trigger authority for the proposed action (or conservatively return MayInterfere whenever resulting triggers have not been proven inert); add a response-level landfall/other-observer regression that would flip back to Shorten if this guard is removed.
[MED] SelfRef and Controller are treated as ownership proofs even though they only prove control. Evidence: shortcut_efficacy.rs:149-161 returns true for both; :411-420 and :473-475 use that proof to classify zone moves and sacrifices as confined. The PR itself accurately notes the counterexample at :1963-1977: sacrificing a controlled-but-opponent-owned permanent moves it to its owner's graveyard. That can change an opponent's resource / break the loop, but an all-confined action set still becomes Accept at ai_support/mod.rs:1592-1600.
Suggested fix: pass object ownership from the probe/action boundary into the confinement check, or default SelfRef/Controller to MayInterfere unless ownership is proven; add a controlled-not-owned response-level regression.
✅ Clean
The previous mana false-Accept is fixed on this head: Effect::Mana now reaches the conservative fallback at shortcut_efficacy.rs:423-454, with current-head response-level coverage for cast mana and sacrifice-for-mana. The Room presence gate and its printed-card pipeline test are also conservative.
Recommendation: request changes. The design must make the two ownership/event paths fail closed before this policy can accept a shortcut.
`WindowReach::OwnResourcesOnly` was emitted from an AST-only proof whose
premises are live-state facts the AST structurally cannot carry. The module
was fail-closed on unrecognized *variants* but fail-open on known-incomplete
*inputs*. Three gates close three premises, all at `any_action_may_interfere`
— the one seam that already holds `&GameState`.
Ownership (CR 110.2, CR 109.5, CR 701.21a). `SelfRef`/`Controller` prove
CONTROL, never ownership, and a sacrificed permanent goes to its OWNER's
graveyard — so a controlled-but-not-owned permanent changes another player's
resources while every AST predicate still reads confined. Proven set-level:
`TargetFilter::Controller` is a bare unit variant and `Typed` is a set
predicate, so per-object threading is structurally impossible.
Board observers (CR 603.2, CR 603.6a, CR 113.6). Trigger matching is a
board-wide scan; the fold read one object. A tapped, self-targeted fetch was
provably confined while an opponent's Hedron Crab ("Landfall — Whenever a land
you control enters, target player mills three cards") reached every player.
Relief is a disjunction of independently sufficient reliefs, gated
zone-of-function first via the existing CR 113.6 authority. The subject
carve-out is `SelfRef`-EXACT: reusing `filter_is_actor_owned` returns true for
`Typed{controller: You}` — exactly the crab's filter — and provably fails to
fix this, which is pinned as a negative control.
Parse completeness. A parse cannot testify about its own omissions, so
`parse_warnings` now gates confinement and moves from the staleness guard's
NOT-RULES-BEARING bucket to GATED. That one line — "parser diagnostics, never
consulted at runtime" — was the root cause stated as a certification.
The relief predicate lives in `ai_support`, not `game/triggers.rs`: relief is
not intrinsic to a trigger, it holds only relative to the confined-action
allowlists this module defines. `LifeLost` relief is sound only because
`AbilityCost::PayLife` falls to `MayInterfere`; its doc names both allowlists
and that witness so widening either forces a re-audit.
Measured, not assumed: `Milled*`, `EntersOrAttacks` and
`EntersOrHauntedCreatureDies` are NOT relieved — their matchers read a generic
`ZoneChanged` that an allowlisted `ChangeZone` emits (CR 701.17a), so relieving
them would have been a fresh false-Accept. The flagship acceptance seat keeps
`Accept` (171 defs -> 168 zone-gated -> 3 survivors, all relieved); P1 is safe
via the stage-1 short-circuit, not because no board fires.
All 14 revert-probes executed PASS->FAIL, none non-discriminating. 30 CR
numbers verified against the rules text; one self-caught miscite corrected
(701.12a is Exchange, not Fight -> 701.14a).
Assisted-by: ClaudeCode:claude-opus-4.8
…tries
Three defects found by independent review of the previous commit.
Hidden-information leak, introduced by that commit (CR 400.2: a hand and a
library are hidden zones). `GameObject::parse_warnings` was not cleared by
`hide_card`, so an opponent received a card whose `name` read "Hidden Card"
while its diagnostics carried the printed rules text verbatim — and
`SwallowedClause.description` IS that text. Because the field is skipped when
empty, it appeared on exactly the 891 cards that carry one, making it a
per-card fingerprint for reading hands and libraries.
The sibling redactor `redact_face_down_identity_from_observer` had the same
hole — measured, not assumed: a manifested card's diagnostics survived the
face-down transform and reached the observer's payload (CR 708.2, CR 708.5).
Both are cleared, each with a matched-pair test: empty for the non-viewer,
non-empty for the owner, so the negative arm cannot pass vacuously.
The root cause is structural and larger than this field: `hide_card` is an
allowlist-shaped problem implemented as a denylist, so any new `GameObject`
field defaults to LEAKED. Passing the serialization partition guard proves
nothing about visibility. An audit run against the wire found `base_name`
(every hidden card's real name), `spellbook`, `unimplemented_mechanics`,
`token_rules_text` and the `base_*` characteristics all leaking. Those are
PRE-EXISTING and are deliberately NOT fixed here — the correct repair is to
project a minimal object rather than extend a hand-maintained clear-list, and
that carries its own test matrix and client-consumer audit.
A battlefield entry is now confined only from `Zone::Library`. The previous
commit's doc claimed the arriving card "is still in the library, so neither
input contains it" — false for 46 abilities returning from graveyard, hand or
exile, whose objects ARE in `state.objects`. Witness: Helping Hand returning a
Fleshbag Marauder is classified confined while making every player sacrifice.
The precise fix needs an entering `ObjectId` that does not exist at decision
time (`Typed{controller: You}` is an unchosen target), so this narrows the
allowlist instead — moving verdicts only toward `MayInterfere` and making the
paragraph true rather than retreating from it. Measured cost: of 493
document-wide tapped entries, Library's 247 keep classification; 245 lose it.
The flagship fetch rows are untouched — that disjunct already required a
library origin.
`t2_10`'s decoder pin is now order-insensitive, so reordering `TriggerMode`
no longer reds an `ai_support` test.
All four revert-probes executed, each the sole failure of its suite. Full
engine suite 23,638 passed / 0 failed; workspace check and clippy clean.
Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Both findings fixed — head
|
| field | status |
|---|---|
base_name |
leaks every hidden card's real name verbatim — GameObject::new sets base_name: name.clone() |
spellbook, unimplemented_mechanics, token_rules_text |
leak |
base_power/base_toughness and the other base_* characteristics |
uncleared |
The root cause is structural: hide_card is an allowlist-shaped problem implemented as a denylist, so any new GameObject field defaults to leaked. Passing the serialization partition guard proves nothing about visibility — that is exactly how this PR's own regression got in. The correct repair is to project a minimal object rather than extend a hand-maintained clear-list, which carries its own test matrix and client-consumer audit. Blast radius for the cheap ones is measured: base_name and spellbook have zero frontend consumers, unimplemented_mechanics has three. Say the word and I will open that PR — it is a bigger correctness issue than the one you filed, and it is not mine to bundle into this review silently.
A correction to my own previous reply, and a cost you should weigh
My last reply's residual paragraph claimed the arriving card "is still in the library, so neither input contains it." That was false for 46 abilities returning from graveyard, hand or exile — those objects are in state.objects. Witness: Helping Hand returning a Fleshbag Marauder was classified confined while making every player sacrifice. Pre-existing, not a regression, but the prose read as closed when it was not, which is worse than the deferral it replaced.
A battlefield entry is now confined only from Zone::Library. The precise fix needs an entering ObjectId that does not exist at decision time (Typed{controller:You} is an unchosen target), so this narrows the allowlist instead — moving verdicts only toward MayInterfere.
Measured cost, so you can push back on it: of 493 document-wide tapped battlefield entries, Library's 247 keep classification and 245 lose it. The flagship fetch rows are untouched — that disjunct already required a library origin. This is the judgement call in the change most worth challenging; the v10d Accept control is what proves it is a distinction rather than a blanket flip.
Evidence
- 18 revert-probes executed (14 from the first commit, 4 from the fix), each observed
PASS → FAIL, each the sole failure of its suite. None asserted without running. - Full engine suite 23,638 passed / 0 failed;
cargo check --workspaceandclippy -D warningsclean. - 42 CR numbers verified against the rules text, 0 unverified — including two self-caught miscites corrected during the work (
701.12ais Exchange, not Fight →701.14a; and the hidden-zone rule is400.2, not400.1/108.3). - One measured deviation from the approved plan:
Milled*,EntersOrAttacksandEntersOrHauntedCreatureDiesare not relieved — their matchers read a genericZoneChangedthat an allowlistedChangeZoneemits (CR 701.17a), so relieving them would have shipped a fresh false-Acceptof exactly the class you filed.
CI has not yet bound to this head; treat it as unreported until it does.
|
Current head
This is a non-terminal CI/evidence hold, not an approval: Rust test shards are still pending, and the existing |
|
🤖 AI text below 🤖 Both evidence holds are cleared at
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested — the prior two fixes are resolved, but this head still has a false-Accept path and an incomplete diagnostic-face contract.
🔴 Blocker
[HIGH] A tapped library search can still select an interfering permanent, while the classifier treats the search as confined. Evidence: crates/engine/src/ai_support/shortcut_efficacy.rs:500-510 admits Library -> Battlefield with TargetFilter::Any whenever entry is tapped; :1080-1091 scans only presently functioning triggers, which correctly excludes the unchosen library card. That is insufficient proof: Elvish Reclaimer searches for any land and puts it onto the battlefield tapped (data/mtgish-cards.json:8334), and the selectable Bojuka Bog has an ETB that can exile any target player's graveyard (data/mtgish-cards.json:3217). This is a live cross-player choice after Shorten, yet any_action_may_interfere can return false. The current regression encodes the same unsupported premise by expecting Accept for Rampant Growth at crates/engine/tests/integration/shorten_efficacy.rs:2808-2888; its vanilla-Swamp fixture does not cover a legal non-inert selection.
Suggested fix: keep this class fail-closed until stage two can prove every legal library selection inert (or conservatively classify library-to-battlefield searches as MayInterfere). Add a response-level Elvish-Reclaimer-to-Bojuka-Bog regression that flips back to Accept if that guard is removed.
[MED] parse_warnings is declared as data for the displayed face but is not transferred when a double-faced card transforms. Evidence: the new field is on GameObject at crates/engine/src/game/game_object.rs:552-578, but BackFaceData has no equivalent at :215-245; consequently face apply/swap only copies the prior fields (crates/engine/src/game/printed_cards.rs:267-348) and transform snapshots omit diagnostics (:722-751; game/transform.rs:121-130). A front-clean/back-warning card can therefore look clean to carries_unreadable_rules_content, and the inverse keeps a stale warning after transforming back.
Suggested fix: thread a serde-defaulted diagnostic vector through BackFaceData, both snapshot helpers, and both face-application directions, with a matched front/back transform regression.
Recommendation: request changes. The library-selection authority needs a sound boundary before this policy can accept a shortcut; complete the face-data threading in the same change or remove the new serialized diagnostic field from this PR.
|
🤖 AI text below 🤖 Filed the pre-existing leak separately as #7201 — Hidden-zone card names leak to opponents: Short version: It is filed rather than fixed here for the reason given in the issue: the one-line clear is a stopgap, and the structural repair is to project a minimal object so a new field defaults to hidden instead of leaked. That wants its own test matrix and a client-consumer audit, and bundling it into this review would make this diff unreviewable against its own gate. #7101 still fixes the regression it introduced ( Cross-referenced to #6732, which is the same redaction seam failing in the opposite direction — face-down cards unreadable to the player who should be able to identify them, versus hidden cards readable by players who should not. I could not apply labels (no permission on this repo); it will need triage. |
… is inert The classifier admitted `Library -> Battlefield` arrivals as confined whenever the permanent entered tapped, on the argument that the card is an unchosen member of a hidden zone and therefore unreadable. That argument is measurably false. CR 701.23a: "To search for a card in a zone, look at all cards in that zone (even if it's a hidden zone)". Probed on the enrolled 4p dump, 91 of 91 library objects resolve in `state.objects` with full parsed rules content — and the hazard is already on that board: the seat's own library holds Bojuka Bog, whose ETB exiles TARGET PLAYER's graveyard. An unrestricted "search for a land card" could select it, so the seat was accepting away the only window that reaches a live cross-player choice. `library_arrivals_are_inert` reads the actor's pooled hidden zones and asks whether every card matching the union of the object's search filters is inert, reusing `carries_unreadable_rules_content` as the single inertness authority rather than growing a second one. It is applied at both entry points beside that gate, not threaded into the AST classifier: two players holding the same printed card get opposite verdicts, so this is a board fact and presenting it as an AST fact would mislabel it. A non-empty pool with an empty match set fails closed — 24 cards carry a search filter that reads the ability's own targets, which this context cannot evaluate, and a false `Accept` is the one outcome not worth risking. No corpus assumption is made. "Basic lands are vanilla" would have been the cheap boundary and it is not proven — a future set can print a Basic land with a trigger. The flagship row survives on measurement instead: Terramorphic searches Basic-only and all 22 basics in that deck are vanilla. Elvish Reclaimer, the card the report named, cannot carry the regression: its `Sacrifice(Typed[Land])` cost is unqualified, so `cost_window_reach` already answers `MayInterfere` and the row reads `Shorten` with and without the new gate. Measured, then replaced with its structural siblings Reshape the Earth and Urza's Cave, which exercise the spell and activated paths respectively. `parse_warnings` now rides `BackFaceData` and all four face-copy paths, so a transform swaps diagnostics with the face they describe instead of leaving a front-clean/back-warning card reading clean. The base-face snapshot line is load-bearing: `apply_face_down_creature_characteristics` blanks name, types, abilities and definitions but not diagnostics, so without it turning face up would have cleared what turning face down preserved — a regression this change would otherwise have introduced. Adding a `BackFaceData` field is compile-forced across 34 struct literals; each is the same single mechanical initializer. Seven revert-probes executed, each observed pass->fail. Engine suite 18,827 lib + 4,790 integration passed, 0 failed; workspace check and clippy clean. One self-caught miscite removed (CR 701.23c is undefined qualities in hidden zones, not a search limit). Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Both findings fixed — head
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/game/transform.rs (1)
330-330: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSeed a diagnostic and assert its preservation.
parse_warnings: vec![]cannot detect diagnostic loss during front-to-back or back-to-front transforms. Initialize this fixture with a non-empty warning and assert thattransform_permanentpreserves it in both directions.apply_card_face_to_back_facealready defines this propagation contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/transform.rs` at line 330, Update the fixture containing parse_warnings to initialize it with a non-empty diagnostic, then add assertions around transform_permanent for both front-to-back and back-to-front transformations to verify the warning is preserved. Use apply_card_face_to_back_face as the propagation contract and keep the existing transformation behavior unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/game_object.rs`:
- Around line 242-260: Change GameObject::parse_warnings and
BackFaceData::parse_warnings from plain vectors to an explicit
unknown-versus-known state so missing persisted diagnostics remain Unknown
rather than becoming clean. Update serde handling, face-application copies, and
carries_unreadable_rules_content/shortcut_efficacy consumers to treat Unknown as
fail-closed while preserving Known diagnostics behavior.
---
Nitpick comments:
In `@crates/engine/src/game/transform.rs`:
- Line 330: Update the fixture containing parse_warnings to initialize it with a
non-empty diagnostic, then add assertions around transform_permanent for both
front-to-back and back-to-front transformations to verify the warning is
preserved. Use apply_card_face_to_back_face as the propagation contract and keep
the existing transformation behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b47ef9a-8fc3-4b0d-a09c-52e2b880f141
📒 Files selected for processing (43)
crates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/shortcut_efficacy.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/day_night.rscrates/engine/src/game/effects/become_copy.rscrates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/flip_coin.rscrates/engine/src/game/effects/flip_permanent.rscrates/engine/src/game/effects/prepare.rscrates/engine/src/game/effects/set_room_door_lock.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/effects/transform_effect.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/engine_mdfc_land_tests.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/flip.rscrates/engine/src/game/game_object.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/specialize.rscrates/engine/src/game/stack.rscrates/engine/src/game/transform.rscrates/engine/src/game/triggers.rscrates/engine/src/game/zones.rscrates/engine/tests/integration/azors_gateway_transform_condition.rscrates/engine/tests/integration/copied_ability_transform_generation.rscrates/engine/tests/integration/craft_tithing_blade_transform.rscrates/engine/tests/integration/deterministic_game_state_serde.rscrates/engine/tests/integration/integration_adventure.rscrates/engine/tests/integration/issue_2425_fable_chapter_iii_transform.rscrates/engine/tests/integration/issue_4001_frolicking_familiar_adventure_instant.rscrates/engine/tests/integration/issue_5326_avatar_aang_transform.rscrates/engine/tests/integration/issue_6403_moonmist_mass_transform.rscrates/engine/tests/integration/issue_691_sheoldred_saga_lore.rscrates/engine/tests/integration/kamigawa_flip_cards.rscrates/engine/tests/integration/room_door_lock_unlock.rscrates/engine/tests/integration/rules/battle.rscrates/engine/tests/integration/shorten_efficacy.rscrates/engine/tests/integration/specialize_runtime.rscrates/engine/tests/integration/std_s07_batch5b.rscrates/engine/tests/integration/stolen_goodies_zero_targets.rscrates/engine/tests/integration/tamiyo_inquisitive_student_flip.rscrates/engine/tests/integration/wedding_announcement_transform.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/game/specialize.rs
- crates/engine/src/game/printed_cards.rs
| /// Parser diagnostics for THIS face — the `BackFaceData` half of | ||
| /// [`GameObject::parse_warnings`], and per-face for the same reason `abilities` | ||
| /// is: the two faces are parsed independently, so a card whose front reads | ||
| /// cleanly and whose back does not is the normal case rather than a corner one. | ||
| /// | ||
| /// Without this field the diagnostic was not a per-face fact at all. Face | ||
| /// application copies field by field, so a transform kept the FRONT face's | ||
| /// diagnostics on the object while displaying the back face's rules text: a | ||
| /// front-clean card looked clean after transforming into a back face the parser | ||
| /// could not fully read, and a front-dirty one kept a diagnostic that no longer | ||
| /// described anything. `ai_support::shortcut_efficacy` reads the object field as | ||
| /// evidence that the printed rules text is fully modelled, so the stale answer | ||
| /// was load-bearing in both directions. | ||
| /// | ||
| /// `serde(default)` keeps every persisted dump loadable — an older | ||
| /// `BackFaceData` simply carries no diagnostics — and `skip_serializing_if` | ||
| /// keeps a clean parse byte-identical on the way out. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub parse_warnings: Vec<crate::parser::oracle_ir::diagnostic::OracleDiagnostic>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'parse_warnings|CopiableValues|CopyValues|is_copy|object_window_reach|deserialize|from_slice' \
crates/engine/src crates/engine/testsRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- game_object outline ---\n'
ast-grep outline crates/engine/src/game/game_object.rs --view compact || true
printf '\n--- game_object parse_warnings/copy fields ---\n'
rg -n -C 8 'parse_warnings|BackFaceData|CopiableValues|CopyValues|is_copy|object_window_reach|impl .+Copiable|struct BackFaceData|struct GameObject' crates/engine/src/game/game_object.rs
printf '\n--- shortcut_efficacy relevant block ---\n'
sed -n '3480,3565p' crates/engine/src/ai_support/shortcut_efficacy.rs
printf '\n--- object_window_reach definitions ---\n'
rg -n -C 10 'enum ObjectWindowReach|fn object_window_reach|object_window_reach|object_window_reach' crates/engine/src crates/engine/testsRepository: phase-rs/phase
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- game_object outline ---'
ast-grep outline crates/engine/src/game/game_object.rs --view compact || true
printf '%s\n' ''
printf '%s\n' '--- game_object parse_warnings/copy fields ---'
rg -n -C 8 'parse_warnings|BackFaceData|CopiableValues|CopyValues|is_copy|object_window_reach|impl .+Copiable|struct BackFaceData|struct GameObject' crates/engine/src/game/game_object.rs
printf '%s\n' ''
printf '%s\n' '--- shortcut_efficacy relevant block ---'
sed -n '3480,3565p' crates/engine/src/ai_support/shortcut_efficacy.rs
printf '%s\n' ''
printf '%s\n' '--- object_window_reach definitions ---'
rg -n -C 12 'enum ObjectWindowReach|fn object_window_reach|object_window_reach|object_window_reach|pub enum ObjectWindowReach' crates/engine/src crates/engine/testsRepository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- carries_unreadable_rules_content implementation/usages ---'
rg -n -C 10 'fn carries_unreadable_rules_content|carries_unreadable_rules_content|parse_warnings\.is_empty|!object\.parse_warnings\.is_empty' crates/engine/src/ai_support/shortcut_efficacy.rs
printf '%s\n' ''
printf '%s\n' '--- CopyValues / copiable / copy object sources ---'
rg -n -C 12 'struct CopyValues|enum CopyValues|CopyValues|CopiableValues|is_copy|copy_of|clone.*object|new_object|parse_warnings' crates/engine/src crates/engine/tests -g '*.rs' --glob '*copy*' --glob 'game_object.rs' --glob 'game.rs' | head -n 360Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact copy construction sites and parse_warnings/default object creation ---'
rg -n -C 9 '\.parse_warnings|parse_warnings:\s*(vec!\[\]|Vec::new\(\)|self\.[A-Za-z_0-9]+\.parse_warnings|#[A-Za-z_0-9]+\.parse_warnings|\{\})|new_object\(|create_object\(' crates/engine/src -g '*.rs' --glob '*copy*' --glob 'game_object.rs' --glob 'game.rs' --glob 'effects.rs' --glob '**copy*.rs'
printf '%s\n' ''
printf '%s\n' '--- CopyValues implementation ---'
file="$(rg -l 'struct CopyValues|impl .+CopyValues|enum CopyValues' crates/engine/src -g '*.rs' | head -n 5)"
for f in $file; do
printf '%s\n' "FILE $f"
rg -n -C 12 'struct CopyValues|impl .+CopyValues|enum CopyValues|copy_warnings|parse_warnings' "$f"
done
printf '%s\n' ''
printf '%s\n' '--- parser diagnostics copy into object ---'
rg -n -C 8 'parse_warnings' crates/engine/src/game crates/engine/src/types --glob '*.rs'Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CopyValues implementation/definition ---'
rg -l 'struct CopyValues|impl .+CopyValues|enum CopyValues' crates/engine/src -g '*.rs'
while IFS= read -r f; do
rg -n -C 20 'struct CopyValues|impl .+CopyValues|enum CopyValues|parse_warnings|copy_warnings|warnings' "$f"
done < <(rg -l 'struct CopyValues|impl .+CopyValues|enum CopyValues' crates/engine/src -g '*.rs')
printf '%s\n' ''
printf '%s\n' '--- object copy construction and parse_warnings only ---'
rg -n -C 12 'parse_warnings' crates/engine/src types crates/engine/tests --glob '*copy*.rs' --glob 'game_object.rs' --glob 'game.rs' --glob 'effects.rs' --glob '*clone*.rs' --glob '*copy*.rs'Repository: phase-rs/phase
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '409,610p' crates/engine/src/game/game_object.rs
sed -n '960,1010p' crates/engine/src/game/game_object.rs
sed -n '1320,1410p' crates/engine/src/game/game_object.rs
sed -n '3503,3542p' crates/engine/src/ai_support/shortcut_efficacy.rs
printf '%s\n' '--- all visible CopyValues mentions tracked by git ---'
git ls-files 'crates/engine/src/**/*.rs' 'crates/engine/tests/**/*.rs' | xargs rg -n 'CopyValues|copiable|parse_warnings|BackFaceData::parse_warnings|apply_back_face_to_object|transform_permanent|create_object|new_object' | head -n 300
printf '%s\n' ''
printf '%s\n' '--- focused search for object construction fields around base/new object ---'
rg -n -C 10 'parse_warnings:\s*vec!\[\]|parse_warnings:\s*Vec::new\(\)|parse_warnings:' crates/engine/src/game crates/engine/src/types -g '*.rs'Repository: phase-rs/phase
Length of output: 50371
Preserve unknown parse status instead of collapsing it to clean.
carries_unreadable_rules_content evaluates non-empty parse_warnings as MayInterfere, but serde makes missing fields and clean parses both deserialize as Vec::new(). Use an explicit Unknown versus Known(Vec<OracleDiagnostic>) state for GameObject::parse_warnings and BackFaceData::parse_warnings, or recompute diagnostics before shortcut_efficacy reads them; copies should remain fail-closed when provenance is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/game/game_object.rs` around lines 242 - 260, Change
GameObject::parse_warnings and BackFaceData::parse_warnings from plain vectors
to an explicit unknown-versus-known state so missing persisted diagnostics
remain Unknown rather than becoming clean. Update serde handling,
face-application copies, and carries_unreadable_rules_content/shortcut_efficacy
consumers to treat Unknown as fail-closed while preserving Known diagnostics
behavior.
Source: Path instructions
|
🤖 AI text below 🤖 Evidence bound to
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head 0db0d6b.
The library-arrival gate now evaluates the actor-owned selectable pool using the resolver’s owner-zone filter authority, so the prior Bojuka Bog false-Accept is closed; the exact-head parse-diff reports no card parse changes. The transform fixture now seeds distinct front/back diagnostics and asserts both swaps, resolving the current CodeRabbit test-strengthening finding.
I also checked the open unknown-diagnostics suggestion: restored server sessions rehydrate printed faces from the card database before AI decisions, so an omitted compatibility field is not the live shortcut-efficacy provenance path. It does not block this focused change.
2 participants
🤖 AI text below 🤖
Summary
smart_shortcut_responsedocumented "a way to break the loop" but calledhas_meaningful_priority_action— the auto-pass gate, a strictly weaker predicate. On a real 4-player board a seat holding only a basic-land fetch answeredShortenagainst a mandatory trigger cascade it could not touch: rules-legal under CR 732.2b, strategically vacuous, and paid for with a real priority window. This adds a stage-2 efficacy classifier (WindowReach) on a fourth orthogonal axis — controller-relative confinement — so a seat whose only actions write inside its own resources accepts, while a seat that can reach past them still shortens.Files changed
crates/engine/src/ai_support/shortcut_efficacy.rs— new: theWindowReachclassifiercrates/engine/src/ai_support/mod.rs— stage-2 wiring,stage_two_action_set, sharedshortcut_probecrates/engine/src/game/engine_resolution_choices.rs— citation-gate enrolment floor 6 → 8 (test-side)crates/engine/tests/integration/shorten_efficacy.rs— new: acceptance rowscrates/engine/tests/integration/main.rs—mod shorten_efficacy;crates/engine/tests/fixtures/dina_noff_turn5_4p.json.gz— new: the real 4p dump the acceptance rows driveTrack
Developer
LLM
Model: claude-opus-4.8
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
13 distinct CR numbers added, each verified against
docs/MagicCompRules.txtbefore writing (not from memory):CR 104.2a, CR 104.4b, CR 106.4, CR 108.3, CR 302.6, CR 400.1, CR 400.3, CR 701.21a, CR 732.1b, CR 732.2a, CR 732.2b, CR 732.2c, CR 732.5
No CR licence is claimed for stage 2 itself: CR 732.2b is unconditioned and CR 732.2c requires only a different choice. This is AI policy, and the module doc says so.
Verification
All 9 completion checks ran at the committed head in a clean detached worktree, each with
head_before == head_after == e72a0c8fand porcelain 0, recorded in the run receipt (sha25625ca6711dcc09a4dfa849f25cb4ac7275be8a053133642061b33931a74b06ee5, 61/61 artifact digests re-verified from disk):cargo fmt --all -- --check— exit 0cargo clippy -p phase-engine --all-targets -- -D warnings— exit 0, 0 warningscargo test(workspace) — 23219 passed / 0 failed (18578 + 12 + 9 + 4620 + 0)cargo ai-gate— exit 0;0 FAIL, 2 WARN, 1 PASS, 0 NEW, 0 REMOVED; no baseline refreshedRead the ai-gate result honestly: it is green but non-probative for this seam. The suite provably never executes the changed code —
duel_suitebuilds every state throughGameState::new_two_player→GameState::new, which initialisesloop_detection: LoopDetectionMode::Off, andphase-ainever reaches the one projecting authority (GameState::set_match_config):MatchConfig0 hits inphase-aivs 88 incrates/. All fiveWaitingFor::LoopShortcutassignments inphase-aiare#[cfg(test)], so no shortcut prompt can be synthesised around the flag either. Corroborated behaviourally: a build with stage 2 deleted produces 30/30 games identical in winner and turn count. Both WARNs are baseline-side confounds — the stored baseline recordsgit_sha: 2d686880b94b, which does not resolve (git cat-file -t→fatal: Not a valid object name), and different card data (56b03366…) than this run (863f5422…); the same shifts reproduce in the stage-2-deleted build, so they cannot be attributed to this change. Filed separately as a lane-external gap:duel_suite::comparereads neither provenance field, while its siblingduel_suite/perf.rsalready implements exactly that guard.Parser evidence:
parser_evidence=PROJECTED_PARSE_DIFF(forced bySOURCE_HASH_DIFFERENCE, base9e829d1edfe972b8→ candidate4043f185c0f90c61). Projectedcard-data.jsonis byte-identical on both sides (953a51b2…66784); the projection is not a stale-binary artifact — the twooracle-genbinaries differ (2b920770…vs1586b035…) and so doescoverage-data.json(828cefde…vs2418b06e…).Acceptance rides a real 4-player dump (844846 B, sha256
9843d5165cbbf7dd7bca4171c7888c190b7eba7e52a2ed095b44ff76fadd7886) driven through the production restore andapply(), not a synthetic scenario.Gate A
Gate A PASS head=e72a0c8f6a04e52dce55686cc267760abe312cdc base=674b3a999573e88df8ec120805ff2a7730b6f64e
Anchored on
crates/engine/src/ai_support/mod.rs:1136—flat_actions_have_meaningful_priority, the stage-1 predicate whose action set stage 2 must cover;stage_two_action_setis built from the same predicate so the two cannot drift apartcrates/engine/src/game/ability_scan.rs:4268—ability_definition_axes, the existing classifier-at-a-seam precedent this follows; the new wildcard arm is deliberatelyMayInterfere, the opposite of that default, because a wrong Accept can lose a game while a wrong Shorten costs a beatFinal review-impl
Final review-impl PASS head=e72a0c8f6a04e52dce55686cc267760abe312cdc
Five review rounds (r1–r5), reports persisted on disk. r5: 0 MAJOR. Its 4 MINOR + 3 NIT were all count/label imprecision in non-repo supporting artifacts, not in the committed head, and were corrected after the round without a sixth review — each fix re-measured with a positive control. Disclosed rather than omitted.
Claimed parse impact
None.
Projected parse-diff over the pinned
AtomicCards.json: 0 clusters, 0added_cards, 0removed_cards.Scope Expansion
None.
Frozen scope is 6 paths, byte-matching the run's
scope-paths.nul(sha2564b4b68b17216252c0c633d0dad912ef1e62efcd6dbd8f00fb3d9953ed8388634) and equal togit diff --name-only -z BASE CANDIDATE.Validation Failures
None.
Two disclosures that are not failures but which a reviewer should not have to discover:
game::engine::no_living_player_has_meaningful_priority_actionstill inlines the same seven-statement probe recipe rather than routing through the new sharedai_support::shortcut_probe. Routing it would touch the CR 732.5 mandatory-loop firewall — both its production callers are mandatory-loop gates — which is outside this change's scope. Left as a follow-up rather than claimed as done.any_action_may_interferedocuments one accepted miss with its cost: a fetched permanent that itself enables interference is not modelled, so a confined fetch classifiesOwnResourcesOnlyeven if the fetched land could later enable a real answer. The across-window cost is bounded (one shortcut); the within-window cost is not bounded on anUntilLethaloffer. Measured witness for the related parser residual:Invoke Justice, whose "distribute four +1/+1 counters … target player controls" clause is dropped with noUnimplementedmarker.CI Failures
None.
This branch is based at
674b3a999, 12 commits behindupstream/main. Rebase deferred deliberately: the only two paths overlapping upstream's drift areengine_resolution_choices.rsandtests/integration/main.rs, and both are drift-free on measurement — upstream still carries 6 citation-gate-enrolled files with none removed, so this commit's 2 additions keepenrolled >= ENROLLED_FLOOR(8) satisfied after any rebase, andgit merge-treereports no conflicts.Summary by CodeRabbit
New Features
Bug Fixes
Tests