docs(custom-format): design pass for CombatDamageTiming::OnStack (combat damage on the stack)

[rykerwilliams]

Summary

This is the design pass for CombatDamageTiming::OnStack, the last LegacyRuleSet axis. PLAN.md §6–§8 and IMPLEMENTATION_PLAN.md require a design before this axis is built. The PR is docs only, with no code: it adds docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md and a README entry.

Two findings correct the merged charter:

• Era: the charter calls this rule "pre-6th-edition". The archived Comprehensive Rules show combat damage on the stack was introduced by Classic Sixth Edition (1999) and removed by M10 (July 2009). §1 quotes the 1999 and 2009 text verbatim.
• Numbering: the pre-M10 combat damage step is rule 310, which is Battles in the current CR. §8 sets a convention so historical numbers never reach engine CR annotations.

Design in brief

Model. Each combat damage sub-step puts one StackEntryKind::CombatDamage object on the stack. It holds the frozen assignments, keyed by source and recipient incarnation. It resolves through the existing apply_combat_damage batch; nothing in that path is duplicated.

Classification and control. A new StackObjectClass { Spell, Ability, CombatDamage } makes the object untargetable by counters and Stifle. The class also carries the controller, and combat damage has none: 2009 600.4a says "A player leaving the game doesn't affect combat damage on the stack", and current CR 800.4a would otherwise erase pending damage.

Dealing. Rules 310.4a–c are applied at resolution:

• A source still on the battlefield uses its live characteristics.
• A source that has left uses last-known information for the frozen incarnation.
• Damage to a recipient that has left is not dealt.

Source identity. An incarnation-aware damage-source identity (D10) runs through protection, prevention filters, damage events, trigger matchers and the Circle of Protection "choose a source" shield.

Correctness fixes found in review:

• A typed origin on the parked lifelink batch, so resuming it can't skip the priority window after resolution.
• CR 500.6 step-start triggers are put on the stack above the damage object.
• An empty first-strike sub-step still gets its CR 510.3 priority window.

Phases (§9), each its own PR:

• 3a: stack object and classification. No behavior change.
• 3b: damage-source identity. No behavior change, with an explicit noncombat stamping rule and a pinning test.
• 3c: OnStack behavior, with 18 paired tests (each OnStack result checked against a Modern control).
• 3d: flip the gate and register middle_school().

Decision recorded (Q4). Classic Magic's operative ruleset is its enumerated "Differences from Modern" list, so the design fits it. Its one gap, format-scoped text for Time Vault and Illusionary Mask, is outside this axis. Middle School registers first. Classic Magic is deferred to its own PR that adds those overrides.

Review

The design went through three rounds, each pairing an architecture review (review-engine-plan, checked against source) with an adversarial fact-check of the rules premises against the archived 1999/2009 CR and the Eternal Central pages:

Round	Architecture verdict	Main changes
1	REVISE	lifelink resume, step-start triggers, a typed dealing record, incarnation-keyed LKI
2	REVISE	trigger ordering, source identity reaching protection and prevention, Option controller, a fourth phase
3	APPROVE WITH CHANGES	all changes applied

§11 maps every finding to the change it produced.

Test plan

• Every current-CR number cited was grep-verified against docs/MagicCompRules.txt.
• Historical rules quotes were verified against the archived CR text by an independent reviewer.
• Every file:line citation is pinned to 05c27e0d5.
• No code change, so there is nothing to build or test. Each implementation phase will get its own plan review and implementation review.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Documentation
  • Added a design proposal documenting historical “combat damage on the stack” timing from Sixth Edition through M10.
  • Describes how combat damage would interact with priority, changing sources or recipients, prevention, protection, lifelink, and triggers.
  • Defines a phased implementation plan, including planned support for the Middle School format.
  • Updated the custom format engine documentation to reference the new proposal.

[coderabbitai]

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: afc522d8-48c3-4ead-9f6b-51bdc3244c28

📥 Commits

Reviewing files that changed from the base of the PR and between 46a8cbb and 8808e9d.

📒 Files selected for processing (1)

• docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: eca8e587-0e89-4bff-b31e-6c5da0203d1d

📥 Commits

Reviewing files that changed from the base of the PR and between 9d919c6 and 46a8cbb.

📒 Files selected for processing (2)

• docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md
• docs/proposals/custom-format-engine/README.md

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

This PR adds a design specification for pre-M10 combat damage on the stack. It defines the proposed engine changes, compatibility surfaces, phased implementation plan, review decisions, and README reference. No implementation code changes are included.

Changes

Combat Damage on Stack

Layer / File(s)	Summary
Rules and engine scope docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md	Documents the historical rules, current combat-damage flow, integration seam, scope decisions, and current-CR citation requirements.
Stack behavior and compatibility docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md	Defines the stack object, resolution semantics, source identity, classification, serialized state, frontend representation, AI handling, and tracked risks.
Implementation plan and validation docs/proposals/custom-format-engine/COMBAT_DAMAGE_ON_STACK.md, docs/proposals/custom-format-engine/README.md	Defines four implementation phases, resolves open questions, records review rounds and research inputs, and adds the design document to the proposal index.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 46a8c

The proposal and index entry can be merged without introducing runtime behavior changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the documentation change and its main subject: the design for CombatDamageTiming::OnStack, or combat damage on the stack.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthewevans]

Changes requested for current head 46a8cbb9e2fd0c77f633f8a8f03aef16d4d4e6e4.

MED — empty combat-damage assignment still needs the historical stack object. The proposal quotes 310.1 at COMBAT_DAMAGE_ON_STACK.md:93-110: all combat-damage assignments go on the stack as one object, and 310.4 resolves/removes that object. But :427-434 and :1052-1063 deliberately push no object for zero/no assignments. A 0- or negative-power creature still assigns combat damage under 310.2a, so preserving only the priority window is not rules-faithful and omits an observable stack state. Require an inert CombatDamage entry for every assignment step, with a zero/negative-power regression proving it appears and resolves, or explicitly narrow/prevent exposure of this axis.

[rykerwilliams]

Thanks. Agreed: a 0- or negative-power creature still assigns under 2009 310.2a, so the object exists and players could see it. Addressed in 8808e9d93.

• Always one object per combat damage step (§3.2). OnStack pushes exactly one CombatDamage entry per step, unconditionally. That includes an inert one, whether its assignments are all 0 or its list is empty (for example, every blocker was removed). There is no empty branch anymore, which also removes the round-3 C1 re-entry hazard.
• Zero-amount assignments are recorded under OnStack. The combat_damage_amount == 0 → continue skip (combat_damage.rs:768, :938) becomes an exhaustive match on the policy:
  • Modern keeps the CR 510.1a skip.
  • OnStack records a 0-amount entry per 310.2a. A 0 amount has only one possible division, so no assignment prompt is raised.
  • Creatures under "assigns no combat damage", or with no legal recipient (310.2b–d), make no entry.
• Resolution: a 0-amount entry deals nothing, so no DamageDealt event, no damage triggers and no prevention event (current CR 120.8 / 2009 419.5a).
• §1.4: a new row records that OnStack follows 2009 310.2a over CR 510.1a here, because the assignment is part of the observable stack object.
• Regression tests:
  • New test 3c-19: a power-0 and a power −2 attacker. Under OnStack, exactly one entry with 0-amount assignments appears and resolves with no damage events. The Modern control has no entry. Mutation proof: restoring the skip under OnStack fails the entry assertion.
  • Tests 3c-10 and 3c-12: now assert that the inert first-strike object appears and resolves before the regular step, and that step-start triggers sit above it.
• Q1 is closed, and §11 has a PR-review row mapping this finding to the change.

[matthewevans]

The current-head design review at 8808e9d9338588ff458784cdb8a60b2b9bcba3d5 is clean: the zero/no-assignment combat-damage stack object requirement is addressed.

This is not approved or enqueued yet because the required Rust and Frontend CI aggregates are still pending. Once those checks are terminal and green on this head, we will replace the stale requested-changes review and proceed with final approval/enqueue.

[rykerwilliams]

CI is terminal and green on the current head 8808e9d9338588ff458784cdb8a60b2b9bcba3d5 — 18 checks reported, 16 success and 2 skipped (Draft pools smoke, Tauri), with none pending and none failing. Both aggregates you named are in that set: Rust (fmt, clippy, test, coverage-gate) and Frontend (lint, type-check, test), alongside the four Rust test shards, Rust lint, WASM, card data, lobby worker, and the Android build.

The outstanding CHANGES_REQUESTED review is pinned to the superseded head 46a8cbb9e — the zero/no-assignment stack-object finding it raised was addressed in 8808e9d93, which you confirmed reads clean.

No further pushes are planned on this branch, so the head should stay put for the review swap.

[matthewevans]

Approved on current head 8808e9d: the prior zero/no-assignment stack-object finding is resolved, this docs-only design is internally consistent, and required CI is green.