feat: implement Masterwork to Master rank trigger

[martincjarvis]

Summary

Implements Epic 4.8 - Masterwork → Master rank trigger functionality.

Changes

• Added MasterRankAchievedEvent to track when characters reach Master rank (level 10)
• Modified CraftingSystem to detect Masterwork outcomes and automatically grant Master rank
• Added comprehensive test coverage with 4 new tests in CraftingSystemMasterworkTests

Implementation Details

When a character crafts a Masterwork item:

1. The character immediately reaches Master rank (level 10) in all classes associated with that recipe's skill requirements
2. A MasterRankAchievedEvent is emitted with trigger reason "Masterwork"
3. Characters already at or above Master rank do not emit duplicate events

Test Coverage

• ✅ Masterwork craft grants Master rank to novice
• ✅ Masterwork craft emits MasterRankAchievedEvent
• ✅ Already-Master characters don't get duplicate events
• ✅ Non-masterwork crafts don't grant Master rank

Verification

• All 515 tests pass (88 Simulation + 427 Core)
• Build succeeds with --warnaserror (zero warnings)
• Follows TDD approach (tests written first)

Closes #39

🤖 Generated with Claude Code

[github-actions]

	Messages
📖	✅ Issue properly linked: Closes #39
📖	✅ Summary section found
📖	✅ PR title follows Conventional Commits format

Recommended Persona Reviews

💻 .NET Specialist - C# code changes detected

See docs/roles/ for persona details.

---

Danger analysis complete for feat: implement Masterwork to Master rank trigger. 5 files changed, 3 commits.

Generated by 🚫 dangerJS against e128622

[mcj-coder]

Code Review: Epic 4.8 - Masterwork to Master Rank Trigger

Strengths

✅ Clean Event Design - MasterRankAchievedEvent follows established patterns perfectly

✅ Comprehensive Test Coverage - 4 well-structured tests covering happy path, events, edge cases, and negatives

✅ Correct XP Calculation - Properly sums XP requirements to reach level 10

✅ Proper Integration - Cleanly integrates without disrupting existing crafting flow

✅ Multi-Class Support - Correctly handles recipes with multiple skill requirements

✅ Zero Build Warnings - Maintains project's zero-warning policy

---

Issues to Address

Important (Should Fix Before Merge)

1. Duplicate Class Progress Updates

Location: CraftingSystem.cs:88-89, 125

Issue: The code performs three separate updates to classProgress and updatedCharacter (lines 79-88, 91-124, 130), creating unnecessary intermediate states.

Fix: Consolidate all class progress modifications before creating the final updatedCharacter. Accumulate all XP changes (normal + Masterwork) into a single classProgress variable, then apply it once at the end.

2. Magic Constant Duplication

Location: CraftingSystem.cs:93

Issue: const int MasterRankLevel = 10 is defined locally, duplicating knowledge that should be shared.

Fix: Extract to a class-level constant or (preferably) add to ClassProgressCollection as public const int MasterRankLevel = 10.

---

Recommendations (Optional)

💡 Consider extracting Master rank logic to a dedicated MasterRankRule class (following the pattern of CraftingOutcomeRule and XpCalculator)

💡 Add integration test verifying complete flow including event ordering

💡 Add XML documentation explaining the game design rationale for Masterwork → Master rank

---

Assessment

Status: Request Changes (Important issues should be fixed)

Verdict: Implementation is functionally correct and fully tested. The Important issues are code quality improvements for maintainability, not correctness bugs.

Test Results: ✅ 515 tests passing (427 Core + 88 Simulation), 0 warnings

Plan Alignment: ✅ All Epic 4.8 requirements met

Once the Important issues are addressed, this will be ready to merge with auto-merge enabled.

[mcj-coder]

Fixed. Changes verified - ready for re-review.

Changes Made:

• Consolidated class progress updates into single variable (eliminates duplicate updates)
• Extracted MasterRankLevel constant to ClassProgressCollection (eliminates magic number duplication)
• Added missing using statement

Verification:

• Build: 0 warnings
• Tests: All 515 tests passed (427 Core + 88 Simulation)

[mcj-coder]

Re-Review: Fixes Applied Successfully ✅

All Important issues from the previous review have been addressed.

Issues Resolved

✅ Issue #1: Duplicate Class Progress Updates

• Consolidated all modifications into single classProgress variable
• Single immutable update at end: character.WithClassProgress(classProgress).WithActiveCraft(null)
• Clean, idiomatic pattern for immutable data structures

✅ Issue #2: Magic Constant Duplication

• Extracted MasterRankLevel = 10 to ClassProgressCollection.MasterRankLevel
• Added XML documentation explaining Master rank threshold
• Updated all references in production and test code

Additional Improvements

• Added proper using statement for ClassProgressCollection
• Updated comments to reference "Master rank" terminology
• Consistent constant usage across codebase

Verification

✅ All 515 tests passing (427 Core + 88 Simulation)
✅ Build succeeds with --warnaserror (zero warnings)
✅ All CI checks passing

Assessment

Status: Approved ✅

Ready to merge: Yes - enabling auto-merge now.

Excellent work addressing the feedback promptly and thoroughly!