feat(signore): v1.0.0 rewrite with CLI, upstream hook, and Sigil of Power#2282
Open
feat(signore): v1.0.0 rewrite with CLI, upstream hook, and Sigil of Power#2282
Conversation
…ower * Complete module encapsulation under Signore namespace *CLI parser with persistent (--enable/--disable) and session (--use/--no-use) overrides * Upstream hook for live command processing without restart * Add Sigil of Power (9718) with stamina-aware casting guard * Comprehensive YARD documentation
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/signore.lic`:
- Around line 612-616: The call to dothistimeout uses
Regexp.union(antimagic_pattern, success_pattern) so berserk_pattern is never
captured; update that call to include berserk_pattern in the union (e.g.,
Regexp.union(antimagic_pattern, success_pattern, berserk_pattern)) so the result
can match berserk_pattern and handle_cast_result can correctly detect and handle
berserk outcomes; apply the same change to the related call around lines 727-739
that mirrors this logic.
- Around line 73-74: The GUI_CLOSE_TIMEOUT default of 5 seconds is too short and
can prematurely close the setup window; update the default or remove the timeout
handling in wait_for_window_close: change GUI_CLOSE_TIMEOUT to a larger sensible
default (e.g., 0/ nil to disable timeout or 60 for a minute) in the definition
of GUI_CLOSE_TIMEOUT and modify wait_for_window_close to treat nil/<=0 as "no
timeout" (i.e., only exit on actual window close or script termination). Make
the same change where the timeout logic appears (references: GUI_CLOSE_TIMEOUT
and wait_for_window_close) so the code consistently respects a disabled or
extended timeout.
- Around line 539-543: The method power_active? uses in_main_thread { ...
}.result which returns a Try that may hold an exception (a truthy StandardError)
and thus falsely signal active; change the logic to inspect the Try's success
before using its value (e.g., check Try#success?/ok? or use pattern that returns
the Try's value only when successful and fall back to false otherwise) so
power_active? returns false on in_main_thread failures; update the code paths in
power_active? (and keep existing
temporary_override_active?/has_temporary_override? checks) to safely handle the
Try result from in_main_thread instead of unconditionally calling .result.
- Around line 545-550: toggle_power currently flips the temporary (override)
state because it calls power_active? which reflects overrides; change it to flip
the persisted/configured state instead — call or implement a helper that returns
the persisted state (e.g., persisted_power_active?(power) or read the persisted
value from the config) and pass the inverse of that to save_power_state; keep
save_power_state(power, ...) and only use power_active? for UI display/override
logic, not for deciding the saved toggle value.
- Around line 265-285: The argument parser branches for flags '--room-rm',
'--enable', '--disable', '--use', and '--no-use' currently read `@args`[index + 1]
without checking it; add a bounds check before using that element (e.g., verify
index + 1 < `@args.length` and `@args`[index + 1] is not nil/empty) and if missing
raise a clear error or exit (consistent with existing error handling) instead of
pushing nil into `@options` arrays or assigning it to `@options`[:room_rm]; update
the cases for `@options`[:room_rm], `@options`[:enable], `@options`[:disable],
`@options`[:use], and `@options`[:no_use] to perform this validation and return the
same index increments only when a valid value was consumed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba2e2e98-b411-440b-b1fb-3eac98921d67
📒 Files selected for processing (1)
scripts/signore.lic
…rsistence, berserk detection - Add bounds checking for CLI flags that consume a next argument (--room-rm, --enable, --disable, --use, --no-use) via onsume_next_arg - Change GUI_CLOSE_TIMEOUT to nil (disabled); wait_for_window_close now only exits on window close or script termination - Guard power_active? against truthy StandardError from failed Try - Add persisted_power_active? so toggle_power flips saved state, not temporary override state - Include berserk_pattern in dothistimeout Regexp.union so handle_cast_result can reach the berserk branch
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/signore.lic (1)
750-766: Consider explicit nil check fordothistimeouttimeout case.The
elsebranch correctly handles the timeout case (whenresultis nil), but an explicitnilcheck would be more self-documenting.♻️ Optional: Make timeout handling explicit
def handle_cast_result(result, _power) case result when success_pattern true when antimagic_pattern add_anti_magic_room(Room.current.id) when berserk_pattern wait_for_berserk_end + when nil + # Timeout or no match - wait and retry + wait_until { get } + sleep(1) else + # Unexpected output wait_until { get } sleep(1) end endBased on learnings: "when dothistimeout returns nil on timeout or failed to match, prefer checking result.nil? rather than result == false to distinguish timeout/unmatched cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/signore.lic` around lines 750 - 766, The method handle_cast_result currently uses an else branch to handle timeouts/unmatched results from dothistimeout; change that to an explicit nil check so timeouts are self-documenting: add a when result.nil? branch (before a final else for truly unmatched values if needed) that calls wait_until { get } and sleep(1), leaving the existing when success_pattern, when antimagic_pattern, and when berserk_pattern branches unchanged; reference handle_cast_result, dothistimeout, success_pattern, antimagic_pattern, berserk_pattern, wait_until and get when making the edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/signore.lic`:
- Around line 750-766: The method handle_cast_result currently uses an else
branch to handle timeouts/unmatched results from dothistimeout; change that to
an explicit nil check so timeouts are self-documenting: add a when result.nil?
branch (before a final else for truly unmatched values if needed) that calls
wait_until { get } and sleep(1), leaving the existing when success_pattern, when
antimagic_pattern, and when berserk_pattern branches unchanged; reference
handle_cast_result, dothistimeout, success_pattern, antimagic_pattern,
berserk_pattern, wait_until and get when making the edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dfd988b-f75f-4b80-918f-d3630a38a37a
📒 Files selected for processing (1)
scripts/signore.lic
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 by CodeRabbit
New Features
Refactor