feat(runtime-api): add session save, undo/retry, and snapshot endpoints for GUI#2808
feat(runtime-api): add session save, undo/retry, and snapshot endpoints for GUI#2808gaord wants to merge 1 commit into
Conversation
|
Thanks @gaord for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new HTTP endpoints to the runtime API, including session saving, thread turn undoing/retrying, and snapshot listing/restoring. It also updates the TUI engine to support retrieving session snapshots via oneshot channels and improves message reconstruction from tool calls and file changes. The review feedback highlights a compilation error in the git diff calculation where Result::and_then is incorrectly used with an Option-returning closure, suggesting the use of the existing run_git helper instead. Additionally, the feedback recommends explicitly handling std::io::ErrorKind::NotFound when loading sessions to prevent silent overwrites on other I/O errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let diff_stat = | ||
| crate::dependencies::Git::command() | ||
| .and_then(|mut git| { | ||
| git.args(["diff", "--stat"]) | ||
| .current_dir(&workspace) | ||
| .output() | ||
| .ok() | ||
| .and_then(|o| { | ||
| let s = String::from_utf8_lossy(&o.stdout) | ||
| .trim() | ||
| .to_string(); | ||
| if s.is_empty() { | ||
| None | ||
| } else { | ||
| Some(s) | ||
| } | ||
| }) | ||
| }); |
There was a problem hiding this comment.
The diff_stat calculation chains .and_then on crate::dependencies::Git::command(). If Git::command() returns a Result<Command, Error>, calling .and_then with a closure returning Option<String> will cause a compilation error because Result::and_then expects a closure returning a Result. \n\nAdditionally, there is already a local helper function run_git defined in this file that executes git commands and returns Option<String>. We can simplify this entire block and avoid compilation issues by using run_git directly.
let diff_stat = run_git(&workspace, &["diff", "--stat"])\n .map(|s| s.trim().to_string())\n .filter(|s| !s.is_empty());| let session = if let Some(ref existing_id) = req.session_id { | ||
| match manager.load_session(existing_id) { | ||
| Ok(existing) => { | ||
| let mut updated = crate::session_manager::update_session( | ||
| existing, | ||
| &snapshot.messages, | ||
| snapshot.total_tokens, | ||
| snapshot.system_prompt.as_ref(), | ||
| ); | ||
| updated.metadata.model = snapshot.model.clone(); | ||
| updated.metadata.mode = Some(snapshot.mode.clone()); | ||
| updated | ||
| } | ||
| Err(_) => { | ||
| crate::session_manager::create_saved_session_with_id_and_mode( | ||
| existing_id.clone(), | ||
| &snapshot.messages, | ||
| &snapshot.model, | ||
| &snapshot.workspace, | ||
| snapshot.total_tokens, | ||
| snapshot.system_prompt.as_ref(), | ||
| Some(snapshot.mode.as_str()), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
When attempting to load an existing session, any error (such as PermissionDenied or corrupted JSON data) is caught by the wildcard Err(_) pattern and silently ignored, falling back to creating a new session. This can lead to silent overwrites and data loss of existing session files.\n\nIt is safer to explicitly match std::io::ErrorKind::NotFound for the fallback creation path, and propagate any other I/O errors back to the caller.
let session = if let Some(ref existing_id) = req.session_id {\n match manager.load_session(existing_id) {\n Ok(existing) => {\n let mut updated = crate::session_manager::update_session(\n existing,\n &snapshot.messages,\n snapshot.total_tokens,\n snapshot.system_prompt.as_ref(),\n );\n updated.metadata.model = snapshot.model.clone();\n updated.metadata.mode = Some(snapshot.mode.clone());\n updated\n }\n Err(e) if e.kind() == std::io::ErrorKind::NotFound => {\n crate::session_manager::create_saved_session_with_id_and_mode(\n existing_id.clone(),\n &snapshot.messages,\n &snapshot.model,\n &snapshot.workspace,\n snapshot.total_tokens,\n snapshot.system_prompt.as_ref(),\n Some(snapshot.mode.as_str()),\n )\n }\n Err(e) => {\n return Err(ApiError::internal(format!("Failed to load existing session: {e}")));\n }\n }\n }![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let (forked_thread, original_user_text) = state | ||
| .runtime_threads | ||
| .fork_at_user_message(&id, depth) | ||
| .await | ||
| .map_err(map_thread_err)?; | ||
|
|
||
| let retry_prompt = req | ||
| .prompt | ||
| .or(original_user_text) | ||
| .unwrap_or_default(); | ||
| if retry_prompt.trim().is_empty() { | ||
| return Err(ApiError::bad_request( | ||
| "No user message to retry — the dropped turn had no user text", | ||
| )); | ||
| } | ||
|
|
||
| let turn = state | ||
| .runtime_threads | ||
| .start_turn( | ||
| &forked_thread.id, | ||
| StartTurnRequest { | ||
| prompt: retry_prompt, | ||
| model: None, | ||
| mode: None, | ||
| allow_shell: None, | ||
| trust_mode: None, | ||
| auto_approve: None, | ||
| input_summary: None, | ||
| }, | ||
| ) | ||
| .await | ||
| .map_err(map_thread_err)?; |
There was a problem hiding this comment.
fork_at_user_message commits the new thread to the database before the subsequent validation and start_turn call. Two paths leave the DB with an empty, unrecoverable thread:
- If
retry_prompt.trim().is_empty()(line 1764), the fork already exists when the 400 is returned. - If
start_turnfails (line 1770), the fork persists with no turns.
In both cases the caller receives an error response but a new thread appears silently in the thread list. Move the retry_prompt validation before the fork_at_user_message call to fix case 1. For case 2, consider deleting the forked thread on start_turn error, or document that callers must handle cleanup.
| async fn patch_undo_thread_turn( | ||
| State(state): State<RuntimeApiState>, | ||
| Path(id): Path<String>, | ||
| Json(req): Json<UndoTurnRequest>, | ||
| ) -> Result<(StatusCode, Json<PatchUndoResponse>), ApiError> { | ||
| let depth = req.depth.unwrap_or(0); | ||
|
|
||
| // Step 1: Try snapshot-based file rollback (patch_undo). | ||
| let thread = state | ||
| .runtime_threads | ||
| .get_thread(&id) | ||
| .await | ||
| .map_err(map_thread_err)?; | ||
|
|
||
| let workspace = PathBuf::from(&thread.workspace); | ||
| let patch_result = match crate::snapshot::SnapshotRepo::open_or_init(&workspace) { | ||
| Ok(repo) => { | ||
| match repo.list(20) { | ||
| Ok(snapshots) => { | ||
| // Find the newest tool: or pre-turn: snapshot that differs | ||
| // from the current workspace — same logic as TUI's patch_undo. | ||
| let target = snapshots | ||
| .iter() | ||
| .filter(|s| { | ||
| s.label.starts_with("tool:") || s.label.starts_with("pre-turn:") | ||
| }) | ||
| .find(|s| { | ||
| matches!( | ||
| repo.work_tree_matches_snapshot(&s.id), | ||
| Ok(false) | Err(_) | ||
| ) | ||
| }); | ||
|
|
||
| match target { | ||
| Some(target) => match repo.restore(&target.id) { | ||
| Ok(()) => { | ||
| // Compute diff stat for the summary. | ||
| let diff_stat = | ||
| crate::dependencies::Git::command() | ||
| .and_then(|mut git| { | ||
| git.args(["diff", "--stat"]) | ||
| .current_dir(&workspace) | ||
| .output() | ||
| .ok() | ||
| .and_then(|o| { | ||
| let s = String::from_utf8_lossy(&o.stdout) | ||
| .trim() | ||
| .to_string(); | ||
| if s.is_empty() { | ||
| None | ||
| } else { | ||
| Some(s) | ||
| } | ||
| }) | ||
| }); | ||
|
|
||
| let short = | ||
| &target.id.as_str()[..target.id.as_str().len().min(8)]; | ||
| let summary = match diff_stat { | ||
| Some(ref stat) => { | ||
| format!( | ||
| "Restored snapshot '{}' ({}). Files affected:\n{stat}", | ||
| target.label, short | ||
| ) | ||
| } | ||
| None => { | ||
| format!( | ||
| "Restored snapshot '{}' ({}). No diff changes detected.", | ||
| target.label, short | ||
| ) | ||
| } | ||
| }; | ||
|
|
||
| PatchUndoResult { | ||
| files_restored: true, | ||
| summary: Some(summary), | ||
| snapshot_label: Some(target.label.clone()), | ||
| } | ||
| } | ||
| Err(e) => PatchUndoResult { | ||
| files_restored: false, | ||
| summary: Some(format!("Restore failed: {e}")), | ||
| snapshot_label: None, | ||
| }, | ||
| }, | ||
| None => PatchUndoResult { | ||
| files_restored: false, | ||
| summary: Some( | ||
| "No older tool or pre-turn snapshots differ from the current workspace.".to_string(), | ||
| ), | ||
| snapshot_label: None, | ||
| }, | ||
| } | ||
| } | ||
| Err(e) => PatchUndoResult { | ||
| files_restored: false, | ||
| summary: Some(format!("Failed to list snapshots: {e}")), | ||
| snapshot_label: None, | ||
| }, | ||
| } | ||
| } | ||
| Err(e) => PatchUndoResult { | ||
| files_restored: false, | ||
| summary: Some(format!("Snapshot repo unavailable: {e}")), | ||
| snapshot_label: None, | ||
| }, | ||
| }; | ||
|
|
||
| // Step 2: Remove the last conversation turn (undo_conversation). | ||
| let (forked_thread, original_user_text) = state | ||
| .runtime_threads | ||
| .fork_at_user_message(&id, depth) | ||
| .await | ||
| .map_err(map_thread_err)?; |
There was a problem hiding this comment.
patch_undo
Step 1 restores workspace files from the snapshot, then Step 2 calls fork_at_user_message. If fork_at_user_message fails (e.g., the thread has no user turns, or a DB write fails), the workspace files have already been reverted but the conversation history still contains the "undone" turn. The user is left in a split state: filesystem rolled back, conversation history pointing at turns that reference the old file state. There is no automatic recovery — the user must manually re-apply changes or delete the partially-reverted state.
| TurnItemKind::ToolCall | TurnItemKind::FileChange => { | ||
| // Reconstruct a minimal tool_use block for the session | ||
| // so the file change cards render correctly when viewing | ||
| // a resumed session. | ||
| let name = item.summary.clone(); | ||
| let input: serde_json::Value = item | ||
| .metadata | ||
| .clone() | ||
| .unwrap_or_else(|| serde_json::Value::Object(Default::default())); | ||
| let id = item.id.clone(); | ||
| let tool_use_id = id.clone(); | ||
| messages.push(Message { | ||
| role: "assistant".to_string(), | ||
| content: vec![ContentBlock::ToolUse { | ||
| id: id.clone(), | ||
| name: name.clone(), | ||
| input: input.clone(), | ||
| caller: None, | ||
| }], | ||
| }); | ||
| // Synthesize a tool_result message so downstream parsers | ||
| // (e.g. turn restoration) see a complete tool round-trip. | ||
| let output = item.detail.clone().unwrap_or_default(); | ||
| messages.push(Message { | ||
| role: "user".to_string(), | ||
| content: vec![ContentBlock::ToolResult { | ||
| tool_use_id, | ||
| content: output, | ||
| is_error: Some(matches!( | ||
| item.status, | ||
| TurnItemLifecycleStatus::Failed | ||
| )), | ||
| content_blocks: None, | ||
| }], | ||
| }); | ||
| } |
There was a problem hiding this comment.
When a single assistant turn contained multiple ToolCall (or FileChange) items alongside an AgentMessage, this loop generates alternating assistant:[ToolUse] / user:[ToolResult] messages for each item, rather than batching all ToolUse blocks into one assistant message. The Anthropic API requires that all tool-use blocks from one assistant response live in a single assistant message, followed by a single user message containing all corresponding tool_result blocks. Sending them as separate alternating messages will cause API rejection on the next turn for any resumed session that contained multi-tool turns.
| let depth = req.depth.unwrap_or(0); | ||
| let (forked_thread, original_user_text) = state | ||
| .runtime_threads | ||
| .fork_at_user_message(&id, depth) | ||
| .await | ||
| .map_err(map_thread_err)?; | ||
|
|
||
| let retry_prompt = req | ||
| .prompt | ||
| .or(original_user_text) | ||
| .unwrap_or_default(); | ||
| if retry_prompt.trim().is_empty() { | ||
| return Err(ApiError::bad_request( | ||
| "No user message to retry — the dropped turn had no user text", | ||
| )); | ||
| } |
There was a problem hiding this comment.
| let depth = req.depth.unwrap_or(0); | |
| let (forked_thread, original_user_text) = state | |
| .runtime_threads | |
| .fork_at_user_message(&id, depth) | |
| .await | |
| .map_err(map_thread_err)?; | |
| let retry_prompt = req | |
| .prompt | |
| .or(original_user_text) | |
| .unwrap_or_default(); | |
| if retry_prompt.trim().is_empty() { | |
| return Err(ApiError::bad_request( | |
| "No user message to retry — the dropped turn had no user text", | |
| )); | |
| } | |
| let depth = req.depth.unwrap_or(0); | |
| // Validate prompt availability before forking so a failed validation | |
| // cannot leave an orphaned empty thread in the database. | |
| if req.prompt.as_deref().map(str::trim) == Some("") { | |
| return Err(ApiError::bad_request( | |
| "No user message to retry — the dropped turn had no user text", | |
| )); | |
| } | |
| let (forked_thread, original_user_text) = state | |
| .runtime_threads | |
| .fork_at_user_message(&id, depth) | |
| .await | |
| .map_err(map_thread_err)?; | |
| let retry_prompt = req | |
| .prompt | |
| .or(original_user_text) | |
| .unwrap_or_default(); | |
| if retry_prompt.trim().is_empty() { | |
| let _ = state.runtime_threads.delete_thread(&forked_thread.id).await; | |
| return Err(ApiError::bad_request( | |
| "No user message to retry — the dropped turn had no user text", | |
| )); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| Ok(Json(json!({ | ||
| "restored": id, | ||
| }))) | ||
| } | ||
|
|
||
| fn map_thread_err(err: anyhow::Error) -> ApiError { | ||
| let message = err.to_string(); | ||
| if message.contains("not found") { |
There was a problem hiding this comment.
list_snapshots and restore_snapshot use state.workspace, not the thread's workspace
patch_undo_thread_turn opens the snapshot repo at thread.workspace (line 1618), but list_snapshots and restore_snapshot use state.workspace. If a thread was started with a workspace different from the global state workspace, the IDs returned by GET /v1/snapshots will refer to a different repo than the one PATCH /v1/threads/{id}/patch-undo acts on. A caller that lists snapshots and then uses an ID for a patch-undo may silently restore the wrong snapshot or get a "not found" error.
|
Thanks @gaord, this is a useful GUI-runtime direction and the endpoint list maps well to what desktop clients will eventually need. For the v0.9 stewardship branch I am going to leave this PR open rather than harvest it as-is. The current integration branch already has a narrower So my current disposition is: good intent, not a narrow v0.9 maintainer harvest yet. The safest next shape would be a smaller follow-up that keeps the GUI API thin but adds tests for atomicity/error paths before exposing file restore or retry behavior. |
Harvest the safe GUI-facing snapshot list slice from PR #2808 without exposing restore, retry, patch-undo, or other runtime mutation endpoints. The endpoint is protected by the existing runtime API token middleware and mirrors the /restore list bound. Refs #2808, #2580. Co-authored-by: gaord <9567937+gaord@users.noreply.github.com>
|
Follow-up v0.9 stewardship update: I harvested the safe read-only snapshot-list slice into #2822, which is now merged on What landed:
What I intentionally did not harvest yet: restore, retry, patch-undo, and conversation/file mutation endpoints. Those still need atomicity and message-reconstruction tests so GUI calls cannot leave orphaned fork threads, split filesystem/conversation state, or provider-invalid tool-message history. Thanks @gaord for the GUI runtime API direction; the #2822 commit, changelog, and PR body preserve that credit. |
|
Follow-up v0.9 stewardship update: another safe read-only GUI slice landed in #2862 ( What landed from the same GUI/runtime direction:
What still intentionally did not land from this PR: retry, undo, patch-undo, restore-over-HTTP, and active-session mutation behavior. Those still need atomicity/error-path tests so GUI clients cannot leave orphaned fork threads, split filesystem/conversation state, or provider-invalid reconstructed message history. Thanks again @gaord for the GUI runtime API direction; #2862 keeps that credit trail in the PR body and changelog while preserving the v0.9 read-only cutline. |
2 participants
Summary
Add Runtime API endpoints to align GUI capabilities with TUI, following the principle of reusing TUI's existing capabilities rather than reimplementing logic in the API layer.
New Endpoints
POST /v1/sessionsEngine::get_session_snapshot(), reusing TUI'sbuild_session_snapshotcode pathPOST /v1/threads/{id}/undofork_at_user_messagePOST /v1/threads/{id}/patch-undo/undocommand)POST /v1/threads/{id}/retryGET /v1/snapshotsPOST /v1/snapshots/{id}/restoreKey Design Decisions
Session saving reuses Engine state: Instead of reconstructing messages from turn items,
save_current_sessioncallsengine.get_session_snapshot()which returns the authoritative messages and token usage directly from the Engine. This ensures data consistency with TUI's internal state.Oneshot channel for snapshot:
Op::GetSessionSnapshotuses a oneshot channel to return the snapshot directly to the caller, avoiding competition with the SSE event stream on the mpsc receiver.API as thin adapter: Endpoints are thin wrappers that receive HTTP requests, call TUI's existing functions (
SessionManager,SnapshotRepo,fork_at_user_message), and return results. No business logic is reimplemented in the API layer.Changes
core/ops.rs: AddSessionSnapshotstruct andGetSessionSnapshotOpcore/engine.rs: HandleGetSessionSnapshotin the engine loopcore/engine/handle.rs: AddEngineHandle::get_session_snapshot()methodruntime_api.rs: Add 6 new endpointsruntime_threads.rs: Addget_engine()public wrapper; enhancereconstruct_messages_from_turnswith ToolCall/FileChange supportTesting
cargo buildpassesGreptile Summary
This PR adds six Runtime API endpoints (session save, undo, patch-undo, retry, and snapshot list/restore) by delegating to existing TUI primitives (
fork_at_user_message,SnapshotRepo,SessionManager) rather than reimplementing logic in the API layer.POST /v1/sessionssnapshots engine state via a newOp::GetSessionSnapshotoneshot-channel pattern;POST /v1/threads/{id}/retryand/undowrapfork_at_user_message;GET /v1/snapshotsandPOST /v1/snapshots/{id}/restoreexpose the existingSnapshotRepo.reconstruct_messages_from_turnsis extended to emit syntheticToolUse/ToolResultmessage pairs forToolCallandFileChangeitems when a thread is loaded into the engine.retry_thread_turncan leave an orphaned forked thread ifstart_turnfails;patch_undo_thread_turnrestores files before forking the conversation, leaving a split state if the fork step errors; and per-item tool message reconstruction produces an alternatingassistant/userpattern that violates the Anthropic API's single-message-per-turn requirement for multi-tool turns.Confidence Score: 2/5
Not safe to merge without addressing the atomicity gaps in retry and patch-undo, and the message-structure bug in tool call reconstruction.
Three bugs affect correctness on common paths: retry creates a permanent orphaned thread in the DB when validation or turn-start fails; patch-undo leaves the workspace and conversation history out of sync when the fork step errors; and the new ToolCall/FileChange reconstruction emits one assistant message per tool item, which breaks the Anthropic API's requirement that all tool-use blocks for a single turn be grouped in one message — any resumed session with multi-tool turns will fail on the next API call.
crates/tui/src/runtime_api.rs (retry and patch-undo atomicity) and crates/tui/src/runtime_threads.rs (tool message grouping in reconstruct_messages_from_turns)
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant API as runtime_api.rs participant RTM as RuntimeThreadManager participant Engine as EngineHandle participant SR as SnapshotRepo participant SM as SessionManager Note over Client,SM: POST /v1/sessions (save) Client->>API: POST /v1/sessions API->>RTM: get_engine(thread_id) RTM-->>API: EngineHandle API->>Engine: get_session_snapshot() Engine-->>API: SessionSnapshot (via oneshot) API->>SM: save_session(session) SM-->>Client: session_id + session Note over Client,SR: POST /v1/threads/{id}/undo Client->>API: "POST /v1/threads/{id}/undo" API->>RTM: fork_at_user_message(id, depth) RTM-->>Client: thread + original_user_text Note over Client,SR: POST /v1/threads/{id}/patch-undo Client->>API: "POST /v1/threads/{id}/patch-undo" API->>SR: open_or_init(thread.workspace) SR->>SR: list + filter + find differing snapshot SR->>SR: restore(target.id) ⚠️ files changed here API->>RTM: fork_at_user_message(id, depth) RTM-->>Client: patch_result + thread Note over Client,SR: POST /v1/threads/{id}/retry Client->>API: "POST /v1/threads/{id}/retry" API->>RTM: fork_at_user_message ⚠️ fork persisted RTM-->>API: forked_thread API->>RTM: start_turn(forked_thread.id, prompt) RTM-->>Client: thread + turnReviews (1): Last reviewed commit: "feat(runtime-api): add session save, und..." | Re-trigger Greptile