refactor(egemhoarder): v2.0.0 address persistent bugs and clean up code#2317
Open
mrjeckel wants to merge 25 commits into
Open
refactor(egemhoarder): v2.0.0 address persistent bugs and clean up code#2317mrjeckel wants to merge 25 commits into
mrjeckel wants to merge 25 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors egemhoarder into GemHoarder: adds YAML persistence at DATA_DIR/.../gemhoarder.yaml, normalizes gem/jar matching, provides inventory/locker helpers, and rewrites deposit and raid workflows with a centralized command dispatcher. ChangesGemHoarder Module Refactor
Sequence Diagram(s)sequenceDiagram
participant Player
participant GemHoarder
participant Bin
participant Chest
participant Data
Player->>GemHoarder: deposit or raid command
GemHoarder->>Bin: open/read partial jars (fill phase)
GemHoarder->>Chest: open/read empty jars (start new jars / return empties)
GemHoarder->>Data: load/update persisted jar records (save_data)
GemHoarder->>Player: report deposited/raided/leftover
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/egemhoarder.lic`:
- Around line 302-306: The loop that calls open_locker_part for each slot (the
[:wardrobe, :bin, :chest] iteration) currently logs failures and continues,
which makes a partial scan appear successful; change the behavior so that if
open_locker_part returns false the loop immediately returns false (or sets an
overall failure flag and returns false after the loop) instead of using next,
and ensure the caller newlocker does not clear existing location data before
invoking this scan (or only clears after the scan succeeds); apply the same fix
to the similar block referenced around lines 359-361 so any compartment failure
causes the scan to fail rather than overwriting known-good data.
- Around line 335-343: The current logic in the block that searches data[:jars]
(using existing, gem_name, location, count, is_full) merges multiple physical
jars into one aggregated record; instead, stop collapsing matches — when adding
jars, do not update an existing entry's :count/:full; instead append one record
per physical jar (e.g., push count times a new hash with gem: gem_name, count:
1, full: is_full, location: location) so each physical jar is represented
individually and the downstream raid code won't lose inventory.
- Around line 674-687: The fput calls that use "put my ... in my #{sack.noun}"
are ambiguous when multiple containers share the same noun; update the two fput
invocations (the one after finding `dropped` and the one after finding `held`)
to target the resolved container by using `sack.id` instead of `sack.noun` so
the command references the specific object id; locate the fput calls in the
block handling the jar raid (the branch that decrements `jar_record[:count]` and
the `/free hand/` branch) and modify the command string to use the sack's id to
ensure deterministic stowing of raided gems.
- Around line 239-244: The user-supplied filter is interpolated directly into
/#{f}/ which lets regex metacharacters break or broaden matching; fix by
escaping the input with Regexp.escape before using it in a pattern: compute f =
Regexp.escape(filter.strip.downcase) (then apply your plural/ending
substitutions against that escaped string as needed, e.g. replace escaped
"ies\z"/"y\z"/"s\z" with the intended '(?:y|ies)'/'(?:y|ies)'/'s?' tokens), and
then build the regexp with Regexp.new(f) (or use /#{f}/ after escaping) in the
display.select! block that references f and display.select!.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5226b9c-d457-411d-bda9-415d5ec3cbce
📒 Files selected for processing (1)
scripts/egemhoarder.lic
mrjeckel
changed the title
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/egemhoarder.lic (3)
475-476:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSame-gem jars still aren't matched back to the correct persisted record.
The scan stores per-jar records, but deposit/raid later look records and physical jars up by gem name only. If a locker has two partial ruby jars, phase 1 can keep incrementing the first persisted record while filling the second physical jar, and
raidcan keep selecting the first physical jar while decrementing or deleting a different record. That will drift counts even though the scan itself is per-jar.This needs a real jar-to-record identity when mutating state, or a one-to-one rematch of the opened slot before any counts are updated.
Also applies to: 546-548, 773-777
🤖 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 `@scripts/egemhoarder.lic` around lines 475 - 476, The per-jar scan builds records with only gem name/count etc. (see new_records << { gem: gem_name, count: count, full: is_full, location: location }) but later mutation paths (deposit, raid) lookup by gem name, causing mismatches when a locker has multiple partial jars; change the scan to emit a stable jar identity (e.g., jar_id or slot_id) into each new_records entry and persist that id with the record, then update deposit and raid to match and mutate by that jar_id (or, alternatively, when an open slot is selected re-resolve the exact persisted record for that physical slot before adjusting counts) so mutations always target the exact jar record rather than the gem name only.
413-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
setcollapses multi-jar state back into one record.
delete_ifremoves every same-gem entry for the locker, then the command recreates a single hash with an aggregatecount. That breaks the one-record-per-physical-jar invariant the new scan/raid logic relies on, so;egemhoarder setcan corrupt lockers that legitimately have multiple jars for the same gem.At minimum, this path should refuse to rewrite a gem/location that already has more than one record instead of collapsing it.
🤖 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 `@scripts/egemhoarder.lic` around lines 413 - 420, The current `set` path collapses multiple same-gem entries at a location because it always deletes all matches then appends a single aggregated hash; to fix, before calling data[:jars].delete_if check how many existing entries match (e.g. entries = data[:jars].select { |j| clean_name(j[:gem]).casecmp?(clean_name(gem_name)) && j[:location] == loc }), and if entries.size > 1 and count > 0 then refuse to rewrite — emit an error/echo and return a nonzero status (or skip the write) instead of deleting and appending; otherwise continue with the current delete_if-and-append logic so single-entry updates still work.
341-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape the final search token before building
matcher.
wordsare escaped, butlastis still interpolated as regex syntax. Inputs likesearch ruby[can still raiseRegexpError, and./+/*will broaden matches unexpectedly.Suggested fix
- last = case last.downcase - when /ies\z/ then last.sub(/ies\z/i, '(?:y|ies)') - when /ves\z/ then last.sub(/ves\z/i, '(?:f|ves)') - when /es\z/ then last.sub(/es\z/i, '(?:es)?') - when /s\z/ then last.sub(/s\z/i, 's?') - when /y\z/ then last.sub(/y\z/i, '(?:y|ies)') - when /z\z/ then last + 'es?' - else last + 's?' + escaped_last = Regexp.escape(last) + last = case escaped_last.downcase + when /ies\\z/ then escaped_last.sub(/ies\\z/i, '(?:y|ies)') + when /ves\\z/ then escaped_last.sub(/ves\\z/i, '(?:f|ves)') + when /es\\z/ then escaped_last.sub(/es\\z/i, '(?:es)?') + when /s\\z/ then escaped_last.sub(/s\\z/i, 's?') + when /y\\z/ then escaped_last.sub(/y\\z/i, '(?:y|ies)') + when /z\\z/ then escaped_last + 'es?' + else escaped_last + 's?' end🤖 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 `@scripts/egemhoarder.lic` around lines 341 - 355, The code builds a regex `matcher` from words and a modified final token `last` but fails to escape `last`, allowing user input to inject regex metacharacters; fix by escaping the user token and then applying the pluralization alternations against the original to compute only the suffix replacement: capture the original `last` (e.g. last_orig = last), compute safe_last = Regexp.escape(last_orig), determine the suffix and replacement exactly as in the case branches (using last_orig to detect which branch applies), then produce the final `last` by replacing the escaped suffix in safe_last with the intended replacement (e.g. safe_last.sub(Regexp.escape(suffix), replacement) or append replacement when no suffix), and continue using that `last` when building `pattern` and `matcher` (so `words` remain escaped and `last` is now safe).
🤖 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 `@scripts/egemhoarder.lic`:
- Around line 803-860: The shake result handling can miss timeouts/unexpected
lines and still demote jars to bin; add a tracking flag (e.g., gem_removed or
empty_confirmed) that you set to true inside the branches where you decrement
jar_record[:count] or set it to 0 (the branches that match /fall into your/,
/free hand/, and /nothing falls out|realize that it is empty/), and do NOT clear
jar_record[:full] or move the jar to bin/chest unless one of those flags is
true; for unexpected or timed-out dothistimeout results, log/echo a warning and
skip changing :full or moving the jar so the jar stays in its original slot for
retry.
---
Duplicate comments:
In `@scripts/egemhoarder.lic`:
- Around line 475-476: The per-jar scan builds records with only gem name/count
etc. (see new_records << { gem: gem_name, count: count, full: is_full, location:
location }) but later mutation paths (deposit, raid) lookup by gem name, causing
mismatches when a locker has multiple partial jars; change the scan to emit a
stable jar identity (e.g., jar_id or slot_id) into each new_records entry and
persist that id with the record, then update deposit and raid to match and
mutate by that jar_id (or, alternatively, when an open slot is selected
re-resolve the exact persisted record for that physical slot before adjusting
counts) so mutations always target the exact jar record rather than the gem name
only.
- Around line 413-420: The current `set` path collapses multiple same-gem
entries at a location because it always deletes all matches then appends a
single aggregated hash; to fix, before calling data[:jars].delete_if check how
many existing entries match (e.g. entries = data[:jars].select { |j|
clean_name(j[:gem]).casecmp?(clean_name(gem_name)) && j[:location] == loc }),
and if entries.size > 1 and count > 0 then refuse to rewrite — emit an
error/echo and return a nonzero status (or skip the write) instead of deleting
and appending; otherwise continue with the current delete_if-and-append logic so
single-entry updates still work.
- Around line 341-355: The code builds a regex `matcher` from words and a
modified final token `last` but fails to escape `last`, allowing user input to
inject regex metacharacters; fix by escaping the user token and then applying
the pluralization alternations against the original to compute only the suffix
replacement: capture the original `last` (e.g. last_orig = last), compute
safe_last = Regexp.escape(last_orig), determine the suffix and replacement
exactly as in the case branches (using last_orig to detect which branch
applies), then produce the final `last` by replacing the escaped suffix in
safe_last with the intended replacement (e.g.
safe_last.sub(Regexp.escape(suffix), replacement) or append replacement when no
suffix), and continue using that `last` when building `pattern` and `matcher`
(so `words` remain escaped and `last` is now safe).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dafd6949-7700-4752-a2dd-ea483f7a9e52
📒 Files selected for processing (1)
scripts/egemhoarder.lic
mrhoribu
changed the title
renames module back to EGemHoarder and various echo statements to use that as well, ensuring no conflict with original gemhoarder module namescope.
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 by CodeRabbit
New Features
Improvements