Phase 4 LLD: Chain System Backlog — Comprehensive Design Document

[DiamondDagger590]

Summary

This PR adds the Phase 4 Low-Level Design (LLD) document that comprehensively specifies all 13 remaining backlog items for the quest chain system. The document provides detailed implementation guidance for timestamp modernization, quest reload safety, ability unregistration reversibility, chain repeatability, availability windows, quest expiration behaviors, chain start conditions, lifecycle events, and content introspection.

Key Sections

The 2100+ line LLD covers:

1. Timestamp Refactor (§1) — Migrate quest/chain timestamps from Long epoch millis to Instant across QuestInstance, QuestChainPlayerState, and all DAOs with boundary conversions

2. Quest Reload Fixes (§2) — Implement finished quest cache invalidation and active instance reconciliation to cancel stale instances when definitions change, with player notifications

3. Ability Unregistration Reversibility (§3) — Track soft-disabled abilities in AbilityRegistry instead of permanent removal, enabling re-registration when quest definitions are restored on reload

4. Tutorial Bypass Scope Fix (§4) — Harden bypass permission check to reference tutorial chain key directly instead of source key

5. AbilityType Refactor Deferred Items (§5) — Extract isAlwaysAvailable() predicate, create AbilityNameResolver collaborator, add comprehensive filter tests

6. Chain Repeatability (§6) — Implement RepeatEvaluator for all repeat modes (UNLIMITED, COOLDOWN, LIMITED, COOLDOWN_LIMITED) with cooldown math and completion limits

7. Availability Windows (§7) — Design sealed WindowBoundary interface (fixed/recurring), AvailabilityWindowDefinition with year-wrapping logic, ChainAvailabilityChecker scheduled task, and three window-close policies (EXPIRE_ACTIVE, ALLOW_FINISH, EXPIRE_WITH_GRACE)

8. Quest Expiration Behaviors (§8) — Dispatch on-quest-expire actions (retry, restart-chain, skip) with retry counters and grace periods

9. Chain Start Conditions & TimeGateChainCondition (§9) — Define QuestChainStartCondition interface and implement first built-in TimeGateChainCondition for time-gated chains

10. Chain Lifecycle Events (§10) — Add QuestChainExpireEvent, QuestChainRestartEvent, QuestChainStepRetryEvent

11. Content Introspection Commands (§11) — Chat-based commands for listing/inspecting quests, chains, abilities, and their properties

12. Implementation Order (§12) — Sequenced 5-phase rollout prioritizing foundational changes (timestamps, reload fixes) before feature-heavy items (availability windows, expiration behaviors)

13. File & Locale Keys — Complete summary of new files, modified files, and required localization keys

Notable Design Decisions

• Timestamp boundary conversions happen at DAO layer; Java APIs use Instant exclusively
• Soft-disable pattern for abilities allows reversible unregistration without losing ability metadata
• Year-wrapping windows use comparison logic to handle recurring boundaries that cross calendar year boundaries (e.g., Dec 1 to Jan 3)
• Grace periods for EXPIRE_WITH_GRACE policy use in-memory task tracking with immediate player warnings
• Retry counters are in-memory only (not persisted) to avoid permanent lockout from misconfigured quest durations
• Availability checker is a configurable scheduled task (default 60s interval) that detects window transitions and applies policies
• Repeat evaluation respects availability windows — chains cannot be re-started if currently unavailable

Testing Strategy

Comprehensive test coverage specified for all major components:

• Timestamp conversions and calculations
• Reload reconciliation and cache invalidation
• Soft-disable/re-enable lifecycle
• All repeat mode combinations with cooldown/limit edge cases
• Window boundary resolution and year-wrapping
• Availability checker state

https://claude.ai/code/session_01Ln8kpPanSWnnjZEsfMNKwg

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d2d6b5f-4684-4c1e-b54b-431c5d6d20a9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/wonderful-brahmagupta-DiIg6

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

Server Owner Review

Looking at this diff, I need to focus on what matters to me as a server owner: the YAML resource files, plugin.yml, config changes, and anything that affects my ability to run the server without breaking things. The diff is heavily documentation and internal Java planning files, but let me extract what affects server owners.

---

Server Owner Review

---

CONCERN: config.yml adds a tutorial.enabled toggle but the HLD explicitly states "No config-version bump needed — no migration logic required for this addition." However, any structural addition to a config file requires a config-version increment so that automated migration tooling (and server owners) know the file has changed shape.
WHY: If a server owner copies their old config.yml forward, the new tutorial.enabled key will be absent. The plugin will either crash on a missing key, silently use a default that isn't documented in their file, or behave unexpectedly — with no indication in the config that anything changed.
WHERE: src/main/resources/config.yml → tutorial.enabled / config-version

---

CONCERN: The tutorial.enabled config key has no documented # comment in the diff explaining what it does, valid values (true/false), or what breaks if it is set wrong (e.g., "if false, tutorial quests will never auto-start for any player regardless of chain state").
WHY: A server owner opening config.yml for the first time will not know whether false disables the chain entirely, disables rewards, or just suppresses the auto-start. They will have to ask in a support channel or read source code.
WHERE: src/main/resources/config.yml → tutorial.enabled

---

CONCERN: The <tutorial> palette placeholder (<color:#E8C97A>) is added to config.yml but there is no visible comment block in the diff explaining what it is, where it appears in-game, or what happens if a server owner changes or removes it.
WHY: Server owners customizing their color palette will not know this entry drives tutorial quest name coloring. If they delete it thinking it is unused, tutorial quest names render as raw MiniMessage markup in player chat/GUI.
WHERE: src/main/resources/config.yml → palette section → tutorial entry

---

CONCERN: Seven new tutorial quest YAML files and a chain.yml are added under src/main/resources/quests/tutorial/, but there is no indication in the diff that these files contain # comments on every non-obvious key (e.g., on-start-messages, on-quest-expire, repeat-mode, auto-start.trigger). The HLD YAML snippets shown in the docs lack inline comments entirely.
WHY: Server owners who want to customize tutorial quests (change objectives, tweak rewards, translate messages) will open these files and encounter undocumented keys. on-quest-expire: fail-chain with no comment explaining valid values (retry, fail-chain, restart-chain) is a silent footgun.
WHERE: src/main/resources/quests/tutorial/chain.yml and all seven *.yml quest files → every key

---

CONCERN: The chain.yml introduces a new source: mcrpg:tutorial key. There is no # comment in any visible YAML explaining what source controls, what valid values are, or what happens if a server owner types a nonexistent source key.
WHY: Server owners creating their own chain definitions (third-party content) will copy the tutorial chain as a template. A typo in source will silently misconfigure the chain with no clear error at load time.
WHERE: src/main/resources/quests/tutorial/chain.yml → source:

---

CONCERN: Multiple new permission nodes are introduced (mcrpg.quest.chain.restart, mcrpg.quest.chain.reset, mcrpg.quest.chain.advance, mcrpg.quest.chain.skip, mcrpg.quest.chain.status, mcrpg.tutorial.bypass, mcrpg.quest.chain.*, mcrpg.admin.*) but the diff does not show these added to plugin.yml with description: fields and explicit default: values.
WHY: Without entries in plugin.yml, LuckPerms and other permission plugins cannot list or auto-complete these nodes. Server owners granting mcrpg.quest.chain.* via wildcard will not know which sub-nodes it covers. Missing description: means /permissions info shows blank entries.
WHERE: plugin.yml → permissions: block

---

CONCERN: The diff references renaming getCompletedQuestKeys() to getNonSkippedCompletedQuestKeys() (finding X6 in AUDIT_RESULTS.md) with no migration note for server owners who may have LuckPerms permission entries or command aliases referencing old behavior. More critically, if any permission nodes were renamed or removed, existing LuckPerms grants are silently revoked.
WHY: Even if X6 is a Java API rename (invisible to server owners), the audit document also lists events and API surface changes without confirming whether any plugin.yml permission node names changed. Silent permission revocation breaks admin workflows without any error message.
WHERE: AUDIT_RESULTS.md → X6 / plugin.yml → any renamed permission nodes

---

CONCERN: The new on-start-messages: YAML section supports two formats — a locale key (key: tutorial.first-steps.welcome) and inline messages (messages: [...]). Neither format is visible in the shipped quest YAML files in the diff, and no comment in those files explains which format takes priority or what happens if both are present.
WHY: A server owner translating or customizing tutorial text will not know whether to edit the locale file or the quest YAML. If they set both, the behavior is undefined from their perspective.
WHERE: src/main/resources/quests/tutorial/*.yml → on-start-messages: blocks

---

CONCERN: The en_quest.yml localization file receives new keys (tutorial text, cascade batch headers, chain GUI strings) but the diff does not show config-version being incremented on this file, nor is there any automated migration or manual guide for server owners who have already customized en_quest.yml.
WHY: A server owner who has edited their localization file will not receive the new tutorial keys on update. Tutorial on-start messages and cascade batch summaries will either be blank or throw missing-key errors in chat, with no indication of what file to edit.
WHERE: src/main/resources/localization/english/en_quest.yml → config-version (if present) / new tutorial and chain key blocks

---

CONCERN: The DisableTutorialSetting is a new player setting, but there is no upgrade note explaining what happens to existing players on update. Do they start with false (tutorial enabled, which would auto-start the chain on their next login)? For existing players who have already experienced the content, this could re-trigger the tutorial chain.
WHY: On a live server updating from a prior version, all existing players could have the tutorial auto-start on their next login because their DisableTutorialSetting row does not exist in the database and defaults to false. This is a potentially disruptive experience at scale.
WHERE: DB player settings table → DisableTutorialSetting default value / upgrade notes

---

CONCERN: Two new database tables are created (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log) via UpdateTableFunction. The AUDIT_RESULTS document flags E1 — that setTableVersion() is called outside the try-catch for ALTER TABLE, meaning if the ALTER fails, the version is bumped and the column is never added on retry. This is an unresolved finding in the diff.
WHY: If table migration fails on first startup (e.g., database permissions issue, locked table), the version is permanently bumped. On every subsequent restart, the migration is silently skipped, the column is never added, and chain state persistence will fail or corrupt silently. The server owner sees no actionable error.
WHERE: src/main/java/us/eunoians/mcrpg/database/table/quest/chain/QuestChainCompletionLogDAO.java → attemptUpdateTable() / QuestChainStateDAO.java → attemptUpdateTable()

---

CONCERN: The HLD documents a grace-period: 48h syntax and on-window-close: expire-with-grace policy for availability windows, but neither the valid duration format (48h, 2d, ISO-8601?) nor valid on-window-close values are described with # comments in any YAML file visible in the diff.
WHY: A server owner setting up a seasonal event chain will guess at the duration format. grace-period: 2 days vs grace-period: 48h vs grace-period: PT48H — one of these will silently fail to parse, falling back to an undocumented default, and the grace period will not function.
WHERE: src/main/resources/quests/*/chain.yml → availability.grace-period / availability.on-window-close

---

Migration required: YES — two new database tables, new player setting with default behavior affecting existing players, new config keys in config.yml and localization files.

Reload-safe: PARTIAL — chain definitions and quest definitions support /mcrpg admin reload per the HLD, but the AUDIT_RESULTS document flags that active quest instances are not reconciled on reload (finding E3, T10 in backlog context), and the AvailabilityWindowChecker thread-safety issues (C1–C6) mean the scheduler-driven expiry path is unsafe until those concurrency fixes land.

[github-actions]

Extensibility Review

Breaking change risk: MEDIUM — multiple public API signatures changed (Long→Instant, method renames, new mandatory parameters) without deprecation bridges, and new chain events have nullable-consistency gaps that will silently drop offline-player notifications from addon listeners.

---

CONCERN: QuestChainRestartEvent and QuestChainStepRetryEvent require @NotNull Player, but offline player scenarios silently skip firing these events entirely.
WHY: An addon listening for QuestChainRestartEvent or QuestChainStepRetryEvent will never receive them when the player is offline at the moment of the transition. Other chain events (QuestChainFailEvent, QuestChainExpireEvent, QuestChainAbandonEvent) accept @Nullable Player per the AUDIT_RESULTS.md (X9/X10). An addon that counts restarts or retries for analytics will produce silently wrong numbers — no exception, no warning, just missing events. The inconsistency also means addon developers must consult source code to know which events fire offline and which do not; the API surface alone cannot tell them.
WHERE: event/quest/chain/QuestChainRestartEvent.java, event/quest/chain/QuestChainStepRetryEvent.java (X9/X10 in AUDIT_RESULTS.md; PLAN_03_EXTENSIBILITY.md §X9/X10)

---

CONCERN: QuestChainStartCondition is marked @ApiStatus.Experimental but ships in a public registry with no default implementations and no documented stability contract.
WHY: An addon implementing QuestChainStartCondition and registering it via QuestChainStartConditionContentPack has no guarantee the interface signature will not change in the next release. @ApiStatus.Experimental communicates instability but provides no migration path. If evaluate() gains a mandatory parameter (as already happened once per AUDIT_RESULTS.md X1 — Instant added to QuestChainStartCondition.evaluate()), all addon implementations break at compile time with no deprecation bridge. The AUDIT_RESULTS.md explicitly calls out X1 as a breaking signature change.
WHERE: quest/chain/QuestChainStartCondition.java, expansion/content/QuestChainStartConditionContentPack.java

---

CONCERN: getCompletedQuestKeys() was renamed to getNonSkippedCompletedQuestKeys() without a @Deprecated alias.
WHY: Any addon that called getCompletedQuestKeys() on QuestChainPlayerData or wherever this method resided will fail to compile after upgrading. There is no deprecated forwarding method, no migration notice in Javadoc, and no bridge. AUDIT_RESULTS.md lists this as X6 but categorizes it as "acceptable if no third-party addons exist yet" — a risk assessment, not a fix. An addon developer picking up a stale dependency will get a NoSuchMethodError at runtime if shading, or a compile error otherwise, with no actionable message.
WHERE: Wherever getCompletedQuestKeys() was declared (likely QuestChainPlayerData or QuestChainPlayerState) per AUDIT_RESULTS.md X6

---

CONCERN: QuestChainCompleteEvent carries completionCount for repeat tracking, but there is no corresponding QuestChainExpireEvent carrying equivalent context, and the backlog documents QuestChainExpireEvent as "Pending (needs availability windows)."
WHY: An addon building a chain leaderboard or analytics dashboard needs symmetric events across all terminal state transitions. Complete fires with count context; Expire does not exist yet. The addon developer cannot treat EXPIRED as a first-class terminal state from events alone — they must poll player state or listen for a different signal. This is a missing interception point, not just a future feature, because the availability window system is implemented (AvailabilityWindowChecker exists and transitions chains to EXPIRED), but the event is absent.
WHERE: event/quest/chain/ package — QuestChainExpireEvent missing; quest/availability/AvailabilityWindowChecker.java performs the EXPIRED transition

---

CONCERN: softDisableAbility() fires AbilityUnregisterEvent with no flag distinguishing soft-disable from permanent removal.
WHY: An addon listening for AbilityUnregisterEvent to clean up per-ability state (remove ability-keyed entries from a cache, cancel scheduled tasks, notify players) will incorrectly treat a temporary soft-disable as a permanent removal. The addon has no way to know whether to destroy its state permanently or merely suspend it. AUDIT_RESULTS.md (X11) acknowledges this and proposes a boolean isSoftDisable() on the event, but the diff does not implement it. The PLAN_03 document defers the decision.
WHERE: ability/AbilityRegistry.java softDisableAbility(), AbilityUnregisterEvent (McCore or McRPG event package)

---

CONCERN: QuestPool constructor gained a mandatory TimeProvider parameter (AUDIT_RESULTS.md X8) without a deprecated no-TimeProvider overload.
WHY: Any addon that instantiates QuestPool directly — for example, a custom board generation strategy extending or composing QuestPool — will fail to compile. There is no deprecated bridge constructor and no factory method. The change is not flagged as requiring a migration path anywhere in the diff.
WHERE: quest/board/generation/QuestPool.java constructor (AUDIT_RESULTS.md X8)

---

CONCERN: ChainAutoStartTrigger is an extensible interface registered via ChainAutoStartTriggerContentPack, but the interface contract (specifically what tryStartChain() invocation guarantees — thread, timing, re-entrancy) is not documented in Javadoc.
WHY: An addon implementing a custom trigger (e.g., myplugin:region_enter) must know: Is it safe to call QuestChainManager.tryStartChain() from an async thread? Can it be called multiple times concurrently for the same player? What happens if called during a login event before player data is fully loaded? The HLD says "each custom trigger provides its own listener that calls QuestChainManager.tryStartChain(player, chainKey)" but provides no threading or reentrancy contract. Given that AUDIT_RESULTS.md documents active concurrency bugs (C1–C6) in the availability checker, an addon trigger firing from async context would hit the same class of bug.
WHERE: quest/chain/trigger/ChainAutoStartTrigger.java, quest/chain/QuestChainManager.java tryStartChain() method Javadoc

---

CONCERN: QuestChainPlayerState.getLastCompletedAt() changed from Optional<Long> to Optional<Instant> (AUDIT_RESULTS.md X2) and QuestInstance timestamp accessors changed from long to Instant (X3) with no deprecated bridge accessors.
WHY: These are public API methods on player-facing state objects. An addon reading getLastCompletedAt() for cooldown math or display will break at compile time. The Optional<Long> → Optional<Instant> change is not source-compatible: .map(ts -> new Date(ts)) style call sites fail. No @Deprecated long getLastCompletedAtEpochMilli() bridges exist. AUDIT_RESULTS.md defers this as "acceptable if no addons exist" but that assumption is not enforced anywhere.
WHERE: quest/chain/QuestChainPlayerState.java getLastCompletedAt(), quest/QuestInstance.java timestamp accessors (AUDIT_RESULTS.md X2, X3)

---

CONCERN: ContentHandlerType.QUEST_CHAIN_START_CONDITION was removed from the initial implementation scope (PLAN and HLD both say "deferred"), but it is unclear whether the enum constant was added and then removed, or never added — and the QuestChainStartConditionContentPack class ships without a corresponding ContentHandlerType entry.
WHY: If an addon registers a QuestChainStartConditionContentPack and the ContentHandlerType entry is absent, the registration call will either throw at runtime or silently no-op. The HLD (File Changes Summary) lists ContentHandlerType.QUEST_CHAIN as added but explicitly omits QUEST_CHAIN_START_CONDITION. An addon developer reading the QuestChainStartConditionContentPack class signature would expect to find a matching handler type; its absence makes the registration path non-functional without any compile-time signal.
WHERE: expansion/content/QuestChainStartConditionContentPack.java, ContentHandlerType.java (HLD §"Extension infrastructure" bullet point)

[github-actions]

Testing Review

Looking at this diff, I need to identify what changed in production code vs. test code, then apply every checklist item.

The diff is almost entirely documentation files (.md, .mdc) — AUDIT_RESULTS.md, PLAN_01_CONCURRENCY.md through PLAN_06_GUI_UX.md, chain-system-backlog.md, tutorial-quest-system.md, CLAUDE.md, and cursor rule files. No production Java source files and no test Java source files are present in this diff.

---

Production files changed: CLAUDE.md, .cursor/rules/core.mdc, .cursor/rules/persona-error-handling.mdc, .cursor/rules/persona-performance.mdc, docs/hld/tutorial/tutorial-quest-system.md, docs/hld/tutorial/chain-system-backlog.md, AUDIT_RESULTS.md, PLAN_01_CONCURRENCY.md through PLAN_06_GUI_UX.md, docs/hld/tutorial/PICKUP-GUIDE.md (deleted) | Test files present: None | Coverage gaps: N/A — no executable code changed

No testing concerns found.

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively documentation files (.md, .mdc), project convention rules, and planning/audit documents. No Java source code is changed — there are no deserialize() calls, no performCommand() / dispatchCommand() invocations, no SQL query construction, no permission checks, and no GUI onClick() handlers in this diff. All checklist items are inapplicable to documentation-only changes.

[github-actions]

GUI/UX Review

The diff is almost entirely documentation, cursor rules, and planning markdown files. The only GUI-relevant content surfaced is in AUDIT_RESULTS.md (which reproduces earlier findings) and PLAN_06_GUI_UX.md (which describes fixes for those findings). No actual GUI Java source files, YAML localization files, or slot implementations are present in this diff.

Before I can apply the checklist, I need the source files themselves.

Please paste or attach one or more of the following:

• GatedChainStepSlot.java
• QuestChainHistoryDetailGui.java
• QuestChainHistorySlot.java
• DisableTutorialConfirmGui.java and its slot classes (DisableTutorialConfirmSlot.java, DisableTutorialInfoSlot.java, DisableTutorialCancelSlot.java)
• DisableTutorialSettingSlot.java
• Any modified GUI slots from the existing quest/slot/ directory
• en_quest.yml and en_gui.yml (the relevant sections, or the full files)

Which of these do you have available?

[github-actions]

Server Owner Review

Looking at this diff, I need to focus on what a server owner actually cares about: the YAML files, plugin.yml, config changes, and whether their server will survive an update. The diff is heavily documentation and internal planning files, but there are real config and permission implications buried in the HLD and tutorial system docs.

Let me work through the checklist systematically.

---

Server Owner Review

---

CONCERN: config.yml adds a tutorial.enabled toggle, but there is no evidence of a config-version bump in this diff.
WHY: The HLD explicitly says "No config-version bump needed — no migration logic required for this addition," but adding a new key changes the structure a server owner might be editing. More critically, if a server owner upgrades and their old config.yml lacks tutorial.enabled, the plugin must either crash, default silently, or log nothing — there is no migration path documented. The comment "no migration required" contradicts the checklist rule: any structural change requires a bump and a plan.
WHERE: docs/hld/tutorial/tutorial-quest-system.md → Section 9 / src/main/resources/config.yml (not in diff — absence is the problem)

---

CONCERN: The tutorial.enabled key has no # comment in any YAML shown in this diff explaining what it does, valid values (true/false), or what breaks if set wrong.
WHY: A server owner opening config.yml after update sees a new key with no explanation. They won't know whether false disables the tutorial chain entirely, prevents auto-start only, or suppresses the rewards too. The HLD says it gates TutorialPreQuestStartListener but that's Java source — not available to server owners.
WHERE: src/main/resources/config.yml → tutorial.enabled (key not present in diff at all)

---

CONCERN: Eight new permission nodes are introduced (mcrpg.quest.chain.restart, mcrpg.quest.chain.reset, mcrpg.quest.chain.advance, mcrpg.quest.chain.skip, mcrpg.quest.chain.status, mcrpg.tutorial.bypass, mcrpg.quest.chain.*, mcrpg.admin.*) but none of them appear in plugin.yml in this diff.
WHY: Permissions not declared in plugin.yml don't get description: fields, don't resolve default: inheritance, and won't auto-populate in permission managers like LuckPerms. A server owner granting mcrpg.admin.* as a wildcard will get nothing unless plugin.yml declares the children: hierarchy. The HLD says "Standard Bukkit permission inheritance applies — granting mcrpg.* implicitly grants every child node. The plugin.yml children block declares the full hierarchy" — but that block is not in this diff.
WHERE: src/main/resources/plugin.yml (not modified in this diff)

---

CONCERN: The HLD references mcrpg.admin.tutorial.reset in the original design, then replaces it with mcrpg.quest.chain.reset in the updated table. If any server has already granted the old node (e.g., to a Moderator role in LuckPerms), that grant is silently revoked and replaced with a node that was never granted.
WHY: Permission renames are invisible to LuckPerms. Staff members who had mcrpg.admin.tutorial.reset will lose access silently. No migration note or upgrade warning appears in any config or upgrade notes file in this diff.
WHERE: docs/hld/tutorial/tutorial-quest-system.md → Permissions table (old node vs. new node)

---

CONCERN: Seven tutorial quest YAML files are added (first_steps.yml, mcrpg_menu.yml, etc.) but none of them appear in this diff — only their filenames are listed. It is impossible to verify whether they have # comments on keys, correct config-version fields, safe default numerics, or valid reward amounts.
WHY: These files are the primary thing a server owner will open to customize tutorial rewards and objectives. If they lack comments, server owners editing reward amounts (e.g., changing 1000 boosted XP) have no guidance on valid ranges, what boosted_experience means vs. redeemable_experience, or what happens if they set amount: 0.
WHERE: src/main/resources/quests/tutorial/*.yml (listed but not shown in diff)

---

CONCERN: chain.yml (the tutorial chain definition) is referenced throughout but its content is not in this diff. The YAML schema shown in the HLD uses trigger: mcrpg:first_join — it is unclear whether this key must exactly match a registered ChainAutoStartTrigger key, and there is no comment in the schema example warning server owners that a typo here silently disables the tutorial auto-start.
WHY: If a server owner copies the chain YAML format and misspells mcrpg:first_join (e.g., writes first-join as shown in an earlier version of the same doc), the tutorial never starts for new players and there is no error message. The HLD backlog item E6 notes WindowClosePolicy.fromString() swallows parse failures — the same risk likely applies to trigger key resolution.
WHERE: src/main/resources/quests/tutorial/chain.yml (not shown); docs/hld/tutorial/tutorial-quest-system.md → auto-start trigger YAML

---

CONCERN: Two new database tables are introduced (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log) via UpdateTableFunction. The audit finding E1 explicitly states: "DB migration bumps version on ALTER TABLE failure — version incremented even when ALTER TABLE fails. Column never added on retry." This is a confirmed data-loss-adjacent bug in QuestChainCompletionLogDAO and QuestChainStateDAO that is documented but not fixed in this diff.
WHY: If the initial table creation or schema migration fails partway through (e.g., disk full, DB locked), the version counter advances and the broken schema is never repaired on subsequent startups. Players' chain progress data would be silently dropped or cause errors. A server owner has no way to detect this from YAML or logs without knowing to look for it.
WHERE: AUDIT_RESULTS.md → E1; src/main/java/us/eunoians/mcrpg/database/table/quest/chain/QuestChainCompletionLogDAO.java and QuestChainStateDAO.java (not shown in diff)

---

CONCERN: The localization files (en_quest.yml) are modified to add tutorial text and chain UI strings, but no config-version field is shown on the localization files in this diff, and no mention is made of whether existing server-customized locale files will be overwritten or merged on update.
WHY: Server owners commonly edit locale files to customize quest messages. If the update overwrites en_quest.yml with new keys and the server owner's edits are in the same file, their customizations are lost. If the update does NOT overwrite, new locale keys (e.g., quest-chain.cascade.batch-header) are missing and players see raw key strings in chat.
WHERE: src/main/resources/localization/english/en_quest.yml (not shown in diff)

---

CONCERN: The <tutorial> palette placeholder (<color:#E8C97A>) is added to config.yml but there is no fallback documented if a server owner deletes or misspells this entry. Players would see raw <tutorial> MiniMessage tags in quest names.
WHY: Palette entries that are referenced in quest YAML and locale strings but missing from config produce visible markup corruption in-game. Server owners customizing colors need to know: (a) this placeholder exists, (b) it affects tutorial quest name rendering, (c) removing it breaks display.
WHERE: docs/hld/tutorial/tutorial-quest-system.md → Section 5 palette table; src/main/resources/config.yml (not shown)

---

CONCERN: The admin commands are restructured — the path changes from /mcrpg admin quest chain ... to /mcrpg quest chain ... (the admin literal is removed per "Chain admin command path restructure" in Phase 3). Any server documentation, scripts, or staff macros using the old command path will silently fail.
WHY: Command renames are invisible breaking changes for server owners. Staff members who have /mcrpg admin quest chain reset in a macro or admin guide will get "Unknown command" after the update. No upgrade note or alias is present in this diff.
WHERE: docs/hld/tutorial/tutorial-quest-system.md → Phase 3 notes ("Chain admin command path restructure"); plugin.yml command registration (not shown)

---

CONCERN: The DisableTutorialSetting player setting is new, but there is no documentation of how it is stored (DB column? player data file?) or what happens to a player's tutorial state if the setting row is missing on load (default assumed? error?).
WHY: Server owners migrating from a pre-tutorial version will have players with no DisableTutorialSetting row. If the default behavior is "tutorial enabled" but the setting fails to load, those players may get the tutorial unexpectedly re-triggered or have it silently suppressed. Neither outcome is documented in any config or upgrade note.
WHERE: docs/hld/tutorial/tutorial-quest-system.md → Section 9; setting/impl/DisableTutorialSetting.java (not shown)

---

CONCERN: The HLD reward summary shows significantly higher values than earlier versions (e.g., Q1 went from 500 to 1,000 boosted XP, Q7 from 1,000 to 2,500), but no comment in any YAML shown in this diff explains the scaling equation or provides sample outputs at key thresholds.
WHY: Server owners tuning reward balance need to understand what "1,000 boosted XP" means in practice. The HLD provides level-up math inline (200+(0.8*(skill_level^1.5))), but this is in a design doc, not in the reward YAML as a # comment. Server owners editing amount: 1000 in first_steps.yml have no on-file guidance about safe ranges.
WHERE: src/main/resources/quests/tutorial/*.yml (not shown in diff)

---

Summary

Migration required: YES — new DB tables, new config keys, new permission nodes, renamed command paths, renamed permissions.

Reload-safe: PARTIAL — chain definitions reload (documented), but active quest reconciliation during reload has known gaps (audit finding E3: offline players missed; finding re: stale active instances). The DB migration bug (E1) means a failed migration is unrecoverable without manual DB intervention.

Critical items for server owners before this ships:

1. plugin.yml must be shown — new permissions need description: and default: fields and the children: wildcard hierarchy
2. config.yml must be shown with config-version bump and # comments on tutorial.enabled and <tutorial> palette
3. The renamed permission (mcrpg.admin.tutorial.reset → mcrpg.quest.chain.reset) and renamed command path need an explicit upgrade note
4. E1 (DB migration version bump on failure) must be fixed before any production deployment
5. Tutorial quest YAML files must be shown to verify they have comments, safe defaults, and valid reward ranges

[github-actions]

Extensibility Review

Breaking change risk: MEDIUM — multiple public API signatures changed (Long→Instant, method renames, new mandatory parameters) without deprecation bridges, and new chain events have nullable-player inconsistencies that will silently drop offline notifications from addon listeners.

---

CONCERN: QuestChainRestartEvent and QuestChainStepRetryEvent require @NotNull Player while sibling events (QuestChainFailEvent, QuestChainExpireEvent, QuestChainAbandonEvent) accept @Nullable Player
WHY: An addon listening to these events to track offline chain lifecycle will never receive them for offline players — the manager silently skips firing the events. The event API surface appears uniform (all chain events) but has an invisible behavioral gap. An addon developer reading only the event class signatures has no way to discover this asymmetry.
WHERE: event/quest/chain/QuestChainRestartEvent.java, event/quest/chain/QuestChainStepRetryEvent.java, QuestChainManager.handleExpireRestartChain(), QuestChainManager.handleExpireRetry() (documented as X9/X10 in AUDIT_RESULTS.md but not yet fixed in this diff)

---

CONCERN: getCompletedQuestKeys() renamed to getNonSkippedCompletedQuestKeys() with no @Deprecated alias
WHY: Any addon calling getCompletedQuestKeys() breaks at compile time with no migration path. The rename is a hard binary-incompatible break — no default method, no forwarding stub. The old name is simply gone.
WHERE: Documented as X6 in AUDIT_RESULTS.md; the public interface or class carrying this method (likely QuestChainPlayerData or QuestChainPlayerState)

---

CONCERN: QuestChainStartCondition.evaluate() gained a mandatory Instant parameter with no default method
WHY: Any third-party implementation of this interface compiled against the previous signature now fails with AbstractMethodError at runtime (or a compile error if they try to recompile). There is no default bridge returning evaluate(player) ignoring the timestamp.
WHERE: quest/chain/QuestChainStartCondition.java (documented as X1 in AUDIT_RESULTS.md)

---

CONCERN: QuestPool constructor gained a mandatory TimeProvider parameter with no overload preserving the old signature
WHY: Any addon that constructs a QuestPool directly breaks at compile time. Even if QuestPool is primarily internal, it is a public class — if it appeared in any public API return type or factory, addons may have constructed it.
WHERE: quest/board/generation/QuestPool.java (documented as X8 in AUDIT_RESULTS.md)

---

CONCERN: QuestChainPlayerState.getLastCompletedAt() changed return type from Optional<Long> to Optional<Instant> with no deprecation bridge
WHY: Source-incompatible change. Addons storing or comparing the returned value against epoch-millis arithmetic (state.getLastCompletedAt().map(t -> System.currentTimeMillis() - t)) silently compile to a type error or, if they cast, produce a ClassCastException at runtime.
WHERE: quest/chain/QuestChainPlayerState.java (documented as X2 in AUDIT_RESULTS.md)

---

CONCERN: softDisableAbility() fires AbilityUnregisterEvent — addons cannot distinguish a soft (reversible) disable from a permanent unregistration
WHY: An addon reacting to AbilityUnregisterEvent to clean up dependent state (remove player buffs, cancel cooldowns, update a custom UI) will perform permanent teardown when the ability is only temporarily soft-disabled, then miss the re-enable because no corresponding re-enable event exists. The flag (isSoftDisable()) documented in PLAN_03 is not yet present on the event in this diff.
WHERE: ability/AbilityRegistry.softDisableAbility(), event/ability/AbilityUnregisterEvent.java (documented as X11 in AUDIT_RESULTS.md)

---

CONCERN: ChainAutoStartTrigger is a new extensible interface with no documented contract for thread safety or which thread tryStartChain() is expected to be called from
WHY: A third-party trigger (e.g., myplugin:region_enter) that calls QuestChainManager.tryStartChain() from an async context will trigger the thread-safety bugs documented in C1–C6 — firing Bukkit events and mutating player state from an async thread. Without a @ApiStatus annotation, a @MainThread annotation, or Javadoc stating the threading requirement, addon authors have no signal that the call must be on the main thread.
WHERE: quest/chain/trigger/ChainAutoStartTrigger.java

---

CONCERN: QuestChainStartCondition is marked @ApiStatus.Experimental in CLAUDE.md but is a registered extensible interface — addons implementing it have no stability guarantee and the interface is not yet wired into QuestChainManager
WHY: An addon registering a custom condition via QuestChainStartConditionContentPack will register successfully but the condition will never be evaluated (the manager doesn't call it yet per the backlog). The addon silently does nothing. The @ApiStatus.Experimental marker helps, but the fact that registration succeeds without evaluation is invisible without reading the backlog document.
WHERE: quest/chain/QuestChainStartCondition.java, QuestChainManager (condition evaluation not wired)

---

CONCERN: McRPGLocalizationManager.formatDisplayDate() parameter changed from long (epoch millis) to Instant with no overload
WHY: Addons calling this utility method for display formatting break at compile time. This is a utility method on a manager — exactly the kind of stable API that addon authors depend on without reading changelogs.
WHERE: McRPGLocalizationManager.formatDisplayDate() (documented as X7 in AUDIT_RESULTS.md)

---

CONCERN: CoreGuiOpenEvent is placed in McCore and carries Optional<NamespacedKey> — addons that have already wrapped McCore's GUI events at a specific package path will need to update their imports, and the Optional return for the key is not annotated with @NotNull on the Optional itself
WHY: If the Optional<NamespacedKey> return is not @NotNull-annotated, an addon doing event.getGuiKey().ifPresent(...) may encounter a NullPointerException if an implementation returns null instead of Optional.empty(). The contract that Optional itself is never null must be explicit via @NotNull Optional<NamespacedKey>.
WHERE: CoreGuiOpenEvent (McCore), KeyedGui.getGuiKey()

---

CONCERN: QuestChainExpireEvent is listed in the backlog as "Pending (needs availability windows)" but QuestChainState.EXPIRED is already a live state — chains can reach EXPIRED state today with no corresponding event fired
WHY: An addon listening for QuestChainExpireEvent to react to chain expiration (remove player buffs, log analytics, send messages) will register a listener that never fires, even when chains transition to EXPIRED. There is no way to intercept this transition from the outside.
WHERE: event/quest/chain/ (missing QuestChainExpireEvent), QuestChainManager expiration path, chain-system-backlog.md §5

[github-actions]

Testing Review

Looking at this diff, I need to examine what's actually changed in terms of testable production logic versus what test coverage exists.

The diff is primarily documentation files (AUDIT_RESULTS.md, PLAN_*.md, chain-system-backlog.md, updated CLAUDE.md/core.mdc/tutorial-quest-system.md). There are no production Java source files and no test Java source files changed in this diff.

However, the documentation itself — specifically AUDIT_RESULTS.md and PLAN_05_TESTING.md — explicitly catalogs testing gaps in the already-implemented production code. I need to audit those documented gaps against the checklist.

---

CONCERN: AvailabilityConfig.isCurrentlyAvailable() has zero test coverage
WHY: This is the central decision method of the availability window subsystem. It has non-trivial branching logic (multiple windows, open/closed states) and is called in the checker's async-to-main-thread dispatch path. No corresponding test class exists. AUDIT_RESULTS.md T1 and PLAN_05_TESTING.md §T1 both acknowledge this gap but no test file has been added.
WHERE: src/main/java/us/eunoians/mcrpg/quest/availability/AvailabilityConfig.java / no test file

---

CONCERN: AvailabilityWindowDefinition.isActive() has zero test coverage, specifically the year-wrapping range logic
WHY: Year-wrapping date ranges (e.g., December 1 – January 31) are a known edge case class that is trivially incorrect to implement. This is pure-Java logic with no Bukkit dependency — a plain JUnit test is all that's needed. The gap is acknowledged in AUDIT_RESULTS.md T2 but no test has been added.
WHERE: src/main/java/us/eunoians/mcrpg/quest/availability/AvailabilityWindowDefinition.java / no test file

---

CONCERN: WindowBoundary.Fixed and WindowBoundary.Recurring toZonedDateTime() methods have zero test coverage
WHY: These are the foundation of the window system. Recurring in particular involves year-offset arithmetic across timezone boundaries — exactly the kind of logic that fails silently on DST transitions. Pure-Java, no Bukkit needed. Gap acknowledged in T3.
WHERE: src/main/java/us/eunoians/mcrpg/quest/availability/WindowBoundary.java / no test file

---

CONCERN: TimeGateCondition.evaluate() has zero test coverage
WHY: Timezone conversion edge cases (e.g., evaluating "after 2026-06-15T00:00:00" when the server runs UTC but the config specifies America/New_York) are a correctness risk. This is pure-Java logic. Gap acknowledged in T4. No test added.
WHERE: src/main/java/us/eunoians/mcrpg/quest/chain/builtin/TimeGateChainCondition.java / no test file

---

CONCERN: TimeGateChainConditionType.parse() has zero test coverage for its three validation paths
WHY: Missing after field, invalid date format, and invalid timezone are three distinct failure paths. Per the checklist, every new public method with non-trivial logic (>3 lines) requires a corresponding test. Gap acknowledged in T5. No test added.
WHERE: src/main/java/us/eunoians/mcrpg/quest/chain/builtin/TimeGateChainConditionType.java / no test file

---

CONCERN: WindowClosePolicy.fromString() has zero test coverage, including the silent-failure branch
WHY: The method returns Optional.empty() on unrecognized input with no diagnostic (also flagged as E6 in AUDIT_RESULTS.md). Neither the happy path (case-insensitive match, kebab-case normalization) nor the failure path is tested. Gap acknowledged in T6.
WHERE: src/main/java/us/eunoians/mcrpg/quest/chain/availability/WindowClosePolicy.java / no test file

---

CONCERN: FixedWindowBoundaryType.parse() and RecurringWindowBoundaryType.parse() have zero test coverage
WHY: Config deserialization methods with multiple validation paths (missing fields, invalid format) must be tested at the boundary. Both happy path and failure branches are untested. Gap acknowledged in T7. No test added.
WHERE: src/main/java/us/eunoians/mcrpg/quest/availability/FixedWindowBoundaryType.java, RecurringWindowBoundaryType.java / no test file

---

CONCERN: QuestPool.filterByAvailability() has no test coverage despite QuestPoolTest being modified
WHY: The modified test file did not add a test for the new availability filtering method. There are three branches to cover: template with closed window is filtered out, template with open window is kept, template with no availability config is kept. Gap acknowledged in T8.
WHERE: src/main/java/us/eunoians/mcrpg/quest/board/generation/QuestPool.java / src/test/java/us/eunoians/mcrpg/quest/board/generation/QuestPoolTest.java

---

CONCERN: QuestChainManager expire handlers (handleExpireRetry, handleExpireSkip, handleExpireRestartChain) have zero test coverage across approximately 150 lines of branching logic
WHY: Each handler has multiple branches: retry counter increment, max-retries exhaustion falling through to fail, skip creating a log entry, restart clearing completion log. These are all public-behavior paths with observable side effects (state transitions, event firing). Gap acknowledged in T9.
WHERE: src/main/java/us/eunoians/mcrpg/quest/chain/QuestChainManager.java / src/test/java/us/eunoians/mcrpg/quest/chain/QuestChainManagerTest.java

---

CONCERN: AbilityRegistry.softDisableAbility() and reEnableAbility() are untested new public API
WHY: Per the checklist, every new public method with non-trivial logic requires a corresponding test. softDisableAbility() involves multi-map bookkeeping, event firing (AbilityUnregisterEvent), and state mutation. reEnableAbility() reverses it. Neither the pass nor fail branches (e.g., re-enabling an ability that was never soft-disabled) are tested. Gap acknowledged in T10.
WHERE: src/main/java/us/eunoians/mcrpg/ability/AbilityRegistry.java / no corresponding test additions

---

CONCERN: ContentExpansionManager.getRegisteredExpansions() and getContentPacks() are new public API with zero tests
WHY: These are the introspection methods backing the new admin content commands. Correctness of the returned collections (especially getContentPacks() handling of null or empty expansion) is untested. Gap acknowledged in T11.
WHERE: src/main/java/us/eunoians/mcrpg/expansion/ContentExpansionManager.java / no test file

---

CONCERN: QuestManager.warnStaleDefinitions() has zero test coverage
WHY: The method iterates active quests against a definition map and produces warnings. The three branches (no active quests, active quest with valid definition, active quest with missing definition) are all untested. Gap acknowledged in T12.
WHERE: src/main/java/us/eunoians/mcrpg/quest/QuestManager.java / existing QuestManager test file

---

CONCERN: Multiple test files call Instant.now() directly instead of using the spy'd TimeProvider
WHY: Per the TimeProvider checklist item, all time-based logic in tests must go through TimeProvider so tests can inject a fixed clock. QuestChainCompletionLogDAOTest, QuestCompletionLogDAOTest, QuestInstanceDAOTest, and QuestChainManagerTest all use Instant.now() directly. While AUDIT_RESULTS.md T13 labels this "low severity," it is a structural violation of the stated testing convention — tests that insert timestamps via Instant.now() and then assert on those values will produce nondeterministic results under load or on slow CI machines. Any test that stores and later reads back a timestamp is affected.
WHERE: QuestChainCompletionLogDAOTest, QuestCompletionLogDAOTest, QuestInstanceDAOTest, QuestChainManagerTest

---

CONCERN: PLAN_05_TESTING.md proposes tests for the availability window subsystem (T1–T7) as future work but the production code implementing that subsystem is marked as already implemented ("All three phases implemented" in tutorial-quest-system.md)
WHY: The checklist rule is unambiguous: non-Bukkit logic added without corresponding test additions is a coverage gap. The availability window subsystem is pure-Java and entirely testable without MockBukkit. Deferring the tests to a future plan while shipping the production code violates the project's own stated completion criterion ("The entire test suite must pass before a task is considered complete").
WHERE: All availability window production classes / PLAN_05_TESTING.md

---

Production files changed: .cursor/rules/core.mdc, .cursor/rules/persona-error-handling.mdc, .cursor/rules/persona-performance.mdc, CLAUDE.md, docs/hld/tutorial/tutorial-quest-system.md, docs/hld/tutorial/chain-system-backlog.md (new), docs/hld/tutorial/PICKUP-GUIDE.md (deleted), AUDIT_RESULTS.md (new), PLAN_01_CONCURRENCY.md through PLAN_06_GUI_UX.md (new)

Test files present: None added or modified in this diff

Coverage gaps: AvailabilityConfig.isCurrentlyAvailable(), AvailabilityWindowDefinition.isActive() (year-wrap), WindowBoundary.Fixed/Recurring.toZonedDateTime(), TimeGateChainCondition.evaluate(), TimeGateChainConditionType.parse() (3 paths), WindowClosePolicy.fromString() (happy + silent-fail paths), FixedWindowBoundaryType.parse() / RecurringWindowBoundaryType.parse(), QuestPool.filterByAvailability() (3 branches), QuestChainManager expire handlers (handleExpireRetry, handleExpireSkip, handleExpireRestartChain), AbilityRegistry.softDisableAbility() / reEnableAbility(), ContentExpansionManager.getRegisteredExpansions() / getContentPacks(), QuestManager.warnStaleDefinitions(), Instant.now() direct usage in 4 DAO/manager test files

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively documentation files (.md, .mdc), cursor rules, and planning/backlog documents. No Java source code is present. Every checklist item — MiniMessage injection, performCommand/dispatchCommand concatenation, permission bypass, and SQL/data injection — requires executable code paths to evaluate. Documentation and configuration rules cannot themselves introduce player-exploitable vulnerabilities.

When the implementation files referenced in these planning documents (e.g., QuestChainManager.java, GatedChainStepSlot.java, QuestChainStateDAO.java, AvailabilityWindowChecker.java) are submitted in a diff, apply the full checklist at that time. Findings G1 and G2 noted in AUDIT_RESULTS.md (raw string sent via resolvePaletteColors() instead of a Component, dead CHAIN_PREVIEW_REWARDS_HIDDEN key) warrant attention in that future review — particularly G1, which could expose unrendered MiniMessage tags to players if the slot is ever wired into a live GUI.

[github-actions]

GUI/UX Review

The diff is primarily documentation, rule files, and planning markdown. The only GUI-relevant material is the AUDIT_RESULTS.md file (which references previously identified GUI findings G1–G4) and the PLAN_06_GUI_UX.md file (which documents remediation plans for those findings). No actual GUI Java source files, YAML localization files, or slot class implementations are present in this diff.

Applying the checklist against what is present:

---

CONCERN: G1 from AUDIT_RESULTS.md — GatedChainStepSlot.onClick() sends a raw String via resolvePaletteColors() instead of a rendered Component, meaning players receive literal MiniMessage markup in chat (e.g., <gold>This step is locked.</gold>) rather than formatted text.
WHY: This is a player-visible rendering failure. Every player who clicks a gated chain step sees unrendered markup, which breaks immersion and signals a defect to the server owner. The checklist item "MiniMessage must never be called directly" and "all text rendering delegated to the localization manager" are both violated.
WHERE: AUDIT_RESULTS.md §GUI/UX G1 / GatedChainStepSlot line 69 (class not present in diff — flagged because the audit confirms the defect exists and PLAN_06_GUI_UX.md confirms it is not yet fixed)

---

CONCERN: G2 from AUDIT_RESULTS.md — CHAIN_PREVIEW_REWARDS_HIDDEN is registered as a localization key and present in en_quest.yml but is never referenced in any Java slot or GUI class.
WHY: A dead localization key creates confusion for server owners customizing en_quest.yml — they will translate a string that never appears in-game. It also represents a divergence between the localization file and the actual feature surface, making future audits harder.
WHERE: AUDIT_RESULTS.md §GUI/UX G2 / LocalizationKey enum + en_quest.yml (files not present in diff — flagged because the audit confirms the defect exists and PLAN_06_GUI_UX.md confirms no resolution has been applied)

---

CONCERN: G3 from AUDIT_RESULTS.md — GatedChainStepSlot is never instantiated by any GUI class, making it dead code that contributes nothing to the player-facing interface.
WHY: A slot class that is never placed in a GUI cannot be tested through normal play and will not surface localization or rendering bugs until it is finally wired in — at which point G1 and G4 become live defects rather than pre-production ones. The checklist requires empty/null state and navigation to be handled; a slot that is never reached cannot satisfy any UX requirement.
WHERE: AUDIT_RESULTS.md §GUI/UX G3 / GatedChainStepSlot (class not present in diff)

---

CONCERN: G4 from AUDIT_RESULTS.md — When step.previewItem() is non-null, GatedChainStepSlot.getItem() returns the cloned ItemStack directly without running it through palette resolution or the localization manager, meaning any MiniMessage tags or palette placeholders in the item's display name or lore will be displayed as raw markup to the player.
WHY: Any server owner or content pack author who supplies a previewItem with <gold>, <primary>, or other tags will see unrendered strings in-game. This is inconsistent with every other item construction path in the codebase and violates the rule that all player-facing text must be rendered through the localization manager.
WHERE: AUDIT_RESULTS.md §GUI/UX G4 / GatedChainStepSlot.getItem() lines 97–98 (class not present in diff)

---

CONCERN: PLAN_06_GUI_UX.md proposes fixing G4 by running resolvePaletteColors() on the preview item's display name and lore. resolvePaletteColors() is a direct MiniMessage call path (McRPGMethods.getMiniMessage()). The checklist explicitly states "MiniMessage must never be called directly — not even via McRPGMethods.getMiniMessage()." The proposed fix would introduce a new violation of the same category as G1.
WHY: The remediation plan will create the same class of bug it is trying to fix. The correct fix is to route through the localization manager's component-rendering method, not to call resolvePaletteColors() on the raw item.
WHERE: PLAN_06_GUI_UX.md §G4 fix description

---

No other GUI source files, slot classes, or localization YAML files are present in this diff. The five concerns above cover all GUI/UX issues surfaced by the changed content.

[github-actions]

Server Owner Review

Looking at this diff, I need to focus on what actually matters for a server owner: YAML files, plugin.yml, config files, permission nodes, migration safety, and reload behavior. The bulk of this diff is documentation, cursor rules, and Java source — I'll filter to what's relevant.

Let me work through the checklist against the YAML resources and config-touching changes visible in the diff.

---

Findings

---

CONCERN: config.yml tutorial toggle added with no config-version bump
WHY: The diff explicitly states "No config-version bump needed — no migration logic required for this addition" in the HLD. However, a new top-level tutorial: section with an enabled: key IS a structural change to config.yml. Without a version bump, automated migration tooling cannot distinguish servers that have the new key from those that don't. A server owner upgrading from the prior version gets no indication that their config is missing the new section, and any tooling that checks config-version to decide whether to patch the file will silently skip adding the tutorial: block. The server owner opens their config, doesn't see tutorial:, and has no idea the feature exists.
WHERE: src/main/resources/config.yml / tutorial.enabled

---

CONCERN: tutorial.enabled config key has no comment in the diff showing its valid values, default, or what breaks if removed
WHY: The HLD says the key exists and defaults to true, but no YAML comment is shown in the diff. A server owner who opens config.yml for the first time after upgrade and sees a bare enabled: true under an unexplained tutorial: section has no idea whether setting it to false globally disables the chain for all new players, prevents existing players from being affected, or only gates the auto-start trigger. "What breaks if set wrong" is entirely absent.
WHERE: src/main/resources/config.yml / tutorial.enabled

---

CONCERN: New <tutorial> palette placeholder added to config.yml with no comment explaining what it controls or valid MiniMessage color syntax
WHY: The HLD documents it as <color:#E8C97A> default, but server owners editing the palette file will see a new tutorial: entry with no explanation of which UI elements use it. If a server owner sets an invalid MiniMessage string (e.g., <bold> without a color), all tutorial quest name rendering silently breaks with raw markup visible to players — exactly the G1 bug category the audit already flagged in GUIs.
WHERE: src/main/resources/config.yml (palette section) / tutorial palette entry

---

CONCERN: Seven new tutorial quest YAML files are referenced but their content is not shown — it is impossible to verify whether they have inline comments, correct key naming, or safe default values
WHY: The files listed in the diff (first_steps.yml, mcrpg_menu.yml, natural_talent.yml, your_arsenal.yml, unleashed_power.yml, combo_strike.yml, quest_board.yml) are described in the HLD but their actual YAML content is truncated. As a server owner, if these ship without # comments on every key, I cannot tune reward amounts, change objectives, or understand the on-start-messages syntax without reading Java source. This is the single most server-owner-facing change in this PR.
WHERE: src/main/resources/quests/tutorial/*.yml — all seven quest definition files

---

CONCERN: chain.yml uses trigger: mcrpg:first_join with no comment explaining valid trigger values or what happens if an invalid key is provided
WHY: The HLD shows the YAML schema, but if the shipped chain.yml file has no # Valid values: mcrpg:first_join | mcrpg:login | mcrpg:manual comment, a server owner copying this pattern to create their own chain will type an invalid trigger key and get a silent failure (chain never starts) with no error in console. The HLD acknowledges fromString() silent parse failures as finding E6.
WHERE: src/main/resources/quests/tutorial/chain.yml / auto-start.trigger

---

CONCERN: on-quest-expire step field has no comment in the YAML schema showing all valid values and which ones are currently functional vs. deferred
WHY: The HLD explicitly states "For the initial implementation, only fail-chain is functional" — retry, restart-chain, and skip are parsed but not implemented. If chain.yml ships without a comment like # Valid values: fail-chain (others parsed but not yet functional — see backlog), a server owner who writes on-quest-expire: retry expecting retry behavior will see fail-chain behavior silently. This is a correctness trap with no visible indication.
WHERE: src/main/resources/quests/tutorial/chain.yml / steps.<step>.on-quest-expire

---

CONCERN: repeat-mode field is parsed but only ONCE is functional — no comment warning server owners that other values silently fall back to ONCE
WHY: Same problem as above. A server owner who reads the backlog doc or the HLD and writes repeat-mode: unlimited expecting unlimited repeat behavior gets ONCE silently. The config file itself must communicate this, not a separate design document.
WHERE: src/main/resources/quests/tutorial/chain.yml / repeat-mode

---

CONCERN: New permission nodes for chain admin commands are added but the diff does not show the plugin.yml changes — it is impossible to verify naming convention, default: values, description: fields, or parent children: inheritance
WHY: The HLD lists eight new permission nodes (mcrpg.quest.chain.*, mcrpg.quest.chain.restart, mcrpg.quest.chain.reset, mcrpg.quest.chain.advance, mcrpg.quest.chain.skip, mcrpg.quest.chain.status, mcrpg.tutorial.bypass, plus the content introspection mcrpg.admin.content). The diff truncates before showing the actual plugin.yml diff. If any of these nodes are missing description: fields, have incorrect default: values, or are absent from the children: block under mcrpg.*, server owners using LuckPerms wildcard grants (mcrpg.*) will not automatically inherit the new nodes — silently locking admins out of chain commands.
WHERE: plugin.yml / permissions block — entire section not visible in diff

---

CONCERN: mcrpg.tutorial.bypass is listed with default: op but the chain admin commands (mcrpg.quest.chain.*) are described as "permission: op" in prose — if default: is omitted from plugin.yml, these nodes default to false (no-one has access, including ops by default unless ops-permissions: true in bukkit.yml)
WHY: Bukkit's behavior when default: is absent from a permission node in plugin.yml is default: op only if the server has ops-permissions: true. On servers where that's false (common on networks), the omission means no one has the permission until explicitly granted. An admin who expects /mcrpg quest chain reset to work as an op and finds it silently denied will have no idea why.
WHERE: plugin.yml / permissions.mcrpg.quest.chain.* through mcrpg.quest.chain.status

---

CONCERN: QuestChainCompletionLogDAO and QuestChainStateDAO introduce new DB tables via UpdateTableFunction — but finding E1 in the audit (version bumped even on ALTER TABLE failure) is documented as a known bug and not fixed in this PR
WHY: If the ALTER TABLE in the migration fails (e.g., column already exists on a partially-migrated server, or DB is locked), the version number is already bumped. On the next server start, the migration is skipped because the version check passes, but the column was never added. The server will crash or silently corrupt data when the code tries to read a column that doesn't exist. This is a data-loss risk for any server that experiences a crash or unclean shutdown during the first post-upgrade start.
WHERE: src/main/java/us/eunoians/mcrpg/database/table/quest/chain/QuestChainCompletionLogDAO.java / attemptUpdateTable() and QuestChainStateDAO.java / attemptUpdateTable()

---

CONCERN: Two new SQL tables (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log) are introduced with no visible rollback path or upgrade guide for server owners who need to downgrade
WHY: If a server owner installs this version, the tables are created. If they roll back to the previous JAR, the old code does not know about these tables and does not try to read them — so rollback is safe in terms of crashes. However, the server owner has no documented answer to "what do I do with these tables if I downgrade?" A brief migration note is needed.
WHERE: database/table/quest/chain/QuestChainStateDAO.java + QuestChainCompletionLogDAO.java

---

CONCERN: en_quest.yml locale file gains new keys for tutorial text and chain UI — no config-version equivalent shown, and it is unclear whether existing servers with customized en_quest.yml files will have the new keys populated automatically or silently missing
WHY: Servers that have copied and customized their locale files will not receive the new keys on upgrade. The tutorial UI (quest names, on-start messages, chain history GUI) will show raw locale key paths (e.g., tutorial.first-steps.welcome) instead of resolved text. This is visible to every new player. The upgrade guide must tell server owners to merge new locale keys.
WHERE: src/main/resources/localization/english/en_quest.yml — all new tutorial.* and quest-chain.* keys

---

CONCERN: CHAIN_PREVIEW_REWARDS_HIDDEN localization key is registered but never referenced in Java code (audit finding G2) — it will appear in en_quest.yml as a dead key confusing server owners who try to translate it
WHY: A server owner translating the locale file will spend time translating a key that is never shown to players. Worse, if they accidentally delete it thinking it's unused, and it later gets wired up, they'll have missing text with no warning. Either wire it in or remove it before shipping.
WHERE: src/main/resources/localization/english/en_quest.yml / CHAIN_PREVIEW_REWARDS_HIDDEN key path

---

CONCERN: GatedChainStepSlot is never instantiated (audit finding G3) — dead code — but it has a corresponding locale key that ships in en_quest.yml
WHY: Server owners translating the locale file will find GUI strings for a slot that players never see. This creates confusion and wasted translation effort.
WHERE: src/main/resources/localization/english/en_quest.yml / keys consumed only by GatedChainStepSlot

---

Summary

Migration required: YES

• Two new database tables created on first start (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log)
• New tutorial: section added to config.yml (server owners with existing configs will not have it without manual addition or automated migration)
• New locale keys in en_quest.yml (servers with customized locale files must manually merge)

Reload-safe: PARTIAL

• Chain definitions reload via /mcrpg admin reload (documented in HLD)
• Tutorial enabled toggle in config.yml is hot-reloadable per HLD
• Database schema changes (table creation) require restart — they run only at startup via UpdateTableFunction
• The E1 bug (version bumped on ALTER failure) means a failed migration is not retried on reload — restart is the only recovery path, and it won't help if the version was already written

[github-actions]

Extensibility Review

Breaking change risk: MEDIUM — multiple public API signatures changed (Long→Instant, method renames, constructor additions) without deprecation bridges, and two chain events inconsistently require @NotNull Player where parallel events accept null.

---

CONCERN: QuestChainRestartEvent and QuestChainStepRetryEvent require @NotNull Player, making them unfireable for offline players
WHY: Addon listeners registering for these events will never receive them when the triggering player is offline — the manager silently skips the event entirely. By contrast, QuestChainFailEvent and QuestChainExpireEvent accept @Nullable Player, so addons already expect nullable-player chain events. A listener that audits all chain state transitions will have blind spots, and there's no documented reason for the inconsistency.
WHERE: event/quest/chain/QuestChainRestartEvent.java, event/quest/chain/QuestChainStepRetryEvent.java (AUDIT_RESULTS.md X9, X10; PLAN_03_EXTENSIBILITY.md)

---

CONCERN: getCompletedQuestKeys() renamed to getNonSkippedCompletedQuestKeys() with no @Deprecated alias
WHY: Any addon calling getCompletedQuestKeys() will fail to compile against the new version with no migration path. The rename is a silent binary-incompatible break with no forwarding method and no deprecation warning to guide addon authors.
WHERE: AUDIT_RESULTS.md X6 — QuestChainPlayerState or equivalent holder

---

CONCERN: QuestChainStartCondition.evaluate() gained a mandatory Instant parameter without a default method bridge
WHY: Any third-party addon implementing this interface (it is explicitly advertised as an extensible interface via QuestChainStartConditionContentPack) will fail to compile. There is no default implementation on the old single-argument signature, so every existing implementor breaks immediately.
WHERE: AUDIT_RESULTS.md X1 — QuestChainStartCondition.evaluate(); expansion/content/QuestChainStartConditionContentPack.java

---

CONCERN: QuestChainPlayerState.getLastCompletedAt() changed from Optional<Long> to Optional<Instant> with no bridge
WHY: Addons reading the last-completed timestamp (e.g., to enforce their own cooldown logic or display it in a GUI) break at compile time. The SQL column stays BIGINT, so the wire format is stable, but the Java API is not. No deprecated getLastCompletedAtMillis() bridge is provided.
WHERE: AUDIT_RESULTS.md X2 — QuestChainPlayerState.getLastCompletedAt()

---

CONCERN: QuestInstance timestamp accessors changed from long to Instant with no bridge methods
WHY: Addons accessing getStartTime(), getEndTime(), or getExpirationTime() as long break at compile time. These are fundamental accessors any progress-tracking addon would use. No @Deprecated long getStartTimeMillis() alternatives are provided during the transition.
WHERE: AUDIT_RESULTS.md X3 — QuestInstance timestamp accessors

---

CONCERN: QuestPool constructor gained a mandatory TimeProvider parameter with no overload preserving the old signature
WHY: Any addon constructing a QuestPool directly (e.g., a custom board generation plugin) breaks immediately. There is no old-signature constructor forwarding to the new one, and no factory method that accepts a default TimeProvider.
WHERE: AUDIT_RESULTS.md X8 — QuestPool constructor

---

CONCERN: McRPGLocalizationManager.formatDisplayDate() parameter changed from long to Instant with no overload
WHY: Addons calling this utility method to format timestamps in their own GUIs or messages break at compile time. It is a public utility on the localization manager — a natural touchpoint for any display-layer addon.
WHERE: AUDIT_RESULTS.md X7 — McRPGLocalizationManager.formatDisplayDate()

---

CONCERN: softDisableAbility() fires AbilityUnregisterEvent indistinguishably from permanent removal
WHY: Addons listening for AbilityUnregisterEvent to, for example, remove the ability from player loadouts or notify players cannot distinguish a reversible soft-disable (fixable config) from a permanent unregistration. The event carries no isSoftDisable() flag. An addon that cleans up player data on unregister will incorrectly wipe data for abilities that will be re-enabled on next reload.
WHERE: AUDIT_RESULTS.md X11 — AbilityRegistry.softDisableAbility(); PLAN_03_EXTENSIBILITY.md X11

---

CONCERN: QuestChainStep record gained a previewItem component with no documented construction path for addons
WHY: Records use canonical constructors — adding a component is a source-incompatible change for any addon constructing QuestChainStep directly. If QuestChainStep.simple(key) is the only public construction path, that must be explicitly documented and the factory method must supply a default for the new component. Neither is visible in the diff surface.
WHERE: AUDIT_RESULTS.md X4 — QuestChainStep record, previewItem component

---

CONCERN: CoreGuiOpenEvent is placed in McCore but the backlog explicitly warns against McRPG-specific logic in McCore
WHY: The event carries Optional<NamespacedKey> resolved via McRPG's KeyedGui interface pattern. If McCore ships this event as a general-purpose hook, other McCore consumers unrelated to McRPG's quest system receive it on every GUI open. The core.mdc rule "No McRPG-specific logic in McCore" is violated. Addon authors who depend on McCore directly may be surprised to find a quest-oriented event firing in their non-quest GUIs, or may build integrations against it that break if McCore later narrows the scope.
WHERE: tutorial-quest-system.md — "McCore CoreGuiOpenEvent" section; core.mdc anti-pattern rule

---

CONCERN: QuestChainStartCondition is annotated @ApiStatus.Experimental but is advertised as a third-party extension point via QuestChainStartConditionContentPack
WHY: Addons registering conditions via the content pack are building against an interface explicitly flagged as subject to breaking change. The annotation's meaning is not documented — does "experimental" mean "signature may change" or "not yet wired"? The AUDIT_RESULTS.md (X1) confirms the signature already changed once (gained Instant param). Addon authors have no safe contract to code against.
WHERE: QuestChainStartCondition.java; expansion/content/QuestChainStartConditionContentPack.java; AUDIT_RESULTS.md X1

---

CONCERN: New McRPGManagerKey.QUEST_CHAIN and McRPGRegistryKey.QUEST_CHAIN constants lack Javadoc on what operations are safe
WHY: Addon authors accessing the chain manager via McRPGManagerKey.QUEST_CHAIN need to know: which methods are safe to call from async threads, which require main-thread context, and what the lifecycle contract is. The AUDIT_RESULTS.md (C1–C6) documents that the manager has active thread-safety bugs — an addon calling QuestChainManager from the wrong thread context will corrupt state silently. Without Javadoc on the registry key stating thread requirements, addon authors have no signal.
WHERE: McRPGManagerKey.QUEST_CHAIN; McRPGRegistryKey.QUEST_CHAIN; AUDIT_RESULTS.md C1–C6

[github-actions]

Testing Review

Looking at this diff, I need to identify what's actually testable code vs. documentation/planning files, then apply the checklist.

Observation: The diff is almost entirely documentation files (AUDIT_RESULTS.md, PLAN_*.md, CLAUDE.md, chain-system-backlog.md, .cursor/rules/*.mdc, tutorial-quest-system.md). The diff truncates before showing any production Java source or test Java source. However, AUDIT_RESULTS.md explicitly documents 13 testing gaps (T1–T13) that were identified against the production code that was already committed — meaning those production files exist but no corresponding tests were added as part of this change.

I'll report on what the diff itself reveals, including the self-documented gaps.

---

CONCERN: Availability window subsystem (T1–T7) has zero test coverage despite being new non-trivial pure-Java logic.
WHY: AUDIT_RESULTS.md explicitly lists AvailabilityConfig.isCurrentlyAvailable(), AvailabilityWindowDefinition.isActive() (year-wrapping logic), WindowBoundary.Fixed/Recurring.toZonedDateTime(), TimeGateCondition.evaluate(), TimeGateChainConditionType.parse(), WindowClosePolicy.fromString(), and both boundary parse paths as untested. These are pure-Java methods with non-trivial branching (year-wrapping, timezone conversion, 3-path validation, enum normalization). The diff adds PLAN_05_TESTING.md acknowledging the gap but adds no tests.
WHERE: Production: AvailabilityConfig, AvailabilityWindowDefinition, WindowBoundary, TimeGateCondition, TimeGateChainConditionType, WindowClosePolicy, FixedWindowBoundaryType, RecurringWindowBoundaryType / Test files: none added.

---

CONCERN: QuestPool.filterByAvailability() is a new public method with non-trivial logic and has no test covering the pass or fail branch.
WHY: AUDIT_RESULTS.md T8 notes QuestPoolTest was modified but no test for availability filtering was added. This method has at minimum two meaningful branches (template with closed window filtered out, template with open window kept) and a third (no availability config = kept). None are covered.
WHERE: Production: QuestPool / Test: QuestPoolTest

---

CONCERN: QuestChainManager expire handlers (handleExpireRetry, handleExpireSkip, handleExpireRestartChain) — approximately 150 lines of branching logic — have no test coverage.
WHY: AUDIT_RESULTS.md T9 documents this. handleExpireRetry alone has at least two branches (retry counter incremented vs. max retries exceeded falling through to fail). handleExpireRestartChain fires a lifecycle event and clears the completion log. Each handler is a public-facing behavioral path reachable via QuestExpireEvent. Zero assertions exist for any of these paths.
WHERE: Production: QuestChainManager / Test: QuestChainManagerTest

---

CONCERN: AbilityRegistry.softDisableAbility() and reEnableAbility() are new public API methods with event firing and multi-map bookkeeping, and have no tests.
WHY: AUDIT_RESULTS.md T10 documents this. softDisableAbility() removes from the abilities map and fires AbilityUnregisterEvent; reEnableAbility() adds back. Both branches of the enable/disable cycle are untested. getSoftDisabledAbilities() has no assertion confirming correct set membership.
WHERE: Production: AbilityRegistry / Test: none added

---

CONCERN: ContentExpansionManager.getRegisteredExpansions() and getContentPacks() are new public introspection methods with no corresponding tests.
WHY: AUDIT_RESULTS.md T11 documents this. These methods are the foundation of the new /mcrpg admin content command suite. If they return incorrect data, every admin command that depends on them silently misbehaves. No test verifies the return values against a known set of registered expansions/packs.
WHERE: Production: ContentExpansionManager / Test: none added

---

CONCERN: QuestManager.warnStaleDefinitions() has no test covering its branching logic.
WHY: AUDIT_RESULTS.md T12 documents this. The method iterates active quests and performs definition map lookups with at least three branches: no active quests, active quest with valid definition, active quest with missing definition. The missing-definition branch is the one that would silently fail in production without a test to catch regressions.
WHERE: Production: QuestManager / Test: existing QuestManager test, no new cases added

---

CONCERN: Tests in QuestChainCompletionLogDAOTest, QuestCompletionLogDAOTest, QuestInstanceDAOTest, and QuestChainManagerTest call Instant.now() directly instead of using the bootstrapped TimeProvider spy.
WHY: AUDIT_RESULTS.md T13 documents this as a convention violation. The checklist requires that all time-based logic go through TimeProvider so tests can inject a fixed clock. Even if these specific tests do not assert on the timestamp values today, using wall-clock time means any future assertion added to these tests will be flaky by design, and the pattern teaches the wrong convention to future contributors. McRPG.getInstance().getTimeProvider() with when(timeProvider.now()).thenReturn(...) is the required approach.
WHERE: QuestChainCompletionLogDAOTest, QuestCompletionLogDAOTest, QuestInstanceDAOTest, QuestChainManagerTest

---

CONCERN: QuestChainStartCondition is a new public interface — @ApiStatus.Experimental — shipped with no built-in implementations and no test verifying the contract a conforming implementation must satisfy.
WHY: The checklist requires that every new public method with non-trivial logic have a corresponding test. The interface itself defines a contract (evaluate(player, Instant) returning boolean). Without at least one test that exercises a conforming implementation against both true and false return cases, the interface's behavioral contract is unverified. The backlog doc defers all built-in conditions but the interface ships now.
WHERE: Production: QuestChainStartCondition / Test: none

---

CONCERN: ChainAutoStartTrigger implementations (FirstJoinAutoStartTrigger, LoginAutoStartTrigger, ManualAutoStartTrigger) — all new public classes — have no test coverage for their trigger evaluation logic.
WHY: These are non-trivial in the aggregate: FirstJoinAutoStartTrigger must only fire when chain state doesn't exist for a player; LoginAutoStartTrigger must fire on every login; ManualAutoStartTrigger must never auto-evaluate. None of these behaviors are verified by a test. At minimum the shouldActivate()-equivalent logic in each trigger needs both pass and fail branch coverage per the checklist.
WHERE: Production: FirstJoinAutoStartTrigger, LoginAutoStartTrigger, ManualAutoStartTrigger / Test: none

---

CONCERN: CascadeOrchestrator — described as a non-trivial Phase 3 collaborator handling same-tick recursive cascade batching, message deferral, and batch summary delivery — has no tests listed anywhere in the diff.
WHY: The checklist requires a test for every new public method with non-trivial logic (>3 lines). CascadeOrchestrator and CascadeContext implement batching logic with deferred message suppression and configurable summary delivery. This is exactly the class of logic that is hardest to debug in production and most valuable to unit-test. PLAN_05_TESTING.md does not mention it.
WHERE: Production: CascadeOrchestrator, CascadeContext / Test: none

---

CONCERN: TutorialPreQuestStartListener three-gate evaluation logic (config toggle → bypass permission → DisableTutorialSetting) has no test covering each gate independently.
WHY: This listener has at least six distinct paths: each of the three gates independently cancelling the event, all three gates passing (event not cancelled), and combinations. The checklist requires both pass and fail branch coverage for shouldActivate()-equivalent logic. This listener is the primary opt-out mechanism for the tutorial and a regression here would either force tutorials on all players or disable them globally.
WHERE: Production: TutorialPreQuestStartListener / Test: none listed in diff

---

CONCERN: The seven chain admin commands (ChainResetCommand, ChainRestartCommand, ChainAdvanceCommand, ChainStatusCommand, ChainSkipCommand) each have documented fail cases with distinct branching behavior and none have tests.
WHY: tutorial-quest-system.md documents 7+ distinct fail cases per command (chain key not in registry, player offline, no chain state, terminal state, last step, all steps already complete). These are non-trivial branches in public command handlers. A test for each command's happy path and primary fail case is the minimum. The diff adds zero command tests.
WHERE: Production: ChainResetCommand, ChainRestartCommand, ChainAdvanceCommand, ChainStatusCommand, ChainSkipCommand / Test: none

---

Production files changed: CLAUDE.md, .cursor/rules/core.mdc, .cursor/rules/persona-error-handling.mdc, .cursor/rules/persona-performance.mdc, docs/hld/tutorial/tutorial-quest-system.md, docs/hld/tutorial/chain-system-backlog.md (new), AUDIT_RESULTS.md (new), PLAN_01_CONCURRENCY.md through PLAN_06_GUI_UX.md (new), plus the production Java files implied by AUDIT_RESULTS.md T1–T13 (not visible in truncated diff but confirmed to exist by the audit findings referencing them) | Test files present: None added in this diff; AUDIT_RESULTS.md states 16 test files exist against 63 production files, with 23 new production files and only 1 new test file | Coverage gaps: T1–T7 availability window subsystem (7 untested classes), T8 QuestPool.filterByAvailability(), T9 three expire handlers in QuestChainManager, T10 AbilityRegistry soft-disable cycle, T11 ContentExpansionManager introspection, T12 QuestManager.warnStaleDefinitions(), T13 Instant.now() convention violations in 4 test files, plus QuestChainStartCondition contract, all three ChainAutoStartTrigger implementations, CascadeOrchestrator/CascadeContext, TutorialPreQuestStartListener gate logic, and all five chain admin commands — approximately 20+ distinct coverage gaps across newly shipped production code.

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively documentation files (.md, .mdc), project convention rules, and planning/audit artifacts. No Java source code is changed. There are no calls to MiniMessage.deserialize(), performCommand(), dispatchCommand(), SQL query construction, or permission checks introduced in this diff. All security-relevant Java implementation is referenced only in prose descriptions within the planning documents, and those documents are out of scope for this review — the actual code changes are not present in the diff.

[github-actions]

GUI/UX Review

Looking at this diff, the primary GUI-relevant changes are the new quest chain GUI files and slots referenced in AUDIT_RESULTS.md, PLAN_06_GUI_UX.md, and the updated tutorial-quest-system.md. The diff does not include the actual Java source for the new GUI classes — only documentation, planning files, and audit results. However, the audit results themselves (AUDIT_RESULTS.md G1–G4) and the plan (PLAN_06_GUI_UX.md) contain enough concrete detail to apply the checklist against what is described. I will treat the documented findings as the ground truth for what the code does, and flag anything the audit missed or mischaracterized, plus any new concerns introduced by the documentation itself.

---

CONCERN: GatedChainStepSlot.onClick() sends a raw String with unrendered MiniMessage markup instead of a rendered Component.
WHY: Players see literal tag text like <gold>This step is locked</gold> in chat rather than formatted output. This is a hard player-facing rendering defect.
WHERE: GatedChainStepSlot.java line 69 (documented in AUDIT_RESULTS.md G1 / PLAN_06_GUI_UX.md G1)

---

CONCERN: PLAN_06_GUI_UX.md G1 proposes fixing the rendering bug by calling localizationManager.getLocalizedMessageAsComponent() directly, but the checklist requires that MiniMessage must never be called directly — not even via McRPGMethods.getMiniMessage(). The proposed fix must route through the localization manager's rendering pipeline, which it does, but the plan does not explicitly prohibit a direct MiniMessage.miniMessage().deserialize() call as an alternative fix path. A developer reading the plan could implement it either way.
WHY: If the fix is implemented with a direct MiniMessage call rather than the localization manager, the rendering bug is fixed but the architectural rule is violated, creating a second defect in the same line.
WHERE: PLAN_06_GUI_UX.md — G1 fix description

---

CONCERN: GatedChainStepSlot is documented as never instantiated by any GUI class — it is dead code.
WHY: A slot that is never placed in a GUI cannot be reviewed for correctness, cannot be tested through normal play, and its localization keys cannot be verified as reachable. The slot's onClick() MiniMessage bug (G1) is only discoverable because the code exists — if the slot were wired in tomorrow without re-review, the bug ships to players.
WHERE: GatedChainStepSlot.java (documented in AUDIT_RESULTS.md G3 / PLAN_06_GUI_UX.md G3)

---

CONCERN: The previewItem branch in GatedChainStepSlot.getItem() returns a cloned ItemStack whose display name and lore bypass palette resolution and localization entirely.
WHY: Items sourced from step.previewItem() will display raw hex codes, legacy § codes, or unresolved MiniMessage tags depending on how the ItemStack was originally constructed, producing visual inconsistency with every other slot in the GUI that correctly routes through the localization/palette pipeline.
WHERE: GatedChainStepSlot.getItem() lines 97–98 (documented in AUDIT_RESULTS.md G4 / PLAN_06_GUI_UX.md G4)

---

CONCERN: CHAIN_PREVIEW_REWARDS_HIDDEN is registered as a localization key and has a corresponding en_quest.yml entry but is never referenced in Java code. The parallel CHAIN_PREVIEW_OBJECTIVES_HIDDEN key presumably is referenced; the rewards key is not.
WHY: Dead localization keys mislead translators into translating strings that are never shown. If a server owner customizes this key, they will never see their customization. The key will also cause confusion during future maintenance when a developer searches for its usage and finds none.
WHERE: LocalizationKey enum / en_quest.yml (documented in AUDIT_RESULTS.md G2 / PLAN_06_GUI_UX.md G2)

---

CONCERN: The QuestChainHistoryDetailGui and its slot classes (QuestChainHistorySlot, GatedChainStepSlot) are listed in CLAUDE.md under gui/quest/chain/slot/ but neither the audit results nor the plan document verifies whether these classes extend McRPGPaginatedGui (if paginated) or implement McRPGSlot, or whether GuiManager.trackPlayerGui() is called before paintInventory() and openInventory().
WHY: If QuestChainHistoryDetailGui extends raw PaginatedGui instead of McRPGPaginatedGui, it bypasses McRPG-specific filler painting and palette application. If GuiManager.trackPlayerGui() is not called first, CoreGuiOpenEvent does not fire for this GUI, meaning the mcrpg:gui_open quest objective cannot be satisfied by opening the chain history GUI — silently breaking quest progression for any quest that targets that GUI key.
WHERE: QuestChainHistoryDetailGui.java / gui/quest/chain/slot/ (not audited in G1–G4)

---

CONCERN: The tutorial disables itself via DisableTutorialConfirmGui and its slots (DisableTutorialConfirmSlot, DisableTutorialCancelSlot, DisableTutorialInfoSlot), but neither the audit results nor the plan document verifies whether clicking the confirm slot produces both a visual confirmation (chat or action bar) AND a sound effect, or whether it explains why the tutorial cannot be re-enabled without an admin reset.
WHY: A player who clicks "disable tutorial" with no feedback may click again believing the first click did not register. Without an explanation that re-enabling requires admin intervention, the player has no recovery path if they clicked accidentally.
WHERE: DisableTutorialConfirmSlot.java / DisableTutorialConfirmGui.java (not audited in G1–G4)

---

CONCERN: PLAN_06_GUI_UX.md G3 offers two resolutions for the dead GatedChainStepSlot: wire it into QuestChainHistoryDetailGui, or leave it with a Javadoc note. The "leave it" option does not remove the dead localization key (CHAIN_PREVIEW_REWARDS_HIDDEN) or the unreachable onClick() MiniMessage bug — both defects persist even with a Javadoc note.
WHY: Choosing "leave as-is with a note" resolves G3 on paper while leaving G1 and G2 as live defects in unreachable code that will eventually be wired in. The plan should require that if the slot is left unwired, G1 and G4 are still fixed within the slot's implementation so it is correct when it is eventually used.
WHERE: PLAN_06_GUI_UX.md — G3 fix description

---

CONCERN: The McRPGPreviousGuiSlot presence is not audited for any of the new chain GUIs (QuestChainHistoryDetailGui, DisableTutorialConfirmGui). These are non-home GUIs reachable through navigation from other GUIs, and the checklist requires McRPGPreviousGuiSlot in every non-home GUI.
WHY: Without a back slot, players navigating to the chain history detail or the tutorial disable confirmation screen have no in-GUI escape path and must close the inventory entirely, losing their navigation context and requiring them to re-open the parent GUI from scratch.
WHERE: QuestChainHistoryDetailGui.java / DisableTutorialConfirmGui.java (not audited in G1–G4)

---

CONCERN: The DisableTutorialSettingSlot in the settings GUI is described as toggling a setting, but neither the audit results nor the plan verifies whether the slot uses distinct materials or names for enabled vs. disabled state, and whether clicking it produces both a visual confirmation and a sound effect.
WHY: A PlayerSettingSlot override that uses the same material and name for both states makes it impossible for the player to tell whether the tutorial is currently enabled or disabled. Absence of audio/visual feedback on toggle leaves the player uncertain whether their click registered.
WHERE: DisableTutorialSettingSlot.java (not audited in G1–G4)

[github-actions]

Server Owner Review

Looking at this diff, I need to focus on what a server owner actually cares about: YAML config files, plugin.yml, migration safety, and reload behavior. The bulk of this diff is documentation (.md files, .mdc cursor rules) and Java source — but I can extract the server-owner-relevant signals from the HLD docs, the implied config changes, and what the diff describes about YAML schemas and permissions.

Let me work through the checklist systematically.

---

Server Owner Review

---

CONCERN: config.yml gains a tutorial.enabled toggle but the diff contains no config-version bump and no migration path.
WHY: The HLD (tutorial-quest-system.md) explicitly states "No config-version bump needed" but the original HLD draft said "Requires config-version bump (current 2 → 3)". The decision to skip the bump means existing servers will have no tutorial section in their config.yml after update. If the plugin reads tutorial.enabled without a default fallback, tutorial chains may silently fail to auto-start or silently auto-start for every player on every server — whichever way the missing-key branch resolves. Server owners will have no idea a new key exists.
WHERE: src/main/resources/config.yml → tutorial.enabled

---

CONCERN: The <tutorial> palette entry is described as being added to config.yml but no YAML comment is shown explaining what it controls, valid hex values, or what breaks if removed.
WHY: Server owners customizing palette colors frequently break MiniMessage rendering by using invalid hex strings or deleting entries. Without an inline comment like # Hex color for tutorial quest names. Must be a valid hex color: #RRGGBB, an owner editing the palette section has no guidance.
WHERE: src/main/resources/config.yml → palette section → tutorial entry

---

CONCERN: Seven new YAML quest definition files are introduced under src/main/resources/quests/tutorial/ plus a chain.yml, but there is no evidence in the diff that any of these shipped files contain # comments explaining their keys.
WHY: The chain YAML schema introduces keys that do not exist anywhere else in the plugin: auto-start.trigger, on-quest-expire, max-retries, on-start-messages, repeat-mode. A server owner copying the tutorial chain as a template for their own event chain will have no inline documentation. Wrong values (e.g., on-quest-expire: fail_chain instead of fail-chain, or a misspelled trigger key) silently fall back to defaults with no warning visible to the server owner at startup.
WHERE: src/main/resources/quests/tutorial/chain.yml and all sibling quest YAMLs

---

CONCERN: WindowClosePolicy.fromString() is documented in the backlog audit (E6) as silently swallowing unrecognized values and returning Optional.empty() — but the chain.yml schema uses on-window-close: expire-active as a string value. If a server owner typos this (e.g., expire_active), the policy silently becomes undefined with no console warning.
WHY: The server owner sees no error at startup and their seasonal event chain never expires active instances during a window close — players are stuck in a chain state indefinitely.
WHERE: src/main/resources/quests/*/chain.yml → availability.on-window-close

---

CONCERN: The new permission hierarchy introduces mcrpg.quest.chain.* and six child nodes, but the diff does not show the corresponding plugin.yml entries. If these are absent from plugin.yml, LuckPerms and similar plugins cannot enumerate them, tab-complete them, or resolve the wildcard mcrpg.quest.chain.*.
WHY: An admin granting mcrpg.quest.chain.* in LuckPerms will see it listed as an undefined permission and the wildcard may not propagate to the children nodes on all permission plugins, silently leaving staff unable to use chain admin commands.
WHERE: plugin.yml → permissions: block

---

CONCERN: The old permission documented in the HLD for tutorial admin was mcrpg.admin.tutorial.reset (default: op). The new permission set replaces this with mcrpg.quest.chain.reset. Any server that granted mcrpg.admin.tutorial.reset to a staff role in LuckPerms before this update will silently lose that grant after update.
WHY: Staff members who had tutorial reset permission will no longer be able to reset tutorial chains for players without the admin re-granting the new permission nodes. This is a silent permission revocation — no log, no warning, no migration note visible to the server owner.
WHERE: plugin.yml → permissions rename; affects any prior LuckPerms/PermissionsEx grants

---

CONCERN: The chain YAML repeat-mode field accepts ONCE, UNLIMITED, COOLDOWN, LIMITED, COOLDOWN_LIMITED, but only ONCE is functionally implemented. If a server owner sets repeat-mode: cooldown today (reasonable given the YAML schema is shipped), they will get ONCE behavior silently — their seasonal event chain will never repeat.
WHY: No YAML comment warns that non-ONCE values are parsed but ignored in this version. A server owner building a holiday event chain on a test server will appear to work fine, deploy to production, and then get player reports that the chain never restarted. There is no console warning when a non-ONCE mode is parsed.
WHERE: src/main/resources/quests/*/chain.yml → repeat-mode

---

CONCERN: Similarly, on-quest-expire step field accepts retry, fail-chain, restart-chain, skip — but only fail-chain is functional. Same silent-misbehavior risk as repeat-mode.
WHY: Server owner sets on-quest-expire: retry for a timed challenge step. Players fail the quest and the chain immediately terminates with FAILED instead of retrying. No warning logged.
WHERE: src/main/resources/quests/*/chain.yml → steps.<step>.on-quest-expire

---

CONCERN: The DB migration audit finding E1 documents that QuestChainCompletionLogDAO and QuestChainStateDAO bump setTableVersion outside the try-catch for ALTER TABLE. If ALTER TABLE fails, the version is bumped and the column is never added on retry.
WHY: A server running an older DB schema version upgrades, the ALTER TABLE fails (e.g., column already exists from a partial prior migration, or DB permissions issue), the version is marked as migrated, and on every subsequent server start the column is still missing. Chain completion counts or last_completed_at will silently read as null/zero for all players — potentially causing repeat-mode cooldown logic to always allow restarts, or breaking completion log writes.
WHERE: src/main/java/us/eunoians/mcrpg/database/table/quest/chain/QuestChainCompletionLogDAO.java and QuestChainStateDAO.java — attemptUpdateTable() method

---

CONCERN: Two entirely new SQL tables are created (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log). There is no mention of what happens if table creation fails on a fresh install and the server is restarted — specifically, whether UpdateTableFunction is used and whether version-0 → version-1 creation is idempotent.
WHY: If table creation is not wrapped in IF NOT EXISTS and UpdateTableFunction, a fresh server that fails partway through setup (e.g., disk full) will fail to create the table on the next start but may not attempt creation again if the version row was written before the failure.
WHERE: QuestChainStateDAO.attemptUpdateTable() and QuestChainCompletionLogDAO.attemptUpdateTable()

---

CONCERN: The on-start-messages YAML key introduced on quest definitions is a new top-level key with no equivalent in existing quest definitions. There is no config-version mechanism on individual quest YAML files, so existing hand-authored quest YAMLs from server owners won't break — but server owners who look at the new tutorial quest files and try to add on-start-messages to their own board quests need documentation that this is valid. Without a comment in the tutorial quest files explaining the key, they won't know it's universally available.
WHY: Server owners frequently use shipped quest files as templates. Unexplained keys in the template propagate as cargo-culted YAML that owners include without understanding.
WHERE: src/main/resources/quests/tutorial/first_steps.yml (and siblings) → on-start-messages:

---

CONCERN: The availability section in chain YAML (windows, timezone, on-window-close, grace-period) uses a non-obvious time format --12-01T00:00:00 for recurring yearly windows. There is no comment in the schema example (or presumably in the shipped YAML files) explaining that -- prefix means "any year" vs. a full ISO-8601 date for one-shot events.
WHY: A server owner setting up a Christmas event chain who writes 2026-12-01T00:00:00 thinking it will recur yearly will find their event permanently closed after December 2026. The distinction between one-shot and recurring is controlled entirely by the year presence, with no YAML comment to explain this.
WHERE: Any chain YAML using availability.windows.<name>.from / .until

---

CONCERN: The grace-period field accepts 48h duration strings. There is no comment explaining valid units (h, m, s, d?), minimum/maximum values, or what happens with 0 or negative values.
WHY: A server owner setting grace-period: 0 or grace-period: -1h may cause immediate expiration with no grace period, or a parse error that silently falls back to no grace period.
WHERE: Chain YAML → availability.grace-period

---

CONCERN: The audit document AUDIT_RESULTS.md and all six PLAN_0*.md files are committed to the repository root. These are internal engineering planning documents with no value to server owners or third-party developers reading the repo.
WHY: Server owners downloading or browsing the repository will see PLAN_01_CONCURRENCY.md through PLAN_06_GUI_UX.md and AUDIT_RESULTS.md in the root — creating confusion about what is documentation vs. implementation status. More importantly, AUDIT_RESULTS.md explicitly documents that G1 (MiniMessage rendering bug in GatedChainStepSlot) and C1-C6 (thread safety bugs) are known unfixed — server owners who find this file will reasonably question whether the plugin is safe to run.
WHERE: /AUDIT_RESULTS.md, /PLAN_01_CONCURRENCY.md through /PLAN_06_GUI_UX.md

---

Summary

Migration required: YES

• Two new DB tables added (must verify UpdateTableFunction is used and creation is idempotent)
• DB migration version-bump-on-failure bug (E1) means schema may silently never complete on partial failure
• Permission node renamed (mcrpg.admin.tutorial.reset → mcrpg.quest.chain.reset) — silently revokes existing LuckPerms grants
• config.yml gains tutorial.enabled and <tutorial> palette key with no config-version bump and no automated migration

Reload-safe: PARTIAL

• Chain definitions appear to support reload (re-resolution logic described in HLD)
• config.yml tutorial toggle and palette entry reload safety is not confirmed — no ReloadableBoolean / ReloadableContent registration is visible for tutorial.enabled
• DB schema changes are not reload-relevant but the version-bump-on-failure bug survives across restarts

[github-actions]

Extensibility Review

Breaking change risk: MEDIUM — multiple public API signatures changed (Long→Instant, method renames, new mandatory parameters) without deprecation bridges, and several new chain events have nullable-player inconsistencies that will silently drop addon intercepts.

---

CONCERN: QuestChainRestartEvent and QuestChainStepRetryEvent require @NotNull Player, but the manager silently skips firing these events for offline players.
WHY: An addon listening for these events to track chain restarts or retries will never receive them when the player is offline. There is no way for the addon to distinguish "event not fired because player offline" from "event not fired because the code path wasn't reached." Other chain events (QuestChainFailEvent, QuestChainExpireEvent) accept @Nullable Player, so the contract is inconsistent — addons can't write a single consistent listener pattern across the chain event family.
WHERE: event/quest/chain/QuestChainRestartEvent.java, event/quest/chain/QuestChainStepRetryEvent.java, QuestChainManager.handleExpireRestartChain(), QuestChainManager.handleExpireRetry() (documented as X9/X10 in AUDIT_RESULTS.md)

---

CONCERN: getCompletedQuestKeys() renamed to getNonSkippedCompletedQuestKeys() with no @Deprecated alias.
WHY: Any addon calling getCompletedQuestKeys() on a QuestChainPlayerData or equivalent holder will get a compile-time break with no migration path. The rename also changes the semantic contract (previously "all completed quests", now explicitly "non-skipped only") — addons that need the old behavior cannot recover it without re-implementing internal logic.
WHERE: AUDIT_RESULTS.md X6; the renamed method on whatever class exposed getCompletedQuestKeys()

---

CONCERN: QuestChainStartCondition.evaluate() gained a mandatory Instant parameter with no default method or deprecation bridge.
WHY: Any third-party addon that implemented QuestChainStartCondition before this change (registering a custom gate condition via QuestChainStartConditionContentPack) now has a broken implementation that won't compile. Since the interface is marked @ApiStatus.Experimental, the break is semi-expected, but there is no Javadoc on the interface explaining that experimental status means signature instability — an addon author reading only the interface would not know to expect this.
WHERE: quest/chain/QuestChainStartCondition.java (AUDIT_RESULTS.md X1)

---

CONCERN: QuestChainPlayerState.getLastCompletedAt() changed from Optional<Long> to Optional<Instant> with no bridge overload.
WHY: Addons reading last-completed timestamps to implement cooldown checks or display logic will get a ClassCastException at runtime (if they stored the return type as Optional) or a compile error. The raw Long epoch-millis contract was observable; Instant is a different type. No getLastCompletedAtEpochMilli() bridge is provided.
WHERE: quest/chain/QuestChainPlayerState.java (AUDIT_RESULTS.md X2/X3)

---

CONCERN: QuestPool constructor gained a mandatory TimeProvider parameter with no deprecated zero-TimeProvider overload.
WHY: Third-party addons that construct QuestPool instances (e.g., for custom board implementations or test harnesses) will fail to compile. QuestPool is a public class used in an extensible board generation path — any addon subclassing or constructing it is broken.
WHERE: quest/board/generation/QuestPool.java (AUDIT_RESULTS.md X8)

---

CONCERN: softDisableAbility() fires AbilityUnregisterEvent — the same event as permanent removal — with no distinguishing flag.
WHY: Addons listening for AbilityUnregisterEvent to track permanently removed abilities (e.g., to clean up loadouts or player data) will receive spurious events for soft-disables. More critically, addons cannot distinguish a reversible disable from a permanent removal, so they cannot decide whether to take destructive cleanup actions. The planned fix (adding boolean isSoftDisable() to the event) is documented in PLAN_03 but not yet implemented.
WHERE: ability/AbilityRegistry.softDisableAbility(), AbilityUnregisterEvent (AUDIT_RESULTS.md X11)

---

CONCERN: ChainAutoStartTrigger is a new public extensible interface with no Javadoc explaining the threading contract for tryStartChain() calls.
WHY: Addon developers implementing a custom trigger (e.g., myplugin:region_enter) need to know: must they call QuestChainManager.tryStartChain() on the main thread? Can they call it from an async listener? The ChainAutoStartTrigger interface ships with no documented thread-safety contract. Given that the audit found AvailabilityWindowChecker has critical threading bugs (C1–C6) from the same uncertainty, addon authors will reproduce the same mistakes.
WHERE: quest/chain/trigger/ChainAutoStartTrigger.java

---

CONCERN: QuestChainStepAdvanceEvent and QuestChainStartEvent carry no information about whether the advance was triggered by a cascade (auto-complete) versus real player action.
WHY: Addons that want to give players feedback on advancement, or that want to suppress duplicate notifications during cascade batching, cannot determine if the event is part of a cascade without re-inspecting CascadeOrchestrator state — which is internal. The CascadeStartEvent and CascadeFinalizeEvent exist but an addon would need to maintain its own cascade-in-progress flag by correlating three event types. A single isCascade() boolean on QuestChainStepAdvanceEvent would make this a one-line check.
WHERE: event/quest/chain/QuestChainStepAdvanceEvent.java, event/quest/chain/QuestChainStartEvent.java

---

CONCERN: McRPGLocalizationManager.formatDisplayDate() parameter changed from long to Instant with no bridge overload.
WHY: Addons calling formatDisplayDate() to render timestamps in GUI lore or chat messages will get a compile error. This is a utility method on a manager — high call-site surface area for addon code. No @Deprecated long overload is provided.
WHERE: McRPGLocalizationManager.formatDisplayDate() (AUDIT_RESULTS.md X7)

---

CONCERN: QuestChainDefinition, QuestChainStep, and ChainAutoStartTrigger have no Javadoc on their public methods explaining what each registry slot retrieves and what thread is safe to call methods from.
WHY: The checklist requirement is that new RegistryKey/ManagerKey constants and new extensible interfaces carry Javadoc explaining what they retrieve and what operations are safe. An addon developer reading only the public API surface cannot determine: Is QuestChainRegistry keyed by NamespacedKey? Is lookup safe from async threads? Is QuestChainManager.tryStartChain() idempotent? Without this, addons must read implementation code — the exact scenario the extensibility standard is designed to prevent.
WHERE: quest/chain/QuestChainDefinition.java, quest/chain/trigger/ChainAutoStartTrigger.java, McRPGManagerKey.QUEST_CHAIN, McRPGRegistryKey.QUEST_CHAIN

[github-actions]

Testing Review

Looking at this diff, I need to assess what production code changed versus what tests exist. The diff is primarily documentation, planning files, and cursor rules — but it references a large implementation (Phases 1–3) that is not shown in the diff itself. The AUDIT_RESULTS.md and PLAN_05_TESTING.md files within the diff explicitly document the testing gaps. I will apply the checklist to what is visible and what is described.

---

CONCERN: AvailabilityConfig.isCurrentlyAvailable() has zero test coverage
WHY: AUDIT_RESULTS.md T1 and PLAN_05_TESTING.md both acknowledge this. This is the central decision method for the entire availability window subsystem — a non-trivial method with branching logic over window collections. No corresponding test file is present or referenced as existing.
WHERE: Production: AvailabilityConfig / Test: none

---

CONCERN: AvailabilityWindowDefinition.isActive() has zero test coverage
WHY: AUDIT_RESULTS.md T2 calls out year-wrapping logic specifically. Year-boundary arithmetic (December–January spanning) is exactly the class of edge case the checklist requires: empty collections, boundary equality, wrap-around. No test exists.
WHERE: Production: AvailabilityWindowDefinition / Test: none

---

CONCERN: WindowBoundary.Fixed and WindowBoundary.Recurring toZonedDateTime() have zero test coverage
WHY: AUDIT_RESULTS.md T3. These are the foundational conversion methods the entire window system depends on. Recurring's yearOffset arithmetic and multi-timezone behavior are non-trivial (>3 lines) with no test.
WHERE: Production: WindowBoundary / Test: none

---

CONCERN: TimeGateCondition.evaluate() has zero test coverage
WHY: AUDIT_RESULTS.md T4. Timezone conversion edge cases — particularly when the configured timezone differs from UTC — are a known source of off-by-one errors at boundaries. No test covers the pass branch, fail branch, boundary equality, or timezone difference scenario.
WHERE: Production: TimeGateCondition / Test: none

---

CONCERN: TimeGateChainConditionType.parse() has zero test coverage
WHY: AUDIT_RESULTS.md T5 documents three validation paths: missing after field, invalid date format, invalid timezone. Each is a distinct fail branch of a public method with non-trivial logic. None are tested.
WHERE: Production: TimeGateChainConditionType / Test: none

---

CONCERN: WindowClosePolicy.fromString() has zero test coverage
WHY: AUDIT_RESULTS.md T6. This method normalizes input (kebab-case → enum) and returns Optional.empty() on failure. The normalization path and the unknown-value path are both untested. The error-handling audit (E6) also notes it silently swallows failures.
WHERE: Production: WindowClosePolicy / Test: none

---

CONCERN: FixedWindowBoundaryType.parse() and RecurringWindowBoundaryType.parse() have zero test coverage
WHY: AUDIT_RESULTS.md T7. Config deserialization methods with multiple validation paths (missing fields → Optional.empty(), invalid format → Optional.empty(), happy path) are non-trivial public methods with no corresponding tests.
WHERE: Production: FixedWindowBoundaryType, RecurringWindowBoundaryType / Test: none

---

CONCERN: QuestPool.filterByAvailability() is untested despite QuestPoolTest being modified
WHY: AUDIT_RESULTS.md T8. The test file was touched but no new test covers availability filtering. The checklist requires both the pass branch (template with open window kept) and the fail branch (template with closed window filtered out).
WHERE: Production: QuestPool / Test: QuestPoolTest

---

CONCERN: QuestChainManager expire handlers (handleExpireRetry, handleExpireSkip, handleExpireRestartChain) have zero test coverage
WHY: AUDIT_RESULTS.md T9. These are ~150 lines of branching logic across three public-facing behaviors. handleExpireRetry alone has a retry-counter-increment path and a max-retries-exceeded fallthrough. No tests cover any branch.
WHERE: Production: QuestChainManager / Test: QuestChainManagerTest

---

CONCERN: AbilityRegistry.softDisableAbility() and reEnableAbility() are untested
WHY: AUDIT_RESULTS.md T10. These are new public API methods (>3 lines each) with event firing and multi-map bookkeeping. Neither the pass branch nor the fail branch of the state change is covered. getSoftDisabledAbilities() is also untested.
WHERE: Production: AbilityRegistry / Test: none identified

---

CONCERN: ContentExpansionManager.getRegisteredExpansions() and getContentPacks() are untested
WHY: AUDIT_RESULTS.md T11. These are new public API methods added to support the introspection commands. No test verifies the returned collections are correct.
WHERE: Production: ContentExpansionManager / Test: none

---

CONCERN: QuestManager.warnStaleDefinitions() is untested
WHY: AUDIT_RESULTS.md T12. Iteration logic over active quests with definition-map lookups, including the warning-log branch when a definition is missing, constitutes non-trivial branching logic with no test.
WHERE: Production: QuestManager / Test: none identified

---

CONCERN: Five test classes call Instant.now() directly rather than using the spy'd TimeProvider
WHY: The TimeProvider checklist item requires that tests injecting time-dependent behavior use McRPG.getInstance().getTimeProvider() with when(timeProvider.now()).thenReturn(...). Direct Instant.now() calls make test behavior depend on wall-clock time and cannot be made deterministic. AUDIT_RESULTS.md T13 flags this as a convention violation.
WHERE: QuestChainCompletionLogDAOTest, QuestCompletionLogDAOTest, QuestInstanceDAOTest, QuestChainManagerTest

---

CONCERN: The entire availability window subsystem (7 new classes with non-trivial logic) contains zero tests, yet PLAN_05_TESTING.md is a planning document — no actual test files were added in this diff
WHY: The diff adds PLAN_05_TESTING.md describing what tests should be written, but adds only documentation files and cursor rules — zero new test files. The checklist asks: "Does the diff add non-Bukkit logic with zero corresponding test additions?" The availability window subsystem is pure-Java (no Bukkit dependency needed per PLAN_05_TESTING.md itself), making the absence of tests a clear structural gap rather than a dependency problem.
WHERE: Production: AvailabilityConfig, AvailabilityWindowDefinition, WindowBoundary, TimeGateCondition, TimeGateChainConditionType, WindowClosePolicy, FixedWindowBoundaryType, RecurringWindowBoundaryType / Test: none

---

CONCERN: No regression test for the DB migration version-bump-on-failure bug (E1)
WHY: AUDIT_RESULTS.md identifies E1 as a high-severity bug: setTableVersion executes even when ALTER TABLE fails, so the column is never added on retry. The checklist requires a regression test when a bug is fixed. PLAN_02_ERROR_HANDLING.md describes the fix but adds no test verifying that setTableVersion is only called after a successful executeUpdate().
WHERE: Production: QuestChainCompletionLogDAO, QuestChainStateDAO / Test: none

---

CONCERN: CascadeOrchestrator and CascadeContext (Phase 3) have no referenced tests
WHY: These are new classes with non-trivial logic (cascade batching, message deferral, batch summary delivery). The file change summary lists them as implemented but no corresponding test files appear in the diff or are described in PLAN_05_TESTING.md's checklist items.
WHERE: Production: CascadeOrchestrator, CascadeContext / Test: none identified

---

CONCERN: ChainPersistenceService and ChainQuestStarter (named collaborators in CLAUDE.md) have no referenced tests
WHY: Both are described as non-trivial collaborators owned by QuestChainManager — ChainPersistenceService serializes async DB writes using a CompletableFuture chain, ChainQuestStarter resolves quest definitions and delegates to QuestManager. Neither appears in any test file referenced in the diff.
WHERE: Production: ChainPersistenceService, ChainQuestStarter / Test: none identified

---

Production files changed: CLAUDE.md, .cursor/rules/core.mdc, .cursor/rules/persona-error-handling.mdc, .cursor/rules/persona-performance.mdc, docs/hld/tutorial/tutorial-quest-system.md, and implicitly all Phase 1–3 production classes described therein (63 production files per AUDIT_RESULTS.md)
Test files present: 16 existing test files (per AUDIT_RESULTS.md) — 1 new test file added across all three phases
Coverage gaps: Entire availability window subsystem (T1–T7, 7 pure-Java classes, zero tests); QuestPool.filterByAvailability() (T8); expire handlers in QuestChainManager (T9); AbilityRegistry soft-disable API (T10); ContentExpansionManager introspection (T11); QuestManager.warnStaleDefinitions() (T12); Instant.now() direct usage in 4–5 test files (T13); no regression test for E1 DB migration bug; CascadeOrchestrator/CascadeContext untested; ChainPersistenceService/ChainQuestStarter untested

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively documentation files (.md, .mdc), project convention rules, and planning documents. No Java source code, SQL queries, GUI handlers, command processors, or data-access objects are present in this changeset. Every checklist item requires executable code paths to evaluate — there are no player input flows, deserialize() calls, performCommand() invocations, permission checks, or DAO methods introduced here.

[github-actions]

GUI/UX Review

Looking at this diff, I need to apply the GUI/UX checklist to the actual GUI-relevant changes. The diff is primarily documentation (HLD, LLD, CLAUDE.md, cursor rules) with references to GUI files. Let me extract every GUI-relevant claim and flag problems.

---

CONCERN: DisableTutorialConfirmGui is listed as a new file but there is no evidence it calls GuiManager.trackPlayerGui() before paintInventory() and openInventory(). The HLD mentions it as a standalone confirm GUI but the LLD diff contains no implementation — only a filename reference.
WHY: If trackPlayerGui() is skipped, CoreGuiOpenEvent never fires, the GUI is invisible to the quest objective system (mcrpg:gui_open), and the player's open is not tracked for navigation state. The tutorial's own GUI-open objective would fail to credit the player for opening this screen.
WHERE: gui/tutorial/DisableTutorialConfirmGui.java (Phase 3 new files list, tutorial-quest-system.md §File Changes Phase 3)

---

CONCERN: QuestChainHistoryDetailGui is listed as a new GUI class but it is described as a "sub-view" opened by clicking a chain entry in QuestHistoryGui. There is no mention of a direct command to open it, and the HLD explicitly states navigation goes: QuestHistoryGui → click chain entry → QuestChainHistoryDetailGui. No command-driven path is documented.
WHY: The checklist requires every GUI to be reachable via a direct command, not only by clicking through another GUI. Without a command, server owners cannot link to this view directly, and the command-driven navigation contract (back-slot emits a command rather than calling open directly) cannot be satisfied for the chain detail view.
WHERE: gui/quest/chain/QuestChainHistoryDetailGui.java (CLAUDE.md directory tree; tutorial-quest-system.md §Quest History GUI chain grouping)

---

CONCERN: DisableTutorialConfirmGui contains three slots (DisableTutorialConfirmSlot, DisableTutorialInfoSlot, DisableTutorialCancelSlot). The HLD describes this as a confirmation screen for a destructive, irreversible action (permanently abandons the tutorial chain). There is no documentation that McRPGPreviousGuiSlot is present in this GUI, nor any mention of how the player navigates back if they decide not to confirm.
WHY: Every non-home GUI must contain McRPGPreviousGuiSlot. A confirmation dialog without a back slot leaves the player with no navigation escape other than closing the inventory entirely, breaking the navigation contract and potentially leaving state inconsistent.
WHERE: gui/tutorial/DisableTutorialConfirmGui.java; gui/tutorial/slot/DisableTutorialCancelSlot.java (Phase 3 new files list)

---

CONCERN: DisableTutorialSettingSlot is a PlayerSettingSlot override. The HLD states toggling "Disable Tutorial" opens a DisableTutorialConfirmGui rather than toggling directly, but the slot class name and its placement in gui/setting/slot/ follow the PlayerSettingSlot pattern. There is no documentation that this slot uses distinct materials or names for enabled vs. disabled state. The HLD only says it opens a confirm GUI — once confirmed and the setting is disabled, what does the slot look like?
WHY: The checklist requires PlayerSettingSlot overrides to use distinct materials or names for enabled vs. disabled state. If the slot always shows the same appearance regardless of whether the tutorial is disabled, the player cannot tell at a glance whether they have already opted out.
WHERE: gui/setting/slot/DisableTutorialSettingSlot.java (Phase 3 new files list)

---

CONCERN: ActiveQuestSlot is listed as modified to show per-quest display-item via locale route and a non-abandonable lore line. The HLD states non-abandonable quests show <body>Tutorial quests cannot be abandoned. as a lore line. This string appears to be hardcoded inline in the HLD description rather than routed through a localization key.
WHY: Every player-facing string must resolve from the localization system. If the non-abandonable lore line is hardcoded in the slot class rather than keyed in en_gui.yml or en_quest.yml, it cannot be translated or overridden by server owners, and it violates the no-hardcoded-strings rule.
WHERE: gui/quest/slot/ActiveQuestSlot.java (Phase 3 modified files list); tutorial-quest-system.md §5 TutorialQuestSource

---

CONCERN: QuestChainHistoryDetailGui opens a sub-view of quest history for a chain run. The HLD describes "for repeatable chains (future), multiple chain entries appear, each expandable." There is no documentation of how the detail GUI handles the case where the chain has zero step completions in the log — the empty/null state for a chain that was started but never advanced.
WHY: Empty/null state must be handled gracefully. If the GUI renders with zero slots populated and no feedback, the player sees a blank inventory with no explanation.
WHERE: gui/quest/chain/QuestChainHistoryDetailGui.java; CLAUDE.md directory tree gui/quest/chain/slot/

---

CONCERN: The mcrpg:gui_open objective type tracks GUI opens via CoreGuiOpenEvent fired from GuiManager.trackPlayerGui(). The HLD states "back-button navigation counts (this is intentional for tutorial purposes)." This means clicking the back button in any GUI fires the event and can satisfy a mcrpg:gui_open objective for the destination GUI. However, the checklist requires that clicking a back/previous-GUI slot emits the command for the previous GUI rather than calling its open method directly. If back-slot navigation is implemented via direct open() calls rather than commands, trackPlayerGui() is still called (so the event fires), but the command-driven navigation contract is broken for server owners who want to intercept navigation.
WHY: Command-driven back navigation lets server owners override routing. If back slots call open methods directly, the navigation cannot be intercepted or redirected, and server owners lose that control point. This is a structural navigation concern independent of whether the event fires.
WHERE: All GUI classes with McRPGPreviousGuiSlot (22 retrofitted GUI classes listed in Phase 1 modified files); tutorial-quest-system.md §6 CoreGuiOpenEvent description

---

CONCERN: QuestChainHistorySlot and the slots under gui/quest/chain/slot/ are new slot classes. The HLD and LLD contain no mention of these slots implementing McRPGSlot. They are listed only as filenames.
WHY: Every slot class must implement McRPGSlot. Without it, slots bypass the McRPG slot contract (click handling, safety, tracking). Given the slots are only described by filename in the diff with no class-level detail, this cannot be confirmed as satisfied.
WHERE: gui/quest/slot/QuestChainHistorySlot.java; gui/quest/chain/slot/ (CLAUDE.md directory tree)

---

CONCERN: The DisableTutorialConfirmGui is described as using a confirmation pattern for a permanently irreversible action (chain abandonment), but the HLD contains no mention of a sound effect accompanying the confirmation or denial interaction. The checklist requires that when a slot click produces a toggled or consequential state change, the player receives BOTH a visual confirmation AND a sound effect.
WHY: Silently completing an irreversible action (disabling the tutorial, permanently abandoning the chain) with only a GUI close and no sound gives no tactile feedback that something permanent just happened. Players may click accidentally and not realize the consequence.
WHERE: gui/tutorial/slot/DisableTutorialConfirmSlot.java (Phase 3 new files list)

---

CONCERN: The HLD describes TutorialQuestSource providing a <tutorial> palette placeholder for quest name color in the Active Quests GUI. The localization note says <tutorial> maps to <color:#E8C97A>. However, the checklist requires that all text rendering be delegated to the localization manager and that MiniMessage must never be called directly — not even via McRPGMethods.getMiniMessage(). The inline hex <color:#E8C97A> embedded in a palette placeholder is a MiniMessage tag that could be expanded directly at the callsite rather than going through the localization manager.
WHY: If ActiveQuestSlot or TutorialQuestSource's display-item builder expands <tutorial> by calling getMiniMessage().deserialize(...) directly rather than routing through McRPGLocalizationManager, it violates the MiniMessage delegation rule and bypasses any server-side palette override hooks.
WHERE: gui/quest/slot/ActiveQuestSlot.java; tutorial-quest-system.md §5 new palette entry table

[github-actions]

Server Owner Review

Server Owner Review

Overview

This is a large diff covering documentation, internal coding rules, and implementation of a quest chain system with tutorial content. From a server owner perspective, I'm focusing on the YAML configs, plugin.yml, config file routing, and database migration safety.

---

CONCERN: New config.yml key tutorial.enabled has no confirmed config-version bump in this PR
WHY: The HLD explicitly states "No config-version bump needed — no migration logic required for this addition." But adding a new required key without a version bump means server owners upgrading from existing installs will silently have no tutorial: section in their config. If the plugin reads tutorial.enabled with no default fallback and the key is absent, the tutorial may fail to enable or throw a null read — the server owner sees no warning and has no idea the key is missing.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § "DisableTutorialSetting" / config.yml tutorial.enabled

---

CONCERN: No config-version increment for palette.tutorial addition to config.yml
WHY: A new palette entry <tutorial> is added alongside tutorial.enabled. Both are new config keys. The HLD justifies skipping the version bump by saying "no migration logic required," but the version exists precisely so server owners and automated tooling know the file has diverged from its defaults. Without a bump, there is no signal to a server owner (or their admin tooling) that their existing config.yml is missing these keys. If the plugin soft-defaults missing palette entries to empty string, tutorial quest names render as blank or broken color tags in chat and GUIs.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § Phase 3, config.yml palette.tutorial

---

CONCERN: The tutorial.enabled config key has no documented comment explaining valid values, default, or what breaks if omitted
WHY: Server owners navigate configs by reading comments. The HLD shows the YAML snippet as just tutorial: enabled: true with no # comment block. If a server owner sets this to false expecting it to disable all tutorial quests but chain states already exist in the DB for players, the behavior (does the chain get abandoned? does it freeze?) is undocumented at the config level. A comment like # Set to false to prevent new players from receiving the tutorial chain. Does not affect players already in progress. is required.
WHERE: src/main/resources/config.yml (not shown in diff, but the key is specified as added in Phase 3)

---

CONCERN: Two new SQL tables (mcrpg_quest_chain_state, mcrpg_quest_chain_completion_log) — no diff showing the UpdateTableFunction registration
WHY: The HLD and backlog reference these tables and state that UpdateTableFunction is the required mechanism. The diff contains no DatabaseManager or UpdateTableFunction implementation showing these tables are actually registered in the migration pipeline. If they are absent, the plugin will throw SQL errors on first startup after update because the tables don't exist. Server owners will see a wall of stack traces and broken quest functionality with no explanation.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § "Chain state persistence" SQL block; modified files list states DatabaseManager — chain state + completion log tables but no corresponding code diff is present

---

CONCERN: New permission nodes use inconsistent naming — mcrpg.quest.chain.* deviates from established mcrpg.<category>.<action> pattern for some nodes
WHY: The HLD defines nodes like mcrpg.quest.chain.restart, mcrpg.quest.chain.reset, mcrpg.quest.chain.advance, mcrpg.quest.chain.skip, mcrpg.quest.chain.status. These look reasonable, but the existing pattern in the project uses mcrpg.admin.* for admin-only operations (see mcrpg.admin.* wildcard also defined in this same PR). Chain admin commands are admin-only but are placed under mcrpg.quest.chain rather than mcrpg.admin.quest.chain. If a server owner grants mcrpg.admin.* expecting to cover all admin actions, they will NOT get the chain commands — silently. The wildcard hierarchy as specified does not have mcrpg.quest.chain.* as a child of mcrpg.admin.*.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § Permissions table; plugin.yml (not shown in diff)

---

CONCERN: mcrpg.tutorial.bypass permission is not listed as a child of mcrpg.admin.* in the permission hierarchy
WHY: The permission table defines mcrpg.admin.* as "All admin commands" but mcrpg.tutorial.bypass is a separate node with no parent declared. An admin who has mcrpg.admin.* via LuckPerms will NOT automatically have mcrpg.tutorial.bypass because it is not declared as a child. Staff accounts that should skip the tutorial will unexpectedly receive it after update.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § Permissions table

---

CONCERN: No plugin.yml diff shown — cannot verify description: fields, default: values, or children: hierarchy for any of the 9+ new permission nodes
WHY: The checklist requires every permission in plugin.yml to have a description: field, explicit default: values, and the children: block must reflect the wildcard hierarchy described in the HLD. Without seeing the actual plugin.yml changes, there is no way to confirm these are correct. Missing description: fields leave server owners with no context when browsing permissions in LuckPerms. A missing children: block means mcrpg.* does not cascade to new nodes.
WHERE: plugin.yml (not present in diff)

---

CONCERN: New chain YAML files (chain.yml, 7 quest definition files) are not shown in the diff — cannot verify comment coverage, key naming, or default value safety
WHY: Eight new YAML files are listed as added (src/main/resources/quests/tutorial/chain.yml plus 7 quest definitions). None of these files appear in the diff. Server owners who open these files to tune reward amounts or disable specific quests need # comments on every key explaining valid values and what breaks if set wrong. Reward amounts (e.g., 1000 boosted XP, 2500 redeemable XP) need sample-output comments. Without seeing the files, these cannot be confirmed.
WHERE: src/main/resources/quests/tutorial/chain.yml and all 7 quest definition YAMLs

---

CONCERN: Repeat modes UNLIMITED, COOLDOWN, LIMITED, COOLDOWN_LIMITED are parsed and stored in the database but not functional — no config comment warns server owners
WHY: If a server owner reads the backlog doc and tries configuring repeat-mode: unlimited for a custom chain, the system will silently treat it as ONCE. The player completes the chain once and can never restart it, with no error logged and no warning in the YAML. The YAML schema must include a # NOTE: Only 'once' is currently enforced. Other modes are accepted but behave as 'once' until a future update. comment directly on the repeat-mode key.
WHERE: src/main/resources/quests/tutorial/chain.yml repeat-mode key (file not in diff); docs/hld/tutorial/tutorial-quest-system.md § Chain Repeat Mode

---

CONCERN: on-quest-expire field on chain steps has the same silent-no-op problem for retry, restart-chain, and skip values
WHY: Only fail-chain is functional in the initial release. The other values are parsed but treated as fail-chain. A server owner who configures on-quest-expire: retry expecting retries gets chain failure with no explanation. The key needs a comment in the YAML schema flagging which values are currently active.
WHERE: Chain step YAML on-quest-expire key (file not in diff)

---

CONCERN: DisableTutorialSetting toggle is described as "permanently abandons the chain" with a confirmation GUI, but no reload-safety documentation exists for this setting
WHY: Server owners using /mcrpg admin reload expect player settings to survive reloads. If DisableTutorialSetting is not wrapped in a ReloadableBoolean or equivalent, or if its backing config is reloaded in a way that clears in-memory state, a player's opt-out preference could be silently lost on reload, causing the tutorial to restart for opted-out players on their next login.
WHERE: setting/impl/DisableTutorialSetting.java (not in diff); related to QuestChainFirstJoinListener gate

---

CONCERN: "Player must be online" restriction on all chain admin commands is not documented in the YAML or in-game help — server owners cannot run these from console for offline players
WHY: The HLD explicitly states all chain commands require the player to be online (quest start/cancel requires main-thread interaction). However, server owners routinely run admin commands from console or scheduled tasks targeting offline players (e.g., resetting a player's tutorial before they rejoin). The command error message "Player must be online" will be confusing without upfront documentation that this is a hard system constraint, not a missing feature. At minimum, the command descriptions in plugin.yml must state this.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § Admin Commands fail cases; plugin.yml command descriptions (not in diff)

---

CONCERN: Chain state re-resolution on reload logs WARNING but takes no player-visible action — players with suspended chains receive no notification
WHY: The reload behavior doc states: if a chain definition is removed, the chain becomes "inert" and a WARNING is logged. The player's active quest instance continues running but the chain will never advance. From the player's perspective, they complete a quest and nothing happens — the chain silently stalls. There is no player-facing message, no admin alert beyond the server log, and no guidance in any config or upgrade note about what to do to resolve this.
WHERE: docs/hld/tutorial/tutorial-quest-system.md § "Chain Reload Behavior" — "Chain definition entirely removed" case

---

Summary

Migration required: YES — Two new SQL tables must be created. No UpdateTableFunction code is present in this diff to confirm this happens automatically. Server owners cannot safely update without verifying the tables are created on startup.

Reload-safe: PARTIAL — Chain definitions reload via /mcrpg admin reload with documented re-resolution logic. However: the tutorial.enabled config toggle reload behavior is unconfirmed; DisableTutorialSetting persistence across reload is unconfirmed; players with suspended chains (definition removed) receive no feedback and the chain stalls silently.

[github-actions]

Extensibility Review

Breaking change risk: MEDIUM — The diff introduces several new public API surfaces (events, interfaces, registry keys, builder-only construction paths) with gaps in nullability contracts, missing cancellable pre-events for chain lifecycle transitions, and one confirmed interface addition without a default implementation that will break existing addon compilations.

---

CONCERN: QuestObjectiveType interface gains a new checkAutoComplete(UUID playerUUID) method with no default implementation.
WHY: Any third-party plugin that implements QuestObjectiveType to register a custom objective type will fail to compile against the updated API. This is a hard binary-incompatible change — existing addons break at compile time, not just at runtime.
WHERE: phase-1-quest-engine-extensions.md Diagram 1 / Diagram 3; QuestObjectiveType interface, checkAutoComplete(playerUUID) OptionalLong

---

CONCERN: QuestStartEvent is modified to add getQuestSource() and getStarterUUID() fields, but the diff does not show a constructor or builder migration path for third-party code that constructs QuestStartEvent directly (e.g., in tests or mock scenarios).
WHY: If QuestStartEvent previously had a public constructor with a fixed parameter list, adding required parameters breaks any addon that instantiates it. There is no @Deprecated alias shown for the old constructor form.
WHERE: tutorial-quest-system.md §2; event/quest/QuestStartEvent.java (modified)

---

CONCERN: PreQuestStartEvent only fires "when the initiating player is online." Chain-triggered quest starts (where the chain manager starts the next step programmatically after the previous step completes) may have no online player check at that boundary — the HLD explicitly states offline/system-initiated starts skip the pre-event.
WHY: An addon listening to PreQuestStartEvent to gate or log all quest starts will silently miss chain-driven starts where the player logged off between steps. The addon cannot rely on the event as a universal hook, which the current Javadoc-level description ("general-purpose hook to gate quest starts") implies it is.
WHERE: tutorial-quest-system.md §2: "offline or system-initiated starts skip the pre-event"; QuestManager.startQuest()

---

CONCERN: QuestChainAbandonEvent and QuestChainFailEvent are documented as non-cancellable — but there is no corresponding cancellable pre-event (PreQuestChainAbandonEvent, PreQuestChainFailEvent) before the state transition occurs.
WHY: An addon that wants to intercept an abandonment (e.g., to show a custom confirmation, apply a penalty, or prevent it under certain conditions) has no hook point. By the time the event fires, the state has already been written. This violates the established McRPG pattern of firing a cancellable event before applying effects.
WHERE: chain-system-backlog.md §5 event table; event/quest/chain/QuestChainAbandonEvent.java, QuestChainFailEvent.java

---

CONCERN: QuestChainStartEvent and QuestChainStepAdvanceEvent are listed as lifecycle events but the diff does not confirm they are cancellable. If they are non-cancellable (notification-only), addons cannot prevent a chain from starting or a step from advancing — including use cases like "block chain start if another chain is active" or "prevent step advance until a cooldown passes."
WHY: Without a cancellable PreQuestChainStartEvent, third-party chain gating logic must resort to listening for PreQuestStartEvent on the first step quest and inferring chain context — fragile and under-documented.
WHERE: tutorial-quest-system.md "Extension infrastructure" bullet; event/quest/chain/QuestChainStartEvent.java, QuestChainStepAdvanceEvent.java

---

CONCERN: ChainAutoStartTrigger is documented as an extensible interface, but the diff does not show @NotNull/@Nullable annotations on getKey() or the callback method that calls QuestChainManager.tryStartChain(). It also does not show whether tryStartChain() itself is a public API method with a documented contract.
WHY: An addon implementing ChainAutoStartTrigger to register a custom trigger (e.g., myplugin:region_enter) cannot safely implement the interface without knowing the nullability contract on getKey() or whether tryStartChain() is safe to call from an async listener.
WHERE: tutorial-quest-system.md "Auto-start triggers" section; quest/chain/trigger/ChainAutoStartTrigger.java

---

CONCERN: QuestChainStartCondition is marked @ApiStatus.Experimental and "not yet wired into QuestChainManager." It ships as a public interface but has no built-in implementations and no content pack initially. Addons implementing it will register their conditions into a registry that is never consulted.
WHY: Third-party developers reading the interface will implement it expecting it to function, then spend significant debugging time discovering it is silently ignored. This is a documented non-functional extension point with no runtime effect.
WHERE: tutorial-quest-system.md §1: "QuestChainStartCondition — @ApiStatus.Experimental interface for gating chain start. Not yet wired into QuestChainManager"

---

CONCERN: QuestDefinition moves to a builder-only construction path (QuestDefinition.Builder) with the class constructor made private. The diff does not show @Deprecated aliases on the old public constructors during the transition period — it states "deprecated constructors may remain temporarily" but the implementation description says "previous constructors removed."
WHY: Any addon constructing QuestDefinition directly (e.g., to register programmatic quest definitions via QuestChainContentPack or QuestContentPack) will break at compile time with no deprecation warning period. The transition language is contradictory and the safe path (keep deprecated constructors) may not have been followed.
WHERE: tutorial-quest-system.md §3 "Schema:" bullet: "QuestDefinition.Builder is the only construction path (previous constructors removed)"; quest/QuestDefinition.java

---

CONCERN: LoadoutAbilityChangeEvent replaces the previous event model (LoadoutAbilityEquipEvent per the HLD design), but the old event name appears in the original HLD and review findings as the planned API. The diff ships LoadoutAbilityChangeEvent instead, with no @Deprecated type alias for the previously designed LoadoutAbilityEquipEvent.
WHY: Any addon that was built against the HLD/design documents (or an intermediate snapshot) expecting LoadoutAbilityEquipEvent will find it does not exist. Since the HLD was publicly referenced during design, this is a soft breaking change for early adopters.
WHERE: tutorial-quest-system.md "New / updated events (Phase 1)": LoadoutAbilityChangeEvent; original design §6: LoadoutAbilityEquipEvent

---

CONCERN: McRPGManagerKey.QUEST_CHAIN is a new registry key constant but the diff provides no Javadoc on what the retrieved manager exposes, what operations are thread-safe vs. main-thread-only, or what the lifecycle of the manager is (e.g., whether it is valid before onEnable completes).
WHY: An addon calling McRPGManagerKey.QUEST_CHAIN to retrieve QuestChainManager has no documented contract on when the manager is available, which methods can be called from async context, or what tryStartChain(player, chainKey) guarantees when the chain definition is absent.
WHERE: McRPGManagerKey.java — QUEST_CHAIN constant; tutorial-quest-system.md §1

---

CONCERN: CoreGuiOpenEvent is placed in McCore rather than McRPG. The diff explicitly notes this as an intentional design — McCore fires the event from GuiManager.trackPlayerGui(). This places McRPG-domain logic (quest objective progress for GUI opens) in a McCore-level event with no McRPG context (no McRPGPlayer, no quest state).
WHY: An addon listening to CoreGuiOpenEvent for quest-related purposes must independently resolve McRPGPlayer from the UUID. More critically, this is flagged in the checklist as "McRPG-specific logic being placed in McCore" — GuiOpenObjectiveType is an McRPG concept driving McCore API design, which creates an upward dependency.
WHERE: tutorial-quest-system.md §6 "McCore CoreGuiOpenEvent"; McCore GuiManager.trackPlayerGui()

---

CONCERN: The on-start-messages system is implemented as a dedicated OnStartMessage concept separate from QuestRewardType, but MessageRewardType (a completion reward) uses the same QuestMessageDeliverer for delivery. The two concepts are not documented as related extension points — an addon author implementing a custom reward type that sends messages has no documented guidance on whether to use QuestMessageDeliverer directly or implement their own delivery.
WHY: QuestMessageDeliverer appears to be a public collaborator (based on it being in the file changes list and referenced from two separate listeners), but its API surface, nullability contracts, and whether it is safe to inject into third-party reward types is completely undocumented in this diff.
WHERE: tutorial-quest-system.md §3 and §4; quest/message/QuestMessageDeliverer.java

---

CONCERN: QuestChainPlayerData maintains a reverse index (questKey → chainKey) for O(1) chain lookup but the diff does not document whether this index is exposed as a public API or is entirely internal. If it is public, addons can query it; if it is internal, addons querying "which chain owns this quest?" must recompute by iterating all chain definitions.
WHY: A common addon use case is "when quest X completes, do something chain-aware." Without a public lookup method, the addon must scan QuestChainRegistry — O(n) over all chains — on every QuestCompleteEvent.
WHERE: CLAUDE.md terminology table: QuestChainPlayerData — "Maintains a reverse index (questKey → chainKey) for O(1) chain lookup"

[github-actions]

Testing Review

Looking at this diff, I need to identify what production code was actually changed versus documentation/config files. The diff is heavily truncated, but I can work with what's visible.

The diff contains:

• Documentation files (.cursor/rules/, CLAUDE.md, docs/)
• No src/main/java/ or src/test/java/ files are visible in the diff

However, the documentation extensively describes production code that was implemented across Phases 1, 2, and 3. The diff references numerous new production classes but contains zero corresponding test file changes. Let me flag the concerns based on what's described as "implemented."

---

CONCERN: The diff documents extensive production code across three implementation phases (Phase 1, 2, 3) as "Implemented" but contains zero test file additions or modifications.
WHY: The HLD and LLD enumerate dozens of new public classes with non-trivial logic — QuestChainManager, QuestChainProgressListener, QuestChainFirstJoinListener, QuestChainLoginListener, CascadeOrchestrator, ChainPersistenceService, ChainQuestStarter, QuestStartAutoCompleteListener, QuestMessageDeliverer, TutorialPreQuestStartListener, all seven objective types, all four reward types, QuestChainConfigLoader, all chain admin commands — none of which have corresponding test file changes in this diff. The checklist requires a test for every new public method with non-trivial logic (>3 lines).
WHERE: All src/main/java/ production files implied by tutorial-quest-system.md and phase-1-quest-engine-extensions.md / phase-2-quest-chain-system.md / phase-3-tutorial-content.md; src/test/java/ (absent)

---

CONCERN: QuestChainManager — the central manager for start/advance/force-advance/restart/reset — has no visible tests covering its core state machine transitions.
WHY: QuestChainManager is described as executing start, advance, force-advance, restart, and reset operations, each producing distinct QuestChainState outcomes (ACTIVE, COMPLETED, ABANDONED, FAILED, EXPIRED). Every branch of shouldActivate()-equivalent logic in a manager of this complexity requires both pass and fail coverage. No test class for this manager appears in the diff.
WHERE: quest/chain/QuestChainManager.java (production, described as implemented); src/test/java/ (absent)

---

CONCERN: Seven new objective types (SkillLevelUpObjectiveType, SkillTargetLevelObjectiveType, GuiOpenObjectiveType, AbilityUnlockObjectiveType, AbilityActivateObjectiveType, LoadoutEquipObjectiveType, QuestBoardAcceptObjectiveType) have no test coverage in this diff.
WHY: Each type has non-trivial parseConfig(), canProcess(), processProgress(), and (for state-based types) checkAutoComplete() logic. Both pass and fail branches of checkAutoComplete() must be tested — returning OptionalLong.empty() versus a populated value produces entirely different quest-start outcomes. There are no corresponding *Test.java files in this diff.
WHERE: quest/objective/type/builtin/Skill*ObjectiveType.java, GuiOpenObjectiveType.java, Ability*ObjectiveType.java, LoadoutEquipObjectiveType.java, QuestBoardAcceptObjectiveType.java; src/test/java/ (absent)

---

CONCERN: Four new reward types (MessageRewardType, BoostedExperienceRewardType, RedeemableExperienceRewardType, RedeemableLevelsRewardType) have no test coverage in this diff.
WHY: Each type has parseConfig(), grant(), serializeConfig(), and fromSerializedConfig() logic. The config-driven amount field on the three XP/level types must be tested at 0 and at a maximum value per the checklist. MessageRewardType.grant() has branching locale-key vs. inline-message logic. No test files appear.
WHERE: quest/reward/builtin/MessageRewardType.java, BoostedExperienceRewardType.java, RedeemableExperienceRewardType.java, RedeemableLevelsRewardType.java; src/test/java/ (absent)

---

CONCERN: CascadeOrchestrator — described as handling same-tick recursive cascade batching with message deferral and batch summary delivery — has no visible tests.
WHY: Cascade batching is the most complex control flow in Phase 3. The decision to suppress vs. deliver on-start messages and to fire CascadeStartEvent / CascadeFinalizeEvent is logic-intensive. Edge cases include a cascade that auto-completes all 7 steps, a cascade that stops mid-way, and a cascade on the final step. None of these are covered by visible tests.
WHERE: quest/chain/CascadeOrchestrator.java; src/test/java/ (absent)

---

CONCERN: QuestChainProgressListener, QuestChainFirstJoinListener, and QuestChainLoginListener have no visible tests for their shouldActivate() equivalent branches.
WHY: Each listener has conditional logic: QuestChainProgressListener must not advance chains for quests not belonging to a chain, and must correctly handle the final-step completion vs. mid-chain advancement. QuestChainFirstJoinListener has a bypass-permission branch and a "chain state already exists" branch. QuestChainLoginListener has re-resolution logic for stale current_quest values. Both pass and fail branches of each condition require tests.
WHERE: listener/quest/QuestChainProgressListener.java, QuestChainFirstJoinListener.java, QuestChainLoginListener.java; src/test/java/ (absent)

---

CONCERN: QuestStartAutoCompleteListener has no tests covering the starter-scoped auto-complete path.
WHY: The listener must only auto-complete for the quest's starterUUID, not for other players on a shared-scope quest. The pass branch (starter, state-based objective already satisfied) and fail branch (non-starter player, or event-based objective) are both non-trivial and untested.
WHERE: listener/quest/QuestStartAutoCompleteListener.java; src/test/java/ (absent)

---

CONCERN: QuestChainConfigLoader has no tests for its YAML validation paths, including the case where a chain references a non-existent quest definition.
WHY: The HLD states that QuestChainConfigLoader validates all quest references at load time and logs SEVERE for missing references. The checklist requires testing config-driven code paths with invalid input (missing quest key), zero steps, and a valid fully-formed definition. The on-quest-expire parsing also has branching (valid behavior string vs. unknown string). None of this is covered.
WHERE: quest/chain/QuestChainConfigLoader.java; src/test/java/ (absent)

---

CONCERN: TutorialPreQuestStartListener has no tests covering its three-gate cancellation logic.
WHY: The listener checks (1) tutorial.enabled config toggle, (2) mcrpg.tutorial.bypass permission, (3) DisableTutorialSetting. Each gate should independently cancel the PreQuestStartEvent. Tests are required for: all three gates active (cancel), only the config toggle off (cancel), only the permission present (cancel), only the setting disabled (cancel), and none active (no cancel). No test file is present.
WHERE: listener/quest/TutorialPreQuestStartListener.java; src/test/java/ (absent)

---

CONCERN: Chain admin commands (ChainResetCommand, ChainRestartCommand, ChainAdvanceCommand, ChainStatusCommand, ChainSkipCommand) have no tests covering their fail cases.
WHY: The HLD documents explicit fail cases for each command (chain key not in registry, player offline, no chain state, terminal state, last step on advance, all steps already in completion log on restart). Each fail case must have a test that asserts the correct error message and confirms no state change occurs. No test files are present.
WHERE: command/admin/chain/Chain*.java; src/test/java/ (absent)

---

CONCERN: QuestChainStateDAO and QuestChainCompletionLogDAO have no tests verifying their SQL read/write paths.
WHY: The checklist requires DAO tests to mock JDBC Connection, PreparedStatement, and ResultSet via Mockito. Both DAOs have insert, update, and select paths. The select path for QuestChainStateDAO must handle NULL current_quest (terminal states) and missing rows (player has no state). None of these are tested.
WHERE: database/table/quest/QuestChainStateDAO.java, QuestChainCompletionLogDAO.java; src/test/java/ (absent)

---

CONCERN: QuestDefinition.Builder — described as the only construction path after removing previous constructors — has no tests for build() invariant validation.
WHY: build() must validate invariants (non-null quest key, non-empty phases, etc.) and throw IllegalArgumentException with a descriptive message. The error-handling persona rule requires build() to validate. No test exercises the builder with missing required fields or with a zero-length phase list.
WHERE: quest/definition/QuestDefinition.java (Builder inner class); src/test/java/ (absent)

---

CONCERN: LoadoutAbilityChangeEvent and LoadoutPositionSwapEvent — the newly introduced loadout events — have no tests confirming they fire on Loadout.equipAbility(), unequipAbility(), and swapAbility(), and do not fire on the now-private addAbility()/removeAbility()/replaceAbility().
WHY: The private/public boundary is a correctness guarantee: only the public API fires events. There are no tests asserting that the event fires exactly once per equip/unequip/swap call, and there are no tests for the edge case where a swap involves an already-empty slot.
WHERE: Loadout.java; src/test/java/ (absent)

---

CONCERN: AbilityObjectiveFilter has no tests covering its matches() logic for the three AbilityType branches (ACTIVE, PASSIVE, INNATE) and the optional specific-ability-key filter.
WHY: AbilityObjectiveFilter is shared by three objective types. Its pass/fail branches (type matches with no key filter, type matches with key filter, type mismatches, key mismatches) are the core of ability objective correctness. No tests are present.
WHERE: quest/objective/type/builtin/AbilityObjectiveFilter.java; src/test/java/ (absent)

---

CONCERN: QuestChainPlayerData reverse index (questKey → chainKey) has no tests verifying O(1) lookup correctness, including the edge case where a single quest key appears in multiple chains.
WHY: The reverse index is described as enabling O(1) chain lookup on quest completion. If the index is built incorrectly (e.g., only last writer wins when a quest key is shared), QuestChainProgressListener silently advances the wrong chain. No test covers index construction or lookup.
WHERE: quest/chain/QuestChainPlayerData.java; src/test/java/ (absent)

---

CONCERN: The LLD (Phase 1, section 7) explicitly promises "Tests (see Phase 1 LLD section 7)" but section 7 of the LLD is truncated in the diff and no corresponding test files appear anywhere in the diff.
WHY: The LLD's own test plan is cited as the coverage authority. If section 7 exists and describes test classes, those test files must be present in the diff. Their complete absence means either the tests were never written (coverage gap) or the diff is incomplete (cannot verify coverage). Either way this is a blocking concern.
WHERE: docs/lld/tutorial-quest-system/phase-1-quest-engine-extensions.md §7; src/test/java/ (absent)

---

Production files changed: CLAUDE.md, .cursor/rules/core.mdc, .cursor/rules/persona-error-handling.mdc, .cursor/rules/persona-performance.mdc, docs/hld/tutorial/tutorial-quest-system.md, docs/hld/tutorial/chain-system-backlog.md, docs/lld/tutorial-quest-system/phase-1-quest-engine-extensions.md (and implied production Java files described as implemented but not shown in the truncated diff)

Test files present: None

Coverage gaps: QuestChainManager state machine, all 7 objective types (both checkAutoComplete branches), all 4 reward types (parseConfig + grant + amount=0 edge case), CascadeOrchestrator cascade batching, QuestChainProgressListener / QuestChainFirstJoinListener / QuestChainLoginListener (pass+fail branches), QuestStartAutoCompleteListener (starter-scoped guard), QuestChainConfigLoader (invalid/missing quest reference), TutorialPreQuestStartListener (3-gate cancellation), all 5 chain admin commands (all documented fail cases), QuestChainStateDAO + QuestChainCompletionLogDAO (JDBC mocking), QuestDefinition.Builder.build() invariant validation, Loadout event-firing on equip/unequip/swap, AbilityObjectiveFilter type+key branching, QuestChainPlayerData reverse index construction and lookup

[github-actions]

Security Review

No security concerns found.

The diff is entirely documentation and developer-tooling content: Markdown files (.md), cursor rule files (.mdc), and CLAUDE.md. No Java source code, SQL, configuration parsing logic, user-input handling, MiniMessage deserialization calls, command dispatch, permission checks, or DAO queries are present in this diff. Every checklist item requires changed executable code as a precondition, and none exists here.