Add flexible loadout resolution by slot, exact name, or substring#201
Merged
Conversation
Loadout commands now accept slot index, exact name, or unique partial name in addition to the previous integer-only slot argument. Resolution priority: slot index → exact name match → substring match. If multiple loadouts match a partial name, the player is shown which loadouts collide so they can be more specific. - Add LoadoutResolution sealed interface (Found / Ambiguous / NotFound) - Add LoadoutHolder.resolveLoadout(String) with MiniMessage tag stripping - Migrate LoadoutSetCommand and LoadoutEditCommand from IntegerParser to StringParser.greedyStringParser() and wire up resolveLoadout - Add LOADOUT_MATCHES placeholder and two new localization keys (ambiguous-matches) for set and edit commands - Add LoadoutHolderResolutionTest covering all resolution branches https://claude.ai/code/session_0137VbcTTRzaaw6Rc4zsZbh2
WalkthroughAdds a loadout resolution feature allowing inputs to be resolved by numeric slot, exact name, or substring. Introduces sealed Changes
Sequence DiagramsequenceDiagram
participant Player as Player
participant Command as LoadoutEditCommand / LoadoutSetCommand
participant Holder as LoadoutHolder
participant Resolution as LoadoutResolution
Player->>Command: provide slot-or-name input
Command->>Holder: resolveLoadout(input)
alt Numeric slot resolved
Holder->>Resolution: Found(loadout)
Resolution-->>Command: Found
Command->>Command: open GUI or set slot
Command-->>Player: success message
else Unique name/substring resolved
Holder->>Resolution: Found(loadout)
Resolution-->>Command: Found
Command->>Command: open GUI or set slot
Command-->>Player: success message
else Multiple matches
Holder->>Resolution: Ambiguous(matches)
Resolution-->>Command: Ambiguous
Command->>Command: format matches -> `loadout-matches`
Command-->>Player: ambiguous message with matches
else No matches
Holder->>Resolution: NotFound()
Resolution-->>Command: NotFound
Command-->>Player: no-match message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.java`:
- Around line 187-193: The input parsing currently accepts slot numbers <=0
because hasLoadout(int) only enforces an upper bound; update the parsing and
helper so invalid low slots are rejected: in LoadoutHolder.replace the try-block
that parses int slot should check slot >= 1 && slot <= getMaxLoadoutAmount()
before returning new LoadoutResolution.Found(getLoadout(slot)), otherwise return
NotFound; also modify LoadoutHolder.hasLoadout(int) (and any shared slot helper
methods) to enforce the same lower bound (slot >= 1) in addition to the existing
upper bound to prevent other callers (e.g., LoadoutSetCommand) from persisting
impossible active slots.
- Around line 198-206: The loop currently calls getLoadout(slot) which
auto-creates missing loadouts and causes side effects; change it to a read-only
lookup that does not materialize slots (e.g., use an existing non-creating
accessor or directly check the internal map/registry) so only existing loadouts
are inspected. Specifically, in LoadoutHolder replace getLoadout(slot) in this
loop with a non-mutating lookup (for example getLoadoutIfPresent(slot) or
loadouts.get(slot)) and only add to namedLoadouts when that lookup returns a
non-null/Optional and its display name is present.
In `@src/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java`:
- Around line 71-101: Add two regression tests in LoadoutHolderResolutionTest
targeting LoadoutHolder.resolveLoadout(String): one that calls
resolveLoadout("0") and resolveLoadout("-1") and asserts the result is an
instance of LoadoutResolution.NotFound (verifying lower-bound handling), and
another that attempts a name-based lookup with a non-matching string and then
asserts the holder's observable state (e.g., currently selected loadout slot or
loadout list via mcRPGPlayer.asSkillHolder() / holder) remains unchanged after
the NotFound result; use the existing patterns from
resolveLoadout_returnsNotFound_whenSlotExceedsMax and
resolveLoadout_returnsFound_whenInputIsValidSlotNumber to locate method names
and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc9f4cbd-1e54-4f08-ad68-38b6a851c229
📒 Files selected for processing (8)
src/main/java/us/eunoians/mcrpg/command/CommandPlaceholders.javasrc/main/java/us/eunoians/mcrpg/command/loadout/LoadoutEditCommand.javasrc/main/java/us/eunoians/mcrpg/command/loadout/LoadoutSetCommand.javasrc/main/java/us/eunoians/mcrpg/configuration/file/localization/LocalizationKey.javasrc/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.javasrc/main/java/us/eunoians/mcrpg/loadout/LoadoutResolution.javasrc/main/resources/localization/english/en_commands.ymlsrc/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ai-persona-review.yml:
- Around line 197-209: The workflow step uses an unverified model name
("claude-sonnet-4-6") and pipes the curl output silently which can hide
failures; update the payload construction to derive the model from a validated
env var (e.g., ANTHROPIC_MODEL) or confirm the exact model string, and add
explicit error handling around the curl/jq commands (check curl/JQ exit codes,
capture and log response errors, and fail the job on non-2xx responses) so
failures are visible; specifically modify the JSON payload where "model":
"claude-sonnet-4-6" is set, and wrap the curl | jq pipeline used before gh pr
comment to detect errors and output a clear error message instead of producing
an empty comment.
- Around line 171-183: The workflow step hardcodes an unverified model name
("claude-sonnet-4-6") and silently proceeds on failures; update the payload so
the model is sourced from a validated variable (e.g., MODEL or ANTHROPIC_MODEL)
or confirm the exact supported model name, and add robust error handling around
the curl/jq pipeline by checking the curl/http exit status and the response for
errors before writing comment.md and running gh pr comment; specifically modify
the JSON payload construction that contains "model": "claude-sonnet-4-6" and add
checks after the curl invocation (and before jq/gh calls) to detect non-2xx
responses or missing .content[0].text, log or write the API error to comment.md,
and fail the step when appropriate.
- Around line 145-157: The workflow step uses a hard-coded model name
"claude-sonnet-4-6" and pipes the curl response into jq without checking for
failures, so update the step to validate or parameterize the model name (replace
the literal "claude-sonnet-4-6" with a workflow input or environment variable
and verify it before the call) and add explicit error handling around the curl
call (check HTTP status, capture curl exit code/response on failure, and fail
the job or write a clear error message instead of producing an empty comment);
locate the block building the JSON payload and the curl invocation (the lines
constructing "model": "claude-sonnet-4-6", the curl -sf
https://api.anthropic.com/v1/messages call, and the subsequent jq -> gh pr
comment pipeline) and modify them so failures are detected and surfaced and the
model name is configurable/validated.
- Around line 115-131: The workflow currently streams pr.diff into the jq
payload without guarding against huge diffs; before the jq invocation (the run
block that builds the JSON with --rawfile prompt
.claude/commands/review-gui-ux.md --rawfile diff pr.diff and posts to the
Anthropiс API), add a size check that computes DIFF_SIZE from pr.diff, compares
it to a MAX_DIFF_SIZE constant, and when over the limit truncates pr.diff into a
temp file (e.g., pr.diff.truncated), appends a short "[Diff truncated due to
size...]" note, and then use that truncated file as the --rawfile diff input (or
set DIFF_FILE variable and pass that to jq) so the constructed payload cannot
exceed the model token/context limits.
- Around line 66-74: The script uses a variable named "test" which shadows the
shell builtin test; rename this variable (e.g., to "is_test" or "tests_changed")
everywhere it's declared and referenced in the while-loop and after the loop,
and update any jq or downstream uses that check this variable's value
accordingly; keep the other flags (gui, config, api) unchanged and ensure all
occurrences of "test" in the file and in the jq command are replaced with the
new name to avoid ambiguity with the shell builtin.
- Around line 119-131: The current pipeline pipes the Anthropic response through
jq and overwrites comment.md without handling non-200 or error-shaped responses;
update the shell block that builds and posts the request (the curl + jq ->
comment.md and gh pr comment steps) to capture HTTP status and raw body, check
for HTTP errors or a JSON "type":"error" payload before extracting
.content[0].text, and write a meaningful diagnostic into comment.md on failure
(include raw response or error.message), then only call gh pr comment when a
valid review body is present; reference the existing curl invocation, the jq
extraction of .content[0].text, the temporary comment.md output, and the gh pr
comment invocation so the check is inserted between those steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: adcd8eff-31cc-4148-9ffc-c56661647ea0
📒 Files selected for processing (1)
.github/workflows/ai-persona-review.yml
…ona review workflow - hasLoadout(int) now enforces slot >= 1, preventing slot 0 and negatives from being treated as valid loadout slots - resolveLoadout name-matching loop now iterates loadouts.values() directly instead of calling getLoadout(slot), which was auto-creating empty Loadout objects for every slot just to check for display names - Add regression tests: slot 0, slot -1 -> NotFound; name resolution with no match leaves getCurrentLoadoutSlot() unchanged - Workflow: rename 'test' bash var to 'tests_changed' (shadows shell builtin) - Workflow: pull model name into job-level ANTHROPIC_MODEL env var - Workflow: add shared claude-helper.sh with explicit HTTP status and .content[0].text checks so failures surface as ::error:: annotations - Workflow: add diff truncation step (128 KB cap) before persona steps https://claude.ai/code/session_0137VbcTTRzaaw6Rc4zsZbh2
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.java (1)
129-130:⚠️ Potential issue | 🟠 MajorClamp resolution to configured slot bounds, not raw map contents.
hasLoadout()still treats any preexisting backing-map key as valid, and the name pass now iterates everyloadouts.values()entry without checkingLoadout#getLoadoutSlot(). If a player already has legacy slot0/-1data orMAX_LOADOUT_AMOUNTis lowered,resolveLoadout()can surface impossible name matches or tripSelectedLoadoutAboveMaxExceptionon numeric input.🔧 Minimal hardening
public boolean hasLoadout(int loadoutSlot) { - return loadoutSlot >= 1 && (loadouts.containsKey(loadoutSlot) || loadoutSlot <= getMaxLoadoutAmount()); + return loadoutSlot >= 1 && loadoutSlot <= getMaxLoadoutAmount(); } ... List<Loadout> namedLoadouts = new ArrayList<>(); for (Loadout loadout : loadouts.values()) { - if (loadout.getDisplay().getDisplayName().isPresent()) { + int slot = loadout.getLoadoutSlot(); + if (slot >= 1 && slot <= getMaxLoadoutAmount() + && loadout.getDisplay().getDisplayName().isPresent()) { namedLoadouts.add(loadout); } }Also applies to: 198-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.java` around lines 129 - 130, hasLoadout currently validates by checking the backing map keys directly which allows legacy/invalid slots (e.g., 0 or negative) and name-based resolution iterates loadouts.values() without verifying each Loadout#getLoadoutSlot(), causing impossible matches or SelectedLoadoutAboveMaxException when max slots change. Fix hasLoadout to ensure loadoutSlot is between 1 and getMaxLoadoutAmount() and that the resolved loadout itself (use getLoadout(loadoutSlot) or check loadouts.get(loadoutSlot) != null AND its getLoadoutSlot() is within bounds); update resolveLoadout/name-lookup (the method resolveLoadout) to only consider entries whose Loadout#getLoadoutSlot() is within 1..getMaxLoadoutAmount() (and reject legacy/negative slots) so name and numeric resolution are clamped to configured slot bounds.src/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java (1)
211-220:⚠️ Potential issue | 🟡 MinorThis regression still misses the old side effect.
The buggy lookup path materialized missing loadouts without touching
currentLoadout, so asserting onlygetCurrentLoadoutSlot()would still pass if that regression came back. Please assert the backing loadout collection is unchanged instead, e.g. by constructingLoadoutHolderwith a sharedMap<Integer, Loadout>and comparingsize()before/after the lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java` around lines 211 - 220, The test currently only asserts getCurrentLoadoutSlot(), which won't catch regressions that mutate the backing loadout collection; update the test to construct the LoadoutHolder with a shared Map<Integer, Loadout> (e.g. create a Map, pass it into the LoadoutHolder/constructor or factory used in the test), capture its size before calling holder.resolveLoadout("nonexistent"), then after asserting result is instance of LoadoutResolution.NotFound and that getCurrentLoadoutSlot() is unchanged, also assert the backing map's size is unchanged (compare size() before/after) to ensure no loadouts were added/removed by resolveLoadout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ai-persona-review.yml:
- Around line 60-65: Update the curl invocation that sets HTTP_CODE to be
resilient: replace the `-s` flag with `-sS` to surface curl errors, add
`--connect-timeout 10` and `--max-time 120` to impose connection and total
request timeouts, and keep the rest of the command that writes to response.json
and reads payload.json unchanged; additionally, set a job-level
`timeout-minutes: 10` in the GitHub Actions job to provide a workflow-level
backstop.
In `@src/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.java`:
- Around line 187-194: The current slot-first logic in LoadoutHolder (the
Integer.parseInt(input) block using hasLoadout and returning
LoadoutResolution.NotFound) prematurely returns NotFound when a numeric input
parses but the slot is invalid; change it so that after parsing the slot you
only return new LoadoutResolution.Found(getLoadout(slot)) when hasLoadout(slot)
is true, but do not return NotFound on a failed slot check—allow execution to
fall through to the subsequent exact and substring name matching logic (keep the
NumberFormatException catch as-is). Ensure references to hasLoadout, getLoadout,
and LoadoutResolution.Found/NotFound are preserved and used as described.
- Around line 240-243: The getPlainDisplayName method is directly calling
McRPG.getInstance().getMiniMessage().deserialize(...) which bypasses the
project's localization pipeline; change this to route the display name
conversion through the localization manager (use the localization
service/component used elsewhere) so that
Loadout->getDisplay()->getDisplayName() is passed into the LocalizationManager
(or equivalent) and then serialized with
PlainTextComponentSerializer.plainText().serialize(...) — update
getPlainDisplayName to call the localization manager's parse/resolve method
instead of McRPG.getInstance().getMiniMessage().deserialize to produce the plain
text string.
---
Duplicate comments:
In `@src/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.java`:
- Around line 129-130: hasLoadout currently validates by checking the backing
map keys directly which allows legacy/invalid slots (e.g., 0 or negative) and
name-based resolution iterates loadouts.values() without verifying each
Loadout#getLoadoutSlot(), causing impossible matches or
SelectedLoadoutAboveMaxException when max slots change. Fix hasLoadout to ensure
loadoutSlot is between 1 and getMaxLoadoutAmount() and that the resolved loadout
itself (use getLoadout(loadoutSlot) or check loadouts.get(loadoutSlot) != null
AND its getLoadoutSlot() is within bounds); update resolveLoadout/name-lookup
(the method resolveLoadout) to only consider entries whose
Loadout#getLoadoutSlot() is within 1..getMaxLoadoutAmount() (and reject
legacy/negative slots) so name and numeric resolution are clamped to configured
slot bounds.
In `@src/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java`:
- Around line 211-220: The test currently only asserts getCurrentLoadoutSlot(),
which won't catch regressions that mutate the backing loadout collection; update
the test to construct the LoadoutHolder with a shared Map<Integer, Loadout>
(e.g. create a Map, pass it into the LoadoutHolder/constructor or factory used
in the test), capture its size before calling
holder.resolveLoadout("nonexistent"), then after asserting result is instance of
LoadoutResolution.NotFound and that getCurrentLoadoutSlot() is unchanged, also
assert the backing map's size is unchanged (compare size() before/after) to
ensure no loadouts were added/removed by resolveLoadout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01f095eb-8635-44b8-b37e-67102b5fe139
📒 Files selected for processing (3)
.github/workflows/ai-persona-review.ymlsrc/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.javasrc/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java
…d curl resilience - resolveLoadout: remove premature NotFound after a numeric input fails the slot check; execution now falls through to name matching so loadouts whose display name happens to be a number (e.g. '5' when max is 3) are reachable - resolveLoadout name loop: filter loadouts.values() to only entries whose slot is within 1..getMaxLoadoutAmount(), guarding against stale map entries left behind when the configured max slot count is lowered - Add LoadoutHolder.getLoadedLoadoutCount() — returns loadouts.size() without auto-creating entries; used by tests to assert no side-effect mutation - Strengthen state-unchanged test to also assert getLoadedLoadoutCount() is unchanged, catching any future re-introduction of the auto-create side-effect - Add test: numeric input beyond max falls through and matches by name - Workflow: add timeout-minutes: 10 at job level as a backstop - Workflow curl: replace -s with -sS (surfaces transport errors), add --connect-timeout 10 and --max-time 120 to prevent indefinite hangs https://claude.ai/code/session_0137VbcTTRzaaw6Rc4zsZbh2
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ai-persona-review.yml:
- Around line 54-67: The jq payload construction can fail but the script always
proceeds to curl; after the jq invocation that writes payload.json (using
--rawfile prompt "$prompt_file" and pr.diff), check jq's exit status and that
payload.json is non-empty, and if either check fails log an error referencing
payload.json and the prompt_file and exit non-zero before running the curl that
sets HTTP_CODE; ensure the failure path returns a non-zero exit code so the
workflow fails fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40f4ea15-90a1-4765-9b45-b885fad81cff
📒 Files selected for processing (3)
.github/workflows/ai-persona-review.ymlsrc/main/java/us/eunoians/mcrpg/entity/holder/LoadoutHolder.javasrc/test/java/us/eunoians/mcrpg/loadout/LoadoutHolderResolutionTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement a flexible loadout resolution system that allows players to reference loadouts by slot number, exact name, or unique partial name match. This improves the user experience for loadout commands by reducing the need to remember exact slot numbers.
Key Changes
New
LoadoutResolutionsealed interface (LoadoutResolution.java): Represents the result of resolving a player's input string to a loadout with three outcomes:Found: A single loadout was unambiguously resolvedAmbiguous: Multiple loadouts matched the inputNotFound: No loadout matched the inputNew
resolveLoadout()method inLoadoutHolder: Implements a priority-based resolution chain:Ambiguousif multiple loadouts match at any stepUpdated
LoadoutEditCommandandLoadoutSetCommand:IntegerParsertoStringParserto accept flexible inputLoadoutResolutionto handle all three resolution outcomesLoadoutHolderLocalization updates (
en_commands.yml):ambiguous-matchesmessage with<loadout-matches>placeholderNew test suite (
LoadoutHolderResolutionTest.java): Comprehensive test coverage with 11 test cases covering:Notable Implementation Details
/loadout set 2always targets slot 2 even if a loadout name contains "2"Ambiguousresult includes the list of matching loadouts for potential future UI enhancementshttps://claude.ai/code/session_0137VbcTTRzaaw6Rc4zsZbh2
Summary by CodeRabbit
New Features
Documentation / Localization
Tests
Chores