Add quest reward grant events and custom objective data persistence#305
Conversation
#282) - Add the tutorial chain + its 7 quests, the example chain, and the seven owner-facing guide markdown files to QuestManager's default-extraction list so they land in plugins/McRPG/quests/ on first boot. This makes the first-join tutorial actually fire on real servers. - Move the owner guides from src/main/java/.../quest/ (never packaged by Gradle) into src/main/resources/quests/ so they ship in the jar. - Delete the dead legacy src/main/resources/quest/ directory (quests_v2.yml, empty upgrade_quests.yml) that had zero code references and misled owners. - Log marker-file read/write failures instead of swallowing them silently. - Add QuestManagerExtractionResourcesTest validating the extraction list and that every referenced resource is packaged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
#291) Three additive third-party API changes: - QuestRewardGrantEvent (cancellable) + QuestRewardGrantedEvent fire around every reward grant, routed through a new QuestRewardGranter collaborator so all three grant pathways (inline, distribution, pending-login) share one interception point, distinguished by RewardGrantContext. Listeners can mutate or veto a batch; the pending path retains rows for a listener-removed or cancelled reward so nothing is lost. - Give every concrete QuestEvent subclass its own HandlerList so base-type listeners no longer silently miss QuestCancelEvent. The abstract QuestEvent base now declares no shared handler list; listening on it is unsupported. - Add a custom-data channel to QuestObjectiveInstance persisted as a JSON custom_data column (QuestObjectiveInstanceDAO table v2, migrated via ensureColumnExists) so custom objective types can store set-like progress. Adds tests for grant interception/mutation/cancellation, distinct per-event handler lists, and custom-data save/load round-trip. Documents the new event and custom-data channel in REWARDS.md/OBJECTIVES.md and the steering files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
…ions (#283) - Track ReloadableRarity/Category/Template configs with ReloadableContentManager and call reloadAllContent() from /mcrpg admin reload so board.yml edits apply without a restart; fix board.yml comments that named a non-existent command. - Validate rotation.time once at construction (parseTimeOrDefault) so a bad value falls back to midnight instead of throwing every scheduler tick. - Fall back with a WARNING instead of bricking onEnable/mid-rotation for an invalid weekly-reset-day, an empty rarity body, and all-zero rarity weights. - Catch unchecked SnakeYAML errors per template file so one malformed file no longer aborts loading of every template. - Resolve a quest's default 'all rarities' lazily (empty-set sentinel) instead of snapshotting the registry before board rarities load, so quests without supported-rarities roll all rarities after a plain restart. - Route category durations through QuestConfigLoader.parseDuration (case-insensitive; plain number = seconds) instead of Methods.getTimeInSeconds. Adds tests for the zero-weight fallback, plain-number/uppercase durations, malformed-template skip, and rotation-time fallback. Documents the load-time validation convention in persona-server-owner.mdc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
…ts pass The resolver's SCALE pot-behavior gates on QuestRewardType.isScalable(), which replaced the old reference-identity probe. The ScalableReward test double never overrode isScalable() (defaulting to false), so the SCALE branch took the non-scalable fallback and granted unscaled amounts — failing six PotBehavior tests. Override isScalable()/withExactAmount() on the double to match a real scalable reward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (29)
WalkthroughChangesQuest extensibility and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Server Owner ReviewServer Owner ReviewCONCERN: CONCERN: CONCERN: Category YAML files ( CONCERN: The CONCERN: The CONCERN: The CONCERN: New CONCERN: The CONCERN: The CONCERN: The Migration required: YES — database schema change (table version 1 → 2, Reload-safe: PARTIAL — |
Extensibility ReviewBreaking change risk: LOW — no public interfaces are broken, but two changes on the public API surface need attention from addon authors. CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: |
Testing ReviewI'll systematically apply every checklist item to the diff. Coverage CompletenessCONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: TimeProvider UsageNo new direct calls to McRPG Test StructureCONCERN: CONCERN: Test QualityCONCERN: CONCERN: CONCERN: CONCERN: SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. The diff contains no player-exploitable injection vulnerabilities within the defined threat model. Specifically:
|
- Restore the 9-arg QuestObjectiveInstance constructor (delegates with empty custom data) so addons compiling against the old signature keep working. - Document the QuestRewardGrantedEvent conditional-fire behavior, the QuestRewardGrantEvent.getRewards() cross-listener mutation semantics, the RewardGrantContext non-exhaustiveness, and the Gson number->Double round-trip for objective custom data (Javadoc + OBJECTIVES.md). - Document valid values and fallback behavior for board.yml rotation.time and weekly-reset-day; add the duration-format hint to appearance-cooldown. - Add tests: reward-grant throwing-reward isolation, empty-batch no-op, and no-granted-event-when-all-throw; custom-data dirty-flag marking; QuestPool empty-supported-rarities matches any rarity. Tighten test-quality nits (key equality over ==, assertNull over assertEquals(null, ...)). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Server Owner ReviewI'll work through every checklist item methodically against the diff, focusing on YAML files, config routing, migration safety, reload behavior, and permission nodes. Review FindingsCONCERN: CONCERN: CONCERN: Duration format comments are added to category files inconsistently — only on
CONCERN: The CONCERN: New CONCERN: CONCERN:
CONCERN: CONCERN: CONCERN: CONCERN: The SummaryMigration required: YES — Reload-safe: PARTIAL — Rarities, categories, and templates in |
Extensibility ReviewBreaking change risk: LOW — no public interfaces broken, no NamespacedKey values changed, but two new public APIs have gaps that could surprise addon developers. CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/us/eunoians/mcrpg/command/admin/ReloadPluginCommand.java`:
- Around line 39-43: Remove the explicit reloadAllContent() invocation and its
now-misleading comment from ReloadPluginCommand, relying on
FileManager.reloadFiles() to perform the reload exactly once.
In
`@src/main/java/us/eunoians/mcrpg/configuration/QuestTemplateConfigLoader.java`:
- Around line 144-150: In the template-loading flow, narrow the catch in
QuestTemplateConfigLoader from RuntimeException to SnakeYAML’s YAMLException.
Preserve the existing warning log and per-file return behavior for malformed
YAML, while allowing unrelated runtime defects to propagate.
In
`@src/main/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAO.java`:
- Around line 96-99: Update ensureColumnExists to return a boolean indicating
whether the column creation succeeded, and change updateTable’s
lastStoredVersion == 1 migration branch to call setTableVersion(..., 2) only
when that result is true. Preserve the existing exception logging and leave the
table version unchanged when ALTER TABLE fails.
- Around line 147-149: Update the SQLException catch in
QuestObjectiveInstanceDAO to emit a WARNING log containing the metadata-query
failure details before continuing with the existing PRAGMA fallback behavior.
Keep the current assumption that the column does not exist and let the ALTER
attempt proceed.
In
`@src/main/java/us/eunoians/mcrpg/listener/entity/player/PlayerJoinListener.java`:
- Around line 136-141: The pending-reward provenance in the join handling must
support multiple rows resolving to the same QuestRewardType instance. Replace
the one-to-one rewardToPendingId mapping and update the associated
grant/deletion logic around rewardsByQuest so each reward identity retains a
queue or list of all pending IDs, consuming the corresponding ID for every
successful grant. Add a regression test using duplicate reconstructed reward
instances to verify both pending rows are deleted and neither reward is granted
again on the next login.
- Around line 155-157: Update the isolated pending-reward reconstruction failure
log in PlayerJoinListener to use Level.WARNING instead of Level.SEVERE. Preserve
the existing message, continued processing behavior, retained reward, and
throwable argument.
In
`@src/main/java/us/eunoians/mcrpg/quest/board/configuration/ReloadableRarityConfig.java`:
- Around line 33-57: Update the RuntimeException handler in the rarity parsing
block of ReloadableRarityConfig to pass exception as the throwable argument to
the warning logger, while retaining the existing malformed-rarity context
message. Remove the direct exception.getMessage() interpolation so the full
stack trace is preserved, including when the message is null.
In `@src/main/java/us/eunoians/mcrpg/quest/board/QuestBoardManager.java`:
- Around line 168-169: Update parseResetDayOrDefault and its callers in
QuestBoardManager so null or blank reset-day values emit a warning containing
the configuration file, key, offending value, and valid options before falling
back. In the IllegalArgumentException catch path, pass the caught throwable to
the logger overload instead of discarding it, preserving actionable diagnostics
for all invalid values.
In
`@src/main/java/us/eunoians/mcrpg/quest/impl/objective/QuestObjectiveInstance.java`:
- Line 36: Make customData in QuestObjectiveInstance thread-safe by declaring
the field volatile and backing it with a ConcurrentHashMap, while preserving the
existing getCustomData API and mutation behavior. Update initialization or
reassignment paths for customData consistently, including the code around lines
85–118, so QuestManager.saveQuestAsync serialization cannot iterate an
unsynchronized HashMap.
In `@src/main/java/us/eunoians/mcrpg/quest/QuestManager.java`:
- Around line 125-144: Replace the mutable array fields
DEFAULT_TUTORIAL_QUEST_RESOURCES and DEFAULT_OWNER_GUIDE_RESOURCES with
immutable List<String> constants initialized via List.of(...). Update every
corresponding addAll usage in QuestManager to consume the lists, preserving the
existing resource order and behavior.
In `@src/main/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTask.java`:
- Around line 103-127: Update parseTimeOrDefault to log a contextual warning
when the configured time is missing or blank before falling back to
DEFAULT_ROTATION_TIME, while preserving the successful parse path. In the
invalid-value catch block, pass the caught exception to the logger alongside the
existing warning context instead of logging only a constructed string.
In `@src/main/resources/quests/BOARD-INTEGRATION.md`:
- Line 116: Update the fenced code block in BOARD-INTEGRATION.md to include an
explicit language identifier, such as text, while preserving its existing
contents.
- Around line 289-293: Update step 4 of the integration guide to reference the
category configuration directory `quest-board/categories/*.yml` (or the loader’s
actual path) instead of `categories.yml`, while preserving the instruction to
add the `SCOPED` category and reference its scope provider.
In `@src/main/resources/quests/CONDITIONS.md`:
- Around line 27-37: Update the TemplateCondition documentation and custom
example to use fromConfig(`@NotNull` Section section, `@NotNull` ConditionParser
parser), and ensure recursive or compound condition parsing passes the same
parser through each fromConfig call.
In `@src/main/resources/quests/QUEST-DEFINITIONS.md`:
- Line 19: Update the fenced code block in QUEST-DEFINITIONS.md to include an
explicit language identifier, such as text, while preserving its existing
contents.
- Around line 68-76: Update the phase-level reward documentation in
QUEST-DEFINITIONS.md to include inline rewards as supported, alongside
reward-distribution, matching the behavior of QuestConfigLoader and
QuestPhaseDefinition and the wording used in REWARDS.md and
SERVER-OWNER-GUIDE.md.
In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md`:
- Line 930: Update the RANGE variable examples in the guide to use the
configured Legendary difficulty multiplier of 3.5x, replacing the inconsistent
2.0x example and global-value description while preserving the scaling
explanation.
- Around line 1222-1225: Update the Performance section to remove the
unqualified “no practical limit” claim and replace it with tested operational
guidance for quest-file count and size, or concrete monitoring recommendations
for startup/reload time and memory usage. Keep the template-quest behavior
statement unchanged.
- Around line 565-570: Update the repeat-mode table in SERVER-OWNER-GUIDE.md to
add the supported COOLDOWN_LIMITED mode. Describe that it allows repeats only
after repeat-cooldown has passed and until repeat-limit total completions is
reached, including both required field names.
- Line 34: Update both fenced code blocks in SERVER-OWNER-GUIDE.md to include
appropriate language identifiers after their opening fences, including the block
near the later referenced section, while preserving their existing contents.
- Around line 716-728: Update the rarity configuration guidance around the “add,
remove, or rename rarities” statement to permit adding new tiers only, and
document that renames or removals require an alias or migration for existing
board metadata, template support lists, and persisted offerings or quest
instances. Keep the MYTHIC example unchanged.
In `@src/main/resources/quests/TEMPLATES.md`:
- Line 262: Update the fenced Markdown code block in TEMPLATES.md to include an
explicit language identifier, such as text, while preserving its contents.
In
`@src/test/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAOTest.java`:
- Around line 88-89: Swap the annotation order on both new test methods in
QuestObjectiveInstanceDAOTest: place `@Test` before `@DisplayName`, including the
methods around the custom-data round-trip test and the second affected test.
In `@src/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.java`:
- Around line 42-45: Update clearRewardEventListeners to retain references to
the listener instances registered by this test, then unregister each
specifically with HandlerList.unregisterAll(listener). Remove the broad
getHandlerList().unregisterAll() calls so shared listeners remain registered.
In `@src/test/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTaskTest.java`:
- Around line 266-267: In the test method with the display name “Invalid
rotation time falls back to midnight and still triggers without crashing,”
reorder the annotations so `@Test` appears before `@DisplayName`, matching the
project’s test annotation convention.
- Around line 272-286: The test around QuestBoardRotationTask should verify the
invalid “6:00” schedule falls back specifically to midnight by starting the
mocked time shortly after 00:00 rather than 14:00. Update the Instant setup and
subsequent time progression while preserving the existing single triggerRotation
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ce38ae5-2c01-4aac-869e-8cbfd9a51756
📒 Files selected for processing (59)
.cursor/rules/persona-extensibility.mdc.cursor/rules/persona-server-owner.mdcCLAUDE.mddocs/hld/tutorial/tutorial-quest-system.mddocs/lld/tutorial-quest-system/phase-2-quest-chain-system.mddocs/lld/tutorial-quest-system/phase-3-tutorial-content.mdsrc/main/java/us/eunoians/mcrpg/command/admin/ReloadPluginCommand.javasrc/main/java/us/eunoians/mcrpg/configuration/BoardCategoryConfigLoader.javasrc/main/java/us/eunoians/mcrpg/configuration/QuestConfigLoader.javasrc/main/java/us/eunoians/mcrpg/configuration/QuestTemplateConfigLoader.javasrc/main/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAO.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestCompleteEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestExpireEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestObjectiveCompleteEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestObjectiveProgressEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestPhaseCompleteEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantedEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestStageCompleteEvent.javasrc/main/java/us/eunoians/mcrpg/event/quest/QuestStartEvent.javasrc/main/java/us/eunoians/mcrpg/listener/entity/player/PlayerJoinListener.javasrc/main/java/us/eunoians/mcrpg/quest/QuestManager.javasrc/main/java/us/eunoians/mcrpg/quest/board/QuestBoardManager.javasrc/main/java/us/eunoians/mcrpg/quest/board/configuration/ReloadableRarityConfig.javasrc/main/java/us/eunoians/mcrpg/quest/board/distribution/RewardDistributionGranter.javasrc/main/java/us/eunoians/mcrpg/quest/board/generation/QuestPool.javasrc/main/java/us/eunoians/mcrpg/quest/board/rarity/QuestRarityRegistry.javasrc/main/java/us/eunoians/mcrpg/quest/impl/QuestInstance.javasrc/main/java/us/eunoians/mcrpg/quest/impl/objective/QuestObjectiveInstance.javasrc/main/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranter.javasrc/main/java/us/eunoians/mcrpg/quest/reward/RewardGrantContext.javasrc/main/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTask.javasrc/main/resources/quest-board/board.ymlsrc/main/resources/quest-board/categories/land-daily.ymlsrc/main/resources/quest-board/categories/land-weekly.ymlsrc/main/resources/quest-board/categories/personal-daily.ymlsrc/main/resources/quest-board/categories/personal-weekly.ymlsrc/main/resources/quest-board/categories/shared-daily.ymlsrc/main/resources/quest-board/categories/shared-weekly.ymlsrc/main/resources/quest/quests_v2.ymlsrc/main/resources/quest/upgrade_quests.ymlsrc/main/resources/quests/BOARD-INTEGRATION.mdsrc/main/resources/quests/CONDITIONS.mdsrc/main/resources/quests/OBJECTIVES.mdsrc/main/resources/quests/QUEST-DEFINITIONS.mdsrc/main/resources/quests/REWARDS.mdsrc/main/resources/quests/SERVER-OWNER-GUIDE.mdsrc/main/resources/quests/TEMPLATES.mdsrc/main/resources/quests/example_quest.ymlsrc/test/java/us/eunoians/mcrpg/configuration/BoardCategoryConfigLoaderTest.javasrc/test/java/us/eunoians/mcrpg/configuration/QuestTemplateConfigLoaderTest.javasrc/test/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAOTest.javasrc/test/java/us/eunoians/mcrpg/event/quest/QuestEventHandlerListTest.javasrc/test/java/us/eunoians/mcrpg/quest/QuestManagerExtractionResourcesTest.javasrc/test/java/us/eunoians/mcrpg/quest/board/distribution/QuestRewardDistributionResolverPotBehaviorTest.javasrc/test/java/us/eunoians/mcrpg/quest/board/rarity/QuestRarityRegistryTest.javasrc/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.javasrc/test/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTaskTest.java
💤 Files with no reviewable changes (1)
- src/main/resources/quest/quests_v2.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (26)
src/main/java/us/eunoians/mcrpg/command/admin/ReloadPluginCommand.java (1)
39-43: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Remove redundant
reloadAllContent()call.
FileManager.reloadFiles()(line 37) already callsreloadAllContent()as its final step, so the explicit call at lines 42-43 invokes it a second time on every reload. The comment "Without this, board.yml edits silently no-op until a restart" is misleading —reloadFiles()already covers this.♻️ Proposed fix
plugin.registryAccess().registry(RegistryKey.MANAGER).manager(McRPGManagerKey.FILE).reloadFiles(); - - // Re-parse all reloadable content (board rarities/categories/templates, ability - // config, etc.) from the freshly reloaded YAML documents. Without this, board.yml - // edits silently no-op until a restart. - plugin.registryAccess().registry(RegistryKey.MANAGER).manager(McRPGManagerKey.RELOADABLE_CONTENT) - .reloadAllContent(); // Invalidate level caches for all online players since leveling equations may have changed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.plugin.registryAccess().registry(RegistryKey.MANAGER).manager(McRPGManagerKey.FILE).reloadFiles(); // Invalidate level caches for all online players since leveling equations may have changed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/command/admin/ReloadPluginCommand.java` around lines 39 - 43, Remove the explicit reloadAllContent() invocation and its now-misleading comment from ReloadPluginCommand, relying on FileManager.reloadFiles() to perform the reload exactly once.src/main/java/us/eunoians/mcrpg/configuration/QuestTemplateConfigLoader.java (1)
144-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Consider narrowing the catch to
YAMLExceptioninstead of broadRuntimeException.Catching
RuntimeExceptionhere will also swallow unrelated bugs (e.g.NullPointerExceptionfrom a future change to this method or BoostedYaml internals) as a silent "skip this file" outcome, masking real defects. SnakeYAML surfaces syntax errors specifically asYAMLException; narrowing the catch keeps the intended fail-safe behavior while letting unrelated bugs surface.♻️ Proposed fix
- } catch (RuntimeException e) { + } catch (org.yaml.snakeyaml.error.YAMLException e) { // SnakeYAML surfaces syntax errors (unclosed quotes, bad indentation) as unchecked // exceptions. Without this, a single malformed template file would escape the directory // walk and abort loading of every other template. Skip just this file with a WARNING. logger.log(Level.WARNING, "Skipping template file " + file.getName() + " due to a YAML syntax error: " + e.getMessage(), e); return; }As per coding guidelines: "Catch SnakeYAML
YAMLExceptionper file so one malformed YAML file does not prevent other files from loading."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} catch (org.yaml.snakeyaml.error.YAMLException e) { // SnakeYAML surfaces syntax errors (unclosed quotes, bad indentation) as unchecked // exceptions. Without this, a single malformed template file would escape the directory // walk and abort loading of every other template. Skip just this file with a WARNING. logger.log(Level.WARNING, "Skipping template file " + file.getName() " due to a YAML syntax error: " + e.getMessage(), e); return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/configuration/QuestTemplateConfigLoader.java` around lines 144 - 150, In the template-loading flow, narrow the catch in QuestTemplateConfigLoader from RuntimeException to SnakeYAML’s YAMLException. Preserve the existing warning log and per-file return behavior for malformed YAML, while allowing unrelated runtime defects to propagate.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAO.java (2)
96-99: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Migration bumps version even when column creation fails.
ensureColumnExistscatches and logs theSQLExceptionfromALTER TABLE, butupdateTableunconditionally callssetTableVersion(..., 2)on line 98. If the ALTER fails, the version is recorded as 2 without the column existing, and all future saves/loads referencingcustom_datawill fail withSQLExceptionon every call.🛡️ Proposed fix: only bump version on successful column creation
// Version 2: add custom_data column for structured per-type objective progress state if (lastStoredVersion == 1) { - ensureColumnExists(connection, CUSTOM_DATA_COLUMN, "TEXT"); - TableVersionHistoryDAO.setTableVersion(connection, TABLE_NAME, 2); + if (ensureColumnExists(connection, CUSTOM_DATA_COLUMN, "TEXT")) { + TableVersionHistoryDAO.setTableVersion(connection, TABLE_NAME, 2); + } }This requires
ensureColumnExiststo returnbooleanindicating success:-private static void ensureColumnExists(`@NotNull` Connection connection, `@NotNull` String columnName, +private static boolean ensureColumnExists(`@NotNull` Connection connection, `@NotNull` String columnName, `@NotNull` String addColumnSqlFragment) { if (columnExists(connection, columnName)) { - return; + return true; } try (PreparedStatement ps = connection.prepareStatement( "ALTER TABLE " + TABLE_NAME + " ADD COLUMN " + columnName + " " + addColumnSqlFragment)) { ps.executeUpdate(); + return true; } catch (SQLException e) { McRPG.getInstance().getLogger().log(Level.SEVERE, "[QuestObjectiveInstanceDAO] Failed to add column " + columnName + " during migration", e); + return false; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (lastStoredVersion == 1) { if (ensureColumnExists(connection, CUSTOM_DATA_COLUMN, "TEXT")) { TableVersionHistoryDAO.setTableVersion(connection, TABLE_NAME, 2); } }private static boolean ensureColumnExists(`@NotNull` Connection connection, `@NotNull` String columnName, `@NotNull` String addColumnSqlFragment) { if (columnExists(connection, columnName)) { return true; } try (PreparedStatement ps = connection.prepareStatement( "ALTER TABLE " + TABLE_NAME + " ADD COLUMN " + columnName + " " + addColumnSqlFragment)) { ps.executeUpdate(); return true; } catch (SQLException e) { McRPG.getInstance().getLogger().log(Level.SEVERE, "[QuestObjectiveInstanceDAO] Failed to add column " columnName + " during migration", e); return false; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAO.java` around lines 96 - 99, Update ensureColumnExists to return a boolean indicating whether the column creation succeeded, and change updateTable’s lastStoredVersion == 1 migration branch to call setTableVersion(..., 2) only when that result is true. Preserve the existing exception logging and leave the table version unchanged when ALTER TABLE fails.Source: Coding guidelines
147-149: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Empty catch block violates coding guidelines.
The
catch (SQLException ignored)block contains only a comment. Per coding guidelines, catch blocks must not be empty or contain only comments; failures must be handled explicitly. A WARNING log here would aid diagnosis when metadata queries fail and the PRAGMA fallback is used.🔧 Proposed fix
- } catch (SQLException ignored) { - // If we can't determine it, assume it does not exist and let the ALTER attempt decide. + } catch (SQLException e) { + McRPG.getInstance().getLogger().log(Level.WARNING, + "[QuestObjectiveInstanceDAO] Failed to check column existence via metadata for " + + columnName + "; falling back to PRAGMA", e); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} catch (SQLException e) { McRPG.getInstance().getLogger().log(Level.WARNING, "[QuestObjectiveInstanceDAO] Failed to check column existence via metadata for " columnName + "; falling back to PRAGMA", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAO.java` around lines 147 - 149, Update the SQLException catch in QuestObjectiveInstanceDAO to emit a WARNING log containing the metadata-query failure details before continuing with the existing PRAGMA fallback behavior. Keep the current assumption that the column does not exist and let the ALTER attempt proceed.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/listener/entity/player/PlayerJoinListener.java (2)
136-141: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve one-to-one provenance for pending rewards.
IdentityHashMapstores only one pending ID per reward object. If two rows reconstruct to the same instance—as the suppliedRecordingReward.fromSerializedConfig()implementation does—the secondputoverwrites the first. Both rewards can grant, but only one row is deleted, so the other is granted again on the next login.Track a queue of pending IDs per identity or carry the pending-row provenance through the grant result, and add a duplicate-row regression test.
Also applies to: 152-153, 161-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/listener/entity/player/PlayerJoinListener.java` around lines 136 - 141, The pending-reward provenance in the join handling must support multiple rows resolving to the same QuestRewardType instance. Replace the one-to-one rewardToPendingId mapping and update the associated grant/deletion logic around rewardsByQuest so each reward identity retains a queue or list of all pending IDs, consuming the corresponding ID for every successful grant. Add a regression test using duplicate reconstructed reward instances to verify both pending rows are deleted and neither reward is granted again on the next login.
155-157: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log isolated reconstruction failures as WARNING.
This catch retains the malformed row and continues processing other rewards, so it is a degraded operational state rather than a plugin-integrity failure.
SEVEREshould be reserved for the latter.As per coding guidelines, use
WARNINGfor degraded but operational states and retain the throwable as context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/listener/entity/player/PlayerJoinListener.java` around lines 155 - 157, Update the isolated pending-reward reconstruction failure log in PlayerJoinListener to use Level.WARNING instead of Level.SEVERE. Preserve the existing message, continued processing behavior, retained reward, and throwable argument.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/quest/board/configuration/ReloadableRarityConfig.java (1)
33-57: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep the parsing exception in the warning log.
The malformed-rarity handler continues correctly, but
exception.getMessage()discards the stack trace and may itself benull. Log the throwable as the second argument.As per coding guidelines, caught exceptions must be logged with the throwable for diagnosis.
Proposed fix
} catch (RuntimeException exception) { - McRPG.getInstance().getLogger().warning("[QuestBoard] Skipping malformed rarity '" + rawKey - + "' in board.yml: " + exception.getMessage()); + McRPG.getInstance().getLogger().log(Level.WARNING, + "[QuestBoard] Skipping malformed rarity '" + rawKey + "' in board.yml", + exception); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} catch (RuntimeException exception) { McRPG.getInstance().getLogger().log(Level.WARNING, "[QuestBoard] Skipping malformed rarity '" + rawKey + "' in board.yml", exception); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/quest/board/configuration/ReloadableRarityConfig.java` around lines 33 - 57, Update the RuntimeException handler in the rarity parsing block of ReloadableRarityConfig to pass exception as the throwable argument to the warning logger, while retaining the existing malformed-rarity context message. Remove the direct exception.getMessage() interpolation so the full stack trace is preserved, including when the message is null.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/quest/board/QuestBoardManager.java (1)
168-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve diagnostics for invalid reset-day configuration.
null/blank values fall back silently, despite the Javadoc and configuration rules requiring a warning with the file, key, offending value, and valid options. TheIllegalArgumentExceptionpath also discards the throwable by callingwarning(String).As per coding guidelines, invalid configuration must be logged with actionable context, and caught exceptions must be passed to the logger.
Proposed fix
if (configured == null || configured.isBlank()) { + plugin().getLogger().warning( + "[QuestBoard] Missing weekly-reset-day in board.yml; expected MONDAY-SUNDAY. " + + "Falling back to MONDAY."); configured = DayOfWeek.MONDAY.name(); } try { return DayOfWeek.valueOf(configured.trim().toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException exception) { - plugin().getLogger().warning("[QuestBoard] Invalid weekly-reset-day '" + rawResetDay - + "' in board.yml (expected a day name such as MONDAY). Falling back to MONDAY."); + plugin().getLogger().log(Level.WARNING, + "[QuestBoard] Invalid weekly-reset-day '" + rawResetDay + + "' in board.yml (expected MONDAY-SUNDAY). Falling back to MONDAY.", + exception); return DayOfWeek.MONDAY; }Also applies to: 266-287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/quest/board/QuestBoardManager.java` around lines 168 - 169, Update parseResetDayOrDefault and its callers in QuestBoardManager so null or blank reset-day values emit a warning containing the configuration file, key, offending value, and valid options before falling back. In the IllegalArgumentException catch path, pass the caught throwable to the logger overload instead of discarding it, preserving actionable diagnostics for all invalid values.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/quest/impl/objective/QuestObjectiveInstance.java (1)
36-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
customDataHashMap is accessed from both main and DB executor threads without synchronization.
getCustomData()returns the liveHashMap, which event handlers mutate on the main thread (per the documented usage inOBJECTIVES.md). DuringQuestManager.saveQuestAsync, the DB executor thread callsserializeCustomData(objective.getCustomData())→GSON.toJson, which iterates the same map. This violates two coding guidelines: non-thread-safe collections must not be accessed from both main and async threads, and non-final fields written from multiple threads requirevolatileor synchronization.At minimum, the field should be
volatileand backed by aConcurrentHashMapto preventConcurrentModificationException. A more robust approach would snapshot the custom data on the main thread before submitting the async save.🔒 Minimum fix: volatile field + ConcurrentHashMap
-import java.util.HashMap; -import java.util.Map; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; - private Map<String, Object> customData; + private volatile Map<String, Object> customData;- this.customData = new HashMap<>(); + this.customData = new ConcurrentHashMap<>();- this.customData = new HashMap<>(customData); + this.customData = new ConcurrentHashMap<>(customData);public void setCustomData(`@NotNull` Map<String, Object> customData) { - this.customData = new HashMap<>(customData); + this.customData = new ConcurrentHashMap<>(customData); questStage.getQuestInstance().markDirty(); }Also applies to: 85-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/quest/impl/objective/QuestObjectiveInstance.java` at line 36, Make customData in QuestObjectiveInstance thread-safe by declaring the field volatile and backing it with a ConcurrentHashMap, while preserving the existing getCustomData API and mutation behavior. Update initialization or reassignment paths for customData consistently, including the code around lines 85–118, so QuestManager.saveQuestAsync serialization cannot iterate an unsynchronized HashMap.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/quest/QuestManager.java (1)
125-144: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use immutable collections for the new default resource constants.
DEFAULT_TUTORIAL_QUEST_RESOURCESandDEFAULT_OWNER_GUIDE_RESOURCESare mutable static arrays. Define them asList<String>withList.of(...)and update theaddAllcalls accordingly.As per coding guidelines, do not introduce new static mutable fields other than the allowed
McRPG.getInstance()singleton.Proposed change
- private static final String[] DEFAULT_TUTORIAL_QUEST_RESOURCES = { - "quests/tutorial/chain.yml", - "quests/tutorial/first_steps.yml", - "quests/tutorial/mcrpg_menu.yml", - "quests/tutorial/natural_talent.yml", - "quests/tutorial/your_arsenal.yml", - "quests/tutorial/unleashed_power.yml", - "quests/tutorial/combo_strike.yml", - "quests/tutorial/quest_board.yml" - }; - private static final String[] DEFAULT_OWNER_GUIDE_RESOURCES = { + private static final List<String> DEFAULT_TUTORIAL_QUEST_RESOURCES = List.of( + "quests/tutorial/chain.yml", + "quests/tutorial/first_steps.yml", + "quests/tutorial/mcrpg_menu.yml", + "quests/tutorial/natural_talent.yml", + "quests/tutorial/your_arsenal.yml", + "quests/tutorial/unleashed_power.yml", + "quests/tutorial/combo_strike.yml", + "quests/tutorial/quest_board.yml" + ); + private static final List<String> DEFAULT_OWNER_GUIDE_RESOURCES = List.of( "quests/CHAIN-GUIDE.md", "quests/SERVER-OWNER-GUIDE.md", "quests/OBJECTIVES.md", "quests/REWARDS.md", "quests/TEMPLATES.md", "quests/CONDITIONS.md", "quests/QUEST-DEFINITIONS.md", "quests/BOARD-INTEGRATION.md" - }; + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private static final List<String> DEFAULT_TUTORIAL_QUEST_RESOURCES = List.of( "quests/tutorial/chain.yml", "quests/tutorial/first_steps.yml", "quests/tutorial/mcrpg_menu.yml", "quests/tutorial/natural_talent.yml", "quests/tutorial/your_arsenal.yml", "quests/tutorial/unleashed_power.yml", "quests/tutorial/combo_strike.yml", "quests/tutorial/quest_board.yml" ); private static final List<String> DEFAULT_OWNER_GUIDE_RESOURCES = List.of( "quests/CHAIN-GUIDE.md", "quests/SERVER-OWNER-GUIDE.md", "quests/OBJECTIVES.md", "quests/REWARDS.md", "quests/TEMPLATES.md", "quests/CONDITIONS.md", "quests/QUEST-DEFINITIONS.md", "quests/BOARD-INTEGRATION.md" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/quest/QuestManager.java` around lines 125 - 144, Replace the mutable array fields DEFAULT_TUTORIAL_QUEST_RESOURCES and DEFAULT_OWNER_GUIDE_RESOURCES with immutable List<String> constants initialized via List.of(...). Update every corresponding addAll usage in QuestManager to consume the lists, preserving the existing resource order and behavior.Source: Coding guidelines
src/main/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTask.java (1)
103-127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve diagnostics for invalid rotation times.
Blank or missing values fall back without a warning, and the broad catch logs only a string. This makes configuration mistakes difficult to diagnose and violates the exception-logging requirement.
As per coding guidelines, configuration fallbacks must emit contextual warnings and caught exceptions must be passed to the logger.
Proposed fix
if (configuredTime == null || configuredTime.isBlank()) { + LOGGER.warning("[QuestBoard] Missing rotation.time in board.yml; " + + "expected HH:mm. Falling back to " + DEFAULT_ROTATION_TIME + "."); configuredTime = DEFAULT_ROTATION_TIME; } try { return LocalTime.parse(configuredTime); } catch (Exception exception) { - LOGGER.warning("[QuestBoard] Invalid rotation time '" + configuredTime - + "' configured in board.yml (expected 24-hour HH:mm, e.g. \"06:00\"). Falling back to " - + DEFAULT_ROTATION_TIME + "."); + LOGGER.log(Level.WARNING, + "[QuestBoard] Invalid rotation time '" + configuredTime + + "' configured in board.yml (expected HH:mm). Falling back to " + + DEFAULT_ROTATION_TIME + ".", + exception); return LocalTime.parse(DEFAULT_ROTATION_TIME); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Parses the configured rotation time (strict ISO {`@code` HH:mm}) once at construction, falling back * to {`@link` `#DEFAULT_ROTATION_TIME`} with a WARNING when the value is missing or malformed. This * mirrors {`@link` `#parseZoneOrDefault`} and prevents a bad {`@code` rotation.time} from throwing a * {`@link` java.time.format.DateTimeParseException} on every interval tick (a per-minute crash loop * with a board that never rotates). * * `@param` time the configured rotation time string * `@return` the parsed {`@link` LocalTime}, or the default on invalid input */ `@NotNull` private LocalTime parseTimeOrDefault(String time) { String configuredTime = time; if (configuredTime == null || configuredTime.isBlank()) { LOGGER.warning("[QuestBoard] Missing rotation.time in board.yml; " "expected HH:mm. Falling back to " + DEFAULT_ROTATION_TIME + "."); configuredTime = DEFAULT_ROTATION_TIME; } try { return LocalTime.parse(configuredTime); } catch (Exception exception) { LOGGER.log(Level.WARNING, "[QuestBoard] Invalid rotation time '" + configuredTime "' configured in board.yml (expected HH:mm). Falling back to " DEFAULT_ROTATION_TIME + ".", exception); return LocalTime.parse(DEFAULT_ROTATION_TIME); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTask.java` around lines 103 - 127, Update parseTimeOrDefault to log a contextual warning when the configured time is missing or blank before falling back to DEFAULT_ROTATION_TIME, while preserving the successful parse path. In the invalid-value catch block, pass the caught exception to the logger alongside the existing warning context instead of logging only a constructed string.Source: Coding guidelines
src/main/resources/quests/BOARD-INTEGRATION.md (2)
116-116: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Add a language identifier such as
textto satisfy the documented Markdown lint rule.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/BOARD-INTEGRATION.md` at line 116, Update the fenced code block in BOARD-INTEGRATION.md to include an explicit language identifier, such as text, while preserving its existing contents.Source: Linters/SAST tools
289-293: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the category configuration path.
The guide says categories use one file per category, but this step refers to
categories.yml. Point users toquest-board/categories/*.ymlor the actual loader path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/BOARD-INTEGRATION.md` around lines 289 - 293, Update step 4 of the integration guide to reference the category configuration directory `quest-board/categories/*.yml` (or the loader’s actual path) instead of `categories.yml`, while preserving the instruction to add the `SCOPED` category and reference its scope provider.src/main/resources/quests/CONDITIONS.md (1)
27-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the correct
fromConfigsignature.The supplied
TemplateConditioncontract requiresfromConfig(@NotNullSection section,@NotNullConditionParser parser), but both the interface and custom example show a one-argument method. Update both snippets and pass the parser through recursive/compound condition parsing.Also applies to: 291-296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/CONDITIONS.md` around lines 27 - 37, Update the TemplateCondition documentation and custom example to use fromConfig(`@NotNull` Section section, `@NotNull` ConditionParser parser), and ensure recursive or compound condition parsing passes the same parser through each fromConfig call.src/main/resources/quests/QUEST-DEFINITIONS.md (2)
19-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Add a language identifier such as
textto satisfy the documented Markdown lint rule.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/QUEST-DEFINITIONS.md` at line 19, Update the fenced code block in QUEST-DEFINITIONS.md to include an explicit language identifier, such as text, while preserving its existing contents.Source: Linters/SAST tools
68-76: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Relevant files ==\n' git ls-files | rg '(^|/)(QUEST-DEFINITIONS\.md|REWARDS\.md|SERVER-OWNER-GUIDE\.md|quest|Quest|reward)' || true printf '\n== Search for reward-related quest schema ==\n' rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \ 'reward-distribution|inline `rewards`|phase-level rewards|rewards list|completion-mode|stages' . printf '\n== Locate parser/config code for quests ==\n' rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \ 'reward-distribution|rewards|completion-mode|stage entries|quest definitions|QUEST-DEFINITIONS' src .Repository: DiamondDagger590/McRPG
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== File sizes / targeted excerpts ==\n' for f in src/main/resources/quests/QUEST-DEFINITIONS.md src/main/resources/quests/REWARDS.md src/main/resources/quests/SERVER-OWNER-GUIDE.md; do if [ -f "$f" ]; then wc -l "$f" echo "--- $f (selected lines) ---" sed -n '1,220p' "$f" | cat -n | sed -n '1,220p' fi done printf '\n== Quest parser/config source candidates ==\n' fd -a -t f '.*' src | rg 'quest|Quest|reward'Repository: DiamondDagger590/McRPG
Length of output: 50381
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Phase/definition model ==\n' ast-grep outline src/main/java/us/eunoians/mcrpg/quest/definition/QuestDefinition.java --view expanded || true ast-grep outline src/main/java/us/eunoians/mcrpg/quest/definition/QuestPhaseDefinition.java --view expanded || true ast-grep outline src/main/java/us/eunoians/mcrpg/quest/definition/QuestStageDefinition.java --view expanded || true ast-grep outline src/main/java/us/eunoians/mcrpg/quest/definition/QuestObjectiveDefinition.java --view expanded || true printf '\n== Parser ==\n' ast-grep outline src/main/java/us/eunoians/mcrpg/command/quest/parser/QuestDefinitionParser.java --view expanded || true printf '\n== Focused searches ==\n' rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \ 'class QuestPhaseDefinition|record QuestPhaseDefinition|rewardDistribution|rewards\(|phase.*rewards|phase-level rewards|parse.*phase|PhaseCompletionMode' \ src/main/java src/test/java src/main/resources printf '\n== Quest definition parser excerpts ==\n' sed -n '1,260p' src/main/java/us/eunoians/mcrpg/command/quest/parser/QuestDefinitionParser.java | cat -n printf '\n== Phase definition file (if present) ==\n' if [ -f src/main/java/us/eunoians/mcrpg/quest/definition/QuestPhaseDefinition.java ]; then sed -n '1,220p' src/main/java/us/eunoians/mcrpg/quest/definition/QuestPhaseDefinition.java | cat -n fiRepository: DiamondDagger590/McRPG
Length of output: 50378
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== QuestConfigLoader phase parsing ==\n' sed -n '430,500p' src/main/java/us/eunoians/mcrpg/configuration/QuestConfigLoader.java | cat -n printf '\n== QuestPhaseDefinition constructor and accessors ==\n' sed -n '1,140p' src/main/java/us/eunoians/mcrpg/quest/definition/QuestPhaseDefinition.java | cat -n printf '\n== Server owner guide reward section around phase rewards ==\n' rg -n -A4 -B4 'Phase-level|Rewards are given when a quest \(or stage, or objective\) is completed|rewards that match the phase|reward-distribution' src/main/resources/quests/SERVER-OWNER-GUIDE.md printf '\n== Rewards guide phase-level wording ==\n' rg -n -A4 -B4 'quest, phase, stage, or objective|phase-level|reward-distribution' src/main/resources/quests/REWARDS.mdRepository: DiamondDagger590/McRPG
Length of output: 13068
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path paths = [ Path("src/main/java/us/eunoians/mcrpg/configuration/QuestConfigLoader.java"), Path("src/main/java/us/eunoians/mcrpg/quest/definition/QuestPhaseDefinition.java"), Path("src/main/resources/quests/REWARDS.md"), Path("src/main/resources/quests/SERVER-OWNER-GUIDE.md"), ] for p in paths: print(f"\n## {p}") if p.exists(): text = p.read_text() for needle in [ "parsePhaseDefinition", "rewardDistribution(phaseSection", "The `rewards:` block can appear at the **quest, phase, stage, or objective** level.", "Phase-level", "reward-distribution", "phase-level rewards", ]: if needle in text: idx = text.index(needle) start = max(0, idx - 250) end = min(len(text), idx + 500) print(text[start:end]) print("\n---") PYRepository: DiamondDagger590/McRPG
Length of output: 4943
Align the phase reward docs.
QuestConfigLoaderandQuestPhaseDefinitionboth support inlinerewardson phases, sosrc/main/resources/quests/QUEST-DEFINITIONS.mdshould not say phases only supportreward-distribution. Update that note to matchREWARDS.mdandSERVER-OWNER-GUIDE.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/QUEST-DEFINITIONS.md` around lines 68 - 76, Update the phase-level reward documentation in QUEST-DEFINITIONS.md to include inline rewards as supported, alongside reward-distribution, matching the behavior of QuestConfigLoader and QuestPhaseDefinition and the wording used in REWARDS.md and SERVER-OWNER-GUIDE.md.src/main/resources/quests/SERVER-OWNER-GUIDE.md (5)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block languages.
Add language identifiers to both fenced blocks to satisfy the documented Markdown lint rule.
Also applies to: 127-127
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md` at line 34, Update both fenced code blocks in SERVER-OWNER-GUIDE.md to include appropriate language identifiers after their opening fences, including the block near the later referenced section, while preserving their existing contents.Source: Linters/SAST tools
565-570: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document
COOLDOWN_LIMITEDin the owner guide.
QUEST-DEFINITIONS.mdlists this supported mode, but the owner-facing repeat-mode table omits it and does not explain its requiredrepeat-cooldownandrepeat-limitfields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md` around lines 565 - 570, Update the repeat-mode table in SERVER-OWNER-GUIDE.md to add the supported COOLDOWN_LIMITED mode. Describe that it allows repeats only after repeat-cooldown has passed and until repeat-limit total completions is reached, including both required field names.
716-728: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restrict this to adding new tiers, or document the migration path for renames/removals. Rarity keys are used in board metadata, template support lists, and persisted board offerings/quest instances; changing or deleting a key without an alias/migration step will strand existing data and configs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md` around lines 716 - 728, Update the rarity configuration guidance around the “add, remove, or rename rarities” statement to permit adding new tiers only, and document that renames or removals require an alias or migration for existing board metadata, template support lists, and persisted offerings or quest instances. Keep the MYTHIC example unchanged.
930-930: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the difficulty multiplier examples consistent.
The guide defines Legendary as
3.5xat Lines 698-701, but later explains Legendary using2.0xand describes2.0as the global value. Align these examples with the actual default configuration.Also applies to: 1005-1008
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md` at line 930, Update the RANGE variable examples in the guide to use the configured Legendary difficulty multiplier of 3.5x, replacing the inconsistent 2.0x example and global-value description while preserving the scaling explanation.
1222-1225: 🚀 Performance & Scalability | 🔵 Trivial
Replace the “no practical limit” claim with tested guidance.
The plugin loads all quest files on startup and reload, so startup time and memory still grow with the number and size of files. Document tested operational limits or provide monitoring guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/SERVER-OWNER-GUIDE.md` around lines 1222 - 1225, Update the Performance section to remove the unqualified “no practical limit” claim and replace it with tested operational guidance for quest-file count and size, or concrete monitoring recommendations for startup/reload time and memory usage. Keep the template-quest behavior statement unchanged.src/main/resources/quests/TEMPLATES.md (1)
262-262: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced-block language.
Add a language identifier such as
textto satisfy the documented Markdown lint rule.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 262-262: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/quests/TEMPLATES.md` at line 262, Update the fenced Markdown code block in TEMPLATES.md to include an explicit language identifier, such as text, while preserving its contents.Source: Linters/SAST tools
src/test/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAOTest.java (1)
88-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Place
@Testbefore@DisplayNameon test methods.The two new test methods have
@DisplayNamebefore@Test, which violates the test coding guidelines. The fix is a trivial annotation swap.🔧 Proposed fix
- `@DisplayName`("Given custom data, when saving then loading, then the structured data round-trips") `@Test` + `@DisplayName`("Given custom data, when saving then loading, then the structured data round-trips") public void customData_roundTripsThroughSaveAndLoad() throws SQLException {- `@DisplayName`("Given empty custom data, when saving, then the custom_data column is null") `@Test` + `@DisplayName`("Given empty custom data, when saving, then the custom_data column is null") public void saveObjectiveInstance_writesNullCustomData_whenEmpty() throws SQLException {Also applies to: 136-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/database/table/quest/QuestObjectiveInstanceDAOTest.java` around lines 88 - 89, Swap the annotation order on both new test methods in QuestObjectiveInstanceDAOTest: place `@Test` before `@DisplayName`, including the methods around the custom-data round-trip test and the second affected test.Source: Coding guidelines
src/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.java (1)
42-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files 'src/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.java' \ 'src/main/java/**/QuestRewardGrantEvent.java' \ 'src/main/java/**/QuestRewardGrantedEvent.java' \ 'src/test/java/**/McRPGBaseTest.java' \ 'src/test/java/**/MockBukkit*' \ 'src/test/java/**/*QuestReward*' printf '\n--- QuestRewardGranterTest ---\n' sed -n '1,220p' src/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.java printf '\n--- Event classes ---\n' sed -n '1,220p' src/main/java/us/eunoians/mcrpg/quest/reward/event/QuestRewardGrantEvent.java sed -n '1,220p' src/main/java/us/eunoians/mcrpg/quest/reward/event/QuestRewardGrantedEvent.java printf '\n--- MockBukkit / base test usage ---\n' rg -n "unregisterAll\\(|getHandlerList\\(|HandlerList\\.unregisterAll|extends McRPGBaseTest|MockBukkit" src/test/java -g '*.java'Repository: DiamondDagger590/McRPG
Length of output: 7773
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- Event classes ---\n' cat -n src/main/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantEvent.java printf '\n---\n' cat -n src/main/java/us/eunoians/mcrpg/event/quest/QuestRewardGrantedEvent.java printf '\n--- Usages of these events ---\n' rg -n "QuestRewardGrantEvent|QuestRewardGrantedEvent" src/main/java src/test/java -g '*.java' printf '\n--- HandlerList / unregisterAll usage in tests ---\n' rg -n "getHandlerList\\(\\)\\.unregisterAll\\(|HandlerList\\.unregisterAll\\(" src/test/java -g '*.java'Repository: DiamondDagger590/McRPG
Length of output: 19235
Unregister only the listeners this test registers.
unregisterAll()clears every listener from these static event lists, so one test can wipe out listeners from shared setup or another test and make the suite order-dependent. Store the listener instances and callHandlerList.unregisterAll(listener)for each one instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/quest/reward/QuestRewardGranterTest.java` around lines 42 - 45, Update clearRewardEventListeners to retain references to the listener instances registered by this test, then unregister each specifically with HandlerList.unregisterAll(listener). Remove the broad getHandlerList().unregisterAll() calls so shared listeners remain registered.src/test/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTaskTest.java (2)
266-267: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Place
@Testbefore@DisplayName.The new test reverses the required annotation order.
As per coding guidelines, test methods must place
@Testbefore@DisplayName.- `@DisplayName`("Invalid rotation time falls back to midnight and still triggers without crashing") `@Test` + `@DisplayName`("Invalid rotation time falls back to midnight and still triggers without crashing")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@Test` `@DisplayName`("Invalid rotation time falls back to midnight and still triggers without crashing")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTaskTest.java` around lines 266 - 267, In the test method with the display name “Invalid rotation time falls back to midnight and still triggers without crashing,” reorder the annotations so `@Test` appears before `@DisplayName`, matching the project’s test annotation convention.Source: Coding guidelines
272-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the test distinguish midnight fallback from other fallback values.
At 14:00, the test passes for any fallback earlier than 14:00; it does not prove that the invalid value falls back specifically to midnight. Run the task shortly after midnight instead.
- Instant instant = instantAt(14, 0); + Instant instant = instantAt(0, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Instant instant = instantAt(0, 1); when(timeProvider.now()).thenReturn(instant); // "6:00" is not strict ISO (needs "06:00"); it must fall back to 00:00 rather than throwing // a DateTimeParseException on every tick. QuestBoardRotationTask task = spy(new QuestBoardRotationTask( McRPG.getInstance(), 0, 1f, "6:00", "UTC")); task.runTask(); MockBukkit.getMock().getScheduler().performOneTick(); instant = instant.plusSeconds(1); when(timeProvider.now()).thenReturn(instant); MockBukkit.getMock().getScheduler().performOneTick(); verify(mockBoardManager, times(1)).triggerRotation(daily.getKey());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/us/eunoians/mcrpg/task/board/QuestBoardRotationTaskTest.java` around lines 272 - 286, The test around QuestBoardRotationTask should verify the invalid “6:00” schedule falls back specifically to midnight by starting the mocked time shortly after 00:00 rather than 14:00. Update the Instant setup and subsequent time progression while preserving the existing single triggerRotation verification.
Testing ReviewI'll systematically apply every checklist item to the diff. AnalysisProduction files with non-trivial new logic
Test files present
FindingsCONCERN:
CONCERN: No test covers the branch in CONCERN: No test covers the branch in CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: (Withdrawn — covered by the identity-map assertion already in the test.) CONCERN: CONCERN: SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewI have reviewed the full diff against every checklist item. Here are my findings:
|
- Remove the redundant reloadAllContent() from ReloadPluginCommand; FileManager.reloadFiles() already reloads tracked reloadable content. - Drop the 9-arg QuestObjectiveInstance constructor (single canonical form). - Fix pending-reward provenance: two rows that reconstruct to the same QuestRewardType instance now each retain their row id via a per-instance queue, so both rows are deleted and neither re-grants forever. Add a duplicate-row regression test. - Make QuestObjectiveInstance.customData a volatile ConcurrentHashMap so the async save cannot hit a ConcurrentModificationException; drop null values on load defensively. - Log the caught throwable (not just getMessage) in ReloadableRarityConfig, QuestBoardManager.parseResetDayOrDefault, QuestBoardRotationTask, and the QuestObjectiveInstanceDAO metadata-probe catch; warn on blank config values; downgrade the pending-reward reconstruction failure to WARNING. - Make the tutorial/guide default-resource lists immutable (List.of). - Test nits: @test before @DisplayName; the rotation-time fallback test now starts at 00:01 to prove the midnight fallback specifically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Server Owner ReviewServer Owner ReviewCONCERN: CONCERN: Duration format comment is added to CONCERN: CONCERN: No CONCERN: The CONCERN: CONCERN: Two files are deleted in this PR — CONCERN: The database table CONCERN: The guide Migration required: YES
Reload-safe: PARTIAL
|
Extensibility ReviewBreaking change risk: LOW — existing addons that listened on CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: The |
Testing ReviewI'll systematically apply every checklist item to the diff. Coverage AnalysisNew Production Classes / Methods Requiring Tests
CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: Naming / Structure ViolationsCONCERN: CONCERN: SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. The diff contains no player-exploitable injection vulnerabilities under the defined threat model. Specific review notes: Adventure API / MiniMessage: No calls to Command injection via Permission bypass: No new command handlers or slot actions are introduced. SQL / data injection: All DAO changes in Stored data → deserialization: Custom data originates from |
Now that McCore's Methods.getTimeInSeconds is case-insensitive and treats a bare number as seconds, the category loader no longer needs to work around it via QuestConfigLoader.parseDuration. This also restores w/y unit support for category durations. Quest files continue to use parseDuration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Server Owner ReviewServer Owner ReviewWorking through the diff systematically against the checklist. CONCERN: CONCERN: CONCERN: CONCERN: Category YAML files ( CONCERN: Duration format comment is inconsistent — added to some category files but not all, and not added to CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: The CONCERN: Migration required: YES — Reload-safe: PARTIAL — |
Extensibility ReviewBreaking change risk: LOW — no public interfaces broken, but two constructor signatures changed and the CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: |
Testing ReviewI'll systematically apply every checklist item to the diff. Coverage AnalysisNew Production Classes / Logic Requiring Coverage
FindingsCONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: SummaryProduction files changed: Test files present: Coverage gaps: |
Security ReviewNo security concerns found. The diff is entirely composed of:
Checked against every checklist item:
|
A third-party listener can add a null to the mutable QuestRewardGrantEvent reward list; the null grant() NPE was caught, but the catch handler then dereferenced the null (reward.getKey()) and escaped, aborting the batch. Skip null entries with a WARNING. Add a null-in-batch test and a malformed-custom_data JSON load test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Server Owner ReviewServer Owner ReviewI'm going through this diff looking at every YAML file, config key, migration path, reload behavior, and permission node that affects a live server. CONCERN: CONCERN: CONCERN: The CONCERN: The CONCERN: CONCERN: Two empty files ( CONCERN: The CONCERN: The new CONCERN: The CONCERN: The SummaryMigration required: YES
Reload-safe: PARTIAL
|
Extensibility ReviewBreaking change risk: LOW — the CONCERN: Public constructor of CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: |
Testing ReviewI'll systematically apply every checklist item to the diff. Coverage AnalysisNew Production Classes / Methods with Non-Trivial Logic
CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: Test Naming ConventionCONCERN:
|
Security ReviewSecurity ReviewApplying all checklist items to the changed code. Adventure API / MiniMessage InjectionNo user-controlled strings (chat, item names, sign text, book content, anvil renames) are passed to Command Injection via
|
Revert the per-subclass HandlerList split: the abstract QuestEvent base now owns the single shared handler list and every concrete subclass inherits it, matching Bukkit's own EntityDamageEvent / EntityDamageByEntityEvent idiom. A listener on the base type again receives every quest event (including QuestCancelEvent, the original divergence bug), while a subclass listener stays filtered to its own subtype via Bukkit's isAssignableFrom executor guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Treat objective custom data as part of the initial schema instead of a v2 upgrade: the column is already in the CREATE TABLE, so the migration branch and the SQLite-only column-existence probe (PRAGMA table_info) are removed. This avoids leaning on SQLite-specific metadata that would complicate future MySQL support. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Server Owner ReviewServer Owner ReviewCONCERN: CONCERN: CONCERN: The (This is partially addressed in the diff — the comment does mention leading zero — but the fallback behavior and single-warning-only nature is not mentioned.) CONCERN: New Migration required: YES CONCERN: CONCERN: The CONCERN: Six category YAML files ( CONCERN: Summary Table
|
Extensibility ReviewBreaking change risk: LOW — one public constructor signature on CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: |
Testing ReviewI'll systematically apply every checklist item to the diff. Coverage AnalysisProduction files with non-trivial new logic
FindingsCONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: CONCERN: There is no test covering the SummaryProduction files changed: Test files present: Coverage gaps:
|
Security ReviewNo security concerns found. Rationale by checklist item: Adventure API / MiniMessage Injection Command Injection via Permission Bypass SQL / Data Injection |
2 participants
Summary
This PR introduces a third-party interception API for quest reward grants and adds structured custom-data persistence for objective instances. It centralizes all reward-granting logic through a new
QuestRewardGranterclass that fires cancellable pre-grant and informational post-grant events, enabling plugins to modify, veto, or audit quest rewards. Additionally, objective instances can now persist arbitrary structured state via a JSON-serializedcustom_datacolumn, supporting set-like objectives and other complex progress tracking.Key Changes
Reward Grant Events & Centralization:
QuestRewardGrantEvent(cancellable, pre-grant) andQuestRewardGrantedEvent(informational, post-grant) tous.eunoians.mcrpg.event.questQuestRewardGranterclass that routes all three grant pathways (inline completion, distribution, pending-reward login) through a single grant methodRewardGrantContextenum to identify which grant pathway an event originates from (INLINE, DISTRIBUTION, PENDING_REWARD_LOGIN)PlayerJoinListenerandRewardDistributionGranterto use the newQuestRewardGranterObjective Custom Data:
customDatafield toQuestObjectiveInstanceas aMap<String, Object>for structured per-type progress stateQuestObjectiveInstanceDAOwith schema version 2: addedcustom_dataTEXT column with JSON serialization via GsonensureColumnExists()helper for safe schema migrationsEvent Handler List Fix:
HandlerListinstead of sharing the abstract base's listQuestStartEvent,QuestCompleteEvent,QuestExpireEvent,QuestCancelEvent,QuestObjectiveProgressEvent,QuestObjectiveCompleteEvent,QuestStageCompleteEvent,QuestPhaseCompleteEvent,QuestRewardGrantEvent,QuestRewardGrantedEvent) to declare private staticHANDLERSand implementgetHandlerList()QuestEventbase to document that it intentionally declares no handler listTesting & Documentation:
QuestRewardGranterTestverifying event firing, cancellation, and listener mutation of reward listsQuestEventHandlerListTestensuring each concrete event has its own handler listQuestManagerExtractionResourcesTestverifying tutorial, example chain, and owner guides are in the extraction listQuestObjectiveInstanceDAOTestwith round-trip test for custom data serializationQuestBoardRotationTask(defaults to midnight)QuestTemplateConfigLoaderto skip malformed YAML files without aborting other templatesBoardCategoryConfigLoaderto parse plain-number and uppercase-unit durationsResource & Configuration Updates:
quests_v2.ymlandquest/upgrade_quests.ymlquests/tutorial/directory with 8 tutorial quest filesQuestManagerto extract tutorial and guide resources on startupNotable Implementation Details
QuestRewardGranterwraps each individualreward.grant(player)call in try/catch to prevent one failinghttps://claude.ai/code/session_01T7RB9ru7v2CnKyY8aAVTPu
Summary by CodeRabbit
New Features
Bug Fixes
Documentation