refactor(engine): move casting_costs/casting_targets into casting/ submodule

[dale053]

Summary

Closes #2814

Reorganizes the casting module from flat files into a casting/ submodule, moving casting.rs → casting/mod.rs, casting_costs.rs → casting/costs.rs, and casting_targets.rs → casting/targets.rs. Updates all import paths across the engine crate. No logic changes.

Files changed

• crates/engine/src/game/casting/mod.rs (renamed from casting.rs)
• crates/engine/src/game/casting/costs.rs (renamed from casting_costs.rs)
• crates/engine/src/game/casting/targets.rs (renamed from casting_targets.rs)
• crates/engine/src/game/mod.rs — removed flat declarations, added pub mod casting
• crates/engine/src/game/cost_payability.rs — import path update
• crates/engine/src/game/effects/collect_evidence.rs — import path update
• crates/engine/src/game/effects/pay.rs — import path update
• crates/engine/src/game/engine.rs — import path update
• crates/engine/src/game/engine_casting.rs — import path update
• crates/engine/src/game/engine_modes.rs — import path update
• crates/engine/src/game/engine_replacement.rs — import path update
• crates/engine/src/game/engine_resolution_choices.rs — import path update
• crates/engine/src/game/engine_stack.rs — import path update
• crates/engine/src/game/filter.rs — import path update
• crates/engine/src/game/mana_abilities.rs — import path update
• crates/engine/src/game/restrictions.rs — import path update
• crates/engine/src/game/splice.rs — import path update
• crates/engine/src/game/triggers.rs — import path update

CR references

No new CR annotations — structural reorganization only, no logic changes.

Track

Developer

LLM

Model: claude-sonnet-4-6
Thinking: medium
Tier: Standard

Verification

• cargo fmt --all — clean
• ./scripts/check-parser-combinators.sh — clean (no parser logic touched)
• ./scripts/tilt-wait.sh clippy test-engine — pending CI

Anchored on

• crates/engine/src/game/effects/mod.rs — existing submodule pattern: effects/ directory with mod.rs re-exporting sibling files, consumed via use crate::game::effects::*
• crates/engine/src/game/mod.rs:12 — existing pub mod casting declaration used as the template for the new submodule entry point

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

[gemini-code-assist]

Code Review

This pull request reorganizes the casting module structure by moving casting_costs and casting_targets into casting::costs and casting::targets respectively, updating module paths, imports, and visibility modifiers across the engine. The review feedback highlights opportunities to improve code robustness and refactoring safety by replacing fragile, deeply nested relative imports (e.g., super::super::) with absolute paths starting with crate::game::, in accordance with the idiomatic Rust style guide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

[MEDIUM] Fragile relative imports. Evidence: crates/engine/src/game/casting/costs.rs:22-36. Why it matters: Using deeply nested relative paths like super::super:: makes the codebase fragile to refactoring and leads to significant code churn when files are moved. Using absolute paths starting with crate::game:: is more idiomatic and robust. Additionally, importing other modules (like quantity, filter, zones, etc.) at the top of the file using crate::game:: absolute paths would allow you to remove the verbose inline super::super:: prefixes throughout the file, preventing future refactoring pain.

Suggested change

	use super::super::effects::counters::add_counter_with_replacement;
	use super::super::engine::EngineError;
	use super::super::mana_abilities;
	use super::super::mana_payment;
	use super::super::mana_sources::{self, ManaSourceOption};
	use super::super::restrictions;
	use super::super::stack;
	use super::emit_targeting_events;
	use super::super::ability_utils::{
	assign_targets_in_chain, auto_select_targets_for_ability, begin_target_selection_for_ability,
	build_target_slots, build_target_slots_labelled, flatten_targets_in_chain,
	modal_choice_for_player, random_select_targets_for_ability, target_constraints_from_modal,
	};
	use super::life_costs::PayLifeCostResult;
	use super::super::life_costs::PayLifeCostResult;
	use crate::game::effects::counters::add_counter_with_replacement;
	use crate::game::engine::EngineError;
	use crate::game::mana_abilities;
	use crate::game::mana_payment;
	use crate::game::mana_sources::{self, ManaSourceOption};
	use crate::game::restrictions;
	use crate::game::stack;
	use super::emit_targeting_events;
	use crate::game::ability_utils::{
	assign_targets_in_chain, auto_select_targets_for_ability, begin_target_selection_for_ability,
	build_target_slots, build_target_slots_labelled, flatten_targets_in_chain,
	modal_choice_for_player, random_select_targets_for_ability, target_constraints_from_modal,
	};
	use crate::game::life_costs::PayLifeCostResult;

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. Enums over stringly-typed data. Absolute imports are preferred over fragile relative paths. ^(link)

[gemini-code-assist]

[MEDIUM] Fragile relative imports. Evidence: crates/engine/src/game/casting/targets.rs:12-21. Why it matters: Using deeply nested relative paths like super::super:: makes the codebase fragile to refactoring. Using absolute paths starting with crate::game:: is more idiomatic and robust.

Suggested change

	use super::super::ability_utils::{
	ability_target_legality_needs_chosen_x, assign_selected_slots_in_chain,
	assign_targets_in_chain, auto_select_targets_for_ability, begin_target_selection_for_ability,
	build_chained_resolved, build_target_slots_labelled, choose_target_for_ability,
	flatten_targets_in_chain, random_select_targets_for_ability, validate_modal_indices,
	validate_selected_targets_for_ability, TargetSelectionAdvance,
	};
	use super::casting::{emit_targeting_events, pay_ability_cost_for_activation};
	use super::casting_costs::{
	use super::super::engine::EngineError;
	use super::super::restrictions;
	use super::super::stack;
	use crate::game::ability_utils::{
	ability_target_legality_needs_chosen_x, assign_selected_slots_in_chain,
	assign_targets_in_chain, auto_select_targets_for_ability, begin_target_selection_for_ability,
	build_chained_resolved, build_target_slots_labelled, choose_target_for_ability,
	flatten_targets_in_chain, random_select_targets_for_ability, validate_modal_indices,
	validate_selected_targets_for_ability, TargetSelectionAdvance,
	};
	use crate::game::engine::EngineError;
	use crate::game::restrictions;
	use crate::game::stack;

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. Enums over stringly-typed data. Absolute imports are preferred over fragile relative paths. ^(link)

[gemini-code-assist]

[MEDIUM] Fragile relative path. Evidence: crates/engine/src/game/effects/collect_evidence.rs:178. Why it matters: Using deeply nested relative paths like super::super:: is fragile and breaks easily when files are moved. Using absolute paths starting with crate::game:: is more robust.

Suggested change

	super::super::casting::costs::pay_and_push(
	crate::game::casting::costs::pay_and_push(

[matthewevans]

/review-impl — PR #2815

Verdict: REQUEST CHANGES — one hard blocker (gittensory accidental file) + a misleading title. The actual code change (a casting-module reorg) is clean, mechanical, and behavior-preserving.

---

BLOCKER 1 — accidental gittensory git-submodule pointer committed at repo root

[HIGH] The diff adds gittensory as a gitlink / submodule entry, not a source file. Evidence: pr2815.diff final hunk —

diff --git a/gittensory b/gittensory
new file mode 160000
+Subproject commit a4429ce8bdf27ae36b6349308c0daf9258b85234

mode 160000 = an embedded git submodule reference. There is no matching .gitmodules, so this is a dangling gitlink that pollutes the tree and breaks git submodule operations on clone/checkout. The same author opened #2778 ("chore: ignore gittensory, …") — gittensory is a local tool/artifact that is meant to be gitignored, so this is an accidental git add of an untracked directory. It is not on origin/main (git ls-tree origin/main gittensory empty). It does not leak a local path and is not a large binary (it's a 1-line gitlink), but it must be removed. Fix: git rm --cached gittensory && git commit, and land #2778's ignore rule so this can't recur.

BLOCKER 2 — title does not describe the change

[MED] PR title: "refactor(engine): extract shared condition-evaluation helpers into ga…". The diff contains no condition-evaluation helper extraction anywhere — it is a structural reorg: casting.rs → casting/mod.rs, casting_costs.rs → casting/costs.rs, casting_targets.rs → casting/targets.rs, plus import-path updates (the body describes this correctly). A title that misrepresents the change pollutes git log/changelog automation and the squash-merge commit message. Fix: retitle to match the body, e.g. refactor(engine): move casting_costs/casting_targets into casting/ submodule.

---

Behavior-preservation (dominant lens for a refactor): PASS

Reviewed every +/- hunk via the API diff. The change is purely mechanical and preserves semantics:

• File moves (similarity index 96–99%) — no logic edits inside the moved bodies.
• Path edits only: super::X → super::super::X (files descended one module level) and super::casting::Y → super::Y (casting is now the parent). Spot-checked restrictions.rs:201 (cost_has_x), splice.rs, triggers.rs:3295 (extract_distribution_total), filter.rs:2041/2764 (cost_has_x), mana_abilities.rs:465, engine_*.rs — all resolve to the same symbols.
• Visibility correctly preserved: 7 balanced pub(super) → pub(in crate::game) conversions (e.g. costs.rs push_activated_ability_to_stack, finalize_cast, auto_tap_mana_sources; targets.rs extract_distribution_total). This is required for correctness — after descending a level, pub(super) would shrink to game::casting, so pub(in crate::game) restores the original game-wide scope. Correct and idiomatic.
• Remaining non-path diffs are rustfmt reflows caused by the longer super::super:: prefixes (match-arm pattern wrapping at costs.rs:130-134, un-wrapping at :402), not logic changes.
• casting/mod.rs correctly declares pub(crate) mod costs; / pub(crate) mod targets; and re-aliases costs as casting_costs to keep call-site churn minimal; game/mod.rs correctly drops the two flat pub(crate) mod declarations.

No subtle per-site semantic drift, no LKI-vs-current or player-scope change. Net -28 lines is genuine (collapsed use lists), not erased behavior.

Seam / authority check: PASS (and the PR title's premise is moot)

There is no new "condition-evaluation helper" and no second authority created — so the concern about drifting from parse_inner_condition / game/filter.rs / restrictions.rs does not apply; nothing in those authorities was touched. The reorg anchors on the existing effects/ submodule pattern (mod.rs + sibling files), which is the right house convention for a file this large (casting.rs is ~1.6 MB on disk). Correct seam.

Refuting Gemini's review

Gemini posted 3 × [MEDIUM] "Fragile relative imports — prefer crate::game:: over super::super::" (casting/costs.rs:22-36, casting/targets.rs:12-21, effects/collect_evidence.rs:178). Refuted as a defect: super::super:: is established house convention in this codebase — the sibling effects/ submodule (the very pattern this PR anchors on) already uses super::super:: in 10+ files / 31 occurrences. Both super::super:: and crate::game:: coexist throughout the engine, so the PR is internally consistent. Gemini's suggestion is a reasonable stylistic preference, not a correctness or consistency issue, and is not a merge blocker. (Optional polish, author's discretion.)

Verification

Did not check out the branch (main + ~15 concurrent agent worktrees present; reviewed via gh api .../2815.diff ground truth to avoid disturbing concurrent work). CI shows only the triage-label job green — the Rust clippy/test-engine jobs have not reported (new-contributor gate, or blocked by the gittensory gitlink at checkout). Because the change is mechanical path/visibility edits, compilation risk is low, but the maintainer must let full clippy + test-engine run green after the gittensory removal before merge — that is the equivalence proof here (the engine test suite exercises every moved casting::costs/casting::targets call site).

Summary

Land after: (1) remove gittensory gitlink, (2) retitle to match the actual reorg, (3) confirm clippy + test-engine green. The refactor itself is correct, idiomatic, and behavior-preserving.

[natefinch]

/review-impl — PR #2815

VERDICT: request-changes

The actual change — relocating casting.rs/casting_costs.rs/casting_targets.rs into a casting/ submodule (mod.rs/costs.rs/targets.rs) and rewriting super:: → super::super:: import paths — is mechanical and behavior-preserving. Two things block merge.

HIGH — Stray gittensory gitlink committed

Evidence: diff adds gittensory as new file mode 160000 → Subproject commit a4429ce8bd…, and there is no .gitmodules entry for it. Why it matters: a dangling submodule pointer breaks git submodule/clean checkout on CI and for every consumer; it is unrelated drift, not part of the reorg. Suggested fix: git rm --cached gittensory and drop the commit that introduced it before re-pushing. (Confirms @matthewevans's blocker.)

MED — Misleading PR title

Evidence: title reads "extract shared condition-evaluation helpers into ga…" but the body and diff are a pure casting-module reorganization with no condition-helper extraction. Why it matters: the title misrepresents the change to reviewers and pollutes the squash-merge history. Suggested fix: retitle to match the body (e.g. "refactor(engine): move casting_costs/casting_targets into casting/ submodule").

Process

PR is currently CONFLICTING against main and needs a rebase before it can land.

Notes (non-blocking)

• Visibility was handled correctly: push_activated_ability_to_stack was widened from pub(super) to pub(in crate::game) to preserve cross-module access under the deeper nesting, and costs/targets are re-declared pub(crate) with an internal use costs::{self as casting_costs} alias keeping call sites intact. No completeness gap found among other pub(super) items (none are referenced outside casting).
• Gemini's super::super:: "fragile relative imports" comments are a readability preference, not a blocker — the repo already mixes relative and absolute paths and clippy is green. Optionally switch the new super::super:: chains to crate::game::… for clarity, but it is not required for this PR.

[natefinch]

/review-impl — PR #2815 (re-review of 444fd58 / head 5b09d65)

VERDICT: approve with comments

The new commit materially changed the PR. Reconciling my prior request-changes (16:22:57Z):

HIGH gittensory gitlink — ADDRESSED ✅

The new file mode 160000 / Subproject commit … gitlink is gone from the current diff and is absent from origin/main (git ls-tree origin/main gittensory empty). The hard blocker is resolved.

Process: CONFLICTING — ADDRESSED ✅

PR is now MERGEABLE (mergeStateStatus BLOCKED is just unsatisfied required checks/review, not a merge conflict).

MED Misleading title — NOT ADDRESSED ❌ (only remaining issue)

Title + commit subject still read "extract shared condition-evaluation helpers into game/conditions.rs", but the diff contains no conditions.rs and no condition-helper extraction. It is a casting-module reorg: casting.rs → casting/mod.rs, casting_costs.rs → casting/costs.rs, casting_targets.rs → casting/targets.rs, plus super:: → super::super:: path rewrites and visibility preservation. Why it matters: the squash-merge uses the PR title as the commit subject, so this lands wrong history in git log/changelog automation. Must fix before merge (retitle the PR and amend the commit subject), e.g. refactor(engine): move casting_costs/casting_targets into casting/ submodule.

Current-diff review on its own merits — clean

• Behavior-preserving. All +/- hunks are file moves (similarity index 96–99%), import-path rewrites (super::casting_costs:: → costs::/super::casting::costs::, super::casting_targets:: → targets::), and rustfmt reflows from the longer prefixes. No logic edits in the moved bodies.
• Visibility correct. extract_distribution_total widened pub(super) → pub(in crate::game) — required after descending a level so triggers.rs/engine_stack.rs (game-level siblings) keep access. casting/mod.rs declares pub(crate) mod costs/targets and re-aliases costs as casting_costs; game/mod.rs drops the two flat decls. WASM + Tauri compile checks are green, confirming no cross-module visibility regressed.
• 27 new // allow-raw-authority: annotations are legitimate. They sit above pre-existing Keyword::X(cost) payload-extraction sites. scripts/check-engine-authorities.sh flags newly-added .keywords.iter( lines, and a rename re-presents these as added under the new path; the authority helpers (has_keyword_kind) answer presence, not "give me the cost inside the keyword", so the stated reason ("no authority helper for parameterized keyword data") is accurate. Not new logic — pre-existing behavior preserved verbatim by the move.

Refuting Gemini

Gemini's super::super:: "fragile imports" comments remain a readability preference, not a defect — super::super:: is established house convention (the sibling effects/ submodule this reorg mirrors uses it in 10+ files). Not a blocker.

Bottom line

The refactor is mechanical, idiomatic, and behavior-preserving; the only open item is the title/commit-subject mismatch (and letting clippy/test-engine/card-data finish green — currently pending). Approving with the title correction as a must-do before squash-merge.

[matthewevans]

Maintainer note: merged current origin/main (commit 231753ab4) — the three conflicts in casting/mod.rs were main's new effective_spell_keywords reads (landed via #2798) vs this branch's allow-raw-authority-annotated raw obj.keywords reads. Resolved decisively for main's side: the granted-keyword authority helper is the correct architecture, and the raw-authority annotations are now unnecessary at those sites. Conflict-area suites (issue_566, spectacle, dash) + cargo check --all-targets + clippy -D warnings green locally.

[matthewevans]

Maintainer sign-off: mechanical casting/ submodule reorg at the right boundary (mod.rs/costs.rs/targets.rs), gitlink blocker removed (natefinch re-review approve at this head); brought current with main, conflicts resolved in favor of the effective_spell_keywords authority. Behavior-preserving by construction; conflict-area suites green.

[matthewevans]

Thanks for taking a swing at the casting module's organization — the mechanical execution is clean (imports updated throughout, compiles, casting tests pass).

Closing this one, though, because as it stands it's a directory rename rather than the decomposition the branch name promises. It moves casting.rs → casting/mod.rs, casting_costs.rs → casting/costs.rs, and casting_targets.rs → casting/targets.rs with no logic changes — but casting/mod.rs is still a single ~42,000-line file (and costs.rs ~14,000). The actual maintainability problem is the size of those files, and a namespace move doesn't touch it.

The cost side is real too: casting.rs is one of the most frequently-modified files in the engine, and splitting one tracked file into a folder defeats git's rename detection — so this PR re-conflicts (and risks silently dropping changes during a no-conflict merge) every time another casting PR lands. We hit exactly that integrating it against #2811/#2799/#2788.

If you'd like to pursue this, a decomposition that actually splits mod.rs along cohesive seams would be genuinely valuable — e.g. cost calculation, payment resolution, casting permissions/timing, alternative costs, and target legality each in their own module, with mod.rs reduced to the public API surface. That's a bigger lift, but it's the version that earns back the merge friction. Happy to review that if you take it on.