feat(version.lic): v1.3.0 add stale session tracking#2265
Open
OSXLich-Doug wants to merge 9 commits intomasterfrom
Open
feat(version.lic): v1.3.0 add stale session tracking#2265OSXLich-Doug wants to merge 9 commits intomasterfrom
OSXLich-Doug wants to merge 9 commits intomasterfrom
Conversation
Updated version to 1.3.0 and added new features including session snapshot integration and simplified reporting.
Contributor
Author
|
new feature adds this (where appropriate!) 
|
mrhoribu
changed the title
Contributor
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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:
📝 WalkthroughWalkthroughAdds session snapshot integration and per-session health analysis to Changes
Sequence Diagram(s)mermaid 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)
📝 Coding Plan
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 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
Contributor
|
@coderabbitai summary |
Contributor
✅ 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.
Contributor
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
Contributor
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.
Contributor
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
Contributor
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.
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.
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