Skip to content

Harden vote preset loading and UI scheduling - #1681

Merged
BenCodez merged 1 commit into
masterfrom
fix/vote-presets-threading-bounds
Sep 30, 2026
Merged

BenCodez merged 1 commit into
masterfrom
fix/vote-presets-threading-bounds

Conversation

@BenCodez

@BenCodez BenCodez commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • keep VotePresets network work on the existing asynchronous CommandHandler execution lane
  • hand ValueRequest/player UI work back to the player's Bukkit/Folia scheduler lane
  • bound GitHub preset directory responses to 256 KiB
  • bound individual preset responses to 64 KiB
  • cap discovered VoteSite presets at 64
  • only accept direct presets/votesites/*.meta.json paths

Scope

This intentionally does not add a bundled preset snapshot, cache, retry state, or a second async scheduling layer.

CommandHandler.runCommand(...) already dispatches command execute(...) asynchronously, so the fix is limited to returning player/UI interaction to the correct lane and hardening the remote response parsing.

Tests

  • validates path filtering and preset-count bounds
  • verifies preset UI completion is scheduled onto the player lane

Validation

  • source-read-only review of the exact intended diff: no findings
  • GitHub Actions: mvn -B -f VotingPlugin/pom.xml package passed on head 1c01b14ad684d65f05a42d462dcc996bc8894142
  • 1,676 tests, 0 failures, 0 errors
  • packaged VotingPlugin JAR: 10,453,982 bytes, under the 10 MiB package limit

Summary by CodeRabbit

  • New Features
    • The “VotePresets (Text)” command can now find a preset matching a vote-site URL and guide you through entering any required placeholders.
  • Bug Fixes
    • Preset listings now exclude invalid paths and report an error if too many presets are returned.
    • Errors during preset lookup or loading are surfaced to players, and interrupted operations are handled without silently continuing.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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: 097800b0-245c-403f-8dc3-fb27042275eb

📥 Commits

Reviewing files that changed from the base of the PR and between d3c6931 and 1c01b14.

📒 Files selected for processing (4)
  • VotingPlugin/src/main/java/com/bencodez/votingplugin/commands/CommandLoader.java
  • VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/GitHubVoteSitePresetLoader.java
  • VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/VoteSitePresetSetupHandler.java
  • VotingPlugin/src/test/java/com/bencodez/votingplugin/presets/VoteSitePresetSafetyTest.java

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

📜 Recent review details
🔇 Additional comments (4)
VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/GitHubVoteSitePresetLoader.java (1)

29-34: LGTM!

Also applies to: 70-78, 84-84, 89-91, 100-108, 114-114, 186-215

VotingPlugin/src/test/java/com/bencodez/votingplugin/presets/VoteSitePresetSafetyTest.java (1)

1-60: LGTM!

VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/VoteSitePresetSetupHandler.java (1)

46-51: LGTM!

Also applies to: 67-70, 73-73, 77-104

VotingPlugin/src/main/java/com/bencodez/votingplugin/commands/CommandLoader.java (1)

2775-2775: LGTM!


📝 Walkthrough

Walkthrough

The GitHub preset loader now bounds responses, validates preset paths, and limits accepted presets. The setup handler schedules player-facing results and handles URL-based lookup. The command delegates URL lookup to the handler.

Changes

Vote preset loading and setup

Layer / File(s) Summary
Bound and validate preset loading
VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/GitHubVoteSitePresetLoader.java, VotingPlugin/src/test/java/com/bencodez/votingplugin/presets/VoteSitePresetSafetyTest.java
The loader applies request timeouts and response-size limits. It accepts only matching vote-site preset paths and throws IOException when the accepted preset count exceeds 64. Tests cover path filtering and the count limit.
Schedule preset setup and URL lookup
VotingPlugin/src/main/java/com/bencodez/votingplugin/presets/VoteSitePresetSetupHandler.java, VotingPlugin/src/main/java/com/bencodez/votingplugin/commands/CommandLoader.java, VotingPlugin/src/test/java/com/bencodez/votingplugin/presets/VoteSitePresetSafetyTest.java
The setup handler schedules player-facing results and restores the interrupt flag when loader operations are interrupted. The command delegates URL lookup to the handler. A test verifies that setup lists presets and schedules a task for the player.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 1c01b

The change bounds remote preset loading and schedules player-facing results without changing the lookup execution lane. No actionable merge-blocking issue was identified; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1c01b

The change narrows accepted remote input and schedules player-facing completion without changing administrator permissions. No introduced security issue was established, but runtime scheduling and callback guarantees remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected command path retains administrator permission routing and affects server-wide vote-site configuration, rather than only the invoking player's state. Remote repository content remains a trusted input to that configuration authority; this exposure predates the PR.

Trust Boundaries and Controls

  • observed — The command-supplied URL is parsed for host matching, not fetched as a request destination. Requests use constructed GitHub API and raw-content URLs. The existing client follows normal redirects and optionally supplies a token; cross-origin redirect credential behavior remains unverified, while the default setup constructor supplies no token.

Resilience and Maintainability Implications

  • observed — Completion uses an at-most-once entity/fallback guard and an empty global fallback, avoiding deliberate player UI or preset application on the global retirement path. Shared preset application still consists of multiple configuration writes without a transaction or operation-level duplicate guard. That implementation and repeated-command exposure predate the PR; whether the new player lane changes effective serialization depends on unresolved external execution and callback contracts.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 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 accurately summarizes the main changes: stronger vote preset loading safeguards and improved UI scheduling.
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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@BenCodez
BenCodez marked this pull request as ready for review September 30, 2026 00:15
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
🔒 Security Review ✅ Completed 2026-09-30T00:20:50.109446Z 1c01b14 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@BenCodez
BenCodez merged commit 5e86b79 into master Sep 30, 2026
6 checks passed
@BenCodez
BenCodez deleted the fix/vote-presets-threading-bounds branch September 30, 2026 01:42
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