feat(threads): add workspace field to UpdateThreadRequest#2640
Conversation
Allow updating a thread's workspace via the PATCH /v1/threads/{id}
endpoint. This fixes the cross-workspace thread loading issue where
a thread created in workspace A would remain bound to A even when
loaded and continued in workspace B, causing the conversation to
stall or output to the wrong workspace.
The GUI client can now update the thread's workspace to the current
workspace when loading a thread from a different workspace, ensuring
the engine processes messages in the correct context.
|
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a workspace field to UpdateThreadRequest and integrates it into the thread update logic. The feedback suggests changing the type of the workspace field from Option<String> to Option<PathBuf> to maintain consistency with CreateThreadRequest and eliminate redundant .into() conversions.
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.
| pub mode: Option<String>, | ||
| pub title: Option<String>, | ||
| pub system_prompt: Option<String>, | ||
| pub workspace: Option<String>, |
There was a problem hiding this comment.
| if let Some(workspace) = req.workspace | ||
| && thread.workspace != workspace | ||
| { | ||
| thread.workspace = workspace.clone().into(); | ||
| changes.insert("workspace".to_string(), json!(workspace)); | ||
| } |
There was a problem hiding this comment.
Since UpdateThreadRequest::workspace is updated to Option<PathBuf>, we can directly assign it to thread.workspace without calling .into(), avoiding unnecessary type conversion.
if let Some(workspace) = req.workspace
&& thread.workspace != workspace
{
thread.workspace = workspace.clone();
changes.insert("workspace".to_string(), json!(workspace));
}| if let Some(workspace) = req.workspace | ||
| && thread.workspace != workspace | ||
| { | ||
| thread.workspace = workspace.clone().into(); | ||
| changes.insert("workspace".to_string(), json!(workspace)); | ||
| } |
There was a problem hiding this comment.
Loaded engine is not evicted when workspace changes
update_thread persists the new workspace to disk but does not remove the thread's entry from active.engines. ensure_engine_loaded (line 1944) short-circuits and returns the cached engine unchanged whenever one is already present. If the engine was loaded before the PATCH call — e.g., the user had already resumed or sent a turn in workspace A — the next turn will still run in workspace A regardless of the update. The PR's core motivation (switching workspaces mid-session) will silently have no effect until the engine is evicted by the LRU. The cached engine entry for this thread should be removed inside update_thread when the workspace field changes.
1. Change UpdateThreadRequest.workspace from Option<String> to Option<PathBuf> for consistency with CreateThreadRequest and ThreadRecord, eliminating unnecessary .into() conversions. 2. Add empty-string validation for workspace, matching the existing guards on model and mode fields. 3. Evict the cached engine when workspace changes. Without this, ensure_engine_loaded would return the stale engine configured for the old workspace, silently ignoring the update — directly undermining the PR's stated purpose. 4. Remove unnecessary .clone() by reversing the insertion/assignment order so workspace is moved directly into thread.workspace.
In the GUI scenario, the TUI process for workspace B never has a cached engine for a thread created in workspace A, so evicting is a no-op. The real fix is simply updating the thread record's workspace field — ensure_engine_loaded will create a new engine with the correct workspace on the next turn.
|
Thanks @gaord — this The landed version accepts workspace updates through Verification recorded for the harvest: Closing this PR as harvested into the integration branch. |
|
@Hmbown could you backport this bit into 0.8.54 as vscode GUI 0.1.2 is expecting this for release. pls let me know if there is any concern. |
Summary
Add
workspacefield toUpdateThreadRequest, allowing the thread's workspace to be updated viaPATCH /v1/threads/{id}.Motivation
When a thread created in workspace A is loaded and continued in workspace B, the thread remains bound to workspace A. This causes the conversation to stall or output to the wrong workspace because the engine processes messages in the context of workspace A.
This is particularly problematic for GUI clients (VSCode extension) where users may open different workspaces and want to resume conversations across them.
Changes
workspace: Option<String>field toUpdateThreadRequestinruntime_threads.rsupdate_thread()methodAPI Contract
Testing
Greptile Summary
Adds
workspace: Option<PathBuf>toUpdateThreadRequestso clients can reassign a thread's working directory viaPATCH /v1/threads/{id}. The persistence side of the change is correct: the empty-string guard, the idempotency check (thread.workspace != workspace), and the store/event update are all in order.UpdateThreadRequestgains aworkspacefield with an empty-path guard consistent with the existingmodel/modeguards.thread.updatedevent with achangespayload.Confidence Score: 4/5
Safe to merge for persistence correctness; the workspace reassignment won't take effect mid-session if an engine is already cached for the thread.
The store-layer changes are correct — validation, idempotency check, and event emission all look right. However, ensure_engine_loaded (line 1950) returns the cached engine unchanged whenever one already exists in active.engines. If the client had already sent a turn before calling PATCH, subsequent turns will still run under the old workspace until the LRU evicts the engine, which silently defeats the PR's stated motivation for mid-session workspace switching.
crates/tui/src/runtime_threads.rs — specifically the interaction between update_thread and the engine cache in active.engines.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client as GUI Client (VSCode) participant API as PATCH /v1/threads/{id} participant RTM as RuntimeThreadManager participant Store as ThreadStore participant Event as EventBus Client->>API: "PATCH /v1/threads/{id} {workspace: /path/to/workspaceB}" API->>RTM: update_thread(id, req) RTM->>RTM: Validate at least one field present RTM->>RTM: Validate workspace not empty RTM->>Store: get_thread(id) Store-->>RTM: "ThreadRecord (workspace = workspaceA)" RTM->>RTM: "thread.workspace != workspace?" alt workspace changed RTM->>RTM: "changes[workspace] = workspaceB" RTM->>RTM: "thread.workspace = workspaceB" RTM->>Store: save_thread(thread) RTM->>Event: "emit thread.updated {changes}" else same workspace RTM->>RTM: skip (no changes) end RTM-->>API: ThreadRecord API-->>Client: 200 OK + updated threadReviews (2): Last reviewed commit: "fix(threads): remove unnecessary engine ..." | Re-trigger Greptile