feat(session): implement session expiration with configurable timeout#83
Conversation
- Add CLAUDE_SESSION_EXPIRY_HOURS configuration variable (default: 24) - Add get_session_file_age_hours() helper with cross-platform stat support - Modify init_claude_session() to check session age and remove expired sessions - Add --session-expiry CLI flag to configure expiration (positive integers only) - Update help text with new option and example - Add 10 new tests for session expiration (TDD approach) Closes #51 Test count: 276 → 286 (100% pass rate)
|
Caution Review failedThe pull request is closed. WalkthroughThe changes implement session expiration support for Claude sessions with a configurable 24-hour default. A cross-platform helper function computes session file age, integrated into session initialization to automatically expire and restart sessions when age exceeds the threshold. A new CLI option ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as ralph_loop.sh (CLI)
participant SessionMgr as init_claude_session()
participant FileSystem as Session File<br/>(.claude_session_id)
User->>CLI: Execute with --session-expiry 48<br/>(or use default CLAUDE_SESSION_EXPIRY_HOURS=24)
CLI->>SessionMgr: Call init_claude_session()
SessionMgr->>FileSystem: Check if session file exists
alt Session file exists
SessionMgr->>SessionMgr: Call get_session_file_age_hours(file)
SessionMgr->>SessionMgr: Compare age >= configured expiry
alt Session expired
SessionMgr->>FileSystem: Delete expired session file
SessionMgr->>SessionMgr: Log: "Session expired (Xh old)"
SessionMgr->>CLI: Return empty (start new session)
else Session valid
SessionMgr->>FileSystem: Read session ID
SessionMgr->>SessionMgr: Log: "Resuming session (Xh old)"
SessionMgr->>CLI: Return session ID
end
else Session file missing
SessionMgr->>SessionMgr: Log: "Starting new session"
SessionMgr->>CLI: Return empty
end
CLI->>User: Proceed with session (new or resumed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
Add configurable session expiration to
|
Code Review - Session Expiration FeatureThis PR implements session expiration functionality for Claude Code sessions. Overall, the implementation is solid with good TDD practices. Here are my findings: Strengths
Code Quality Observations1. Session File Variable Confusion (ralph_loop.sh:492) There appears to be a mismatch in session file variables:
Question: Should this function be checking RALPH_SESSION_FILE instead? Or are we intentionally expiring Claude's native session file? 2. Integer Division Truncation (ralph_loop.sh:482) The code uses integer division which truncates fractional hours. For example:
This is likely fine for the use case, but worth noting that a session at 23h59m won't expire until it hits exactly 24h. 3. Edge Case: Stat Command Failure If stat fails (line 472/475), the function falls back to echo 0, which returns a file age of 0 hours. This means:
Consider whether this is the desired behavior or if a failed stat should trigger session expiration. Test QualityTest Limitations: Several tests only check for the presence of code patterns rather than testing actual behavior:
These are better than nothing, but they don't verify:
Suggestion: Consider adding integration tests that:
Security and PerformanceNo security concerns identified. Performance impact is negligible (one stat call per loop iteration). DocumentationThe PR description is excellent. However, CLAUDE.md should be updated to document:
Recommendations
VerdictApprove with suggestions. The implementation is functionally sound, but the session file variable question should be clarified before merging. The feature meets quality standards and follows TDD practices well. |
…#84) * Reapply "feat(session): implement session expiration with configurable timeout (#83)" This reverts commit 1ba55a4. * fix(session): address code review feedback - Fix integer overflow: return -1 from get_session_file_age_hours on stat failure instead of 0, preventing false expiration - Handle stat failure in init_claude_session with WARN log - Add comprehensive documentation for return values and expiration strategy - Add 6 behavioral integration tests that verify actual functionality - Add inline comments explaining 24-hour default rationale Test count: 286 → 292 (100% pass rate) * fix(test): use grep-based verification to fix CI failures Tests that sourced ralph_loop.sh with --help flag failed in GitHub Actions due to BATS environment differences. Changed behavioral tests to grep-based code verification that checks implementation patterns exist without executing the script. * fix(test): guard main with BASH_SOURCE for safe sourcing - Add BASH_SOURCE check to only execute main when script is run directly - Update tests to source script without --help flag - Convert grep-based verification tests back to functional tests - Fixes CI failures caused by script execution during sourcing --------- Co-authored-by: Test User <test@example.com>
1 participant
Summary
Implements session expiration for
.claude_session_idfiles to prevent stale sessions from causing unpredictable behavior.CLAUDE_SESSION_EXPIRY_HOURSconfiguration variable (default: 24 hours)get_session_file_age_hours()helper function with cross-platform stat support (Linux/macOS)init_claude_session()to check session age and automatically remove expired sessions--session-expiryCLI flag to configure expiration timeTest Plan
Changes
ralph_loop.shtests/unit/test_session_continuity.batsCloses
Closes #51
Summary by CodeRabbit
Release Notes
New Features
--session-expiry HOURScommand-line option to set custom session expiration at runtime.Tests
✏️ Tip: You can customize this high-level summary in your review settings.