fix(engine): tell the player the ∞ they agreed to is bounded, and answer it where the controller is still known - #7045
fix(engine): tell the player the ∞ they agreed to is bounded, and answer it where the controller is still known#7045lgray wants to merge 17 commits into
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 engine now exposes per-player, per-family unbounded collapse state. The client consumes this projection for badge grouping, localized tooltips, and state-specific glyphs. Boundary, protocol, wire, integration, and HUD tests cover scheduling, certainty, attribution, persistence, and rendering. ChangesUnbounded family collapse
Estimated code review effort: 5 (Critical) | ~120 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 |
github-actions
Bot
added
the
needs-maintainer
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/derived_views.rs (1)
841-862: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply accepted shortcut results at the proposed ending point.
The engine leaves accepted shortcut effects unapplied, returns priority, and applies them later at a phase or step boundary. Intervening actions can change replacements, legality, and the final result after all players accepted the shortcut. The shortcut rule requires the game to advance to the final proposed ending point with the proposed choices taken. (media.wizards.com)
crates/engine/src/game/derived_views.rs#L841-L862: remove the deferred-state projection contract after acceptance.crates/engine/src/types/game_state.rs#L19883-L19895: apply accepted materializations atomically during shortcut acceptance instead of retaining them for a boundary prompt.crates/engine/tests/integration/combo_infinite_pile.rs#L1720-L1768: replace the boundary-prompt assertion with a production-path assertion for immediate accepted-shortcut materialization.As per coding guidelines, implement engine behavior according to the Comprehensive Rules. As per path instructions, strict CR fidelity is required.
🤖 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/derived_views.rs` around lines 841 - 862, Apply accepted shortcut results immediately at the proposed ending point instead of deferring them to a later phase or step boundary. In crates/engine/src/game/derived_views.rs:841-862, remove the deferred-state projection contract; in crates/engine/src/types/game_state.rs:19883-19895, update the shortcut-acceptance flow to atomically apply all accepted materializations and avoid retaining a boundary prompt; in crates/engine/tests/integration/combo_infinite_pile.rs:1720-1768, replace the boundary-prompt expectation with an assertion covering immediate production-path materialization.Sources: Coding guidelines, 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 `@client/src/components/hud/HudBadges.tsx`:
- Around line 450-478: The client-side unboundedFamilyViews function must no
longer derive family-level scheduled state from individual rows. Add an
engine-provided grouped family-view projection, thread it through every adapter,
and update the HUD rendering path to consume that projection directly, removing
the client-side family aggregation while preserving the engine’s U3c scheduling
behavior.
In `@crates/engine/src/game/derived_views.rs`:
- Around line 164-199: Replace the boolean scheduled projection in the
derived-view family flow with an engine-owned typed collapse state
distinguishing Unscheduled, Scheduled, and Mixed, computed before family
grouping across all contributing controllers. Ensure the display renders ∞→N
only for Scheduled; Mixed must remain unbounded, and preserve producer identity
or equivalent aggregation data until this state is computed. Update
serialization and frontend consumption to use this engine-provided state rather
than inferring it.
---
Outside diff comments:
In `@crates/engine/src/game/derived_views.rs`:
- Around line 841-862: Apply accepted shortcut results immediately at the
proposed ending point instead of deferring them to a later phase or step
boundary. In crates/engine/src/game/derived_views.rs:841-862, remove the
deferred-state projection contract; in
crates/engine/src/types/game_state.rs:19883-19895, update the
shortcut-acceptance flow to atomically apply all accepted materializations and
avoid retaining a boundary prompt; in
crates/engine/tests/integration/combo_infinite_pile.rs:1720-1768, replace the
boundary-prompt expectation with an assertion covering immediate production-path
materialization.
🪄 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: ce24d4dd-7c7d-4871-bfd0-a90152e20793
📒 Files selected for processing (22)
client/src/adapter/types.tsclient/src/components/board/OpponentSeatHeader.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/OpponentHud.tsxclient/src/components/hud/PlayerHud.tsxclient/src/components/hud/__tests__/UnboundedBadge.test.tsxclient/src/hooks/usePlayerDesignations.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Family-level scheduled state is derived in the frontend and over-promises a bound. Evidence: client/src/components/hud/HudBadges.tsx:450-477 OR-folds row flags by family, client/src/components/hud/PlayerHud.tsx:137-140 renders one family badge, and client/src/components/hud/__tests__/UnboundedBadge.test.tsx:97-106 deliberately locks a scheduled Counter(..) sibling plus an unscheduled one into scheduled: true. Why it matters: the sole ∞→N badge states that a finite amount will be chosen for a family which can still contain an unbounded unscheduled contributing loop; this is game-state derivation in the display layer rather than an engine-owned projection. Suggested fix: emit an engine-owned, per-seat family projection with a typed Unscheduled / Scheduled / Mixed collapse state before producer identity is lost, and render ∞→N only for Scheduled (with discriminating mixed-family coverage).
[MED] A scheduled badge can promise a collapse that the boundary later declines. Evidence: crates/engine/src/game/derived_views.rs:1138-1150 documents that Counter/Life axes may remain ∞ after observer drift, while HudBadges.tsx:536-555 renders the tooltip and glyph claiming that a finite amount will be chosen; crates/engine/tests/integration/combo_infinite_pile.rs:2125-2183 proves the counter axis stays marked after the boundary declines it. Why it matters: a player receives an affirmative bounded-growth promise in the exact accept-to-boundary window, yet the actual outcome can be continued unbounded play. Suggested fix: keep the UI explicitly pending/conditional until the boundary commits, or publish an engine-owned eligible/declined state and wire it through the HUD with a boundary-decline regression test.
db6b400 to
20046ea
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The current head still performs the family collapse-state decision in the frontend and knowingly over-promises a bound. Evidence at 20046ea57fc129396ac8b14f65cca702377a6f46: client/src/components/hud/HudBadges.tsx:450-477 groups engine rows with an OR fold, while its own U3c test (UnboundedBadge.test.tsx:69-76) locks a scheduled Counter(..) row plus an unscheduled Poison row into { family: "counters", scheduled: true }. The badge then states ∞→N for a family that still has an unbounded contributor. That is a game-state projection performed by the display layer, contrary to the engine-authority boundary. Please emit an engine-owned per-seat family projection with typed Unscheduled / Scheduled / Mixed state (computed before producer identity is lost), and show ∞→N only for Scheduled.
[MED] The engine exposes an affirmative scheduled-collapse badge even though the promised collapse can still be declined. Evidence at the current head: crates/engine/src/game/derived_views.rs:1221-1232 says a Counters/Life axis can be flagged and later decline when an observer appears in the accept-to-boundary window, leaving the axis ∞ with no finite application; HudBadges.tsx:528-555 nevertheless renders ∞→N and a tooltip saying a finite amount will be chosen. The cited integration witness covers the drift, so this is a known reachable false promise rather than a hypothetical race. Keep the client state explicitly pending/conditional until boundary commitment, or expose an engine-owned final eligible/declined state and add a boundary-decline UI regression.
matthewevans
removed
the
needs-maintainer
20046ea to
949b972
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🤖 AI text below 🤖 @matthewevans — both MED findings are answered structurally, not documented. Pushed MED-1, mixed-family discriminator. The last-wins OR fold is deleted from the client, not deprecated. The fold is now a commutative join over MED-2, boundary-decline regression. Worth flagging because it inverts the intuition the old badge encoded: the only arm that earns the affirmative Both dispositions this PR previously argued for are answered in place rather than deleted — a removed objection just invites re-derivation. See the table in the body. Two things I want to state plainly rather than let you find them.
Verification highlights: battery 8/8, run twice independently with identical exits (lib 18560/0, integration 4586/0, vitest 291 files / 2613 tests); every check that is silent on success carries a must-fail control, because this round already hit a green from a test filter that matched nothing. The parser projection was forced and re-run at the rebased tip: both sides' |
|
🤖 AI text below 🤖 @matthewevans — mapping your two requested changes to what shipped at MED-1 — "emit an engine-owned, per-seat family projection with a typed
Your U3c evidence is the exact case that flips: the scheduled MED-2 — "keep the UI explicitly pending/conditional until the boundary commits, or publish an engine-owned eligible/declined state and wire it through the HUD with a boundary-decline regression test." Took the second option, and made the state derive from the loop's control flow rather than from its decline policy.
The four One inversion worth your attention, since it reverses what the old badge assumed: the only arm that earns Two disclosures already in the body, repeated here so they aren't missed: round 3 closed under the terminal-round rule rather than a fresh review pass (its Verification box is left unchecked on purpose), and Gate A's PASS is vacuous — 0 files fall under its parser scope. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/src/game/engine_resolution_choices.rs (1)
1402-1416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the certainty mapping into the presentation layer.
materialization_certaintylives in the boundary module but returnscrate::game::derived_views::CollapseCertainty.derived_views::scheduled_display_axesthen calls back into this module. The two modules now name each other's types.
CollapseCertaintyis a display promise, so the rules/boundary module should not have to know it exists.possible_holdalready returns the engine-ownedOption<BoundaryHold>, which is the whole input the mapping needs.Keep
possible_holdhere and perform the mapping inderived_views. The dependency then runs one way: presentation reads the engine census and names its own display vocabulary.♻️ Proposed layering fix
In
crates/engine/src/game/engine_resolution_choices.rs, remove the mapping:-/// What the HUD may promise. `Conditional` iff the kind has a hold. No CR governs this — it is a -/// display promise derived from the census above, not a rules behavior. -/// -/// APPLIED-BUT-NULLIFIED is deliberately `Committed`: a `Counters` item whose bearers all left -/// (CR 400.7, the inner stale-id skip), a `Prevented` mint, and a `DriveSequence` committing k<N all -/// PUSH, so the ∞ genuinely ends. The shipped copy promises "a finite amount will be chosen", not a -/// quantity. -pub(crate) fn materialization_certainty( - item: &PersistentAxisMaterialization, -) -> crate::game::derived_views::CollapseCertainty { - match possible_hold(item) { - Some(_) => crate::game::derived_views::CollapseCertainty::Conditional, - None => crate::game::derived_views::CollapseCertainty::Committed, - } -}In
crates/engine/src/game/derived_views.rs, add the mapping besideCollapseCertaintyand read the census directly:impl CollapseCertainty { /// What the HUD may promise. `Conditional` iff the boundary kind has a hold /// (`engine_resolution_choices::possible_hold`). No CR governs this — it is a display promise /// derived from that census, not a rules behavior. /// /// APPLIED-BUT-NULLIFIED is deliberately `Committed`: a `Counters` item whose bearers all left /// (CR 400.7, the inner stale-id skip), a `Prevented` mint, and a `DriveSequence` committing /// k<N all PUSH, so the ∞ genuinely ends. fn for_materialization(item: &PersistentAxisMaterialization) -> Self { match crate::game::engine_resolution_choices::possible_hold(item) { Some(_) => Self::Conditional, None => Self::Committed, } } }Then in
scheduled_display_axes:let certainty = CollapseCertainty::for_materialization(item);Update
boundary_hold_census_matches_the_apply_loopto assert overpossible_holdplus the relocated mapping.🤖 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_resolution_choices.rs` around lines 1402 - 1416, Remove materialization_certainty from engine_resolution_choices so the boundary module only exposes possible_hold. Add CollapseCertainty::for_materialization in derived_views, mapping possible_hold(item) to Conditional or Committed, and update scheduled_display_axes to call it. Adjust boundary_hold_census_matches_the_apply_loop to validate possible_hold and the relocated presentation mapping.
🤖 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 `@client/src/components/hud/ManaPoolSummary.tsx`:
- Around line 30-33: Add a ManaPoolSummary render test using a nonempty mana
pool and an engine-provided unbounded_families row with family "mana"; assert
that the ∞ marker is rendered. Ensure the fixture supplies the engine-owned
unbounded_families data and does not derive the result from axes or another
fallback, covering the failure path guarded by hasUnboundedMana.
In `@crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs`:
- Around line 536-537: Update the assertion message near the real kilo accept
test to remove the claim that this is the suite’s only Committed family.
Describe it instead as the Committed witness from a real production dump and
identify it as the frame the client reads from unbounded-counter-wire.json,
preserving the existing ∞→N matched-positive context.
---
Nitpick comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 1402-1416: Remove materialization_certainty from
engine_resolution_choices so the boundary module only exposes possible_hold. Add
CollapseCertainty::for_materialization in derived_views, mapping
possible_hold(item) to Conditional or Committed, and update
scheduled_display_axes to call it. Adjust
boundary_hold_census_matches_the_apply_loop to validate possible_hold and the
relocated presentation mapping.
🪄 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: 91a2dc85-9989-42c8-9ad3-e3bb1801ba89
📒 Files selected for processing (28)
client/src/adapter/types.tsclient/src/components/board/OpponentSeatHeader.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/ManaPoolSummary.tsxclient/src/components/hud/OpponentHud.tsxclient/src/components/hud/PlayerHud.tsxclient/src/components/hud/__tests__/PlayerHud.designations.test.tsxclient/src/components/hud/__tests__/UnboundedBadge.test.tsxclient/src/hooks/usePlayerDesignations.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/test/fixtures/unbounded-counter-wire.jsonclient/src/test/fixtures/unbounded-declined-wire.jsonclient/src/test/fixtures/unbounded-family-tags.jsonclient/src/test/fixtures/unbounded-token-wire.jsonclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- client/src/i18n/locales/pl/game.json
- crates/engine/src/types/game_state.rs
- client/src/i18n/locales/it/game.json
- client/src/i18n/locales/pt/game.json
- client/src/components/board/OpponentSeatHeader.tsx
- client/src/i18n/locales/fr/game.json
- client/src/i18n/locales/de/game.json
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- client/src/components/hud/PlayerHud.tsx
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The new engine-owned unbounded_families payload changes the full-game state wire without a full-game protocol bump. Evidence: crates/engine/src/game/derived_views.rs:557 adds the serialized field and client/src/components/hud/PlayerHud.tsx:137-140 renders only that channel, but client/src/adapter/ws-adapter.ts:201-225 and crates/lobby-broker/src/protocol.rs:61 still advertise protocol 23 (which crates/server-core/src/protocol.rs:20-26 inherits for full-game handshakes). Why it matters: a new client can handshake with a pre-change full-game server at v23, receive the old unbounded_resources state without the family projection, and silently lose every infinity HUD badge. Suggested fix: bump the full-game protocol version in lockstep, retain the strict full-game minimum, and update the version history and protocol contract tests.
[LOW] The required parse-impact sticky is not evidence for this head. Evidence: the <!-- coverage-parse-diff --> comment is bound to 20046ea57fc129396ac8b14f65cca702377a6f46, while this PR head is 949b9725056fb95eaa1ffe46b0e2f1ff47799fd5. Why it matters: even a no-change parse result cannot be attributed to the reviewed commit. Suggested fix: regenerate the head-bound parse-diff artifact (or publish an equivalent current-head projection) before requesting approval.
|
🤖 AI text below 🤖 @matthewevans — both findings fixed and pushed as MED — protocol bumpConfirmed, and there's a wrinkle worth putting on the record because it would bite the next person.
The rule is about wire parseability. Your failure mode is capability: this PR deleted the client's row-flag OR-fold fallback, so a v23 server that simply omits the field leaves a v24 client rendering zero ∞ badges — silently, with no parse error anywhere to catch it. The handshake is the only place that pairing is refusable. So I amended the rule as well as the number. Bumping without qualifying the rule leaves the next reader correctly following a rule that is wrong for this class of change.
The contract tests were a near-miss. My first census concluded every site derived the constant. It was wrong: I'd grepped Added the assertion with teeth, because all three of those pass at any value and so none of them detects the regression the bump exists to prevent: fn full_game_floor_is_current_only_not_a_rollout_window() {
assert_eq!(MIN_SUPPORTED_PROTOCOL, PROTOCOL_VERSION, ...);
assert_eq!(LOBBY_MIN_SUPPORTED_PROTOCOL, PROTOCOL_VERSION - 1);
}Revert-probe, three arms, measured: as shipped 315 passed / 0 failed; mutant Scope: this took the change outside its frozen path set, so it's recorded as scope amendment r3 (27 → 30 paths) with your review quoted as the authorizing instrument, new list, new sha256s, and a both-direction LOW — parse-diff sticky bound to the old headCorrect, and re-bound by measurement rather than by regenerating a projection whose inputs provably didn't move.
Verification at
|
|
🤖 AI text below 🤖 @coderabbitai — three findings, measured individually against the code. One confirmed and fixed (and it was worse than reported), two refuted with measurements. 🟠 Major — "Add coverage for the engine-owned Mana family channel" — REFUTEDThat test exists, and this PR added it. it("renders from the family channel alone, with no unbounded axis row", () => {
// Engine says: mana family unbounded. The axis list is empty.
useGameStore.setState({ gameState: withPool({
unbounded_resources: [],
unbounded_families: [{ player: 0, family: "mana", state: { type: "Unscheduled" } }],
})});
render(<PlayerHud />);
expect(within(manaPoolRow()).getByLabelText("Unbounded mana (∞)")).toBeInTheDocument();
});That is your three requested properties exactly: nonempty mana pool ( It also ships as a matched pair with the negative-direction sibling at 🟡 Minor — "ONLY Committed family" claim — CONFIRMED, and worse than reportedYou found one counterexample. A census found two:
Fixed, and deliberately not with your proposed wording, which names only I also swept the PR's six touched files for sibling uniqueness claims, since one false "only" suggests others. Two candidates surfaced ( 🔵 Trivial — layering — observation correct, remedy measured ineffectiveThe mutual naming is real: But moving the mapping doesn't achieve the one-way dependency you describe — it relocates the coupling. Declining with that reasoning rather than silently. Fixes pushed as |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline client/src/components/hud/__tests__/PlayerHud.designations.test.tsx --items all
ast-grep outline crates/engine/src/game/engine_resolution_choices.rs --items all --match 'materialization_certainty|possible_hold'
ast-grep outline crates/engine/src/game/derived_views.rs --items all --match 'CollapseCertainty|materialization_certainty'
printf '\n--- Mana family-channel tests ---\n'
sed -n '260,320p' client/src/components/hud/__tests__/PlayerHud.designations.test.tsx
printf '\n--- Corrected integration-test comments ---\n'
sed -n '820,850p' crates/engine/tests/integration/combo_infinite_pile.rs
sed -n '1200,1230p' crates/engine/tests/integration/loop_shortcut_mana_engine.rs
printf '\n--- Cross-module certainty references ---\n'
rg -n -C 3 '\b(materialization_certainty|CollapseCertainty)\b' \
crates/engine/src/game/engine_resolution_choices.rs \
crates/engine/src/game/derived_views.rs
printf '\n--- ManaPoolSummary source dependency ---\n'
ast-grep outline client/src/components/hud/ManaPoolSummary.tsx --items all
rg -n -C 4 '\b(unboundedFamilies|unbounded_resources|unbounded_families)\b' \
client/src/components/hud/ManaPoolSummary.tsx \
client/src/components/hud/__tests__/PlayerHud.designations.test.tsxLength of output: 31372
The Major finding was incorrect. The Minor finding is addressed. The revised Tokens comment no longer implies that it is the only committed family instance. The Life The Trivial layering observation remains factual. However, moving only 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
🤖 AI text below 🤖 @matthewevans — CI went red on my previous push and is fixed in What broke. That script is well built and I'd rather say so than quietly patch past it: the hardcoded constant forces the bump to be acknowledged in a third place and cross-checks Rust against TypeScript, and it pattern-asserts all four floor expressions — lobby Why my local run missed it. My battery ran Why my census missed the file. It searched Verified with a must-fail control, since a tripwire that can't fail proves nothing:
Recorded as scope amendment r4 (30 → 31 paths), framed as r3 finished rather than new work, with the two-key census miss written out so the mechanism is on record and not just the instance. Nothing else changed: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
-
[HIGH / rules correctness] An accepted loop shortcut is still deferred instead of being taken at its proposed ending point. CR 732.2c says: “Once the last player has either accepted or shortened the shortcut proposal, the shortcut is taken. The game advances to the last proposed ending point, with all game choices contained in the shortcut proposal having been taken.” At crates/engine/src/game/engine.rs:4999-5026, acceptance only registers PersistentAxisMaterialization; crates/engine/src/game/engine_resolution_choices.rs:2461+ applies it later through PayableResource::LoopCollapse. The PR explicitly characterizes that accepted-result deferral as an unlicensed engine deviation at crates/engine/src/types/game_state.rs:19915-19921. This changes the game state after acceptance and can defer/decline proposed growth, so it needs a rules-correct redesign rather than a maintainer fixup.
-
[LOW / review evidence] The only coverage-parse-diff sticky is generated for ae32aa9, not this reviewed head (684335b). Please publish a current-head parse-diff artifact before the next approval request.
|
🤖 AI text below 🤖 @matthewevans — LOW answered, and while answering it I found that a number in my own PR body is wrong. Correcting that first, because it is the more important half. The
|
|
Maintainer correction to the current changes-requested review: I retract only its former LOW review-evidence finding. The current-head parse-diff now satisfies that evidence requirement: it is generated for The HIGH accepted-shortcut / CR 732.2c finding remains the sole blocking condition. |
|
🤖 AI text below 🤖 @matthewevans — thanks for the LOW retraction; noted and not re-litigated. This addresses the HIGH, the sole blocking condition, and resolves the escalation I announced in 5206999365. The north star: a shortcut system that is rules-correct and fun to play. This PR states and enforces the rules-correct design; where the current realization falls short of it, the code now carries a deficiency note pointing at a queued follow-up to make it true all the way down. The repo owner's decision is not to redesign, because the design is already the rules-correct one — and rather than argue that in a thread, the argument is now in the code, annotated at the site that enforces each clause. Comment-only apart from two assertion-message strings ( If any of this is wrong, the annotations are where to push back — that's why they're there rather than here. The invariantELISION ≡ PERFORMANCE. The engine can never advance to a state that performing the proposal's choices would not produce. CR 732.2c defines the advance as reaching the ending point "with all game choices contained in the shortcut proposal having been taken" — so the end state must be the state those choices produce. There are exactly three materialization routes and each preserves that identity:
Route (c) is the half your finding indicts ("defer/decline"), and it is the route that enforces CR 732.2c. Once an observer appears, both alternatives break the identity: a batched advance reaches a state the choices would not produce, and replaying an observer-laden sequence executes a proposal nobody accepted. Decline-to-manual is the only CR 732-faithful option left. The gate isn't the deviation — the two alternatives it forecloses are. More generally: declining an elision can't be rules-incorrect, because the elision is what needs a license; its absence never does. Both the decline and the Measured, not asserted: Clause mapping
The honest boundary, stated because it is the argumentThis engine does not follow the CR 732.2 procedure verbatim. It is a CR 732.1a-licensed variant — "as long as each player in the game understands the intent of each other player, any shortcut system they use is acceptable" — and the published badge/window semantics are what make that mutual understanding hold by construction. Reading the window as illegal requires CR 732.2 to be the exclusive procedure, which CR 732.1a's plain text contradicts. Attacks we ran against our own reading, and why each failed
Where to read it
Where the realization doesn't reach the design yetNamed as notes, not defended, and not fixed here — closing them would expand this PR's scope:
Each note names the queued follow-up ("Shortcut-system rules-correctness completion — true all the way down") so the reader can find it rather than rediscover the gap. And |
…g, with its wire coverage `DerivedViews` regains `scheduled_collapse: Vec<UnboundedResourceView>` — the `(engine-attributed player, axis)` pairs whose ∞ growth has an accepted but not-yet-applied finite collapse waiting at the next step/phase end. phase-rs#7002 removed it as an unconsumed contract; this reintroduces it with the consumer the maintainer's first option asked for (C2 lands the badge). It TAGS THE PENDING COLLAPSE — the accepted stash — and is deliberately NOT a per-row annotation of `unbounded_resources`. Measured: `derive_views` drops an object-growth row whose entire registered backing set has left the battlefield (`object_growth_backing`) while the stash, its CR 732.2c bound and the store mark all survive (`combo_infinite_pile::object_growth_infinity_row_dies_with_ its_last_pile_member` asserts exactly that in its subject arm), so the boundary still cashes that axis out. A tagged axis with no row is therefore CORRECT; gating the tag on the same backing check would under-report a live pending collapse. Consumers iterate ROWS, so an orphan tag renders nothing. `Mana(_)` axes are omitted by documented scope limit: that pool is already spendable and ends at the CR 500.5 mana drain, not via a materialization. CR provenance, unchanged from the block it sits under: the accept→boundary window is an engine deviation licensed by no CR; CR 732.2c governs the CEILING only; CR 500.5 names the timing landmark and governs the mana-pool drain there. Coverage (all four extend existing green tests, so each ships with a pre-existing positive control in the same fn): - R1 after deserialize + populated key emitted, per viewer - R2 across `derive_filtered_views` per viewer, with the ∞-row pin asserted BEFORE the tag axis so every probe arm reaches it - R3 an engine-emitted EMPTY view omits the field and deserializes to empty, paired with its pre-clear positive control - R4 the `Mana(_)` scope limit is falsifiable, not dead code Revert probes (RUN, five parts each): DROP the tag loop => R2/filtered, R3/pre-clear and R4/keep red on `got []` (0 passed / 3 failed) with both pins green; TRIVIALIZE it to `unbounded_resources` => only R3/post-clear reds (2 passed / 1 failed); delete the `Mana(_)` continue => only R4/omit reds; drop the key from ONE golden list => only that golden's drift assert fires; delete the loop AND regenerate => the TS seam row reds (and, without the regeneration, only the Rust drift assert fires — both halves recorded). Both wire goldens regenerated through the emitters, byte-reproducible across independent test processes; neither gained or lost any other key. Assisted-by: ClaudeCode:claude-opus-5
The consumer that justifies C1's `scheduled_collapse` reintroduction. A ∞ HUD
badge whose axis the engine has tagged now reads `∞→N` and carries a tooltip
saying a collapse is pending and the player will choose how many; every other
badge is byte-identical to today's.
`usePlayerDesignations` gains `scheduledCollapse`, derived exactly like
`unboundedResources` — same `forPlayer` filter, same shared empty array. The
hook joins nothing and infers nothing. `hasAny` deliberately ignores it: the
tag only decorates a badge that some ∞ ROW already renders.
The dedup + join moves into one exported pure function, `unboundedFamilyViews`
in `HudBadges.tsx`, beside `familyOf`/`UNBOUNDED_FAMILY` which already own
family mapping. That is a net deletion at the four HUD sites that duplicated
`[...new Set(rows.map(familyOf))]` inline (PlayerHud, OpponentHud ×2,
OpponentSeatHeader), and it makes the join unit-testable with no hook rig. It
lives here, not in the hook, so no hook imports a component module.
It is driven by ROWS, never by the tag. The engine's tag can name an axis whose
∞ row `derive_views` has already dropped — that loop's board backing left play
while the accepted collapse survives, because the boundary still cashes it out.
Such an orphan tag correctly renders nothing.
The join keys on `JSON.stringify(axis)`, not `===`: `ResourceAxis` is
externally tagged, so unit variants arrive as bare strings while data variants
arrive as objects (`{"Counter":["Other","Other"]}` in the committed counter
golden) and reference equality never matches two structurally equal objects.
`axisTag` is not reused as the key — it returns only the variant tag, so two
different `Counter(..)` axes would collide and the join would over-report.
`UnboundedBadge`'s `scheduled` prop is optional and defaults to false, so
`LoopShortcutModal`'s `FamilyBadges` compiles unchanged — and `false` is right
there: that modal renders at OFFER time, before anyone has accepted.
One new i18n key, `badges.unboundedScheduledTooltip`, translated across all 7
locales in this commit so per-locale key parity holds at the boundary.
New `UnboundedBadge.test.tsx`, 7 rows, each labelled by its data source:
U1/U2 are GOLDEN-DRIVEN (the regenerated `unbounded-token-wire.json`, tag read
out of the file); U3/U3b/U5/U6 are composed against the pure function; U4 is
the only render-level row exercising the hook's per-player filter. U5's
rows-empty half is paired in-row with a pinned positive, because a constant
`return []` satisfies the empty half alone.
Revert probes (RUN, over a 44-test blast radius): delete the tooltip ternary =>
only U1 reds; hardcode `scheduled: true` => U2 reds (+3 collateral, all
accessible-name assertions the mutation genuinely breaks); join on
`scheduled.length > 0` => only U3; reference `===` => only U3b; drop the tag's
`forPlayer` => only U4; last-wins fold => only U6; constant `return []` => U5
reds on its positive half (+6 collateral).
Assisted-by: ClaudeCode:claude-opus-5
… the tag falsified Four review findings. One is a latent correctness bug; three are claims this branch's own tag loop made false. `unboundedFamilyViews` keyed its scheduled-set on the axis alone while three separate docs — the Rust view doc, the tag-loop comment and the TS wire doc — all specified `(player, axis)` equality. Given unfiltered inputs, a tag for seat 1 marked seat 0's badge as scheduled. It is not reachable through any caller today: all four pass both arrays out of one `usePlayerDesignations(playerId)`, whose `forPlayer` has already filtered on the seat, which is exactly why nothing caught it. Fixed in the code rather than by weakening the three docs, because the next caller that forgets to pre-filter should get the right answer instead of a silent cross-seat badge. The existing U3 row is named "joins by (player, axis)" but holds every row at the default seat, so it passed identically under both keyings — it never exercised the half of its own name that was broken. U3c does, as a matched pair: same axis different seats must not schedule, same axis same seat must. Reverting the key reds U3c alone, 1 of 8. The three doc corrections are the same defect this branch keeps finding. Adding the tag loop gave `scheduled_collapse_axes` a second production reader, which falsified its own rustdoc twice over — "ONE production caller" and "deliberately not read by `game::derived_views`". The previous commit corrected the mirror sentence in `derived_views` and left the callee's doc standing, which is the copy a future caller reads first. `collapsed_counter_axis`' reachability list gains the tag projection, with its fail-open polarity restated: the miss now also yields an orphan tag, which renders plain ∞ rather than ∞→N — nothing hidden, one affordance not offered. And the section header still counted three unconditional ∞ channels with four sitting below it; the tag is unconditional too, but it is not a surface. Assisted-by: ClaudeCode:claude-opus-5
… is still known Review found the frontend's `(player, axis)` join between `unbounded_resources` and `scheduled_collapse` was not merely mis-layered but wrong. Both channels are keyed by `attribution_player`, so a victim-attributed axis (`Life(p)` and friends) names its victim rather than the loop that produced it. Two controllers draining one victim emit a row from one and a tag from the other under the identical key `(victim, Life(victim))`, and the join marked the wrong controller's row scheduled. The fix is ordering, not layering: the answer is not recoverable after attribution rewrites the key. `UnboundedResourceView::scheduled` is now resolved per controller inside the row loop, before `attribution_player` runs, reusing `scheduled_collapse_axes` as the single authority. The frontend join is deleted. `scheduled_collapse` stays, and stays the authority for the CR 732.2c contract — the accepted bound survives the loss of the growth's board members, so an orphan tag is a rules-mandated state. The row flag is its display shadow; the wire-seam test asserts the two agree. Witnessed by `two_controllers_draining_one_victim_do_not_cross_schedule`: two rows sharing one wire key that must still disagree about `scheduled`, asserted as a multiset so it reds from both directions. Mutating the computation back to the union-join reds exactly that test with zero collateral across 4560 integration tests — the suite was blind to this before. Also from review: - the orphan-tag doctrine's named witness asserted nothing about the tag; it now asserts both halves (tag present, row absent) on real engine state - the over-scoped "object-growth row" claim narrowed to the token axis in all five copies, with the counter route stated at its actual precondition - CR 400.7 corrected to CR 111.7 / CR 704.5d for cease-to-exist (400.7 governs zone change, and is kept only for that disjunct) - the scheduled tooltip dropped its false agency claim in all 7 locales: the badge renders on opponent HUDs and on victim-attributed seats, while the controller is prompted - `∞→N` localized; the trigger-family count corrected to six Assisted-by: ClaudeCode:claude-opus-5
An independent review found the `Mana(_)` scope limit lived in the `scheduled_collapse` tag loop only. The row loop had no such guard, so on a `DriveSequence` accept — whose `collapsed_axes` is the loop's whole `proposal.unbounded` set, mana included — the mana row shipped `scheduled: true` with no tag naming it. The HUD folds that flag into the "mana" family and renders `∞→N` with a "collapse pending; a finite amount will be chosen" tooltip, beside a pool `refill_infinite_mana` is still topping up and beside `ManaPoolSummary`'s plain `∞` for the same pool in the same frame. `scheduled_display_axes` now owns the limit and both loops read it. Writing the guard twice is what produced the defect, so the fix is one authority rather than a second copy. The tag channel is also de-duplicated by `(player, axis)`: attribution collapses victim-attributed axes onto the victim, so two controllers each holding an accepted `Life(victim)` collapse pushed that pair twice, contradicting the field's own "set of pairs" doc. The exclusion's stated reason was false in both places it was written. "Not cashed out by a materialization" is wrong — `clear_collapsed_materializations` does drop the `Mana(_)` axis when a `DriveSequence` collapse applies, and its own doc says so. The limit is right; it rests on the accepted count bounding nothing the player can spend. A true rule with a false reason is what let the guard live in one loop instead of one function. Pinned two-sided by R4/agree in the mana-engine test: the mana row must not be flagged, and the `Life(P0)` row of the same stash and the same `derive_views` call must be. Mutation p6 reproduces the shipped asymmetry and reds R4/agree alone — lib 18527/0 and vitest 288/0 stay green under it, which is the measurement behind "the suite could not see this". Also from the same review: drop the unused per-seat copy of the tag channel from `usePlayerDesignations` (the engine answers `scheduled` on the row, so a display-layer copy had no consumer); state which direction of the row/tag agreement is general and why; replace an unwitnessed "has been measured" claim with the structural argument; and revert 42 lines of unrelated formatter reflow on `manaSourceSelection` across six locales. Assisted-by: ClaudeCode:claude-opus-5
Second independent review at 343488aa5. The mechanism was found correct — row->tag is total and structural, the BTreeSet dedup cannot drop a distinct tag, R4/agree is non-vacuous, all twelve touched CR numbers verified. What it found instead was five claims that no longer match the code, four of them introduced by this PR. The header block above the ∞ projections still said the surface loops "none consults `GameState::scheduled_collapse_axes`" and "no surface reads it". The row loop reads it, through `scheduled_display_axes`, since the scheduled flag moved into the engine. That paragraph already warns that "a stale claim introduced by the commit that exists to fix stale claims is the one defect this change cannot afford" — and had gone stale twice by the same mechanism. It is now phrased as the invariant that is actually true and durable (no surface is FILTERED by the schedule; the schedule is read to annotate) rather than as a census of who reads what. `game_state.rs`' mirror doc had the matching staleness in the other direction, one commit after scolding an earlier commit for exactly that. It now names the calling FUNCTION rather than the loop, so a third consumer inside `derive_views` cannot make it stale a third time. Bound three claims to what the evidence supports. "A downstream join is measurably wrong" overstated the stakes: no consumer has row granularity, and a key collision implies the same family, so the mis-marked row renders identically today. The divergence is real in the data and latent in the display; the layering argument is the sound one and is what now leads. `U5/rows-drive` claimed to pin "an orphan tag renders nothing", which no input to a function taking no tag argument can distinguish. The `Mana(_)` rationale was stated unconditionally when it holds inside the accept->boundary window — the only window the badge renders in; across the boundary a `DriveSequence` replay really is bounded by N. Verified in `turns.rs`, whose own comments place the CR 500.5 mana drain as the first pass and the materialization prompt as the second, with `debug_infinite_mana` seats excluded from the drain. Assisted-by: ClaudeCode:claude-opus-5
…ne at the store A second independent review found `DerivedViews::scheduled_collapse` re-landing with zero production consumers. Census over crates/ and client/src excluding tests: every occurrence was the field, its emission loop, or a doc comment. The engine's boundary reads `pending_unbounded_materialization` directly; the badge reads `UnboundedResourceView::scheduled`. That is the exact condition commit 47151c7 — a review response inside phase-rs#7002 — deleted this field under, and it stated the condition for its return: "it belongs in the PR that builds that UI, arriving with the code that reads it rather than one PR ahead of it." This PR failed that condition because of its own earlier fix: moving the scheduled answer onto the row is what orphaned the channel, and three comments here had grown to instruct readers not to consume it. A wire contract nobody reads is not a contract. Removing it does not lose the CR 732.2c doctrine — it relocates the witness to the authority that actually holds the contract. `object_growth_infinity_row_dies_with_its_last_pile_member` previously asserted that a dropped row keeps its TAG, which only ever proved a projection still projected. It now asserts, on the same post-loss state, that the surviving stash still SCHEDULES the axis, and then drives that state to the real CR 500.5 boundary and collapses it through the public `apply()` path: the growth the table unanimously accepted still lands after its ∞ row is gone. That is the property the doctrine is about, and no projection could have shown it. Two new mutations exist because of the relocation — a stash that stops scheduling its axis, and a boundary that resolves but mints nothing — and the second was unexpressible against the old assertion. R1/R2/R3 in `loop_shortcut.rs` and R4 in `loop_shortcut_mana_engine.rs` re-anchor on the row flag, keeping what each was for: R1 that a SET flag survives the wire (it is `skip_serializing_if`-omitted, so emission is the half that fails silently), R2 that the viewer-filtered broadcast path preserves it, R3 the omit-when-false pair with its pre-clear positive control, R4 the `Mana(_)` scope limit with its `Life(P0)` positive. The lib test cross-checks the flag against `pending_unbounded_materialization` rather than against a second wire channel — a display shadow should be checked against the authority, not another shadow. Also from that review: drop two assertions strictly subsumed by exact-set equality earlier in the same loop body (an assertion no mutation can red is the test analogue of a contract nobody reads); disclose that `Counters`/`Life` axes can be flagged and then decline to collapse when an observer appears in the accept→boundary window, which the `Mana(_)` scope limit does not cover and no flag-time value can fix; and cite CR 119.3 + CR 704.5a consistently for life-axis attribution instead of splitting the pair across sites. Goldens regenerate: the delta against phase-rs#7002 is now exactly the two `scheduled` flags. Wire-seam coverage restated on what these fixtures can actually witness — each holds one scheduled row, so omit-when-false is NOT shown there and now says so, naming R3 and U3b as where it is. Assisted-by: ClaudeCode:claude-opus-5
…ee claims Confirmation review of the corrected head. No behavioural defect; the findings are one misplaced doc block and six claims that overstate or contradict. The new `two_controllers_draining_one_victim_do_not_cross_schedule` had been inserted BETWEEN `real_certificate_axes_project_to_victim_hud`'s doc header and its REVERT-PROBE tail, so the new test wore a header describing an opponent-burn certificate derived via `ResourceVector::unbounded_axes_for` — which it does not do — while the test that does lost its description. Anyone grepping for that witness landed on the wrong function. Header moved back. The anti-staleness paragraph said "three times running" and then "That makes twice", and gave its rationale at both ends. It now enumerates the three falsifications it survived, once. Three claims bound to what their evidence supports. The wire-seam test claimed to pin omit-when-false eighteen lines above its own note that omit-when-false is NOT witnessed there. The family fold's reachability argument proved the wrong side — several axes from one accept are several SCHEDULED axes, under which no over-report exists; the over-report needs a marked-but-unscheduled sibling, so the two paths that would produce one are named as the open question they are, and the tests are described as pinning the fold's behaviour rather than the pair's reachability. `U5` cited a test that asserts a row is DROPPED as its witness for rows-EMPTY; rows-empty is the dominant case (`PlayerHud` calls the fold unconditionally) and needs no reachability argument at all. Disclose the same-frame surface asymmetry: only the HUD row carries the flag, so one loop can show `∞→N` on its badge and plain `∞` on its own token group and counter pill in the same frame. Unlike the mana case this is not a false promise, but the neighbouring comment already reasons about exactly this class of mismatch and did not mention it. Also note on the TS mirror that `scheduled` never guarantees the growth lands. Replace a dangling `§7 RP-E1/RP-E2` citation — the artifact is untracked, so a reader of merged code could not resolve it — with the argument it referred to. Assisted-by: ClaudeCode:claude-opus-5
…ng a bound the boundary can refuse
The `∞→N` badge asserted that a scheduled loop collapse would land. Two of the
boundary's four materialization arms can finish WITHOUT applying: `Tokens` can
park on a copy-token replacement choice, and `Counters`/`Life` can be declined
when an observer of the growing class appears in the accept→boundary window.
In both cases the axis stays unbounded and no finite amount is ever chosen, so
the badge was making a promise the engine does not keep.
Derive the promise from the boundary's own non-push exits instead of from its
decline policy. `possible_hold` is a measured census of the three ways the apply
loop finishes an item without pushing it; `materialization_certainty` maps that
to `Committed` / `Conditional`. Only `DriveSequence` — which holds a
`SimulationProbeGuard`, cannot park, and commits its successful prefix — has no
non-push exit, so the affirmative `∞→N` is now reserved for it alone. Batched
families render `∞→?`.
The frontend was also folding rows into families with a last-wins OR, which let
one seat's accepted stash mark another seat's row. That fold moves into the
engine as a commutative join over `(player, family)`, computed on the producing
controller's key before attribution rewrites it. Collisions yield `Mixed`, which
renders a bare `∞` — the honest answer the OR-fold got wrong.
- `BoundaryHold { ObservedGrowth, CopyTokenPause }` carries the reason as a
value; four `collapsed.push` sites collapse to one, so "reaches the push" is a
one-screen property and `possible_hold` is exactly the set that can skip it.
- `UnboundedResourceView::scheduled` is replaced by `DerivedViews::unbounded_families`
(`UnboundedFamily` x `FamilyCollapseState`), with `rename_all` making the wire
strings the client's literals — one type, no mirror map.
- The accept→boundary window is a documented engine deviation that no CR
licenses; annotations name CR 732.2c as the rule deviated FROM rather than as
authority, cite CR 614.1 + CR 614.1a for why an "instead" replacement still
applies to a source-less mint, scope CR 616.1 to the multi-candidate ingress,
and give ten projection symbols an explicit no-CR verdict.
Both maintainer findings were disclosed in comments by the original author and
rejected as remedies; this replaces the disclosure with a typed engine channel.
Assisted-by: ClaudeCode:claude-opus-5
…r the objection this branch deleted Review of the previous commit found six defects, five of them in what the change says about itself rather than in what it does. The engine delta here is documentation plus a `#[cfg(test)]` guard; the only production behavior change is one line of TSX. The load-bearing one: `possible_hold` was compiler-enforced on the `PersistentAxisMaterialization` variant axis but not on the apply loop's EXIT axis, and the census test asserted it against a hardcoded vector — a transcription check of `possible_hold` against itself. Adding an item-level non-push exit reachable by `DriveSequence`, or dropping the Tokens-last stash ordering, left `DriveSequence => None => Committed` intact and silently restored the false `∞→N`. `boundary_apply_loop_region` now slices the loop out of this file's own source between the sort anchor and the single push, and asserts the control-flow census over it, so the guard reds when the loop changes. - Citations that this change's own +293 lines invalidated are re-anchored to heading text rather than to new line numbers, and the policy that licensed them — an exemption for "files the change edits, because the edit re-derives them" — is rewritten, since that exemption was the defect rather than an oversight. Closed sweep: 9 citations into the six edited files, 7 caused here and repaired, 2 pre-existing in out-of-scope files and queued. - The SAME-FRAME ASYMMETRY disclosure the previous commit deleted is restored onto `UnboundedFamilyView` in the new vocabulary. The phenomenon is unchanged and live — a loop can show `∞→N` on the badge and plain `∞` on its own pill in one frame, because those channels are `ObjectId`-keyed. A reasoned objection gets answered where it stood, never dropped. - `ManaPoolSummary` reads the engine's `unbounded_families` instead of deriving the family client-side, which is what makes the sentence added by the previous commit true; that sentence also narrows to name `LoopShortcutModal`'s offer-time case, where no family channel exists yet. - The `BoundaryHold` census prose states its counting unit and sums. `CollapseCertainty::weaker` and `FamilyCollapseState::merge` get the explicit no-CR verdict their siblings carry. Assisted-by: ClaudeCode:claude-opus-5
Round-3 fix partition for the scheduled-collapse badge. Adds no production behavior: 62 added lines in the production region of engine_resolution_choices.rs are comments, 0 are code; the 15 added in derived_views.rs are likewise all comments. The 171 remaining additions are inside `mod tests`. F3 — the hold census stopped being a transcription check. It now reads the apply loop's own body via `include_str!` and counts real non-push exits, so it audits the code rather than restating `possible_hold` against itself. Both mutants are two-sided and were measured, not predicted: inserting a `break;` moves the exit tuple (2,1,1,0) -> (2,1,0,0); adding a second `collapsed.push(item.clone())` moves the push count 2 -> 1. Each reverted with empty porcelain. F1 — the "cite by symbol, not by line" rule was prose only a reviewer could apply, which is how it shipped false twice. It is now `subsystem_citations_are_symbol_anchored`, a test that walks the crate, enrolls files by an opt-in marker comment, and reds on any surviving line anchor in an enrolled file's production region. Enrollment is discovered, not listed, so the covered set cannot drift out of sync with the files; `ENROLLED_FLOOR` reds a broken walk or a renamed marker rather than letting an empty population pass vacuously. Every line-anchored citation in the six enrolled files is converted to a symbol or greppable heading. Four were already stale at the base commit, one by 5295 lines, and one - engine.rs:1871-1873 - was simultaneously converted in one in-scope file and left in another, so the rule was false inside its own scope. Repo-wide enforcement was measured and deliberately not attempted: crates/engine/src carries 216 such anchors across 61 files, ~20x this change, and that burndown is backlogged rather than smuggled in here. The guard's doc claims exactly what it enforces and points at the residual hole it does not claim: a file that adds citations without adopting the marker is still uncaught. Assisted-by: ClaudeCode:claude-opus-5
The `unbounded_families` channel is `#[serde(default)]`, and this crate's own rule says an optional serde-default field needs no protocol bump. That rule is about wire PARSEABILITY. The hazard here is capability: this PR deleted the client's row-flag OR-fold fallback, so a v23 server that omits the field leaves a v24 client rendering zero infinity badges — silently, with no parse error to catch it. The handshake is the only place that pairing can be refused. So the rule itself is amended with that caveat. Bumping the number without qualifying the rule would leave the next reader talked out of the same bump. Both floors are strict by construction (`MIN_SUPPORTED_PROTOCOL == PROTOCOL_VERSION` server-side, `MIN_SUPPORTED_SERVER_PROTOCOL == PROTOCOL_VERSION` client-side), so the strict full-game minimum needs no edit. Three literal contract assertions break on the bump and are updated: `lobby-broker:409`, `lobby-broker:410`, and `server-core:2261` — the last inside a test NAMED `protocol_version_is_23`, so its name lied too. A first census missed two of these by keying on the wrong separator (a comma, not `=`) and the wrong number. All three pass at any value, so none of them detects the regression the bump exists to prevent. `full_game_floor_is_current_only_not_a_rollout_window` is the assertion with teeth: relaxing the floor to `PROTOCOL_VERSION - 1` reds it while `protocol_version_is_24` stays green (measured, three arms). Also corrects a false uniqueness claim in an assertion message: the real-dump Committed family is not the suite's ONLY one — `combo_infinite_pile` and `loop_shortcut_mana_engine` carry two more. The message now states what makes this witness distinct and names both siblings, so neither reads as redundant. Assisted-by: ClaudeCode:claude-opus-5
`scripts/check-protocol-version.mjs` pins `EXPECTED_PROTOCOL_VERSION` as a deliberate hardcoded constant, so that bumping the wire version forces an explicit acknowledgement in a third place and cross-checks Rust against TypeScript. The previous commit bumped both sources and left the tripwire at 23, so it did exactly what it exists to do and failed CI. Caught by CI, not by the local battery: CI's `Type check` step runs `pnpm run type-check` = `protocol:check && tsc -b --noEmit`, while the battery ran only `tsc -b --noEmit --force`. The battery's command set predates this change having any protocol surface, so its green did not cover the one check built to catch an inconsistent bump. The census that set the previous scope missed this file on two keys at once: directory (`scripts/`, not `crates`/`client/src`) and extension (`.mjs`, not `.rs`/`.ts`/`.tsx`). The right key was never directory-and-extension but "who consumes the protocol version", so the sweep is now a repo-wide `git grep` with no path or extension filter. Verified with a must-fail control: as shipped exit 0; reverting the client constant to 23 reports `Protocol version mismatch: Rust=24, client=23` and exits 1; restored exits 0. The four floor pattern-assertions the script also enforces are unchanged and still hold. Assisted-by: ClaudeCode:claude-opus-5
…n the code The subsystem carried a resident doctrine that the accept->boundary window was an engine deviation "that no CR licenses". That doctrine was wrong, and it was quoted back at the PR as the reason the design needed a rules-correct redesign. This replaces it with the reading the implementation actually satisfies, annotated at the site that enforces each clause so a reviewer can follow it with the code. The invariant, stated once in full at `scheduled_collapse_axes` and referenced by symbol elsewhere: ELISION == PERFORMANCE. Three materialization routes, and each preserves it -- unobserved batch (batch(N) == perform-each(N) by the firewall's own precondition), observed-at-accept DriveSequence (performs the iterations), and became-observed-in-window decline (manual play, the player performs). CR 732.2c defines the advance as reaching the ending point "with all game choices contained in the shortcut proposal having been taken", so the end state must be the state those choices produce. The boundary decline gate therefore ENFORCES CR 732.2c: the two alternatives it forecloses are what would break the identity. Positions, each anchored on a clause rather than an assertion: - timing: the loop ends by proceeding to the phase change with the iterations elided per CR 732.1b; the opposing player then has priority, and that window is the CR 732.2a-legal ending point (a step/phase end is not one -- CR 500.5 is a turn-based action and CR 117.3a gives priority at the next step's beginning). - no conditional actions: pins fix every free choice before the offer and TargetSchedule makes react-to-outcome unrepresentable, with the static coin/die/random rejection and elimination_bounds as companions. - the count: CR 732.2a's "a loop that repeats a specified number of times" is the proposer's to specify; the collapse prompt is that specification, bounded above by the accepted count, and fewer is CR 732.2b/2c shortening realized. - CR 732.1a variant: the engine is the table's shortcut system, which is what covers differences in the system's form while the invariant holds. Lemmas annotated where they are checkable: L1 optionality (is_voluntarily_repeatable is true on an exhaustive match over all three LoopAction variants), L2 unconditionality by construction, L3 prefix consent for the [0, N] range, L5 third-party invariance of the unobserved batch route. Where the current realization under-delivers that design it now carries a deficiency NOTE rather than a defense: ShortcutResponse::Shorten is realized as decline-to-manual instead of taking the shortcut to the responder's named ending point (CR 732.2b), with the same gap noted at its handler and at DeclareShortcut's template field. Each note names the USER-AUTHORIZED follow-up "Shortcut-system rules-correctness completion -- true all the way down" in .deferred-backlog.md. Closing them is out of scope here; the aim is a shortcut system that is rules-correct and fun to play, stated as the design now and made true all the way down next. The citation gate's step (0) inverted: it previously said an existing "no CR licenses" admission ENDED the search for a rule, which was self-sealing -- the admissions predated the reading and the gate stopped anyone checking them. It now requires a license claim to name the invariant it preserves. Swept by mechanism across the repo, including three frontend files a "no CR licenses" key could not match. Comment-only apart from two assertion-message strings; no logic changes. Assisted-by: ClaudeCode:claude-opus-5
4685977 to
2b20aa7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/tests/integration/combo_infinite_pile.rs (2)
1742-1791: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftExercise the replacement-aware zone-change pipeline.
The control and subject arms call
zones::move_to_zonedirectly. That bypassesProposedEvent::ZoneChange, so replacement effects and related event handling are not tested. The test can pass while the production zone-change path behaves differently. Drive both departures through the public replacement-aware path, then keep the schedule and boundary assertions.As per path instructions, zone changes must route through the replacement-aware pipeline (
ProposedEvent::ZoneChange), not a directzones::move_to_zone.🤖 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/combo_infinite_pile.rs` around lines 1742 - 1791, Update the setup for both control and subject departures to route zone changes through ProposedEvent::ZoneChange and the public replacement-aware event pipeline instead of calling zones::move_to_zone directly. Preserve the existing scenario, schedule assertion, boundary prompt, and cash-out assertions after the replacement-aware moves.Source: Path instructions
826-843: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDrive the certainty check through the observed accept route.
This test grafts
PersistentAxisMaterialization::DriveSequenceintopending_unbounded_materializationand has no production-path reach guard formaterialize_object_growth_shortcut. If the accept routing stops producing that stash, this assertion can still pass for the wrong reason. Add the observed-loop accept case withDriveSequence, or keep this branch as a state-shape probe and move the route coverage out.🤖 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/combo_infinite_pile.rs` around lines 826 - 843, Ensure the certainty assertion for DriveSequence is reached through the observed-loop accept route rather than only by directly grafting it into pending_unbounded_materialization. Add an acceptance-path case that produces PersistentAxisMaterialization::DriveSequence and verifies the resulting tokens family state, or move this assertion to a state-shape-only test and add separate route coverage for materialize_object_growth_shortcut.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/analysis/decision_template.rs`:
- Around line 701-710: The comments around predictability_gate overstate that
the function guarantees total coverage, although it only validates
caller-supplied required_slots. Revise the claim to explicitly limit coverage to
the provided slot list, or update predictability_gate and its declare path so
runtime validation proves every free choice is covered.
In `@crates/engine/src/game/engine.rs`:
- Around line 4742-4754: Update the explanatory comment near the
unconditionality gate to distinguish FamilyCollapseState::Unscheduled from
FamilyCollapseState::Scheduled(Conditional): Unscheduled means no staged
collapse exists, while Scheduled(Conditional) indicates an accepted but
declinable or pausable shortcut remains staged. Do not describe every
non-Committed variant as the absence of a proposal; preserve this wire contract.
- Around line 5494-5498: Update the Shorten flow in
crates/engine/src/game/engine.rs at lines 4664-4699 and 5494-5498, and the
corresponding turn handling in crates/engine/src/game/turns.rs at lines 553-565,
so accepted shortcut choices advance through the named at_iteration cycles
before priority is handed off. Use one final shortcut ending point, and bound
the collapse prompt by the responder’s accepted prefix rather than the original
max; ensure all three sites consistently implement this behavior.
In `@crates/engine/src/types/actions.rs`:
- Around line 887-893: The documentation for DeclareShortcut::template is
outdated: the engine accepts Some(DecisionTemplate) when its owner matches
offer.proposer and its pinned declarations are valid. Update the DeclareShortcut
documentation to describe this supported template-pin behavior and remove the
claim that template must remain None; preserve the existing validation rules.
---
Outside diff comments:
In `@crates/engine/tests/integration/combo_infinite_pile.rs`:
- Around line 1742-1791: Update the setup for both control and subject
departures to route zone changes through ProposedEvent::ZoneChange and the
public replacement-aware event pipeline instead of calling zones::move_to_zone
directly. Preserve the existing scenario, schedule assertion, boundary prompt,
and cash-out assertions after the replacement-aware moves.
- Around line 826-843: Ensure the certainty assertion for DriveSequence is
reached through the observed-loop accept route rather than only by directly
grafting it into pending_unbounded_materialization. Add an acceptance-path case
that produces PersistentAxisMaterialization::DriveSequence and verifies the
resulting tokens family state, or move this assertion to a state-shape-only test
and add separate route coverage for materialize_object_growth_shortcut.
🪄 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: af7d83fc-9899-4a7c-9123-d8af3c96ecb4
📒 Files selected for processing (16)
client/src/adapter/types.tsclient/src/components/hud/HudBadges.tsxclient/src/viewmodel/__tests__/unboundedWireSeam.test.tscrates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/turns.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rsscripts/check-protocol-version.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- client/src/viewmodel/tests/unboundedWireSeam.test.ts
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs
- client/src/adapter/types.ts
- crates/engine/src/game/derived_views.rs
- client/src/components/hud/HudBadges.tsx
- crates/engine/src/game/engine_resolution_choices.rs
- crates/engine/tests/integration/loop_shortcut.rs
| /// free choice in the cycle is pinned (TOTAL COVERAGE). | ||
| /// | ||
| /// THIS IS WHERE THE NO-CONDITIONAL-ACTIONS CLAUSE IS SATISFIED, AND IT IS SATISFIED BY | ||
| /// CONSTRUCTION. CR 732.2a requires a proposal describe "the predictable results of the sequence of | ||
| /// choices" and says it "can't include conditional actions, where the outcome of a game event | ||
| /// determines the next action a player takes". Pins fix every free choice BEFORE the offer is made, | ||
| /// so the sequence the table accepts is the sequence that runs. The two companion gates are | ||
| /// `game::engine::try_offer_object_growth_shortcut`'s static rejection of coin flip / die roll / | ||
| /// random discard, and `analysis::resource::elimination_bounds` stopping the count strictly short | ||
| /// of every CR 704 loss threshold. "No conditional on a prior |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 12 '\bpredictability_gate\s*\(' crates/engine/src crates/engine/testsRepository: phase-rs/phase
Length of output: 20500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== decision_template outline =="
ast-grep outline crates/engine/src/analysis/decision_template.rs --view expanded | sed -n '1,220p' || true
echo
echo "== surrounding predictability_gate implementation =="
sed -n '700,760p' crates/engine/src/analysis/decision_template.rs
echo
echo "== slots and predictions types =="
rg -n "pub (enum|struct) (DecisionSlot|PinnedDecision|DecisionPrediction|ShortcutPoint)" crates/engine/src | head -80
echo
echo "== relevant call sites with context =="
sed -n '5288,5328p' crates/engine/src/game/engine.rs
sed -n '8876,8920p' crates/engine/src/game/interaction.rs
sed -n '3824,3848p' crates/engine/src/game/engine.rs
echo
echo "== target_schedule definitions =="
rg -n "enum TargetSchedule|pub struct ShortcutPoint|pub schema|points:" crates/engine/src/analysis/decision_template.rs crates/engine/src/game/engine.rs crates/engine/src/game/interaction.rs | head -200Repository: phase-rs/phase
Length of output: 14650
Remove the unconditional total-coverage claim from predictability_gate.
predictability_gate(t, &required) only checks the caller-supplied required_slots; it does not prove the decision template omits no free choices. The current declare path populates required from offer.schema.points, so the function only validates coverage of schema.points, not every free choice. If schema.points ever under-represents cycle choices, this comment overstates CR 732.2a coverage; either document dependence on the caller-supplied slot list or make completeness part of the runtime check.
🤖 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/analysis/decision_template.rs` around lines 701 - 710, The
comments around predictability_gate overstate that the function guarantees total
coverage, although it only validates caller-supplied required_slots. Revise the
claim to explicitly limit coverage to the provided slot list, or update
predictability_gate and its declare path so runtime validation proves every free
choice is covered.
Source: Path instructions
| // DEFICIENCY NOTE (realization gap vs the design at `types::game_state`'s | ||
| // `scheduled_collapse_axes` doc; full note on `ShortcutResponse`): CR 732.2b makes the | ||
| // named place the new ending point, so the shortcut should still be taken up to there. | ||
| // This hands the responder a real priority window instead. Tracked by the | ||
| // "Shortcut-system rules-correctness completion" follow-up in `.deferred-backlog.md`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)engine\.rs$|(^|/)turns\.rs$'
echo "== relevant snippets =="
sed -n '5450,5525p' crates/engine/src/game/engine.rs
printf '\n---\n'
sed -n '4620,4725p' crates/engine/src/game/engine.rs
printf '\n---\n'
sed -n '520,580p' crates/engine/src/game/turns.rs
echo "== search shortcut/collapse symbols =="
rg -n "ShortcutResponse|Shorten|at_iteration|scheduled_collapse_axes|LoopCollapse|RespondToShortcut|named_place|ending point|CR 732|732.2b|732.2c" crates/engine/srcRepository: phase-rs/phase
Length of output: 50371
🌐 Web query:
MTG Comprehensive Rules 732.2b 732.2c shortcut named place ending point
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rules 732.2b and 732.2c govern how players interact with a proposed game shortcut [1][2]. Rule 732.2b specifies that after a player suggests a shortcut, each other player (in turn order) may either accept the proposal or shorten it [3]. To shorten a shortcut, a player names a specific place—an "ending point"—where they intend to make a game choice different from what was originally proposed [3]. The player is not required to specify what their new choice will be at that time [3]. Once a player names such a place, it becomes the new ending point for the proposed sequence [3]. Rule 732.2c dictates the execution of the shortcut [3]. Once all players have either accepted the proposal or shortened it, the game advances to the final ending point [3]. All game choices contained within the shortcut are considered to have been taken [3]. If the original proposal was shortened, the player who holds priority at that new ending point is then required to make a game choice different from the one originally proposed for them in the shortcut [3].
Citations:
- 1: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
- 2: https://media.wizards.com/2026/downloads/MagicCompRules%2020260807.pdf
- 3: https://mtg.wiki/page/Shortcut
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== focused symbols in engine.rs =="
rg -n "fn handle_respond_to_shortcut|fn apply_confirmed_shortcut|ShortcutProposal|struct ShortcutProposal|pub enum ShortcutResponse|RespondToShortcut \{|PayableResource::LoopCollapse|min:0|max:" crates/engine/src/game/engine.rs
echo "== ShortcutProposal/ShortcutResponse definitions =="
rg -n "struct ShortcutProposal|pub struct ShortcutProposal|enum ShortcutResponse|pub enum ShortcutResponse|at_iteration" crates/engine/src/analysis crates/engine/src/types/game_state.rs crates/engine/src/types/actions.rs
echo "== loop_offer and collapse-related sections in engine =="
sed -n '4725,4930p' crates/engine/src/game/engine.rs
printf '\n--- apply_confirmed_shortcut? ---\n'
rg -n -C 25 "fn apply_confirmed_shortcut" crates/engine/src/game/engine.rs
printf '\n--- LoopCollapse handler in engine ---\n'
rg -n -C 35 "PayableResource::LoopCollapse|LoopCollapse" crates/engine/src/game/engine.rsRepository: phase-rs/phase
Length of output: 29207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== loop_check definitions =="
sed -n '130,235p' crates/engine/src/analysis/loop_check.rs
echo "== shortcut offer/accept/shorten construction section =="
sed -n '5320,5515p' crates/engine/src/game/engine.rs
echo "== RespondToShortcut dispatch section =="
sed -n '8568,8615p' crates/engine/src/game/engine.rs
echo "== fixed shortcut materialization and loop collapse min/max =="
sed -n '5865,6125p' crates/engine/src/game/engine.rs
sed -n '520,570p' crates/engine/src/game/turns.rsRepository: phase-rs/phase
Length of output: 35631
Use a single shortcut ending point and enforce accepted prefixes.
CR 732.2b makes any named earlier place the new ending point, and CR 732.2c advances to the final ending point with shortcut choices already taken. Currently Shorten is realized as decline-to-manual, so the named cycles are never materialized before the priority window. Update the Shorten path to advance through at_iteration cycles before handing priority, and keep the collapse prompt bounded by what the responder accepted rather than exposing the pre-shortened max.
📍 Affects 2 files
crates/engine/src/game/engine.rs#L5494-L5498(this comment)crates/engine/src/game/engine.rs#L4664-L4699crates/engine/src/game/turns.rs#L553-L565
🤖 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.rs` around lines 5494 - 5498, Update the
Shorten flow in crates/engine/src/game/engine.rs at lines 4664-4699 and
5494-5498, and the corresponding turn handling in
crates/engine/src/game/turns.rs at lines 553-565, so accepted shortcut choices
advance through the named at_iteration cycles before priority is handed off. Use
one final shortcut ending point, and bound the collapse prompt by the
responder’s accepted prefix rather than the original max; ensure all three sites
consistently implement this behavior.
…adicts The CodeRabbit round on this branch found two annotations asserting a structural fact that the code they describe does not support; sweeping the diff for that same mechanism found a third. 1. `try_offer_object_growth_shortcut` claimed non-`Committed` `FamilyCollapseState` variants mean no proposal exists. `FamilyCollapseState` has no `Committed` variant at all — that belongs to `CollapseCertainty` — and this branch's own tests assert `Scheduled(Conditional)` *after* an accepted shortcut, in the accept-to-boundary window. Only `Unscheduled`/`Mixed` mean absence; `Scheduled(Conditional)` is a proposal that passed the gate whose boundary can still decline or park. 2. `DeclareShortcut::template` was documented as "MUST be `None`". The declare handler accepts `Some`, binds `template.owner` to the engine-issued `offer.proposer` per CR 603.5, validates the pins, and drives with them; 23 call sites exercise `Some`. The always-null rule is a frontend policy pinned by `LoopShortcutModal`'s T2 test, not this action's contract. 3. The Path-B/Path-C redundancy proof carried four line-number citations, all wrong at the tip that introduced them: `:512-516` pointed at equip and ability-announcement code rather than the draw gate, and the three sibling anchors likewise addressed unrelated code. Converted to symbol anchors, which is what this branch's own citation doctrine requires; engine.rs is outside the citation gate's enrolled population, which is why they stayed green. Comment-only; no behavior change. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @coderabbitai — 2 of 4 landed, and sweeping for the mechanism behind them turned up a third. Fixed in Fixed1. 2. 3. Found by sweeping for your mechanism — four wrong line citations. Both of your findings are one defect class: an annotation asserting a structural fact that its defining code does not support. Sweeping the diff for that class, the Path-B/Path-C redundancy proof carried four line-number anchors and all four address unrelated code — Skipped, with reasons
Your two outside-diff notes on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- HEAD and reported commit ---'
git rev-parse HEAD
git show --stat --oneline --no-renames b0ddffd9c1319a390cffa6ee487547d203324f05
printf '%s\n' '--- Named Rust symbols ---'
ast-grep outline crates/engine/src/game/engine.rs --items all --match 'try_offer_object_growth_shortcut'
ast-grep outline crates/engine/src/types/actions.rs --items all --match 'DeclareShortcut'
ast-grep outline crates/engine/src/analysis/loop_check.rs --items all --match 'ShortcutResponse'
ast-grep outline crates/engine/src/analysis/decision_template.rs --items all --match 'predictability_gate'
ast-grep outline crates/engine/src/game/derived_views.rs --items all --match 'FamilyCollapseState|CollapseCertainty'
printf '%s\n' '--- Family state and offer documentation ---'
rg -n -C 12 '\b(FamilyCollapseState|CollapseCertainty|try_offer_object_growth_shortcut)\b' \
crates/engine/src/game/derived_views.rs \
crates/engine/src/game/engine.rs
printf '%s\n' '--- DeclareShortcut template contract and handler ---'
rg -n -C 12 '\b(DeclareShortcut|template|predictability_gate|validate_pins)\b' \
crates/engine/src/types/actions.rs \
crates/engine/src/game/engine.rs \
crates/engine/src/analysis/decision_template.rs
printf '%s\n' '--- Shorten representation and handling ---'
rg -n -C 12 '\b(ShortcutResponse::Shorten|Shorten\s*\{|match.*ShortcutResponse|ShortcutResponse)\b' \
crates/engine/src/analysis/loop_check.rs \
crates/engine/src/game/engine.rs
printf '%s\n' '--- Conditional accepted-collapse regression evidence ---'
rg -n -C 8 'Scheduled\(CollapseCertainty::Conditional\)|Scheduled\(Conditional\)|Conditional' \
crates/engine/tests/integration/combo_infinite_pile.rs \
crates/engine/src/game/derived_views.rs
printf '%s\n' '--- Pin coverage implementation ---'
rg -n -C 10 '\b(required_slots|predictability_gate|validate_pins)\b' \
crates/engine/src/analysis/decision_template.rs \
crates/engine/src/game/engine.rsLength of output: 201912
The The The The ✏️ Learnings added
You are interacting with an AI system. |
The previous commit fixed two false statements inside the shortcut argument. Correct-but-unconnected is its own failure mode: a reader scanning the conditional-actions gate for a crack finds a proposal that passed the gate and can still be declined, and nothing there says why that is the invariant working rather than a leak. State it. A `Scheduled(Conditional)` family is routes (b)/(c) of ELISION ≡ PERFORMANCE operating as designed, not a conditional action escaping the gate: CR 732.2a governs whether the sequence's next action is determined by a game event's outcome, and none of it is, because the pins fix every free choice before the offer exists. What a boundary may still decline is the ELISION, handing those same iterations back to manual play — which is precisely how the invariant survives an observer appearing mid-window. Declining an elision is never rules-incorrect; the elision needs a license, its absence never does. Likewise `DeclareShortcut::template`: the owner binding plus pin validation IS L2 (unconditionality by construction) enforced at the wire, so accepting `Some` costs the CR 732.2a argument nothing. Comment-only; no behavior change. Assisted-by: ClaudeCode:claude-opus-5
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
-
[HIGH / rules correctness] Acceptance still defers the accepted shortcut's result instead of taking the shortcut at its proposed ending point. Evidence:
crates/engine/src/game/engine.rs:5051-5086only registersPersistentAxisMaterializationafter acceptance, whilecrates/engine/src/game/engine_resolution_choices.rs:2516-2564applies it later throughPayableResource::LoopCollapse; the recent documentation commits do not change either path. CR 732.2c requires that, after acceptance, the shortcut is taken and the game advances with all proposed choices taken. Why it matters: the later boundary can defer or decline the proposed growth, so acceptance does not produce the required game state. Suggested fix: redesign the acceptance/shortening flow so the finite loop result is applied as the shortcut advances to its ending point, rather than stashed for a later boundary choice. -
[LOW / review evidence] Current-head parser evidence is missing. Evidence: the only
<!-- coverage-parse-diff -->sticky identifies head684335b0ab1754ac91105b94342691a29127ae70, not reviewed headb0ddffd9c1319a390cffa6ee487547d203324f05. Why it matters: its no-change result cannot be attributed to this commit. Suggested fix: publish a parse-diff artifact bound to the current head before requesting another approval.
…osing function The two preceding comment-only commits shifted `game/engine.rs`'s producer coordinate by 20 lines, reddening `the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_event`. Comment-only diffs read as incapable of moving a line pin right up until they do, and the round that introduced the drift shipped during a GitHub Actions outage, so CI could not say so. Re-pinned `:11763 ⇒ :11783` with the drift-log convention's evidence: the +20 decomposes over exactly five hunks, all above the producer — four one-line growths in `interactive_loop_bridge` (the citation conversions) plus +16 in `try_offer_object_growth_shortcut`; predicted 11763+20 matches the observed coordinate; the line is sha256-identical to both `684335b0a:11712` and `2b20aa73a:11763`; the text occurs exactly once in the file. Also corrects that row's enclosing-function claim from `apply_retarget` to `begin_pending_trigger_target_selection`. `apply_retarget` ends where the latter begins and the producer sits inside the latter — as this assertion's own message has always said. Same defect class as the three the review round already fixed: a structural claim the defining code contradicts. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @matthewevans — taking the HIGH head-on rather than re-stating the annotations, because I think it rests on a premise the code doesn't support. 1. There is no finite result at acceptance to applyThe suggested fix is to apply "the finite loop result as the shortcut advances to its ending point." That presupposes a finite result exists at acceptance. It does not. What the table accepts is The finite number comes into existence at the boundary, and that is exactly where the engine applies it: Applied literally, the demand cannot mean "apply the result sooner", because there is no result to apply. It can only mean force the controller to pick N at acceptance — which does not move when the result is applied, it moves where the count-choice lives. That is a procedural placement question, and CR 732.1a explicitly leaves it to the shortcut system: "as long as each player in the game understands the intent of each other player, any shortcut system they use is acceptable." 2. The two designs have identical reachable-state setsBy the fidelity invariant documented at So the difference between the two designs is not correctness. It is when the number becomes visible to the table. A demand that cannot change any reachable game state is a presentation preference, not a rules-correctness defect — and it is the one axis on which the two designs genuinely differ. On "the later boundary can defer or decline the proposed growth": declining an elision is never rules-incorrect. The elision is what needs a license; its absence never does. Both the decline and the 3. What the change would actually costThe accept→boundary window is not incidental to the feature — it is the feature. It is the CloseThe in-code argument at On the LOW: agreed, and it's queued rather than dismissed — a parse-diff bound to the current head needs a CI run, and GitHub Actions has been in a major outage (multiple services, queued jobs timing out) for the last hour. It goes up with the next push once Actions is healthy. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
- [HIGH / rules correctness] The current head still defers an accepted shortcut instead of taking it at its accepted ending point. Evidence:
crates/engine/src/game/engine.rs:5063-5097only registersPersistentAxisMaterialization, thenengine.rs:5101-5104restores priority. The pending result is instead processed later throughcrates/engine/src/game/engine_resolution_choices.rs:2516-2570, where the separateLoopCollapsechoice can defer or decline an axis. The current1723f2ddelta from the prior reviewedb0ddffdhead changes only prose/citations and a test pin; it does not change either resolution path. CR 732.2c requires that once all players accept or shorten, “the shortcut is taken ... with all game choices contained in the shortcut proposal having been taken.” Returning priority and later asking whether/how to materialize the accepted growth does not produce that required game state. Please redesign acceptance/shortening so the finite result is applied while advancing to the shortcut's final ending point, rather than stashing it for a later boundary choice.
2 participants
🤖 AI text below 🤖
Summary
Makes the
∞→Nbadge's promise structurally true instead of documented. The badge asserted that an accepted loop collapse would land; two of the boundary's four materialization arms can finish without applying, so for those axes no finite amount is ever chosen and the∞simply persists. The per-axisscheduledboolean is replaced by an engine-owned(player, family)channel carrying a typedFamilyCollapseState, so a family that might be refused renders∞→?and only a family that cannot be refused renders∞→N.Answers both maintainer MED findings on this PR. Both had been disclosed in comments by this PR's own author and rejected as remedies — that rejection is the point of this change, so what follows is a fix, not a better explanation.
Files changed
32 paths, transcribed from
git diff --name-status a0d0b05..684335b.Protocol (4) — added by the review round, see Scope Expansion:
crates/lobby-broker/src/protocol.rscrates/server-core/src/protocol.rsclient/src/adapter/ws-adapter.tsscripts/check-protocol-version.mjsEngine (4):
crates/engine/src/game/derived_views.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rsEngine tests (3):
crates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rsFrontend (10):
client/src/adapter/types.tsclient/src/components/board/OpponentSeatHeader.tsxclient/src/components/hud/HudBadges.tsxclient/src/components/hud/ManaPoolSummary.tsxclient/src/components/hud/OpponentHud.tsxclient/src/components/hud/PlayerHud.tsxclient/src/hooks/usePlayerDesignations.tsclient/src/components/hud/__tests__/PlayerHud.designations.test.tsxclient/src/components/hud/__tests__/UnboundedBadge.test.tsx(added)client/src/viewmodel/__tests__/unboundedWireSeam.test.tsFixtures (4) and i18n (7):
client/src/test/fixtures/unbounded-counter-wire.json,unbounded-token-wire.json,unbounded-declined-wire.json(added),unbounded-family-tags.json(added)client/src/i18n/locales/{de,en,es,fr,it,pl,pt}/game.jsonTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
The load-bearing correction in this PR's annotations. The accept→boundary window is an engine deviation that no CR licenses, and the tree already says so in several places, including
turns.rs("ENGINE TOLERANCE, NOT A RULES ENTITLEMENT — no CR licenses this, and none is cited"). Five consecutive citations were tried and rejected on this branch (616.1 → 614.16 → 732.2b → 732.1b → 732.2a), all one category error: searching a rule space that does not contain the answer. The annotations now take the engine-deviation form already present inderived_views.rs.All verified by grepping
docs/MagicCompRules.txtbefore use, on both antecedent axes — subject and time.Verification
All eight run at committed head
ae32aa9493aaa957921694cafd1df0b0fa734723; the follow-up commit684335b0touches only a build script and was verified with CI’s own commands (below). At the previous head they were additionally run twice independently with all eight exits identical, so the battery itself is a reproduced measurement rather than one sample. Counts transcribed from the logs, never recalled.cargo fmt --all -- --check— exit 0cargo clippy --workspace --all-targets -- -D warnings— exit 0cargo test -p phase-engine --lib— 18560 passed; 0 failed, includinggame::engine_resolution_choices::tests::subsystem_citations_are_symbol_anchored ... okcargo test -p phase-engine --test integration— 4586 passed; 0 failednpx tsc -b --noEmit --force— exit 0npx vitest run— 291 files passed, 3 skipped (294); 2613 passed, 12 todo (2625)npx eslint . --max-warnings=99999— exit 0, 0 errors / 29 warnings (pre-existing)npx vitest run src/components/hud/__tests__/PlayerHud.designations.test.tsx— 1 file, 23 tests passedThe battery under-covered this round twice, and both gaps are named rather than absorbed into the 8/8.
(1) Package scope.
check.2/check.3are scoped-p phase-engine; clippy compilesserver-core/lobby-brokerbut never runs their tests. Run separately at the same commit:cargo test -p server-core -p lobby-broker --lib— server-core 315 passed / 0 failed, lobby-broker 83 passed / 0 failed, includingprotocol_version_is_24andfull_game_floor_is_current_only_not_a_rollout_window.(2) Command set — this one CI caught first, and it is worth stating plainly. The battery ran
npx tsc -b --noEmit --force; CI'sType checkstep runspnpm run type-check, which isprotocol:check && tsc -b --noEmit. The battery therefore omittedscripts/check-protocol-version.mjs— the one check in the suite built specifically to catch an inconsistent protocol bump — and it failed atae32aa9because that script pinsEXPECTED_PROTOCOL_VERSIONas a deliberate third-place acknowledgement. Fixed in684335b0. The head is now verified with CI's exact command rather than a local approximation:pnpm run type-checkexit 0,pnpm run lintexit 0.Both gaps have the same shape and it is worth naming: a reused battery's scope — which packages, which commands — is an assumption inherited from the change it was originally built for. This round grew a protocol surface that battery never had, and an 8/8 would have read as coverage of the very assertions it never executed.
Every check that is silent on success carries a must-fail control, because a green from a check that never ran is the failure mode this round already hit once (a bare
--exacttest filter matched nothing and exited 0 on all three arms).fmtwas made to fail (exit 1, 9 diff lines) andtscwas made to fail (exit 2,TS2322); both plants were reverted and the tree re-verified at porcelain 0.cargo semantic-audit— DECLARED-NOT-RUN, with its reason, because a silently omitted check is worse than a declared one. Two independent grounds: its inputdata/mtgjson/AtomicCards.jsonis gitignored and absent from a fresh worktree, and it is superseded by the forced parser projection, which is a controlled BASE-vs-CANDIDATE comparison where semantic-audit at the candidate alone is one uncontrolled point. The supersession would have expired had the measurement returnedCANNOT_ANSWER; it returnedMEASURED.Gate A
Gate A PASS head=684335b0ab1754ac91105b94342691a29127ae70 base=a0d0b05267625128b3a065b5d4382ef8a2db9ce3
Stated as a vacuous PASS, because it is one.
git diff --name-only a0d0b05..684335b -- crates/engine/src/parser/returns 0 files, so the gate had nothing in its scope to judge. A bareGate A PASSwould imply work the gate did not do. (Gate G PASSalso emitted: router/grant boundary intact.) Base passed explicitly rather than defaulted, since the script's default isorigin/mainandoriginis a fork that lags.Anchored on
Both re-derived at the candidate and verified to pre-exist at
a0d0b05, since the channel the prior body anchored on (unbounded_resources+scheduled) is the thing this PR replaced.crates/engine/src/game/derived_views.rs:122—PlayerConditionKind, an existing typed enum carried per-player onDerivedViews.player_statusand read verbatim by the client atclient/src/hooks/usePlayerDesignations.ts:104. The precedent for putting a typed state on the wire instead of a boolean, which is whatFamilyCollapseStatedoes.crates/engine/src/game/derived_views.rs:454—stack_display_groups, whose authoritative grouping lives ingame::stack::stack_display_groupsand is rendered directly atclient/src/components/stack/StackDisplay.tsx:54with no client-side fold. The precedent for moving a display fold into the engine, which is exactly MED-1's remedy.Final review-impl
Round 2 review-impl PASS head=
44fdeb2f0172277febabdc92e20a7cc771f0a161(Semantic-Impact PASS, Completion PASS, Maintainer-Sim FAIL; 4 MED + 8 LOW). The box above is left unchecked deliberately: no independent review ran at this head, and writing PASS for a review that did not run would be false.Round 3's closure was ruled at dispatch under the terminal-round rule, not skipped. The r2 reviewer itself specified every r3 fix, so round 3 had no design latitude of its own; its two mutation predictions were pre-registered before implementation, with the standing instruction that a "before" probe coming back RED means the finding was wrong and to stop rather than fix a non-defect. All predictions settled as predicted — both mutations two-sided with captured artifacts, the maintainer-simulation matrix gate re-run with its own positive controls, and the errata present with their generating commands. Under that rule a judgment round triggers only on a prediction miss; none missed.
That verdict reaches this head by content identity, not assumption: the canonical 27-path scoped diff is byte-identical pre- and post-rebase (223304 bytes, sha256
ee3c5e61997c510713745b33c63fae062a826644b3c6ea4e68887f2dae89566a). The unreviewed-by-a-fresh-context delta is quantified in Validation Failures.Claimed parse impact
None.
The projection was forced, not skipped, and its result is stronger than "no clusters found".
scripts/engine-source-hash.shgivesdab40d3a0341bb0dat base and0a9d9bc0a0dfe4e5at candidate (16 hex is that script's own output width, not an abbreviation), because it hashes all ofcrates/engine/srcand in-scope files live there. That is content-scoped, not semantics-scoped: every added line undercrates/engine/src/in the fix round is documentation or#[cfg(test)], and the hash still differs — so "no production change" does not exempt a change from projection. The four-path authority diff is empty (0 bytes) ⇒projection_forced_reason=SOURCE_HASH_DIFFERENCE.The two projected
card-data.jsonfiles are byte-identical — both 99502599 bytes, sha256fab9cbe9cacd0c2e7c6186a01cf1375db6787b5d4eb594fb4639860d6ec7acec, over 35657 cards on both sides. The parser's entire output is bit-for-bit the same at base and candidate, so a parse change is not merely unobserved, it is impossible.clusters: []follows from that rather than standing alone. Onlycoverage-data.jsondiffers, placing the divergence strictly downstream of the parser.Correction, measured after this section was first published:
oracle_changedon these two projected files is 2, not 0 —FastandReplenish. Both are exact-name duplicate groups, so this is the key-collision hazard listed under "Residuals, named with their settling instruments" firing, not a parse change: withcard-data.jsonbyte-identical, a genuine oracle-text delta is impossible, and what differs is which member of a duplicate group won the comparator's last-wins name key.card-data.jsonholds 30 duplicate-name groups, of which exactly two —FastandReplenish— carry differing oracle text between their members, so 2 is this hazard's predicted ceiling and the measurement sits exactly on it. Verified with three independent instruments: comparators built at base (ea6bd991) and at head (684335b0), whose outputs are byte-identical (da8585aac25867a803e93724848aa7fa268230604107d32384965c02fa1bd8da), plus a Python reimplementation of the counting rule sharing no code with either. The comparator treatsoracle_changedas an explicit carve-out ("parse legitimately differs for a non-parser reason (errata/reprint) — do not attribute to the PR"), which is why the verdict remains "No card-parse changes detected" in both pipelines. The receipt'sartifact.26recordsoracle_changed: 0for these same inputs; those two rows are mutually inconsistent and the discrepancy is unexplained — see the disclosure comment on this PR.Head-binding to the reviewed head
684335b0, since parse evidence that names an older head is evidence about that older head. The projection above was measured at949b9725. Rather than assert it "still holds", the binding is re-measured:scripts/engine-source-hash.shinvokes no cargo — it hashescrates/engine/{src,data,build.rs,Cargo.toml}— and at684335b0it returns0a9d9bc0a0dfe4e5, identical to the value the receipt records at949b9725, with base stilldab40d3a0341bb0d.git diff --name-only 949b9725..684335b0over exactly those hashed paths lists 0 files; the full changed set across those two commits is five files (ws-adapter.ts, one engine test, the two protocol crates,scripts/check-protocol-version.mjs), none of which the parser reads. The projection's inputs are therefore bit-identical at the reviewed head, so the byte-identicalcard-data.jsonresult carries forward as a measurement reproducible in one command, not as an inference. A parse-diff artifact generated directly at684335b0is published in the review thread, and CI's owncoverage-parse-diffsticky reports the same head independently.One label caveat, stated so it cannot be misread: the receipt records
parser_evidence=PROJECTED_PARSE_DIFF. That is the contract's label for "a projection was forced and performed" — not a claim that parses changed. The substance is the identical digests above.Each side of the comparison carries a content floor assertion. This is not ceremony:
coverage-reportwas observed exiting non-zero while emitting a complete, well-formed, all-zeros JSON (total_cards: 0,cards: []). A valid-shaped empty artifact passes every existence, parse, and digest check and would have produced a confident, internally perfect, substantively false "no parse diff". Both sides assert 35657.Scope Expansion
Yes — one review-directed exit, recorded as scope amendments r3 (27 → 30 paths) and r4 (30 → 31).
Added
crates/lobby-broker/src/protocol.rs,crates/server-core/src/protocol.rs,client/src/adapter/ws-adapter.ts, and — in r4 —scripts/check-protocol-version.mjs.r4 is r3 finished, not new work. The census that produced r3's path set missed the tripwire script on two independent keys at once: directory (it lives in
scripts/, and the sweep searched onlycratesandclient/src) and extension (.mjs, against an allow-list of.rs/.ts/.tsx). Either miss alone would have hidden it. The correct key was never directory-and-extension — it is who consumes the protocol version, and that set includes a build script no source-tree sweep of that shape can see. The replacement sweep is a repo-widegit grepwith no path or extension filter, which confirmed exactly one remaining stale site. The authorizing instrument is the maintainer's own MED finding on this PR asking for the protocol bump; the amendment doc quotes it, and the record is what makes that authority citable later. Recorded on the same procedure as the earlier amendment: new path list, new NUL representation, new sha256s, anddiff == scopere-verified in both directions at the fix-round checkpoint (0 diff paths outside scope).The frozen scope was also amended earlier, from 26 paths to 27, adding
client/src/components/hud/ManaPoolSummary.tsx. That is not scope expansion but the completion of finding F5: a sentence added by this PR asserted the frontend derives no family while that file derived one. Recorded with its reason and a new digest before any edit. The 30 frozen paths are a strict subset of the PR's 31 changed files; the one outside (crates/engine/src/types/game_state.rs) was touched by this PR's earlier commits, not by the fix rounds.Validation Failures
One, and it is a pipeline-shape disclosure rather than a failing check: round 3 was closed by ruling, not by an independent
/review-impl.Round 2 reviewed
44fdeb2f…and returned 4 MED + 8 LOW. Round 3 fixed them — broadening the F3 detector tobreak/?and asserting exactly onecollapsed.push, converting line citations to symbol anchors, and binding the hold census to the loop it describes — and that delta (5729ac44..949b9725, 6 files, +281/−76) was not re-read by a fresh reviewer context. It closed under the terminal-round rule described above (reviewer-specified fixes, pre-registered predictions, zero misses). Recording it here because "closed by ruling" and "reviewed again" are not the same thing, and a reader deserves to know which one this is.What is known about the unreviewed delta, measured rather than asserted: 62 added lines in the production region, 0 of them non-comment (boundary taken at the
#[cfg(test)] mod testsopener in new-file coordinates); 171 additions insidemod tests;derived_views.rs+15, all comments. The full battery above is green at this exact head. That bounds the risk; it does not substitute for the review.CI Failures
None.
What structurally answers each finding
MED-1, mixed-family discriminator. The frontend folded per-axis rows into families with a last-wins OR, so one seat's accepted stash could mark another seat's row. The fold moves into the engine as a commutative join over
(player, family), computed on the producing controller's key beforeattribution_playerrewrites it — which is why it can only be answered engine-side: after the rewrite the producing controller's identity is gone. A collision now yieldsMixed, which renders a bare∞. The OR-fold's answer to a collision was "whichever row came last";Mixedis the honest one.MED-2, boundary-decline regression.
possible_holdis a census of the ways the apply loop finishes an item without reaching its push:Tokenscan park on a copy-token replacement choice,Counters/Lifecan be declined when an observer of the growing class appears in the accept→boundary window.materialization_certaintymaps a hold toConditionaland its absence toCommitted. The promise is now derived from the loop's own control flow rather than from its decline policy.The inversion worth stating, because it reverses the intuition the old badge encoded. The only arm that earns the affirmative
∞→NisDriveSequence— the one that replays real cycles and therefore sounds the least certain. It holds aSimulationProbeGuardso it cannot park, and itbreaks to commit its successful prefix on a failed cycle, so it always reaches the push. The batched arms — which sound like arithmetic already decided — are the ones that can be refused. The old design promised most confidently exactly where it was least entitled to.Four
collapsed.pushsites collapse to one. That is what makes "reaches the push" a one-screen property and letspossible_holdbe checked as exactly the set that can skip it, rather than a hand-maintained list the loop could silently outgrow.Replacing — not deleting — the two dispositions this PR previously argued for. Both were reasoned positions in the prior body and in code comments; a deleted objection invites re-derivation, so each is answered in place:
possible_holdcomputes. The field was redefined to something flag time can actually know.scheduledmeans 'a collapse is scheduled for this axis', never 'this growth will land'; that is stated on the field and at the scope limit."∞→Nand the words "a finite amount will be chosen". The type now carries the distinction the prose was carrying, so the render site cannot restate it wrongly.Change partition
Presented as sections rather than as history. This repo squash-merges, so the PR tip is what gates; manufacturing commits from one verified tree would introduce per-commit states nobody has measured, and the parts do not build in isolation (engine types → engine emit → tests → frontend are interdependent).
BoundaryHold { ObservedGrowth, CopyTokenPause },possible_hold,materialization_certainty,boundary_declinesas the single decline authority, and the four-push-to-one collapse.DerivedViews::unbounded_families(UnboundedFamily×FamilyCollapseState) replacingUnboundedResourceView::scheduled, emitted before the Commander short-circuit so non-Commander games keep the channel.two_controllers_draining_one_victim_do_not_cross_schedule,mixed_family_is_not_scheduled,two_mana_axes_fold_to_one_family_row,family_tag_table_matches_the_client_golden,family_collapse_state_merge_is_a_join,subsystem_citations_are_symbol_anchored,boundary_hold_census_matches_the_apply_loop— plus 11 frontend cases.HudBadgesrenders the typed state; the client-side fold is deleted, not deprecated.Rebase
This PR's base was 9 commits behind
upstream/main, now rebased ontoa0d0b05267625128b3a065b5d4382ef8a2db9ce3(v0.47.0). Those 9 commits touch 62 files with zero overlap against this PR's 27 frozen paths — checked with a positive control: planting an in-scope path into the upstream set makes the overlap check report 1, so the zero is a measurement and not an empty pipeline.Path-overlap alone is not enough, because rebase drift surfaces in verification rather than in conflicts, so the whole battery above was re-derived at the rebased tip rather than inherited.
Scoped-diff identity bridge. The canonical 27-path diff was recomputed against the new base and compared with the pre-rebase artifact: byte-identical, both 223304 bytes, sha256
ee3c5e61997c510713745b33c63fae062a826644b3c6ea4e68887f2dae89566a. (A content-normalization step — strippingindexlines, since raw diff digests embed blob SHAs a rebase can rewrite — was prepared as the fallback and proved unnecessary: upstream touched none of the 27 paths, so even the blob SHAs are unchanged. The bridge carries its own can-fail control.) That identity is what makes the round-2 review verdict transfer to the tip by content, as a measured fact rather than an assumption.The receipt binds to
base_sha=ea6bd991c56fe687d71fa4005e22edd864bef009,candidate_sha=head_sha=949b9725056fb95eaa1ffe46b0e2f1ff47799fd5because the battery and the projection both ran at that tip; a receipt whosehead_*fields contradict its owncandidate_shais self-invalidating. The review-round commitae32aa9advanced the head past that receipt, and the receipt is deliberately NOT re-pointed — it records what was measured where it was measured. What carries to the new head is stated as measurement instead: the full battery re-run there (above), and the source-hash identity under Claimed parse impact. The parser projection was re-run at the rebased tip — the prior body's "not re-run" residual is retired, not carried.Citation anchoring (F1)
Line-number citations go stale from ordinary code growth, and this PR's own additions to
engine_resolution_choices.rsare what shifted several of them. So the remedy is symbol and greppable-heading anchors, not re-pinned line numbers — re-pinning would be correct only until the next edit.Census over all 27 in-scope paths. Three keys are reported because they genuinely differ, and every disagreement this round came from quoting one as if it were another: a line can carry several citations, and a filename-keyed regex cannot see bare back-references (
:517) at all.a0d0b05(before any of this PR)5729ac44949b9725Per file at round-3 start → after:
derived_views.rs7/7/7 → 0,engine_resolution_choices.rs23/26/26 → 0,combo_infinite_pile.rs3/3/3 → 0,loop_shortcut.rs13/8/15 → 0.Generating commands (cite the command, never the figures alone):
The
beforecolumns are the instrument's own control: a zero census proves nothing unless the same instrument returns non-zero on the same population beforehand.Two honest notes:
loop_shortcut.rsscores 8 under the filename-keyed regex and 15 under the guard's own recognizer, because bare back-references carry no filename. The regex column is a floor, not a total — which is why all three keys are shown rather than one tidy number.subsystem_citations_are_symbol_anchoreddiscovers its population by scanning for a marker rather than hardcoding a file list, so the enforced set cannot drift out of sync with the files, and it carries an enrolment floor so a broken walk or a renamed marker reds instead of passing on an empty population. A file that adds citations without opting in is still uncaught; that hole is named in the guard's own doc. Enforcing repo-wide was measured and rejected as out of scope, not waved off: measured at this tip,crates/engine/srccarries 235 filename-keyed instances across 62 files — roughly 7× this PR's in-scope production population (33 instances at round-3 start), on the same key, so the ratio compares like with like.Residuals, named with their settling instruments
Mixedfrom a natural production accept. Both current witnesses hand-mark their loops. Settles with: a real 4p dump where one seat holds two loops and only one is accepted.DriveSequencecommitting k=0.Committed⇒∞→Nplus "a finite amount will be chosen" ships for a drive whose first cycle can fail, leaving nothing applied while the∞clears. A copy question, not a correctness one — the axis does end — but it is the last path by which the affirmative badge can front a no-op. Settles with: a test grafting aDriveSequencewhose first cycle fails.coverage-parse-diffexact-name key collision. The comparator keys its maps on card name, and the corpus holds 30 exact-name duplicate groups (two distinct cards literally named "Fast"), so a real oracle delta on a shadowed printing is invisible toparse-diff.json. It fired this run. Re-measured on the projected files:oracle_changed: 2, onFastandReplenish— precisely the two of the thirty groups whose members carry differing oracle text, i.e. this hazard's predicted ceiling. It is a duplicate-selection artifact and not a parse change, becausecard-data.jsonis byte-identical across base and candidate; the comparator's own carve-out excludes it, so the verdict stays "No card-parse changes detected". An earlier version of this section claimedoracle_changed: 0, which the receipt records but which does not reproduce — corrected here rather than quietly dropped. The hazard is structural, not case-folding — lowercase-only collisions number zero. Pre-existing; backlogged with an owner.refurbished_familiar.rs,skullwinder_chosen_opponent.rs,ability_rw.rs). Pre-existing at base — but this PR's additions toengine_resolution_choices.rsmake two of them materially staler, one going from partially-right to entirely-wrong. Remedy is heading-text re-anchoring, not line re-pinning. Backlogged. Scope boundary stated plainly: every in-scope file is converted (see Citation anchoring), so what remains is out-of-scope only. These become catchable the moment their files adopt the guard's marker — the residual is un-adopted coverage, not an unenforceable rule.Process note
Round 1's completion evidence could not be recorded in the receipt's required form, because the battery was launched with stdout and stderr combined and the contract requires them separately per check. That was not retrofitted or fabricated —
completion_check_count=0would have been false since the checks ran, and pointing four groups at one combined log would have been a conforming-looking record over a measurement never taken that way. It was closed by design change: the round-2 battery captures per-check separated streams at the source, making all 16 contract fields transcribable rather than reconstructable.Summary by CodeRabbit
New Features
Localization
Compatibility