feat: add direct session resume by name#181
feat: add direct session resume by name#181ccrios83 wants to merge 3 commits intompfaffenberger:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces a new direct-load path for autosave sessions triggered by "AUTOSAVE_LOAD_DIRECT:" commands, complementing the existing interactive picker. Extracts session names, loads session history, updates agent message history, and computes token counts with error handling that gracefully falls back to interactive flow. Changes
Sequence DiagramsequenceDiagram
participant User
participant CommandHandler as Command Handler
participant CLIRunner as CLI Runner
participant SessionFile as Session File System
User->>CommandHandler: /autosave_load [session_name]
CommandHandler->>CommandHandler: Parse tokens
alt Session name provided
CommandHandler->>CLIRunner: Return "__AUTOSAVE_LOAD_DIRECT__:<session_name>"
CLIRunner->>SessionFile: Check session file exists
SessionFile->>CLIRunner: File status
CLIRunner->>SessionFile: Load session history
SessionFile->>CLIRunner: History data
CLIRunner->>CLIRunner: Update agent message history<br/>Compute token count
CLIRunner->>User: Emit success with metadata
else No session name
CommandHandler->>CLIRunner: Return "__AUTOSAVE_LOAD__"
CLIRunner->>User: Trigger interactive session picker
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code_puppy/cli_runner.py`:
- Around line 661-668: session_name is used directly to build session_path
allowing path traversal (e.g. "../../foo"); instead of interpolating blindly,
resolve the candidate path and verify it is inside AUTOSAVE_DIR before checking
exists() or deserializing: build base_dir = Path(AUTOSAVE_DIR).resolve(),
construct session_path = (base_dir / session_name).resolve() (or otherwise join
then resolve), then ensure session_path == base_dir or base_dir in
session_path.parents; if the check fails, call emit_error/emit_info and
continue; perform this guard prior to the exists() check and any unpickling to
prevent access outside AUTOSAVE_DIR.
🧹 Nitpick comments (3)
code_puppy/command_line/session_commands.py (1)
203-217: Return type annotation-> boolis incorrect — function returnsstr.
handle_autosave_load_commandis annotated as returningbool, but it actually returns string markers ("__AUTOSAVE_LOAD__"or"__AUTOSAVE_LOAD_DIRECT__:…"). This was likely pre-existing for the__AUTOSAVE_LOAD__case, but now with two distinct string return paths it's worth correcting.Proposed fix
-def handle_autosave_load_command(command: str) -> bool: +def handle_autosave_load_command(command: str) -> str:code_puppy/cli_runner.py (1)
650-692: Consider extracting shared session-load logic into a helper.The direct-load block (lines 650-692) and the interactive-picker block (lines 694-761) share nearly identical steps:
load_session→set_message_history→set_current_autosave_from_session_name→ token counting → success message. Extracting a small helper (e.g.,_apply_loaded_session(session_name, base_dir, agent)) would reduce duplication and keep both paths in sync.code_puppy/plugins/customizable_commands/register_callbacks.py (1)
54-54: Fragile string comparison to select glob pattern.The
*.prompt.mdpattern is selected viadirectory != ".github/prompts", which is a brittle coupling to the exact string in_COMMAND_DIRECTORIES. If someone normalizes or changes that entry, this silently breaks.Consider a small improvement:
Proposed fix
- pattern = "*.md" if directory != ".github/prompts" else "*.prompt.md" + pattern = "*.prompt.md" if "prompts" in Path(directory).parts else "*.md"Or store tuples of
(directory, pattern)in_COMMAND_DIRECTORIES.
Allow users to resume a session directly by name instead of always opening the interactive picker: /resume # Opens picker (unchanged) /resume auto_session_20260214_175543 # Loads directly (NEW) - session_commands.py: Parse optional session_name argument - cli_runner.py: Handle __AUTOSAVE_LOAD_DIRECT__ marker Closes: user feedback about /resume <name> not working
ccrios83
force-pushed
the
feature/direct-session-resume
branch
from
680a89e to
541c60a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
code_puppy/command_line/session_commands.py (1)
203-217: Return type annotation saysboolbut function returnsstr.
handle_autosave_load_commandis annotated-> boolbut returns string markers on both branches. This predates this PR (the__AUTOSAVE_LOAD__return existed before), but the PR extends the pattern. Consider updating the annotation to-> Union[bool, str](orbool | str) so type checkers and readers aren't misled.Also, extra tokens after the session name are silently ignored (e.g.,
/resume foo baruses"foo"and drops"bar"). If that's intentional, a brief comment would help; otherwise you could warn the user or join the tail.Proposed annotation fix and extra-token handling
-def handle_autosave_load_command(command: str) -> bool: +def handle_autosave_load_command(command: str) -> "bool | str": """Load an autosave session. If no session name is provided, opens interactive picker. If session name is provided, loads that session directly. """ tokens = command.split() if len(tokens) == 1: # No argument = open picker return "__AUTOSAVE_LOAD__" + if len(tokens) > 2: + from code_puppy.messaging import emit_warning + emit_warning("Usage: /resume [session_name] — extra arguments ignored.") + # Session name provided = load directly session_name = tokens[1] return f"__AUTOSAVE_LOAD_DIRECT__:{session_name}"code_puppy/cli_runner.py (4)
656-666: Redundant manual existence check —load_sessionalready raisesFileNotFoundError.Lines 657–663 manually construct
session_pathand check.exists(), butload_session(line 666) independently builds the path viabuild_session_pathsand raisesFileNotFoundErrorif it doesn't exist. Two concerns:
- Inconsistency risk: If
build_session_pathsever diverges frombase_dir / f"{session_name}.pkl", the manual check could pass whileload_sessionfails (or vice versa).- Redundancy: The
except Exceptionblock on line 682 already catches errors, so the manual check is purely for a friendlier error message — but you can achieve the same by catchingFileNotFoundErrorspecifically.Proposed simplification — catch FileNotFoundError explicitly
try: from code_puppy.agents.agent_manager import get_current_agent from code_puppy.config import ( set_current_autosave_from_session_name, ) from code_puppy.messaging import emit_error, emit_success from code_puppy.session_storage import load_session base_dir = Path(AUTOSAVE_DIR) - session_path = base_dir / f"{session_name}.pkl" - - # Check if session exists - if not session_path.exists(): - emit_error(f"❌ Session not found: {session_name}") - emit_info("💡 Use /resume to open the session picker.") - continue - - # Load the session history = load_session(session_name, base_dir) agent = get_current_agent() agent.set_message_history(history) # ... + except FileNotFoundError: + emit_error(f"❌ Session not found: {session_name}") + emit_info("💡 Use /resume to open the session picker.") except Exception as e: - from code_puppy.messaging import emit_error - emit_error(f"Failed to load session: {e}") emit_info("💡 Use /resume to open the session picker.") continue
645-687: Significant code duplication between direct-load and interactive-picker paths.The direct-load block (lines 648–687) and the interactive-picker block (lines 690–756) share nearly identical post-load logic:
load_session(name, base_dir)agent.set_message_history(history)set_current_autosave_from_session_name(name)- Token estimation loop
emit_success(...)with session statsConsider extracting a helper (e.g.,
_apply_loaded_session(session_name, history, agent)) to eliminate this duplication. This would also make it easier to keep the success message format consistent between the two paths (the direct path uses{total_tokens:,}with a thousands separator while the interactive path uses{total_tokens}without one).Sketch of extracted helper
def _apply_loaded_session(session_name: str, history, agent) -> str: """Apply loaded session history to agent and return summary string.""" from code_puppy.config import set_current_autosave_from_session_name agent.set_message_history(history) set_current_autosave_from_session_name(session_name) total_tokens = sum(agent.estimate_tokens_for_message(msg) for msg in history) return f"{len(history)} messages ({total_tokens:,} tokens)"Both the direct-load and interactive-picker blocks can then call this helper after
load_session, keeping the logic DRY and the output format consistent.Also applies to: 688-756
645-647: Marker-based control flow is fragile — consider using a structured return type.The
__AUTOSAVE_LOAD_DIRECT__:and__AUTOSAVE_LOAD__magic string markers couplesession_commands.pyandcli_runner.pyvia undocumented string conventions. If a marker is typo'd or the delimiter changes, the bug will be silent. A lightweight dataclass orNamedTuple(e.g.,CommandAction(kind="autosave_load_direct", payload=session_name)) would give you type safety and IDE autocompletion at negligible cost.This is a broader pattern in the existing code, so no need to fix it in this PR, but worth considering for future refactors.
674-676: Token estimation iterates the full history eagerly.
sum(agent.estimate_tokens_for_message(msg) for msg in history)walks every message for a display-only stat. For large histories this could be slow. Not a blocker, but worth noting — ifload_sessionmetadata already stores a token count, you could read it from there instead.
- Validate session_path stays within AUTOSAVE_DIR before loading - Resolve paths to prevent ../../ escape attacks - Fix return type annotation (bool -> bool | str) Addresses CodeRabbit security review feedback
nhicks00
left a comment
nhicks00
left a comment
There was a problem hiding this comment.
Review: Request Changes
Thanks for adding direct session resume! Great feature. Found a few issues that need addressing:
🐛 Critical: emit_info Not Imported
In cli_runner.py, the direct load error path uses emit_info but only imports emit_error and emit_success:
from code_puppy.messaging import emit_error, emit_success # ❌ Missing emit_info!
emit_error(f"❌ Invalid session name: {session_name}")
emit_info("💡 Use /resume to open the session picker.") # 💥 NameError!This will crash when a session is not found or path traversal is detected.
Fix: Add emit_info to the import statement.
⚠️ UX Inconsistency: Missing display_resumed_history
The interactive picker path calls display_resumed_history(history) after loading, but the direct load path skips this. Users get different experiences depending on how they resume.
Suggestion: Add the same display_resumed_history(history) call to the direct load path for consistency.
💡 Nice to Have: DRY Refactor
The session loading logic is duplicated between both paths:
load_session()set_message_history()set_current_autosave_from_session_name()- Token calculation
- Success message
Consider extracting to a shared helper like _load_and_resume_session(session_name).
✅ What Looks Good
- Path traversal protection 👍
- Backward compatibility (no args = picker)
- Updated help text and docstrings
- All tests pass
Let me know if you have questions!
2 participants
Summary
Allow users to resume a session directly by name instead of always opening the interactive picker:
Problem
Users kept getting told they could use
/resume <session_name>but it didn't actually work - it just opened the picker regardless.Changes
session_commands.pycli_runner.py__AUTOSAVE_LOAD_DIRECT__marker with session lookupUser Experience
Success:
Error:
Testing
Ported from Walmart internal implementation
Summary by CodeRabbit