Fix producer-wide granted-trigger MaxTimesPerTurn accounting - #7228
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 change retires active trigger grants during battlefield entry and zone transitions, preserves legacy trigger fire counts, updates replay validation, and adds regression coverage for grant generations and ChangesTrigger-keyed ledger and migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/engine/src/types/ability.rs (2)
91-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on the returned
TriggerEntry.grant_producer, not on the pre-call producer variables.In
identical_grants_from_distinct_producers_remain_distinct_entries, the test callsreconcile_trigger_entriesand receivesentries. The following assertions buildTriggerFireLedgerKey::Grant(first_producer)andTriggerFireLedgerKey::Grant(second_producer)from the local variables captured before the call, not fromentries[0].grant_producerorentries[1].grant_producer.This test does not check that
reconcile_trigger_entriesactually stamped the correct producer key onto each returned entry. A future change that drops thegrant_producer.clone()stamp, or swaps which producer is attached to which entry, would not fail this test.Add:
assert_eq!(entries[0].grant_producer, Some(first_producer.clone())); assert_eq!(entries[1].grant_producer, Some(second_producer.clone()));This closes the gap and ties the assertions directly to the behavior this PR adds.
🤖 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/types/ability.rs` around lines 91 - 142, Update the test identical_grants_from_distinct_producers_remain_distinct_entries to assert that entries[0].grant_producer equals Some(first_producer.clone()) and entries[1].grant_producer equals Some(second_producer.clone()) immediately after reconciliation, while preserving the existing distinctness assertions.
21962-21983: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize grant-entry construction and derive occurrence from producer identity.
reconcile_trigger_entriesandreconcile_trigger_entries_matchingduplicate the active-grant lookup and entry construction. Extract a shared private helper. Changewith_grant_producerto acceptTriggerGrantProducerKeyandTriggerGrantInstanceRef, then derive the occurrence withoccurrence_for_grant. This prevents mismatched occurrence and producer values. Update the callers incrates/engine/src/game/layers.rs,crates/engine/src/game/ledger.rs, and the test fixture.🤖 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/types/ability.rs` around lines 21962 - 21983, Centralize grant-entry creation by updating TriggerEntry::with_grant_producer to accept TriggerGrantProducerKey and TriggerGrantInstanceRef, derive the occurrence via occurrence_for_grant, and remove the caller-provided occurrence. Extract the shared active-grant lookup and construction logic from reconcile_trigger_entries and reconcile_trigger_entries_matching into one private helper, then update callers in layers.rs, ledger.rs, and the test fixture to use the new API.crates/engine/src/types/game_state.rs (1)
228-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the adapter doc comment onto the production module it now describes.
The comment at lines 228-230 explains why the trigger-occurrence ledger is encoded as an explicit entry list. After the split, it sits above
legacy_trigger_definition_ref_map, which is now#[cfg(test)]-only. The productiontrigger_definition_ref_mapmodule (lines 258-294) — the one actually used bytrigger_fire_counts_this_turn— has no doc comment explaining its dualCurrent/Legacywire acceptance.Move or duplicate the doc comment onto
trigger_definition_ref_mapso a future maintainer reading the active serializer understands the untagged-enum compatibility trick without having to find the test-only sibling module first.🤖 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/types/game_state.rs` around lines 228 - 295, Move the existing serde adapter documentation from the test-only legacy_trigger_definition_ref_map module to the production trigger_definition_ref_map module, or duplicate it there. Ensure the comment explains the explicit entry-list encoding and the Current/Legacy compatibility deserialization used by the active adapter.
🤖 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/types/game_state.rs`:
- Around line 316-363: Update migrate_legacy_trigger_fire_counts to aggregate
all legacy Definition counts by producer before mutating
trigger_fire_counts_this_turn. After aggregation, check each producer’s Grant
key for a genuine pre-existing ledger entry, reject only those collisions, then
insert the summed count once per producer while removing the migrated definition
entries.
- Around line 28497-28545: Extend
game_state_deserialize_migrates_legacy_grant_fire_count_to_producer_key with a
second recipient object and trigger definition using the same grant producer but
a distinct TriggerDefinitionRef. Insert a separate legacy Definition-keyed fire
count for that recipient, then assert migration produces one Grant entry for the
shared producer whose count equals both legacy counts combined.
---
Nitpick comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 91-142: Update the test
identical_grants_from_distinct_producers_remain_distinct_entries to assert that
entries[0].grant_producer equals Some(first_producer.clone()) and
entries[1].grant_producer equals Some(second_producer.clone()) immediately after
reconciliation, while preserving the existing distinctness assertions.
- Around line 21962-21983: Centralize grant-entry creation by updating
TriggerEntry::with_grant_producer to accept TriggerGrantProducerKey and
TriggerGrantInstanceRef, derive the occurrence via occurrence_for_grant, and
remove the caller-provided occurrence. Extract the shared active-grant lookup
and construction logic from reconcile_trigger_entries and
reconcile_trigger_entries_matching into one private helper, then update callers
in layers.rs, ledger.rs, and the test fixture to use the new API.
In `@crates/engine/src/types/game_state.rs`:
- Around line 228-295: Move the existing serde adapter documentation from the
test-only legacy_trigger_definition_ref_map module to the production
trigger_definition_ref_map module, or duplicate it there. Ensure the comment
explains the explicit entry-list encoding and the Current/Legacy compatibility
deserialization used by the active adapter.
🪄 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: c50c2b1a-1c94-4a14-8fe1-1101e07d3e34
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/engine/src/analysis/resource.rscrates/engine/src/game/game_object.rscrates/engine/src/game/layers.rscrates/engine/src/game/ledger.rscrates/engine/src/game/triggers.rscrates/engine/src/game/zones.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/nadu_lavaspur_boots_max_times.rscrates/feed-scraper/Cargo.tomlcrates/phase-ai/src/policies/tests/draw_payoff.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked: the producer-wide runtime ledger is correct in the Nadu path, but legacy deserialization rejects the same multi-recipient shape instead of preserving it.
🔴 Blocker
crates/engine/src/types/game_state.rs:339-359 promotes legacy Definition counts one at a time while mutating trigger_fire_counts_this_turn. Two recipient-local definitions granted by the same producer therefore both resolve to Grant(producer): the first inserts it and the second treats that insertion as a collision, returning an error. This is the legacy form of the multiple-recipient Nadu case this PR fixes, so restoring such a saved state fails rather than producing one producer-scoped count.
Aggregate the legacy definition counts by TriggerGrantProducerKey before mutating the map; reject only a pre-existing Grant(producer) entry, then remove the legacy keys and insert each summed producer value. Extend the state-deserialization regression with two distinct recipient definitions sharing one producer and assert the single resulting grant key has the combined count.
✅ Clean
The live targeting/trigger collection regression is a real production-pipeline check: reverting the producer ledger selection would create three recipient keys and fail its two-fire/one-key assertions. The current parse-diff is bound to this head and reports no card-parse changes.
Recommendation: request changes for the migration aggregation and its two-recipient deserialization regression; then re-run the current-head verification.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head: b9de2a35c10ee13986a822646a6400ee46a3dcd6.
[HIGH] MaxTimesPerTurn is keyed by the grantor, not by the independently functioning ability on each recipient. Evidence: crates/engine/src/types/ability.rs:21892-21902 defines TriggerFireLedgerKey::Grant(TriggerGrantProducerKey); crates/engine/src/game/triggers.rs:11270-11285 selects that key for every entry carrying grant_producer; the constraint gate and recording path both consume it at crates/engine/src/game/triggers.rs:9590-9597 and 11358-11371. The new regression codifies the same global limit by asserting a total of 2 and one ledger entry at crates/engine/tests/integration/nadu_lavaspur_boots_max_times.rs:109-122. Why it matters: Nadu grants a separate ability to each creature, and the official Modern Horizons 3 release notes explicitly say those abilities function independently and may trigger twice each turn for each creature, not twice total. The current code therefore suppresses four valid triggers for three targets. Suggested fix: make the MaxTimesPerTurn ledger (including replay/persistence migration) use the recipient-specific TriggerDefinitionRef identity, and add a production-pipeline regression with three creatures targeted twice each that proves all six triggers fire, with two uses retained per recipient.
[MED] The reqwest TLS feature migration is unrelated scope in this trigger-ledger bug fix. Evidence: crates/feed-scraper/Cargo.toml:13 switches from reqwest defaults to explicit Rustls/WebPKI roots and the PR also changes Cargo.lock. Why it matters: it changes the feed scraper's transport/TLS dependency surface, which needs its own platform and runtime justification instead of being coupled to a rules-engine correction. Suggested fix: remove it from this PR or split it into a focused dependency PR with scoped validation.
The green CI checks and current coverage-parse-diff no-change artifact are useful mechanical evidence, but they do not override this rules-correctness failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/triggers.rs`:
- Around line 11272-11274: The trigger_fire_ledger_key function must use grant
provenance from source_context or reconciliation state: derive the ledger key
from the granting producer for granted triggers, and retain the
Definition(definition_ref) key only for ungranted triggers. Add regression
coverage confirming equivalent recipient grants share the producer budget, with
a self-contained CR 603.4 annotation.
In `@crates/engine/tests/integration/nadu_lavaspur_boots_max_times.rs`:
- Around line 112-120: Update the activation loop in the integration test to
attempt each of first_target, second_target, and third_target three times,
exercising the rejected third activation while preserving the expected fire
count of six. Increase the test’s mana pool and library fixture capacity as
needed so all additional attempts reach the MaxTimesPerTurn rejection through
the production pipeline.
🪄 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: 8c00f5ee-e402-4101-9956-1bcc51748f11
📒 Files selected for processing (5)
crates/engine/src/game/ledger.rscrates/engine/src/game/triggers.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/nadu_lavaspur_boots_max_times.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/types/game_state.rs
- crates/engine/src/types/ability.rs
| _source_context: Option<&TriggerSourceContext>, | ||
| ) -> crate::types::ability::TriggerFireLedgerKey { | ||
| crate::types::ability::TriggerFireLedgerKey::Definition(definition_ref.clone()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target function and nearby callers ---'
sed -n '11220,11320p' crates/engine/src/game/triggers.rs
printf '%s\n' '--- all references to trigger_fire_ledger_key ---'
rg -n -C 4 'trigger_fire_ledger_key' crates/engine/src crates/engine/tests
printf '%s\n' '--- producer/source-context and ledger-key definitions ---'
rg -n -C 5 'TriggerSourceContext|TriggerFireLedgerKey|producer|grant' crates/engine/src/game/triggers.rs crates/engine/src/game/ledger.rs crates/engine/src/types/ability.rs
printf '%s\n' '--- relevant ledger implementation ---'
sed -n '450,560p' crates/engine/src/game/ledger.rs
printf '%s\n' '--- project guidance and CR annotations near trigger logic ---'
rg -n -C 3 'CR [0-9]|trigger ledger|MaxTimesPerTurn|producer' CLAUDE.md crates/engine/src/game/triggers.rsRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
log=/tmp/coderabbit-shell-logs/shell-output-Fvkaf0
printf '%s\n' '--- target section from the captured output ---'
line=$(rg -n -- '-- target function and nearby callers ---' "$log" | head -n1 | cut -d: -f1)
if [ -n "$line" ]; then
end=$((line+140))
sed -n "${line},${end}p" "$log"
fi
printf '%s\n' '--- exact definition and call sites in source ---'
rg -n -C 8 'fn trigger_fire_ledger_key|trigger_fire_ledger_key\(' crates/engine/src/game/triggers.rs
printf '%s\n' '--- exact source-context and ledger-key declarations ---'
rg -n -C 8 'struct TriggerSourceContext|enum TriggerFireLedgerKey|TriggerSourceContext|TriggerFireLedgerKey' crates/engine/src crates/engine/tests -g '*.rs' | head -n 500
printf '%s\n' '--- ledger implementation ---'
rg -n -C 12 'record_trigger_fired|trigger_fire_counts_this_turn|ResolvedTriggerLedgerEdit::MaxTimesPerTurn' crates/engine/src/game/ledger.rs crates/engine/src/game/triggers.rs | head -n 500Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- ledger-key replay resolver ---'
sed -n '430,535p' crates/engine/src/game/ledger.rs
printf '%s\n' '--- TriggerDefinitionRef, TriggerEntry, and grant/provenance fields ---'
rg -n -C 10 'struct TriggerDefinitionRef|struct TriggerEntry|enum TriggerEntry|grant|producer|Granted|granting|provenance' \
crates/engine/src/types/ability.rs crates/engine/src/types/game_state.rs crates/engine/src/game/triggers.rs \
-g '*.rs' | head -n 700
printf '%s\n' '--- source-context fields ---'
sed -n '485,610p' crates/engine/src/types/game_state.rs
printf '%s\n' '--- current diff summary and focused trigger diff markers ---'
git diff --stat
git diff --unified=8 -- crates/engine/src/game/triggers.rs | rg -n -C 12 'ledger|producer|grant|MaxTimesPerTurn|trigger_fire_ledger_key|^@@'Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- provenance type definitions ---'
rg -n 'pub (struct|enum) (TriggerDefinitionRef|TriggerEntry|TriggerGrantProducerKey|TriggerProducerOrigin|TriggerOccurrenceState)|struct TriggerDefinitionRef|enum TriggerGrantProducerKey|enum TriggerProducerOrigin' crates/engine/src/types/ability.rs
printf '%s\n' '--- provenance references in trigger collection and reconciliation ---'
rg -n -C 12 'grant_producer|TriggerGrantProducerKey|TriggerProducerOrigin|reconcile_trigger_entries|active_grants|definition_ref\(' crates/engine/src crates/engine/tests -g '*.rs' | head -n 1000
printf '%s\n' '--- complete source-context field range ---'
sed -n '491,600p' crates/engine/src/types/game_state.rs
printf '%s\n' '--- changed files and diff hunks ---'
git diff --stat
git diff --unified=3 -- crates/engine/src/game/triggers.rs | sed -n '1,260p'Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- all grant-producer uses in trigger code ---'
rg -n -C 10 'grant_producer|TriggerGrantProducerKey|TriggerDefinitionOccurrenceRef|occurrence' crates/engine/src/game/triggers.rs
printf '%s\n' '--- trigger-entry materialization and source-context construction ---'
rg -n -C 12 'reconcile_trigger_entries|TriggerEntry::|definition_ref|trigger_definition_ref|set_trigger_source_recursive|TriggerSourceContext' \
crates/engine/src/game crates/engine/src/types -g '*.rs' | head -n 1200
printf '%s\n' '--- exact ledger-key declarations and occurrence types ---'
sed -n '21830,21965p' crates/engine/src/types/ability.rs
printf '%s\n' '--- trigger constraint validation and recording callers ---'
sed -n '9450,9620p' crates/engine/src/game/triggers.rs
sed -n '11270,11370p' crates/engine/src/game/triggers.rsRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- trigger definition identity uses only ---'
rg -n 'definition_ref|trigger_definition_ref|TriggerEntry|TriggerGrantProducerKey' crates/engine/src/game/triggers.rs
printf '%s\n' '--- reconciliation and trigger surface sections ---'
sed -n '1280,1460p' crates/engine/src/game/triggers.rs
sed -n '9250,9475p' crates/engine/src/game/triggers.rs
printf '%s\n' '--- matched-trigger and pending-context structures ---'
rg -n -C 20 'struct Matched|struct MatchedTrigger|definition_ref:|constraint:' crates/engine/src/game/triggers.rs
printf '%s\n' '--- trigger definition references at collection call sites ---'
rg -n -C 15 'collect_matching_triggers_from_context|check_trigger_constraint|check_trigger_condition' crates/engine/src/game/triggers.rs | head -n 900Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- MaxTimesPerTurn tests and grant scenarios ---'
rg -n -C 15 'MaxTimesPerTurn|shared producer|producer budget|grant.*ledger|ledger.*grant|equivalent recipient|same producer' \
crates/engine/src crates/engine/tests -g '*.rs' | head -n 1400
printf '%s\n' '--- ledger-key documentation and all constructors ---'
rg -n -C 8 'TriggerFireLedgerKey::|TriggerFireLedgerKey' crates/engine/src crates/engine/tests -g '*.rs' | head -n 1000
printf '%s\n' '--- occurrence definitions and grant identity mapping ---'
sed -n '21880,21950p' crates/engine/src/types/ability.rs
sed -n '22220,22265p' crates/engine/src/types/ability.rs
printf '%s\n' '--- grant-related trigger tests ---'
sed -n '12840,13040p' crates/engine/src/game/triggers.rs
sed -n '36220,36520p' crates/engine/src/game/triggers.rsRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TriggerSourceContext complete fields ---'
sed -n '491,600p' crates/engine/src/types/game_state.rs
printf '%s\n' '--- trigger definition identity helpers on GameObject ---'
rg -n -C 18 'fn trigger_definition_ref|trigger_source_context\(|sync_trigger_source_context' crates/engine/src/game/game_object.rs crates/engine/src/types/game_state.rs
printf '%s\n' '--- legacy provenance migration ---'
sed -n '1625,1715p' crates/engine/src/game/game_object.rs
printf '%s\n' '--- trigger installation path ---'
sed -n '6985,7055p' crates/engine/src/game/layers.rs
printf '%s\n' '--- exact MaxTimes recording tail ---'
sed -n '11270,11370p' crates/engine/src/game/triggers.rsRepository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete legacy provenance migration ---'
sed -n '1650,1725p' crates/engine/src/game/game_object.rs
printf '%s\n' '--- complete Layer-6 trigger installation ---'
sed -n '6989,7045p' crates/engine/src/game/layers.rs
printf '%s\n' '--- complete MaxTimes recording path ---'
sed -n '11340,11368p' crates/engine/src/game/triggers.rs
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
ability = Path("crates/engine/src/types/ability.rs").read_text()
state = Path("crates/engine/src/types/game_state.rs").read_text()
triggers = Path("crates/engine/src/game/triggers.rs").read_text()
game_object = Path("crates/engine/src/game/game_object.rs").read_text()
assert "pub enum TriggerFireLedgerKey" in ability
ledger_body = ability.split("pub enum TriggerFireLedgerKey", 1)[1].split("/// The immutable occurrence", 1)[0]
assert "Definition(TriggerDefinitionRef)" in ledger_body
assert "Producer" not in ledger_body
assert "pub grant_producer: Option<TriggerGrantProducerKey>" in ability
context_body = state.split("pub struct TriggerSourceContext", 1)[1].split("impl std::fmt::Debug", 1)[0]
assert "pub trigger_entries: Vec<TriggerEntry>" in context_body
helper = triggers.split("fn trigger_fire_ledger_key(", 1)[1].split("/// Record that a constrained trigger", 1)[0]
assert "_source_context: Option<&TriggerSourceContext>" in helper
assert "TriggerFireLedgerKey::Definition(definition_ref.clone())" in helper
migration = game_object.split("pub fn migrate_legacy_trigger_provenance", 1)[1].split("pub fn ", 1)[0]
assert "active_grants()" in migration
assert "fragment `MaxTimesPerTurn` accounting" in migration
print("helper_discards_source_context: true")
print("current_ledger_key_is_definition_only: true")
print("source_context_carries_trigger_entries: true")
print("trigger_entries_carry_grant_producer: true")
print("migration_identifies_fragmented_MaxTimes_accounting: true")
PYRepository: phase-rs/phase
Length of output: 7489
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- identify the failed verifier assertion ---'
python3 - <<'PY'
from pathlib import Path
ability = Path("crates/engine/src/types/ability.rs").read_text()
state = Path("crates/engine/src/types/game_state.rs").read_text()
triggers = Path("crates/engine/src/game/triggers.rs").read_text()
game_object = Path("crates/engine/src/game/game_object.rs").read_text()
checks = [
("ledger enum", "pub enum TriggerFireLedgerKey" in ability),
("definition variant", "Definition(TriggerDefinitionRef)" in ability.split("pub enum TriggerFireLedgerKey", 1)[1].split("/// The immutable occurrence", 1)[0]),
("no producer variant", "Producer" not in ability.split("pub enum TriggerFireLedgerKey", 1)[1].split("/// The immutable occurrence", 1)[0]),
("entry producer", "pub grant_producer: Option<TriggerGrantProducerKey>" in ability),
("context entries", "pub trigger_entries: Vec<TriggerEntry>" in state.split("pub struct TriggerSourceContext", 1)[1].split("impl std::fmt::Debug", 1)[0]),
("helper ignores context", "_source_context: Option<&TriggerSourceContext>" in triggers.split("fn trigger_fire_ledger_key(", 1)[1].split("/// Record that a constrained trigger", 1)[0]),
("helper definition key", "TriggerFireLedgerKey::Definition(definition_ref.clone())" in triggers.split("fn trigger_fire_ledger_key(", 1)[1].split("/// Record that a constrained trigger", 1)[0]),
("migration active grants", "active_grants()" in game_object.split("pub fn migrate_legacy_trigger_provenance", 1)[1].split("pub fn ", 1)[0]),
("migration warning", "fragment `MaxTimesPerTurn` accounting" in game_object.split("pub fn migrate_legacy_trigger_provenance", 1)[1].split("pub fn ", 1)[0]),
]
for name, result in checks:
print(f"{name}: {result}")
PY
printf '%s\n' '--- search for producer-wide accounting terminology ---'
rg -n -C 4 'producer-wide|producer.*budget|shared.*producer|fragment.*MaxTimes|MaxTimes.*fragment|fire ledger|ledger.*producer' \
. -g '*.rs' -g '*.md' -g '*.toml' | head -n 600Repository: phase-rs/phase
Length of output: 2540
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- every production use of grant_producer ---'
rg -n 'grant_producer' crates/engine/src crates/engine/tests -g '*.rs'
printf '%s\n' '--- every producer-derived occurrence and ledger-key relation ---'
rg -n -C 6 'occurrence_for_grant|TriggerFireLedgerKey::Definition|TriggerFireLedgerKey::' \
crates/engine/src/game crates/engine/src/types -g '*.rs' | head -n 1200Repository: phase-rs/phase
Length of output: 14803
Use grant provenance when deriving MaxTimesPerTurn ledger keys.
trigger_fire_ledger_key ignores source_context, although TriggerSourceContext::trigger_entries retains grant_producer. Granted triggers therefore use recipient-local Definition keys instead of sharing the granting producer’s budget. The replay fallback cannot fix this runtime path because it only uses a producer key already present in ledger_key.
Resolve the producer from the captured entry or reconciliation state. Use Definition only for ungranted triggers. Add a regression test for equivalent recipient grants and a self-contained CR 603.4 annotation.
🤖 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/triggers.rs` around lines 11272 - 11274, The
trigger_fire_ledger_key function must use grant provenance from source_context
or reconciliation state: derive the ledger key from the granting producer for
granted triggers, and retain the Definition(definition_ref) key only for
ungranted triggers. Add regression coverage confirming equivalent recipient
grants share the producer budget, with a self-contained CR 603.4 annotation.
Source: Path instructions
| for target in [ | ||
| first_target, | ||
| second_target, | ||
| third_target, | ||
| first_target, | ||
| second_target, | ||
| third_target, | ||
| ] { | ||
| runner.activate(boots, 0).target_object(target).resolve(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the rejected third activation.
The loop activates each target exactly twice. An implementation that ignores MaxTimesPerTurn also produces six fires and passes Lines 123-136. Add a third activation for each target. Keep the expected fire count at six. Increase the mana pool and library fixture for the additional activation attempts.
Suggested test adjustment
for target in [
first_target,
second_target,
third_target,
first_target,
second_target,
third_target,
+ first_target,
+ second_target,
+ third_target,
] {As per path instructions: integration tests must exercise the failure path that the fix prevents through the production pipeline.
🤖 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/tests/integration/nadu_lavaspur_boots_max_times.rs` around
lines 112 - 120, Update the activation loop in the integration test to attempt
each of first_target, second_target, and third_target three times, exercising
the rejected third activation while preserving the expected fire count of six.
Increase the test’s mana pool and library fixture capacity as needed so all
additional attempts reach the MaxTimesPerTurn rejection through the production
pipeline.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The MaxTimesPerTurn regression never exercises the rejecting third activation. Evidence: crates/engine/tests/integration/nadu_lavaspur_boots_max_times.rs:112-120 targets each recipient exactly twice, then :123-136 asserts the resulting six fires and two uses per key. Why it matters: removing or bypassing MaxTimesPerTurn still produces exactly those six fires and all of these assertions pass, so the test does not discriminate the cap this PR changes. Suggested fix: target each recipient a third time through the same production pipeline, provide enough mana and library cards for all nine activations to resolve, and continue asserting exactly six fires with two per recipient.
The current head is a686a87cbffe7e8f200b42face8ad47a739c96a4. CodeRabbit independently reported the same missing third-attempt coverage on this head. The parse-diff artifact remains bound to b9de2a35c10ee13986a822646a6400ee46a3dcd6, so it is stale; required CI is also still pending. This test-discrimination gap alone blocks approval.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked: the third-attempt regression is now discriminating, but the current head retains a producer-provenance redesign after the existing recipient-local trigger identity already supplies the required MaxTimesPerTurn key.
🔴 Blocker
crates/engine/src/game/game_object.rs:1577-1584 already constructs TriggerDefinitionRef from the recipient's ObjectIncarnationRef and that recipient's trigger occurrence. That is the correct CR 113.2c identity for independently functioning granted abilities. The new TriggerFireLedgerKey at crates/engine/src/types/ability.rs:21883-21891 has only Definition(TriggerDefinitionRef), while crates/engine/src/game/triggers.rs:11270-11275 ignores source_context and always returns that sole wrapper. The added TriggerEntry::grant_producer, its restore migration, serialized-map adapter, replay option, and zone-wide grant retirement therefore add a second provenance surface without changing the live ledger identity.
This is not a harmless wrapper: it expands the serialized GameState contract and installs lifecycle mutations in game_object.rs and zones.rs for data which has no runtime consumer. Remove the producer-key/migration/wrapper/retirement path and retain the existing HashMap<TriggerDefinitionRef, u32> authority; if a different behavior is intended, demonstrate a current-main production-path failure that the exact-recipient key cannot represent.
🟡 Non-blocking
crates/engine/tests/integration/nadu_lavaspur_boots_max_times.rs:88-114 asserts that blinking a recipient must allocate a fresh TriggerGrantInstanceRef. The gameplay identity is already made fresh by the recipient incarnation in trigger_definition_ref; this assertion instead locks in the unrelated retirement implementation above. Keep the nine production-pipeline activation attempts at :115-143, but avoid requiring a particular internal grant allocator transition unless it guards separately specified behavior.
✅ Clean
The prior test-discrimination blocker is resolved: nadu_lavaspur_boots_max_times.rs:115-126 now sends every recipient through a third activation, with sufficient mana/library fixtures, while :129-143 preserves the expected six fires and two per recipient. CodeRabbit's same third-attempt finding is therefore addressed. Its older producer-wide-key request is refuted by the recipient-local TriggerDefinitionRef authority cited above.
Recommendation: request changes to reduce this to the existing recipient-local ledger seam and the focused runtime regression; do not enqueue the new serialized/lifecycle machinery.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked: the new head restores the recipient-local ledger correctly, but the remaining lifecycle change is redundant and the PR no longer contains a behavior change worth landing.
🔴 Blocker
crates/engine/src/game/game_object.rs:1577-1584 already forms every TriggerDefinitionRef from the recipient's ObjectIncarnationRef and its trigger occurrence. The live MaxTimes gate and recorder already read/write that exact key at crates/engine/src/game/triggers.rs:9589-9597 and :11338-11352. Consequently a recipient that changes zones cannot collide with its former trigger ledger entry: its source incarnation is already different.
The new retire_all_grants() calls at crates/engine/src/game/zones.rs:925, :1197, :1832, and :2089 therefore add mutable lifecycle work without changing the MaxTimes identity or fixing a reachable cap failure. The new Nadu regression exercises the same recipient-local behavior that main already has; it does not identify a production behavior that fails before this diff. Please remove the redundant retirement/tests and only reopen this work with a current-main reproduction that fails through the production pipeline and changes the owning authority.
🔴 Blocker
crates/phase-ai/src/policies/tests/draw_payoff.rs:15, :581-582, and :606-607 still import and construct TriggerFireLedgerKey, which this head removes. Current CI fails with E0432 in that test module; this is the common cause of the Rust lint and test-shard failures.
✅ Clean
The current coverage-parse-diff artifact is bound to 216aae8a67b6c64d51215d7dfb4c6fc7e9fbf4f7 and reports no card-parse changes. The third activation loop now reaches the cap, so the earlier CodeRabbit test-discrimination thread is resolved.
Recommendation: request changes — do not repair the stale AI test import in isolation; first establish a real current-main regression and make a focused change at the authority it proves.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review: the narrowed regression is registered and discriminates recipient-local MaxTimesPerTurn caps; exact-head CI and parse evidence are clean.
2 participants
Summary
Fixes producer-wide
MaxTimesPerTurnaccounting for granted triggers, covering Nadu, Winged Wisdom and the Lavaspur Boots regression path. Equivalent recipient grants now share the granting producer's fire ledger, with exact legacy migration, replay resolution, and lifecycle retirement across zone changes and merges.Files changed
Track
Developer
LLM
Model: GitHub Copilot (GPT-5.6 Luna)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
None.
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all -- --check— exit 0.cargo clippy --all-targets -- -D warnings— exit 0.cargo test -p phase-engine --all-targets— exit 0.cargo test -p phase-ai --all-targets— exit 0../scripts/gen-card-data.sh— exit 0.Independent receipt validation — 348 fields, 7 projections, 5 completion checks, 90 verified artifacts; pass.
Projection comparator —
oracle_changed: 0,added_cards: [],removed_cards: [],clusters: [].Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=38a863fdcf6afe0426297f78dddb1c632d7842e9 base=b8185d37bb6a1d49ae5b7fe8e955e2ce54a4357c
Anchored on
Final review-impl
Final review-impl PASS head=38a863fdcf6afe0426297f78dddb1c632d7842e9
Claimed parse impact
None.
Scope Expansion
None in engine behavior. The Rustls dependency migration in
crates/feed-scraper/Cargo.tomlwas required to remove the OpenSSL/pkg-config verification blocker.Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests