Skip to content

Cancel a maintenance pay request after one vehicle click - #66

Merged
ryanbarlow97 merged 1 commit into
mainfrom
fix/maintenance-pay-session-one-shot
Sep 27, 2026
Merged

ryanbarlow97 merged 1 commit into
mainfrom
fix/maintenance-pay-session-one-shot

Conversation

@ryanbarlow97

@ryanbarlow97 ryanbarlow97 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

After /faction vehicle maintenance pay bank, a failed payment left the pay request armed. Reasons for failure include "This vehicle has no unpaid maintenance", insufficient funds and an unregistered vehicle. Every right-click on a vehicle was then cancelled and repeated the error until the request timed out (transfer-request-timeout-seconds, 60 s), so the player felt stuck.

Fix

VehicleMaintenancePayListener now clears the session after any payment attempt, not just a successful one. This matches the transfer and release sessions. To retry, the player runs the command again.

The payment step moved into a package-private pay(...) method. ModelEngine is not on the test classpath, so ActiveVehicle cannot be mocked.

Tests

  • New failedPaymentDisarmsSessionSoLaterClicksReachTheVehicle in VehicleMaintenanceBankCommandTest.
  • The full mvn test suite passes locally.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Vehicle maintenance payment sessions are now cleared after every payment attempt, including unsuccessful attempts, preventing stale sessions from affecting later vehicle interactions.

The pay session was only cleared when the payment succeeded. A failed
attempt (no unpaid maintenance, insufficient funds or an unregistered
vehicle) left it armed, so every vehicle right-click was cancelled and
repeated the error until the 60-second timeout.

Clear the session after any payment attempt, matching the transfer and
release sessions. Players rerun the command to try again.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: 5455dcca-ec4b-4f1e-927e-edeffdc9b562

📥 Commits

Reviewing files that changed from the base of the PR and between e5aa57e and ec3a0eb.

📒 Files selected for processing (2)
  • src/main/java/net/tfminecraft/simplefactions/vehicles/maintenance/VehicleMaintenancePayListener.java
  • src/test/java/net/tfminecraft/simplefactions/vehicles/maintenance/VehicleMaintenanceBankCommandTest.java

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


📝 Walkthrough

Walkthrough

The listener now passes the vehicle UUID and type ID to tryPay and clears the payment session for every result. A test covers the NOT_UNPAID outcome and a subsequent vehicle interaction.

Changes

Vehicle maintenance payment handling

Layer / File(s) Summary
Payment attempt and session handling
src/main/java/net/tfminecraft/simplefactions/vehicles/maintenance/VehicleMaintenancePayListener.java, src/test/java/net/tfminecraft/simplefactions/vehicles/maintenance/VehicleMaintenanceBankCommandTest.java
The listener passes the vehicle UUID and type ID to tryPay and clears the payment session before handling the result. The test verifies that a NOT_UNPAID result notifies the player, clears the session, and prevents another payment attempt on a later interaction.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: drefvelin

Merge Risk: ⚪ Minimal · up to ec3a0

Failed payments no longer keep intercepting vehicle clicks. No actionable issue remains before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ec3a0

A failed payment now ends the armed request, so later vehicle clicks are no longer intercepted by that request. The review found no demonstrated new privilege or broader payment exposure, but some exceptional and overlapping-payment behavior remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed interaction remains gated by the interacting player's armed session, and withdrawal uses that player's UUID. The observed change affects how long that payment request remains active, not which account is passed for withdrawal.

Trust Boundaries and Controls

  • observed — The handler takes the player and vehicle from the interaction event, requires an active session for that player, and passes the vehicle identifiers to a service that rejects vehicles without unpaid maintenance and unknown vehicle types.

Resilience and Maintainability Implications

  • inferred — The existing check-then-withdraw-then-clear sequence is not visibly atomic, and the listener removes a session by player UUID after payment returns. Overlapping calls could therefore matter, but production overlap and any PR-worsened security exposure are unverified.

Hardening Proposals

  • proposed — If payment callbacks can overlap, consider consuming the specific armed session before processing and protecting withdrawal and unpaid-record settlement against duplicate attempts. This addresses an unverified pre-existing concurrency exposure, not an established regression in this PR.
🚥 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 4 functions across 2 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 describes the main behavior change: a maintenance payment request is cancelled after one vehicle click, including failed payment attempts.
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 watched the payment flow,
Then saw the session clear and go.
“No second try,” the rabbit said,
And hopped along with ears ahead.
The vehicle rolled; the path stayed free.

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

@ryanbarlow97
ryanbarlow97 merged commit f3e518f into main Sep 27, 2026
2 checks passed
@ryanbarlow97
ryanbarlow97 deleted the fix/maintenance-pay-session-one-shot branch September 27, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant