fix: keep points spent when a player removes a profession upgrade

[Drefvelin]

Removing a profession upgrade gave its points back, so players could respec for free, even though the confirm prompt says "Points are not refunded!". The cause: free points were computed as lifetime points minus the cost of currently held upgrades.

Changes

• RPCharacter tracks forfeitedProfessionPoints per profession (lowercase id). getSpentPointsOnProfession includes them, so a removed upgrade's cost stays spent, and buying it again costs full price.
• /profession confirm (the player's own removal) forfeits the points.
• Forfeits don't count towards getTotalSpentPoints, the max-upgrades cap. Removing an upgrade still frees a slot, just not the points.
• Admin tools are unchanged: removeupgrade still refunds (for fixing mistakes), and refund also clears forfeits.
• Persisted as the optional forfeited-profession-points object on each character. Old data loads with no forfeits.

Tests

• New ProfessionUpgradeForfeitTest. mvn package passes.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Profession points spent on removed upgrades can now be tracked as forfeited and saved between sessions.
  • Player-confirmed upgrade removals forfeit the points spent; command-based removals do not.
  • Refunding a character clears their forfeited profession points.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: bf640d94-9a36-4fa8-b7a3-4644ecec4e9d

📥 Commits

Reviewing files that changed from the base of the PR and between 47efd2f and 28968de.

📒 Files selected for processing (4)

• src/main/java/net/tfminecraft/rpcharacters/database/Database.java
• src/main/java/net/tfminecraft/rpcharacters/objects/RPCharacter.java
• src/main/java/net/tfminecraft/rpcharacters/professions/ProfessionCommandHandler.java
• src/test/java/net/tfminecraft/rpcharacters/professions/ProfessionUpgradeForfeitTest.java

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

Characters now track forfeited profession points, and profession spending includes those points. Confirmed player upgrade removals record forfeits, while admin removals do not. Refunds clear forfeits, and the database loads and saves them.

Changes

Profession Upgrade Forfeits

Layer / File(s)	Summary
Track forfeited profession points src/main/java/net/tfminecraft/rpcharacters/objects/RPCharacter.java, src/test/java/net/tfminecraft/rpcharacters/professions/ProfessionUpgradeForfeitTest.java	RPCharacter stores forfeited points by profession and includes them in profession spending. Tests cover forfeiting held and unheld upgrades, repurchasing, ordinary removal, key normalization, and clearing totals.
Apply forfeits through removal commands src/main/java/net/tfminecraft/rpcharacters/professions/ProfessionCommandHandler.java	Admin removal does not forfeit points. Confirmed player removal does. Refunding an active character clears forfeited points.
Persist forfeited points src/main/java/net/tfminecraft/rpcharacters/database/Database.java	Loading adds numeric values from forfeited-profession-points. Saving writes the map when it is nonempty.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: ryanbarlow97

Merge Risk: ⚪ Minimal · up to 28968

Player removals retain spent profession points, while admin removals and refunds remain distinct. No actionable merge-blocking risk is established.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 28968

The normal removal and rebuy flow keeps points spent, but a confirmation can apply to a different active character, and later profession configuration changes can alter how a forfeiture is valued. The demonstrated effects are confined to a player’s characters; no cross-player access was established.

Retained concerns

• Medium · security · inferred: A pending removal is bound to the player and upgrade, not the character that showed the prompt. If the player switches to another character holding that upgrade before confirming, the newly introduced forfeiture is charged to the other character as well as removing its upgrade. The wrong-character removal was possible before this PR; the persistent point penalty is new.
• Low · architecture · inferred: The new durable forfeiture records the definition’s cost and profession at removal, not at purchase. If operators change those definitions, the balance need not represent the points originally spent. Held-upgrade accounting already used live definitions; this concern is the historical meaning of the newly persisted balance, not unauthorized configuration access.

Security review details

Security Blast Radius

• inferred — The identified confirmation path can affect another active character of the same player if it holds the same upgrade. The inspected path does not resolve another player’s data, so cross-player mutation was not established.

Security Findings and Attack Paths

• inferred — A player can initiate removal on one character, switch to another that holds the same upgrade, and confirm while the request is pending. The active-character check permits removal there, and the new forfeiture branch charges that character. The pending entry is consumed and expires, limiting repetition.

Trust Boundaries and Controls

• observed — Only a Player sender can confirm; the pending entry is retrieved for that player, and removal checks the player’s active character and held upgrade. Privileged correction routes separately check admin status.

Resilience and Maintainability Implications

• inferred — A failed save or interruption after permission and character mutation may leave runtime permissions and durable upgrade accounting out of step. No new cross-player authority follows from the inspected ordering, and the precise recovery outcome is unverified.

Hardening Proposals

• proposed — Bind pending removal to a character ID as well as the player and upgrade, then recheck that ID at confirmation.
• proposed — Define whether forfeits preserve purchase-time or current-config value before changing profession costs or IDs; if purchase-time value is required, persist sufficient provenance and plan reconciliation for existing balances.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving profession points when a player removes a profession upgrade.
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{A rabbit checks the point totals with care
Then hops where the upgrade once sat
The spent points stay counted and saved
A fresh purchase adds to the tally
The burrow keeps each record in line}

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