[Tests] Add unit tests for GetItemRequest, StatisticEntry, ReloadableContent, StartupProfile + fix thread-safety bug

[DiamondDagger590]

Summary

• GetItemRequestTest: Tests CompletableFuture state machine transitions — PENDING_RESPONSE, ITEM_FOUND, ITEM_NOT_FOUND, and ERRORED — including pre-completed futures, pre-failed futures, and cross-thread completion scenarios (0% → 100% line coverage)
• StatisticEntryTest: Tests all typed accessor methods (getAsInt, getAsLong, getAsDouble, getAsString, getAsTimestamp, getAsSetString), defensive copy behavior for set accessor, ClassCastException edge cases, and record equality (28.6% → 100% line coverage)
• ReloadableContentTest: Tests constructor auto-loading via callback, explicit content override, reloadContent() lifecycle, yaml document mutation detection, and accessor methods (0% → 100% line coverage)
• ReloadableContentManagerTest: Tests single and bulk content tracking, reloadAllContent() invocation, empty manager safety, callback invocation counting, and Set-based deduplication (0% → 100% line coverage)
• StartupProfileTest: Tests system property setting for PROD ("true") and TEST ("false"), override behavior, and enum completeness (0% → 100% line coverage)

Bug fix: Thread-safety in GetItemRequest

GetItemRequest is designed for cross-thread polling — the main Minecraft thread reads responseState/item while a database executor thread writes them via CompletableFuture callbacks. Neither field was volatile, which means:

1. Visibility bug: The polling thread could cache a stale PENDING_RESPONSE indefinitely since the JIT is free to hoist the field read into a register
2. Ordering bug: Without volatile, writes to item and responseState could be reordered by the CPU/compiler, letting a reader observe ITEM_FOUND while item is still null

Fix: Added volatile to both responseState and item fields. Three cross-thread tests (100 iterations each) verify visibility for the ITEM_FOUND, ITEM_NOT_FOUND, and ERRORED paths.

Test plan

• All new tests pass via ./gradlew test
• JaCoCo coverage report confirms 100% line coverage on all targeted classes
• No existing tests broken by these additions
• Cross-thread tests verify volatile visibility guarantees

https://claude.ai/code/session_011rsAGFzo1R3u5H6uSzPNLA

Summary by CodeRabbit

• Tests

  • Added comprehensive test coverage for startup profiles, reloadable content management, database request handling, and statistic entries.
  • Added new test utility extension for managed executor services.

• Chores

  • Updated code review configuration to enforce standardized test method naming conventions.
  • Enhanced thread-safety in request handling with volatile field declarations.

[coderabbitai]

Warning

Review limit reached

@DiamondDagger590, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 56 minutes and 58 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1e47a9e3-434f-416f-940b-7a9785eb9a65

📥 Commits

Reviewing files that changed from the base of the PR and between 98bfbc4 and 4df6d0c.

📒 Files selected for processing (4)

• .claude/commands/review-testing.md
• .cursor/rules/core.mdc
• .cursor/rules/persona-testing.mdc
• CLAUDE.md

Walkthrough

This PR adds JUnit 5 tests for five domain classes and supporting infrastructure. StartupProfile enum tests verify system property setting and standard enum methods. ReloadableContent tests cover YAML-backed single-item loading, mutation, and typed extraction. ReloadableContentManager tests verify bulk reload orchestration and deduplication. GetItemRequest tests validate CompletableFuture-driven state transitions with cross-thread visibility, supported by volatile field modifiers and a reusable test executor extension. StatisticEntry tests cover typed accessors, defensive copying for sets, and equality semantics. Repository test naming conventions are also documented.

Changes

Test Coverage for Domain Classes

Layer / File(s)	Summary
StartupProfile enum system property behavior src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java	Tests verify PROD and TEST set mccore.testMode to "true"/"false", subsequent calls override prior values, values() returns both elements in order, valueOf(String) maps correctly, and name() returns the correct constant name. Property is cleaned up after each test.
ReloadableContent single-item loading and mutation src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java	Tests verify constructor immediately extracts YAML values via callback, explicit content overrides YAML, reloadContent() refreshes stored values after YAML mutation, accessors return original instances, typed extraction works for String/Integer/Boolean/Double, and transform callbacks are applied during loading.
ReloadableContentManager bulk reload orchestration src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java	Tests verify reloadAllContent() reloads all tracked instances to updated YAML values, collection tracking works, empty tracking is safe, custom callbacks execute once per reload cycle, and set-based deduplication prevents duplicate reloads of the same instance.
GetItemRequest thread-safe state transitions src/main/java/com/diamonddagger590/mccore/database/response/GetItemRequest.java, src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java, src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java, .coderabbit.yaml	Production fields responseState and item are marked volatile for thread-visible updates from CompletableFuture callbacks. A reusable ManagedExecutorExtension fixture manages single-threaded executor lifecycle. Tests verify pending/found/not-found/errored state transitions, immediate initialization for pre-completed futures, optional item semantics, future identity preservation, and visible state after cross-thread completion with synchronized polls. Repository conventions document expected test naming patterns.
StatisticEntry typed accessors and defensive copying src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java	Tests verify typed getters (getAsInt, getAsLong, getAsDouble, getAsString, getAsTimestamp) return correct values, getAsSetString returns new unmodifiable set with defensive-copy isolation, mismatched type getters throw ClassCastException, record accessors work correctly, and equality/hashCode are consistent.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• DiamondDagger590/McCore#28: Adds additional test coverage for ReloadableContent common and collection subtypes, directly complementing the ReloadableContent and ReloadableContentManager test cases in this PR.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 1.79% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding comprehensive unit tests for five classes and fixing a thread-safety bug in GetItemRequest.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/zen-hamilton-6VwMM

---

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]

Extensibility Review

Breaking change risk: NONE — this diff adds only test classes with no changes to production source code.

The entire diff consists of new files added under src/test/java/. No production classes, interfaces, abstract classes, method signatures, registry constants, or public APIs are modified, added, or removed. Test code is not part of the compiled framework artifact distributed to downstream plugin developers and therefore carries zero contract impact.

No extensibility concerns found.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff.

---

Analysis

Production files changed

The diff contains only test file additions — no production source files are modified. This means I must evaluate whether the tests are structurally correct and whether any implied production code has coverage gaps.

Production files changed: None (test-only diff)
Test files present:

• StartupProfileTest.java
• ReloadableContentManagerTest.java
• ReloadableContentTest.java
• GetItemRequestTest.java
• StatisticEntryTest.java

---

Findings

---

CONCERN: StatisticEntryTest uses Instant.now() directly in a test that asserts on a time-dependent value.
WHY: The checklist requires all time-based logic to go through TimeProvider so tests can inject a fixed clock. Even though this is a test-side call, asserting assertEquals(now, entry.getAsTimestamp()) depends on the Instant captured at test execution time. If the production StatisticEntry stores or transforms the timestamp through any wall-clock logic, this will be fragile. More importantly, the rule is absolute: time-dependent behavior in tests must use an injected fixed clock, not Instant.now(). The test should construct a fixed Instant (e.g., Instant.ofEpochSecond(1_000_000)) instead.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java — getAsTimestamp_returnsValue_whenTypeIsTimestamp()

---

CONCERN: StatisticEntryTest constructs NamespacedKey using the deprecated two-arg string constructor (new NamespacedKey("mccore", "test_stat")).
WHY: This is not a style concern — using the deprecated constructor requires suppressing a warning with @SuppressWarnings("deprecation"), which is present. However, NamespacedKey is a Bukkit class. Constructing it via the deprecated constructor outside of a MockBukkit environment may silently pass today but break if Bukkit's validation logic changes. No MockBukkit.mock() / MockBukkit.unmock() lifecycle is present in this test, yet a Bukkit API class is being instantiated. This means the test is mixing a Bukkit-dependent class into a plain JUnit test without the required MockBukkit setup, making it fragile and architecturally inconsistent with the Bukkit-Dependent vs. Pure-Java Separation rule.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java — class field TEST_KEY

---

CONCERN: StatisticEntryTest has no MockBukkit setup/teardown but directly instantiates a Bukkit API type (NamespacedKey).
WHY: Per the checklist, if a class uses Bukkit API, MockBukkit must be set up with MockBukkit.mock() in @BeforeEach and torn down with MockBukkit.unmock() in @AfterEach. Conversely, if MockBukkit is not needed, the test should not touch Bukkit classes at all. Here, StatisticEntry takes a NamespacedKey as its key — if the intent is to test pure logic, the production StatisticEntry should accept a plain key abstraction and the Bukkit coupling should be isolated. As written, the test spins up no Bukkit environment but calls into a Bukkit class, which is a structural violation.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

---

CONCERN: ReloadableContentTest.java imports CorePlugin, RegistryAccess, RegistryKey, Manager, ManagerRegistry, and RegistryResetExtension but uses none of them in the test body.
WHY: Dead imports are not merely a style issue here — the presence of RegistryResetExtension import suggests the test was scaffolded with the intention of resetting global registry state but the extension was never applied (no @ExtendWith(RegistryResetExtension.class) annotation). If ReloadableContent or its dependencies write to a ManagerRegistry or similar singleton during construction or reload, test isolation is broken without the reset extension. This is a structural correctness problem: either the extension must be applied, or the dead imports must be removed and the test must prove no global state is touched.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java — imports block

---

CONCERN: ReloadableContentManagerTest.java passes null as the CorePlugin argument to ReloadableContentManager constructor with no test verifying null-safety of the constructor or that the plugin reference is never dereferenced during reloadAllContent.
WHY: If ReloadableContentManager stores the plugin reference and dereferences it during any reload path, every test in this file will throw a NullPointerException silently masking a real failure, or the tests pass now but become invalid if the production code changes to use the plugin. The checklist requires edge cases (null inputs) to be explicitly covered with assertions, not merely used as a convenience shortcut to avoid setting up a plugin instance. There is no test asserting that null plugin is safe, and no test using a real/mocked plugin instance to verify behavior under normal conditions.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java — setUp(), manager = new ReloadableContentManager(null)

---

CONCERN: ReloadableContentManagerTest — reloadAllContent_completesSuccessfully_whenNoContentTracked() has no assertion.
WHY: The checklist explicitly states every test method must have at least one assertion; a test with no assertion cannot fail. This test calls manager.reloadAllContent() and silently passes even if the method throws an unchecked exception (JUnit would catch that, but the intent — "completes without error" — is not verified by any assertDoesNotThrow wrapper). The correct form is assertDoesNotThrow(() -> manager.reloadAllContent()).
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java — reloadAllContent_completesSuccessfully_whenNoContentTracked()

---

CONCERN: StartupProfileTest — values_containsBothProfiles() hardcodes ordinal positions (values[0], values[1]) to assert enum identity.
WHY: This is a structural correctness problem: enum ordinal order is an implementation detail of the enum declaration, not a contract. If a new profile is inserted before PROD or between PROD and TEST, the test produces a false failure. The test should assert membership (e.g., assertThat(values).contains(StartupProfile.PROD, StartupProfile.TEST)) rather than positional equality. Additionally, asserting values.length == 2 means adding a legitimate new enum constant will break this test — the test is asserting the schema of the enum rather than the behavior under test, which is an over-constraint.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java — values_containsBothProfiles()

---

CONCERN: StartupProfileTest — name_returnsPROD_forProdProfile() and name_returnsTEST_forTestProfile() test Enum.name(), which is a JDK-guaranteed behavior with zero production logic.
WHY: The checklist limits required tests to "non-trivial logic (>3 lines)." Enum.name() is a final JDK method — it cannot be overridden and has no production implementation to break. These tests provide zero coverage value and inflate the test count without protecting against any real regression. Similarly, valueOf_returnsCorrectProfile() tests JDK enum reflection machinery. These are structural dead weight.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java — name_returnsPROD_forProdProfile(), name_returnsTEST_forTestProfile(), valueOf_returnsCorrectProfile()

---

CONCERN: GetItemRequestTest — no test covers the race condition where complete() is called from a different thread before getItemResponseState() is read.
WHY: GetItemRequest wraps a CompletableFuture whose completion state is observable across threads. All tests complete the future on the same thread that created it, which does not validate thread-visibility guarantees. If the state field in GetItemRequest is not volatile or otherwise safely published, a cross-thread completion could yield a stale PENDING_RESPONSE read. This is a coverage gap for the concurrency contract of the class.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java

---

CONCERN: ReloadableContentTest — getAsSetString_isDefensiveCopy_whenOriginalModified() asserts result.size() == 2 after adding "y" to the original set post-construction, but this test actually verifies a snapshot is taken at construction time yet the test name says "defensive copy." If ReloadableContent stores a live reference to the original set and only wraps it in Collections.unmodifiableSet at getAsSetString() call time, the result would also be size 2 (reflecting the mutation), making the test pass for the wrong reason — it cannot distinguish between "copy at construction" and "copy at getter call."
WHY: The test does not assert the size is 1 immediately after construction (before the mutation) and then 1 again after mutation, which is the only way to prove a true defensive copy at construction. As written, both a construction-time copy and a getter-time copy would pass this test, so it does not actually verify the defensive-copy invariant.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java — getAsSetString_isDefensiveCopy_whenOriginalModified()

---

CONCERN: No test file is placed in src/testFixtures/java/ despite RegistryResetExtension being imported in ReloadableContentTest, suggesting shared test infrastructure exists but is not being made available as a fixture.
WHY: The checklist requires shared fixtures to be placed in src/testFixtures/java/ so downstream repos (McRPG) can depend on them. The import of RegistryResetExtension from com.diamonddagger590.mccore.testing implies such a class exists in the McCore test infrastructure, but none of the new test files expose reusable fixtures (e.g., a standard YamlDocument builder, a ReloadableContentFixture) to testFixtures. If downstream consumers need to test against ReloadableContent or GetItemRequest behavior, they have no fixture support.
WHERE: src/testFixtures/java/ — absent; ReloadableContentTest.java import of RegistryResetExtension

---

Summary

Production files changed: None (test-only additions)

Test files present:

• StartupProfileTest.java
• ReloadableContentManagerTest.java
• ReloadableContentTest.java
• GetItemRequestTest.java
• StatisticEntryTest.java

Coverage gaps:

• No null-input safety test for ReloadableContentManager(null) as an explicit assertion (used as convenience, not verified safe)
• getAsSetString defensive-copy test cannot distinguish construction-time copy from getter-time copy
• No cross-thread completion test for GetItemRequest
• No test for ReloadableContent when the YAML route does not exist (missing key → null callback return)
• No test verifying ReloadableContentManager.trackReloadableContent(Collection) with an empty collection
• RegistryResetExtension imported but never applied in ReloadableContentTest, leaving potential global state leak untested

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively test code (src/test/java/...) — unit tests for StartupProfile, ReloadableContentManager, ReloadableContent, GetItemRequest, and StatisticEntry. No production DAO methods, DDL construction, MiniMessage deserialization, ChatResponse handling, onClick() implementations, or permission-gated operations are introduced or modified. None of the checklist items apply to test-only changes.

[coderabbitai]

Actionable comments posted: 8

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java`:
- Line 21: In StartupProfileTest, rename test method identifiers to the
repository convention givenContext_whenAction_thenOutcome—for example rename
setSystemProperty_setsTrue_whenProd to
givenProdProfile_whenSetSystemProperty_thenReturnsTrue; apply the same pattern
to the other test methods noted in the comment (the methods at lines mentioned)
so their names start with given..., contain _when..._, and end with _then..._;
update any references/usages to those method names (tests and any IDE run
configurations) but leave annotations like `@Test` intact.

In
`@src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java`:
- Line 33: Rename the test methods in ReloadableContentManagerTest to follow the
givenContext_whenAction_thenOutcome convention; specifically change
reloadAllContent_reloadsAll_whenContentTracked to a name like
givenTrackedContent_whenReloadAllContent_thenReloadsAll and similarly rename the
other test methods referenced at lines 54, 70, 76, and 96 to follow the
given..._when..._then... pattern so each method name describes the precondition,
the action, and the expected outcome.
- Around line 70-72: The test method
reloadAllContent_completesSuccessfully_whenNoContentTracked currently contains
no assertions; wrap the manager.reloadAllContent() invocation in an assertion
such as assertDoesNotThrow (or add an observable postcondition assertion) so the
test can fail on regressions — update
ReloadableContentManagerTest.reloadAllContent_completesSuccessfully_whenNoContentTracked
to call assertDoesNotThrow(() -> manager.reloadAllContent()) (or equivalent)
referencing the manager.reloadAllContent() call.

In
`@src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java`:
- Line 37: Rename the test methods in ReloadableContentTest to follow the
givenContext_whenAction_thenOutcome convention; for example, change
constructor_loadsContentImmediately_whenCallbackProvided() to
givenConstructor_whenCallbackProvided_thenLoadsContentImmediately and similarly
rename the other tests in the class (those currently named with the pattern like
constructor_* or other descriptive names) to start with "given..." followed by
"_when..." and end with "_then..." so they all conform to the repository naming
guideline; update only the method names (and any references inside annotations
like `@Test`) to preserve behavior.

In
`@src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java`:
- Around line 13-125: Add a unit test that constructs a GetItemRequest with a
null CompletableFuture (new GetItemRequest<>(null)) and asserts the class
handles this edge case according to project expectations: verify
request.getItemResponseState() is set to ERRORED (or PENDING_RESPONSE if policy
dictates) and request.getItem() is empty, and that getItemCompletableFuture()
returns null (or throws a documented NPE if that is the defined behavior). Place
the test alongside other constructor tests and reference the GetItemRequest
constructor, getItemResponseState(), getItem(), and getItemCompletableFuture()
methods so reviewers can locate the behavior under test.
- Around line 17-124: Rename the test methods in GetItemRequestTest to follow
the givenContext_whenAction_thenOutcome convention: change
constructor_setsPendingState_whenFutureNotComplete ->
givenFutureNotCompleted_whenConstructingRequest_thenPendingResponse;
state_becomesItemFound_whenFutureCompletesWithValue ->
givenFutureCompletesWithValue_whenCompleted_thenItemFound;
state_becomesItemNotFound_whenFutureCompletesWithNull ->
givenFutureCompletesWithNull_whenCompleted_thenItemNotFound;
state_becomesErrored_whenFutureCompletesExceptionally ->
givenFutureCompletesExceptionally_whenCompleted_thenErrored;
getItemCompletableFuture_returnsOriginalFuture ->
givenRequest_whenGetItemCompletableFuture_thenReturnsOriginalFuture;
constructor_setsItemFound_whenFutureAlreadyCompletedWithValue ->
givenFutureAlreadyCompletedWithValue_whenConstructingRequest_thenItemFound;
constructor_setsItemNotFound_whenFutureAlreadyCompletedWithNull ->
givenFutureAlreadyCompletedWithNull_whenConstructingRequest_thenItemNotFound;
constructor_setsErrored_whenFutureAlreadyFailed ->
givenFutureAlreadyFailed_whenConstructingRequest_thenErrored;
state_becomesItemFound_whenFutureCompletesWithComplexObject ->
givenFutureCompletesWithComplexObject_whenCompleted_thenItemFound;
getItem_returnsEmpty_beforeCompletion ->
givenFutureNotCompleted_whenGetItem_thenReturnsEmpty; update each `@DisplayName`
if present to match the new method name semantics and ensure any references to
these method names in test suites are updated.

In `@src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java`:
- Around line 23-126: Tests use names like getAsInt_returnsValue_whenTypeIsInt
that don't follow the repository's givenContext_whenAction_thenOutcome
convention; rename each test method to that pattern (e.g.,
getAsInt_returnsValue_whenTypeIsInt ->
givenTypeInt_whenGetAsInt_thenReturnsValue) for all affected methods such as
getAsLong_returnsValue_whenTypeIsLong,
getAsDouble_returnsValue_whenTypeIsDouble,
getAsString_returnsValue_whenTypeIsString,
getAsTimestamp_returnsValue_whenTypeIsTimestamp,
getAsSetString_returnsUnmodifiableCopy_whenTypeIsSetString,
getAsSetString_isDefensiveCopy_whenOriginalModified,
getAsInt_throwsClassCast_whenValueIsNotInteger,
getAsString_throwsClassCast_whenValueIsNotString,
getAsLong_throwsClassCast_whenValueIsNotLong,
getAsTimestamp_throwsClassCast_whenValueIsNotInstant,
recordAccessors_returnConstructorValues, equals_returnsTrue_whenSameValues;
follow the exact pattern given{Context}_when{Action}_then{Outcome} and update
any references/imports if the test names are used elsewhere.
- Around line 74-84: The test
getAsSetString_isDefensiveCopy_whenOriginalModified in StatisticEntryTest
currently mutates original before reading entry.getAsSetString(), which doesn't
prove defensiveness; change the sequence so you create original with "x",
construct StatisticEntry(TEST_KEY, StatisticType.SET_STRING, original), then
call entry.getAsSetString() and store it to result, then mutate original
(original.add("y")) and finally assert result remains unchanged (e.g.,
assertEquals(1, result.size()) or assertTrue(result.contains("x"))). This uses
the variables and methods StatisticEntry, entry.getAsSetString(), and original
to verify the defensive copy behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1e750c0a-e381-4ca6-800e-efec9f2d0847

📥 Commits

Reviewing files that changed from the base of the PR and between fcb6a13 and 7466470.

📒 Files selected for processing (5)

• src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java
• src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java
• src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java
• src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java
• src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename test methods to givenContext_whenAction_thenOutcome format.

Current test names don’t follow the repository’s required test naming pattern. Rename them to the expected given..._when..._then... style for consistency.

Suggested rename diff

-    void setSystemProperty_setsTrue_whenProd() {
+    void givenProdProfile_whenSetSystemProperty_thenSetsTrue() {
@@
-    void setSystemProperty_setsFalse_whenTest() {
+    void givenTestProfile_whenSetSystemProperty_thenSetsFalse() {
@@
-    void setSystemProperty_overridesPrevious_whenCalledAgain() {
+    void givenExistingTestProperty_whenProdSetsSystemProperty_thenOverridesToTrue() {
@@
-    void values_containsBothProfiles() {
+    void givenStartupProfileEnum_whenValuesCalled_thenContainsProdAndTest() {
@@
-    void valueOf_returnsCorrectProfile() {
+    void givenValidProfileNames_whenValueOfCalled_thenReturnsMatchingProfile() {
@@
-    void name_returnsPROD_forProdProfile() {
+    void givenProdProfile_whenNameCalled_thenReturnsProd() {
@@
-    void name_returnsTEST_forTestProfile() {
+    void givenTestProfile_whenNameCalled_thenReturnsTest() {

As per coding guidelines, "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."

Also applies to: 28-28, 35-35, 45-45, 54-54, 61-61, 67-67

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java`
at line 21, In StartupProfileTest, rename test method identifiers to the
repository convention givenContext_whenAction_thenOutcome—for example rename
setSystemProperty_setsTrue_whenProd to
givenProdProfile_whenSetSystemProperty_thenReturnsTrue; apply the same pattern
to the other test methods noted in the comment (the methods at lines mentioned)
so their names start with given..., contain _when..._, and end with _then..._;
update any references/usages to those method names (tests and any IDE run
configurations) but leave annotations like `@Test` intact.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename test methods to the required given..._when..._then... convention.

These names are understandable, but they do not match the mandated repository test naming pattern.

As per coding guidelines: "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."

Also applies to: 54-54, 70-70, 76-76, 96-96

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java`
at line 33, Rename the test methods in ReloadableContentManagerTest to follow
the givenContext_whenAction_thenOutcome convention; specifically change
reloadAllContent_reloadsAll_whenContentTracked to a name like
givenTrackedContent_whenReloadAllContent_thenReloadsAll and similarly rename the
other test methods referenced at lines 54, 70, 76, and 96 to follow the
given..._when..._then... pattern so each method name describes the precondition,
the action, and the expected outcome.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align test method names with given..._when..._then... format.

These method names are readable, but they do not follow the repository’s required test naming convention. Please rename them to givenContext_whenAction_thenOutcome for consistency.

As per coding guidelines: "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."

Also applies to: 46-46, 55-55, 66-66, 75-75, 84-84, 93-93, 102-102, 111-111, 125-125

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java`
at line 37, Rename the test methods in ReloadableContentTest to follow the
givenContext_whenAction_thenOutcome convention; for example, change
constructor_loadsContentImmediately_whenCallbackProvided() to
givenConstructor_whenCallbackProvided_thenLoadsContentImmediately and similarly
rename the other tests in the class (those currently named with the pattern like
constructor_* or other descriptive names) to start with "given..." followed by
"_when..." and end with "_then..." so they all conform to the repository naming
guideline; update only the method names (and any references inside annotations
like `@Test`) to preserve behavior.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename test methods to givenContext_whenAction_thenOutcome for guideline compliance.

The current method names are descriptive but do not follow the required naming convention for tests in this repository.

💡 Example rename pattern

- void constructor_setsPendingState_whenFutureNotComplete() {
+ void givenPendingFuture_whenConstructingRequest_thenStateIsPendingResponse() {

As per coding guidelines, "Follow the givenContext_whenAction_thenOutcome naming convention for test methods."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java`
around lines 17 - 124, Rename the test methods in GetItemRequestTest to follow
the givenContext_whenAction_thenOutcome convention: change
constructor_setsPendingState_whenFutureNotComplete ->
givenFutureNotCompleted_whenConstructingRequest_thenPendingResponse;
state_becomesItemFound_whenFutureCompletesWithValue ->
givenFutureCompletesWithValue_whenCompleted_thenItemFound;
state_becomesItemNotFound_whenFutureCompletesWithNull ->
givenFutureCompletesWithNull_whenCompleted_thenItemNotFound;
state_becomesErrored_whenFutureCompletesExceptionally ->
givenFutureCompletesExceptionally_whenCompleted_thenErrored;
getItemCompletableFuture_returnsOriginalFuture ->
givenRequest_whenGetItemCompletableFuture_thenReturnsOriginalFuture;
constructor_setsItemFound_whenFutureAlreadyCompletedWithValue ->
givenFutureAlreadyCompletedWithValue_whenConstructingRequest_thenItemFound;
constructor_setsItemNotFound_whenFutureAlreadyCompletedWithNull ->
givenFutureAlreadyCompletedWithNull_whenConstructingRequest_thenItemNotFound;
constructor_setsErrored_whenFutureAlreadyFailed ->
givenFutureAlreadyFailed_whenConstructingRequest_thenErrored;
state_becomesItemFound_whenFutureCompletesWithComplexObject ->
givenFutureCompletesWithComplexObject_whenCompleted_thenItemFound;
getItem_returnsEmpty_beforeCompletion ->
givenFutureNotCompleted_whenGetItem_thenReturnsEmpty; update each `@DisplayName`
if present to match the new method name semantics and ensure any references to
these method names in test suites are updated.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Rename test methods to the required givenContext_whenAction_thenOutcome format.

Current method names (for example, getAsInt_returnsValue_whenTypeIsInt) do not follow the enforced test naming convention for this repository.

As per coding guidelines: "Follow the givenContext_whenAction_thenOutcome naming convention for test methods (e.g., givenEmptyPage_whenGetSlots_thenReturnsEmpty)."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java`
around lines 23 - 126, Tests use names like getAsInt_returnsValue_whenTypeIsInt
that don't follow the repository's givenContext_whenAction_thenOutcome
convention; rename each test method to that pattern (e.g.,
getAsInt_returnsValue_whenTypeIsInt ->
givenTypeInt_whenGetAsInt_thenReturnsValue) for all affected methods such as
getAsLong_returnsValue_whenTypeIsLong,
getAsDouble_returnsValue_whenTypeIsDouble,
getAsString_returnsValue_whenTypeIsString,
getAsTimestamp_returnsValue_whenTypeIsTimestamp,
getAsSetString_returnsUnmodifiableCopy_whenTypeIsSetString,
getAsSetString_isDefensiveCopy_whenOriginalModified,
getAsInt_throwsClassCast_whenValueIsNotInteger,
getAsString_throwsClassCast_whenValueIsNotString,
getAsLong_throwsClassCast_whenValueIsNotLong,
getAsTimestamp_throwsClassCast_whenValueIsNotInstant,
recordAccessors_returnConstructorValues, equals_returnsTrue_whenSameValues;
follow the exact pattern given{Context}_when{Action}_then{Outcome} and update
any references/imports if the test names are used elsewhere.

[github-actions]

Extensibility Review

Breaking change risk: NONE — this diff adds only test classes with no modifications to any production source files.

No extensibility concerns found.

All changes are confined to src/test/java and introduce new JUnit 5 test classes covering existing production types (StartupProfile, ReloadableContentManager, ReloadableContent, GetItemRequest, StatisticEntry). No public interfaces, abstract classes, method signatures, registry constants, GUI framework types, database framework types, or @Deprecated symbols are touched. There are no contract changes of any kind for downstream plugin consumers.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff, examining each new test file and inferring what production classes are being exercised.

---

Analysis

Production files changed (inferred from test targets):

• com.diamonddagger590.mccore.bootstrap.StartupProfile (enum)
• com.diamonddagger590.mccore.configuration.ReloadableContentManager
• com.diamonddagger590.mccore.configuration.ReloadableContent
• com.diamonddagger590.mccore.database.response.GetItemRequest
• com.diamonddagger590.mccore.statistic.StatisticEntry

Test files present:

• StartupProfileTest.java
• ReloadableContentManagerTest.java
• ReloadableContentTest.java
• GetItemRequestTest.java
• StatisticEntryTest.java

---

Findings

---

CONCERN: StatisticEntryTest uses new NamespacedKey("mccore", "test_stat") with the deprecated two-arg string constructor, which requires a Bukkit server instance to be initialized on some Bukkit versions, yet MockBukkit is never set up (MockBukkit.mock() / MockBukkit.unmock() are absent).
WHY: The deprecated NamespacedKey(String, String) constructor bypasses plugin-owner validation but still instantiates a Bukkit type. If the Bukkit version in the classpath performs any static server-side validation during construction, tests will fail non-deterministically or with opaque errors. More importantly, the test claims to be pure-Java but silently depends on a Bukkit type. If NamespacedKey is truly inert here, this is at minimum an undocumented environmental assumption; if it is not, the test suite is broken without MockBukkit scaffolding.
WHERE: StatisticEntryTest.java / com.diamonddagger590.mccore.statistic.StatisticEntry

---

CONCERN: StatisticEntryTest has no @BeforeEach/@AfterEach MockBukkit lifecycle, yet it uses a Bukkit NamespacedKey as a field-level constant (static final), meaning the constant is initialized at class-load time — before any JUnit lifecycle method could set up MockBukkit.
WHY: Static field initialization happens before @BeforeEach, so even adding MockBukkit setup later would not protect TEST_KEY initialization. The correct fix is either to use a non-Bukkit key type in tests or to switch to a MockBukkit-backed approach where the key is created inside an instance method after MockBukkit.mock().
WHERE: StatisticEntryTest.java (line: private static final NamespacedKey TEST_KEY = new NamespacedKey("mccore", "test_stat"))

---

CONCERN: StatisticEntryTest exercises StatisticType.TIMESTAMP by constructing an Instant via Instant.ofEpochSecond(1_000_000) directly, which is correct for the value itself, but StatisticEntry stores a timestamp — if the production StatisticEntry internally calls Instant.now() or System.currentTimeMillis() anywhere (e.g., a default-timestamp constructor overload or an audit field), those paths would not be covered by a TimeProvider-injected clock.
WHY: The diff does not show the StatisticEntry production source, so it cannot be confirmed that no wall-clock calls exist. If any exist, they violate the TimeProvider rule and the tests do not inject a fixed clock.
WHERE: StatisticEntryTest.java / com.diamonddagger590.mccore.statistic.StatisticEntry — requires inspection of production source.

---

CONCERN: ReloadableContentManagerTest passes null as the constructor argument (new ReloadableContentManager(null)) with no test verifying what happens when reloadAllContent() is called and the manager attempts to use that null argument internally (e.g., a plugin reference for logging).
WHY: This is a latent null-tolerance gap: the tests pass because the null is never dereferenced by the paths under test, but no test explicitly asserts the null-safe contract. If the production class stores the null and later dereferences it in a path not yet exercised, a future test addition will see confusing NullPointerExceptions with no documented rationale for why null is acceptable.
WHERE: ReloadableContentManagerTest.java / com.diamonddagger590.mccore.configuration.ReloadableContentManager

---

CONCERN: ReloadableContentManagerTest does not test trackReloadableContent with a null argument (single item or null collection), nor does it test reloadAllContent after tracking content whose reload callback throws an exception.
WHY: Checklist item: edge cases — null inputs and exception propagation from callbacks are not covered. If the production code does not guard against a null-returning or exception-throwing callback, unhandled exceptions during reload could silently swallow errors or corrupt state for remaining items.
WHERE: ReloadableContentManagerTest.java / com.diamonddagger590.mccore.configuration.ReloadableContentManager

---

CONCERN: ReloadableContentTest does not test what happens when the YAML document does not contain the requested route (missing key). doc.getString(missingRoute) may return null, and neither the constructor nor reloadContent() is tested for that path.
WHY: Checklist item: null inputs / missing data edge cases. If ReloadableContent wraps the result in an Optional or has null-protection, that path is untested. If it does not, callers receive a silent null.
WHERE: ReloadableContentTest.java / com.diamonddagger590.mccore.configuration.ReloadableContent

---

CONCERN: StartupProfileTest includes tests for values(), valueOf(), and name() — these are compiler-generated enum methods with zero custom logic. Testing them adds noise and provides no meaningful coverage signal.
WHY: Not a style preference — this is a structural problem: the checklist requires tests for "non-trivial logic (>3 lines)." Compiler-generated enum intrinsics have no logic to break. These tests cannot fail unless the enum constants are renamed, which a compilation error would catch anyway. They inflate the test count without guarding against any real regression.
WHERE: StartupProfileTest.java — values_containsBothProfiles, valueOf_returnsCorrectProfile, name_returnsPROD_forProdProfile, name_returnsTEST_forTestProfile

---

CONCERN: StartupProfileTest does not test the behavior when setSystemProperty() is called on an already-set property from a different JVM test (i.e., no test verifies the property is absent before being set, meaning test ordering could affect results if teardown fails).
WHY: The @AfterEach clears the property correctly for normal cases, but if setSystemProperty_overridesPrevious_whenCalledAgain fails mid-execution (after the first assertEquals but before the second setSystemProperty call), the property remains set to "false" for subsequent tests. This is a minor but real isolation risk because the @AfterEach would still run — this is actually handled. Upon closer review, @AfterEach does protect correctly. Withdrawing this as a structural concern.

---

CONCERN: GetItemRequestTest does not test concurrent completion — i.e., what happens if getItemResponseState() is called while the future is being completed on another thread. If GetItemRequest derives state lazily from the future on each call (rather than caching it), concurrent reads are safe. If it caches state, a race exists. Neither path is tested.
WHY: CompletableFuture is explicitly a concurrency primitive. The class under test is a database response wrapper. The absence of any thread-safety or memory-visibility test is a coverage gap for a class whose primary use case is cross-thread completion.
WHERE: GetItemRequestTest.java / com.diamonddagger590.mccore.database.response.GetItemRequest

---

CONCERN: GetItemRequestTest does not test GetItemResponseState.PENDING_RESPONSE transitioning to ITEM_FOUND and then being read again — i.e., the state is only observed once after completion. If getItemResponseState() is computed from a snapshot at construction time rather than re-derived, a second call post-completion may return stale state.
WHY: There is no test that calls getItemResponseState() before completion, then completes the future, then calls it again and asserts the updated state. state_becomesItemFound_whenFutureCompletesWithValue only reads state after completion, not before-and-after in sequence.
WHERE: GetItemRequestTest.java / com.diamonddagger590.mccore.database.response.GetItemRequest

---

CONCERN: No test file is placed in src/testFixtures/java/. If downstream repositories (e.g., McRPG) need to create ReloadableContent, StatisticEntry, or GetItemRequest instances in their own tests, they have no shared fixtures to depend on.
WHY: Checklist item: shared fixtures belong in src/testFixtures/java/. Builders, factory helpers, or pre-configured test instances for these new types are absent.
WHERE: src/testFixtures/java/ — absent entirely for all new types

---

CONCERN: ReloadableContentTest.reloadContent_reflectsUpdate_whenYamlModified declares throws IOException in the method signature but reloadContent() itself does not throw checked exceptions. The IOException is a leftover from yamlDocument.set(...) — worth flagging only if set genuinely throws IOException and the test is suppressing it silently without asserting the no-throw contract.
WHY: If yamlDocument.set(route, "updated") can throw IOException in the production library version in use, the test should either assert it does not throw or handle the case. Currently the exception would propagate as a test failure with no descriptive assertion, making failures harder to diagnose.
WHERE: ReloadableContentTest.java — reloadContent_reflectsUpdate_whenYamlModified

---

Summary

Production files changed: StartupProfile, ReloadableContentManager, ReloadableContent, GetItemRequest, StatisticEntry

Test files present: StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java

Coverage gaps:

1. StatisticEntryTest uses Bukkit NamespacedKey as a static final without MockBukkit — structural Bukkit-dependency risk
2. StatisticEntryTest Bukkit NamespacedKey initialized at class-load time, before any lifecycle method can guard it
3. StatisticEntry timestamp paths unverified for wall-clock calls (TimeProvider compliance unconfirmed)
4. ReloadableContentManager null-argument constructor contract undocumented and untested
5. ReloadableContentManager missing edge cases: null tracked content, exception-throwing callbacks
6. ReloadableContent missing edge case: missing/null YAML route key
7. StartupProfileTest tests compiler-generated enum methods with no custom logic (structural noise)
8. GetItemRequest missing concurrent-completion and state-transition (before→after) tests
9. No src/testFixtures/java/ fixtures provided for any new type

[github-actions]

Security Review

No security concerns found.

The diff contains only test classes (*Test.java) under src/test/. No production framework code — DAO methods, DDL migration functions, MiniMessage deserialization call sites, ChatResponse implementations, Slot.onClick() overrides, or permission-gated operations — was added or modified. None of the checklist items apply to this changeset.

[github-actions]

Extensibility Review

Breaking change risk: NONE — all production source changes are internal field visibility modifiers; all other additions are test-only.

---

Reviewing each change against the checklist:

Production source changes (GetItemRequest.java):
Adding volatile to two private fields (responseState, item) is a pure implementation detail. It changes no public API surface — not the class name, not method signatures, not return types, not annotations. Binary compatibility is fully preserved. No new abstract methods, no renamed symbols, no contract changes.

Test source additions:
All four new files are in src/test/java and are package-private test classes (class without public). They are never shipped in the production artifact and are invisible to downstream consumers. They exercise existing public APIs but introduce no new public contracts themselves.

---

Scanning specifically for checklist items across all changed files:

• No new abstract methods added to any class
• No public symbol renamed
• No @Deprecated usage introduced
• No method signature changes
• No GUI framework types touched (BaseGui, PaginatedGui, Slot, GuiManager)
• No UpdateTableFunction or raw DDL changes
• No new public methods requiring @NotNull/@Nullable annotation audit
• No new RegistryKey/ManagerKey constants
• No consumer-plugin-specific logic embedded
• No Optional<T> / @Nullable mixing on new method boundaries
• No extension point gaps for new functionality (no new functionality exposed publicly)

---

No extensibility concerns found.

[github-actions]

Testing Review

I'll systematically apply every checklist item to each production file changed and each test file added.

---

Production Files Analysis

GetItemRequest.java

The change adds volatile to responseState and item. This is a concurrency correctness fix — the motivation is that without volatile, writes from the CompletableFuture completion thread may not be visible to threads reading these fields.

New Test Files

• StartupProfileTest.java
• ReloadableContentManagerTest.java
• ReloadableContentTest.java
• GetItemRequestTest.java
• StatisticEntryTest.java

---

Checklist Application

Coverage Completeness

CONCERN: The volatile fix is a concurrency correctness bug fix, but the concurrent visibility tests in GetItemRequestTest do not actually prove that volatile is necessary or that stale reads were possible without it.
WHY: The concurrent tests (getItemResponseState_isVisible_whenFutureCompletedFromAnotherThread, etc.) use a CountDownLatch to synchronize before reading. The await() call on the latch acts as a happens-before barrier, meaning the test would pass even with non-volatile fields — the latch's release already guarantees visibility. These tests cannot serve as regression tests for the volatile fix because they don't stress the actual exposure window. A true regression test would need to read the fields without a synchronization barrier between writer and reader threads. As written, these tests do not prove the fix is needed, and they will not fail if volatile is removed.
WHERE: GetItemRequestTest.java / GetItemRequest.java

---

CONCERN: StatisticEntryTest instantiates NamespacedKey using the deprecated new NamespacedKey("mccore", "test_stat") constructor, which requires a Bukkit runtime or at minimum a specific internal class path to resolve without throwing. The test file imports org.bukkit.NamespacedKey but does not set up MockBukkit.
WHY: NamespacedKey's deprecated two-arg constructor (String, String) does not require a Plugin instance, so it may work without MockBukkit — but this is relying on Bukkit internal behavior that is not guaranteed across versions and is explicitly deprecated. More critically, the test class has zero MockBukkit setup (MockBukkit.mock() / MockBukkit.unmock()), yet it directly uses a Bukkit API class. If NamespacedKey validation or static initialization ever touches Bukkit's server state, these tests will produce misleading failures unrelated to their intent. The @SuppressWarnings("deprecation") annotation acknowledges the problem but does not resolve it.
WHERE: StatisticEntryTest.java / StatisticEntry.java

---

CONCERN: There is no test for StatisticEntry with a null value, a null key, or a mismatched StatisticType vs. value type at construction time.
WHY: The checklist requires edge cases including null inputs. StatisticEntry is a record, so null key or null value are legal at construction. Whether getAsInt(), getAsLong(), etc. throw NullPointerException or behave otherwise on a null value is untested. Similarly, whether passing a null StatisticType produces a meaningful error is not covered.
WHERE: StatisticEntryTest.java / StatisticEntry.java

---

CONCERN: There is no test for ReloadableContent with a null value returned by the reload callback (i.e., when the YAML key does not exist).
WHY: The checklist requires null input and edge case coverage. A missing YAML key causes doc.getString(r) to return null. The behavior of ReloadableContent when the callback returns null — whether getContent() returns null, throws, or falls back to a default — is completely untested.
WHERE: ReloadableContentTest.java / ReloadableContent.java

---

CONCERN: ReloadableContentManagerTest passes null as the constructor argument to ReloadableContentManager (new ReloadableContentManager(null)) without a test verifying the contract around that null argument, and without a test for what happens when trackReloadableContent is called with null.
WHY: The checklist requires null input edge cases. If ReloadableContentManager stores or uses the constructor argument, passing null may produce a NullPointerException in paths not exercised here, and there is no test verifying the null-safety contract of either the constructor or trackReloadableContent(null).
WHERE: ReloadableContentManagerTest.java / ReloadableContentManager.java

---

CONCERN: StartupProfileTest tests values() by asserting ordinal order (values[0] is PROD, values[1] is TEST). This encodes enum declaration order as a test assertion.
WHY: This is a structural problem: if the enum values are reordered, the test fails for a reason unrelated to behavior. The test should assert membership, not position. This creates a false coupling between enum declaration order and test correctness. The correct approach is to assert Set.of(StartupProfile.values()).containsAll(Set.of(StartupProfile.PROD, StartupProfile.TEST)) and size equals 2 — or use EnumSet.
WHERE: StartupProfileTest.java / StartupProfile.java

---

CONCERN: StartupProfileTest tests valueOf() and name() — these are Java-generated enum methods, not custom logic in StartupProfile. Testing them provides zero coverage of actual production behavior.
WHY: The checklist says to flag coverage gaps, not style, but this is a structural problem: tests that only exercise compiler-generated behavior consume test maintenance budget without providing signal about production logic. More importantly, the coverage these tests appear to provide (StartupProfile appears well-tested) is illusory — the actual behavior of setSystemProperty() is the only non-trivial logic, and it is adequately tested in other methods. The valueOf and name tests should be removed and replaced with tests for any actual conditional logic in StartupProfile that is not yet covered.
WHERE: StartupProfileTest.java

---

CONCERN: GetItemRequestTest's cross-thread tests run 100 iterations in a tight loop on a single-threaded executor without resetting the executor between iterations. If one iteration fails, the executor may be in an unexpected state for subsequent iterations, making failure diagnosis unreliable.
WHY: This is a structural test quality problem. Each iteration creates a new future and request, but all share the same executor and latch pattern. A single slow iteration that times out on await() will cause the assertion to fire mid-loop, leaving the executor's pending work from remaining iterations unfinished. The shutdown() in finally without awaitTermination() means those tasks may not complete before the test exits.
WHERE: GetItemRequestTest.java

---

CONCERN: GetItemRequest.getItem() returns an Optional<T>. The tests verify isEmpty() and isPresent() / get(), but there is no test for the behavior when getItem() is called multiple times after completion — specifically, whether the Optional is constructed fresh on each call or cached.
WHY: If getItem() is computed from volatile T item on every call, repeated calls after a null completion should consistently return empty. If item is set once and the Optional wrapping happens inside getItem(), there is a question of whether the volatile read is always performed. This is relevant to the correctness of the fix but is not exercised.
WHERE: GetItemRequestTest.java / GetItemRequest.java

---

TimeProvider Usage

No new code in the diff calls System.currentTimeMillis() or Instant.now() directly. StatisticEntryTest uses Instant.ofEpochSecond(1_000_000) as a fixed value, not Instant.now(). No TimeProvider concerns.

---

MockBukkit Usage

CONCERN: StatisticEntryTest uses org.bukkit.NamespacedKey without MockBukkit setup or teardown.
WHY: A Bukkit class is used in a test with no MockBukkit.mock() / MockBukkit.unmock() lifecycle. Even if the deprecated two-arg constructor currently works without a server instance, this is fragile and not consistent with the framework's test infrastructure requirements. If NamespacedKey ever validates against the server, the test will fail with an opaque Bukkit error rather than a meaningful assertion failure.
WHERE: StatisticEntryTest.java

---

Bukkit-Dependent vs. Pure-Java Separation

CONCERN: StatisticEntryTest spins up zero Bukkit APIs beyond constructing a NamespacedKey, yet it does not use MockBukkit. The NamespacedKey dependency on StatisticEntry couples what should be a pure-data record test to the Bukkit API surface.
WHY: If StatisticEntry is a pure data record that happens to use NamespacedKey as an identifier, the test should either use MockBukkit properly or the production class should accept a more abstract key type so the test remains purely JUnit. As currently structured, the test is neither a proper Bukkit-lifecycle test nor a clean pure-Java test.
WHERE: StatisticEntryTest.java / StatisticEntry.java

---

Framework Test Quality

CONCERN: The getItemResponseState_isVisible_* cross-thread tests lack @AfterEach cleanup for the ExecutorService. While executor.shutdown() is called in finally, there is no executor.awaitTermination(), meaning the executor's worker thread may still be running when the test completes. Across multiple cross-thread tests, executors from earlier tests may be live during later tests.
WHY: Resource leak across tests — a structural problem rather than a style preference.
WHERE: GetItemRequestTest.java

---

CONCERN: ReloadableContentManagerTest.trackReloadableContent_deduplicates_whenSameInstanceTrackedTwice asserts that tracking the same instance twice results in it being reloaded once, with the comment "due to Set". This test is asserting an implementation detail (that the backing collection is a Set) rather than the observable contract.
WHY: If ReloadableContentManager changes its backing collection from Set to List for a valid reason (e.g., ordering guarantees), this test will fail even if the reload behavior remains correct. The test should assert behavior, not structure. The DisplayName even says "due to Set" — this is encoding implementation assumption into the test.
WHERE: ReloadableContentManagerTest.java

---

Summary

Production files changed: GetItemRequest.java

Test files present: StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java

Coverage gaps:

• Cross-thread volatile regression tests do not actually expose the race condition they intend to guard against (latch provides its own happens-before barrier)
• StatisticEntry null key, null value, null type construction not tested
• ReloadableContent callback returning null (missing YAML key) not tested
• ReloadableContentManager constructor null argument contract and trackReloadableContent(null) not tested
• GetItemRequest.getItem() repeated-call consistency after completion not tested
• StatisticEntryTest uses Bukkit NamespacedKey without MockBukkit lifecycle — neither a proper Bukkit test nor a clean pure-Java test
• Enum ordinal-order assertion in StartupProfileTest.values_containsBothProfiles encodes declaration order, not membership
• values(), valueOf(), name() tests in StartupProfileTest exercise compiler-generated behavior only, providing false coverage signal
• Executor not awaited in finally blocks in cross-thread tests — potential resource leak across test methods

[github-actions]

Security Review

No security concerns found.

The diff contains two categories of changes, neither of which touches an attack surface identified in the checklist:

1. GetItemRequest.java — The only production change is adding volatile to responseState and item. This is a thread-visibility correctness fix with no bearing on injection, permission gating, MiniMessage deserialization, or onClick() return values.

2. All *Test.java files — These are new unit tests (StartupProfileTest, ReloadableContentManagerTest, ReloadableContentTest, GetItemRequestTest, StatisticEntryTest). Test code does not run on the server and is not reachable by players. No test constructs SQL queries, calls deserialize(), issues commands, or interacts with inventory slots.

No checklist item applies to any changed line.

[github-actions]

Extensibility Review

Breaking change risk: NONE — all changes are confined to test sources, test fixtures, and an internal volatile qualifier addition with no public API surface changes.

---

Reviewing each file against the checklist:

GetItemRequest.java — The only production change is adding volatile to two private fields (responseState, item). This is an internal visibility fix; no public method signatures, return types, or field names are changed. Binary and source compatibility is fully preserved.

ManagedExecutorExtension.java — Lives in src/testFixtures/. This is test infrastructure, not a shipped API. It is not in McCore's public framework contract. No concerns.

StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java — All in src/test/. No production code is modified. No concerns.

.coderabbit.yaml — CI/review configuration only. No impact on compiled artifacts or downstream consumers.

---

No extensibility concerns found.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff.

---

Findings

---

CONCERN: StatisticEntryTest constructs NamespacedKey using the deprecated two-arg string constructor without MockBukkit running, relying on internal Bukkit state that may behave differently across CraftBukkit versions or fail entirely if Bukkit validation is tightened.
WHY: The @SuppressWarnings("deprecation") annotation signals the constructor is deprecated. The deprecated NamespacedKey(String, String) constructor bypasses the plugin-owner check, but it still touches Bukkit's namespace validation. The test spins up zero MockBukkit infrastructure yet uses a Bukkit type. If Bukkit ever enforces plugin context in this constructor, all 12 tests silently break. The correct approach is either NamespacedKey.fromString("mccore:test_stat") (no Bukkit context needed) or running MockBukkit and using new NamespacedKey(plugin, "test_stat").
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java (line private static final NamespacedKey TEST_KEY = new NamespacedKey("mccore", "test_stat"))

---

CONCERN: StatisticEntryTest has no test for a null value passed to the constructor.
WHY: Edge-case coverage checklist requires null-input testing for non-trivial public methods. StatisticEntry is a record whose constructor accepts Object value. If the production code or a downstream caller passes null, getAsInt(), getAsString(), etc. will throw NullPointerException rather than ClassCastException. There is no test documenting or asserting that contract, leaving the null-value path completely uncovered.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / com.diamonddagger590.mccore.statistic.StatisticEntry

---

CONCERN: StatisticEntryTest has no test for getAsSetString when the stored set is empty.
WHY: Edge-case coverage checklist requires empty-collection testing. An empty LinkedHashSet is a distinct boundary condition for getAsSetString; the defensive-copy and unmodifiable-wrapper logic should still hold, but it is not exercised.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / StatisticEntry.getAsSetString()

---

CONCERN: StatisticEntryTest has no test verifying that getAsDouble throws ClassCastException when the stored value is not a Double.
WHY: The cross-type cast tests cover INT→String, INT→Long, STRING→int, and STRING→Instant, but getAsDouble miscast is not covered. This leaves an asymmetric gap in the negative-path matrix relative to the other typed accessors.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / StatisticEntry.getAsDouble()

---

CONCERN: StartupProfileTest.values_containsBothProfiles asserts on ordinal position (values[0], values[1]), making the test fragile and encoding an undocumented contract about enum declaration order.
WHY: This is a structural problem, not a style preference. If a third profile is inserted before PROD or between PROD and TEST, the test fails for the wrong reason and gives a misleading signal. The correct assertion is membership (assertThat(values).contains(PROD, TEST)) plus a length check, or individual assertSame without positional indexing. As written, any reordering in the production enum silently inverts what the test is verifying.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java:values_containsBothProfiles

---

CONCERN: StartupProfileTest contains two tests (valueOf_returnsCorrectProfile, name_returnsPROD_forProdProfile, name_returnsTEST_forTestProfile) that test only Java language-level enum mechanics, not any authored logic.
WHY: These tests add no coverage of authored code — Enum.valueOf() and Enum.name() are guaranteed by the JLS. A test that cannot possibly fail against production code does not satisfy the "every test must have at least one meaningful assertion" spirit of the checklist. More importantly, if StartupProfile is changed to a non-enum class, these tests disappear harmlessly, meaning they never guarded anything. These slots should instead be used to cover the untested logic paths identified below.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java

---

CONCERN: There is no test for StartupProfile behavior when setSystemProperty is called and the system property key is already set to a non-boolean-looking value by an external agent.
WHY: setSystemProperty always calls System.setProperty, which overwrites regardless. However, the test suite does not cover the case where a pre-existing foreign value exists before any setSystemProperty call — only post-set overwriting (setSystemProperty_overridesPrevious_whenCalledAgain) is tested. While the production behavior is trivially deterministic, the @AfterEach teardown only calls System.clearProperty, meaning a test failure mid-suite could leave the property set and affect later tests. The missing edge case is: what is the property value before setSystemProperty is ever called? There is no assertion that it is null / absent, so tests are implicitly order-dependent.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java / com.diamonddagger590.mccore.bootstrap.StartupProfile

---

CONCERN: ReloadableContentManagerTest constructs ReloadableContentManager with null as its constructor argument but does not test that passing null is explicitly supported or that it throws a documented exception.
WHY: manager = new ReloadableContentManager(null) is used as test scaffolding, but there is no test asserting the null-argument contract of ReloadableContentManager's constructor. If the constructor gains a @NotNull enforcement, the existing tests all break simultaneously rather than a single null-arg test failing. The null path must be tested explicitly.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java:setUp / com.diamonddagger590.mccore.configuration.ReloadableContentManager

---

CONCERN: ReloadableContentManagerTest has no test for trackReloadableContent with a null single element or a collection containing null.
WHY: Edge-case coverage checklist requires null-input testing. Both overloads of trackReloadableContent are public API. If a null element is passed, the behavior (NPE on track, NPE on reload, silent skip) is untested and undocumented.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java / com.diamonddagger590.mccore.configuration.ReloadableContentManager.trackReloadableContent

---

CONCERN: ReloadableContentManagerTest has no test for trackReloadableContent with an empty collection.
WHY: Edge-case coverage checklist requires empty-collection testing. trackReloadableContent(List.of()) is a legal call; the test suite does not verify it completes without error and that a subsequent reloadAllContent is also safe.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java / com.diamonddagger590.mccore.configuration.ReloadableContentManager.trackReloadableContent

---

CONCERN: ReloadableContentTest has no test for the two-argument constructor (YamlDocument, Route, BiFunction) when the YAML key is absent (returns null from BoostedYAML).
WHY: Edge-case coverage checklist requires null-output edge cases. If doc.getString(r) returns null because the key does not exist, ReloadableContent stores null as its content. There is no test asserting that getContent() returns null (or that the constructor throws) in this scenario, leaving the missing-key path undocumented and uncovered.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java / com.diamonddagger590.mccore.configuration.ReloadableContent

---

CONCERN: GetItemRequestTest's three concurrent stress tests (getItemResponseState_isVisible_*) loop 100 times inside a single test method, but the CountDownLatch only guarantees the submitting thread's latch.countDown() has been called — it does not guarantee that the whenComplete callback on the CompletableFuture has finished executing before the assertions run.
WHY: Structural correctness problem. CompletableFuture.complete() triggers whenComplete callbacks, but those callbacks may run on a different thread (the completing thread, a common pool thread, or the caller). After latch.await() returns, the completing thread has called latch.countDown() but the whenComplete handler updating responseState and item may not yet have been observed by the test thread, even with volatile. The test needs either a second synchronization point after latch.await() (e.g., future.join()) or an assertion loop with timeout to avoid a data-race between the callback write and the test-thread read.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java:getItemResponseState_isVisible_whenFutureCompletedFromAnotherThread (and the two sibling concurrent tests)

---

CONCERN: The .coderabbit.yaml change defines a test naming convention (methodUnderTest_expectedOutcome_whenCondition) that conflicts with the Testing Auditor checklist's givenContext_whenAction_thenOutcome convention, but the new test files added in this same diff use the givenContext_whenAction_thenOutcome pattern in @DisplayName and a mixed approach in method names.
WHY: Structural consistency problem. StartupProfileTest method names follow methodUnderTest_setsTrue_whenProd (matching the .coderabbit.yaml convention), while @DisplayName strings use "Given/When/Then" sentences. GetItemRequestTest method names such as constructor_throwsNullPointerException_whenFutureIsNull also match the YAML convention. This is internally consistent within this diff. However, the auditor checklist mandates givenContext_whenAction_thenOutcome method names, creating a direct conflict between the .coderabbit.yaml instruction added here and the auditor's naming requirement. This needs explicit resolution: which convention is authoritative for McCore?
WHERE: .coderabbit.yaml / all new test files in src/test/java/

---

CONCERN: GetItemRequestTest does not test concurrent writes — two threads completing the same CompletableFuture simultaneously.
WHY: The volatile fix on responseState and item was motivated by cross-thread visibility. The tests cover single-writer cross-thread scenarios but not the race between two threads both calling future.complete() (one of which will win; the other is a no-op). The whenComplete handler will fire once, but without a test, the correctness of the winner-takes-all path under the new volatile fields is not verified.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java / com.diamonddagger590.mccore.database.response.GetItemRequest

---

Summary

Production files changed:
src/main/java/com/diamonddagger590/mccore/database/response/GetItemRequest.java

Test files present:
src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java,
src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java,
src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java,
src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java,
src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java,
src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java

Coverage gaps:

• StatisticEntry: null value constructor input; empty SET_STRING; getAsDouble miscast path; Bukkit-context-free NamespacedKey construction
• StartupProfile: pre-existing property state; ordinal-position enum assertion fragility; language-mechanic-only tests consuming test slots
• ReloadableContentManager: null single-element arg; null-containing collection; empty collection to trackReloadableContent; explicit null-constructor contract
• ReloadableContent: missing YAML key returning null from callback
• GetItemRequest (concurrent): whenComplete callback vs. CountDownLatch synchronization gap; concurrent dual-writer scenario

[github-actions]

Security Review

No security concerns found.

The diff contains only:

1. .coderabbit.yaml — CI review configuration; no runtime code.
2. GetItemRequest.java — addition of volatile to responseState and item fields; this is a thread-safety correctness fix with no bearing on injection, permission gating, or any other checklist item.
3. StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java — test-only code; no production DAO methods, DDL constructors, MiniMessage calls, ChatResponse handlers, onClick() implementations, or permission-gated operations are introduced or modified.
4. ManagedExecutorExtension.java — test fixture providing a managed ExecutorService; contains no player-data handling, query building, or message deserialization.

None of the changed files touch SQL query construction, DDL generation, MiniMessage deserialization paths, ChatResponse lifecycle, inventory slot callbacks, or framework permission gates. All checklist items pass with nothing to flag.

[github-actions]

Extensibility Review

Breaking change risk: NONE — all changes are test infrastructure, documentation, and an internal volatile correctness fix with no public API surface changes.

---

Reviewing each modified surface area against the checklist:

Production code: Only GetItemRequest.java changes, and the change is narrowly private field modifiers (volatile). No public method signatures, return types, or class hierarchy are altered. Downstream plugins that call getItemResponseState(), getItem(), or getItemCompletableFuture() compile and behave identically — and are strictly safer due to the visibility fix.

Test fixtures (ManagedExecutorExtension): Placed correctly in src/testFixtures/, exported for downstream repos. The class is public, non-final, implements standard JUnit 5 interfaces, carries @NotNull on all public parameters and return values, and Javadoc covers the usage contract including lifecycle. No McCore-internal types leak into the public surface of the fixture. The getExecutor() guard (IllegalStateException when executor == null) is the correct behavior for a misused extension.

Documentation files (.claude/, .cursor/, .coderabbit.yaml, CLAUDE.md): Internal tooling guidance only. No contract surface.

New tests: All test classes are package-private, use @RegisterExtension with ManagedExecutorExtension for cross-thread cases, follow the methodUnderTest_expectedOutcome_whenCondition naming convention, carry @DisplayName in Given/When/Then format, and include @AfterEach teardown where system state (System.setProperty) is mutated.

---

One observation worth noting, though it does not rise to a breaking concern:

CONCERN: ManagedExecutorExtension.getExecutor() is documented as returning a single-thread executor, but the return type is the broad ExecutorService interface, which does not expose thread-count semantics. A caller who submits tasks expecting serial ordering is relying on undocumented behavior of the concrete type.
WHY: If a downstream test fixture consumer submits two tasks and asserts ordering, they are coupling to newSingleThreadExecutor() internals. If the implementation is ever widened to a pool (e.g., to support parallel task tests), those tests will silently become racy.
WHERE: src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java — getExecutor() Javadoc and method return type.

Recommend adding a Javadoc sentence to getExecutor() explicitly stating: "The returned executor is single-threaded; tasks submitted to it execute serially in submission order." This makes the ordering guarantee part of the documented contract rather than an implementation detail.

---

No other extensibility concerns found.

[github-actions]

Extensibility Review

Breaking change risk: NONE — all changes are test infrastructure, documentation, and a non-breaking volatile qualifier addition to private fields.

---

Reviewing each touched surface against the checklist:

Production code changes (GetItemRequest.java): Adding volatile to two private fields (responseState, item). These fields are private — no downstream consumer can access them directly. The public API surface (getItemResponseState(), getItem(), getItemCompletableFuture()) is unchanged in signature, return type, and nullability contract. Binary compatibility is fully preserved.

Test fixture (ManagedExecutorExtension): Placed in src/testFixtures/, which is the correct location for shared downstream consumption (e.g., McRPG). The class is public, non-abstract, implements standard JUnit 5 interfaces. getExecutor() is annotated @NotNull. No concern.

Test files (StartupProfileTest, ReloadableContentTest, ReloadableContentManagerTest, GetItemRequestTest, StatisticEntryTest): Test-only, no impact on the public API contract.

Documentation files (.claude/commands/, .coderabbit.yaml, .cursor/rules/, CLAUDE.md): No code contract changes.

---

One observation worth noting, though it falls below a formal concern threshold:

ManagedExecutorExtension.getExecutor() is documented as returning a single-thread executor, but the return type is ExecutorService — the broader interface. Downstream consumers who need single-thread ordering guarantees (e.g., sequential task submission) cannot rely on that property through the declared type. This is a documentation gap, not a breaking change, but if downstream test code starts depending on single-thread ordering, it creates a silent coupling to the implementation detail. Consider either narrowing the return type to ExecutorService with a Javadoc note on the threading model, or simply reinforcing in Javadoc that callers must not assume thread count — which the current Javadoc already partially does by saying "single-thread" in the class description. This is sufficient for now.

---

No extensibility concerns found.

[github-actions]

Testing Review

I'll systematically apply every checklist item to the diff.

---

Analysis

Production files changed:

• src/main/java/com/diamonddagger590/mccore/database/response/GetItemRequest.java (fields made volatile)

Documentation/config files changed (no production logic):

• .claude/commands/review-testing.md
• .coderabbit.yaml
• .cursor/rules/persona-testing.mdc
• CLAUDE.md

Test files present:

• src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java
• src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java
• src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java
• src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java
• src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

Test fixture files present:

• src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java

---

Checklist Findings

Coverage Completeness

CONCERN: StatisticEntryTest uses NamespacedKey via its deprecated two-arg string constructor without MockBukkit, which may fail or produce unexpected behavior depending on whether NamespacedKey has a Bukkit-server dependency at construction time.
WHY: NamespacedKey is a Bukkit class. The deprecated constructor used here (new NamespacedKey("mccore", "test_stat")) does not require a plugin instance, but it still pulls in the Bukkit API. If NamespacedKey performs any validation against a running server (e.g., in future Bukkit versions), these tests will break silently. More critically, the test class has zero MockBukkit setup (MockBukkit.mock() / MockBukkit.unmock()), yet it instantiates a Bukkit type. This is structurally inconsistent with the checklist rule: "Does any test spin up MockBukkit but call zero Bukkit APIs?" — the inverse also applies: tests that use Bukkit types without MockBukkit are fragile.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / StatisticEntry production class

---

CONCERN: ReloadableContentManagerTest passes null as the constructor argument for ReloadableContentManager without any test verifying null-safety or documenting that null is a valid input.
WHY: new ReloadableContentManager(null) silently succeeds. If the constructor parameter is a plugin reference (common in McCore), null is likely not a valid production value. There is no test for what happens when the manager is constructed with a non-null plugin, and no test documenting that null is intentionally supported. This is a coverage gap: the constructor's behavior under the actual production input is untested, and the null path is only exercised accidentally.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java line 29 / ReloadableContentManager production class

---

CONCERN: ReloadableContentManagerTest.trackReloadableContent_deduplicates_whenSameInstanceTrackedTwice asserts deduplication via Set semantics, but does not verify that two distinct but equal ReloadableContent instances are also deduplicated — or conversely, that they are not deduplicated if ReloadableContent uses reference equality.
WHY: The test only exercises the same-reference case. If ReloadableContent does not override equals/hashCode, then two logically identical instances backed by the same route and document will both be tracked and both reloaded, which would be a behavioral gap. The current test does not distinguish between reference-equality deduplication and value-equality deduplication, leaving that behavior untested.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java / ReloadableContentManager production class

---

CONCERN: There is no test covering GetItemRequest under concurrent writes — specifically, no test verifies that if two threads race to complete the same CompletableFuture, the responseState and item fields are consistent with each other (both reflect the same completion outcome) after the race.
WHY: The volatile fix is specifically motivated by cross-thread visibility of responseState and item. The existing cross-thread tests only verify visibility after a CountDownLatch ensures the write has completed — they do not test the race condition where the reader thread observes a partially-written state (e.g., responseState == ITEM_FOUND but item still null). This is the exact bug the volatile keyword was added to prevent, and there is no regression test that would catch a reversion to non-volatile fields under concurrent access.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java / src/main/java/com/diamonddagger590/mccore/database/response/GetItemRequest.java

---

CONCERN: StartupProfileTest.values_containsBothProfiles and valueOf_returnsCorrectProfile test auto-generated enum methods (values(), valueOf(), name()). These are Java-compiler-generated and cannot have bugs. They pad the test count without providing meaningful coverage.
WHY: This is a coverage gap in the opposite direction — time spent testing compiler-generated behavior crowds out tests for actual StartupProfile logic. Missing: a test for what happens when setSystemProperty is called and the property was already set by another profile in the same JVM run without the @AfterEach teardown (e.g., parallel test execution). More importantly, there is no test verifying the behavior of any other non-trivial logic in StartupProfile if it exists beyond setSystemProperty.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java

---

TimeProvider Usage

No new production code calls System.currentTimeMillis() or Instant.now() directly in the diff. StatisticEntryTest uses Instant.ofEpochSecond(1_000_000) as a fixed value (not wall-clock), which is correct. No TimeProvider concerns flagged.

---

MockBukkit Usage

CONCERN: StatisticEntryTest instantiates NamespacedKey (a Bukkit API class) without MockBukkit.mock() / MockBukkit.unmock().
WHY: Per the checklist: "Is MockBukkit set up and torn down correctly — not leaked across tests?" The test uses a Bukkit type without any MockBukkit lifecycle. If NamespacedKey's constructor accesses Bukkit server state in any code path (validation, registry lookup in newer Paper API versions), the test will throw an uninformative NPE or IllegalStateException rather than a meaningful assertion failure. The test should either use MockBukkit for the full class, or — if StatisticEntry can be redesigned to use a plain string key — the Bukkit dependency should be removed from the pure-logic test.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

---

Bukkit-Dependent vs. Pure-Java Separation

CONCERN: StatisticEntryTest mixes a Bukkit type (NamespacedKey) into what is otherwise a pure-Java test of value retrieval and casting behavior.
WHY: Per the checklist: "Does any class mix pure logic with Bukkit API calls where only the pure logic is tested? Extract and unit-test the pure logic separately." The casting and type-access logic in StatisticEntry is pure Java. The NamespacedKey is only used as a key carrier — it is never exercised by any assertion in the test. All assertions operate on getAsInt(), getAsLong(), etc. The Bukkit dependency is dead weight in this test and creates an unnecessary coupling to the Bukkit classpath.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

---

Framework Test Quality

CONCERN: ManagedExecutorExtension does not expose a way to configure the thread count or executor type. All tests using it are locked to a single-thread executor. If a future test needs a multi-thread executor to reproduce a race condition, developers will either create a raw ExecutorService manually (violating the documented rule) or the extension will need to be extended. The extension should at minimum document this constraint and ideally accept a factory parameter.
WHY: This is a structural limitation of the shared fixture. Tests in GetItemRequestTest that attempt to verify concurrent visibility run 100 iterations on a single-thread executor, which means the "concurrent" path is actually sequential — the submitting thread and the completing thread never overlap. The latch ensures the write finishes before the read, making these tests equivalent to the single-threaded completion tests and providing no additional confidence about the volatile fix under true concurrency.
WHERE: src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java / src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java

---

CONCERN: ReloadableContentTest.reloadContent_reflectsUpdate_whenYamlModified declares throws IOException but the only IOException-throwing call is yamlDocument.set(route, "updated") — if that does not actually throw IOException, the declaration is misleading and the exception path is never exercised.
WHY: A declared checked exception that cannot actually be thrown means the test's exception handling is dead code. More importantly, if YamlDocument.set() can throw IOException in some implementations, there is no test for that error path.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java

---

Summary

Production files changed: GetItemRequest.java (volatile fields)

Test files present: StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java

Test fixture files present: ManagedExecutorExtension.java

Coverage gaps:

1. No regression test demonstrating that non-volatile fields allow torn reads (the actual bug being fixed)
2. StatisticEntryTest uses NamespacedKey without MockBukkit lifecycle — structurally incorrect for a plain JUnit test
3. ReloadableContentManager null-constructor path is exercised without documenting or asserting null is valid
4. Deduplication test only covers same-reference case, not distinct-but-equal instances
5. Cross-thread tests in GetItemRequestTest are sequential in practice due to single-thread executor — they do not exercise true concurrent visibility
6. Compiler-generated enum methods tested in StartupProfileTest add noise without coverage value

[github-actions]

Security Review

No security concerns found.

The diff contains exclusively: documentation updates (.claude/, .cursor/, .coderabbit.yaml, CLAUDE.md), a volatile correctness fix on GetItemRequest fields, a new ManagedExecutorExtension test fixture, and new unit tests for StartupProfile, ReloadableContent, ReloadableContentManager, GetItemRequest, and StatisticEntry. None of the changed or added code touches SQL query construction, DDL generation, MiniMessage deserialization, ChatResponse handling, Slot.onClick() return values, or permission-gated framework operations.

[github-actions]

Testing Review

Testing Audit Report

---

CONCERN: StatisticEntryTest imports and instantiates NamespacedKey via the deprecated no-MockBukkit constructor (new NamespacedKey("mccore", "test_stat"))
WHY: NamespacedKey is a Bukkit API class. The deprecated two-arg String constructor exists for testing, but it still loads Bukkit class infrastructure. More critically, zero MockBukkit setup/teardown is present in this test class. If the class works today only because the deprecated constructor happens to not require a live server, it is a latent fragility — and the test class should either use MockBukkit lifecycle or eliminate the Bukkit type from a pure-Java test entirely. StatisticEntry itself may be a pure-Java record, in which case the key field should be typed to a plain identifier (or the test should use MockBukkit).
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / StatisticEntry production class

---

CONCERN: StatisticEntryTest has no @SuppressWarnings scope control — the suppression is on the field but the deprecated constructor is the structural risk; the real concern is no MockBukkit.mock() / MockBukkit.unmock() pairing for a test that touches a Bukkit type.
WHY: Per the checklist: "Is MockBukkit set up and torn down correctly — not leaked across tests?" There is no @BeforeEach calling MockBukkit.mock() and no @AfterEach calling MockBukkit.unmock(). If any other test in the suite initialises MockBukkit and leaves state, or if this test runs in isolation, Bukkit class state is uncontrolled.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

---

CONCERN: StatisticEntryTest uses Instant.now() — wait, it does not; it uses Instant.ofEpochSecond(1_000_000) which is correct. However, the production StatisticEntry record stores an Object value field for TIMESTAMP typed as Instant. There is no test covering what happens when a TIMESTAMP entry is constructed with a null value and getAsTimestamp() is called.
WHY: Null input is an explicit checklist edge case: "Are edge cases covered: null inputs…?" A null value cast to Instant will produce a NullPointerException not a ClassCastException, and the behaviour should be documented and tested.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java / StatisticEntry

---

CONCERN: StatisticEntryTest does not test getAsSetString() with a null value or an empty set.
WHY: Edge cases checklist — empty collections and null inputs. An empty SET_STRING entry and a null-valued SET_STRING entry are both realistic and untested.
WHERE: src/test/java/com/diamonddagger590/mccore/statistic/StatisticEntryTest.java

---

CONCERN: ReloadableContentManagerTest constructs ReloadableContentManager(null) — passing null for what appears to be a plugin/owner parameter — and no test verifies behaviour when trackReloadableContent is called with a null argument.
WHY: Null input is an explicit checklist edge case. The production trackReloadableContent signature accepting a single ReloadableContent<?> should be tested with null to confirm it throws, returns gracefully, or documents the contract.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java / ReloadableContentManager

---

CONCERN: ReloadableContentManagerTest does not test trackReloadableContent(Collection) with a null collection or a collection containing a null element.
WHY: Edge cases checklist — null inputs, null elements within collections. If the underlying Set.addAll is called without null guards, a NullPointerException at runtime is possible.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentManagerTest.java

---

CONCERN: ReloadableContentTest does not cover the behaviour when the YAML key is missing (the route resolves to null).
WHY: Edge cases checklist — null inputs. doc.getString(missingRoute) returns null from BoostedYaml, and wrapping that in a reload callback that does .toUpperCase() will NullPointerException. The constructor_appliesTransformCallback test would be the natural place to see this fail, but the missing-key path is never exercised.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java

---

CONCERN: ReloadableContentTest does not test the two-arg constructor (new ReloadableContent(doc, route, callback)) with a null document or null route.
WHY: Null input checklist item. These are @NotNull-annotated parameters on public constructors; a test confirming NullPointerException is thrown documents the contract and guards against future annotation removal.
WHERE: src/test/java/com/diamonddagger590/mccore/configuration/ReloadableContentTest.java

---

CONCERN: The volatile changes to GetItemRequest.responseState and GetItemRequest.item are the only production code changes, and they are motivated by a concurrency correctness fix. There is no regression test that fails without volatile to pin the fix.
WHY: "If a bug was fixed, is there a regression test?" The cross-thread tests in GetItemRequestTest use a CountDownLatch to sequence completion before the assertion, which means the happens-before relationship established by CountDownLatch.await() already guarantees visibility even without volatile. The tests therefore do not demonstrate that volatile was necessary and would not catch a regression if volatile were removed.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java / src/main/java/com/diamonddagger590/mccore/database/response/GetItemRequest.java

---

CONCERN: GetItemRequestTest cross-thread tests loop 100 times inline inside a single test method. If any iteration fails, the failure message does not indicate which iteration failed.
WHY: Structural problem — not a style preference. A flaky failure in iteration 73 produces the same assertion message as iteration 1. Use @RepeatedTest(100) or assertAll with iteration context so failures are diagnosable.
WHERE: src/test/java/com/diamonddagger590/mccore/database/response/GetItemRequestTest.java (three cross-thread test methods)

---

CONCERN: StartupProfileTest.values_containsBothProfiles asserts ordinal position (values[0] is PROD, values[1] is TEST). This is an ordering assertion on an enum that has no documented ordering contract.
WHY: Structural fragility — not a style preference. If the enum declaration order changes, this test breaks for the wrong reason. The test should assert containment (assertThat(values).contains(PROD, TEST)) not positional identity.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java

---

CONCERN: StartupProfileTest does not test what happens when setSystemProperty is called from a context where a SecurityManager prohibits system property writes, nor does it test the property key constant itself against the actual string used inside StartupProfile.setSystemProperty().
WHY: The test class defines SYSTEM_PROPERTY_KEY = "mccore.testMode" as a local constant. If the production key changes, the @AfterEach cleanup silently stops clearing the right property and other tests could be poisoned by leaked state. The test should derive the key from the production class or at minimum assert the expected key is what the production method actually sets.
WHERE: src/test/java/com/diamonddagger590/mccore/bootstrap/StartupProfileTest.java / StartupProfile

---

CONCERN: ManagedExecutorExtension provides only a single-thread executor. Tests that need to verify concurrent interleaving from multiple threads (e.g., two threads completing a future simultaneously) cannot use this extension as written.
WHY: Coverage gap in the fixture itself — not a style preference. The getExecutor() API returns the raw ExecutorService; a caller could submit many tasks, but parallel concurrency requires a pool. A factory method or constructor parameter for thread count would make the fixture generally reusable. As-is, downstream tests will create their own executors anyway, defeating the fixture's purpose.
WHERE: src/testFixtures/java/com/diamonddagger590/mccore/testing/ManagedExecutorExtension.java

---

CONCERN: .claude/commands/review-testing.md, .coderabbit.yaml, .cursor/rules/core.mdc, .cursor/rules/persona-testing.mdc, and CLAUDE.md are documentation/tooling files — they contain no executable logic and have no corresponding tests. This is expected and correct; flagged only for completeness per the instruction to list all changed production files.
WHY: N/A — not a concern.
WHERE: Documentation files only.

---

Production files changed: GetItemRequest.java, StartupProfile (inferred from test), ReloadableContentManager (inferred from test), ReloadableContent (inferred from test), StatisticEntry (inferred from test), .claude/commands/review-testing.md, .coderabbit.yaml, .cursor/rules/core.mdc, .cursor/rules/persona-testing.mdc, CLAUDE.md

Test files present: StartupProfileTest.java, ReloadableContentManagerTest.java, ReloadableContentTest.java, GetItemRequestTest.java, StatisticEntryTest.java, ManagedExecutorExtension.java (testFixtures)

Coverage gaps:

• No regression test that fails without volatile on GetItemRequest fields
• No null-input tests for ReloadableContent constructor, ReloadableContentManager.trackReloadableContent, StatisticEntry typed accessors with null values
• No empty-collection test for SET_STRING in StatisticEntry
• No missing-YAML-key test in ReloadableContentTest
• No MockBukkit lifecycle in StatisticEntryTest despite use of NamespacedKey
• StartupProfileTest key constant not cross-validated against production class
• ManagedExecutorExtension fixture only supports single-thread concurrency

[github-actions]

Security Review

No security concerns found.

The diff contains only documentation updates (.claude/, .cursor/, .coderabbit.yaml, CLAUDE.md), a test-fixture lifecycle utility (ManagedExecutorExtension), and new unit tests. No DAO methods, DDL-generating functions, MiniMessage deserialization calls, ChatResponse implementations, Slot.onClick() overrides, or framework permission gates were added or modified. The one production change — adding volatile to GetItemRequest.responseState and item — is a concurrency correctness fix with no injection surface.