fix: prevent duplicate experiment refunds

[ryanbarlow97]

Breaking a research lectern while its owner has an experiment item in the open menu returned that item twice: once during station cleanup and again from the inventory-close handler. The close handler now owns the refund and clears the slot before returning the item, so subsequent close handling cannot refund it again.

Validation: four regression cases reproduced duplicate refunds before the fix and pass afterward, covering station breaks and repeated close handling with available inventory space and overflow drops. Maven clean verify, JAR validation, and git diff --check passed. No live gameplay test has been performed.

Summary by CodeRabbit

• Bug Fixes
  • Fixed experiment item refunds when a research station is broken or its menu is closed. Items are now returned once, either to the player’s inventory or dropped at the station if the inventory is full.
  • The experiment slot is cleared after the item is returned, preventing it from appearing in the menu after closing. When a station is broken, its menu closes and the station is removed.

[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: d71691d6-40a9-4bb2-81ec-1ff71f1cce25

📥 Commits

Reviewing files that changed from the base of the PR and between 09d2f8e and 44ca6d7.

📒 Files selected for processing (3)

• pom.xml
• src/main/java/net/tfminecraft/research/manager/ResearchManager.java
• src/test/java/net/tfminecraft/research/manager/ResearchRefundTest.java

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

---

📝 Walkthrough

Walkthrough

The station-break path now relies on the inventory-close handler to return the experiment. The handler clears the experiment slot before returning the item. Parameterized tests cover station breaks, repeated close events, and full or non-full player inventories.

Changes

Experiment Refund Handling

Layer / File(s)	Summary
Refund flow and regression coverage src/main/java/net/tfminecraft/research/manager/ResearchManager.java, pom.xml, src/test/java/net/tfminecraft/research/manager/ResearchRefundTest.java	onInventoryClose clears the experiment slot before returning its item. scrapStation no longer returns the item directly before closing the GUI. Tests cover station breaks and repeated close events with full and non-full inventories. The build adds JUnit Jupiter, Mockito Core, and Maven Surefire.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 44ca6

No confirmed refund issue remains before merge. Normal checks are appropriate; live gameplay has not been tested.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 44ca6

The change prevents the demonstrated duplicate refund in the tested flows. It also makes station-break refunds depend on inventory-close handling; unusual close failures have not been validated in live gameplay. No new attacker path is demonstrated.

Retained concerns

• Low · reliability · inferred: Station-break cleanup now has no direct refund fallback. If closing the owner’s matching menu does not deliver its close event, station deletion can finish without transferring the experiment item. This is a conditional failure-containment risk, not a demonstrated normal-gameplay failure.

Security review details

Security Blast Radius

• inferred — The changed refund acts on the closing player’s transient menu item, not station-wide persistent item storage. A failed close-event handoff would affect that item; broader exposure is not established.

Trust Boundaries and Controls

• observed — Normal menu entry is owner-gated, and station scrapping closes the station owner’s menu only when its tracked location matches. The close handler does not repeat those identity checks, but this PR does not add that title-only trust decision.

Resilience and Maintainability Implications

• inferred — Clearing the slot before transfer protects against a repeated or reentrant close refund. The station-break path’s ownership handoff nevertheless depends on close-event delivery, which the tests assume rather than establish for interrupted teardown.

Hardening Proposals

• proposed — Validate close-event behavior during disconnect or interrupted menu teardown. If a fallback is needed, make it share the slot-clearing ownership rule so it cannot restore the duplicate refund.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 …	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: preventing duplicate experiment refunds. It matches the implementation and regression tests.
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.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.)

• 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 watched the station close
The slot was cleared before it goes
One item back, or dropped nearby
The tests check both as days go by
Then off I hop beneath the sky}

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

[ryanbarlow97]

Diff review: no blocking findings. Station cleanup now delegates the experiment refund to the existing close handler, which clears the slot before delivery. Overflow handling and station deletion remain intact.

Validation: all four regression cases failed with duplicate addItem calls on the original code and pass with this change. Local clean verify, runtime JAR validation, and the PR build pass. Tests simulate Bukkit inventory-close dispatch; a live gameplay reproduction has not been performed.

[ryanbarlow97]

CodeRabbit approved commit 44ca6d7 with no actionable comments. Its advisory about interrupted close-event delivery remains a live-test limitation; no such failure was demonstrated. The docstring-coverage warning is advisory and does not affect the passing build or refund regression tests. Keeping this patch scoped to the confirmed duplicate refund.