ship/cr733 p1 mana provenance - #6331
Merged
Merged
Conversation
The P1 journal had no steady-state truncation, growing for the whole game. A turn transition cannot begin with a payment in flight (empty stack, settled prompts, pools drained per CR 106.4), so the boundary clear is safe until the CR 733 settlement consumer defines the real retention window. Regression test drives a real PassPriority rollover.
debit_cost_with_plan now returns the exact spent units for provenance; the simulation-only caller try_pay_with_hybrid_plan discards them.
…; journal is a provenance_sink matrix field CR 603.3d: a triggered mana ability still resolves after its source leaves. P1's exact-identity expect panicked for synthetic/pre-P04 callers with the source absent; the node is now conditional and mana falls back to the automatic Proposal attribution, preserving pip conservation. The P0 field-set ratchet correctly caught the journal itself as a new written field — classified provenance_sink (the sink is never journaled), fixtures rebuilt against the P1-tree census with line-agnostic classification carry-forward (2590/2590 carried, 4 new journal-sink sites, 0 unmatched).
matthewevans
enabled auto-merge
July 22, 2026 10:37
Parse changes introduced by this PR✓ No card-parse changes detected. |
Contributor
📝 WalkthroughWalkthroughAdds a resolved-rules journal with typed execution nodes and exact mana provenance, integrates attribution through mana activation and payment, resets and redacts journal state, and adds integration coverage for provenance, retention, allocator repair, and visibility. ChangesResolved mana provenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant ManaAbilities
participant GameState
participant ResolvedRulesJournal
participant ManaPool
Player->>ManaAbilities: activate mana ability
ManaAbilities->>GameState: begin execution node
ManaAbilities->>ManaPool: produce and add ManaUnit
GameState->>ResolvedRulesJournal: record produced pip
Player->>ManaAbilities: pay activation cost
ManaAbilities->>GameState: record spent ManaUnit
GameState->>ResolvedRulesJournal: link payment to producer
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/casting.rs`:
- Around line 14256-14257: Move the state.record_mana_payment call in each of
the three payment funnels, identified by the nearby mana_payment_recipient
usages, until after the complete life_payments validation succeeds. Ensure paths
returning InsufficientLife or Prohibited do not record spent_units, while
successful payments retain the existing recipient and journal entry behavior.
In `@crates/engine/src/game/engine_replacement.rs`:
- Line 639: Update the replacement-resume path around add_mana_to_pool to
capture its returned pool-stamped mana units instead of discarding them, then
pass those units through the existing provenance-recording helper used by other
mana-production callers. Add a regression test covering mana produced after
accepting a replacement and verifying its journal provenance record.
In `@crates/engine/src/game/turns.rs`:
- Around line 941-947: The start_next_turn turn-transition flow must clear
resolved_rules_journal immediately after incrementing turn_number so
skipped-turn early returns also reset provenance; remove the later reset near
the normal transition completion, preserving the one-turn retention behavior.
In `@crates/engine/tests/integration/cr733_resolved_commands_p1.rs`:
- Around line 117-156: Strengthen the journal-boundedness test around the
in-turn loop by asserting that the journal remains non-empty while turn_number
still equals start_turn, after the phase/step transition caused by PassPriority.
Keep the existing pre-turn and post-turn checks, and gate the new assertion on
the unchanged turn number so the test specifically verifies truncation occurs
only at the turn boundary.
🪄 Autofix (Beta)
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: b823ccc2-3e07-4bcd-9c1f-66ba133c5081
📒 Files selected for processing (30)
.agents/cr733/RUN4-REPORT.mdcrates/engine/src/ai_support/candidates.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/collect_evidence.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_debug.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_payment.rscrates/engine/src/game/scenario.rscrates/engine/src/game/turns.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/fixtures/cr733/authority_matrix.jsoncrates/engine/tests/fixtures/cr733/blocked_write_sites.jsoncrates/engine/tests/fixtures/cr733/rng_allocator_map.jsoncrates/engine/tests/fixtures/cr733/side_effect_map.jsoncrates/engine/tests/integration/ashling_delayed_sacrifice_unless_pay.rscrates/engine/tests/integration/captain_america_throw.rscrates/engine/tests/integration/cr733_resolved_commands_p0.rscrates/engine/tests/integration/cr733_resolved_commands_p1.rscrates/engine/tests/integration/main.rs
Comment on lines
+14256
to
+14257
| let recipient = state.mana_payment_recipient(source_id, player); | ||
| state.record_mana_payment(player, recipient, &spent_units); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Commit provenance only after the complete payment succeeds.
All three funnels record spent_units before the life_payments loop can return InsufficientLife or Prohibited. A rejected action can therefore leave a journal entry claiming mana was spent; move recording below life-payment validation, or roll it back on failure.
Also applies to: 14735-14738, 14894-14895
🤖 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/casting.rs` around lines 14256 - 14257, Move the
state.record_mana_payment call in each of the three payment funnels, identified
by the nearby mana_payment_recipient usages, until after the complete
life_payments validation succeeds. Ensure paths returning InsufficientLife or
Prohibited do not record spent_units, while successful payments retain the
existing recipient and journal entry behavior.
| // CR 118.3a: stamp a stable pip id on pool entry so the unit | ||
| // can be pinned to direct payment. | ||
| state.add_mana_to_pool(player_id, unit); | ||
| let _ = state.add_mana_to_pool(player_id, unit); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not discard the inserted mana units.
add_mana_to_pool now returns the pool-stamped units needed for exact provenance, but this replacement-resume path drops them. Mana produced after accepting a replacement can therefore be available for payment without a corresponding journal record. Capture the returned units and route them through the same provenance-recording helper as the other mana-production callers, with a regression test for this path.
🤖 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/engine_replacement.rs` at line 639, Update the
replacement-resume path around add_mana_to_pool to capture its returned
pool-stamped mana units instead of discarding them, then pass those units
through the existing provenance-recording helper used by other mana-production
callers. Add a regression test covering mana produced after accepting a
replacement and verifying its journal provenance record.
Comment on lines
+941
to
+947
| // P1 retention policy (not a CR rule): the resolved-rules provenance | ||
| // journal only has consumers within a payment/announcement window, and a | ||
| // turn transition cannot begin with a payment in flight (the stack is | ||
| // empty, prompts are settled, and mana pools drained at step end per | ||
| // CR 106.4). Truncating here bounds journal growth to one turn until the | ||
| // CR 733 settlement consumer defines the real retention window. | ||
| state.resolved_rules_journal = Default::default(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset the journal before skipped-turn recursion.
start_next_turn increments turn_number at Line [813] but returns at Lines [836] and [847] before reaching this reset. Skipped turns therefore do not clear resolved_rules_journal, allowing prior-turn provenance to survive into the next turn and violating the one-turn retention contract. Clear it immediately after the turn increment, then remove this late-only reset.
🤖 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/turns.rs` around lines 941 - 947, The start_next_turn
turn-transition flow must clear resolved_rules_journal immediately after
incrementing turn_number so skipped-turn early returns also reset provenance;
remove the later reset near the normal transition completion, preserving the
one-turn retention behavior.
Comment on lines
+117
to
+156
| fn provenance_journal_is_truncated_at_the_turn_boundary() { | ||
| let mut scenario = GameScenario::new_n_player(2, 7); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let mut runner = scenario.build(); | ||
| let _ = runner.state_mut().add_mana_to_pool( | ||
| P0, | ||
| ManaUnit::new(ManaType::Green, ObjectId(99), false, Vec::new()), | ||
| ); | ||
| assert!( | ||
| !runner | ||
| .state() | ||
| .resolved_rules_journal | ||
| .produced_mana() | ||
| .is_empty(), | ||
| "journal must have provenance before the boundary" | ||
| ); | ||
| runner.state_mut().players[0].mana_pool.clear(); | ||
|
|
||
| let start_turn = runner.state().turn_number; | ||
| let mut guard = 0; | ||
| while runner.state().turn_number == start_turn { | ||
| runner | ||
| .act(GameAction::PassPriority) | ||
| .expect("passing priority must advance an empty-stack game"); | ||
| guard += 1; | ||
| assert!( | ||
| guard < 200, | ||
| "turn must roll over within a bounded pass count" | ||
| ); | ||
| } | ||
| assert!( | ||
| runner | ||
| .state() | ||
| .resolved_rules_journal | ||
| .produced_mana() | ||
| .is_empty() | ||
| && runner.state().resolved_rules_journal.nodes().is_empty(), | ||
| "the turn boundary must truncate the provenance journal" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing mid-turn assertion that the journal survives phase/step transitions.
The docstring states the journal is "bounded to one turn," but the test only checks the journal before any PassPriority and after the turn number has already changed. It never verifies the journal stays non-empty across an in-turn phase/step transition (e.g., after the first PassPriority that changes phase but not turn_number). If truncation were accidentally implemented at phase/step granularity instead of turn granularity, this test would still pass, since the final check only runs once turn_number has already rolled over.
Add an assertion inside the loop, gated on turn_number still equalling start_turn, that the journal remains non-empty — this proves truncation is scoped to the turn boundary specifically, not any earlier step/phase transition.
As per path instructions, "Test adequacy is the highest-frequency contributor finding — scrutinize it," and a test must "exercise the FAILURE path the fix prevents."
✅ Suggested mid-loop reach-check
let start_turn = runner.state().turn_number;
let mut guard = 0;
+ let mut observed_mid_turn_persistence = false;
while runner.state().turn_number == start_turn {
runner
.act(GameAction::PassPriority)
.expect("passing priority must advance an empty-stack game");
+ if runner.state().turn_number == start_turn
+ && !runner.state().resolved_rules_journal.produced_mana().is_empty()
+ {
+ observed_mid_turn_persistence = true;
+ }
guard += 1;
assert!(
guard < 200,
"turn must roll over within a bounded pass count"
);
}
+ assert!(
+ observed_mid_turn_persistence,
+ "journal must persist across phase/step transitions within the same turn"
+ );🤖 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/cr733_resolved_commands_p1.rs` around lines
117 - 156, Strengthen the journal-boundedness test around the in-turn loop by
asserting that the journal remains non-empty while turn_number still equals
start_turn, after the phase/step transition caused by PassPriority. Keep the
existing pre-turn and post-turn checks, and gate the new assertion on the
unchanged turn number so the test specifically verifies truncation occurs only
at the turn boundary.
Source: Path instructions
…amped sentinel Pre-provenance saves deserialize next_pip_id to 0 (serde default), and normalize_for_loop zeroes it for CR 104.4b comparison. Minting from 0 stamped ManaPipId(0) — the exact sentinel the resolved-mana appliers fail closed on — which panicked add_mana_to_pool inside the AI cast feasibility probe on phase-ai community scenarios (PR #6331 CI).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/tests/integration/cr733_resolved_commands_p1.rs (1)
52-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert the Signet’s produced-mana records, not only the consumed land pip.
Using
.first()andany(...)does not prove exact provenance: the test never verifies the two units produced by the Signet, nor rejects duplicate or missing records. Assert the expected Signet-produced records, their source/producer, and their cardinality.As per path instructions, integration tests must exercise the failure path the fix prevents and drive 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/cr733_resolved_commands_p1.rs` around lines 52 - 74, The integration test currently validates only the first spent pip and uses any(), so it does not verify exact Signet-produced mana provenance or cardinality. Update the assertions around spent_mana() and produced_mana() to identify and assert both expected Signet-produced records, including their units, source, producer, and exact count; ensure the test drives the production pipeline and would fail for missing or duplicate records.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/types/game_state.rs`:
- Around line 15129-15139: Remove the “CR 118.3a” citation from the
documentation for next_pip_id, while retaining the comments describing its
monotonic allocation and guarantee against ManaPipId(0). Do not alter the
implementation or the separate explanation of zero allocator recovery.
- Around line 15137-15141: Update the pip ID allocation logic around next_pip_id
to use checked addition and fail closed when the maximum ID is reached, rather
than allowing wraparound to zero and resetting to one. Preserve unique ManaPipId
provenance by refusing further minting at exhaustion, and add a test covering
the maximum-boundary behavior.
In `@crates/engine/tests/integration/cr733_resolved_commands_p1.rs`:
- Around line 158-176: Strengthen the test
legacy_zero_pip_allocator_self_heals_instead_of_minting_the_sentinel by
inserting a second mana unit after the first allocation. Assert both returned
pip_id values are nonzero and distinct, proving the repaired allocator advances
rather than remaining at zero or reusing the same ID.
---
Outside diff comments:
In `@crates/engine/tests/integration/cr733_resolved_commands_p1.rs`:
- Around line 52-74: The integration test currently validates only the first
spent pip and uses any(), so it does not verify exact Signet-produced mana
provenance or cardinality. Update the assertions around spent_mana() and
produced_mana() to identify and assert both expected Signet-produced records,
including their units, source, producer, and exact count; ensure the test drives
the production pipeline and would fail for missing or duplicate records.
🪄 Autofix (Beta)
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: e0577b0e-f4c5-4cc5-afc8-1bd74bf56682
📒 Files selected for processing (2)
crates/engine/src/types/game_state.rscrates/engine/tests/integration/cr733_resolved_commands_p1.rs
Comment on lines
15129
to
+15139
| /// CR 118.3a: Mint the next stable `ManaPipId` for a pool unit. Monotonic, | ||
| /// never returns the `ManaPipId(0)` unstamped sentinel (counter starts at 1). | ||
| fn next_pip_id(&mut self) -> ManaPipId { | ||
| // ManaPipId(0) is the "unstamped" sentinel the resolved-mana appliers | ||
| // fail closed on. Two legitimate states carry a zero allocator: a | ||
| // pre-provenance save (`#[serde(default)]`) and a loop-normalized | ||
| // clone (`normalize_for_loop` zeroes it for CR 104.4b comparison). | ||
| // Self-heal at mint so neither can ever stamp the sentinel. | ||
| if self.next_pip_id == 0 { | ||
| self.next_pip_id = 1; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- cited file context ---\n'
sed -n '15120,15155p' crates/engine/src/types/game_state.rs
printf '\n--- CR 118.3a in docs/MagicCompRules.txt ---\n'
rg -n "118\.3a|118\.3" docs/MagicCompRules.txt | head -n 20
printf '\n--- nearby rules text ---\n'
python3 - <<'PY'
from pathlib import Path
text = Path('docs/MagicCompRules.txt').read_text(errors='ignore').splitlines()
for i, line in enumerate(text):
if '118.3a' in line or ('118.3' in line and '118.3a' not in line):
start=max(0,i-2); end=min(len(text),i+8)
for j in range(start,end):
print(f"{j+1}: {text[j]}")
print('---')
PYRepository: phase-rs/phase
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '15120,15155p' crates/engine/src/types/game_state.rs
rg -n "118\.3a|118\.3" docs/MagicCompRules.txt | head -n 20Repository: phase-rs/phase
Length of output: 2225
🌐 Web query:
Magic Comprehensive Rules 118.3a mana pool paying mana text
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, section 118.3a states: Paying mana is done by removing the indicated mana from a player’s mana pool. (Players can always pay 0 mana.) If excess mana remains in that player’s mana pool after making that payment, the player announces what mana is still there [1][2].
Citations:
Remove the CR 118.3a citation here
118.3a covers paying mana from a mana pool, not minting or stamping ManaPipIds. Keep the pip-ID invariant documented separately.
🤖 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 15129 - 15139, Remove the
“CR 118.3a” citation from the documentation for next_pip_id, while retaining the
comments describing its monotonic allocation and guarantee against ManaPipId(0).
Do not alter the implementation or the separate explanation of zero allocator
recovery.
Source: Path instructions
Comment on lines
+15137
to
15141
| if self.next_pip_id == 0 { | ||
| self.next_pip_id = 1; | ||
| } | ||
| let id = self.next_pip_id; | ||
| self.next_pip_id += 1; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first
ast-grep outline crates/engine/src/types/game_state.rs --view expanded > /tmp/game_state_outline.txt
cat /tmp/game_state_outline.txt
echo
echo "=== Relevant lines around next_pip_id ==="
sed -n '15110,15160p' crates/engine/src/types/game_state.rs | cat -n
echo
echo "=== Search for next_pip_id usages ==="
rg -n "next_pip_id|pip_id" crates/engine/src/types/game_state.rs crates/engine/src -g '*.rs'
echo
echo "=== Search for provenance/journal duplicate pip ID handling ==="
rg -n "duplicate.*pip|pip.*duplicate|provenance|journal" crates/engine/src -g '*.rs'Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/engine/src/types/game_state.rs')
text = p.read_text()
# Extract a wider context around the allocator and the nearby CR citation.
needle = "next_pip_id"
idx = text.find(needle)
start = max(0, idx - 1500)
end = min(len(text), idx + 1200)
print(text[start:end])
print("\n=== occurrences of CR 118.3a in file ===")
for i, line in enumerate(text.splitlines(), 1):
if "CR 118.3a" in line:
print(f"{i}: {line}")
PYRepository: phase-rs/phase
Length of output: 3656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== next_pip_id allocator and nearby comments ==="
sed -n '15120,15155p' crates/engine/src/types/game_state.rs | cat -n
echo
echo "=== direct next_pip_id references ==="
rg -n -C 3 "next_pip_id" crates/engine/src/types/game_state.rs
echo
echo "=== CR 118.3a in rules text ==="
rg -n -C 2 "^118\.3a|118\.3a" docs/MagicCompRules.txtRepository: phase-rs/phase
Length of output: 7750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('crates/engine/src/types/game_state.rs').read_text().splitlines()
for i, line in enumerate(text, 1):
if 'next_pip_id' in line:
for j in range(max(1, i-5), min(len(text), i+12)+1):
print(f"{j}: {text[j-1]}")
print("---")
PYRepository: phase-rs/phase
Length of output: 16072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('docs/MagicCompRules.txt')
text = p.read_text(errors='ignore').splitlines()
for i, line in enumerate(text, 1):
if line.startswith('118.3a') or '118.3a' in line:
for j in range(max(1, i-2), min(len(text), i+4)+1):
print(f"{j}: {text[j-1]}")
print('---')
PYRepository: phase-rs/phase
Length of output: 702
Fail closed when pip IDs wrap.
next_pip_id += 1 can overflow to 0; the zero-reset branch then starts minting 1 again, which can collide with already stamped ManaPipIds and corrupt provenance. Use checked_add and stop minting before exhaustion, with a max-boundary test.
🤖 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 15137 - 15141, Update the
pip ID allocation logic around next_pip_id to use checked addition and fail
closed when the maximum ID is reached, rather than allowing wraparound to zero
and resetting to one. Preserve unique ManaPipId provenance by refusing further
minting at exhaustion, and add a test covering the maximum-boundary behavior.
Comment on lines
+158
to
+176
| /// A pre-provenance save deserializes `next_pip_id` to 0 (serde default). The | ||
| /// allocator must self-heal rather than mint the ManaPipId(0) sentinel, which | ||
| /// the resolved-mana appliers fail closed on (this panicked the phase-ai | ||
| /// community scenarios in PR #6331's first CI run). | ||
| #[test] | ||
| fn legacy_zero_pip_allocator_self_heals_instead_of_minting_the_sentinel() { | ||
| let mut state = GameState::new_two_player(11); | ||
| state.next_pip_id = 0; | ||
| let inserted = state | ||
| .add_mana_to_pool( | ||
| P0, | ||
| ManaUnit::new(ManaType::Red, ObjectId(7), false, Vec::new()), | ||
| ) | ||
| .expect("insert into a known player's pool must succeed"); | ||
| assert_ne!( | ||
| inserted.pip_id.0, 0, | ||
| "a legacy zero allocator must never stamp the unstamped sentinel" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise a second allocation after repairing the zero cursor.
This single-insertion test does not prove self-healing. An allocator that returns one nonzero ID but remains at zero or reuses the same ID would pass. Insert a second unit and assert both IDs are nonzero and distinct.
Suggested regression strengthening
assert_ne!(
inserted.pip_id.0, 0,
"a legacy zero allocator must never stamp the unstamped sentinel"
);
+ let inserted_again = state
+ .add_mana_to_pool(
+ P0,
+ ManaUnit::new(ManaType::Red, ObjectId(8), false, Vec::new()),
+ )
+ .expect("the repaired allocator must support subsequent inserts");
+ assert_ne!(inserted_again.pip_id.0, 0);
+ assert_ne!(inserted.pip_id, inserted_again.pip_id);As per path instructions, integration tests must exercise the failure path the fix prevents.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// A pre-provenance save deserializes `next_pip_id` to 0 (serde default). The | |
| /// allocator must self-heal rather than mint the ManaPipId(0) sentinel, which | |
| /// the resolved-mana appliers fail closed on (this panicked the phase-ai | |
| /// community scenarios in PR #6331's first CI run). | |
| #[test] | |
| fn legacy_zero_pip_allocator_self_heals_instead_of_minting_the_sentinel() { | |
| let mut state = GameState::new_two_player(11); | |
| state.next_pip_id = 0; | |
| let inserted = state | |
| .add_mana_to_pool( | |
| P0, | |
| ManaUnit::new(ManaType::Red, ObjectId(7), false, Vec::new()), | |
| ) | |
| .expect("insert into a known player's pool must succeed"); | |
| assert_ne!( | |
| inserted.pip_id.0, 0, | |
| "a legacy zero allocator must never stamp the unstamped sentinel" | |
| ); | |
| } | |
| /// A pre-provenance save deserializes `next_pip_id` to 0 (serde default). The | |
| /// allocator must self-heal rather than mint the ManaPipId(0) sentinel, which | |
| /// the resolved-mana appliers fail closed on (this panicked the phase-ai | |
| /// community scenarios in PR `#6331`'s first CI run). | |
| #[test] | |
| fn legacy_zero_pip_allocator_self_heals_instead_of_minting_the_sentinel() { | |
| let mut state = GameState::new_two_player(11); | |
| state.next_pip_id = 0; | |
| let inserted = state | |
| .add_mana_to_pool( | |
| P0, | |
| ManaUnit::new(ManaType::Red, ObjectId(7), false, Vec::new()), | |
| ) | |
| .expect("insert into a known player's pool must succeed"); | |
| assert_ne!( | |
| inserted.pip_id.0, 0, | |
| "a legacy zero allocator must never stamp the unstamped sentinel" | |
| ); | |
| let inserted_again = state | |
| .add_mana_to_pool( | |
| P0, | |
| ManaUnit::new(ManaType::Red, ObjectId(8), false, Vec::new()), | |
| ) | |
| .expect("the repaired allocator must support subsequent inserts"); | |
| assert_ne!(inserted_again.pip_id.0, 0); | |
| assert_ne!(inserted.pip_id, inserted_again.pip_id); | |
| } |
🤖 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/cr733_resolved_commands_p1.rs` around lines
158 - 176, Strengthen the test
legacy_zero_pip_allocator_self_heals_instead_of_minting_the_sentinel by
inserting a second mana unit after the first allocation. Assert both returned
pip_id values are nonzero and distinct, proving the repaired allocator advances
rather than remaining at zero or reusing the same ID.
Source: Path instructions
This was referenced Jul 22, 2026
jsdevninja
pushed a commit
to jsdevninja/phase
that referenced
this pull request
Jul 24, 2026
* cr733(p1): record mana provenance journal * cr733(p1): document run 4 handoff * cr733(p1): bound the provenance journal to one turn The P1 journal had no steady-state truncation, growing for the whole game. A turn transition cannot begin with a payment in flight (empty stack, settled prompts, pools drained per CR 106.4), so the boundary clear is safe until the CR 733 settlement consumer defines the real retention window. Regression test drives a real PassPriority rollover. * cr733(p1): map the hybrid-plan affordability probe back to Option<()> debit_cost_with_plan now returns the exact spent units for provenance; the simulation-only caller try_pay_with_hybrid_plan discards them. * cr733(p1): appease clippy — tuple-variant constructors and to_vec * cr733(p1): widen active_rules_execution_node to pub(crate) for FRU construction * cr733(p1): source-gone triggered mana degrades to ambient attribution; journal is a provenance_sink matrix field CR 603.3d: a triggered mana ability still resolves after its source leaves. P1's exact-identity expect panicked for synthetic/pre-P04 callers with the source absent; the node is now conditional and mana falls back to the automatic Proposal attribution, preserving pip conservation. The P0 field-set ratchet correctly caught the journal itself as a new written field — classified provenance_sink (the sink is never journaled), fixtures rebuilt against the P1-tree census with line-agnostic classification carry-forward (2590/2590 carried, 4 new journal-sink sites, 0 unmatched). * cr733(p1): appease clippy in journal unit tests * cr733(p1): self-heal a zero pip allocator instead of minting the unstamped sentinel Pre-provenance saves deserialize next_pip_id to 0 (serde default), and normalize_for_loop zeroes it for CR 104.4b comparison. Minting from 0 stamped ManaPipId(0) — the exact sentinel the resolved-mana appliers fail closed on — which panicked add_mana_to_pool inside the AI cast feasibility probe on phase-ai community scenarios (PR phase-rs#6331 CI). --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This was referenced Jul 26, 2026
Merged
Merged
This was referenced Jul 26, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Privacy
Bug Fixes