feat: parcel return logic (GH-69)

[Jakubk15]

Closes #69.

Players can return collected parcels by depositing the same items back at any locker. The returned parcel ships back to its original entry locker like a normal parcel.

How it works

• Collecting a parcel now marks it COLLECTED (new status) instead of deleting it, and keeps its content row as the collection snapshot used to validate a later return.
• The locker GUI gains a Return button → list of returnable parcels (with remaining return-window time in the lore) → per-size deposit GUI where the player puts the items back and confirms.
• Deposited items are validated against the snapshot with configurable strictness: checkDurability, checkItemName, checkLore, checkEnchantments, checkNbt (all true by default = items must be identical; relaxing flags loosens individual attributes).
• A per-size return fee (5.0 / 12.5 / 25.0 by default) is charged, bypassed by parcellockers.fee.bypass.
• On success the parcel is reversed (sender ↔ receiver, entry ↔ destination lockers), flipped back to SENT, and delivered with the normal priority-aware delay.
• Returns are time-limited (parcel-return-window, default 7 days). A background task purges expired collected parcels (parcel + content + collected row) every 30 minutes.

Correctness notes

• All status flips are conditional UPDATEs (markCollected only from DELIVERED, markReturned only from COLLECTED), so double-collect, double-return, return-vs-purge and return-vs-admin-delete races resolve safely; covered by integration tests.
• ParcelSendTask now aborts unless the parcel is still SENT, preventing re-delivery duplication across restarts.
• The post-return "commit point" in ParcelReturnService is structurally isolated from the refund/give-back recovery path (including synchronous throws), so a committed return can never be undone into duplicated items.
• The admin status toggle refuses to change a COLLECTED parcel's status (it would otherwise re-arm delivery of already-handed-out items).
• Return-window expiry is compared temporally in Java rather than via SQL string comparison (InstantPersister stores ISO-8601 strings, which don't order correctly lexicographically).

Config additions

parcel-return-window, small/medium/large-parcel-return-fee, return-checks.* (5 flags), new GUI items/titles for the return list and deposit GUIs, and new notices (returned, cannotReturn, returnItemsMismatch, returnWindowExpired, returnFeeWithdrawn, admin.statusLocked).

Testing

• 40 unit tests across 8 classes, all green locally (equivalence, validator, return-window logic, send-task guard, admin guard, and existing suites).
• ⚠️ 3 Testcontainers integration test classes (collected-parcel repository, conditional collect/return flips — the race guards) have not run yet: locally blocked by a machine-specific Docker port-relay conflict (wslrelay.exe vs com.docker.backend loopback binding), and the CI workflow currently runs only shadowJar, not ./gradlew test. They should be run once (locally after a wsl --shutdown, or by adding a test step to CI) before merge.
• runServer smoke test passed: plugin enables cleanly, collected_parcels table created, no errors.

Notes

• Branch is based on fix/issue-222 (fix/issue 222 #230, merged) because this feature reworks the same collect flow.
• Went through subagent-driven development with per-task review plus a final whole-branch review; both review blockers (admin-toggle resurrection, post-commit exception routing) are fixed in the last two commits. A small batch of follow-up cleanups (cosmetic notices, stale-row cleanup in purge, a narrow stale-admin-GUI race) will be filed as a separate issue.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive parcel return system, including return validation, configurable item equivalence checks, database persistence for collected parcels, a periodic purge task for expired returns, and dedicated return GUIs. The code review highlights critical thread-safety issues with asynchronous Vault economy calls, potential NullPointerExceptions due to missing null/air checks in item validation, a potential ConcurrentModificationException when modifying item enchantments, and a performance concern regarding full table scans for expired parcels.

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.

[Jakubk15]

@claude review

[claude]

Claude finished @Jakubk15's task in 2m 21s —— View job

---

Claude's Review

• Gather context (PR description, diff, prior Gemini review comments)
• Check whether prior Gemini review findings were already addressed by recent commits
• Review new return-flow code for correctness/thread-safety/race issues
• Review GUI, config, and repository/purge-task changes
• Post final review

Prior Gemini findings — status

Finding	Status
withdrawPlayer on async thread (ParcelReturnService)	✅ Fixed in 0131680 — now wrapped in this.scheduler.complete(...)
depositPlayer on async thread (ParcelReturnService.refund, line 260)	✅ Fixed in 0131680 — now wrapped in this.scheduler.run(...)
Full table scan in CollectedParcelRepositoryOrmLite.findExpired	Not changed, but this is a deliberate, documented tradeoff (line 41-44 comment) — the table only ever holds parcels inside the return window, and the purge runs every 30 min, so the scan cost is bounded and negligible. Not a blocker.
Missing null checks in ReturnItemEquivalence.test / ParcelReturnValidator.matches	Not changed. Traced the only call site (ReturnDepositGui.takeDepositedItems, line 99-106) — it already filters null/isEmpty() items before they ever reach the validator, so this isn't currently reachable. Still a reasonable defensive hardening for a public API that could gain other callers later — low priority, non-blocking.
Possible ConcurrentModificationException in ReturnItemEquivalence.normalize (enchant removal)	Likely a false positive: ItemMeta#getEnchants() returns a defensive copy (ImmutableMap.copyOf(...)) in the CraftBukkit/Paper implementation, so mutating meta via removeEnchant while iterating the returned keySet() doesn't touch the same backing map. The new ArrayList<>(...) wrap costs nothing and is a cheap hedge, but not required for correctness with current Bukkit APIs.

Own review of the return flow

Went through ParcelReturnService, ParcelServiceImpl.collect, ParcelRepositoryOrmLite (conditional markCollected/markReturned/updateIfStatus), AdminParcelService.changeStatus, ParcelSendTask, and ReturnWindowPurgeTask. This is careful, well-reasoned code:

• All state transitions are conditional UPDATEs guarded by expected status (markCollected only from DELIVERED, markReturned only from COLLECTED, updateIfStatus for the admin-edit guard), so the double-collect/double-return/return-vs-purge/return-vs-admin-edit races are closed at the DB layer, not just in-memory.
• The post-commit section of ParcelReturnService.proceedWithReturn (ParcelReturnService.java:186-236) is correctly isolated from the refund/give-back path with a try/catch that only logs — a nice bit of defense against a partially-applied return duplicating items.
• ParcelSendTask.decide aborting when the parcel isn't SENT correctly prevents re-delivery after a restart.
• AdminParcelService.changeStatus refusing to touch COLLECTED parcels, combined with updateIfStatus for the TOCTOU between GUI-open and edit-submit, closes the "resurrect a collected parcel" issue called out in the PR description.
• countParcelsByDestinationLocker correctly excludes COLLECTED parcels from locker occupancy (items have already left).
• ReturnWindowPurgeTask deletes the parcel/content first and only removes the collected_parcels row after that succeeds, so a partial failure is safely retried on the next run rather than leaking or double-deleting.

No new correctness issues found beyond what's listed above — nothing here blocks merging on my read. The remaining integration tests (blocked locally by the Docker port-relay conflict mentioned in the PR description) are the main outstanding item before merge, as called out by the author.