[script][status-monitor] Refactor: bug fixes, SOLID decomposition, SQLite storage - #7402
Open
MahtraDR wants to merge 55 commits into
Open
[script][status-monitor] Refactor: bug fixes, SOLID decomposition, SQLite storage#7402MahtraDR wants to merge 55 commits into
MahtraDR wants to merge 55 commits into
Conversation
- Fix Gdk::RBGA typo that crashed health bar when HP < 75% - Fix Kernel#open -> File.open (command injection risk) - Fix file handle leaks in Marshal.load calls - Fix similarity_scrub mutating its argument - Fix @room_players unbounded growth (reset on room change) - Fix @filter_strings busy-wait (retry with backoff) - Fix double Slack notification - Remove duplicate @non_useful_tags entries - Decompose monolithic GameFilter into focused classes: MessageStore, MessageFilter, SpamDetector, AlertHandler, CommandDetector, Monitor - Replace Marshal .dat persistence with SQLite (WAL mode) with auto-migration from legacy .dat files - Update Slack API from removed register_slackbot to Lich::DragonRealms::SlackBot.direct_message - Add YARD documentation per dr-scripts style guide - Add status-monitor-import.lic for populating SQLite corpus from game session logs (handles .log and .log.gz, resumable) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds a CLI script to import and normalize character logs into per-character SQLite corpora, and refactors the status monitor into modular SQLite-backed storage, filtering, spam detection, alert handling, command detection, and runtime integration with comprehensive tests. ChangesLog Import Pipeline
StatusMonitor Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LogFiles
participant StatusMonitorImport
participant SQLite
LogFiles->>StatusMonitorImport: discover pending log files
StatusMonitorImport->>LogFiles: stream plain or gzip lines
StatusMonitorImport->>StatusMonitorImport: clean and similarity-scrub lines
StatusMonitorImport->>SQLite: batch INSERT OR IGNORE and record file
sequenceDiagram
participant GameStream
participant Monitor
participant MessageFilter
participant SpamDetector
participant MessageStore
GameStream->>Monitor: process raw line
Monitor->>MessageFilter: clean and filter line
Monitor->>SpamDetector: check processed line
Monitor->>MessageStore: check unseen line
Monitor-->>GameStream: return unseen state or queued alert
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
status-monitor.lic (1)
640-641:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the debug copy when the poll returns no line.
script.gets?can returnnilin GTK mode; withdebugenabled,nil.dupraises and kills the monitor on an idle poll.Suggested fix
- back = line.dup if args.debug + back = line.dup if args.debug && line🤖 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 `@status-monitor.lic` around lines 640 - 641, The code calls script.gets? and then unconditionally dup's it when args.debug is true, causing a crash if script.gets? returned nil; modify the logic around the call to script.gets? and the debug copy creation (the variables line, script.gets?, back, and args.debug) so you only call line.dup when line is non-nil (e.g., check line before duplicating or combine the conditions), ensuring back is only assigned from line.dup if both args.debug is true and line is truthy.
🤖 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 `@status-monitor-import.lic`:
- Line 216: The block argument idx from files.each_with_index is unused and
triggers RuboCop Lint/UnusedBlockArgument; change the iteration to avoid the
unused variable by either using files.each (replace files.each_with_index) or,
if the index is needed elsewhere, rename the unused arg to _idx to mark it
intentionally unused; update the call site referencing files.each_with_index
accordingly so no unused block arguments remain.
- Around line 159-165: flush_batch currently returns lines.size even though
INSERT OR IGNORE may skip duplicates; change flush_batch (the method that runs
db.transaction and executes the INSERT OR IGNORE loop) to compute and return the
actual number of rows inserted by summing the database's change count after each
execute (e.g., after each db.execute call call the DB change counter and add to
an inserted_count) or call an equivalent API (total_changes/delta) supported by
your sqlite adapter and return that inserted_count instead of lines.size.
- Around line 193-195: The --reset branch only deletes import_log
(db.execute('DELETE FROM import_log')) but leaves previously inserted rows (with
source = 'import') in the main tables because flush_batch uses INSERT OR IGNORE;
update the reset handling to fully remove or mark for reprocessing all rows
originating from imports by deleting rows where source = 'import' (or otherwise
truncating the affected tables) and/or change the import logic so reprocessing
can overwrite existing rows (remove INSERT OR IGNORE in flush_batch or replace
with INSERT OR REPLACE/UPSERT) so that rerunning after --reset will truly
rebuild the corpus and reapply scrubbing/cleaning logic.
- Around line 40-44: The timestamp skip regex in SKIP_PATTERNS is too permissive
and matches lines that only start with a timestamp before clean_line strips
prefixes; update the timestamp pattern in SKIP_PATTERNS to only match bare
timestamp-only lines (e.g. add an end anchor so /^\d{4}-\d{2}-\d{2}
\d{2}:\d{2}:\d{2}$/) so normal log lines with a timestamp prefix are not
dropped, or alternatively move the SKIP_PATTERNS check inside the clean_line
function to run after the prefix-stripping logic; reference SKIP_PATTERNS and
clean_line when making the change.
In `@status-monitor.lic`:
- Around line 560-569: The headless branch enters the infinite loop (nowindow)
before the shutdown hook is registered, so monitor.save never runs; move the
before_dying registration to run before entering the nowindow loop (register
before_dying { monitor.save } or equivalent), ensuring the same hook is set in
both the headless path and the normal path; update the code around nowindow and
the infinite loop that calls monitor.process, write_debug_log, and
monitor.consume_spam_line so the before_dying registration occurs prior to
entering the loop (also apply the same change to the analogous block around
lines 590-595).
- Around line 485-488: The code currently checks `@store.unseen`? before calling
`@detector.check`, so exact repeats never reach SpamDetector#check and
counts.values.max stays at 1; move the call to `@detector.check`(scrubbed) so
duplicates are fed into the detector before you short-circuit on
`@store.unseen`?(scrubbed), and then mark the scrubbed line as seen after
detector.check returns (i.e., call `@detector.check`(scrubbed) first, then return
false unless `@store.unseen`?(scrubbed), or alternatively always call
`@detector.check` and only skip further processing when unseen? is false) so the
repeat-counting branch in SpamDetector can increment properly.
- Around line 525-526: The lookup of filter strings must be nil-guarded so a nil
return from get_data('filters') doesn't raise and prevents the retry/backoff;
update the code that computes filters (the assignment that uses
get_data('filters') and the key 'filter_strings') to safely handle a nil data
value (e.g., use safe navigation or a nil-coalescing default) before calling map
so filters becomes nil or an empty list instead of raising, allowing the
surrounding retry loop to continue.
- Around line 417-433: The two scanners can emit the same command twice (e.g.,
"J_U_M_P")—modify the logic to deduplicate per input line: create a local
collection (e.g., sent = Set.new or array) and before calling
fput(normalized_command) check if the normalized command (use a consistent form
like downcase or upcase) is already in sent; if not, add it to sent and call
fput. Apply this check for both the uppercase-strip scanner (the block using
line.gsub(...).scan(/[A-Z\s]{3,}/) and its fput(match.downcase)) and the token
scanner (the block iterating line.split(' ').each and fput(cleaned.downcase)),
ensuring both use the same normalization and that the pause calls around fput in
the first scanner remain unchanged.
---
Outside diff comments:
In `@status-monitor.lic`:
- Around line 640-641: The code calls script.gets? and then unconditionally
dup's it when args.debug is true, causing a crash if script.gets? returned nil;
modify the logic around the call to script.gets? and the debug copy creation
(the variables line, script.gets?, back, and args.debug) so you only call
line.dup when line is non-nil (e.g., check line before duplicating or combine
the conditions), ensuring back is only assigned from line.dup if both args.debug
is true and line is truthy.
🪄 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: 39814e55-da3b-4174-af9d-e9c3a6e17729
📒 Files selected for processing (2)
status-monitor-import.licstatus-monitor.lic
- Add MessageStore#shutdown and Monitor#shutdown to explicitly close the SQLite connection after flushing pending data - Register before_dying with monitor.shutdown before the nowindow branch so both headless and GTK modes flush and close on exit - Rename line_hash column to line_text (stores full scrubbed text, not a hash digest) - Change import progress labels from "lines" to "lines scanned" since INSERT OR IGNORE skips duplicates silently Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove timestamp regex from import SKIP_PATTERNS -- it was matching all timestamped log lines, skipping ~85% of content before the TIMESTAMP_PATTERN strip could extract it - Reset now also deletes source='import' rows from seen_messages so re-import actually re-derives the corpus - Deduplicate CommandDetector matches with a Set before executing, preventing double-execution of obfuscated tokens like J_U_M_P that match both scanners - Run SpamDetector before the unseen? gate so repeated lines within the 10-minute recent window are still counted by the detector, restoring the upstream repeat-threshold behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused require 'fileutils' from import script - Add sync comments on duplicated similarity_scrub and schema between status-monitor.lic and status-monitor-import.lic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add 115 adversarial specs covering SpamDetector, MessageStore, CommandDetector, MessageFilter, Monitor, and StatusMonitorImport - Fix sqlite3 2.x compatibility: wrap bind params in arrays for execute() and get_first_value() calls (splat args removed in 2.x) - Specs found this real bug: all INSERT/SELECT with bind params would crash on sqlite3 >= 2.0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Re: out-of-diff comment about No fix needed. |
Collaborator
Author
|
@coderabbitai - update review on this pr to include all commits. |
Contributor
|
Triggering a full review now to cover all commits in the PR. ✏️ Learnings added
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/status_monitor_spec.rb (1)
619-623: 💤 Low valueUnused helper method
stub_get_data.This helper is defined but never called—all Monitor tests define
get_datainline viaObject.send(:define_method, ...)instead. Consider removing it to reduce confusion.🧹 Proposed removal
- def stub_get_data - define_method(:get_data) do |_type| - OpenStruct.new('filter_strings' => ['ignore_this']) - end - end - # get_data must be defined at top level for Monitor to call 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 `@spec/status_monitor_spec.rb` around lines 619 - 623, Remove the unused helper method stub_get_data (which defines get_data) since none of the Monitor tests call it and every test defines get_data inline via Object.send(:define_method,...); delete the stub_get_data method definition to avoid confusion or, if you prefer to keep a reusable helper, replace inline Object.send(:define_method, ...) usages with calls to stub_get_data so get_data is defined consistently across tests.
🤖 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 `@status-monitor-import.lic`:
- Around line 138-140: The similarity scrub currently checks only
scrubbed.empty?, so whitespace-only strings like " " pass through; update the
conditional around the result of similarity_scrub (the local variable scrubbed
used after calling similarity_scrub(cleaned)) to strip whitespace before
checking emptiness (e.g., use scrubbed.strip.empty? or strip! then empty?) so
whitespace-only rows are skipped and not inserted.
---
Nitpick comments:
In `@spec/status_monitor_spec.rb`:
- Around line 619-623: Remove the unused helper method stub_get_data (which
defines get_data) since none of the Monitor tests call it and every test defines
get_data inline via Object.send(:define_method,...); delete the stub_get_data
method definition to avoid confusion or, if you prefer to keep a reusable
helper, replace inline Object.send(:define_method, ...) usages with calls to
stub_get_data so get_data is defined consistently across tests.
🪄 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: 29e8fac7-474a-4ea7-b363-98c53ec02346
📒 Files selected for processing (4)
spec/status_monitor_import_spec.rbspec/status_monitor_spec.rbstatus-monitor-import.licstatus-monitor.lic
- Add .strip before .empty? check so whitespace-only strings left by currency/digit removal don't get inserted as noise rows. - Remove unused stub_get_data helper from spec (never called, also would not work correctly -- uses bare define_method instead of Object.send). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressing @CodeRabbit review (4307953260): status-monitor-import.lic:138-140 (whitespace after similarity_scrub) -- Fixed. spec/status_monitor_spec.rb:619-623 (unused Both fixes in 00b2f49. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Both addressed in If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Collaborator
Author
|
@coderabbitai review this pr again taking all commits into account together. |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
spec/status_monitor_spec.rb (1)
619-670: ⚡ Quick winAvoid redefining
Object#get_datain these examples.Each example adds and then removes a global
Objectmethod. If another spec/support file already definesget_data, this file will delete it inensureand make the suite order-dependent. StubStatusMonitor::Monitor#get_datainstead.Proposed refactor
RSpec.describe StatusMonitor::Monitor do let(:tmpdir) { Dir.mktmpdir('status-monitor-test') } + let(:filter_data) { OpenStruct.new('filter_strings' => []) } let(:settings) do OpenStruct.new( unique_line_threshold: 3, @@ before do $echo_messages.clear `@original_dir` = Dir.pwd Dir.chdir(tmpdir) + allow_any_instance_of(described_class) + .to receive(:get_data).with('filters').and_return(filter_data) end @@ it 'detector runs before unseen? gate (spam detection regression test)' do - # Define get_data in scope - Object.send(:define_method, :get_data) do |_type| - OpenStruct.new('filter_strings' => []) - end - monitor = described_class.new(settings) @@ - ensure - Object.send(:remove_method, :get_data) if Object.method_defined?(:get_data) 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 `@spec/status_monitor_spec.rb` around lines 619 - 670, These examples are defining and removing Object#get_data globally; instead stub the instance method on the monitor class (StatusMonitor::Monitor / described_class) — replace Object.send(:define_method, :get_data) { ... } and the ensure Object.remove_method calls with a local stub such as stubbing described_class (or monitor) to receive(:get_data).and_return(OpenStruct.new('filter_strings' => [])) before creating/using monitor (affecting examples that call monitor.process and monitor.consume_spam_line), so the tests no longer mutate Object and you can remove the ensure cleanup blocks.
🤖 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 `@status-monitor-import.lic`:
- Around line 33-36: The TIMESTAMP_PATTERN currently only matches prefixes
ending with ": " so bare timestamp-only headers (e.g., "2026-01-04 18:59:20.727
+13:00") survive cleaning and get into seen_messages; update TIMESTAMP_PATTERN
to also match standalone timestamps (no trailing colon/space) and ensure
clean_line uses this updated pattern to strip those headers before
similarity_scrub runs so timestamp-only lines are removed and never added to
seen_messages; reference TIMESTAMP_PATTERN, clean_line, similarity_scrub, and
seen_messages when making the change.
- Around line 217-224: Wrap the call to import_file (inside the
files.each_with_index loop that currently calls already_imported?, import_file,
and record_import) in a begin/rescue that catches Zlib::GzipFile::Error,
EOFError, and SystemCallError, logs the error with the file name, increments the
skipped/failed counter, and then nexts to continue the loop; keep record_import
only on successful imports and ensure any other exceptions still bubble (or are
handled separately) as intended.
In `@status-monitor.lic`:
- Around line 143-144: The backup path currently prepends "seen_messages_" to
File.basename(`@legacy_dat`, '.dat'), causing duplication because `@legacy_dat`
already includes that prefix; change the backup creation to not add the extra
prefix (e.g., build backup using "backup/#{File.basename(`@legacy_dat`,
'.dat')}.bak" or strip a leading "seen_messages_" from
File.basename(`@legacy_dat`, '.dat') before concatenation) and keep the
File.rename(backup, "#{backup}.migrated") if File.exist?(backup) logic unchanged
so the correct backup/seen_messages_<char>.bak file is migrated.
- Around line 510-518: After calling similarity_scrub and storing it in
scrubbed, skip processing when the scrubbed line collapses to only whitespace
(e.g. check scrubbed.strip.empty?), so do not call `@detector.check`,
`@alerter.fire`, or `@store.unseen`? and do not set `@spam_line` for that input;
implement this as a guard immediately after scrubbed =
`@filter.similarity_scrub`(cleaned) to return/next or wrap the downstream logic in
an unless scrubbed.strip.empty? block so whitespace-only tokens are dropped
before passing to SpamDetector and MessageStore.
- Line 20: The code calls Set.new in the CommandDetector.check method but never
requires Ruby's stdlib Set, causing NameError at runtime; add require 'set' near
the other requires (e.g., alongside require 'sqlite3') so Set is loaded before
CommandDetector.check invokes Set.new and any other Set use.
---
Nitpick comments:
In `@spec/status_monitor_spec.rb`:
- Around line 619-670: These examples are defining and removing Object#get_data
globally; instead stub the instance method on the monitor class
(StatusMonitor::Monitor / described_class) — replace Object.send(:define_method,
:get_data) { ... } and the ensure Object.remove_method calls with a local stub
such as stubbing described_class (or monitor) to
receive(:get_data).and_return(OpenStruct.new('filter_strings' => [])) before
creating/using monitor (affecting examples that call monitor.process and
monitor.consume_spam_line), so the tests no longer mutate Object and you can
remove the ensure cleanup blocks.
🪄 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: 697b315e-ed73-4dcb-b3e9-cc7be50c3d45
📒 Files selected for processing (4)
spec/status_monitor_import_spec.rbspec/status_monitor_spec.rbstatus-monitor-import.licstatus-monitor.lic
CodeRabbit fixes: - Skip bare timestamp session headers in import clean_line - Rescue corrupt/truncated .gz files in import loop, continue with remaining - Fix doubled seen_messages_ prefix in backup migration path - Guard against whitespace-only lines after similarity_scrub Hardening: - CommandDetector: strip non-ASCII before scanning (closes zero-width char evasion) - SpamDetector: replace 20-entry rolling buffer with time-windowed counts (entries expire after 90s, no fixed-size cap to flush) - MessageFilter: proper stream state tracking (like xmlparser.rb) for all filtered streams, not just percWindow. Fixes multi-line stream content leaking through and closes percWindow injection gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Import specs: - Bare timestamp edge cases (milliseconds-only, trailing content guard) - Corrupt .gz file: raises directly, skips in loop and continues - Truncated .gz file (EOFError path) - Pipeline: lines that clean but scrub to empty/whitespace are excluded Monitor specs: - Marshal migration with non-Hash data (Array) survives via rescue - Backup path correctness assertion (verifies no doubled prefix) - unseen? rejects tabs, newlines, mixed whitespace - Double shutdown is safe (before_dying can fire twice) - SpamDetector: time-windowed retention, buffer flooding resistance, expiry - SpamDetector: similarity percentage boundaries (0% and 100%) - CommandDetector: dot/hyphen separators, bare commands, embedded uppercase, Cyrillic evasion (known gap), zero-width evasion (now detected), mixed-case separators, long lines - MessageFilter: all filtered streams block content, non-filtered pass, percWindow injection no longer triggers from embedded text - AlertHandler: beeps, command execution, response/no-response, exit/no-exit - Monitor pipeline: scrub-to-whitespace returns false, filtered line returns false Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review commit 1553e94 and commit b2fa2d6 These two commits address all findings from your previous review: 1553e94 - Source fixes:
b2fa2d6 - Comprehensive edge case, boundary, and adversarial specs (36 new tests, 152 total) |
Contributor
|
✅ Actions performedFull review triggered. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/status_monitor_spec.rb (1)
249-261: 💤 Low valueUnused
let(:original_dir)definition.Line 250 defines
let(:original_dir)but thebefore/afterblocks use@original_dirinstance variable instead. Remove the unusedletto avoid confusion.Remove unused let
RSpec.describe StatusMonitor::MessageStore do let(:tmpdir) { Dir.mktmpdir('status-monitor-test') } - let(:original_dir) { Dir.pwd } before do $echo_messages.clear `@original_dir` = Dir.pwd🤖 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 `@spec/status_monitor_spec.rb` around lines 249 - 261, Remove the unused let definition let(:original_dir) from the spec; the setup/teardown use the `@original_dir` instance variable in the before and after blocks, so delete the redundant let(:original_dir) declaration to avoid confusion and keep only the `@original_dir` usage in before/after.spec/status_monitor_import_spec.rb (1)
24-24: ⚡ Quick winOrphaned temp directory at module load time.
LICH_DIRis created viaDir.mktmpdirwhen the spec file loads but is never cleaned up. This will accumulate orphaned directories across test runs. Consider using anafter(:all)hook or moving this into aletwith proper cleanup.Proposed fix using after(:all) cleanup
-LICH_DIR = Dir.mktmpdir('lich-test-import') unless defined?(LICH_DIR) +LICH_DIR = Dir.mktmpdir('lich-test-import') unless defined?(LICH_DIR) + +RSpec.configure do |config| + config.after(:suite) do + FileUtils.rm_rf(LICH_DIR) if defined?(LICH_DIR) && File.exist?(LICH_DIR) + end +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 `@spec/status_monitor_import_spec.rb` at line 24, LICH_DIR is being created at module load with Dir.mktmpdir and never removed, leaving orphaned temp dirs; change this so LICH_DIR is created in a test lifecycle hook (e.g., before(:all)) or as a memoized let and add an after(:all) that calls FileUtils.remove_entry_secure(LICH_DIR) (ensure FileUtils is required), or if using let add an around hook to ensure cleanup; update references to use the new LICH_DIR initialization point and remove the top-level Dir.mktmpdir call.
🤖 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.
Nitpick comments:
In `@spec/status_monitor_import_spec.rb`:
- Line 24: LICH_DIR is being created at module load with Dir.mktmpdir and never
removed, leaving orphaned temp dirs; change this so LICH_DIR is created in a
test lifecycle hook (e.g., before(:all)) or as a memoized let and add an
after(:all) that calls FileUtils.remove_entry_secure(LICH_DIR) (ensure FileUtils
is required), or if using let add an around hook to ensure cleanup; update
references to use the new LICH_DIR initialization point and remove the top-level
Dir.mktmpdir call.
In `@spec/status_monitor_spec.rb`:
- Around line 249-261: Remove the unused let definition let(:original_dir) from
the spec; the setup/teardown use the `@original_dir` instance variable in the
before and after blocks, so delete the redundant let(:original_dir) declaration
to avoid confusion and keep only the `@original_dir` usage in before/after.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 090c5e53-cd52-4a86-a40b-9da59c56ca19
📒 Files selected for processing (4)
spec/status_monitor_import_spec.rbspec/status_monitor_spec.rbstatus-monitor-import.licstatus-monitor.lic
🚧 Files skipped from review as they are similar to previous changes (2)
- status-monitor-import.lic
- status-monitor.lic
- Remove unused `let(:original_dir)` in status_monitor_spec.rb - Add after(:suite) hook to clean up LICH_DIR temp directory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai Both nitpicks from your review are fixed in 509d174:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Thanks for the thorough follow-through on all the review findings! 🐇✨ |
AlertHandler#send_slack gated delivery on `@slackbot&.initialized?`. That gate is wrong: SlackBot#direct_message already self-heals via `reconnect! unless initialized?` (see lib/dragonrealms/commons/slackbot.rb). By returning early whenever the bot was not initialized, we guaranteed the reconnect path could never run. Concrete failure: status-monitor is commonly auto-started at login. On a session with no valid cached UserVars.slack_token, SlackBot.new needs lnet, which is frequently not connected yet, so the constructor returns with @initialized == false (it does not raise). From that point send_slack returned early for the entire session and NO alert was ever delivered, even though direct_message would have reconnected on the first real alert. The pre-refactor code did not have this bug: send_slackbot_message in dependency.lic only checked that the instance existed, then let direct_message handle (re)connection. Fix: guard on the presence of @slackbot, not on initialized?, so the self-healing reconnect inside direct_message is reached on the first alert. Adds a regression test (with a network-free fake SlackBot) proving delivery happens when the bot reports initialized? == false. Verified the test fails against the old gated code and passes with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AlertHandler previously built the SlackBot in its constructor via init_slackbot, and AlertHandler.new runs inside Monitor.new at script startup (before the GTK window is created). SlackBot.new can block for several seconds: it may start lnet, wait up to 30s for the lnet connection, fetch the Slack users list, and sleep on randomized jitter/backoff. That delayed the status window appearing on every launch. Defer construction to the first alert: the window comes up immediately, and by the time a spam alert actually fires, lnet is far more likely to be connected (which also improves the odds the very first send succeeds). - Replaces init_slackbot with a memoized private #slackbot builder. - Precomputes @slack_enabled from slack_username so send_slack short-circuits cheaply when Slack is not configured. - Construction failures are swallowed (logged) and retried on the next alert, matching direct_message's own resilience. Tests: assert no SlackBot is constructed at initialize time, none is constructed without a username, and exactly one is constructed and reused across multiple alerts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
find_log_files hardcoded the "DR-" prefix on the per-character log directory, so it silently ignored logs from any other instance. Lich names these directories "<GAMECODE>-<Character>", and real installs contain DR-, DRT- (test), DRX- (platinum) and GSIV- directories side by side. A character played on more than one instance had all non-prime logs skipped, with only a misleading "Log directory not found" message. The live seen-messages database is scoped by character only, not by instance, so the corpus already blends instances. Match that by globbing every "*-<Character>" directory and importing them all. The glob is anchored on "-<Character>", so it will not match a different character whose name is a superstring (e.g. importing "Zz" does not pull in "DR-Zzextra"). Tests: multi-instance discovery, .log + .log.gz inclusion, superstring non-match, and the empty-result path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CommandDetector evasion tests embedded a literal Cyrillic capital Em and a literal zero-width space, which trip the repo's Custom/AsciiOnlySource cop (2 offenses). That cop also rejects \uXXXX escapes that resolve to non-ASCII, so escaping the literals is not an option either. Build the offending characters at runtime from their code points via Integer#chr(Encoding::UTF_8). The source file is now pure ASCII, the runtime test inputs are byte-for-byte identical to before, and the assertions (Cyrillic lookalike is missed, zero-width space is tolerated) are unchanged. rubocop: 4 files inspected, no offenses detected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clean stripped XML tags with line.gsub!(...), mutating the caller's string. This was inconsistent with the similarity_scrub mutation fix already made in this refactor, and it would raise FrozenError if the game stream ever handed us a frozen line. The only thing relying on that mutation was the debug logging path: because clean rewrote `line` in place, the loops could log both the raw line (a pre-cleaned dup) and the cleaned line from the same variable. Removing the mutation would have made those two logged values identical. Changes: - clean now returns a new String via gsub (no bang), never mutating input. - Monitor exposes #last_clean_line, set on every process call, so the debug path has an explicit handle on the cleaned text. - Both run loops drop the `back = line.dup` dance and log (raw_line, last_clean_line, last_player_line). The GTK idle-pause guard is rewritten from `unless back` to the equivalent `unless args.debug && line`, preserving the exact prior behavior (pause when not in debug, or on nil lines even in debug). Tests: clean does not mutate its argument, and clean does not raise on a frozen input line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
run() sliced the file list with `files.first(limit)` BEFORE filtering out already-imported files. On a resumed run, the first N files are typically the ones already imported, so `--limit=N` could select an all-imported slice and import zero new files while reporting "N skipped" -- no forward progress. Extract selection into pending_files(db, files, limit:), which filters out already-imported files first and only then applies the limit. run() now iterates the pending set directly (no in-loop already_imported? check), and the progress/summary math counts against the pending total. Added an explicit "all already imported, nothing to do" short-circuit. reset still works unchanged: it clears import_log first, so every file is pending again. Tests: pending_files excludes imported files, applies the limit to the pending set (regression), returns all when unlimited, and returns [] when everything is imported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…TL constant Two small documentation/clarity gaps from the review: - MessageFilter#last_player_line had no YARD @return; added one. - The 10-minute recent-cache window was a bare `600` literal in migrate_recent while the class doc described it as "10 minutes". Introduced MessageStore::RECENT_TTL_SECONDS = 600 and referenced it from both the code and the doc so the number and its description cannot drift apart. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cution On a spam alert, CommandDetector auto-executes any VALID_COMMAND embedded in the suspicious line. VALID_COMMANDS includes destructive/session-ending verbs (QUIT, SELL, DROP, DUMP, EMPTY, WITHDRAW, RELEASE, GET, PUT, KILL). Empirically the scanner only ever fires a BARE verb -- targets never survive scanning (an all-uppercase phrase concatenates into an invalid token, and obfuscated tokens reduce to the verb alone). So item verbs are effectively no-ops today (bare "sell"/"drop" just prompt), and QUIT is the one match with a real effect: a forced logout in response to a probe, which has no upside. Add DENYLISTED_COMMANDS and skip them at execution time. This neutralizes the QUIT logout vector now, and keeps the item verbs safe if the scanner is ever extended to attach arguments. A legitimate GM sanity-probe never asks you to run these, so no useful behavior is lost. Tests: denylisted verbs (plain and obfuscated) never execute, and a safe verb in the same line as a denylisted one still executes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review this PR in full |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spec/status_monitor_spec.rb (1)
929-1006: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
get_datastub/teardown.The
Object.send(:define_method, :get_data) {...} ... ensure Object.send(:remove_method, :get_data) ...pattern is duplicated identically across all 5 examples in this describe block. Anaround(:each)hook (or astub_get_datahelper) would remove the repetition.♻️ Suggested refactor
+ around do |example| + Object.send(:define_method, :get_data) { |_type| OpenStruct.new('filter_strings' => []) } + example.run + ensure + Object.send(:remove_method, :get_data) if Object.method_defined?(:get_data) + end + it 'detector runs before unseen? gate (spam detection regression test)' do - Object.send(:define_method, :get_data) do |_type| - OpenStruct.new('filter_strings' => []) - end - monitor = described_class.new(settings) 4.times { monitor.process(+'A mysterious voice whispers to you') } expect(monitor.spam_line).not_to be_nil, "SpamDetector should have fired after 4 repeats with threshold 3" - ensure - Object.send(:remove_method, :get_data) if Object.method_defined?(:get_data) end(the 'returns false for lines matching a filter pattern' example would need its own inline override since it uses non-empty
filter_strings)🤖 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 `@spec/status_monitor_spec.rb` around lines 929 - 1006, Extract the duplicated top-level get_data definition and cleanup from the five examples into a shared around(:each) hook or helper within this describe block. Preserve the default empty filter_strings behavior for the shared setup, while keeping the “returns false for lines matching a filter pattern” example’s non-empty filter_strings override local and ensuring get_data is always removed after each example.
🤖 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 `@status-monitor-import.lic`:
- Around line 50-70: Update the shared connection setup in open_database to set
db.busy_timeout to 5000 immediately after creating the SQLite3::Database
instance, ensuring concurrent writes wait through transient lock contention
before continuing with PRAGMA and table initialization.
In `@status-monitor.lic`:
- Around line 383-457: Reorder the calls in AlertHandler#fire so fput('exit')
executes before send_slack(counts) when `@quit_on_flag` is enabled. Keep the
existing Slack gating and response behavior unchanged, ensuring the potentially
blocking SlackBot initialization cannot delay the auto-quit path.
---
Nitpick comments:
In `@spec/status_monitor_spec.rb`:
- Around line 929-1006: Extract the duplicated top-level get_data definition and
cleanup from the five examples into a shared around(:each) hook or helper within
this describe block. Preserve the default empty filter_strings behavior for the
shared setup, while keeping the “returns false for lines matching a filter
pattern” example’s non-empty filter_strings override local and ensuring get_data
is always removed after each example.
🪄 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: ee621cac-556e-4690-bda6-f3f3d062f7ea
📒 Files selected for processing (4)
spec/status_monitor_import_spec.rbspec/status_monitor_spec.rbstatus-monitor-import.licstatus-monitor.lic
Comment on lines
+50
to
+70
| def self.open_database(character_name) | ||
| db_path = "seen_messages_#{character_name}.db" | ||
| db = SQLite3::Database.new(db_path) | ||
| db.execute('PRAGMA journal_mode=WAL') | ||
| db.execute('PRAGMA synchronous=NORMAL') | ||
| db.execute(<<~SQL) | ||
| CREATE TABLE IF NOT EXISTS seen_messages ( | ||
| line_text TEXT PRIMARY KEY, | ||
| first_seen_at DATETIME DEFAULT CURRENT_TIMESTAMP, | ||
| source TEXT DEFAULT 'live' | ||
| ) | ||
| SQL | ||
| db.execute(<<~SQL) | ||
| CREATE TABLE IF NOT EXISTS import_log ( | ||
| file_path TEXT PRIMARY KEY, | ||
| lines_imported INTEGER, | ||
| imported_at DATETIME DEFAULT CURRENT_TIMESTAMP | ||
| ) | ||
| SQL | ||
| db | ||
| end |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the importer and live monitor files
fd -a 'status-monitor-import\.lic|status-monitor\.lic' .
# Map the relevant file structure first
for f in $(fd -a 'status-monitor-import\.lic|status-monitor\.lic' .); do
echo "### OUTLINE: $f"
ast-grep outline "$f" --view expanded || true
doneRepository: elanthia-online/dr-scripts
Length of output: 384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant regions with line numbers
for f in $(fd -a 'status-monitor-import\.lic|status-monitor\.lic' .); do
echo "### FILE: $f"
wc -l "$f"
done
# Try to locate database open/setup code and lock/error handling
rg -n -C 3 "busy_timeout|journal_mode|synchronous|SQLite3::BusyException|open_database|import_log|seen_messages" .Repository: elanthia-online/dr-scripts
Length of output: 29465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sqlite3
print(sqlite3.sqlite_version)
PYRepository: elanthia-online/dr-scripts
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact open_database implementations and nearby error handling
for f in $(fd -a 'status-monitor-import\.lic|status-monitor\.lic' .); do
echo "### $f"
rg -n -C 4 'def self\.open_database|busy_timeout|journal_mode|synchronous|rescue|BusyException|open_database' "$f" || true
echo
doneRepository: elanthia-online/dr-scripts
Length of output: 3537
Add a busy timeout to the shared SQLite connection.
status-monitor-import.lic and status-monitor.lic both write to seen_messages_<character>.db, but neither sets busy_timeout. Concurrent live writes can raise SQLite3::BusyException, and run doesn’t rescue it, so the import can abort mid-run. db.busy_timeout = 5000 would avoid failing fast on transient lock contention.
🤖 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 `@status-monitor-import.lic` around lines 50 - 70, Update the shared connection
setup in open_database to set db.busy_timeout to 5000 immediately after creating
the SQLite3::Database instance, ensuring concurrent writes wait through
transient lock contention before continuing with PRAGMA and table
initialization.
Comment on lines
+383
to
457
| # Handles alert responses when spam is detected: audio beeps, | ||
| # optional auto-response, Slack notification, and optional auto-quit. | ||
| # | ||
| # @note Configurable via YAML settings: | ||
| # - `status_monitor_respond` [Boolean] whether to auto-respond | ||
| # - `quit_on_status_warning` [Boolean] whether to auto-quit | ||
| # - `slack_username` [String] Slack user to notify | ||
| class AlertHandler | ||
| # @param settings [OpenStruct] user settings from get_settings | ||
| def initialize(settings) | ||
| @settings = settings | ||
| @responses = ["'Hmmm?", "'Yes", "'Ok?"].shuffle | ||
| @quit_on_flag = settings.quit_on_status_warning | ||
| @slack_enabled = !@settings.slack_username.nil? && !@settings.slack_username.to_s.strip.empty? | ||
| @slackbot = nil | ||
| end | ||
|
|
||
| # Fires an alert: beeps, auto-responds, notifies Slack, optionally quits. | ||
| # | ||
| # @param line [String] the suspicious line that triggered the alert | ||
| # @param counts [String] debug info about detection counts | ||
| # @return [void] | ||
| def fire(line, counts) | ||
| 3.times do | ||
| echo("\a") | ||
| pause 0.25 | ||
| end | ||
| CommandDetector.check(line) | ||
| fput @responses.first if @settings.status_monitor_respond | ||
| echo(line) | ||
| send_slack(counts) | ||
| fput('exit') if @quit_on_flag | ||
| @responses.rotate! | ||
| pause 2 | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Lazily constructs the SlackBot on first use. | ||
| # | ||
| # SlackBot.new can block for several seconds (lnet startup, users.list | ||
| # fetch, jitter/backoff). Building it here rather than in the constructor | ||
| # keeps script startup and the status window responsive, and defers the | ||
| # connection until an alert actually needs to be sent (by which point lnet | ||
| # is far more likely to be up). | ||
| # | ||
| # @return [Lich::DragonRealms::SlackBot, nil] the bot, or nil if construction failed | ||
| def slackbot | ||
| return @slackbot if @slackbot | ||
|
|
||
| @slackbot = Lich::DragonRealms::SlackBot.new | ||
| rescue => e | ||
| echo "SlackBot init failed: #{e.message}" | ||
| nil | ||
| end | ||
|
|
||
| # @param message [String] message body to send | ||
| # @return [void] | ||
| # | ||
| # @note Delegates the connection check to SlackBot#direct_message, which | ||
| # reconnects itself when not yet initialized. Gating on #initialized? | ||
| # here would permanently suppress delivery whenever the first connection | ||
| # attempt failed (e.g. lnet not up yet at login), because the self-healing | ||
| # reconnect path inside direct_message would never be reached. | ||
| def send_slack(message) | ||
| return unless @slack_enabled | ||
|
|
||
| bot = slackbot | ||
| return unless bot | ||
|
|
||
| bot.direct_message(@settings.slack_username, message.to_s) | ||
| rescue => e | ||
| echo "SlackBot error: #{e.message}" | ||
| end | ||
| lev_array[source_length][compare_length] | ||
| end |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant area with line numbers.
sed -n '383,457p' status-monitor.lic | cat -n
printf '\n---\n'
# Find the SlackBot definition and any direct_message docs/usages.
rg -n "class SlackBot|def direct_message|SlackBot.new|direct_message\(" status-monitor.licRepository: elanthia-online/dr-scripts
Length of output: 3609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate quit handling and any cleanup that depends on the alert path.
rg -n "quit_on_status_warning|fput\\('exit'\\)|fput\\(\"exit\"\\)|def fput\\b|\\bexit\\b" status-monitor.lic
printf '\n---\n'
# Show nearby code around any additional quit-related logic.
sed -n '300,380p' status-monitor.lic | cat -nRepository: elanthia-online/dr-scripts
Length of output: 3672
Move send_slack after fput('exit').
SlackBot.new is documented as potentially blocking for several seconds, so sending Slack before the auto-quit path delays the safety-critical exit on the first alert.
🤖 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 `@status-monitor.lic` around lines 383 - 457, Reorder the calls in
AlertHandler#fire so fput('exit') executes before send_slack(counts) when
`@quit_on_flag` is enabled. Keep the existing Slack gating and response behavior
unchanged, ensuring the potentially blocking SlackBot initialization cannot
delay the auto-quit path.
… alert
Two robustness fixes from PR review:
- open_database (both StatusMonitorImport and MessageStore, kept in sync) now
sets db.busy_timeout = 5000 immediately after opening the connection.
Default busy_timeout is 0, so a concurrent writer -- e.g. a running
status-monitor and a status-monitor-import both touching the same
per-character db -- would raise SQLITE_BUSY on transient lock contention.
Waiting up to 5s lets the transient lock clear.
- AlertHandler#fire now queues fput('exit') before send_slack(counts) when
quit_on_status_warning is set. Because the SlackBot is constructed lazily on
the first alert and that can block for seconds (lnet/users.list), sending
Slack first could delay the auto-quit. The exit command is queued to the
game, so the Slack notification still goes out immediately afterward.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…itor specs - Assert PRAGMA busy_timeout == 5000 on both open_database connections. - Assert the auto-quit is queued before the Slack send in AlertHandler#fire, via a shared $event_log recording both fput and Slack sends in order. Verified this fails against the old (slack-first) ordering. - Extract the duplicated top-level get_data define/remove from the Monitor examples into a single around(:each) hook (default empty filter_strings, always removed in ensure). The one example needing a non-empty filter redefines get_data locally before building the Monitor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
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
1 participant
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
Refactors status-monitor.lic from a monolithic
GameFilterclass into focused, testable modules and fixes several bugs in the existing implementation.Bug fixes
death,percWindowentriesStructural changes
GameFilterinto focused classes:MessageStore,MessageFilter,SpamDetector,AlertHandler,CommandDetector,Monitor.datpersistence with SQLite (WAL mode) for the seen-messages corpus.datfiles on first run, then renames to.dat.migratedregister_slackbot/send_slackbot_messageglobals to a dedicated, lazily-constructedLich::DragonRealms::SlackBotinstance (delivery delegates reconnection todirect_message)New script
status-monitor-import.licfor bulk-importing game session logs (.logand.log.gz) into the SQLite corpus. Resumable, tracks processed files.Review follow-ups
AlertHandler#send_slackno longer gates oninitialized?; that gate permanently suppressed delivery whenever the first connection attempt failed (e.g. lnet not up at login), becausedirect_message's own reconnect path was never reachedSlackBotis constructed on the first alert instead of at script start, so the status window is no longer blocked behind lnet/users.list startupfind_log_filesglobs every*-<Character>log directory (DR/DRT/DRX/GSIV) instead of hardcoding theDR-prefix--limitsemantics -- the limit now caps not-yet-imported files, so a resumed limited run makes forward progress instead of re-selecting already-imported filesMessageFilter#clean-- no longer mutates its argument (tag stripping returns a new String; safe against frozen input)CommandDetectorlast_player_line, named the recent-cache TTL constant, and kept the adversarial spec inputs ASCII-only so the repo cops passTest plan
;status-monitoron a character with an existing.datfile -- verify auto-migration to SQLite and.dat.migratedrename;status-monitoron a fresh character with no prior data -- verify clean SQLite database creation;status-monitor-importto populate corpus from logs, then re-run to verify resumability--limit=Nimports N not-yet-imported files on a resumed run--resetflag on import clears tracking and re-processes all files🤖 Generated with Claude Code
Summary by CodeRabbit
--reset, and progress/completion reporting.MessageStore, improved message cleaning/similarity normalization, spam alerting, and embedded command detection/execution.