feat(version.lic): v1.3.0 add stale session tracking#2265
Conversation
Updated version to 1.3.0 and added new features including session snapshot integration and simplified reporting.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds SessionsSettings-driven session snapshot integration, per-session normalization and health metrics (stale/duplicate/active), recommendation generation, autostart script loader, expanded report/detail rendering, and safer StandardError-based error handling in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Invoker
participant VS as VersionScript
participant SS as SessionsSettings
participant FS as Filesystem
participant R as Reporter
CLI->>VS: invoke report
VS->>SS: request session snapshot (if feature enabled)
SS-->>VS: snapshot or error
VS->>VS: normalize snapshot (or stub) and compute metrics (active, stale, duplicate)
VS->>FS: load_autostart_scripts(scope)
FS-->>VS: discovered scripts list
VS->>R: assemble summary, recommendation, and (detail mode) per-session diagnostics
R-->>CLI: render output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
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 `@scripts/version.lic`:
- Around line 331-335: session_summary currently sets total: active_sessions
which mislabels the total count; update the session_summary assignment (symbol:
session_summary) so that total uses the real total value (session_state[:total])
instead of active_sessions, and keep stale and issue logic the same (stale:
stale_sessions, issue: stale_sessions.positive? ||
duplicate_sessions.positive?); alternatively, if the intent is to report active
sessions, change the external label that prints the value (where it reads "Total
ruby sessions") to "Active ruby sessions" to match active_sessions—pick the
correct semantics and make the corresponding change consistently (references:
session_summary, active_sessions, session_state[:total],
session_recommendation).
- Around line 265-272: active_session_count and the detail rendering assume
symbol keys; update them to tolerate string keys by normalizing or
dual-accessing keys: use Array(session_state[:sessions] ||
session_state['sessions']) in active_session_count, and for each session (s)
check marker via s[:marker] || s['marker']; if marker exists compare to
'active'; otherwise read state via s[:state] || s['state'] and stale via
s[:stale] || s['stale'] to decide active-ness; apply the same normalization
pattern to the detail output code paths referenced around lines 468-473 so
`"marker"=>"stale"`/`"state"=>"exited"` are handled correctly.
- Around line 199-205: The potential_stale_sessions_count method currently
returns session_state[:stale].to_i whenever the :stale key exists (via
session_state.key?(:stale)), but normalization always injects that key so the
row-level fallback never runs; change the guard to detect a real value instead
of mere presence — replace the session_state.key?(:stale) check with a nil-check
(e.g. return session_state[:stale].to_i if !session_state[:stale].nil?) so that
when :stale is not set (nil) the method falls back to iterating
Array(session_state[:sessions]) and applying the marker/heartbeat_age logic.
- Around line 176-179: The current mapping keeps non-Hash entries by returning
them from the block; change the normalization to drop malformed rows by first
filtering to only Hashes and then mapping: replace the
Array(snapshot[:sessions]).map { |session| next session unless
session.is_a?(Hash); ... } logic with a two-step filter-then-map (e.g.
Array(snapshot[:sessions]).select { |s| s.is_a?(Hash) }.map { |session|
session.key?(:game_code) ? session : session.merge(game_code: session[:game]) })
so only Hash session rows are kept and malformed entries are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 634bdf4f-614f-4e35-b04a-bc9e4fc84715
📒 Files selected for processing (1)
scripts/version.lic
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
## What was fixed: - Dropped malformed non-Hash session rows during snapshot normalization. - Made stale-count fallback reachable by switching from key?(:stale) to nil-aware value checks. - Added symbol/string key tolerance in: -- active session counting -- stale fallback logic -- detail rendering rows - Kept active-session semantics and relabeled output from Total ruby sessions to Active ruby sessions for consistency.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/version.lic (1)
238-239: Consider adding string-key fallback for consistency.Line 239 accesses
session_state[:sessions]without a string-key fallback, unlikepotential_stale_sessions_count(line 208) andactive_session_count(line 276) which usesession_state[:sessions] || session_state['sessions'].While this works correctly because
normalize_session_snapshotalways outputs symbol keys, adding the fallback would maintain defensive consistency across all methods.♻️ Suggested fix
def self.duplicate_live_session_count(session_state) - live_sessions = Array(session_state[:sessions]).select { |s| session_os_seen?(s) == true } + live_sessions = Array(session_state[:sessions] || session_state['sessions']).select { |s| session_os_seen?(s) == true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/version.lic` around lines 238 - 239, The method duplicate_live_session_count currently accesses session_state[:sessions] without a string-key fallback; change the retrieval to mirror the other helpers (e.g., use session_state[:sessions] || session_state['sessions'] wrapped in Array(...)) so it defensively handles both symbol and string keys in duplicate_live_session_count, leaving the rest of the logic (including the session_os_seen? predicate) unchanged.
🤖 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/version.lic`:
- Around line 238-239: The method duplicate_live_session_count currently
accesses session_state[:sessions] without a string-key fallback; change the
retrieval to mirror the other helpers (e.g., use session_state[:sessions] ||
session_state['sessions'] wrapped in Array(...)) so it defensively handles both
symbol and string keys in duplicate_live_session_count, leaving the rest of the
logic (including the session_os_seen? predicate) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bef05c7b-f142-4b34-95d3-47050f6f5b6c
📒 Files selected for processing (1)
scripts/version.lic
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/version.lic (1)
163-178:⚠️ Potential issue | 🟠 MajorPreserve
stale=nilwhen the snapshot omits the top-level stale count.This still normalizes “missing” to
0, sopotential_stale_sessions_countnever reaches the row-level marker/heartbeat fallback. On cores that only populate per-session stale data, the new summary will always report0stale sessions. This hunk is also the RuboCop failure from CI.Suggested fix
def self.normalize_session_snapshot(snapshot) base = stub_session_snapshot return base unless snapshot.is_a?(Hash) + stale = snapshot[:stale] + stale = snapshot['stale'] if stale.nil? base.merge( supported: true, source: snapshot[:source] || snapshot['source'] || 'SessionSettings', total: (snapshot[:total] || snapshot['total']).to_i, idle_over_30m: (snapshot[:idle_over_30m] || snapshot['idle_over_30m']).to_i, - stale: (snapshot[:stale] || snapshot['stale']).to_i, + stale: stale.nil? ? nil : stale.to_i, running: (snapshot[:running] || snapshot['running']).to_i, sleeping: (snapshot[:sleeping] || snapshot['sleeping']).to_i, hidden: (snapshot[:hidden] || snapshot['hidden']).to_i, sessions: Array(snapshot[:sessions] || snapshot['sessions']) - .select { |session| session.is_a?(Hash) } - .map { |session| + .select { |session| session.is_a?(Hash) } + .map { |session|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/version.lic` around lines 163 - 178, The normalize_session_snapshot method currently coerces missing top-level counts (e.g., stale) to 0 by calling .to_i on (snapshot[:stale] || snapshot['stale']), which prevents preserving nil when the snapshot omits that key; update the merge logic for stale (and similarly for any other counts that must distinguish "missing" vs zero) to detect whether the snapshot actually provided the key (using snapshot.key?(:stale) or snapshot.key?('stale') or fetch with nil) and only call .to_i when a value exists, otherwise set stale to nil so potential_stale_sessions_count can fall back to per-session markers. Target normalize_session_snapshot and the stale/total/etc. merge entries in your change.
🤖 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/version.lic`:
- Around line 241-250: The grouping logic treats blank/nil PIDs and mixed
String/Integer PID types as distinct and also drops rows that only have a name;
update the pipeline around keys/grouped to normalize session_pid_value and
session_name_value before grouping: coerce PIDs to a single canonical type
(e.g., integer when numeric, otherwise nil) and normalize blank or nil PIDs to a
consistent sentinel (or nil) so that identical processes compare equal, and
ensure you don't drop rows solely because game is empty if name should still
count; apply the same normalization for the other occurrences noted (the blocks
around lines producing keys at 255-257 and 265-267) so grouped.count uses
normalized pid uniqueness checks rather than raw mixed types/blank values.
- Around line 305-307: The ALL/full/details branch only globs top-level files so
scripts/custom/*.lic are skipped; update the scripts_to_check assignment (where
Script.current.vars[1] is checked and show_report is set) to use a recursive
glob that includes subdirectories (e.g. a "**" pattern under SCRIPT_DIR) and
keep the existing /\.lic$/i filter so custom/*.lic files are included in the
all/full/details report.
- Around line 482-489: The marker fallback incorrectly synthesizes "active" for
rows where state == "exited"; update the marker assignment around session_info
(where stale and state are read) to avoid producing "active" for exited sessions
— e.g., when computing marker (currently using session_info[:marker] || ... ||
(stale ? 'stale' : 'active')), change the fallback logic to check state first
and return 'exited' (or a non-active marker) when state == 'exited', otherwise
return 'stale' if stale, otherwise 'active'; ensure the updated logic still
respects explicit session_info[:marker]/session_info['marker'] and that
session_report["Session #{pid}"] uses the new marker.
---
Duplicate comments:
In `@scripts/version.lic`:
- Around line 163-178: The normalize_session_snapshot method currently coerces
missing top-level counts (e.g., stale) to 0 by calling .to_i on
(snapshot[:stale] || snapshot['stale']), which prevents preserving nil when the
snapshot omits that key; update the merge logic for stale (and similarly for any
other counts that must distinguish "missing" vs zero) to detect whether the
snapshot actually provided the key (using snapshot.key?(:stale) or
snapshot.key?('stale') or fetch with nil) and only call .to_i when a value
exists, otherwise set stale to nil so potential_stale_sessions_count can fall
back to per-session markers. Target normalize_session_snapshot and the
stale/total/etc. merge entries in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a357f7ed-12f9-40bf-bc85-25c19bd4c0c2
📒 Files selected for processing (1)
scripts/version.lic
Changes made:
Preserved missing stale as nil in snapshot normalization (instead of forcing 0): version.lic (line 163)
Normalized duplicate-session PID handling ("123" and 123 now canonicalize the same; blank/non-numeric -> nil), and made session array read tolerant of symbol/string keys: version.lic (line 241), version.lic (line 268)
Switched all/full/details scan to recursive glob so nested custom/*.lic are included: version.lic (line 317)
Fixed detail marker fallback so exited sessions are not synthesized as active: version.lic (line 500)
NB: Anything about whether rows should exist / be filtered needs to be addressed at source: Redirect to core. This script is a reporting script only and does not maintenance / lifecycle ativities on the data itself.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/version.lic (1)
258-260:session_name_valueshould fall back to:namefor consistency.The detail output at Line 496 falls back to
:name/'name'when:session_nameis absent, but this helper does not. Sessions from older core versions that only expose:namewould return empty strings here, causing duplicate detection to skip them entirely (via therejecton Line 250).Proposed fix
def self.session_name_value(session) - (session[:session_name] || session['session_name']).to_s.strip.downcase + value = session[:session_name] || session['session_name'] || + session[:name] || session['name'] + value.to_s.strip.downcase end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/version.lic` around lines 258 - 260, The session_name_value helper currently only checks session[:session_name] and session['session_name']; update it to also fall back to session[:name] and session['name'] so it returns the older key when session_name is absent. Modify the logic in def self.session_name_value(session) (used by the duplicate detection path) to check keys in order [:session_name, 'session_name', :name, 'name'], then to_s.strip.downcase before returning.
🤖 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/version.lic`:
- Around line 258-260: The session_name_value helper currently only checks
session[:session_name] and session['session_name']; update it to also fall back
to session[:name] and session['name'] so it returns the older key when
session_name is absent. Modify the logic in def self.session_name_value(session)
(used by the duplicate detection path) to check keys in order [:session_name,
'session_name', :name, 'name'], then to_s.strip.downcase before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad5669e8-e238-412e-aa60-69c3d405beed
📒 Files selected for processing (1)
scripts/version.lic
Quick rubocop test on alignment
No logic changes
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/version.lic (1)
244-250:⚠️ Potential issue | 🟠 MajorNormalize duplicate-session identities before grouping.
Line 250 still keeps rows with
pid=nil, and Line 259 ignores rows that only exposename. That means the reboot recommendation can still false-positive on incomplete rows and miss real duplicates from older snapshot shapes.Possible fix
keys = live_sessions.map { |s| [ session_name_value(s), session_game_code_value(s), session_pid_value(s) ] - }.reject { |name, game, _pid| name.empty? || game.empty? } + }.reject { |name, game, pid| name.empty? || game.empty? || pid.nil? } @@ def self.session_name_value(session) - (session[:session_name] || session['session_name']).to_s.strip.downcase + value = session[:session_name] || session['session_name'] || + session[:name] || session['name'] + value.to_s.strip.downcase endAlso applies to: 258-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/version.lic` around lines 244 - 250, Normalize session identity triples before filtering and grouping: when building keys from live_sessions (the live_sessions.map that produces [session_name_value(s), session_game_code_value(s), session_pid_value(s)]), normalize each element (strip and downcase name and game, convert pid to a string or empty string when nil) and then reject only fully-empty rows (e.g., where name.empty? && game.empty? && pid.to_s.empty?), so rows that only expose name (or name+game) are preserved and pid=nil is normalized rather than left as nil.
🤖 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/version.lic`:
- Around line 179-181: The chained call starting with Array(snapshot[:sessions]
|| snapshot['sessions']) should have each method on its own indented line to
satisfy RuboCop: place .select on the next line aligned under the opening of the
receiver expression and .map on the following line aligned with .select, e.g.
start with "sessions: Array(snapshot[:sessions] || snapshot['sessions'])" then
newline with ".select { |session| session.is_a?(Hash) }" and another newline
with ".map { |session| ... }", keeping the block braces/contents unchanged so
the Array(...).select.map chain is properly indented.
---
Duplicate comments:
In `@scripts/version.lic`:
- Around line 244-250: Normalize session identity triples before filtering and
grouping: when building keys from live_sessions (the live_sessions.map that
produces [session_name_value(s), session_game_code_value(s),
session_pid_value(s)]), normalize each element (strip and downcase name and
game, convert pid to a string or empty string when nil) and then reject only
fully-empty rows (e.g., where name.empty? && game.empty? && pid.to_s.empty?), so
rows that only expose name (or name+game) are preserved and pid=nil is
normalized rather than left as nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a046e98c-74da-4ab4-b85c-78614e551bc0
📒 Files selected for processing (1)
scripts/version.lic
No logic changes
Add a method to check if session summary output is enabled based on feature flags. If feature flags not available, or if feature flag for session summary is set to false, behavior is exactly the same as version 1.2.2 If feature flags is available AND the feature flag for session summary is set to true, new reporting behavior on sessions is avaialble.
|
@coderabbitai hide all previous comments and review again |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/version.lic (1)
256-266:⚠️ Potential issue | 🟡 MinorAdd nil PID check to avoid false duplicate positives.
Sessions with unknown PIDs (when
session_pid_valuereturnsnil) are currently counted as duplicates if paired with a session having a known PID. Since the function aims to detect the same character+game running on multiple resident processes, sessions without valid PIDs should be excluded from duplicate detection:}.reject { |name, game, _pid| name.empty? || game.empty? } + }.reject { |name, game, pid| name.empty? || game.empty? || pid.nil? }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/version.lic` around lines 256 - 266, The duplicate-detection currently treats sessions with nil PIDs as valid and can produce false duplicates; when building keys (the keys array from live_sessions) filter out entries where session_pid_value(s) is nil (e.g., extend the existing reject to also exclude rows with nil pid) before grouping, so grouped (and the subsequent count on grouped) only considers entries with a valid pid; update the keys construction or add a .compact/.reject { |_name, _game, pid| pid.nil? } prior to grouped to exclude nil PIDs.
🤖 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/version.lic`:
- Around line 270-272: session_name_value currently only checks
session[:session_name] and session['session_name'], causing sessions that only
provide :name/'name' to return empty and be excluded from duplicate counting;
update session_name_value to follow the same multi-level fallback as
session_game_code_value by also checking session[:name] and session['name'],
then convert the selected value with to_s.strip.downcase so the detail output
and duplicate counting use the same normalized name.
---
Duplicate comments:
In `@scripts/version.lic`:
- Around line 256-266: The duplicate-detection currently treats sessions with
nil PIDs as valid and can produce false duplicates; when building keys (the keys
array from live_sessions) filter out entries where session_pid_value(s) is nil
(e.g., extend the existing reject to also exclude rows with nil pid) before
grouping, so grouped (and the subsequent count on grouped) only considers
entries with a valid pid; update the keys construction or add a .compact/.reject
{ |_name, _game, pid| pid.nil? } prior to grouped to exclude nil PIDs.
🪄 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: 503fd403-9657-4901-bc80-300209205c72
📒 Files selected for processing (1)
scripts/version.lic
| def self.session_name_value(session) | ||
| (session[:session_name] || session['session_name']).to_s.strip.downcase | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if core SessionsSettings always provides :session_name or if :name fallback is needed
ast-grep --pattern 'session_name'
rg -n ':session_name|:name' --type ruby -C 2 | head -50Repository: elanthia-online/scripts
Length of output: 3385
🏁 Script executed:
# First, locate and read the relevant parts of scripts/version.lic
wc -l scripts/version.licRepository: elanthia-online/scripts
Length of output: 90
🏁 Script executed:
# Read lines 260-280 to see session_name_value method
sed -n '260,280p' scripts/version.licRepository: elanthia-online/scripts
Length of output: 893
🏁 Script executed:
# Read lines 495-515 to see detail output reference at line 503
sed -n '495,515p' scripts/version.licRepository: elanthia-online/scripts
Length of output: 1287
🏁 Script executed:
# Search for where sessions are created and what keys they contain
rg -n 'session\[' scripts/version.lic | head -20Repository: elanthia-online/scripts
Length of output: 520
🏁 Script executed:
# Look for the reject clause mentioned in the scratchpad
rg -n '\.reject' scripts/version.lic -A 2 -B 2 | head -30Repository: elanthia-online/scripts
Length of output: 251
🏁 Script executed:
# Check what method contains the reject clause at line 262
sed -n '240,270p' scripts/version.licRepository: elanthia-online/scripts
Length of output: 1136
🏁 Script executed:
# Verify all uses of session_name_value
rg -n 'session_name_value' scripts/version.licRepository: elanthia-online/scripts
Length of output: 144
🏁 Script executed:
# Check if there are any other extractors to understand the pattern
rg -n 'def self\.session_' scripts/version.licRepository: elanthia-online/scripts
Length of output: 404
Align session_name_value fallback logic with detail output.
session_name_value (line 270) only checks :session_name/'session_name', but the detail output (line 505) also falls back to :name/'name'. If a session only provides :name, this method returns an empty string, which causes the session to be excluded from duplicate counting (line 262 rejects empty names) while still appearing in the detail output. Other extractors like session_game_code_value follow the multi-level fallback pattern.
Add the missing fallback:
Suggested change
def self.session_name_value(session)
- (session[:session_name] || session['session_name']).to_s.strip.downcase
+ value = session[:session_name] || session['session_name'] ||
+ session[:name] || session['name']
+ value.to_s.strip.downcase
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/version.lic` around lines 270 - 272, session_name_value currently
only checks session[:session_name] and session['session_name'], causing sessions
that only provide :name/'name' to return empty and be excluded from duplicate
counting; update session_name_value to follow the same multi-level fallback as
session_game_code_value by also checking session[:name] and session['name'],
then convert the selected value with to_s.strip.downcase so the detail output
and duplicate counting use the same normalized name.

What Changed
Integrated core snapshot module
Updated snapshot source from Lich::Common::SessionSettings to Lich::Common::SessionsSettings.
Preserved fallback behavior when core snapshot is unavailable.
Added user-facing session health summary
Introduced top-level computed summary fields:
Added helper methods to compute:
stale count (potential_stale_sessions_count)
duplicate session-name count (duplicate_session_name_count)
recommendation text (session_recommendation)
Reduced default output noise
Moved technical per-session rows to all/full/details mode only.
Default ;version now favors concise, actionable output.
Updated placement and emphasis
Moved session summary out of the top report body to a dedicated section near the end.
Added visual separators.
Added monsterbold recommendation label via Lich::Messaging.monsterbold('Recommendation..........').
Script metadata/versioning
Bumped script header version to 1.3.0.
Added changelog entries for the above behavior changes.
Why
Align version.lic with new core session summary infrastructure.
Provide non-technical users actionable guidance without overwhelming detail.
Keep deep diagnostics available when explicitly requested.
NOTE: Requires Lich core PR# 1247
Summary by CodeRabbit
New Features
Improvements