From b110ffa9c6038253476bd303b5b3d42129313b73 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 19:33:51 +0800 Subject: [PATCH 01/17] feat(desktop): select the durable store instead of searching for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A session id is unique only inside one durable store, and each attached project has one, so two projects may hold records with the same id and the sidebar renders both. The record-addressing ops did not say which one they meant. `session.archive` and `session.unarchive` took `{session}` alone, and `session.load` / `session.load_archived` took the store as an optional hint. Without it the host searched `@session_store.known_roots()`, which yields the global store first — so archiving one project's row could archive another project's record, and loading one conversation could read the other's transcript. All four ops now carry `workspace` as a required field, spelled the way `session.delete_archived` already spells it: a host-reported project path, or `""` for the global store. `@session_store.ArchivedStore` becomes `StoreSelection` because it now resolves live stores too; it is the single place a client's spelling turns into a root, and it is required rather than optional on `claim_session_family_move`, which drops that function's search fallback and its workspace back-inference. `@session_store.live_root_of` has no callers left. Refusals gain the store they were about: naming a detached workspace is refused by name, a record missing from the selected store says so instead of reporting the id as globally absent, and the live/archived twin check now looks in that same store rather than any store. Search remains where a request genuinely carries no store — the archived correlation defense guarding `agent.start`, and the whole-host deletion sweep. --- desktop/internal/api/api.mbt | 8 +- desktop/internal/api/payload.mbt | 13 +- desktop/internal/engine/archive.mbt | 206 +++++++++--------- .../internal/engine/archive_lookup_wbtest.mbt | 151 ++++++++++--- .../engine/archive_submodule_wbtest.mbt | 27 ++- desktop/internal/engine/ops.mbt | 84 +++---- desktop/internal/engine/pkg.generated.mbti | 4 +- .../internal/engine/worktree_seam_wbtest.mbt | 24 +- .../internal/protocol/desktop_messages.mbt | 25 ++- desktop/internal/protocol/pkg.generated.mbti | 3 +- .../internal/session_store/pkg.generated.mbti | 6 +- desktop/internal/session_store/store.mbt | 45 ++-- 12 files changed, 354 insertions(+), 242 deletions(-) diff --git a/desktop/internal/api/api.mbt b/desktop/internal/api/api.mbt index 2c2aae537..e5e23f894 100644 --- a/desktop/internal/api/api.mbt +++ b/desktop/internal/api/api.mbt @@ -471,6 +471,7 @@ pub fn endpoints( @engine.archive_session( state.manager, session, + payload.workspace, (moved, workspace) => { publish_session_changed(state, "archived", moved, workspace) }, @@ -482,9 +483,10 @@ pub fn endpoints( }), @endpoint.Endpoint::remote(@commands.session_unarchive, async fn(payload) { let session = payload.canonical_session() - @engine.unarchive_session(state.manager, session, (moved, workspace) => { - publish_session_changed(state, "unarchived", moved, workspace) - }) + @engine.unarchive_session(state.manager, session, payload.workspace, ( + moved, + workspace, + ) => publish_session_changed(state, "unarchived", moved, workspace)) }), @endpoint.Endpoint::remote(@commands.session_delete_archived, async fn( payload, diff --git a/desktop/internal/api/payload.mbt b/desktop/internal/api/payload.mbt index 8b4215e97..d53a95808 100644 --- a/desktop/internal/api/payload.mbt +++ b/desktop/internal/api/payload.mbt @@ -177,9 +177,20 @@ test "goal payload distinguishes set from clear by the text field" { } ///| -test "session move payload has one canonical id" { +test "session move payload has one canonical id and a required store" { let payload : SessionPayload = @json.from_json({ "session": " s-canonical ", + "workspace": "", }) assert_eq(payload.canonical_session(), "s-canonical") + assert_eq(payload.workspace, "") + // The store is half the record's identity, so a payload without it is not + // a valid move request — it would leave the host searching again. + let storeless = try { + let decoded : SessionPayload = @json.from_json({ "session": "s-canonical" }) + Some(decoded) + } catch { + _ => None + } + assert_true(storeless is None) } diff --git a/desktop/internal/engine/archive.mbt b/desktop/internal/engine/archive.mbt index e3eccb779..9107244b5 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -65,14 +65,13 @@ async fn EngineManager::sweep_archived_deletions(self : EngineManager) -> Unit { priv struct SessionFamilyMove { manager : EngineManager root : @pathx.Absolute - // The registered project whose store owns this family, when this page's - // registry still holds it. Permanent deletion uses this exact owner to - // remove the retained worktree placement even if the project detaches - // meanwhile. - workspace : @pathx.Absolute? - // Canonical protocol spelling of `workspace`, "" when no registered project - // claims the store. Every session.changed callback carries it so same-ID - // records in other stores do not inherit this family's disposition. + // The registered project whose store owns this family. Permanent deletion + // uses this exact owner to remove the retained worktree placement even if + // the project detaches meanwhile. + workspace : @pathx.Absolute + // Canonical protocol spelling of `workspace`. Every session.changed callback + // carries it so same-ID records in other stores do not inherit this family's + // disposition. workspace_token : String members : Array[String] guards : Array[(String, SessionRecordGuard)] @@ -94,15 +93,15 @@ fn SessionFamilyMove::release(self : SessionFamilyMove) -> Unit { } ///| -/// Locate and exclusively claim one family in either the live or archived -/// twin. The workspace lifecycle lock covers discovery through record and -/// slot claims; after it is released those claims make detach and competing -/// operations observe the move until `SessionFamilyMove::release`. +/// Locate and exclusively claim one family in the selected store's live or +/// archived twin. The workspace lifecycle lock covers discovery through +/// record and slot claims; after it is released those claims make detach and +/// competing operations observe the move until `SessionFamilyMove::release`. async fn EngineManager::claim_session_family_move( manager : EngineManager, session : String, archived~ : Bool, - selected? : @session_store.ArchivedStore, + selected~ : @session_store.StoreSelection, ) -> SessionFamilyMove { manager.workspace_lifecycle_lock.acquire() let mut placement_locked = true @@ -123,62 +122,47 @@ async fn EngineManager::claim_session_family_move( manager.workspace_lifecycle_lock.release() } } - if selected is Some(selected) { - // `from_workspace` runs before this lock is acquired. Detach uses the - // same lock, so repeat the registration check here and keep the selected - // store stable until the record and pending-operation claims are visible. - guard @workspaces.registered_dir(selected.workspace) is Some(registered) && - @workspaces.store_root(registered) == selected.root else { - raise EngineError( - "\{selected.workspace_token} is not a registered workspace", - ) - } - } - let root = if selected is Some(selected) { - let record = if archived { - @session_store.archived_root(selected.root).join("sessions").join(session) - } else { - selected.root.join("sessions").join(session) - } - if @fsx.is_dir(record.to_path()) { - Some(selected.root) - } else { - None - } - } else if archived { - @session_store.archived_root_of(session) - } else { - @session_store.live_root_of(session) + // `from_workspace` runs before this lock is acquired. Detach uses the same + // lock, so repeat the registration check here and keep the selected store + // stable until the record and pending-operation claims are visible. + guard @workspaces.registered_dir(selected.workspace) is Some(registered) && + @workspaces.store_root(registered) == selected.root else { + raise EngineError( + "\{selected.workspace_token} is not a registered workspace", + ) } - guard root is Some(root) else { - if archived { + let root = selected.root + let source = if archived { @session_store.archived_root(root) } else { root } + guard @fsx.is_dir(source.join("sessions").join(session).to_path()) else { + // Name the state the caller can act on rather than reporting a bare + // miss. The twin in this same store is the common one — a stale client + // asking to archive what it already archived, or the reverse — and the + // record still being live elsewhere means the client named the wrong + // store, not that the conversation is gone. + if @fsx.is_dir( + (if archived { root } else { @session_store.archived_root(root) }) + .join("sessions") + .join(session) + .to_path(), + ) { raise EngineError( - if selected is Some(_) { - "no archived record for this conversation in the selected store" + if archived { + "this conversation is not archived" } else { - "no archived record for this conversation" + "this conversation is already archived" }, ) } - // Name the state the caller can act on. A record already sitting in an - // archived twin is not "missing" — the stale client just resyncs; and a - // conversation whose workspace detached needs that workspace re-attached, - // not a hunt for a lost record. - if @session_store.archived_root_of(session) is Some(_) { - raise EngineError("this conversation is already archived") - } - guard @workspaces.global_store_root() is Some(_) else { - raise EngineError( - "cannot determine a session root; set OPENSEEK_SESSION_ROOT", - ) - } raise EngineError( - "no durable record for this conversation in the global store or any attached workspace", + if archived { + "no archived record for this conversation in the selected store" + } else { + "no durable record for this conversation in the selected store" + }, ) } - let source = if archived { @session_store.archived_root(root) } else { root } let members = @session_store.family_in(source, session) - if archived && selected is Some(_) { + if archived { for session_id in members { guard !@fsx.is_dir(root.join("sessions").join(session_id).to_path()) else { raise EngineError( @@ -187,22 +171,8 @@ async fn EngineManager::claim_session_family_move( } } } - let (workspace, workspace_token) = if selected is Some(selected) { - (Some(selected.workspace), selected.workspace_token) - } else { - let mut workspace : @pathx.Absolute? = None - for project in @workspaces.registered() { - if @workspaces.store_root(project) == root { - workspace = Some(project) - break - } - } - let workspace_token = match workspace { - Some(project) => project.resource_path() - None => "" - } - (workspace, workspace_token) - } + let workspace = selected.workspace + let workspace_token = selected.workspace_token for session_id in members { let record_state = manager.claim_record_move(session_id) guards.push((session_id, record_state)) @@ -275,8 +245,8 @@ async fn SessionFamilyMove::remove_worktree_placements( self : SessionFamilyMove, on_committed? : (String, Array[WorktreeInfo]) -> Unit, ) -> Unit { - guard self.workspace is Some(workspace) else { return } - guard @worktree.drop_placements(workspace, self.members) is Some(remaining) else { + guard @worktree.drop_placements(self.workspace, self.members) + is Some(remaining) else { return } if on_committed is Some(on_committed) { @@ -382,10 +352,13 @@ pub async fn archived_sessions(manager : EngineManager) -> SessionsReply { /// the conversation in a blocked `MissingTree` state until the user rebuilds /// it. Committed work survives on the branch. A dirty checkout refuses unless /// `force`, which is how the client's discard-confirmation dialog answers; -/// `on_worktree_changed` broadcasts the checkout's disappearance. +/// `on_worktree_changed` broadcasts the checkout's disappearance. `workspace` +/// names the store holding the record, so a same-id record elsewhere is never +/// the one that moves. pub async fn archive_session( manager : EngineManager, session : String, + workspace : String, on_committed : (String, String) -> Unit, force? : Bool = false, on_worktree_changed? : (String, Array[WorktreeInfo]) -> Unit, @@ -397,6 +370,7 @@ pub async fn archive_session( let refusal = archive_session_commit( manager, session, + @session_store.StoreSelection::from_workspace(workspace), on_committed, force~, on_worktree_changed?, @@ -414,11 +388,16 @@ pub async fn archive_session( async fn archive_session_commit( manager : EngineManager, session : String, + selected : @session_store.StoreSelection, on_committed : (String, String) -> Unit, force? : Bool = false, on_worktree_changed? : (String, Array[WorktreeInfo]) -> Unit, ) -> ArchiveNeedsForceReply? { - let family = manager.claim_session_family_move(session, archived=false) + let family = manager.claim_session_family_move( + session, + archived=false, + selected~, + ) defer family.release() // The checkout goes with the conversation. A dirty refusal // lands here — after the idle engine already closed, which is harmless @@ -441,19 +420,25 @@ async fn archive_session_commit( ///| /// Restore one archived conversation family into its live store and return -/// the updated archived groups. The root record is looked up across every -/// store's `archived` twin — the global root first, then each registered -/// workspace — and descendant records beside it move in the same operation. +/// the updated archived groups. `workspace` names the store whose `archived` +/// twin holds the root record; descendant records beside it move in the same +/// operation. pub async fn unarchive_session( manager : EngineManager, session : String, + workspace : String, on_committed : (String, String) -> Unit, ) -> SessionsReply { let session = session.trim().to_owned() guard !session.is_empty() && @session_store.safe_session_id(session) else { raise EngineError("invalid session id") } - unarchive_session_commit(manager, session, on_committed) + unarchive_session_commit( + manager, + session, + @session_store.StoreSelection::from_workspace(workspace), + on_committed, + ) archived_sessions(manager) } @@ -461,9 +446,14 @@ pub async fn unarchive_session( async fn unarchive_session_commit( manager : EngineManager, session : String, + selected : @session_store.StoreSelection, on_committed : (String, String) -> Unit, ) -> Unit { - let family = manager.claim_session_family_move(session, archived=true) + let family = manager.claim_session_family_move( + session, + archived=true, + selected~, + ) defer family.release() move_session_family(family, to_archive=false, on_committed) } @@ -506,7 +496,7 @@ async fn delete_archived_session_commit( on_committed : (String, String) -> Unit, on_worktree_changed? : (String, Array[WorktreeInfo]) -> Unit, ) -> Unit { - let selected = @session_store.ArchivedStore::for_workspace(workspace) + let selected = @session_store.StoreSelection::from_workspace(workspace) let family = manager.claim_session_family_move( session, archived=true, @@ -665,16 +655,19 @@ async test "archive refuses a durable record while the pump is stopped" { defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-archive-stopped-") @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") - ignore(attach_run_workspace(Path(dir))) + let workspace = attach_run_workspace(Path(dir)) let store = dir + "/ws/.openseek" @fsx.ensure_dir(Path(store + "/sessions/s-stopped")) let manager = new_engine_manager("unused") let committed = Ref(false) let detail = try { ignore( - archive_session_commit(manager, "s-stopped", (_, _) => { - committed.val = true - }), + archive_session_commit( + manager, + "s-stopped", + @session_store.StoreSelection::from_workspace(workspace), + (_, _) => committed.val = true, + ), ) "no error" } catch { @@ -704,7 +697,7 @@ async test "archive publishes its move before a cancelled listing reply" { // conversation needs a real attached one; the env root now only locates the // registry that `attach_run_workspace` writes. @sys.set_env_var("OPENSEEK_SESSION_ROOT", root.join("global").to_string()) - ignore(attach_run_workspace(root)) + let workspace = attach_run_workspace(root) let store = root.join("ws/.openseek") @fsx.ensure_dir(store.join("sessions/s-commit")) @fs.write_file( @@ -719,7 +712,9 @@ async test "archive publishes its move before a cancelled listing reply" { @async.with_task_group(group => { let request = group.spawn(() => { ignore( - archive_session(manager, " s-commit ", (_, _) => committed.val = true), + archive_session(manager, " s-commit ", workspace, (_, _) => { + committed.val = true + }), ) }) while !@fsx.exists(Path(marker)) { @@ -751,7 +746,7 @@ async test "archive and restore move a subagent session family together" { defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-archive-family-") @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") - ignore(attach_run_workspace(Path(dir))) + let workspace = attach_run_workspace(Path(dir)) let store = dir + "/ws/.openseek" let live = store + "/sessions/" let archived = store + "/archived/sessions/" @@ -767,7 +762,12 @@ async test "archive and restore move a subagent session family together" { manager.pump = Serving(sessions=Map([])) let archived_events : Array[String] = [] assert_true( - archive_session_commit(manager, "chat", (id, _) => archived_events.push(id)) + archive_session_commit( + manager, + "chat", + @session_store.StoreSelection::from_workspace(workspace), + (id, _) => archived_events.push(id), + ) is None, ) assert_eq(archived_events.length(), family.length()) @@ -782,7 +782,12 @@ async test "archive and restore move a subagent session family together" { assert_true(@fsx.is_dir(Path(live + "chat-sr-x"))) assert_true(@fsx.is_dir(Path(live + "other"))) let restored_events : Array[String] = [] - unarchive_session_commit(manager, "chat", (id, _) => restored_events.push(id)) + unarchive_session_commit( + manager, + "chat", + @session_store.StoreSelection::from_workspace(workspace), + (id, _) => restored_events.push(id), + ) assert_eq(restored_events.length(), family.length()) assert_eq(restored_events[0], "chat") for session_id in family { @@ -968,7 +973,7 @@ async test "selected archived store is revalidated after detachment" { let manager = new_engine_manager("unused") manager.pump = Serving(sessions=Map([])) let workspace_token = workspace.resource_path() - let selected = @session_store.ArchivedStore::for_workspace(workspace_token) + let selected = @session_store.StoreSelection::from_workspace(workspace_token) ignore(detach_workspace(manager, workspace_token, fn(_) { () })) let detail = try { let family = manager.claim_session_family_move( @@ -999,7 +1004,7 @@ async test "a claimed child prevents a partial family archive" { defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-archive-family-busy-") @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") - ignore(attach_run_workspace(Path(dir))) + let workspace = attach_run_workspace(Path(dir)) let store = dir + "/ws/.openseek" @fsx.ensure_dir(Path(store + "/sessions/chat")) @fsx.ensure_dir(Path(store + "/sessions/chat-sr-1")) @@ -1013,7 +1018,14 @@ async test "a claimed child prevents a partial family archive" { ) defer child_claim.release(manager) let detail = try { - ignore(archive_session_commit(manager, "chat", (_, _) => ())) + ignore( + archive_session_commit( + manager, + "chat", + @session_store.StoreSelection::from_workspace(workspace), + (_, _) => (), + ), + ) "no error" } catch { EngineError(detail) => detail diff --git a/desktop/internal/engine/archive_lookup_wbtest.mbt b/desktop/internal/engine/archive_lookup_wbtest.mbt index 0abadb752..365b66b25 100644 --- a/desktop/internal/engine/archive_lookup_wbtest.mbt +++ b/desktop/internal/engine/archive_lookup_wbtest.mbt @@ -1,4 +1,4 @@ -// Archive's record lookup against stores a stale client can still name: a +// Archive's store selection against stores a stale client can still name: a // conversation row can outlive its workspace's registration (detach) or its // directory (external deletion), and a second archive click can race the // first one's commit. Each state must answer with its own actionable @@ -6,20 +6,22 @@ ///| #cfg(not(platform="windows")) -async test "archive names the missing store after a workspace detach" { +async test "archive names the detached workspace the client selected" { ambient_env_test_lock.acquire() defer ambient_env_test_lock.release() let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-archive-detach-") let workspace = dir + "/ws" + let other = dir + "/other" let global_root = dir + "/global" @sys.set_env_var("OPENSEEK_SESSION_ROOT", global_root) @fsx.ensure_dir(Path(global_root)) @fsx.ensure_dir(Path(workspace + "/.openseek/sessions/s-ws")) + @fsx.ensure_dir(Path(other + "/.openseek")) @fs.write_file( global_root + "/workspaces.json", - ({ "workspaces": [workspace] } : Json).stringify(), + ({ "workspaces": [workspace, other] } : Json).stringify(), create_mode=CreateOrTruncate, ) let manager = new_engine_manager("unused") @@ -27,24 +29,20 @@ async test "archive names the missing store after a workspace detach" { // The sidebar's remove button: detach commits and the registry forgets // the workspace, while the record stays on disk in the project store. let detached = detach_workspace(manager, workspace, fn(_) { }) - assert_true(detached is Workspaces([])) - // A stale sidebar can still offer the conversation; archiving it now - // reports the record as unreachable instead of pretending it never - // existed. + assert_true(detached is Workspaces([_])) + // A stale sidebar can still offer the conversation, selecting the store it + // last saw. Naming a store the host no longer owns is refused by name + // rather than silently redirected to whatever else holds the id. + let selected = @pathx.Path(workspace).resolve().resource_path() let detail = try { - ignore(archive_session(manager, "s-ws", (_, _) => ())) + ignore(archive_session(manager, "s-ws", selected, (_, _) => ())) "no error" } catch { EngineError(detail) => detail error if @async.is_being_cancelled() => raise error error => "\{error}" } - debug_inspect( - detail, - content=( - #|"no durable record for this conversation in the global store or any attached workspace" - ), - ) + assert_true(detail.has_suffix("is not a registered workspace")) assert_true(@fsx.is_dir(Path(workspace + "/.openseek/sessions/s-ws"))) // The failed attempt must leave no wedged host state behind: no record // move claim, no pending slot, and the next attempt reports the same @@ -52,8 +50,18 @@ async test "archive names the missing store after a workspace detach" { assert_true(manager.record_guards.get("s-ws") is None) guard manager.pump is Serving(sessions~) else { fail("pump stopped") } assert_true(sessions.get("s-ws") is None) - let second = try { - ignore(archive_session(manager, "s-ws", (_, _) => ())) + // The record is not reachable through another attached store either: + // selection is exact, so the id alone opens no back door into a store that + // does not hold it. + let other_attempt = try { + ignore( + archive_session( + manager, + "s-ws", + @pathx.Path(other).resolve().resource_path(), + (_, _) => (), + ), + ) "no error" } catch { EngineError(detail) => detail @@ -61,11 +69,13 @@ async test "archive names the missing store after a workspace detach" { error => "\{error}" } debug_inspect( - second, + other_attempt, content=( - #|"no durable record for this conversation in the global store or any attached workspace" + #|"no durable record for this conversation in the selected store" ), ) + assert_true(@fsx.is_dir(Path(workspace + "/.openseek/sessions/s-ws"))) + assert_true(manager.record_guards.get("s-ws") is None) @fs.rmdir(dir, recursive=true) } @@ -93,7 +103,14 @@ async test "archive reports a workspace record whose directory vanished" { // its dead entry addressable, so the sidebar still shows the workspace. @fs.rmdir(workspace, recursive=true) let detail = try { - ignore(archive_session(manager, "s-ws", (_, _) => ())) + ignore( + archive_session( + manager, + "s-ws", + @pathx.Path(workspace).resolve().resource_path(), + (_, _) => (), + ), + ) "no error" } catch { EngineError(detail) => detail @@ -103,7 +120,7 @@ async test "archive reports a workspace record whose directory vanished" { debug_inspect( detail, content=( - #|"no durable record for this conversation in the global store or any attached workspace" + #|"no durable record for this conversation in the selected store" ), ) @fs.rmdir(dir, recursive=true) @@ -118,24 +135,46 @@ async test "loading a conversation from a detached workspace names that state" { defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-load-detached-") let workspace = dir + "/ws" + let other = dir + "/other" let global_root = dir + "/global" @sys.set_env_var("OPENSEEK_SESSION_ROOT", global_root) @fsx.ensure_dir(Path(global_root)) @fsx.ensure_dir(Path(workspace + "/.openseek/sessions/s-ws")) + @fsx.ensure_dir(Path(other + "/.openseek")) @fs.write_file( global_root + "/workspaces.json", - ({ "workspaces": [workspace] } : Json).stringify(), + ({ "workspaces": [workspace, other] } : Json).stringify(), create_mode=CreateOrTruncate, ) let manager = new_engine_manager("unused") manager.pump = Serving(sessions=Map([])) let detached = detach_workspace(manager, workspace, fn(_) { }) - assert_true(detached is Workspaces([])) - // A stale client's transcript reload names the conversation without a - // workspace hint; the reply must say the record is unreachable rather - // than surface the engine's raw file error. + assert_true(detached is Workspaces([_])) + // A stale client's transcript reload still names the store it last saw; + // the reply must say that store is gone rather than read the global one or + // surface the engine's raw file error. let detail = try { - ignore(load_session(manager, { session: "s-ws", workspace: None })) + ignore( + load_session(manager, { + session: "s-ws", + workspace: @pathx.Path(workspace).resolve().resource_path(), + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(detail.has_suffix("is not a registered workspace")) + // Nor does another attached store answer for the id it never held. + let other_attempt = try { + ignore( + load_session(manager, { + session: "s-ws", + workspace: @pathx.Path(other).resolve().resource_path(), + }), + ) "no error" } catch { EngineError(detail) => detail @@ -143,14 +182,66 @@ async test "loading a conversation from a detached workspace names that state" { error => "\{error}" } debug_inspect( - detail, + other_attempt, content=( - #|"no attached workspace holds this conversation; attach its project first" + #|"no durable record for this conversation in the selected store" ), ) @fs.rmdir(dir, recursive=true) } +///| +#cfg(not(platform="windows")) +async test "archive moves the selected store's record, not the same id elsewhere" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + defer restore_session_root_env(previous) + let dir = @fs.tmpdir(prefix="openseek-archive-same-id-") + let workspace = dir + "/ws" + let other = dir + "/other" + let global_root = dir + "/global" + @sys.set_env_var("OPENSEEK_SESSION_ROOT", global_root) + @fsx.ensure_dir(Path(global_root)) + // The same session id in two attached projects — what the sidebar renders + // as two rows. Registration order decides which one a search would find + // first, so archiving without a store would always move that one. + @fsx.ensure_dir(Path(other + "/.openseek/sessions/s-both")) + @fsx.ensure_dir(Path(workspace + "/.openseek/sessions/s-both")) + @fs.write_file( + global_root + "/workspaces.json", + ({ "workspaces": [other, workspace] } : Json).stringify(), + create_mode=CreateOrTruncate, + ) + let manager = new_engine_manager("unused") + manager.pump = Serving(sessions=Map([])) + let moved : Array[(String, String)] = [] + let selected = @pathx.Path(workspace).resolve().resource_path() + assert_true( + archive_session(manager, "s-both", selected, (id, store) => { + moved.push((id, store)) + }) + is Archived(_), + ) + // Only the selected project's record moved, and the notification names its + // store, so no client applies the disposition to the other project's row. + assert_eq(moved.length(), 1) + assert_eq(moved[0].0, "s-both") + assert_true(moved[0].1.has_suffix("/ws")) + assert_false(@fsx.exists(Path(workspace + "/.openseek/sessions/s-both"))) + assert_true( + @fsx.is_dir(Path(workspace + "/.openseek/archived/sessions/s-both")), + ) + assert_true(@fsx.is_dir(Path(other + "/.openseek/sessions/s-both"))) + assert_false(@fsx.exists(Path(other + "/.openseek/archived/sessions/s-both"))) + // Unarchiving restores the selected record beside the untouched one in the + // other project instead of colliding with it. + ignore(unarchive_session(manager, "s-both", selected, (_, _) => ())) + assert_true(@fsx.is_dir(Path(workspace + "/.openseek/sessions/s-both"))) + assert_true(@fsx.is_dir(Path(other + "/.openseek/sessions/s-both"))) + @fs.rmdir(dir, recursive=true) +} + ///| #cfg(not(platform="windows")) async test "archiving an already-archived conversation names that state" { @@ -160,13 +251,13 @@ async test "archiving an already-archived conversation names that state" { defer restore_session_root_env(previous) let dir = @fs.tmpdir(prefix="openseek-archive-again-") @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") - ignore(attach_run_workspace(Path(dir))) + let workspace = attach_run_workspace(Path(dir)) let store = dir + "/ws/.openseek" @fsx.ensure_dir(Path(store + "/archived/sessions/s-done")) let manager = new_engine_manager("unused") manager.pump = Serving(sessions=Map([])) let detail = try { - ignore(archive_session(manager, "s-done", (_, _) => ())) + ignore(archive_session(manager, "s-done", workspace, (_, _) => ())) "no error" } catch { EngineError(detail) => detail diff --git a/desktop/internal/engine/archive_submodule_wbtest.mbt b/desktop/internal/engine/archive_submodule_wbtest.mbt index 2e480a2ef..7942847e5 100644 --- a/desktop/internal/engine/archive_submodule_wbtest.mbt +++ b/desktop/internal/engine/archive_submodule_wbtest.mbt @@ -58,13 +58,21 @@ async test "archive deletes a submodule checkout that git worktree remove refuse b"x", create_mode=CreateOrTruncate, ) - guard archive_session(manager, "s-sub-dirty", (_, _) => ()) + guard archive_session(manager, "s-sub-dirty", repo.resource_path(), (_, _) => { + () + }) is NeedsForce(refusal) else { fail("dirty submodule checkout must ask for confirmation") } assert_eq(refusal.worktree, "wt-1") assert_eq(refusal.dirty_paths, ["junk.txt"]) - guard archive_session(manager, "s-sub-dirty", (_, _) => (), force=true) + guard archive_session( + manager, + "s-sub-dirty", + repo.resource_path(), + (_, _) => (), + force=true, + ) is Archived(_) else { fail("forced archive must delete the submodule checkout") } @@ -84,7 +92,10 @@ async test "archive deletes a submodule checkout that git worktree remove refuse ) let clean_checkout = repo.join(".worktrees/wt-2") assert_true(@fsx.exists(clean_checkout.join("deps/one/.git").to_path())) - guard archive_session(manager, "s-sub-clean", (_, _) => ()) is Archived(_) else { + guard archive_session(manager, "s-sub-clean", repo.resource_path(), (_, _) => { + () + }) + is Archived(_) else { fail("clean submodule checkout must archive without confirmation") } assert_false(@fsx.exists(clean_checkout.to_path())) @@ -168,7 +179,15 @@ async test "worktree deletion refuses a replaced checkout" { } assert_true(remove_detail != "no error") let archive_detail = try { - ignore(archive_session(manager, "s-replaced", (_, _) => (), force=true)) + ignore( + archive_session( + manager, + "s-replaced", + repo.resource_path(), + (_, _) => (), + force=true, + ), + ) "no error" } catch { error if @async.is_being_cancelled() => raise error diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index f7cf7bef4..c494aa84f 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -569,23 +569,10 @@ pub async fn load_session( // B's archive after a host restart. let record_guard = manager.begin_record_read(session) defer manager.end_record_read(session, record_guard) - // The client's workspace hint says which registered store to read. A - // detached workspace is rejected instead of silently probing another - // store, which could return a different history for the same session id. - let root = if payload.workspace is Some(hint) && !hint.is_blank() { - guard @pathx.Absolute::from_uri_path(hint) is Some(absolute) && - @workspaces.registered_dir(absolute) is Some(workspace) else { - raise EngineError("\{hint} is not a registered workspace") - } - Some(@workspaces.store_root(workspace)) - } else { - @session_store.root_of(session) - } - guard root is Some(root) else { - raise EngineError( - "no attached workspace holds this conversation; attach its project first", - ) - } + // The client names which store to read. A detached workspace is rejected + // instead of silently probing another store, which could return a + // different history for the same session id. + let root = @session_store.StoreSelection::from_workspace(payload.workspace).root // A followed session (a live engine is writing to it) is read through its // follower actor: the snapshot then comes out of the same serial loop // that broadcasts `session.event` commits, so the two can never disagree @@ -598,15 +585,20 @@ pub async fn load_session( } } // Name the missing-record states before spending an engine read that can - // only fail with a raw file error: a stale client asks for conversations - // whose store just detached (or whose record moved to an archived twin), - // and the answer should say that, not ENOENT. + // only fail with a raw file error: a stale client asks for a conversation + // whose record moved to this store's archived twin, and the answer should + // say that, not ENOENT. if !@fsx.is_dir(root.join("sessions").join(session).to_path()) { - if @session_store.archived_root_of(session) is Some(_) { + if @fsx.is_dir( + @session_store.archived_root(root) + .join("sessions") + .join(session) + .to_path(), + ) { raise EngineError("this conversation is archived; unarchive it first") } raise EngineError( - "no durable record for this conversation in any attached workspace", + "no durable record for this conversation in the selected store", ) } let output = session_show_stdout(manager, session, root) catch { @@ -636,35 +628,17 @@ pub async fn load_archived_session( } let record_guard = manager.begin_record_read(session) defer manager.end_record_read(session, record_guard) - // A workspace hint selects that registered store's archived twin. Without - // one, find the archived record across all attached stores and the global - // store, matching the placement rules of the archived index. - let root = if payload.workspace is Some(hint) && !hint.is_blank() { - guard @pathx.Absolute::from_uri_path(hint) is Some(absolute) && - @workspaces.registered_dir(absolute) is Some(workspace) else { - raise EngineError("\{hint} is not a registered workspace") - } - Some(@session_store.archived_root(@workspaces.store_root(workspace))) - } else { - match @session_store.archived_root_of(session) { - Some(root) => Some(@session_store.archived_root(root)) - None => None - } - } - guard root is Some(root) else { - if @session_store.live_root_of(session) is Some(_) { + // The client names the store whose archived twin to read. + let store = @session_store.StoreSelection::from_workspace(payload.workspace).root + let root = @session_store.archived_root(store) + if !@fsx.is_dir(root.join("sessions").join(session).to_path()) { + if @fsx.is_dir(store.join("sessions").join(session).to_path()) { raise EngineError("this conversation is not archived") } raise EngineError( - "no archived record for this conversation in the global store or any attached workspace", + "no archived record for this conversation in the selected store", ) } - if !@fsx.is_dir(root.join("sessions").join(session).to_path()) { - if @session_store.live_root_of(session) is Some(_) { - raise EngineError("this conversation is not archived") - } - raise EngineError("no archived record for this conversation") - } let output = session_show_stdout(manager, session, root) catch { EngineError(detail) => raise EngineError("archived session load failed: " + detail) @@ -1411,9 +1385,9 @@ async test "session load preserves a future item without hiding known events" { let engine = dir + "/engine.sh" @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") defer restore_session_root_env(previous) - // A hint-less load resolves the store from where the record actually - // lives, so the record goes in an attached workspace's own store. - ignore(attach_run_workspace(Path(dir))) + // The load names the store it reads, so the record goes in an attached + // workspace's own store and the request spells that workspace. + let workspace = attach_run_workspace(Path(dir)) @fsx.ensure_dir(Path(dir + "/ws/.openseek/sessions/s-future")) @fs.write_file( engine, @@ -1423,7 +1397,7 @@ async test "session load preserves a future item without hiding known events" { assert_eq(@process.run("chmod", ["+x", engine]), 0) let reply = load_session(new_engine_manager(engine), { session: "s-future", - workspace: None, + workspace, }) guard reply.session.events is Some(events) else { fail("expected session events") @@ -1444,9 +1418,9 @@ async test "archived session load reads without restoring the record" { let engine = dir + "/engine.sh" @sys.set_env_var("OPENSEEK_SESSION_ROOT", dir + "/global") defer restore_session_root_env(previous) - // A hint-less load resolves the store from where the record actually - // lives, so the record goes in an attached workspace's own store. - ignore(attach_run_workspace(Path(dir))) + // The load names the store it reads, so the record goes in an attached + // workspace's own store and the request spells that workspace. + let workspace = attach_run_workspace(Path(dir)) let store = dir + "/ws/.openseek" @fsx.ensure_dir(Path(store + "/archived/sessions/s-archived")) @fs.write_file( @@ -1458,7 +1432,7 @@ async test "archived session load reads without restoring the record" { let manager = new_engine_manager(engine) let reply = load_archived_session(manager, { session: "s-archived", - workspace: None, + workspace, }) guard reply.session.events is Some(events) else { fail("expected archived session events") @@ -1494,7 +1468,7 @@ async test "load rejects a detached workspace hint instead of probing global" { ignore( load_session(manager, { session: "s-detached", - workspace: Some(dir + "/detached-workspace"), + workspace: dir + "/detached-workspace", }), ) "no error" diff --git a/desktop/internal/engine/pkg.generated.mbti b/desktop/internal/engine/pkg.generated.mbti index f1010b57a..7cf20a339 100644 --- a/desktop/internal/engine/pkg.generated.mbti +++ b/desktop/internal/engine/pkg.generated.mbti @@ -9,7 +9,7 @@ import { } // Values -pub async fn archive_session(EngineManager, String, (String, String) -> Unit, force? : Bool, on_worktree_changed? : (String, Array[@protocol.WorktreeInfo]) -> Unit) -> @protocol.ArchiveReply +pub async fn archive_session(EngineManager, String, String, (String, String) -> Unit, force? : Bool, on_worktree_changed? : (String, Array[@protocol.WorktreeInfo]) -> Unit) -> @protocol.ArchiveReply pub async fn archived_sessions(EngineManager) -> @protocol.SessionsReply @@ -47,7 +47,7 @@ pub async fn start_run(EventSink, EngineManager, @protocol.StartPayload) -> @pro pub fn steer_run(EngineManager, @protocol.SteerPayload) -> @protocol.SteerReply -pub async fn unarchive_session(EngineManager, String, (String, String) -> Unit) -> @protocol.SessionsReply +pub async fn unarchive_session(EngineManager, String, String, (String, String) -> Unit) -> @protocol.SessionsReply pub async fn update_settings(EngineManager, SettingsPatch, legacy_migration? : Bool, on_committed? : (@protocol.SettingsStatusPayload) -> Unit) -> @protocol.SettingsStatusPayload diff --git a/desktop/internal/engine/worktree_seam_wbtest.mbt b/desktop/internal/engine/worktree_seam_wbtest.mbt index 32669086e..cea2cdcc7 100644 --- a/desktop/internal/engine/worktree_seam_wbtest.mbt +++ b/desktop/internal/engine/worktree_seam_wbtest.mbt @@ -515,7 +515,8 @@ async test "archive keeps worktree placement for explicit repair after unarchive ) // A dirty checkout returns a structured confirmation state before anything // irreversible: the record stays live and the checkout stays on disk. - guard archive_session(manager, "s-arc", (_, _) => ()) is NeedsForce(refusal) else { + guard archive_session(manager, "s-arc", repo.resource_path(), (_, _) => ()) + is NeedsForce(refusal) else { fail("dirty archive returned no force confirmation") } assert_eq(refusal.worktree, "wt-1") @@ -531,10 +532,14 @@ async test "archive keeps worktree placement for explicit repair after unarchive // the branch, archived history, and placement row all survive. The retained // row broadcasts as missing before the conversation is archived. let changes : Array[(String, Array[WorktreeInfo])] = [] - guard archive_session(manager, "s-arc", (_, _) => (), force=true, on_worktree_changed=( - workspace, - rows, - ) => changes.push((workspace, rows))) + guard archive_session( + manager, + "s-arc", + repo.resource_path(), + (_, _) => (), + force=true, + on_worktree_changed=(workspace, rows) => changes.push((workspace, rows)), + ) is Archived(_) else { fail("forced archive did not commit") } @@ -568,7 +573,9 @@ async test "archive keeps worktree placement for explicit repair after unarchive assert_false(retained.present) // Unarchive restores only the durable conversation. The retained placement // keeps every workspace surface blocked until the user explicitly repairs. - ignore(unarchive_session(manager, "s-arc", (_, _) => ())) + ignore( + unarchive_session(manager, "s-arc", repo.resource_path(), (_, _) => ()), + ) assert_true( @fsx.is_dir( @workspaces.store_root(repo).join("sessions").join("s-arc").to_path(), @@ -599,7 +606,10 @@ async test "archive keeps worktree placement for explicit repair after unarchive @fsx.ensure_dir( @workspaces.store_root(repo).join("sessions").join("s-arc2").to_path(), ) - assert_true(archive_session(manager, "s-arc2", (_, _) => ()) is Archived(_)) + assert_true( + archive_session(manager, "s-arc2", repo.resource_path(), (_, _) => ()) + is Archived(_), + ) assert_false(@fsx.exists(repo.join(".worktrees/wt-2").to_path())) guard @worktree.list(repo.to_string()).worktrees is [first, second] else { fail("both worktree placements must survive archive") diff --git a/desktop/internal/protocol/desktop_messages.mbt b/desktop/internal/protocol/desktop_messages.mbt index 3a4370081..b8335ceee 100644 --- a/desktop/internal/protocol/desktop_messages.mbt +++ b/desktop/internal/protocol/desktop_messages.mbt @@ -79,28 +79,35 @@ pub(all) struct GoalPayload { workspace : String? } derive(Debug, Eq, FromJson, ToJson) +// Session ids are unique only within one durable store, so every op that +// addresses an existing record names the store beside the id: the +// host-reported project resource path, the same spelling `session.changed` +// and the sidebar's listings report. The host validates it against its +// registry rather than trusting a client to name an arbitrary directory, and +// selects that store exactly instead of searching, so a same-id record in +// another store can never be read or moved by mistake. + ///| +/// `session.load` / `session.load_archived` read one durable record in one +/// exact store. pub(all) struct LoadSessionPayload { session : String - // The workspace holding the session, when the client already knows it. - workspace : String? + workspace : String } derive(Debug, Eq, FromJson, ToJson) ///| -/// `session.archive` / `session.unarchive` address a conversation by its -/// durable session id. Archiving takes the conversation's worktree with -/// it; `force` (archive only) discards that checkout's uncommitted -/// changes — the client sends it after its confirmation dialog. +/// `session.archive` / `session.unarchive` move one durable record between +/// one exact store and its archived twin. Archiving takes the conversation's +/// worktree with it; `force` (archive only) discards that checkout's +/// uncommitted changes — the client sends it after its confirmation dialog. pub(all) struct SessionPayload { session : String + workspace : String force : Bool? } derive(Debug, Eq, FromJson, ToJson) ///| /// Permanent deletion addresses one archived record in one exact store. -/// `workspace` is the host-reported project resource path; the host validates -/// it against its registry instead of trusting the client to choose an -/// arbitrary directory. pub(all) struct ArchivedSessionPayload { session : String workspace : String diff --git a/desktop/internal/protocol/pkg.generated.mbti b/desktop/internal/protocol/pkg.generated.mbti index 856fee1fd..7bc7aa100 100644 --- a/desktop/internal/protocol/pkg.generated.mbti +++ b/desktop/internal/protocol/pkg.generated.mbti @@ -605,7 +605,7 @@ pub(all) struct ListAppsReply { pub(all) struct LoadSessionPayload { session : String - workspace : String? + workspace : String } derive(Eq, ToJson, @debug.Debug, @json.FromJson) pub(all) struct LoadSessionReply { @@ -904,6 +904,7 @@ pub impl @json.FromJson for SessionListEntry pub(all) struct SessionPayload { session : String + workspace : String force : Bool? } derive(Eq, ToJson, @debug.Debug, @json.FromJson) diff --git a/desktop/internal/session_store/pkg.generated.mbti b/desktop/internal/session_store/pkg.generated.mbti index 9b222dfe7..947e1e9dd 100644 --- a/desktop/internal/session_store/pkg.generated.mbti +++ b/desktop/internal/session_store/pkg.generated.mbti @@ -19,8 +19,6 @@ pub async fn family_in(@pathx.Absolute, String) -> Array[String] pub async fn known_roots() -> Array[@pathx.Absolute] -pub async fn live_root_of(String) -> @pathx.Absolute? - pub async fn root_of(String) -> @pathx.Absolute? pub fn safe_session_id(String) -> Bool @@ -33,12 +31,12 @@ pub suberror StoreError { } derive(@debug.Debug) // Types and methods -pub struct ArchivedStore { +pub struct StoreSelection { root : @pathx.Absolute workspace : @pathx.Absolute workspace_token : String } -pub async fn ArchivedStore::for_workspace(String) -> Self +pub async fn StoreSelection::from_workspace(String) -> Self // Type aliases diff --git a/desktop/internal/session_store/store.mbt b/desktop/internal/session_store/store.mbt index 65a0deadb..328b56478 100644 --- a/desktop/internal/session_store/store.mbt +++ b/desktop/internal/session_store/store.mbt @@ -48,10 +48,10 @@ pub fn deleting_root(root : @pathx.Absolute) -> @pathx.Absolute { } ///| -/// Every store that can own a session id — one per attached workspace. -/// Correlation and stale-client defenses must search all of them: request -/// hints are optional and cannot be trusted to identify where an existing -/// record lives. +/// Every store that can own a session id — one per attached workspace. Ops +/// that address one record name its store and select it exactly; this is for +/// the whole-host sweeps and for the archived-correlation defense, whose +/// request carries no store at all. pub async fn known_roots() -> Array[@pathx.Absolute] { let roots : Array[@pathx.Absolute] = [] for path in @workspaces.registered() { @@ -100,19 +100,20 @@ pub async fn root_of(session : String) -> @pathx.Absolute? { } ///| -/// The exact archived store selected by a host-produced sidebar row. Every -/// durable record lives in a registered workspace's store, and paths are -/// accepted only while that workspace is registered. Naming the store this -/// exactly is what prevents a same-ID record in another one from being chosen -/// by search order. -pub struct ArchivedStore { +/// The exact durable store a client's request names: a registered workspace's +/// in-project store, accepted only while that workspace is registered. Every +/// op that addresses an existing record goes through this, so a same-id record +/// in another store can never be chosen by search order. +pub struct StoreSelection { root : @pathx.Absolute workspace : @pathx.Absolute workspace_token : String } ///| -pub async fn ArchivedStore::for_workspace(workspace : String) -> ArchivedStore { +pub async fn StoreSelection::from_workspace( + workspace : String, +) -> StoreSelection { guard @pathx.Absolute::from_uri_path(workspace) is Some(path) && @workspaces.registered_dir(path) is Some(registered) else { raise StoreError("\{workspace} is not a registered workspace") @@ -131,8 +132,10 @@ pub async fn ArchivedStore::for_workspace(workspace : String) -> ArchivedStore { } ///| -/// The store root whose archived `sessions/` holds this conversation's -/// durable record. +/// Any store whose `archived` twin holds this id. Unlike the ops that move or +/// read one named record, the archived-correlation defense guarding a start +/// has no store to go on: it guards a request that carries only an id, so it +/// must search. pub async fn archived_root_of(session : String) -> @pathx.Absolute? { guard !session.is_empty() else { return None } for root in known_roots() { @@ -143,22 +146,6 @@ pub async fn archived_root_of(session : String) -> @pathx.Absolute? { None } -///| -/// The store root whose live `sessions/` holds this conversation's -/// durable record, searched like `archived_root_of`. Archive must find the -/// record where it actually lives: a stale client can name a conversation long -/// after its workspace detached, and the session id alone says nothing about -/// which store owns it. -pub async fn live_root_of(session : String) -> @pathx.Absolute? { - guard !session.is_empty() else { return None } - for root in known_roots() { - if @fsx.is_dir(root.join("sessions").join(session).to_path()) { - return Some(root) - } - } - None -} - ///| pub async fn ensure_not_archived(session : String) -> Unit { if archived_root_of(session) is Some(_) { From d78ef88c9337b9a1655591b92c91302abe062f81 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 19:34:05 +0800 Subject: [PATCH 02/17] fix(desktop): act on the sidebar row's own durable record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live rows carried their store in the component id (so two same-id rows keep distinct keys) but dropped it everywhere downstream: `OpenSession`, `ArchiveSession`, and `UnarchiveSession` travelled as `(channel, session)`. Clicking one project's row could therefore open the other's record — `session_project` resolved the id against the first same-id listing — and both rows projected the same conversation's active/running state. `ArchiveRecordKey` becomes `RecordKey`, since it is no longer only about archiving: it is the durable identity `(store, session)` that opening, archiving, restoring, deleting, and their notifications all carry. The sidebar dispatcher rebuilds it from the clicked row's own `root`, so a row acts on the record its group renders. What this deliberately does not change: the page still holds at most one open conversation per `(channel, session id)`, because the host runs at most one engine per session id — `Model::find_conversation` stays the id-keyed lookup that live run events route through. The store says which record that conversation is bound to. Opening the other store's row rebinds it, exactly as opening an archived twin already did, and `find_record_conversation` is what everything asking about a *record* uses instead: a row now reads live state only from a conversation bound to its own store, so the idle row stays idle while the project record runs. `session.load` and the archive ops now always name the store, and the discard-confirmation dialog retains the record the refused attempt named so its forced retry cannot land in a different store. --- desktop/frontend/archive.mbt | 33 ++- desktop/frontend/boot.mbt | 32 ++- desktop/frontend/bridge.mbt | 28 ++- desktop/frontend/composer_input.mbt | 2 +- desktop/frontend/console.mbt | 12 +- desktop/frontend/model.mbt | 95 ++++++--- desktop/frontend/update.mbt | 305 ++++++++++++++++++++-------- desktop/frontend/view.mbt | 98 +++++++-- desktop/frontend/workspaces.mbt | 4 +- desktop/frontend/worktrees.mbt | 69 ++++++- 10 files changed, 515 insertions(+), 163 deletions(-) diff --git a/desktop/frontend/archive.mbt b/desktop/frontend/archive.mbt index 717144830..29ec81e3c 100644 --- a/desktop/frontend/archive.mbt +++ b/desktop/frontend/archive.mbt @@ -13,7 +13,7 @@ /// the dialog appear to authorize a different conversation. priv struct ArchivedDeleteConfirm { channel : @interop.ChannelId - key : ArchiveRecordKey + key : RecordKey // The parent plus every descendant visible in the same archived snapshot. // Host notifications still cover a child created after that snapshot. sessions : Array[String] @@ -57,16 +57,19 @@ fn fetch_archived( fn archive_session( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, - session : String, + key : RecordKey, connection_generation : Int, sessions_request_generation : Int, archived_request_generation : Int, force? : Bool = false, ) -> @cmd.Cmd { + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none + } @cmd.custom_cmd(scheduler => { @js.async_run(() => { archive_session_into( - scheduler, dispatch, channel, session, connection_generation, sessions_request_generation, + scheduler, dispatch, channel, key, workspace, connection_generation, sessions_request_generation, archived_request_generation, force, ) }) @@ -78,7 +81,8 @@ async fn archive_session_into( scheduler : &@cmd.Scheduler, dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, - session : String, + key : RecordKey, + workspace : String, connection_generation : Int, sessions_request_generation : Int, archived_request_generation : Int, @@ -90,7 +94,15 @@ async fn archive_session_into( // Archiving removes the conversation's checkout but retains its placement; // the retry after the discard-confirmation dialog carries force on the // wire. - { session, force: if force { Some(true) } else { None } }, + { + session: key.session, + workspace, + force: if force { + Some(true) + } else { + None + }, + }, ) catch { error => { scheduler.add( @@ -105,7 +117,7 @@ async fn archive_session_into( NeedsForce(refusal) => scheduler.add( dispatch.map((msg : DeviceMsg) => FromDevice(channel, msg))( - ArchiveNeedsForce(session~, refusal~), + ArchiveNeedsForce(key~, refusal~), ), ) Archived(reply) => @@ -120,16 +132,19 @@ async fn archive_session_into( fn unarchive_session( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, - session : String, + key : RecordKey, connection_generation : Int, sessions_request_generation : Int, archived_request_generation : Int, ) -> @cmd.Cmd { + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none + } archive_action( dispatch, channel, @commands.session_unarchive, - { session, force: None }, + { session: key.session, workspace, force: None }, connection_generation, sessions_request_generation, archived_request_generation, @@ -143,7 +158,7 @@ fn unarchive_session( fn delete_archived_session( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, - key : ArchiveRecordKey, + key : RecordKey, sessions : Array[String], connection_generation : Int, archived_request_generation : Int, diff --git a/desktop/frontend/boot.mbt b/desktop/frontend/boot.mbt index 9a003b8f7..f3a613000 100644 --- a/desktop/frontend/boot.mbt +++ b/desktop/frontend/boot.mbt @@ -161,7 +161,11 @@ fn SidebarDispatcher::conversation_open( return self.unroutable("open", "channel \{id.channel()}") } match id.source() { - "openseek.live" => self.emit(OpenSession(channel, id.conversation())) + "openseek.live" => + match row_record_key(channel, id) { + Some(key) => self.emit(OpenSession(channel, key)) + None => self.unroutable("open", "live row with no store") + } "openseek.archived" => { // The row carries the store its record lives in; without one the click // would name a session id in no particular store. @@ -179,6 +183,21 @@ fn SidebarDispatcher::conversation_open( } } +///| +/// The durable record a sidebar row stands for. Its `root` is the store its +/// group renders, so the row acts on that record even when another store +/// holds one with the same session id. +fn row_record_key( + channel : @interop.ChannelId, + id : @conversation.Id, +) -> RecordKey? { + guard id.root() is Some(root) && + @resource.of_path(channel, root) is Some(workspace) else { + return None + } + Some({ session: id.conversation(), workspace }) +} + ///| fn SidebarDispatcher::conversation_archive( self : SidebarDispatcher, @@ -188,7 +207,11 @@ fn SidebarDispatcher::conversation_archive( return self.unroutable("archive", "channel \{id.channel()}") } match id.source() { - "openseek.live" => self.emit(ArchiveSession(channel, id.conversation())) + "openseek.live" => + match row_record_key(channel, id) { + Some(key) => self.emit(ArchiveSession(channel, key)) + None => self.unroutable("archive", "live row with no store") + } "codex.live" => self.emit(CodexArchiveThread(id.conversation())) source => self.unroutable("archive", "\{source}@\{id.channel()}") } @@ -204,7 +227,10 @@ fn SidebarDispatcher::conversation_restore( } match id.source() { "openseek.archived" => - self.emit(UnarchiveSession(channel, id.conversation())) + match row_record_key(channel, id) { + Some(key) => self.emit(UnarchiveSession(channel, key)) + None => self.unroutable("restore", "archived row with no store") + } "codex.archived" => self.emit(CodexUnarchiveThread(id.conversation())) source => self.unroutable("restore", "\{source}@\{id.channel()}") } diff --git a/desktop/frontend/bridge.mbt b/desktop/frontend/bridge.mbt index 636ddf700..186df1ac7 100644 --- a/desktop/frontend/bridge.mbt +++ b/desktop/frontend/bridge.mbt @@ -256,11 +256,7 @@ fn session_changed_msg( channel : @interop.ChannelId, payload : @protocol.SessionChangedPayload, ) -> DeviceMsg? { - guard ArchiveRecordKey::from_protocol( - channel, - payload.session, - payload.workspace, - ) + guard RecordKey::from_protocol(channel, payload.session, payload.workspace) is Some(key) else { return None } @@ -1202,13 +1198,14 @@ async fn refresh_sessions_into( ///| /// Load a durable session's transcript so the conversation can be resumed. -/// `workspace` says which registered store holds it; `None` selects the -/// default store. The protocol conversion happens only in `load_session_cmd`. +/// `workspace` is the store holding the record — a record is addressed by +/// `(store, session)`, never by id alone. The protocol conversion happens +/// only in `load_session_cmd`. fn open_session( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, session_id : String, - workspace~ : @common.Uri?, + workspace~ : @common.Uri, generation~ : Int, archived? : Bool = false, ) -> @cmd.Cmd { @@ -1255,7 +1252,7 @@ fn refresh_session( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, session_id : String, - workspace~ : @common.Uri?, + workspace~ : @common.Uri, generation~ : Int, archived? : Bool = false, ) -> @cmd.Cmd { @@ -1288,15 +1285,14 @@ fn refresh_session( fn load_session_cmd( channel : @interop.ChannelId, session_id : String, - workspace~ : @common.Uri?, + workspace~ : @common.Uri, archived~ : Bool, on_loaded~ : (@transcript.SessionLoadView) -> @cmd.Cmd, on_failed~ : (String) -> @cmd.Cmd, ) -> @cmd.Cmd { - match workspace { - Some(resource) if !@resource.belongs_to(resource, channel) => - return @cmd.none - _ => () + let key : RecordKey = { session: session_id, workspace } + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none } @cmd.custom_cmd(scheduler => { @js.async_run(() => { @@ -1304,12 +1300,12 @@ fn load_session_cmd( if archived { @interop.bridge_request(channel, @commands.session_load_archived, { session: session_id, - workspace: workspace.map(path => path.path), + workspace, }) } else { @interop.bridge_request(channel, @commands.session_load, { session: session_id, - workspace: workspace.map(path => path.path), + workspace, }) } } catch { diff --git a/desktop/frontend/composer_input.mbt b/desktop/frontend/composer_input.mbt index 045b74aaa..846089479 100644 --- a/desktop/frontend/composer_input.mbt +++ b/desktop/frontend/composer_input.mbt @@ -123,7 +123,7 @@ fn Model::composer_input( name, conversation_noun: "chat", rebuild: dispatch(RepairWorktree), - archive: dispatch(ArchiveSession(conv.device, conv.session_id)), + archive: dispatch(ArchiveSession(conv.device, conv.record_key())), }.render(), ) Some(_) => () diff --git a/desktop/frontend/console.mbt b/desktop/frontend/console.mbt index 7610745b8..e25030825 100644 --- a/desktop/frontend/console.mbt +++ b/desktop/frontend/console.mbt @@ -654,7 +654,11 @@ test "an action for another roster device cannot move page focus" { custom_api_url: "https://old.example/chat", host_settings_revision: 7, } - let (_, opened) = update(dispatch, OpenSession(Remote("d_b"), "s-b"), seeded) + let (_, opened) = update( + dispatch, + OpenSession(Remote("d_b"), listed_record(Remote("d_b"), "s-b")), + seeded, + ) assert_true(opened.focused == Remote("d_a")) assert_true(opened.active_session != "s-b") assert_true(opened.api_provider is Custom) @@ -717,7 +721,11 @@ test "a row click that raced its device's removal is dropped" { Console(RosterLoaded(metas, generation=1)), signed, ) - let (_, next) = update(dispatch, OpenSession(Remote("gone"), "s-x"), listed) + let (_, next) = update( + dispatch, + OpenSession(Remote("gone"), listed_record(Remote("gone"), "s-x")), + listed, + ) assert_true(next.focused == Remote("d_a")) inspect( next.conversations diff --git a/desktop/frontend/model.mbt b/desktop/frontend/model.mbt index 6216902d1..5250d250d 100644 --- a/desktop/frontend/model.mbt +++ b/desktop/frontend/model.mbt @@ -905,17 +905,22 @@ priv enum ArchiveDisposition { } derive(Eq) ///| -/// One durable archive identity. Session ids may repeat across project -/// stores, so archive actions, notifications, and tombstones always retain -/// the host-listed workspace beside the id. -priv struct ArchiveRecordKey { +/// One durable record identity: the store that holds it and the session id +/// within that store. Session ids repeat across project stores, so everything +/// that names an existing record — opening, archiving, restoring, deleting, +/// and the notifications and tombstones that answer them — carries the +/// host-listed workspace beside the id. Nothing here identifies a +/// *conversation*: the page holds at most one open conversation per session +/// id (`Model::find_conversation`), because the host runs at most one engine +/// per session id. This says which record that conversation is bound to. +priv struct RecordKey { session : String workspace : @common.Uri } derive(Eq) ///| -fn ArchiveRecordKey::matches( - self : ArchiveRecordKey, +fn RecordKey::matches( + self : RecordKey, item : @transcript.SessionListItem, ) -> Bool { item.id == self.session && item.workspace == self.workspace @@ -925,22 +930,30 @@ fn ArchiveRecordKey::matches( /// Bind a host protocol workspace path to the channel that produced it. A /// blank or malformed path invalidates the notification: every durable /// record belongs to a workspace store, so a key without one names nothing. -fn ArchiveRecordKey::from_protocol( +fn RecordKey::from_protocol( channel : @interop.ChannelId, session : String, workspace : String, -) -> ArchiveRecordKey? { +) -> RecordKey? { guard @resource.of_path(channel, workspace) is Some(resource) else { return None } Some({ session, workspace: resource }) } +///| +/// The durable record an open conversation is bound to. A worktree +/// conversation reports its owning project, which is also the store holding +/// its record. +fn Conversation::record_key(self : Conversation) -> RecordKey { + { session: self.session_id, workspace: self.workspace.project() } +} + ///| /// Encode the selected store for a request only when its resource belongs to /// the channel that produced it. -fn ArchiveRecordKey::workspace_payload( - self : ArchiveRecordKey, +fn RecordKey::workspace_payload( + self : RecordKey, channel : @interop.ChannelId, ) -> String? { if @resource.belongs_to(self.workspace, channel) { @@ -955,7 +968,7 @@ fn ArchiveRecordKey::workspace_payload( /// so they are reconciled through this override until the next connection /// boundary starts a fresh authoritative list round. priv struct ArchiveOverride { - key : ArchiveRecordKey + key : RecordKey disposition : ArchiveDisposition } derive(Eq) @@ -1592,8 +1605,10 @@ priv enum Msg { // fence the delayed id instead of activating it on the newly focused host. SessionRotated(@interop.ConversationKey) // Open a conversation on one device's explicit channel, selecting that - // channel first when it differs from the current one. - OpenSession(@interop.ChannelId, String) + // channel first when it differs from the current one. The row names the + // exact durable record, so a same-id record in another store is never the + // one opened. + OpenSession(@interop.ChannelId, RecordKey) // Open a row from the archived index. Its workspace is part of the row's // durable placement and selects the archived twin without restoring it. OpenArchivedSession( @@ -1683,15 +1698,15 @@ priv enum Msg { // (discarding the worktree's uncommitted changes), or stand down. ConfirmArchiveDiscard CancelArchiveDiscard - // Archive a conversation on its device: the host moves its durable - // record into the store's `archived` twin and the sidebar moves the row - // under the Archived group. - ArchiveSession(@interop.ChannelId, String) - // Restore an archived conversation into its device's live list. - UnarchiveSession(@interop.ChannelId, String) + // Archive a conversation on its device: the host moves the named durable + // record into that same store's `archived` twin and the sidebar moves the + // row under the Archived group. + ArchiveSession(@interop.ChannelId, RecordKey) + // Restore an archived record into its own store's live list. + UnarchiveSession(@interop.ChannelId, RecordKey) // Stage, confirm, or cancel permanent deletion of an archived conversation. // The confirmation's display title comes from the archived record itself. - DeleteArchivedSession(@interop.ChannelId, ArchiveRecordKey) + DeleteArchivedSession(@interop.ChannelId, RecordKey) ConfirmDeleteArchivedSession CancelDeleteArchivedSession // The host's dark-mode preference changed. It updates the palette only while @@ -1864,11 +1879,7 @@ priv enum DeviceMsg { // The host's identity-bearing `session.changed` broadcast. Archive changes // invalidate that exact live target immediately; both sidebar lists are // then re-read for their durable contents. - SessionsChanged( - change~ : String, - key~ : ArchiveRecordKey, - fresh_session~ : String? - ) + SessionsChanged(change~ : String, key~ : RecordKey, fresh_session~ : String?) SessionLoaded( session~ : String, items~ : Array[@transcript.TranscriptItem], @@ -1986,7 +1997,7 @@ priv enum DeviceMsg { // An archive was refused because the conversation's worktree holds // uncommitted changes; the client stages the discard-confirmation dialog. ArchiveNeedsForce( - session~ : String, + key~ : RecordKey, refusal~ : @protocol.ArchiveNeedsForceReply ) // The archived conversations — the reply to `archived_sessions` and to a @@ -2001,7 +2012,7 @@ priv enum DeviceMsg { // send target, so it can tombstone and leave an active deleted record even // when the broadcast arrives later. ArchivedDeleted( - key~ : ArchiveRecordKey, + key~ : RecordKey, sessions~ : Array[String], items~ : Array[@transcript.SessionListItem], connection_generation~ : Int, @@ -2627,6 +2638,11 @@ fn Model::display_status(self : Model) -> Status { } ///| +/// The one conversation this channel holds open for a session id, whichever +/// store it is bound to. Live-run routing goes through here: the host runs at +/// most one engine per session id, so an event carrying only the id can name +/// only this conversation. Anything that acts on a durable *record* must ask +/// `find_record_conversation` instead. fn Model::find_conversation( self : Model, channel : @interop.ChannelId, @@ -2640,6 +2656,25 @@ fn Model::find_conversation( None } +///| +/// The open conversation bound to exactly this durable record. A same-id +/// conversation bound to another store is not it: it renders its own sidebar +/// row, and acting on this record must not read that one's live state. +fn Model::find_record_conversation( + self : Model, + channel : @interop.ChannelId, + key : RecordKey, +) -> Conversation? { + guard self.find_conversation(channel, key.session) is Some(conv) else { + return None + } + if conv.workspace.project() == key.workspace { + Some(conv) + } else { + None + } +} + ///| /// Capture the exact lifecycle identity of every session currently known to /// this page. Absence is meaningful too: a snapshot may introduce an unknown @@ -2829,7 +2864,7 @@ fn DeviceState::placement_for_store_root( /// store-qualified record. fn DeviceState::archive_override( self : DeviceState, - key : ArchiveRecordKey, + key : RecordKey, ) -> ArchiveDisposition? { for fact in self.archive_overrides { if fact.key == key { @@ -2845,7 +2880,7 @@ fn DeviceState::archive_override( fn Model::archive_override( self : Model, channel : @interop.ChannelId, - key : ArchiveRecordKey, + key : RecordKey, ) -> ArchiveDisposition? { guard self.device_state(channel) is Some(dev) else { return None } dev.archive_override(key) @@ -2858,7 +2893,7 @@ fn Model::archive_override( fn Model::with_archive_override( self : Model, channel : @interop.ChannelId, - key : ArchiveRecordKey, + key : RecordKey, disposition : ArchiveDisposition, ) -> Model { guard self.device_state(channel) is Some(dev) else { return self } diff --git a/desktop/frontend/update.mbt b/desktop/frontend/update.mbt index 93a4dba34..fb2b359c7 100644 --- a/desktop/frontend/update.mbt +++ b/desktop/frontend/update.mbt @@ -16,10 +16,14 @@ fn session_snapshot_command( generation : Int, activate_on_success : Bool, ) -> @cmd.Cmd { + // A snapshot reads one record, so it needs that record's store. An open + // conversation carries it; otherwise the listing places the id, and a page + // that can place it nowhere has nothing to read. let (workspace, archived) = match model.find_conversation(channel, session) { Some(conv) => (Some(conv.project()), conv.is_archived_read_only()) None => (model.session_project(channel, session), false) } + guard workspace is Some(workspace) else { return @cmd.none } if activate_on_success { open_session(dispatch, channel, session, workspace~, generation~, archived~) } else { @@ -89,15 +93,20 @@ fn focus_channel( ///| /// The OpenSession body against an explicit channel — the caller has -/// already moved focus there, so `channel == model.focused` throughout. +/// already moved focus there, so `channel == model.focused` throughout. `key` +/// names the durable record the clicked row stands for; an open conversation +/// bound to another store's same-id record is not this one and is replaced, +/// exactly as opening an archived twin already does. fn open_session_on( dispatch : @cmd.Emit[Msg], channel : @interop.ChannelId, model : Model, - session_id : String, + key : RecordKey, ) -> (@cmd.Cmd, Model) { + let session_id = key.session let owner : @interop.ConversationKey = { channel, session: session_id } - if session_id == model.active_session { + if session_id == model.active_session && + model.find_record_conversation(channel, key) is Some(_) { let base = { ..model, screen: Chat } // Re-selecting is an explicit catch-up point for writes made by an // idle CLI or another client. It also restarts an exhausted retry @@ -132,7 +141,7 @@ fn open_session_on( }, ) } - } else if model.find_conversation(channel, session_id) is Some(conv) { + } else if model.find_record_conversation(channel, key) is Some(conv) { // Switching focus; the transcript re-pins at the tail. The store is // also re-read behind the switch — the local copy may date from // before a reconnect, or another client may be driving this @@ -164,17 +173,12 @@ fn open_session_on( } (@cmd.batch(commands), next) } else { - // The sidebar item says which project store holds the session, while the + // The row names the project store holding the record, while the // already-loaded worktree registry may place it in a specific checkout. - // Join both snapshots before the conversation exists: no later registry - // reply is guaranteed after a user opens an already-listed session. - // - // A session this page can place in no project is not openable: there is - // no store to read it from. That is a row on its way out — its project - // was detached — so the click is dropped rather than reported. - guard model.session_project(channel, session_id) is Some(project) else { - return (@cmd.none, model) - } + // Join both before the conversation exists: no later registry reply is + // guaranteed after a user opens an already-listed session. The clicked + // row names the store, so nothing here has to place the id. + let project = key.workspace let conversation = empty_conversation( channel, session_id, @@ -495,12 +499,10 @@ fn adopt_projects( // `activate` just placed this session, so it is on screen. None => activated } - let (fallback_cmd, next) = open_session_on( - dispatch, - channel, - activated, - fallback.id, - ) + let (fallback_cmd, next) = open_session_on(dispatch, channel, activated, { + session: fallback.id, + workspace: fallback.workspace, + }) commands.push(fallback_cmd) return (@cmd.batch(commands), next) } @@ -3259,7 +3261,7 @@ fn update_msg( archive_session( dispatch, confirm.channel, - confirm.session, + confirm.key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3555,9 +3557,10 @@ fn update_msg( }), ) } - ArchiveSession(channel, session_id) => { + ArchiveSession(channel, key) => { // Archiving acts on the row's own device and never moves focus. - guard model.device_state(channel) is Some(dev) else { + guard model.device_state(channel) is Some(dev) && + key.workspace_payload(channel) is Some(_) else { return (@cmd.none, model) } let sessions_request_generation = dev.sessions_request_generation + 1 @@ -3566,7 +3569,7 @@ fn update_msg( archive_session( dispatch, channel, - session_id, + key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3578,8 +3581,9 @@ fn update_msg( }), ) } - UnarchiveSession(channel, session_id) => { - guard model.device_state(channel) is Some(dev) else { + UnarchiveSession(channel, key) => { + guard model.device_state(channel) is Some(dev) && + key.workspace_payload(channel) is Some(_) else { return (@cmd.none, model) } let sessions_request_generation = dev.sessions_request_generation + 1 @@ -3588,7 +3592,7 @@ fn update_msg( unarchive_session( dispatch, channel, - session_id, + key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3641,18 +3645,18 @@ fn update_msg( model.conversation_by_run(model.focused, run_id) } if conv is Some(conv) { - update_msg(dispatch, OpenSession(conv.device, conv.session_id), model) + update_msg(dispatch, OpenSession(conv.device, conv.record_key()), model) } else { (@cmd.none, model) } } - OpenSession(channel, session_id) => + OpenSession(channel, key) => // Selecting a conversation always returns to the chat — including // re-selecting the active one from the Skills page. The explicit channel // may also move device selection before the focused-scoped mirrors // refetch and the conversation opens there; current sidebar rows pass // the selected channel. - if session_id.is_empty() { + if key.session.is_empty() { (@cmd.none, model) } else { // A row click that raced its device's removal has nowhere to go. @@ -3660,7 +3664,7 @@ fn update_msg( return (@cmd.none, model) } let (focus_cmd, model) = focus_channel(dispatch, model, channel) - let (cmd, model) = open_session_on(dispatch, channel, model, session_id) + let (cmd, model) = open_session_on(dispatch, channel, model, key) (@cmd.batch([focus_cmd, cmd]), model) } OpenArchivedSession(channel, session~, workspace~) => { @@ -3670,7 +3674,7 @@ fn update_msg( // The row may have been restored while its click was queued. Re-read // the current archived index and use its placement rather than opening // a stale archived target or trusting an obsolete workspace hint. - let key : ArchiveRecordKey = { session, workspace } + let key : RecordKey = { session, workspace } guard dev.archived.iter().any(item => key.matches(item)) && !(dev.archive_override(key) is Some(ArchiveLive)) else { return (@cmd.none, model) @@ -4922,7 +4926,7 @@ fn update_device_msg( // the override admits new commits immediately, and the new transcript // generation fences the archived request started by BridgeReady. for conv in next.conversations { - let key : ArchiveRecordKey = { + let key : RecordKey = { session: conv.session_id, workspace: conv.project(), } @@ -5259,9 +5263,9 @@ fn update_device_msg( // confirming it later would fire a stale forced archive. let archive_confirm = match next.archive_confirm { Some(confirm) if confirm.channel == channel && - next.find_conversation(channel, confirm.session) is Some(conv) && - conv.project() == workspace && - !rows.iter().any(row => row.session == Some(confirm.session)) => None + confirm.key.workspace == workspace && + !rows.iter().any(row => row.session == Some(confirm.key.session)) => + None other => other } (@cmd.none, { ..next, conversations, archive_confirm }) @@ -5283,14 +5287,11 @@ fn update_device_msg( }), ) } - ArchiveNeedsForce(session~, refusal~) => + ArchiveNeedsForce(key~, refusal~) => // The host refused to archive over the worktree's uncommitted // changes; stage the discard-confirmation dialog, whose confirm - // retries with force. - ( - @cmd.none, - { ..model, archive_confirm: Some({ channel, session, refusal }) }, - ) + // retries with force against the same record. + (@cmd.none, { ..model, archive_confirm: Some({ channel, key, refusal }) }) ArchivedLoaded( items, connection_generation~, @@ -5326,10 +5327,7 @@ fn update_device_msg( // so a stale live-list reply cannot reopen it in that delivery gap. let mut next = model for item in items { - let key : ArchiveRecordKey = { - session: item.id, - workspace: item.workspace, - } + let key : RecordKey = { session: item.id, workspace: item.workspace } if next.archive_override(channel, key) is None { next = next.with_archive_override(channel, key, ArchiveStored) } @@ -5701,7 +5699,7 @@ fn update_device_msg( // Archiving removes live client state after the hub's ordered final // commit. A stale/replayed commit seen after that local fact must not // resurrect the archived record as a live hidden conversation. - let key : ArchiveRecordKey = { session, workspace: commit_project } + let key : RecordKey = { session, workspace: commit_project } let archived = match dev.archive_override(key) { Some(ArchiveStored | ArchiveDeleted) => true Some(ArchiveLive) => false @@ -5946,7 +5944,7 @@ fn update_device_msg( // distinguish same-id records in different stores and leave it intact. let model = match (session_root, started_project) { (Some(_), Some(workspace)) => { - let key : ArchiveRecordKey = { session, workspace } + let key : RecordKey = { session, workspace } if dev.archive_override(key) is Some(ArchiveDeleted) { model.with_archive_override(channel, key, ArchiveLive) } else { @@ -7188,6 +7186,15 @@ fn running_conversation() -> Conversation { } } +///| +/// The record a fixture row stands for: the fixture project's store on the +/// channel that owns it. Naming the store explicitly keeps the +/// store-qualified messages readable, and keeps a remote channel's record +/// from being addressed with a local path. +fn listed_record(channel : @interop.ChannelId, session : String) -> RecordKey { + { session, workspace: channel.test_project() } +} + ///| fn test_model() -> Model { { @@ -8510,7 +8517,10 @@ test "open session is a no-op for the current session" { let dispatch = test_dispatch() let (_, next) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), test_model(), ) inspect(next.active_session, content="desktop-test") @@ -8535,12 +8545,18 @@ test "open session keeps live state on screen while re-reading the store" { let model = running_model().replace_conversation(background) let (_, once) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-bg"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-bg"), + ), model, ) let (_, twice) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-bg"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-bg"), + ), model, ) inspect(once.active_session, content="desktop-bg") @@ -8739,10 +8755,11 @@ test "device focus retires the outgoing watcher and file index" { file_ctx(model), ) assert_true(loading.index is @fileeditor.IndexLoading(_)) - let (_, next) = update(dispatch, OpenSession(remote, "remote-session"), { - ..model, - file_panel: loading, - }) + let (_, next) = update( + dispatch, + OpenSession(remote, listed_record(remote, "remote-session")), + { ..model, file_panel: loading }, + ) // The old Local HostConnection receives `fs.unwatch` before the common sync // replaces its owner. That sync also retires the in-flight Local index key; // the new remote conversation starts without either resource. @@ -9127,7 +9144,10 @@ test "open session tracks the latest load and ignores stale replies" { let dispatch = test_dispatch() let (_, loading_a) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-a"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-a"), + ), test_model().with_listed(["desktop-a", "desktop-b"]), ) inspect(loading_a.active_session, content="desktop-test") @@ -9137,7 +9157,10 @@ test "open session tracks the latest load and ignores stale replies" { ) let (_, loading_b) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-b"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-b"), + ), loading_a, ) assert_true( @@ -9205,7 +9228,10 @@ test "a restored session keeps the placement its listing named" { ) let (_, loading) = update( dispatch, - OpenSession(@interop.ChannelId::Local, session), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, session), + ), listed, ) guard loading.find_conversation(@interop.ChannelId::Local, session) @@ -9270,12 +9296,15 @@ test "same-id session loads are fenced by channel owner" { } let (_, loading_remote) = update( dispatch, - OpenSession(remote, "shared"), + OpenSession(remote, listed_record(remote, "shared")), model, ) let (_, loading_local) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "shared"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "shared"), + ), loading_remote, ) assert_true( @@ -9317,6 +9346,77 @@ test "same-id session loads are fenced by channel owner" { inspect(loaded.messages[0].content, content="remote loaded") } +///| +test "a row opens its own store's record beside a same-id conversation" { + let dispatch = test_dispatch() + let first = @interop.ChannelId::Local.test_project() + let project = @interop.ChannelId::Local.test_resource("/work/alpha") + // The page already holds this id as the first project's conversation — and + // that project is listed first, so a search by id alone would settle there. + let model = test_model() + .replace_conversation({ + ..test_conversation(), + session_id: "desktop-shared", + workspace: Project(root=first), + messages: [ + { kind: User, content: "first copy", ts: None, sequence: None }, + ], + }) + .with_dev(dev => { + ..dev, + projects: [first, project], + sessions: [ + { + id: "desktop-shared", + title: Some("first copy"), + workspace: first, + workspace_name: "work", + updated_at_ms: None, + }, + { + id: "desktop-shared", + title: Some("project copy"), + workspace: project, + workspace_name: "alpha", + updated_at_ms: None, + }, + ], + }) + let (_, opened) = update( + dispatch, + OpenSession(@interop.ChannelId::Local, { + session: "desktop-shared", + workspace: project, + }), + model, + ) + guard opened.find_conversation(@interop.ChannelId::Local, "desktop-shared") + is Some(conv) else { + fail("the project row opened no conversation") + } + // The clicked row's store wins: the conversation is bound to the second + // project's record and starts empty rather than showing the first's + // transcript. + assert_eq(conv.workspace, Project(root=project)) + assert_eq(conv.messages.length(), 0) + assert_eq(opened.dev().active_project, Some(project)) + // Clicking the first project's row afterwards rebinds the same way. + let (_, back) = update( + dispatch, + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-shared"), + ), + opened, + ) + guard back.find_conversation(@interop.ChannelId::Local, "desktop-shared") + is Some(conv) else { + fail("the first project's row opened no conversation") + } + assert_eq(conv.workspace, Project(root=first)) + assert_eq(back.dev().active_project, Some(first)) +} + ///| test "reselecting a synced active session reloads idle external writes" { let dispatch = test_dispatch() @@ -9334,7 +9434,10 @@ test "reselecting a synced active session reloads idle external writes" { }) let (_, reloading) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), model, ) assert_true( @@ -9346,7 +9449,10 @@ test "reselecting a synced active session reloads idle external writes" { } let (_, still_single_flight) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), reloading, ) inspect(still_single_flight.transcript_request_generation, content="1") @@ -9392,7 +9498,10 @@ test "returning to a fresh active chat does not load a missing session" { } let (_, next) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), model, ) assert_true(next.screen is Chat) @@ -9407,7 +9516,10 @@ test "pruning and reopening cannot reuse a transcript request token" { let dispatch = test_dispatch() let (_, opening) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-pruned"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-pruned"), + ), test_model().with_listed(["desktop-pruned"]), ) guard opening.find_conversation(@interop.ChannelId::Local, "desktop-pruned") @@ -9426,7 +9538,10 @@ test "pruning and reopening cannot reuse a transcript request token" { ) let (_, reopened) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-pruned"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-pruned"), + ), pruned, ) guard reopened.find_conversation(@interop.ChannelId::Local, "desktop-pruned") @@ -9484,7 +9599,10 @@ test "archive removal and reopening cannot reuse a transcript request token" { let dispatch = test_dispatch() let (_, opening) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-archived"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-archived"), + ), test_model().with_listed(["desktop-archived"]), ) let (_, archived) = update( @@ -9501,7 +9619,10 @@ test "archive removal and reopening cannot reuse a transcript request token" { // unarchive is what puts it back, and the row is what names its store. let (_, reopened) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-archived"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-archived"), + ), restored.with_listed(["desktop-archived"]), ) guard reopened.find_conversation( @@ -9546,7 +9667,10 @@ test "an abandoned session load retries silently" { let dispatch = test_dispatch() let (_, loading) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-a"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-a"), + ), test_model().with_listed(["desktop-a"]), ) let (_, abandoned) = update(dispatch, NewChat, loading) @@ -9569,7 +9693,10 @@ test "a foreground load preserves state through retries and reports exhaustion" let dispatch = test_dispatch() let (_, loading) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-a"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-a"), + ), test_model().with_listed(["desktop-a"]), ) let failure = SessionLoadFailed( @@ -9827,7 +9954,10 @@ test "session loaded swaps the active conversation onto the stored session" { }) let (_, loading) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-stored"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-stored"), + ), model, ) let items : Array[@transcript.TranscriptItem] = [ @@ -9889,7 +10019,10 @@ test "session loaded adopts the store transcript and keeps live run state" { // The active conversation runs; loading some other stored session is fine. let (_, loading_other) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-stored"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-stored"), + ), model, ) let (_, other) = update_dev( @@ -13031,7 +13164,10 @@ test "a runs snapshot cannot retire a steer submitted after its frontier" { } let (_, switched) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-other"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-other"), + ), after_r2.replace_conversation(other), ) guard switched.find_conversation(@interop.ChannelId::Local, "desktop-test") @@ -15930,7 +16066,7 @@ test "archived deletion keeps same-id rows in other stores" { }), active_session: "shared", } - let key : ArchiveRecordKey = { session: "shared", workspace: project } + let key : RecordKey = { session: "shared", workspace: project } let (_, staged) = update( dispatch, DeleteArchivedSession(@interop.ChannelId::Local, key), @@ -16945,7 +17081,10 @@ test "selecting a conversation returns from the skills page to the chat" { // Re-selecting the active conversation is enough to switch back… let (_, back) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), opened, ) assert_true(back.screen is Chat) @@ -18969,7 +19108,10 @@ test "reopening an active failed reload retries and preserves other local notice let model = { ..model, transcript_request_generation: 1 } let (_, reopened) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-test"), + OpenSession( + @interop.ChannelId::Local, + listed_record(@interop.ChannelId::Local, "desktop-test"), + ), model, ) assert_true( @@ -20057,11 +20199,14 @@ test "full-screen modals hide the native view and closing restores it" { // archive-discard confirmation and Quick Open. let (_, archiving) = update_dev( dispatch, - ArchiveNeedsForce(session="desktop-test", refusal={ - worktree: "wt-1", - dirty_paths: ["desktop/lepus"], - dirty_path_count: 1, - }), + ArchiveNeedsForce( + key=listed_record(@interop.ChannelId::Local, "desktop-test"), + refusal={ + worktree: "wt-1", + dirty_paths: ["desktop/lepus"], + dirty_path_count: 1, + }, + ), restored, ) assert_true(archiving.archive_confirm is Some(_)) diff --git a/desktop/frontend/view.mbt b/desktop/frontend/view.mbt index 9394dd581..f748c62e8 100644 --- a/desktop/frontend/view.mbt +++ b/desktop/frontend/view.mbt @@ -124,17 +124,22 @@ fn session_component_input( channel : @interop.ChannelId, session_id : String, label : String, + store : @common.Uri, archivable? : Bool = false, nested? : Bool = false, worktree? : String, subrun? : Bool = false, - root? : String, children? : Array[@conversation.Input] = [], ) -> @conversation.Input { + // Every live-state question this row asks is about the record in this + // group's store. A same-id conversation open against another store gets + // its own row there and must not light this one up. + let key : RecordKey = { session: session_id, workspace: store } + let conv = model.find_record_conversation(channel, key) let active = model.screen is Chat && channel == model.focused && - session_id == model.active_session - let conv = model.find_conversation(channel, session_id) + session_id == model.active_session && + conv is Some(_) let running = conv is Some(conv) && conv.is_running() // The row's status dot: pulsing while a run is in flight, then — when the // run finished off screen — settling into a solid dot (accent for success, @@ -161,12 +166,13 @@ fn session_component_input( { // The store placement rides in `root`: session ids may repeat across // project stores, and without it two same-id rows would collide on one - // key in the sidebar's keyed container. + // key in the sidebar's keyed container. It is also what the dispatcher + // reads back to name the record a click acts on. id: @conversation.Id( source="openseek.live", channel=channel.uri_authority(), conversation=session_id, - root?, + root=store.path, ), title, label, @@ -219,9 +225,6 @@ fn Model::push_group_component_inputs( archived~ : (String) -> Bool, ) -> Unit { let composing = self.composing_new_chat() - // Every row in this group shares the group's store placement; it becomes - // the `root` half of each conversation's component id. - let root = workspace.path for conv in self.conversations.rev_iter() { if conv.device != dev.channel || conv.project() != workspace || @@ -256,9 +259,9 @@ fn Model::push_group_component_inputs( } else { "New chat" }, + workspace, nested=true, worktree?, - root~, ), ), ) @@ -276,7 +279,8 @@ fn Model::push_group_component_inputs( // is the one on screen (its title settles at the first send), and // otherwise comes from the host's listing. for tree in @transcript.session_tree(durable) { - let parent_label = if self.find_conversation(dev.channel, tree.item.id) + let parent_key : RecordKey = { session: tree.item.id, workspace } + let parent_label = if self.find_record_conversation(dev.channel, parent_key) is Some(conv) && dev.channel == self.focused && tree.item.id == self.active_session { @@ -305,9 +309,9 @@ fn Model::push_group_component_inputs( dev.channel, child.id, child_label, + workspace, nested=true, subrun=true, - root~, ), ) } @@ -323,6 +327,7 @@ fn Model::push_group_component_inputs( dev.channel, tree.item.id, parent_label, + workspace, // Ordinary children are folded below a live parent and receive no // action. A child-shaped root is legacy split state (or belongs to a // parent in another store), so it remains independently archivable; @@ -330,7 +335,6 @@ fn Model::push_group_component_inputs( archivable=true, nested=true, worktree?=worktree_name_for(dev, workspace, tree.item.id), - root~, children~, ), ), @@ -686,7 +690,7 @@ fn Conversation::archived_read_only_view( class="archived-read-only-restore", type_="button", title="Restore this conversation before continuing it", - on_click=dispatch(UnarchiveSession(self.device, self.session_id)), + on_click=dispatch(UnarchiveSession(self.device, self.record_key())), "Restore", ), ]), @@ -2772,6 +2776,74 @@ test "same-id records in two project stores keep distinct row keys" { assert_true(keys[0] != keys[1]) } +///| +test "a same-id row lights up only for the store its conversation is bound to" { + let other = @interop.ChannelId::Local.test_project() + let project = @interop.ChannelId::Local.test_resource("/work/alpha") + // One open conversation, running, bound to the second project's store. The + // other project holds its own record under the same id. + let model = test_model() + .replace_conversation({ + ..running_conversation(), + session_id: "desktop-shared", + workspace: Project(root=project), + }) + .with_dev(dev => { + ..dev, + projects: [project, other], + sessions: [ + { + id: "desktop-shared", + title: Some("project copy"), + workspace: project, + workspace_name: "alpha", + updated_at_ms: None, + }, + { + id: "desktop-shared", + title: Some("other copy"), + workspace: other, + workspace_name: "work", + updated_at_ms: None, + }, + ], + }) + let model = { + ..model.activate(@interop.ChannelId::Local, "desktop-shared", project), + screen: Chat, + } + let mut in_project : @conversation.Input? = None + let mut in_other : @conversation.Input? = None + // Both rows are project rows now, so each is picked by the store its group + // renders rather than by its section kind. + for section in model.sidebar_component_sections(model.dev()) { + for entry in section.entries { + guard entry is @section.Project(row) else { continue } + for conversation in row.conversations { + guard conversation.id.conversation() == "desktop-shared" else { + continue + } + if conversation.id.root() == Some(project.path) { + in_project = Some(conversation) + } else if conversation.id.root() == Some(other.path) { + in_other = Some(conversation) + } + } + } + } + guard in_project is Some(in_project) && in_other is Some(in_other) else { + fail("both stores must render a row for the shared id") + } + // The run belongs to the second project's record; the other project's row + // is a different durable record and stays idle and unselected. + assert_true(in_project.running) + assert_true(in_project.active) + assert_true(in_project.run_mark is RunningMark) + assert_false(in_other.running) + assert_false(in_other.active) + assert_true(in_other.run_mark is NoRunMark) +} + ///| test "token counts compact to k/M with whole values undotted" { inspect(compact_count(0), content="0") diff --git a/desktop/frontend/workspaces.mbt b/desktop/frontend/workspaces.mbt index 2e339f6ae..34496fd46 100644 --- a/desktop/frontend/workspaces.mbt +++ b/desktop/frontend/workspaces.mbt @@ -527,7 +527,7 @@ test "opening a stored session joins its project and loaded worktree" { }) let (_, opening) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-w"), + OpenSession(@interop.ChannelId::Local, { session: "desktop-w", workspace }), model, ) assert_eq(opening.dev().active_project, Some(workspace)) @@ -566,7 +566,7 @@ test "opening a stored session joins its project and loaded worktree" { }) let (_, worktree_opening) = update( dispatch, - OpenSession(@interop.ChannelId::Local, "desktop-w"), + OpenSession(@interop.ChannelId::Local, { session: "desktop-w", workspace }), bound, ) guard worktree_opening.find_conversation(Local, "desktop-w") is Some(pending) else { diff --git a/desktop/frontend/worktrees.mbt b/desktop/frontend/worktrees.mbt index 0b3f5e412..b84612ce4 100644 --- a/desktop/frontend/worktrees.mbt +++ b/desktop/frontend/worktrees.mbt @@ -39,7 +39,9 @@ priv struct ProjectWorktrees { /// and carries a bounded path preview without exposing arbitrary host errors. priv struct ArchiveConfirm { channel : @interop.ChannelId - session : String + // The exact record the refused attempt named. The forced retry must reach + // that same store, not whichever one a fresh lookup would pick. + key : RecordKey refusal : @protocol.ArchiveNeedsForceReply } derive(Eq) @@ -1110,12 +1112,18 @@ test "a missing checkout stays bound while rebuild and archive remain reachable" // until the host moves the conversation out of the live list. let (_, archiving) = update( dispatch, - ArchiveSession(@interop.ChannelId::Local, "desktop-test"), + ArchiveSession(@interop.ChannelId::Local, { + session: "desktop-test", + workspace, + }), gone, ) let (_, archiving_again) = update( dispatch, - ArchiveSession(@interop.ChannelId::Local, "desktop-test"), + ArchiveSession(@interop.ChannelId::Local, { + session: "desktop-test", + workspace, + }), gone, ) assert_true( @@ -1482,7 +1490,10 @@ test "a remote removal closes this client's discard dialog" { }) let (_, staged) = update_dev( dispatch, - ArchiveNeedsForce(session="desktop-test", refusal=test_archive_refusal()), + ArchiveNeedsForce( + key={ session: "desktop-test", workspace }, + refusal=test_archive_refusal(), + ), model, ) assert_true(staged.archive_confirm is Some(_)) @@ -1605,7 +1616,7 @@ test "a dirty archive stages the discard dialog and confirm retries forced" { let (_, staged) = update_dev( dispatch, ArchiveNeedsForce( - session="desktop-test", + key=listed_record(@interop.ChannelId::Local, "desktop-test"), refusal=test_archive_refusal( dirty_paths=["desktop/lepus", "editor/tokenizer.mbt"], dirty_path_count=4, @@ -1616,7 +1627,7 @@ test "a dirty archive stages the discard dialog and confirm retries forced" { guard staged.archive_confirm is Some(confirm) else { fail("refusal staged no dialog") } - inspect(confirm.session, content="desktop-test") + inspect(confirm.key.session, content="desktop-test") assert_eq(confirm.refusal.worktree, "wt-1") assert_eq(confirm.refusal.dirty_paths[0], "desktop/lepus") let warning = @composer.WorktreeArchiveDialog::{ @@ -1634,7 +1645,10 @@ test "a dirty archive stages the discard dialog and confirm retries forced" { // a plain archive click) and closes the dialog. let (_, restaged) = update_dev( dispatch, - ArchiveNeedsForce(session="desktop-test", refusal=test_archive_refusal()), + ArchiveNeedsForce( + key=listed_record(@interop.ChannelId::Local, "desktop-test"), + refusal=test_archive_refusal(), + ), cancelled, ) let before = restaged.dev().archived_request_generation @@ -1646,6 +1660,47 @@ test "a dirty archive stages the discard dialog and confirm retries forced" { assert_true(inert.archive_confirm is None) } +///| +test "the discard dialog retries against the store the refusal named" { + let dispatch = test_dispatch() + let workspace = @interop.ChannelId::Local.test_resource("/proj") + let model = test_model().with_dev(dev => { ..dev, projects: [workspace] }) + // The refused attempt named the project store; the retry must carry that + // same store rather than re-deriving one from the session id, which a + // same-id Scratch record would answer first. + let (_, staged) = update_dev( + dispatch, + ArchiveNeedsForce( + key={ session: "desktop-shared", workspace }, + refusal=test_archive_refusal(), + ), + model, + ) + guard staged.archive_confirm is Some(confirm) else { + fail("refusal staged no dialog") + } + assert_eq(confirm.key.workspace, workspace) + assert_eq(confirm.key.session, "desktop-shared") + let before = staged.dev().archived_request_generation + let (_, confirmed) = update(dispatch, ConfirmArchiveDiscard, staged) + assert_eq(confirmed.dev().archived_request_generation, before + 1) + // A record whose store belongs to another channel cannot be encoded for + // this one, so the request is refused rather than sent storeless. + let foreign = @interop.ChannelId::Remote("d_other").test_resource("/proj") + let (_, foreign_archive) = update( + dispatch, + ArchiveSession(@interop.ChannelId::Local, { + session: "desktop-shared", + workspace: foreign, + }), + model, + ) + assert_eq( + foreign_archive.dev().archived_request_generation, + model.dev().archived_request_generation, + ) +} + ///| test "a start payload never names a worktree" { // Creation is the binding; the host routes a bound session's cwd through From c85c049fbaf88f4cefed65f1cb8b0951b588188f Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 19:34:05 +0800 Subject: [PATCH 03/17] docs: require the store on every record-addressing session op `session.load`, `session.load_archived`, `session.archive` and `session.unarchive` now take `workspace` as a required field, and the host selects that store exactly instead of searching. A new *Naming the store* section states the encoding once, says why `session.list` can return two rows with one id, and tells clients to persist `(workspace, session)` together. --- docs/remote-protocol.md | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/docs/remote-protocol.md b/docs/remote-protocol.md index 24a63fd6f..f8e6193d2 100644 --- a/docs/remote-protocol.md +++ b/docs/remote-protocol.md @@ -282,12 +282,31 @@ Notifications: | method | params | result | |---|---|---| | `session.list` | `{}` | the session index | -| `session.load` | `{session, workspace?}` — a non-blank workspace must still be registered; omitted/blank locates the session across registered stores, then the global store | `{session: , watermark?}` — current hosts include `watermark`, the highest event `sequence` the snapshot contains (0 for an empty record); older hosts may omit it, in which case clients derive it from the stored events' own sequences. | -| `session.load_archived` | `{session, workspace?}` — the same store selection rules as `session.load`, but reads only from that store's archived twin | `{session: , watermark?}` without restoring or otherwise changing the archived record | +| `session.load` | `{session, workspace}` — the exact store to read, as a registered project path (see *Naming the store* below) | `{session: , watermark?}` — current hosts include `watermark`, the highest event `sequence` the snapshot contains (0 for an empty record); older hosts may omit it, in which case clients derive it from the stored events' own sequences. | +| `session.load_archived` | `{session, workspace}` — the same store selection as `session.load`, but reads only from that store's archived twin | `{session: , watermark?}` without restoring or otherwise changing the archived record | | `session.list_archived` | `{}` | the archived index | -| `session.archive` | `{session, force?}` | success returns the archived session index (the legacy `{groups}` shape) and moves the conversation plus every sibling `-sr-N` descendant transcript as one family. A dirty checkout returns `{kind:"needs_force", worktree, dirty_paths, dirty_path_count}` without changing durable state; `dirty_paths` previews up to 8 paths and `dirty_path_count` counts all status rows. Clients show a discard-confirmation dialog and retry with `force` only after explicit confirmation. On success the conversation's checkout goes with it, but its name/branch/session placement remains registered (the branch survives; a `worktree.changed` broadcast reports `present: false`). "Dirty" means tracked modifications or non-ignored untracked files; ignored files count as disposable and are removed with the checkout, matching `git worktree remove`'s own semantics | -| `session.unarchive` | `{session}` | outcome — restores the conversation and every archived subagent descendant record together. A retained worktree placement whose checkout was removed returns as missing, so clients offer Repair before any agent, terminal, or file operation can continue | -| `session.delete_archived` | `{session, workspace}` | permanently deletes the archived conversation record from the exact host-listed project store and every archived subagent descendant record in that store, then returns the archived index. Its retained worktree placement is removed and broadcast, but project files and the Git branch remain. The operation refuses unknown stores, live records, and running/compacting family members | +| `session.archive` | `{session, workspace, force?}` — `workspace` selects the store exactly, the way `session.delete_archived` does | success returns the archived session index (the legacy `{groups}` shape) and moves the conversation plus every sibling `-sr-N` descendant transcript as one family. A dirty checkout returns `{kind:"needs_force", worktree, dirty_paths, dirty_path_count}` without changing durable state; `dirty_paths` previews up to 8 paths and `dirty_path_count` counts all status rows. Clients show a discard-confirmation dialog and retry with `force` only after explicit confirmation. On success the conversation's checkout goes with it, but its name/branch/session placement remains registered (the branch survives; a `worktree.changed` broadcast reports `present: false`). "Dirty" means tracked modifications or non-ignored untracked files; ignored files count as disposable and are removed with the checkout, matching `git worktree remove`'s own semantics | +| `session.unarchive` | `{session, workspace}` — the store whose archived twin holds the record | outcome — restores the conversation and every archived subagent descendant record together. A retained worktree placement whose checkout was removed returns as missing, so clients offer Repair before any agent, terminal, or file operation can continue | +| `session.delete_archived` | `{session, workspace}` | permanently deletes the archived conversation record from the named store and every archived subagent descendant record in that store, then returns the archived index. Its retained worktree placement is removed and broadcast, but project/scratch files and the Git branch remain. The operation refuses unknown stores, live records, and running/compacting family members | + +#### Naming the store + +A session id is unique only within one durable store, so every op above that +addresses an existing record carries `workspace` beside the id: the +host-reported project resource path, the same spelling `session.changed` and +both listings report. The host +validates a project path against its registry (a detached workspace is +refused by name) and then selects that store exactly. It never falls back to +searching, so a same-id record in another store can be neither read nor moved +by mistake, and a client that guesses gets an error instead of the wrong +conversation. + +This matters because `session.list` really can return two rows with one id — +one per attached project — and a client that keeps only `session` is +unable to tell them apart. Persist `(workspace, session)` together for +anything you can act on later; a client that persists a workspace session +across a host restart must send that workspace back, because the record is +unreachable without it once the store is no longer the default. Notifications: @@ -435,12 +454,10 @@ directory or sessions. Clients keep the workspace attached to already-open conversation state, so a stale attempt names that now-unregistered path and is rejected instead of silently relocating the session into the global store. -That workspace hint is part of a client's resume state. A protocol client -that persists a workspace session across a host restart must persist and send -its `workspace` too: once the workspace is no longer registered, an omitted -hint leaves a missing id indistinguishable from a brand-new session. -The current wire has no persistent detached-session tombstone; the bundled -client retains the hint and therefore gets the intended rejection. +That workspace is part of a client's resume state — see *Naming the store* +above, where every record-addressing op requires it. The current wire has no +persistent detached-session tombstone; the bundled client retains the store +and therefore gets the intended rejection instead of a silent relocation. ### worktree.* From 95fd076f437009440961743d2506d96826dc47f4 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 19:48:12 +0800 Subject: [PATCH 04/17] fix(desktop): filter sidebar rows by record, not by session id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by E2E: archiving one project's row made the other's row disappear from the live list too, even though only the project record moved on disk. The sidebar's two group filters asked about an id, not a record — `dev.sessions.any(item.id == id)` and `dev.archived.any(item.id == id)` — so an archived record in one store suppressed the live row of a same-id record in another. The model layer was already store-qualified; only these predicates and the archived section's `active` check were not. They now take a `RecordKey` and use `RecordKey::matches`, which compares store and id. `can_reload_transcript` asked the same half-question: a fresh conversation whose id matched another store's listing was told it had a durable record, and would then issue a snapshot load naming its own (empty) store. --- desktop/frontend/model.mbt | 6 ++- desktop/frontend/view.mbt | 88 +++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/desktop/frontend/model.mbt b/desktop/frontend/model.mbt index 5250d250d..ae3de752c 100644 --- a/desktop/frontend/model.mbt +++ b/desktop/frontend/model.mbt @@ -2931,8 +2931,12 @@ fn Model::can_reload_transcript(self : Model, conv : Conversation) -> Bool { conv.overlay.iter().any(notice => notice.anchor is SnapshotCut(..)) { return true } + // Whether THIS conversation's record is listed. A same-id record in + // another store is a different conversation, and reloading against it + // would ask the host to read a store this conversation is not bound to. match self.device_state(conv.device) { - Some(dev) => dev.sessions.iter().any(item => item.id == conv.session_id) + Some(dev) => + dev.sessions.iter().any(item => conv.record_key().matches(item)) None => false } } diff --git a/desktop/frontend/view.mbt b/desktop/frontend/view.mbt index f748c62e8..0ad248822 100644 --- a/desktop/frontend/view.mbt +++ b/desktop/frontend/view.mbt @@ -221,15 +221,16 @@ fn Model::push_group_component_inputs( rows : Array[(RowStamp, @conversation.Input)], dev : DeviceState, workspace : @common.Uri, - listed~ : (String) -> Bool, - archived~ : (String) -> Bool, + listed~ : (RecordKey) -> Bool, + archived~ : (RecordKey) -> Bool, ) -> Unit { let composing = self.composing_new_chat() for conv in self.conversations.rev_iter() { + let key : RecordKey = { session: conv.session_id, workspace } if conv.device != dev.channel || conv.project() != workspace || - listed(conv.session_id) || - archived(conv.session_id) { + listed(key) || + archived(key) { continue } // A fresh worktree conversation knows its binding locally; once the @@ -268,7 +269,7 @@ fn Model::push_group_component_inputs( } let durable : Array[@transcript.SessionListItem] = [] for item in dev.sessions { - if item.workspace == workspace && !archived(item.id) { + if item.workspace == workspace && !archived({ session: item.id, workspace }) { durable.push(item) } } @@ -399,11 +400,18 @@ fn Model::sidebar_component_sections( self : Model, dev : DeviceState, ) -> Array[@section.Input] { - let listed = (id : String) => dev.sessions.iter().any(item => item.id == id) - // An archived record has moved out of the live store, so the live list - // normally cannot contain it; the filter covers the moment between the - // archive reply and the list refetch that follows it. - let archived = (id : String) => dev.archived.iter().any(item => item.id == id) + // Both filters answer about one durable record, not one id: a record + // archived in a project store says nothing about the Scratch record beside + // it, and suppressing that row would read as archiving both. + let listed = (key : RecordKey) => { + dev.sessions.iter().any(item => key.matches(item)) + } + // An archived record has moved out of its own live store, so that store's + // live list normally cannot contain it; the filter covers the moment + // between the archive reply and the list refetch that follows it. + let archived = (key : RecordKey) => { + dev.archived.iter().any(item => key.matches(item)) + } let channel = dev.channel.uri_authority() let chat_selected = self.screen is Chat && dev.channel == self.focused // The activation identity handed to sections and projects: which @@ -510,7 +518,11 @@ fn Model::sidebar_component_sections( let item = tree.item let active = self.focused == dev.channel && self.active_session == item.id && - self.find_conversation(dev.channel, item.id) is Some(conv) && + self.find_record_conversation(dev.channel, { + session: item.id, + workspace: item.workspace, + }) + is Some(conv) && conv.is_archived_read_only() archived_entries.push( @section.Conversation({ @@ -2776,6 +2788,60 @@ test "same-id records in two project stores keep distinct row keys" { assert_true(keys[0] != keys[1]) } +///| +test "archiving one store's record leaves the same id listed in the other" { + let other = @interop.ChannelId::Local.test_project() + let project = @interop.ChannelId::Local.test_resource("/work/alpha") + // What the sidebar shows right after one project's record is archived: the + // archived index holds it, and the live list still holds the other's. + let model = test_model().with_dev(dev => { + ..dev, + projects: [project, other], + sessions: [ + { + id: "desktop-shared", + title: Some("other copy"), + workspace: other, + workspace_name: "work", + updated_at_ms: None, + }, + ], + archived: [ + { + id: "desktop-shared", + title: Some("project copy"), + workspace: project, + workspace_name: "alpha", + updated_at_ms: None, + }, + ], + }) + let live : Array[String?] = [] + let mut archived_rows = 0 + for section in model.sidebar_component_sections(model.dev()) { + for entry in section.entries { + match entry { + @section.Project(row) => + for conversation in row.conversations { + if conversation.id.conversation() == "desktop-shared" { + live.push(conversation.id.root()) + } + } + @section.Conversation(conversation) => + if conversation.id.conversation() == "desktop-shared" && + conversation.id.source() == "openseek.archived" { + archived_rows = archived_rows + 1 + } + _ => () + } + } + } + // The other project's row survives its neighbour's archive; only the + // selected record moved into the Archived group. + assert_eq(live, [Some(other.path)]) + assert_eq(archived_rows, 1) +} + ///| test "a same-id row lights up only for the store its conversation is bound to" { let other = @interop.ChannelId::Local.test_project() From 4bd19bee069491e61b5f4ba7b36433ed928cd398 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 19:57:38 +0800 Subject: [PATCH 05/17] fix(desktop): never rebind an open conversation from an ambiguous listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by E2E: after unarchiving the project record, the project group showed the other project's conversation — its transcript, under the wrong folder. `SessionsLoaded` reconciles a conversation materialized from a broadcast before `session.list` named its store. That reconcile matched by id alone and did not stop at the first hit, so the LAST same-id row won — and `list_sessions` orders one group per registered workspace. A conversation the user had open was therefore rebound to the project store: mislabelled in the sidebar, and pointed at a record it had never opened for its next load or send. The listing is now the authority on placement only while it is unambiguous. `listed_project` keeps the conversation's own binding when a row confirms it or when several rows carry the id, and adopts a row's store only when exactly one bears the id — which is the provisional-placement case this reconcile exists for. The active-project follow does the same, preferring the active conversation's own placement. The unarchive handler's `foreground` test asked `session_project` — first same-id row wins — although it already held the conversation bound to the exact record; being the active id is the whole question there. --- desktop/frontend/update.mbt | 132 ++++++++++++++++++++++++++++++++---- 1 file changed, 120 insertions(+), 12 deletions(-) diff --git a/desktop/frontend/update.mbt b/desktop/frontend/update.mbt index fb2b359c7..4b19929ba 100644 --- a/desktop/frontend/update.mbt +++ b/desktop/frontend/update.mbt @@ -91,6 +91,32 @@ fn focus_channel( ) } +///| +/// Where `session.list` says this conversation's record lives. +/// +/// The list is the authority on placement only while it is unambiguous. A +/// conversation materialized from a broadcast before the list arrived carries +/// a provisional store, and the single row bearing its id names the real one. +/// But the list really can carry two rows for one id — Scratch and a project +/// — and then the conversation's own binding is the better answer: it came +/// from the row the user clicked or from the store root a commit named, +/// whereas picking a row by id alone would rebind the conversation to a +/// record it never opened. +fn listed_project( + items : Array[@transcript.SessionListItem], + conv : Conversation, +) -> @common.Uri { + let current = conv.workspace.project() + let matches = items.filter(item => item.id == conv.session_id) + if matches.iter().any(item => item.workspace == current) { + return current + } + match matches { + [only] => only.workspace + _ => current + } +} + ///| /// The OpenSession body against an explicit channel — the caller has /// already moved focus there, so `channel == model.focused` throughout. `key` @@ -4889,16 +4915,10 @@ fn update_device_msg( // the default store. let conversations = model.conversations.map(conv => { if conv.device == channel { - let mut project = conv.project() - for item in items { - if item.id == conv.session_id { - project = item.workspace - } - } { ..conv, workspace: dev.conversation_workspace( - project, + listed_project(items, conv), conv.session_id, conv.workspace, ), @@ -4907,11 +4927,18 @@ fn update_device_msg( conv } }) + // The active conversation's own placement is the store the sidebar + // should follow. Only when the page holds no conversation for the + // active id does the listing have to answer, under the same rule. let mut active_project = dev.active_project if channel == model.focused { - for item in items { - if item.id == model.active_session { - active_project = Some(item.workspace) + if model.find_conversation(channel, model.active_session) + is Some(active) { + active_project = Some(listed_project(items, active)) + } else { + let matches = items.filter(item => item.id == model.active_session) + if matches is [only] { + active_project = Some(only.workspace) } } } @@ -5546,9 +5573,12 @@ fn update_device_msg( sync: Synced, }) if generation is Some(generation) { + // `conv` above is already the conversation bound to this exact + // record, so being the active id is the whole question — asking + // the listing again would answer for whichever store happens to + // list this id first. let foreground = channel == next.focused && - next.active_session == key.session && - next.session_project(channel, key.session) == Some(key.workspace) + next.active_session == key.session let next = if foreground { { ..next, @@ -9346,6 +9376,84 @@ test "same-id session loads are fenced by channel owner" { inspect(loaded.messages[0].content, content="remote loaded") } +///| +test "a session list holding one id twice never rebinds an open conversation" { + let dispatch = test_dispatch() + let first = @interop.ChannelId::Local.test_project() + let project = @interop.ChannelId::Local.test_resource("/work/alpha") + // The first project's record is the one on screen. The list then arrives + // carrying both stores' rows for this id — the second project last, as + // `list_sessions` orders it (one group per registered workspace). + let model = test_model() + .replace_conversation({ + ..test_conversation(), + session_id: "desktop-shared", + workspace: Project(root=first), + messages: [ + { kind: User, content: "first copy", ts: None, sequence: None }, + ], + }) + .with_dev(dev => { ..dev, projects: [first, project] }) + let model = { + ..model.activate(@interop.ChannelId::Local, "desktop-shared", first), + screen: Chat, + } + let (_, listed) = update( + dispatch, + sessions_loaded([ + { + id: "desktop-shared", + title: Some("first copy"), + workspace: first, + workspace_name: "work", + updated_at_ms: None, + }, + { + id: "desktop-shared", + title: Some("project copy"), + workspace: project, + workspace_name: "alpha", + updated_at_ms: None, + }, + ]), + model, + ) + guard listed.find_conversation(@interop.ChannelId::Local, "desktop-shared") + is Some(conv) else { + fail("the open conversation disappeared") + } + // It stays bound to the first project. Adopting the last same-id row would + // move a transcript the user opened there into the other project's group. + assert_eq(conv.workspace, Project(root=first)) + assert_eq(listed.dev().active_project, Some(first)) + // A conversation whose store the page does NOT yet know still learns it + // from an unambiguous listing — that is what this reconcile is for. + let (_, provisional) = update( + dispatch, + sessions_loaded([ + { + id: "desktop-only-there", + title: Some("project copy"), + workspace: project, + workspace_name: "alpha", + updated_at_ms: None, + }, + ]), + listed.replace_conversation({ + ..test_conversation(), + session_id: "desktop-only-there", + }), + ) + guard provisional.find_conversation( + @interop.ChannelId::Local, + "desktop-only-there", + ) + is Some(adopted) else { + fail("the provisional conversation disappeared") + } + assert_eq(adopted.workspace, Project(root=project)) +} + ///| test "a row opens its own store's record beside a same-id conversation" { let dispatch = test_dispatch() From 56fd5b4fb30b5ec5b80a89cd1595a74e06e0a709 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 20:05:56 +0800 Subject: [PATCH 06/17] refactor(desktop): index the session listings instead of rescanning them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The store-qualified filters kept the shape of the id-only ones they replaced: a linear scan of the listing per row. The sidebar re-renders on every dispatch and asks twice per row — once against the live listing, once against the archived one — so a device with a few hundred records did tens of thousands of comparisons per frame, and `SessionsLoaded` allocated a filtered array per conversation. `RecordKey` derives `Hash`, so both listings index into a `HashSet[RecordKey]` built once per render and each row's question is a membership test. `@common.Uri` already implements `Hash` and `Eq`; nothing in the editor submodule changes. Placement reconciliation indexes by id into `ListedStore`, which names the single store carrying an id or says several do — the exact distinction `listed_project` needs, so it becomes one lookup with no scan and no first/last-match subtlety left to get wrong. The two accumulation loops this touched are now `filter`/`fold`. --- desktop/frontend/goal.mbt | 4 +-- desktop/frontend/model.mbt | 16 +++++++++++- desktop/frontend/moon.pkg | 1 + desktop/frontend/update.mbt | 52 +++++++++++++++++++++++++------------ desktop/frontend/view.mbt | 26 +++++++------------ 5 files changed, 64 insertions(+), 35 deletions(-) diff --git a/desktop/frontend/goal.mbt b/desktop/frontend/goal.mbt index 56960bc4e..3a3303184 100644 --- a/desktop/frontend/goal.mbt +++ b/desktop/frontend/goal.mbt @@ -878,8 +878,8 @@ test "a kept goal draft is a conversation you can get back to" { rows, dev, @interop.ChannelId::Local.test_project(), - listed=_ => false, - archived=_ => false, + listed=listed_records([]), + archived=listed_records([]), ) assert_true( rows.iter().any(row => row.1.id().conversation() == "desktop-fresh"), diff --git a/desktop/frontend/model.mbt b/desktop/frontend/model.mbt index ae3de752c..bca6c1f98 100644 --- a/desktop/frontend/model.mbt +++ b/desktop/frontend/model.mbt @@ -916,7 +916,21 @@ priv enum ArchiveDisposition { priv struct RecordKey { session : String workspace : @common.Uri -} derive(Eq) +} derive(Eq, Hash) + +///| +/// The records a host listing names, for membership questions asked once per +/// row: the sidebar re-renders on every dispatch, and scanning the live and +/// archived listings per row is quadratic in the number of conversations. +fn listed_records( + items : Array[@transcript.SessionListItem], +) -> @hashset.HashSet[RecordKey] { + @hashset.HashSet::from_iter( + items + .iter() + .map(item => ({ session: item.id, workspace: item.workspace } : RecordKey)), + ) +} ///| fn RecordKey::matches( diff --git a/desktop/frontend/moon.pkg b/desktop/frontend/moon.pkg index a0da21add..2a2e53fac 100644 --- a/desktop/frontend/moon.pkg +++ b/desktop/frontend/moon.pkg @@ -1,6 +1,7 @@ import { "moonbitlang/core/cmp", "moonbitlang/core/debug", + "moonbitlang/core/hashset", "moonbitlang/core/json", "moonbitlang/core/string", "moonbit-community/rabbita", diff --git a/desktop/frontend/update.mbt b/desktop/frontend/update.mbt index 4b19929ba..a242dbe03 100644 --- a/desktop/frontend/update.mbt +++ b/desktop/frontend/update.mbt @@ -91,6 +91,33 @@ fn focus_channel( ) } +///| +/// What a host listing says about one session id: the single store carrying +/// it, or that several do. Session ids repeat only across stores, so this +/// collapses to `OneStore` for every ordinary id. +priv enum ListedStore { + OneStore(@common.Uri) + SeveralStores +} + +///| +/// Index a listing by session id, so placement reconciliation is one lookup +/// per conversation rather than a scan of the whole listing. +fn listed_stores( + items : Array[@transcript.SessionListItem], +) -> Map[String, ListedStore] { + items + .iter() + .fold(init=Map([]), (index, item) => { + index[item.id] = match index.get(item.id) { + None => OneStore(item.workspace) + Some(OneStore(store)) if store == item.workspace => OneStore(store) + Some(_) => SeveralStores + } + index + }) +} + ///| /// Where `session.list` says this conversation's record lives. /// @@ -103,17 +130,12 @@ fn focus_channel( /// whereas picking a row by id alone would rebind the conversation to a /// record it never opened. fn listed_project( - items : Array[@transcript.SessionListItem], + index : Map[String, ListedStore], conv : Conversation, ) -> @common.Uri { - let current = conv.workspace.project() - let matches = items.filter(item => item.id == conv.session_id) - if matches.iter().any(item => item.workspace == current) { - return current - } - match matches { - [only] => only.workspace - _ => current + match index.get(conv.session_id) { + Some(OneStore(store)) => store + Some(SeveralStores) | None => conv.workspace.project() } } @@ -4913,12 +4935,13 @@ fn update_device_msg( // that provisional placement from the list before any later send: an // empty workspace hint here would otherwise fork the same session into // the default store. + let stores = listed_stores(items) let conversations = model.conversations.map(conv => { if conv.device == channel { { ..conv, workspace: dev.conversation_workspace( - listed_project(items, conv), + listed_project(stores, conv), conv.session_id, conv.workspace, ), @@ -4934,12 +4957,9 @@ fn update_device_msg( if channel == model.focused { if model.find_conversation(channel, model.active_session) is Some(active) { - active_project = Some(listed_project(items, active)) - } else { - let matches = items.filter(item => item.id == model.active_session) - if matches is [only] { - active_project = Some(only.workspace) - } + active_project = Some(listed_project(stores, active)) + } else if stores.get(model.active_session) is Some(OneStore(store)) { + active_project = Some(store) } } let mut next = { diff --git a/desktop/frontend/view.mbt b/desktop/frontend/view.mbt index 0ad248822..9e2404044 100644 --- a/desktop/frontend/view.mbt +++ b/desktop/frontend/view.mbt @@ -221,16 +221,16 @@ fn Model::push_group_component_inputs( rows : Array[(RowStamp, @conversation.Input)], dev : DeviceState, workspace : @common.Uri, - listed~ : (RecordKey) -> Bool, - archived~ : (RecordKey) -> Bool, + listed~ : @hashset.HashSet[RecordKey], + archived~ : @hashset.HashSet[RecordKey], ) -> Unit { let composing = self.composing_new_chat() for conv in self.conversations.rev_iter() { let key : RecordKey = { session: conv.session_id, workspace } if conv.device != dev.channel || conv.project() != workspace || - listed(key) || - archived(key) { + listed.contains(key) || + archived.contains(key) { continue } // A fresh worktree conversation knows its binding locally; once the @@ -267,12 +267,10 @@ fn Model::push_group_component_inputs( ), ) } - let durable : Array[@transcript.SessionListItem] = [] - for item in dev.sessions { - if item.workspace == workspace && !archived({ session: item.id, workspace }) { - durable.push(item) - } - } + let durable = dev.sessions.filter(item => { + item.workspace == workspace && + !archived.contains({ session: item.id, workspace }) + }) // Sub-run transcripts fold under the conversation that launched them: they // are that turn's evidence, not conversations of their own. They ride the // parent's component input, so their disclosure state lives in the @@ -403,15 +401,11 @@ fn Model::sidebar_component_sections( // Both filters answer about one durable record, not one id: a record // archived in a project store says nothing about the Scratch record beside // it, and suppressing that row would read as archiving both. - let listed = (key : RecordKey) => { - dev.sessions.iter().any(item => key.matches(item)) - } + let listed = listed_records(dev.sessions) // An archived record has moved out of its own live store, so that store's // live list normally cannot contain it; the filter covers the moment // between the archive reply and the list refetch that follows it. - let archived = (key : RecordKey) => { - dev.archived.iter().any(item => key.matches(item)) - } + let archived = listed_records(dev.archived) let channel = dev.channel.uri_authority() let chat_selected = self.screen is Chat && dev.channel == self.focused // The activation identity handed to sections and projects: which From 7610d8a6d7f120ca283db7bfb909dc5ed5c9b0e0 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 20:11:19 +0800 Subject: [PATCH 07/17] fix(desktop): free the checkout of the project the archive selected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Archive moved the selected store's record but then asked `@worktree.archive_bound_checkout` to find the worktree by session id, and `@session_store.workspace_of` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved. Reported by Codex on #886. --- desktop/internal/engine/archive.mbt | 4 +- desktop/internal/worktree/pkg.generated.mbti | 2 + desktop/internal/worktree/worktree.mbt | 72 +++++++++++++++----- 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/desktop/internal/engine/archive.mbt b/desktop/internal/engine/archive.mbt index 9107244b5..8f49742d9 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -404,8 +404,10 @@ async fn archive_session_commit( // (the next prompt respawns) — so nothing irreversible has happened yet; // a removal that succeeds while the record move below then fails leaves a // live conversation safely blocked on its retained missing placement. - let refusal = @worktree.WorktreeOwner::openseek(session).archive_checkout( + let refusal = @worktree.archive_bound_checkout( + session, force, + family.workspace, on_committed?=on_worktree_changed, ) if refusal is Some(refusal) { diff --git a/desktop/internal/worktree/pkg.generated.mbti b/desktop/internal/worktree/pkg.generated.mbti index 7b2b890fa..4927115e6 100644 --- a/desktop/internal/worktree/pkg.generated.mbti +++ b/desktop/internal/worktree/pkg.generated.mbti @@ -9,6 +9,8 @@ import { } // Values +pub async fn archive_bound_checkout(String, Bool, @pathx.Absolute?, on_committed? : (String, Array[@protocol.WorktreeInfo]) -> Unit) -> @protocol.ArchiveNeedsForceReply? + pub async fn create(WorktreeHost, String, WorktreeOwner, (Array[@protocol.WorktreeInfo]) -> Unit, name? : String) -> @protocol.WorktreeCreateReply pub async fn drop_placements(@pathx.Absolute, Array[String]) -> Array[@protocol.WorktreeInfo]? diff --git a/desktop/internal/worktree/worktree.mbt b/desktop/internal/worktree/worktree.mbt index 9e01ed638..d7a52aebd 100644 --- a/desktop/internal/worktree/worktree.mbt +++ b/desktop/internal/worktree/worktree.mbt @@ -418,21 +418,17 @@ pub async fn run_dir( } ///| -/// Find this owner's registry row while `worktree_registry_lock` is held. -/// OpenSeek can start from its durable session store; Codex deliberately scans -/// only Desktop registries, never app-server or Git path conventions. +/// Find this owner's registry row in one of `within`, while +/// `worktree_registry_lock` is held. The caller names those projects, because +/// which of them may hold the row is a question about the owner: an OpenSeek +/// record's worktree belongs to the project whose store owns it, while a Codex +/// thread lives in no session store and is deliberately looked up only in +/// Desktop registries, never in app-server state or Git path conventions. async fn WorktreeOwner::binding_unlocked( self : WorktreeOwner, + within : Array[@pathx.Absolute], ) -> (@pathx.Absolute, WorktreeEntry)? { - let workspaces = match self { - OpenSeekSession(session) => - match @session_store.workspace_of(session) { - Some(workspace) => [workspace] - None => [] - } - CodexThread(_) => @workspaces.registered() - } - for workspace in workspaces { + for workspace in within { for entry in read_worktrees_unlocked(workspace) { if entry.belongs_to(self) { return Some((workspace, entry)) @@ -446,13 +442,18 @@ async fn WorktreeOwner::binding_unlocked( /// Inspect whether archiving would discard uncommitted, untracked, or ignored /// checkout files. This preflight changes nothing; the final removal repeats /// the check after the backend writer has been archived, closing the race -/// between confirmation and deletion. +/// between confirmation and deletion. Codex archives through this preflight +/// because its thread lives in no session store; OpenSeek's archive names the +/// store it selected and goes straight to `archive_bound_worktree_checkout`. pub async fn WorktreeOwner::archive_check( self : WorktreeOwner, ) -> ArchiveNeedsForceReply? { worktree_registry_lock.acquire() defer worktree_registry_lock.release() - guard self.binding_unlocked() is Some((workspace, entry)) else { return None } + guard self.binding_unlocked(@workspaces.registered()) + is Some((workspace, entry)) else { + return None + } let target = worktree_dir(workspace, entry.name) let present = @fsx.is_dir(target.to_path()) catch { error if @async.is_being_cancelled() => raise error @@ -477,14 +478,17 @@ pub async fn WorktreeOwner::archive_check( /// checkout refuses; committed work stays on the branch either way. The caller /// must first fence its own backend writer (OpenSeek's slot claim or a /// successful Codex `thread/archive`). An owner without a worktree is a no-op. -pub async fn WorktreeOwner::archive_checkout( +async fn WorktreeOwner::archive_checkout_within( self : WorktreeOwner, + within : Array[@pathx.Absolute], force : Bool, on_committed? : (String, Array[WorktreeInfo]) -> Unit, ) -> ArchiveNeedsForceReply? { worktree_registry_lock.acquire() defer worktree_registry_lock.release() - guard self.binding_unlocked() is Some((workspace, entry)) else { return None } + guard self.binding_unlocked(within) is Some((workspace, entry)) else { + return None + } let entries = read_worktrees_unlocked(workspace) let target = worktree_dir(workspace, entry.name) let present = @fsx.is_dir(target.to_path()) catch { @@ -542,6 +546,42 @@ pub async fn drop_placements( Some(worktree_infos(workspace, remaining)) } +///| +/// Free the checkout of a Codex thread. A thread lives in no session store, so +/// its registry row is looked up across Desktop's registered projects. +pub async fn WorktreeOwner::archive_checkout( + self : WorktreeOwner, + force : Bool, + on_committed? : (String, Array[WorktreeInfo]) -> Unit, +) -> ArchiveNeedsForceReply? { + self.archive_checkout_within(@workspaces.registered(), force, on_committed?) +} + +///| +/// Free the checkout bound to `session`, if any, while retaining its registry +/// entry. Archiving frees the directory, but the name/branch/session row +/// remains the durable placement needed to make a later unarchive read as +/// `MissingTree` and offer an explicit rebuild. +/// +/// `workspace` is the project whose store the archive selected, not a project +/// rediscovered from the session id: a worktree belongs to the project that +/// owns the record being moved, and looking the id up again would find the +/// first registered project holding it — a forced archive would then delete +/// another project's uncommitted checkout. Scratch records own no worktree. +pub async fn archive_bound_checkout( + session : String, + force : Bool, + workspace : @pathx.Absolute?, + on_committed? : (String, Array[WorktreeInfo]) -> Unit, +) -> ArchiveNeedsForceReply? { + guard workspace is Some(workspace) else { return None } + WorktreeOwner::openseek(session).archive_checkout_within( + [workspace], + force, + on_committed?, + ) +} + ///| /// The tree root an absolute in-workspace `path` belongs to: the registered /// worktree containing it when one does, otherwise the workspace itself. A From f821b4034dd0530e63e5cacedbd2527d973b5b2c Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 20:34:20 +0800 Subject: [PATCH 08/17] feat(desktop): stop creating same-id records, and scope slots to a store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session ` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle record was refused while another project's conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 --- desktop/internal/engine/archive.mbt | 48 ++++- desktop/internal/engine/engine.mbt | 38 +++- desktop/internal/engine/follower.mbt | 12 ++ desktop/internal/engine/ops.mbt | 1 + .../internal/engine/store_identity_wbtest.mbt | 189 ++++++++++++++++++ desktop/internal/worktree/pkg.generated.mbti | 4 +- desktop/internal/worktree/worktree.mbt | 23 ++- docs/remote-protocol.md | 15 ++ 8 files changed, 307 insertions(+), 23 deletions(-) create mode 100644 desktop/internal/engine/store_identity_wbtest.mbt diff --git a/desktop/internal/engine/archive.mbt b/desktop/internal/engine/archive.mbt index 8f49742d9..7a243042f 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -57,6 +57,40 @@ async fn EngineManager::sweep_archived_deletions(self : EngineManager) -> Unit { } } +///| +/// Refuse to open a *second* record for an id another store already owns. +/// +/// Session ids are unique only within one store, so the desktop must read and +/// move records by `(store, id)`. That is a client-side truth it cannot +/// impose on the format: the `openseek` CLI takes `--session ` verbatim +/// under any `--session-root`, and a project's store travels inside the +/// project directory. What the host *can* guarantee is that the app never +/// adds to the population — every duplicate a user meets came from outside +/// it. `worktree.create` has enforced the same rule since it began binding +/// conversations; this is the equivalent for the ordinary start path. +/// +/// A start whose selected store already holds the record is a continuation, +/// not a creation, and passes untouched — including for an id that is +/// duplicated on disk, because refusing to continue an existing record would +/// strand the user with a conversation they can open but never answer. +async fn ensure_new_record_is_unique( + session : String, + selected? : @pathx.Absolute, +) -> Unit { + guard selected is Some(selected) else { return } + if @fsx.is_dir(selected.join("sessions").join(session).to_path()) { + return + } + for root in @session_store.known_roots() { + guard root.to_string() != selected.to_string() else { continue } + if @fsx.is_dir(root.join("sessions").join(session).to_path()) { + raise EngineError( + "this conversation already has a record in \{root}; start a new one here", + ) + } + } +} + ///| /// The record and engine claims held while a whole conversation family moves. /// Claims are acquired for every member before any writer is closed or record @@ -193,8 +227,15 @@ async fn EngineManager::claim_session_family_move( // Validate the complete family before closing any idle process. A running // child can be reached by opening its transcript and continuing it, even // though its ordinary nested sidebar row has no independent archive action. + // + // Only engines writing the selected store are this move's business. The + // pending claims above stay store-blind on purpose — they are held for the + // length of one operation, and refusing a concurrent same-id op costs a + // retry — but an idle engine can hold its slot for hours, so treating + // another store's writer as this record's would refuse a move the user can + // see is archivable, and then close a conversation they are still using. for slot in slots { - if slot.engine is Some(live) { + if slot.engine is Some(live) && live.writes_store(root) { match live.phase { Running(..) => raise EngineError( @@ -209,12 +250,13 @@ async fn EngineManager::claim_session_family_move( } } for slot in slots { - if slot.engine is Some(live) { + if slot.engine is Some(live) && live.writes_store(root) { live.close_and_wait() } } for session_id in members { - if manager.followers.get(session_id) is Some(follower) { + if manager.followers.get(session_id) is Some(follower) && + follower.tails_store(root) { stop_follower_instance(manager, follower, writer_exited=true) } } diff --git a/desktop/internal/engine/engine.mbt b/desktop/internal/engine/engine.mbt index c42e97516..f19a01d62 100644 --- a/desktop/internal/engine/engine.mbt +++ b/desktop/internal/engine/engine.mbt @@ -374,6 +374,23 @@ fn ServeEngine::is_current(self : ServeEngine, manager : EngineManager) -> Bool physical_equal(current, self) } +///| +/// Whether this engine's durable writes land in `root`. Session slots are +/// keyed by id alone — the host runs at most one engine per id — so every +/// lifecycle operation that addresses one named store must ask this before +/// treating a live engine as its own. A same-id record in another store is a +/// different conversation: blocking on its engine refuses a move that store +/// never contended for, and closing it tears down a writer the operation was +/// never given. An engine spawned without a durable store writes no record at +/// all, so it belongs to no store's move. +fn ServeEngine::writes_store( + self : ServeEngine, + root : @pathx.Absolute, +) -> Bool { + self.session_root is Some(session_root) && + session_root.to_string() == root.to_string() +} + ///| /// What the serve engine is doing right now, and which run it is doing it for /// — one value instead of loose booleans beside a separate run id, so the @@ -927,7 +944,9 @@ pub fn EngineManager::worktree_host( session_is_live: session => { self.pump is Serving(sessions~) && sessions.get(session) is Some(_) }, - drain_sessions: sessions => self.close_engines_for_sessions(sessions), + drain_sessions: (sessions, root) => { + self.close_engines_for_sessions(sessions, root~) + }, } } @@ -968,7 +987,7 @@ async fn EngineManager::close_engines_for_workspace( workspace : @pathx.Absolute, ) -> Unit { guard self.pump is Serving(sessions~) else { return } - let root = @workspaces.store_root(workspace).to_string() + let root = @workspaces.store_root(workspace) let idle : Array[ServeEngine] = [] for _, slot in sessions { if slot.pending is Some(_) { @@ -976,9 +995,7 @@ async fn EngineManager::close_engines_for_workspace( "another conversation operation is still being prepared; wait for it to finish", ) } - if slot.engine is Some(engine) && - engine.session_root is Some(session_root) && - session_root.to_string() == root { + if slot.engine is Some(engine) && engine.writes_store(root) { match engine.phase { Running(..) => raise EngineError( @@ -1011,7 +1028,7 @@ async fn EngineManager::close_engines_for_workspace( // held, and identity checks make already-retired captures harmless. let retained : Array[SessionFollower] = [] for _, follower in self.followers { - if follower.root.to_string() == root { + if follower.tails_store(root) { retained.push(follower) } } @@ -1028,9 +1045,14 @@ async fn EngineManager::close_engines_for_workspace( /// start/compact claim can appear between this scan and the registry /// commit. A pending operation or a busy engine is refused; idle writers /// receive EOF and join their follower's final durable scan. +/// +/// `root` is the workspace's store, not a store rediscovered from the ids: +/// a worktree's conversations record there, and a same-id record in another +/// store has nothing to do with the checkout being removed. async fn EngineManager::close_engines_for_sessions( self : EngineManager, ids : Array[String], + root~ : @pathx.Absolute, ) -> Unit { guard self.pump is Serving(sessions~) else { return } let idle : Array[ServeEngine] = [] @@ -1041,7 +1063,7 @@ async fn EngineManager::close_engines_for_sessions( "another conversation operation is still being prepared; wait for it to finish", ) } - if slot.engine is Some(engine) { + if slot.engine is Some(engine) && engine.writes_store(root) { match engine.phase { Running(..) => raise EngineError( @@ -1066,7 +1088,7 @@ async fn EngineManager::close_engines_for_sessions( // for the idle-engine pass above to discover. Followers are keyed by // session id, so the mapped ids find them directly. for id in ids { - if self.followers.get(id) is Some(follower) { + if self.followers.get(id) is Some(follower) && follower.tails_store(root) { stop_follower_instance(self, follower, writer_exited=true) } } diff --git a/desktop/internal/engine/follower.mbt b/desktop/internal/engine/follower.mbt index 87fecc3b8..3d9f56519 100644 --- a/desktop/internal/engine/follower.mbt +++ b/desktop/internal/engine/follower.mbt @@ -120,6 +120,18 @@ priv struct SessionFollower { mut consumed : FileSignature? } +///| +/// Whether this actor tails a record in `root`. The follower map is keyed by +/// session id, so a lifecycle operation on one store must ask this before +/// draining an actor — see `ServeEngine::writes_store` for why the id alone +/// cannot decide it. +fn SessionFollower::tails_store( + self : SessionFollower, + root : @pathx.Absolute, +) -> Bool { + self.root.to_string() == root.to_string() +} + ///| /// Remember that `run_id` needs an exact post-Finished durable boundary. /// Registration is idempotent across the semantic-terminal and failed-command diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index c494aa84f..f9184738a 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -34,6 +34,7 @@ pub async fn start_run( defer manager.end_record_op(key, record_guard) let config = run_config(manager, payload) @session_store.ensure_not_archived(key) + ensure_new_record_is_unique(key, selected?=config.session_root) let sessions = manager.serving() let slot = slot_for(sessions, key) // Busy-ness is a property of the handle, not of admission: a condemned diff --git a/desktop/internal/engine/store_identity_wbtest.mbt b/desktop/internal/engine/store_identity_wbtest.mbt new file mode 100644 index 000000000..e5c2a3cd5 --- /dev/null +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -0,0 +1,189 @@ +// A session id names a record only inside one store, so two attached projects +// can hold the same id — the `openseek` CLI takes `--session ` verbatim +// under any `--session-root`, and a project's store travels inside the project +// directory. Two host behaviors follow, and neither can be read off the id: +// +// * the desktop never *adds* to that population — a start opens a record in +// the store it named or continues one already there, never a second twin; +// * lifecycle operations that address one store ignore live engines and +// followers serving another, even though both are filed under the id. +// +// Both are exercised against a real pump: the refusals under test are the +// ones a live host issues, and a start that slips past them must show up as a +// spawned engine rather than as a stalled request. + +///| +/// A host whose engine binary accepts a prompt and then holds the turn open, +/// so a conversation can be left running while another store is acted on. +#cfg(not(platform="windows")) +async fn store_identity_host(prefix : String) -> (@pathx.Absolute, EngineActor) { + let root = worktree_test_root(prefix) + let engine = root.join("bin/engine.sh") + let runtime = root.join("runtime") + @sys.set_env_var("OPENSEEK_SESSION_ROOT", root.join("global").to_string()) + prepare_fake_moonbit_seed( + root.join("bin/toolchains/moonbit/macos-arm64").to_path(), + version="0.10.0-test", + ) + prepare_fake_bundled_moonbit_home( + runtime.join("toolchains/moonbit/macos-arm64").to_path(), + version="0.10.0-test", + ) + @fs.write_file( + engine.to_string(), + "#!/bin/sh\nif [ \"$1\" = \"sessions\" ]; then printf '%s' '{\"events\":[]}' ; exit 0; fi\nwhile IFS= read -r line; do sleep 30; done\n", + create_mode=CreateOrTruncate, + ) + assert_eq(@process.run("chmod", ["+x", engine.to_string()]), 0) + let actor = new_engine_actor( + engine.to_string(), + runtime_dir=runtime.to_path(), + ) + ignore( + update_settings(actor.manager(), { + provider: Some("custom"), + custom_api_url: Some("https://example.invalid/chat/completions"), + deepseek_api_key: None, + custom_api_key: None, + }), + ) + (root, actor) +} + +///| +#cfg(not(platform="windows")) +async test "a start cannot open a second record for an id another store owns" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + let (root, actor) = store_identity_host("openseek-start-unique-") + defer restore_session_root_env(previous) + let manager = actor.manager() + let owner = root.join("owner") + let ws = root.join("ws") + @fsx.ensure_dir(ws.to_path()) + @fsx.ensure_dir(owner.to_path()) + @fsx.ensure_dir( + @workspaces.store_root(owner).join("sessions/s-dup").to_path(), + ) + let sink = EventSink(fn(_) { }) + @async.with_task_group(group => { + group.spawn_bg(no_wait=true, allow_failure=true, () => { + actor.run(sink, fn(_) { }) + }) + while manager.pump is Stopped { + @async.sleep(1) + } + ignore(@workspaces.add(owner.to_string(), fn(_) { })) + ignore(@workspaces.add(ws.to_string(), fn(_) { })) + // The first project owns `s-dup`. Running it in the second would file a + // second record under the same id — the fork this refusal exists to stop. + let detail = try { + ignore( + start_run(sink, manager, { + task: "fork me", + submission_id: None, + model: None, + max_steps: None, + session: "s-dup", + workspace: Some(ws.to_string()), + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(detail.has_suffix("; start a new one here")) + assert_true(detail.contains(@workspaces.store_root(owner).to_string())) + guard manager.pump is Serving(sessions~) else { fail("pump stopped") } + assert_true(sessions.get("s-dup") is None) + // The refusal is about opening a *second* record, so it must not reach + // the ordinary cases: a genuinely new id in the project, and the store + // that already owns this one continuing it. + let fresh = start_run(sink, manager, { + task: "mine", + submission_id: None, + model: None, + max_steps: None, + session: "s-fresh", + workspace: Some(ws.to_string()), + }) + assert_eq(fresh.status, "accepted") + let owned = start_run(sink, manager, { + task: "continue", + submission_id: None, + model: None, + max_steps: None, + session: "s-dup", + workspace: Some(owner.to_string()), + }) + assert_eq(owned.status, "accepted") + group.return_immediately(()) + }) + @fs.rmdir(root.to_string(), recursive=true) +} + +///| +#cfg(not(platform="windows")) +async test "archiving one store's record ignores another store's live engine" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + let (root, actor) = store_identity_host("openseek-store-slot-") + defer restore_session_root_env(previous) + let manager = actor.manager() + let other = root.join("other") + let ws = root.join("ws") + @fsx.ensure_dir(ws.to_path()) + @fsx.ensure_dir(other.to_path()) + let sink = EventSink(fn(_) { }) + @async.with_task_group(group => { + group.spawn_bg(no_wait=true, allow_failure=true, () => { + actor.run(sink, fn(_) { }) + }) + while manager.pump is Stopped { + @async.sleep(1) + } + ignore(@workspaces.add(ws.to_string(), fn(_) { })) + ignore(@workspaces.add(other.to_string(), fn(_) { })) + // The first project's record runs a turn that never finishes. + let reply = start_run(sink, manager, { + task: "keep running", + submission_id: None, + model: None, + max_steps: None, + session: "s-dup", + workspace: Some(ws.to_string()), + }) + assert_eq(reply.status, "accepted") + // A record with the same id in the other project, idle — the CLI could + // have written it, or it travelled in with that directory. Its archive is + // the user's to take: the busy engine belongs to the first project's + // conversation. + let other_store = @workspaces.store_root(other) + @fsx.ensure_dir(other_store.join("sessions/s-dup").to_path()) + assert_true( + archive_session(manager, "s-dup", other.resource_path(), (_, _) => ()) + is Archived(_), + ) + assert_true( + @fsx.is_dir(other_store.join("archived/sessions/s-dup").to_path()), + ) + assert_false(@fsx.exists(other_store.join("sessions/s-dup").to_path())) + // The first project's conversation is untouched: same process, still running, + // and its follower still rooted in the store it tails. + guard manager.pump is Serving(sessions~) else { fail("pump stopped") } + guard sessions.get("s-dup") is Some(slot) && slot.engine is Some(live) else { + fail("the other store's engine was closed by an unrelated archive") + } + assert_true(live.phase is Running(..)) + assert_true(live.writes_store(@workspaces.store_root(ws))) + guard live.follower is Some(follower) else { fail("follower retired") } + assert_true(follower.tails_store(@workspaces.store_root(ws))) + assert_false(follower.retired) + group.return_immediately(()) + }) + @fs.rmdir(root.to_string(), recursive=true) +} diff --git a/desktop/internal/worktree/pkg.generated.mbti b/desktop/internal/worktree/pkg.generated.mbti index 4927115e6..bd2921d32 100644 --- a/desktop/internal/worktree/pkg.generated.mbti +++ b/desktop/internal/worktree/pkg.generated.mbti @@ -9,7 +9,7 @@ import { } // Values -pub async fn archive_bound_checkout(String, Bool, @pathx.Absolute?, on_committed? : (String, Array[@protocol.WorktreeInfo]) -> Unit) -> @protocol.ArchiveNeedsForceReply? +pub async fn archive_bound_checkout(String, Bool, @pathx.Absolute, on_committed? : (String, Array[@protocol.WorktreeInfo]) -> Unit) -> @protocol.ArchiveNeedsForceReply? pub async fn create(WorktreeHost, String, WorktreeOwner, (Array[@protocol.WorktreeInfo]) -> Unit, name? : String) -> @protocol.WorktreeCreateReply @@ -44,7 +44,7 @@ pub struct CodexWorktreeBinding { pub(all) struct WorktreeHost { lifecycle_lock : @async.Mutex session_is_live : (String) -> Bool - drain_sessions : async (Array[String]) -> Unit + drain_sessions : async (Array[String], @pathx.Absolute) -> Unit } type WorktreeOwner derive(Eq, @debug.Debug) diff --git a/desktop/internal/worktree/worktree.mbt b/desktop/internal/worktree/worktree.mbt index d7a52aebd..8cc9be15c 100644 --- a/desktop/internal/worktree/worktree.mbt +++ b/desktop/internal/worktree/worktree.mbt @@ -33,11 +33,14 @@ impl Show for WorktreeError with fn output(self, logger) { /// conversation already has a slot in the running pump (binding is only for /// conversations that are new everywhere), and `drain_sessions` closes the /// engines of conversations whose checkout is about to be deleted — an idle -/// serve process still has its cwd inside that directory. +/// serve process still has its cwd inside that directory. It takes the store +/// root beside the ids because engine slots are keyed by session id alone: a +/// same-id conversation recording in another store is a different one, and +/// draining it would tear down a writer this removal was never given. pub(all) struct WorktreeHost { lifecycle_lock : @async.Mutex session_is_live : (String) -> Bool - drain_sessions : async (Array[String]) -> Unit + drain_sessions : async (Array[String], @pathx.Absolute) -> Unit } ///| @@ -567,14 +570,13 @@ pub async fn WorktreeOwner::archive_checkout( /// rediscovered from the session id: a worktree belongs to the project that /// owns the record being moved, and looking the id up again would find the /// first registered project holding it — a forced archive would then delete -/// another project's uncommitted checkout. Scratch records own no worktree. +/// another project's uncommitted checkout. pub async fn archive_bound_checkout( session : String, force : Bool, - workspace : @pathx.Absolute?, + workspace : @pathx.Absolute, on_committed? : (String, Array[WorktreeInfo]) -> Unit, ) -> ArchiveNeedsForceReply? { - guard workspace is Some(workspace) else { return None } WorktreeOwner::openseek(session).archive_checkout_within( [workspace], force, @@ -1180,7 +1182,7 @@ pub async fn remove( ) } if entry.session is Some(bound) { - (host.drain_sessions)([bound]) + (host.drain_sessions)([bound], @workspaces.store_root(dir)) } let target = worktree_dir(dir, name) let present = @fsx.is_dir(target.to_path()) catch { @@ -1247,17 +1249,18 @@ fn restore_session_root_env(previous : String?) -> Unit { ///| /// A host with no engine behind it: `live` names the conversations the pump /// would report a slot for, and every drained session is appended to -/// `drained` instead of closing a process. +/// `drained` — with the store it was drained for — instead of closing a +/// process. fn test_host( live? : Array[String] = [], - drained? : Array[String] = [], + drained? : Array[(String, String)] = [], ) -> WorktreeHost { { lifecycle_lock: Mutex(), session_is_live: session => live.contains(session), - drain_sessions: sessions => { + drain_sessions: (sessions, root) => { for session in sessions { - drained.push(session) + drained.push((session, root.to_string())) } }, } diff --git a/docs/remote-protocol.md b/docs/remote-protocol.md index f8e6193d2..00feb3457 100644 --- a/docs/remote-protocol.md +++ b/docs/remote-protocol.md @@ -308,6 +308,21 @@ anything you can act on later; a client that persists a workspace session across a host restart must send that workspace back, because the record is unreachable without it once the store is no longer the default. +The host does not add to that population. `agent.start` opens a record in the +store its `workspace` selects, or continues one already there; a start that +would file a *second* record for an id another store owns is refused, naming +the store that holds it — the rule `worktree.create` has always applied to a +binding, now applied to the ordinary start path. Duplicate ids therefore +arrive only from outside the app (the `openseek` CLI takes `--session ` +verbatim under any `--session-root`, and a project's store travels inside the +project directory), which is why clients must still handle them rather than +assume they cannot occur. + +Because a host runs at most one engine per session id, a live engine is also +filed under the id alone. Every lifecycle op compares its store before +treating one as its own, so archiving a Scratch record is not refused by — and +does not close — a project conversation that merely shares the id. + Notifications: | method | params | From 9f871bd0d3980e54391903d467ed3bf20d39ea50 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 21:32:20 +0800 Subject: [PATCH 09/17] docs(desktop): fix the vocabulary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several words in this tree are ordinary English already spent on something specific, so a loose second use silently merges two concepts. Write them down. The load-bearing one is that every conversation answers two independent directory questions — where its record lives and where the agent works — and they coincide only for a plain project conversation. A worktree conversation records in its project's store while working in its own checkout, and a Scratch conversation's two directories are in unrelated trees, so neither may ever be derived from the other. "placement" already means the retained worktree registry row and the client's four-case view of a checkout; both are on the checkout axis, so it must never be stretched to cover a store. "root" is qualified everywhere except when it means a store's own root. --- desktop/AGENTS.md | 3 ++ desktop/CONTEXT.md | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 desktop/CONTEXT.md diff --git a/desktop/AGENTS.md b/desktop/AGENTS.md index 5d56c50b3..f8ea478b1 100644 --- a/desktop/AGENTS.md +++ b/desktop/AGENTS.md @@ -1,5 +1,8 @@ # Desktop Agent Notes +`CONTEXT.md` fixes the vocabulary — store, root, workspace, checkout, +placement, family. Read it before naming anything in this tree. + ## Frontend And Host Compatibility - The desktop frontend and host are versioned and shipped together. Do not diff --git a/desktop/CONTEXT.md b/desktop/CONTEXT.md new file mode 100644 index 000000000..4d11bd664 --- /dev/null +++ b/desktop/CONTEXT.md @@ -0,0 +1,88 @@ +# Desktop Vocabulary + +Words that mean one exact thing in `desktop/`. Several of them are ordinary +English that the codebase has already spent on something specific, so a loose +second use silently merges two concepts. Prefer these; when a term below says +"never", it is because that use once existed and cost a bug. + +## Two axes, never one + +Every conversation answers two independent directory questions. They coincide +for a plain project conversation, which is why they get conflated. + +| Conversation | Record lives in (store) | Agent works in (checkout) | +|---|---|---| +| Project `/p` | `/p/.openseek` | `/p` | +| Worktree `wt-1` of `/p` | `/p/.openseek` | `/p/.worktrees/wt-1` | + +The worktree row shows the two are siblings, neither containing the other. +Nothing may derive one from the other — each follows from its own input. + +## Durable identity + +- **session id** — a conversation's name. **Unique only within one store.** + Generators differ in collision resistance: desktop mints + `desktop-YYYYMMDD-HHMMSS-mmm-sssssssss` with a random salt + (`frontend/session.mbt`), the TUI's `tui-...` form has no salt + (`SessionId::generated`'s `salt?` defaults to empty), and the CLI takes + `--session ` verbatim. Never treat an id alone as an identity. +- **record** — one conversation's durable directory, `/sessions//`: + transcript, title, standing goal, review base. Deleting it is what "delete a + conversation" means; project files are never touched. +- **store** (session store) — the root a record lives under: one per + registered workspace (`@workspaces.store_root(w)` = `/.openseek`), + enumerated by `@session_store.known_roots()`. A store *has* a root; the two + words are not interchangeable. +- **root** — the path of a store, and the value passed as `--session-root`. + Only ever say "root" unqualified about a store. Other roots exist + (`workspace_root()`, `checkout_root`) and are checkouts, not stores — always + qualify those. +- **family** — a record plus every `-sr-N` descendant sub-run record beside it. + Records are flat sibling directories even though the sidebar draws a tree, so + a family is discovered by id structure (`is_descendant_session`), never by a + textual prefix. Archive, unarchive, and delete move a whole family or none of + it. +- **archived twin** — `/archived/sessions/`, the same layout one + level down, so listing archived conversations is one more `sessions list`. + `/archived/deleting/` holds condemned records; a rename into it is + permanent deletion's commit point. + +## Directories + +- **workspace** — a *registered project directory*. The host registry, never a + request, decides which directories qualify. Every conversation belongs to + one, and its store is that project's. +- **worktree** — `/.worktrees/`, a checkout owned by exactly + one conversation. Its records still live in the **project's** store. +- **checkout** / **cwd** — the directory the agent, terminals, and file + operations work in. Derived from (workspace, id) through the worktree + registry, never searched for. +- **placement** — already means two things, both about checkouts, and must + never be stretched to cover a store: (1) the retained worktree registry row + that survives archiving so an unarchive reads as `MissingTree` and can offer + a rebuild; (2) `CheckoutPlacement` (`frontend/interop/channel.mbt`), the + client's four-case view of a conversation's checkout. + +## Runtime + +- **serve engine** — one `openseek serve` child process. The host runs **at + most one per session id**; slots are keyed by id alone, so any operation + naming a store must ask whether a live engine actually writes that store + before treating it as its own. +- **slot** — a session id's entry in the manager's map: its engine, its pending + claim, its follower generation. +- **follower** — the actor tailing one record's durable tail. +- **run** — one turn: a prompt through its terminal event. A conversation + outlives its runs; a run never outlives its conversation. + +## Wire versus host + +The protocol has no word for a store. Every op addressing a record spells one +as `workspace`: the registered project's resource path the host itself +reported. The host validates it against the registry rather than trusting the +spelling, and selects that store exactly instead of searching. + +That one field answers both directory questions — which store holds the +record, and which project the run works in — because a project owns both. Say +which one you mean when the difference matters, as it does for a worktree +conversation, whose checkout is not its project's directory. From 45d03c87e53e571e7369ba2e3f41f6597c5d5fc4 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 21:32:33 +0800 Subject: [PATCH 10/17] refactor(desktop): carry the selected store as one value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A family move was handed three loose values that always travelled together: a root, the project owning it, and `workspace_token` — the protocol spelling of that project. They come from one place, `StoreSelection`, and every consumer had to keep them consistent by hand. `SessionFamilyMove` now carries that selection itself, so the store it moves records in, the owner permanent deletion drops a worktree placement from, and the token every `session.changed` callback carries are one value with one source. Nothing reachable changes. --- desktop/internal/engine/archive.mbt | 48 +++++++------------ desktop/internal/engine/config.mbt | 10 ++-- desktop/internal/engine/engine.mbt | 26 +++++----- desktop/internal/engine/follower.mbt | 6 +-- desktop/internal/engine/ops.mbt | 6 +-- .../internal/engine/worktree_seam_wbtest.mbt | 8 ++-- .../internal/session_store/pkg.generated.mbti | 1 + desktop/internal/session_store/store.mbt | 18 ++++--- 8 files changed, 59 insertions(+), 64 deletions(-) diff --git a/desktop/internal/engine/archive.mbt b/desktop/internal/engine/archive.mbt index 7a243042f..d1da20104 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -98,15 +98,12 @@ async fn ensure_new_record_is_unique( /// its siblings. priv struct SessionFamilyMove { manager : EngineManager - root : @pathx.Absolute - // The registered project whose store owns this family. Permanent deletion - // uses this exact owner to remove the retained worktree placement even if - // the project detaches meanwhile. - workspace : @pathx.Absolute - // Canonical protocol spelling of `workspace`. Every session.changed callback - // carries it so same-ID records in other stores do not inherit this family's + // The store holding every record this move claims. Permanent deletion uses + // its exact project owner to remove the retained worktree placement even if + // the project detaches meanwhile, and every session.changed callback carries + // its token so same-id records in other stores do not inherit this family's // disposition. - workspace_token : String + store : @session_store.StoreSelection members : Array[String] guards : Array[(String, SessionRecordGuard)] claims : Array[PendingClaim] @@ -205,8 +202,6 @@ async fn EngineManager::claim_session_family_move( } } } - let workspace = selected.workspace - let workspace_token = selected.workspace_token for session_id in members { let record_state = manager.claim_record_move(session_id) guards.push((session_id, record_state)) @@ -264,16 +259,7 @@ async fn EngineManager::claim_session_family_move( claim.require_current(manager) } handed_off = true - { - manager, - root, - workspace, - workspace_token, - members, - guards, - claims, - active: true, - } + { manager, store: selected, members, guards, claims, active: true } } ///| @@ -287,12 +273,12 @@ async fn SessionFamilyMove::remove_worktree_placements( self : SessionFamilyMove, on_committed? : (String, Array[WorktreeInfo]) -> Unit, ) -> Unit { - guard @worktree.drop_placements(self.workspace, self.members) + guard @worktree.drop_placements(self.store.workspace, self.members) is Some(remaining) else { return } if on_committed is Some(on_committed) { - on_committed(self.workspace_token, remaining) + on_committed(self.store.workspace_token, remaining) } } @@ -307,8 +293,10 @@ async fn move_session_family( to_archive~ : Bool, on_committed : (String, String) -> Unit, ) -> Unit { - let live = family.root.join("sessions") - let archived = @session_store.archived_root(family.root).join("sessions") + let live = family.store.root.join("sessions") + let archived = @session_store.archived_root(family.store.root).join( + "sessions", + ) let source = if to_archive { live } else { archived } let destination = if to_archive { archived } else { live } let order = if to_archive { @@ -361,7 +349,7 @@ async fn move_session_family( } } for session_id in order { - on_committed(session_id, family.workspace_token) + on_committed(session_id, family.store.workspace_token) } }) } @@ -449,7 +437,7 @@ async fn archive_session_commit( let refusal = @worktree.archive_bound_checkout( session, force, - family.workspace, + family.store.workspace, on_committed?=on_worktree_changed, ) if refusal is Some(refusal) { @@ -551,7 +539,7 @@ async fn delete_archived_session_commit( // Physical erasure happens after the commit: the records are already // invisible, so a failed or cancelled attempt here only defers their // cleanup to the next sweep instead of undoing a published deletion. - manager.sweep_archived_deletions_in(family.root) + manager.sweep_archived_deletions_in(family.store.root) } ///| @@ -566,8 +554,8 @@ async fn SessionFamilyMove::delete_archived_records( on_committed : (String, String) -> Unit, on_worktree_changed? : (String, Array[WorktreeInfo]) -> Unit, ) -> Unit { - let archived = @session_store.archived_root(self.root).join("sessions") - let deleting = @session_store.deleting_root(self.root) + let archived = @session_store.archived_root(self.store.root).join("sessions") + let deleting = @session_store.deleting_root(self.store.root) let order = self.members[1:].to_owned() order.push(self.members[0]) for session_id in order { @@ -620,7 +608,7 @@ async fn SessionFamilyMove::delete_archived_records( } } for session_id in order { - on_committed(session_id, self.workspace_token) + on_committed(session_id, self.store.workspace_token) } }) } diff --git a/desktop/internal/engine/config.mbt b/desktop/internal/engine/config.mbt index b2d06cb48..bffe479ec 100644 --- a/desktop/internal/engine/config.mbt +++ b/desktop/internal/engine/config.mbt @@ -235,7 +235,7 @@ test "a different endpoint cannot reuse a live engine process" { engine: "openseek", cwd: None, session: "s-fingerprint", - session_root: None, + session_root: Some(@pathx.Path("/sessions").resolve()), } let custom = { ..base, @@ -393,8 +393,8 @@ async test "host chooses durable placement from workspace state" { workspace: None, }) assert_eq( - config.session_root.map(root => root.to_string()), - Some(workspace + "/.openseek"), + config.session_root.map(root => root.to_string()).unwrap_or(""), + workspace + "/.openseek", ) assert_eq(config.cwd.map(cwd => cwd.to_string()), Some(workspace)) // A hint-less session that no attached workspace claims has nowhere to run. @@ -427,8 +427,8 @@ async test "host chooses durable placement from workspace state" { workspace: Some(workspace_resource), }) assert_eq( - attached.session_root.map(root => root.to_string()), - Some(workspace + "/.openseek"), + attached.session_root.unwrap().to_string(), + workspace + "/.openseek", ) assert_eq(attached.cwd.map(cwd => cwd.to_string()), Some(workspace)) @fs.rmdir(dir, recursive=true) diff --git a/desktop/internal/engine/engine.mbt b/desktop/internal/engine/engine.mbt index f19a01d62..c737622d7 100644 --- a/desktop/internal/engine/engine.mbt +++ b/desktop/internal/engine/engine.mbt @@ -18,9 +18,9 @@ priv struct ServeEngine { // The conversation this process serves: its slot in the manager's map. session_key : String - // Canonical durable store selected from host-owned workspace state when - // this process was spawned. Workspace detach uses this placement to close - // idle writers before hiding their store, and to refuse while one is busy. + // The durable store selected from host-owned workspace state when this + // process was spawned. Workspace detach uses it to close idle writers before + // hiding their store, and to refuse while one is busy. session_root : @pathx.Absolute? // Fingerprint of the config this process was spawned with; a prompt whose // config differs cannot reuse it. @@ -381,8 +381,7 @@ fn ServeEngine::is_current(self : ServeEngine, manager : EngineManager) -> Bool /// treating a live engine as its own. A same-id record in another store is a /// different conversation: blocking on its engine refuses a move that store /// never contended for, and closing it tears down a writer the operation was -/// never given. An engine spawned without a durable store writes no record at -/// all, so it belongs to no store's move. +/// never given. fn ServeEngine::writes_store( self : ServeEngine, root : @pathx.Absolute, @@ -1562,11 +1561,9 @@ async fn engine_process_env( // Desktop engines always run in durable-session mode, so the conversation // survives the process and is shared with the CLI/TUI. env["OPENSEEK_SESSION"] = config.session - if config.session_root is Some(root) { - env["OPENSEEK_SESSION_ROOT"] = root.to_string() - } else { - env.remove("OPENSEEK_SESSION_ROOT") - } + env["OPENSEEK_SESSION_ROOT"] = config.session_root + .map(root => root.to_string()) + .unwrap_or("") add_moonbit_path_for_native_engine(env, config, runtime_dir) env } @@ -2331,7 +2328,7 @@ async test "native engine env has one authoritative bundled moon path" { engine: engine.to_string(), cwd: None, session: "s-env", - session_root: None, + session_root: Some(@pathx.Path("/sessions").resolve()), } let env = engine_process_env(config, Some(runtime)) catch { error => { @@ -2343,7 +2340,12 @@ async test "native engine env has one authoritative bundled moon path" { } @fs.rmdir(root.to_string(), recursive=true) assert_eq(env["OPENSEEK_SESSION"], "s-env") - assert_true(env.get("OPENSEEK_SESSION_ROOT") is None) + // The child's store is the config's, never an ambient OPENSEEK_SESSION_ROOT + // inherited from the launching shell. + assert_eq( + env["OPENSEEK_SESSION_ROOT"], + @pathx.Path("/sessions").resolve().to_string(), + ) let moon_bin = moon_home.join("bin").to_string() let expected_path = if ambient_path is Some(path) && !path.is_empty() { "\{moon_bin}\{@env.path_sep}\{path}" diff --git a/desktop/internal/engine/follower.mbt b/desktop/internal/engine/follower.mbt index 3d9f56519..f9825deb8 100644 --- a/desktop/internal/engine/follower.mbt +++ b/desktop/internal/engine/follower.mbt @@ -504,9 +504,7 @@ fn EngineManager::kick_follower(self : EngineManager, session : String) -> Unit /// pump's spawn path, so the follower task lives in the same task group as /// the engines. The returned identity is present only when this call installed /// a new follower; its caller owns startup rollback until the engine reader -/// and consumer have both been scheduled. A session root can still be absent -/// only when the durable store cannot be resolved; in that case there is no -/// record for a follower to scan. +/// and consumer have both been scheduled. async fn[G] ensure_follower( sink~ : EventSink, group~ : @async.TaskGroup[G], @@ -1500,7 +1498,7 @@ async test "failed command stays unusable through a close timeout" { ) let engine : ServeEngine = { session_key: "s-timeout", - session_root: None, + session_root: Some(@pathx.Path("/sessions").resolve()), spec_key: "test", writer, commands: Queue(kind=Unbounded), diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index f9184738a..9973b2fa5 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -630,10 +630,10 @@ pub async fn load_archived_session( let record_guard = manager.begin_record_read(session) defer manager.end_record_read(session, record_guard) // The client names the store whose archived twin to read. - let store = @session_store.StoreSelection::from_workspace(payload.workspace).root - let root = @session_store.archived_root(store) + let live = @session_store.StoreSelection::from_workspace(payload.workspace).root + let root = @session_store.archived_root(live) if !@fsx.is_dir(root.join("sessions").join(session).to_path()) { - if @fsx.is_dir(store.join("sessions").join(session).to_path()) { + if @fsx.is_dir(live.join("sessions").join(session).to_path()) { raise EngineError("this conversation is not archived") } raise EngineError( diff --git a/desktop/internal/engine/worktree_seam_wbtest.mbt b/desktop/internal/engine/worktree_seam_wbtest.mbt index cea2cdcc7..035543397 100644 --- a/desktop/internal/engine/worktree_seam_wbtest.mbt +++ b/desktop/internal/engine/worktree_seam_wbtest.mbt @@ -210,8 +210,8 @@ async test "a bound session routes every start through its checkout" { ) // The durable record stays in the MAIN repository's store. assert_eq( - first.session_root.map(dir => dir.to_string()), - Some(ws.join(".openseek").to_string()), + first.session_root.unwrap().to_string(), + ws.join(".openseek").to_string(), ) // A hint-less resume goes through the durable record and the mapping. @fsx.ensure_dir(ws.join(".openseek/sessions/s-bind").to_path()) @@ -228,8 +228,8 @@ async test "a bound session routes every start through its checkout" { Some(ws.join(".worktrees/wt").to_string()), ) assert_eq( - resumed.session_root.map(dir => dir.to_string()), - Some(ws.join(".openseek").to_string()), + resumed.session_root.unwrap().to_string(), + ws.join(".openseek").to_string(), ) @fs.rmdir(root.to_string(), recursive=true) } diff --git a/desktop/internal/session_store/pkg.generated.mbti b/desktop/internal/session_store/pkg.generated.mbti index 947e1e9dd..0aa4275d6 100644 --- a/desktop/internal/session_store/pkg.generated.mbti +++ b/desktop/internal/session_store/pkg.generated.mbti @@ -37,6 +37,7 @@ pub struct StoreSelection { workspace_token : String } pub async fn StoreSelection::from_workspace(String) -> Self +pub fn StoreSelection::of_workspace(@pathx.Absolute) -> Self raise StoreError // Type aliases diff --git a/desktop/internal/session_store/store.mbt b/desktop/internal/session_store/store.mbt index 328b56478..750f67097 100644 --- a/desktop/internal/session_store/store.mbt +++ b/desktop/internal/session_store/store.mbt @@ -118,17 +118,23 @@ pub async fn StoreSelection::from_workspace( @workspaces.registered_dir(path) is Some(registered) else { raise StoreError("\{workspace} is not a registered workspace") } + StoreSelection::of_workspace(registered) +} + +///| +/// The store of a workspace the caller has already put through the registry. +/// `from_workspace` is the entry point that validates a client's spelling; +/// this one states the same value for a directory the host itself resolved. +pub fn StoreSelection::of_workspace( + workspace : @pathx.Absolute, +) -> StoreSelection raise StoreError { // Resource URIs use their authority for the device, so a UNC path has no // reversible encoding and is rejected explicitly instead of being published // under a new host. - guard registered.to_uri_path() is Some(workspace_token) else { + guard workspace.to_uri_path() is Some(workspace_token) else { raise StoreError("UNC paths are not supported by frontend resources") } - { - root: @workspaces.store_root(registered), - workspace: registered, - workspace_token, - } + { root: @workspaces.store_root(workspace), workspace, workspace_token } } ///| From 64f07529283eaff2f83daf21f235fd99097ed5ba Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 22:57:00 +0800 Subject: [PATCH 11/17] fix(desktop): write a session command only to its own store's engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for one project's record was written to the other project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886. --- desktop/internal/engine/config.mbt | 48 +++++--- desktop/internal/engine/ops.mbt | 43 ++++++- .../internal/engine/store_identity_wbtest.mbt | 115 +++++++++++++++++- 3 files changed, 184 insertions(+), 22 deletions(-) diff --git a/desktop/internal/engine/config.mbt b/desktop/internal/engine/config.mbt index bffe479ec..33e88e584 100644 --- a/desktop/internal/engine/config.mbt +++ b/desktop/internal/engine/config.mbt @@ -138,6 +138,33 @@ async fn session_command_config( serve_config("", manager, model?, max_steps?, session~, workspace?) } +///| +/// The store a session command addresses: the workspace it names while that +/// workspace is registered, else the one whose store already holds the +/// record. Every op that spawns an engine resolves it here, and so does every +/// op that must decide whether a live engine is writing the record it names — +/// the two answers have to agree. +async fn requested_store( + session : String, + workspace? : String, +) -> @pathx.Absolute { + if workspace is Some(path) { + guard @pathx.Absolute::from_uri_path(path) is Some(absolute) else { + raise EngineError("workspace is not a supported absolute resource path") + } + match @workspaces.registered_dir(absolute) { + Some(dir) => @workspaces.store_root(dir) + None => raise EngineError("\{path} is not a registered workspace") + } + } else if @session_store.root_of(session) is Some(root) { + root + } else { + raise EngineError( + "this conversation has no workspace; attach a project first", + ) + } +} + ///| /// The validated configuration shared by every op that can spawn a serve /// engine. The endpoint (provider, key, URL) comes from the host-owned @@ -155,27 +182,18 @@ async fn serve_config( let (settings, _) = manager.settings_snapshot() let endpoint = resolved_endpoint(settings) // A workspace run happens in the project directory itself, with the - // session's durable record in the in-project store. Only registered - // workspaces qualify — the registry, not the payload, decides which - // directories a run may work in. - let hinted : @pathx.Absolute? = if workspace is Some(path) { + // session's durable record in that project's store. The hint names one + // while it is registered; a hint-less client resuming an existing + // conversation recovers the workspace that already owns the record, so one + // id can never fork into two histories. + let dir = if workspace is Some(path) { guard @pathx.Absolute::from_uri_path(path) is Some(absolute) else { raise EngineError("workspace is not a supported absolute resource path") } match @workspaces.registered_dir(absolute) { - Some(dir) => Some(dir) + Some(dir) => dir None => raise EngineError("\{path} is not a registered workspace") } - } else { - None - } - // Every run belongs to a workspace. The hint names one; a hint-less client - // resuming an existing conversation recovers the workspace that already - // owns the durable record, so one id can never fork into two histories. - // With neither, there is nowhere to run — the client must attach a project - // rather than have the host invent a directory. - let dir = if hinted is Some(dir) { - dir } else if @session_store.workspace_of(session) is Some(dir) { dir } else { diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index 9973b2fa5..48d5dba9e 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -170,6 +170,37 @@ async fn ensure_run_cwd(config : RunConfig) -> Unit { } } +///| +/// The live engine a command addressed to `store` may be written to, if any. +/// +/// Session slots are keyed by id alone — the host runs at most one engine per +/// id — so the engine holding this slot may be the one writing a same-id +/// record in *another* store. Handing it the command would mutate that record +/// and route its events into this conversation, so a mismatched engine is not +/// returned: the caller spawns its own, which replaces it. Taking the slot +/// from an idle conversation costs it only a warm process (the next prompt +/// respawns, exactly as after a fingerprint change), but taking it from one +/// mid-turn would destroy work this request never named, so that is refused. +fn engine_writing_store( + slot : SessionSlot, + store : @pathx.Absolute, +) -> ServeEngine? raise EngineError { + guard slot.live() is Some(live) else { return None } + if live.writes_store(store) { + return Some(live) + } + guard live.phase is Idle(..) else { + let elsewhere = match live.session_root { + Some(root) => root.to_string() + None => "another store" + } + raise EngineError( + "a conversation with this id is still active in \{elsewhere}; wait for it to finish", + ) + } + None +} + ///| /// Ask the pump to spawn (or replace) this conversation's serve engine and /// wait for its ack; the spawn failure, if any, is re-raised here. @@ -214,6 +245,11 @@ pub async fn compact_run( let record_guard = manager.begin_record_op(session) defer manager.end_record_op(session, record_guard) @session_store.ensure_not_archived(session) + // Which record this compaction rewrites, resolved before the slot is read: + // the engine on that slot counts as this conversation's only if it writes + // the same store. Resolved here rather than from `compact_config` because a + // reused engine never builds one, and the store must be known either way. + let store = requested_store(session, workspace?=payload.workspace) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before anything else — even with a live engine. A start between @@ -225,7 +261,7 @@ pub async fn compact_run( defer claim.release(manager) placement_locked = false manager.workspace_lifecycle_lock.release() - let live = if slot.live() is Some(live) { + let live = if engine_writing_store(slot, store) is Some(live) { live } else { let config = compact_config(manager, payload) @@ -313,6 +349,9 @@ pub async fn goal_run( let record_guard = manager.begin_record_op(session) defer manager.end_record_op(session, record_guard) @session_store.ensure_not_archived(session) + // The goal is durable state of one record, so the store is resolved before + // the slot is read — see `compact_run`. + let store = requested_store(session, workspace?=payload.workspace) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before looking at the engine, exactly as compact_run does: this op @@ -322,7 +361,7 @@ pub async fn goal_run( defer claim.release(manager) placement_locked = false manager.workspace_lifecycle_lock.release() - let live = if slot.live() is Some(live) { + let live = if engine_writing_store(slot, store) is Some(live) { live } else { let config = goal_config(manager, payload) diff --git a/desktop/internal/engine/store_identity_wbtest.mbt b/desktop/internal/engine/store_identity_wbtest.mbt index e5c2a3cd5..8d14d3326 100644 --- a/desktop/internal/engine/store_identity_wbtest.mbt +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -5,16 +5,19 @@ // // * the desktop never *adds* to that population — a start opens a record in // the store it named or continues one already there, never a second twin; -// * lifecycle operations that address one store ignore live engines and -// followers serving another, even though both are filed under the id. +// * an operation that addresses one store — moving its record, compacting +// it, setting its goal — ignores live engines and followers serving +// another, even though both are filed under the id. // // Both are exercised against a real pump: the refusals under test are the // ones a live host issues, and a start that slips past them must show up as a // spawned engine rather than as a stalled request. ///| -/// A host whose engine binary accepts a prompt and then holds the turn open, -/// so a conversation can be left running while another store is acted on. +/// A host whose engine binary accepts every command and reports nothing, so a +/// conversation can be left mid-turn while another store is acted on. It reads +/// until stdin closes, so an engine this host replaces exits on EOF instead of +/// waiting out the graceful-close timeout. #cfg(not(platform="windows")) async fn store_identity_host(prefix : String) -> (@pathx.Absolute, EngineActor) { let root = worktree_test_root(prefix) @@ -31,7 +34,7 @@ async fn store_identity_host(prefix : String) -> (@pathx.Absolute, EngineActor) ) @fs.write_file( engine.to_string(), - "#!/bin/sh\nif [ \"$1\" = \"sessions\" ]; then printf '%s' '{\"events\":[]}' ; exit 0; fi\nwhile IFS= read -r line; do sleep 30; done\n", + "#!/bin/sh\nif [ \"$1\" = \"sessions\" ]; then printf '%s' '{\"events\":[]}' ; exit 0; fi\nwhile IFS= read -r line; do :; done\n", create_mode=CreateOrTruncate, ) assert_eq(@process.run("chmod", ["+x", engine.to_string()]), 0) @@ -125,6 +128,108 @@ async test "a start cannot open a second record for an id another store owns" { @fs.rmdir(root.to_string(), recursive=true) } +///| +#cfg(not(platform="windows")) +async test "a command addressed to one store never reaches a same-id twin's engine" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + let (root, actor) = store_identity_host("openseek-store-command-") + defer restore_session_root_env(previous) + let manager = actor.manager() + let owner = root.join("owner") + let other = root.join("other") + @fsx.ensure_dir(owner.to_path()) + @fsx.ensure_dir(other.to_path()) + let sink = EventSink(fn(_) { }) + @async.with_task_group(group => { + group.spawn_bg(no_wait=true, allow_failure=true, () => { + actor.run(sink, fn(_) { }) + }) + while manager.pump is Stopped { + @async.sleep(1) + } + ignore(@workspaces.add(owner.to_string(), fn(_) { })) + ignore(@workspaces.add(other.to_string(), fn(_) { })) + // Both projects hold a record under each id, so nothing below can be + // answered by which store happens to own the id. + for workspace in [owner, other] { + for session in ["s-busy", "s-idle"] { + @fsx.ensure_dir( + @workspaces.store_root(workspace) + .join("sessions") + .join(session) + .to_path(), + ) + } + } + // `owner`'s conversation holds an open turn. Compacting `other`'s record + // would be written to that engine — rewriting the wrong history and + // reporting its progress into this conversation — so it is refused by + // name rather than silently steered or silently replaced. + let started = start_run(sink, manager, { + task: "keep running", + submission_id: None, + model: None, + max_steps: None, + session: "s-busy", + workspace: Some(owner.to_string()), + }) + assert_eq(started.status, "accepted") + let detail = try { + ignore( + compact_run(manager, { + session: "s-busy", + model: None, + max_steps: None, + workspace: Some(other.to_string()), + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(detail.contains(@workspaces.store_root(owner).to_string())) + guard manager.pump is Serving(sessions~) else { fail("pump stopped") } + guard sessions.get("s-busy") is Some(busy) && busy.engine is Some(live) else { + fail("an unrelated compaction replaced the running engine") + } + assert_true(live.phase is Running(..)) + assert_true(live.writes_store(@workspaces.store_root(owner))) + // An idle twin holds nothing but a warm process, so the goal takes the + // slot instead of being refused — and lands on an engine writing the + // store it named. + ignore( + goal_run(manager, { + session: "s-idle", + text: Some("owner goal"), + auto: None, + model: None, + max_steps: None, + workspace: Some(owner.to_string()), + }), + ) + ignore( + goal_run(manager, { + session: "s-idle", + text: Some("other goal"), + auto: None, + model: None, + max_steps: None, + workspace: Some(other.to_string()), + }), + ) + guard sessions.get("s-idle") is Some(idle) && idle.engine is Some(rebound) else { + fail("the goal left no engine on the slot") + } + assert_true(rebound.writes_store(@workspaces.store_root(other))) + group.return_immediately(()) + }) + @fs.rmdir(root.to_string(), recursive=true) +} + ///| #cfg(not(platform="windows")) async test "archiving one store's record ignores another store's live engine" { From 2e1ddd2f54513972e9385dab44f77eb4ff7e526f Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 23:08:32 +0800 Subject: [PATCH 12/17] feat(desktop): name the store on every op that writes a record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `@session_store.workspace_of` — the first registered project whose store holds that id — so a conversation sharing an id with another project's record submitted into *that* project: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path. `@session_store.Store::from_workspace` is the single resolver, and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional placement for a record it never opened. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886. --- desktop/frontend/bridge.mbt | 38 +++-- desktop/frontend/update.mbt | 12 +- desktop/frontend/worktrees.mbt | 20 ++- desktop/internal/api/payload.mbt | 44 +++++- desktop/internal/engine/config.mbt | 141 ++++++++---------- desktop/internal/engine/ops.mbt | 26 ++-- desktop/internal/engine/settings.mbt | 6 +- .../internal/engine/store_identity_wbtest.mbt | 25 +++- .../internal/engine/worktree_seam_wbtest.mbt | 31 +--- .../internal/protocol/desktop_messages.mbt | 41 +++-- desktop/internal/protocol/pkg.generated.mbti | 6 +- docs/remote-protocol.md | 31 ++-- 12 files changed, 237 insertions(+), 184 deletions(-) diff --git a/desktop/frontend/bridge.mbt b/desktop/frontend/bridge.mbt index 186df1ac7..604398a49 100644 --- a/desktop/frontend/bridge.mbt +++ b/desktop/frontend/bridge.mbt @@ -1035,7 +1035,10 @@ fn start_openseek( session : String, workspace~ : @common.Uri, ) -> @cmd.Cmd { - guard @resource.belongs_to(workspace, channel) else { return @cmd.none } + let key : RecordKey = { session, workspace } + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none + } let payload = start_payload( task, selected_model, @@ -1108,19 +1111,20 @@ async fn request_agent_start( ///| /// The start payload. Optional fields are trimmed and omitted when blank. -/// `session` is required: every frontend conversation is minted with a -/// durable id before it can submit a prompt, and the host rejects a blank id. -/// It carries no `cwd` (`workspace`, the conversation's attached project, -/// is the only placement the host takes; everything else it derives from -/// the session id) and no credentials — the endpoint is -/// resolved host-side from the settings store. +/// `session` and `workspace` are the record this run writes: every frontend +/// conversation is minted with a durable id before it can submit a prompt, +/// and the store says which of the same-id records that id means. It carries +/// no `cwd` (`workspace`, the conversation's attached project, is the only +/// placement the host takes; everything else it derives from the session id) +/// and no credentials — the endpoint is resolved host-side from the settings +/// store. fn start_payload( task : String, selected_model : EngineModel, max_steps : String, session : String, submission_id : String, - workspace~ : @common.Uri, + workspace~ : String, ) -> @protocol.StartPayload { let session = session.trim().to_owned() { @@ -1129,7 +1133,7 @@ fn start_payload( model: Some(selected_model.wire()), max_steps: parsed_max_steps(max_steps), session, - workspace: Some(workspace.path), + workspace, } } @@ -1389,7 +1393,10 @@ fn compact_openseek( max_steps~ : String, workspace~ : @common.Uri, ) -> @cmd.Cmd { - guard @resource.belongs_to(workspace, channel) else { return @cmd.none } + let key : RecordKey = { session, workspace } + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none + } let dispatch = dispatch.map((msg : DeviceMsg) => FromDevice(channel, msg)) @cmd.custom_cmd(scheduler => { @js.async_run(() => { @@ -1401,9 +1408,9 @@ fn compact_openseek( @commands.agent_compact, { session, + workspace, model: Some(model.wire()), max_steps: parsed_max_steps(max_steps), - workspace: Some(workspace.path), }, lane=@interop.session_command_lane(channel, session), ) catch { @@ -1444,7 +1451,10 @@ fn goal_openseek( command : String, text : String?, ) -> @cmd.Cmd { - guard @resource.belongs_to(workspace, channel) else { return @cmd.none } + let key : RecordKey = { session, workspace } + guard key.workspace_payload(channel) is Some(workspace) else { + return @cmd.none + } let payload = goal_payload(session, model, max_steps, workspace, text) @cmd.custom_cmd(scheduler => { @js.async_run(() => { @@ -1472,11 +1482,12 @@ fn goal_payload( session : String, model : EngineModel, max_steps : String, - workspace : @common.Uri, + workspace : String, text : String?, ) -> @protocol.GoalPayload { { session, + workspace, text, // Always manual. `auto` arms the engine's autonomous continuation, and // the desktop cannot yet see the turns that continuation starts: serve @@ -1487,7 +1498,6 @@ fn goal_payload( auto: None, model: Some(model.wire()), max_steps: parsed_max_steps(max_steps), - workspace: Some(workspace.path), } } diff --git a/desktop/frontend/update.mbt b/desktop/frontend/update.mbt index a242dbe03..7c03c47ff 100644 --- a/desktop/frontend/update.mbt +++ b/desktop/frontend/update.mbt @@ -4932,9 +4932,10 @@ fn update_device_msg( } // A commit or Started broadcast can materialize a conversation before // session.list tells this page which durable store owns it. Reconcile - // that provisional placement from the list before any later send: an - // empty workspace hint here would otherwise fork the same session into - // the default store. + // that provisional placement from the list before any later send: the + // store this page believes in is the one every request names, so a + // provisional Scratch placement would send the next prompt to a Scratch + // record instead of the project one the conversation came from. let stores = listed_stores(items) let conversations = model.conversations.map(conv => { if conv.device == channel { @@ -7451,13 +7452,16 @@ fn running_model() -> Model { ///| fn encoded_start_workspace(conv : Conversation) -> String? { + guard conv.record_key().workspace_payload(conv.device) is Some(workspace) else { + return None + } let payload = start_payload( "continue", High, "1000", conv.session_id, "test-submission", - workspace=conv.project(), + workspace~, ) let json = payload.to_json() guard json is Object(fields) else { return None } diff --git a/desktop/frontend/worktrees.mbt b/desktop/frontend/worktrees.mbt index b84612ce4..502262e24 100644 --- a/desktop/frontend/worktrees.mbt +++ b/desktop/frontend/worktrees.mbt @@ -185,13 +185,16 @@ fn start_in_new_worktree( WorktreeCreated(workspace~, name=reply.name, session~), ), ) + // The store is this project: `start_in_new_worktree` accepted the + // workspace for this channel, and a worktree conversation records + // in its project's store while working in the checkout. let start = start_payload( task, selected_model, max_steps, session, submission_id, - workspace~, + workspace=workspace.path, ) request_agent_start( scheduler, dispatch, channel, start, session, submission_id, @@ -329,8 +332,13 @@ fn set_goal_in_new_worktree( WorktreeCreated(workspace~, name=reply.name, session~), ), ) + // The project this goal's record lives in — see the start chain. let payload = goal_payload( - session, engine_model, max_steps, workspace, text, + session, + engine_model, + max_steps, + workspace.path, + text, ) request_agent_goal( scheduler, dispatch, channel, payload, session, command, text, @@ -1712,12 +1720,12 @@ test "a start payload never names a worktree" { "1000", "desktop-wt", "sub-1", - workspace~, + workspace=workspace.path, ) - // The typed wire shape has no worktree field at all; the workspace hint - // is the only placement the start carries. + // The typed wire shape has no worktree field at all; the store the record + // lives in is the only placement the start carries. assert_true( payload.to_json() is Object(fields) && fields.get("worktree") is None, ) - assert_eq(payload.workspace, Some("/proj")) + assert_eq(payload.workspace, "/proj") } diff --git a/desktop/internal/api/payload.mbt b/desktop/internal/api/payload.mbt index d53a95808..629ef1e3a 100644 --- a/desktop/internal/api/payload.mbt +++ b/desktop/internal/api/payload.mbt @@ -114,6 +114,7 @@ test "start and steer payloads preserve submission ids" { let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", + "workspace": "", "submission_id": "submission-start", }) assert_eq(start.submission_id, Some("submission-start")) @@ -130,6 +131,7 @@ test "start and steer payloads may omit submission ids" { let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", + "workspace": "", }) assert_true(start.submission_id is None) let steer : @protocol.SteerPayload = @json.from_json({ "text": "more detail" }) @@ -138,41 +140,75 @@ test "start and steer payloads may omit submission ids" { ///| test "client-authored session roots are ignored" { + // A client names its store as `workspace`, which the host validates against + // its registry. `session_root` is a host-derived fact it reports back, never + // one it accepts: a payload carrying one is decoded without it. let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", + "workspace": "", "session_root": "/client/chosen/store", }) assert_eq(start.session, "s-start") - assert_true(start.workspace is None) + assert_eq(start.workspace, "") let compact : @protocol.CompactPayload = @json.from_json({ "session": "s-compact", + "workspace": "", "session_root": "/client/chosen/store", }) assert_eq(compact.session, "s-compact") - assert_true(compact.workspace is None) + assert_eq(compact.workspace, "") } ///| test "start payload requires a session" { let decoded : @protocol.StartPayload? = Some( - @json.from_json({ "task": "hello" }), + @json.from_json({ "task": "hello", "workspace": "" }), ) catch { _ => None } assert_true(decoded is None) } +///| +test "a run payload without a store is not a valid request" { + // Half of a record's identity. A start, compaction, or goal that omitted it + // would leave the host inferring the store from the id — the search that + // hands one store's command to another store's record. + let start : @protocol.StartPayload? = Some( + @json.from_json({ "task": "hello", "session": "s-start" }), + ) catch { + _ => None + } + assert_true(start is None) + let compact : @protocol.CompactPayload? = Some( + @json.from_json({ "session": "s-compact" }), + ) catch { + _ => None + } + assert_true(compact is None) + let goal : @protocol.GoalPayload? = Some( + @json.from_json({ "session": "s-goal", "text": "ship it" }), + ) catch { + _ => None + } + assert_true(goal is None) +} + ///| test "goal payload distinguishes set from clear by the text field" { let set : @protocol.GoalPayload = @json.from_json({ "session": "s-goal", + "workspace": "", "text": "ship the feature", "model": "deepseek-v4-pro", }) assert_eq(set.text, Some("ship the feature")) assert_eq(set.model, Some("deepseek-v4-pro")) - let clear : @protocol.GoalPayload = @json.from_json({ "session": "s-goal" }) + let clear : @protocol.GoalPayload = @json.from_json({ + "session": "s-goal", + "workspace": "", + }) assert_true(clear.text is None) } diff --git a/desktop/internal/engine/config.mbt b/desktop/internal/engine/config.mbt index 33e88e584..3edddc4eb 100644 --- a/desktop/internal/engine/config.mbt +++ b/desktop/internal/engine/config.mbt @@ -83,7 +83,7 @@ async fn run_config( model?=payload.model, max_steps?=payload.max_steps, session~, - workspace?=payload.workspace, + workspace=payload.workspace, ) } @@ -102,7 +102,7 @@ async fn compact_config( payload.session, model?=payload.model, max_steps?=payload.max_steps, - workspace?=payload.workspace, + workspace=payload.workspace, ) } @@ -119,7 +119,7 @@ async fn goal_config( payload.session, model?=payload.model, max_steps?=payload.max_steps, - workspace?=payload.workspace, + workspace=payload.workspace, ) } @@ -129,40 +129,36 @@ async fn session_command_config( session : String, model? : String, max_steps? : Int, - workspace? : String, + workspace~ : String, ) -> RunConfig { let session = session.trim().to_owned() guard !session.is_empty() else { raise EngineError("session id must not be empty") } - serve_config("", manager, model?, max_steps?, session~, workspace?) + serve_config("", manager, model?, max_steps?, session~, workspace~) } ///| -/// The store a session command addresses: the workspace it names while that -/// workspace is registered, else the one whose store already holds the -/// record. Every op that spawns an engine resolves it here, and so does every -/// op that must decide whether a live engine is writing the record it names — -/// the two answers have to agree. -async fn requested_store( - session : String, - workspace? : String, -) -> @pathx.Absolute { - if workspace is Some(path) { - guard @pathx.Absolute::from_uri_path(path) is Some(absolute) else { - raise EngineError("workspace is not a supported absolute resource path") - } - match @workspaces.registered_dir(absolute) { - Some(dir) => @workspaces.store_root(dir) - None => raise EngineError("\{path} is not a registered workspace") - } - } else if @session_store.root_of(session) is Some(root) { - root - } else { - raise EngineError( - "this conversation has no workspace; attach a project first", - ) +/// The registered project a request names. The registry, not the payload, +/// decides which directories a run may work in, so a path that is not +/// registered — a workspace the user detached, say — is refused by name +/// rather than redirected to whatever else holds the id. +async fn named_workspace(workspace : String) -> @pathx.Absolute { + guard @pathx.Absolute::from_uri_path(workspace) is Some(absolute) else { + raise EngineError("workspace is not a supported absolute resource path") } + match @workspaces.registered_dir(absolute) { + Some(dir) => dir + None => raise EngineError("\{workspace} is not a registered workspace") + } +} + +///| +/// The store a session command addresses. Every op that spawns an engine +/// resolves it here, and so does every op that must decide whether a live +/// engine is writing the record it names — the two answers have to agree. +async fn requested_store(workspace : String) -> @pathx.Absolute { + @workspaces.store_root(named_workspace(workspace)) } ///| @@ -170,37 +166,26 @@ async fn requested_store( /// engine. The endpoint (provider, key, URL) comes from the host-owned /// settings store, never from the request — a client carries no credentials /// (docs/remote-protocol.md). +/// +/// A workspace run happens in the project directory itself, with the session's +/// durable record in the in-project store. Only registered workspaces qualify: +/// the registry, not the payload, decides which directories a run may work in. async fn serve_config( task : String, manager : EngineManager, model? : String, max_steps? : Int, session~ : String, - workspace? : String, + workspace~ : String, ) -> RunConfig { let engine = manager.engine let (settings, _) = manager.settings_snapshot() let endpoint = resolved_endpoint(settings) // A workspace run happens in the project directory itself, with the - // session's durable record in that project's store. The hint names one - // while it is registered; a hint-less client resuming an existing - // conversation recovers the workspace that already owns the record, so one - // id can never fork into two histories. - let dir = if workspace is Some(path) { - guard @pathx.Absolute::from_uri_path(path) is Some(absolute) else { - raise EngineError("workspace is not a supported absolute resource path") - } - match @workspaces.registered_dir(absolute) { - Some(dir) => dir - None => raise EngineError("\{path} is not a registered workspace") - } - } else if @session_store.workspace_of(session) is Some(dir) { - dir - } else { - raise EngineError( - "this conversation has no workspace; attach a project first", - ) - } + // session's durable record in that project's store. The request names the + // workspace; nothing here recovers it from the id, so one id can never fork + // into two histories behind a client that forgot which store it meant. + let dir = named_workspace(workspace) { task, api_key: endpoint.api_key, @@ -335,7 +320,7 @@ async test "run config ignores OPENSEEK_API_URL in favor of the settings" { model: None, max_steps: None, session: "s-default", - workspace: Some(workspace), + workspace, }) assert_true(config.api_url is None) assert_eq(config.api_key, "sk-official") @@ -354,7 +339,7 @@ async test "run config rejects a blank session" { model: None, max_steps: None, session, - workspace: None, + workspace: "", }), ) "no error" @@ -387,7 +372,9 @@ async test "host chooses durable placement from workspace state" { let dir = @fs.realpath(@fs.tmpdir(prefix="openseek-config-workspace-")) let global_root = dir + "/global" let workspace = dir + "/workspace" + let other = dir + "/other" let workspace_resource = @pathx.Path(workspace).resolve().resource_path() + let other_resource = @pathx.Path(other).resolve().resource_path() @sys.set_env_var("OPENSEEK_SESSION_ROOT", global_root) defer (if previous is Some(value) { @sys.set_env_var("OPENSEEK_SESSION_ROOT", value) @@ -396,37 +383,52 @@ async test "host chooses durable placement from workspace state" { }) @fsx.ensure_dir(Path(global_root)) @fsx.ensure_dir(Path(workspace + "/.openseek/sessions/s-workspace")) + @fsx.ensure_dir(Path(other + "/.openseek")) @fs.write_file( global_root + "/workspaces.json", - ({ "workspaces": [workspace] } : Json).stringify(), + ({ "workspaces": [workspace, other] } : Json).stringify(), create_mode=CreateOrTruncate, ) let manager = new_engine_manager("openseek") - let config = run_config(manager, { + // The store comes from what the request named, never from which store + // happens to hold the id: a request that names the other project is that + // project's request, even for an id this one owns. (`start_run` refuses + // this particular one — it would open a second record for an id another + // store owns — but the placement it refuses is decided here.) + let elsewhere = run_config(manager, { task: "resume", submission_id: None, model: None, max_steps: None, session: "s-workspace", - workspace: None, + workspace: other_resource, + }) + assert_eq(elsewhere.session_root.unwrap().to_string(), other + "/.openseek") + let attached = run_config(manager, { + task: "attached", + submission_id: None, + model: None, + max_steps: None, + session: "s-workspace", + workspace: workspace_resource, }) assert_eq( - config.session_root.map(root => root.to_string()).unwrap_or(""), + attached.session_root.unwrap().to_string(), workspace + "/.openseek", ) - assert_eq(config.cwd.map(cwd => cwd.to_string()), Some(workspace)) - // A hint-less session that no attached workspace claims has nowhere to run. - // The host refuses instead of inventing a directory and a store for it, - // which is what would fork one id into two histories. - let unclaimed = try { + assert_eq(attached.cwd.map(cwd => cwd.to_string()), Some(workspace)) + // An unregistered directory is refused by name. The registry, not the + // payload, decides which directories a run may work in — and a refusal + // keeps a stale client from silently filing the record in Scratch instead. + let detail = try { ignore( run_config(manager, { - task: "fresh", + task: "detached", submission_id: None, model: None, max_steps: None, - session: "s-unclaimed", - workspace: None, + session: "s-workspace", + workspace: @pathx.Path(dir + "/elsewhere").resolve().resource_path(), }), ) "no error" @@ -435,19 +437,6 @@ async test "host chooses durable placement from workspace state" { error if @async.is_being_cancelled() => raise error error => "\{error}" } - assert_true(unclaimed.contains("has no workspace")) - let attached = run_config(manager, { - task: "attached", - submission_id: None, - model: None, - max_steps: None, - session: "s-attached", - workspace: Some(workspace_resource), - }) - assert_eq( - attached.session_root.unwrap().to_string(), - workspace + "/.openseek", - ) - assert_eq(attached.cwd.map(cwd => cwd.to_string()), Some(workspace)) + assert_true(detail.has_suffix("is not a registered workspace")) @fs.rmdir(dir, recursive=true) } diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index 48d5dba9e..0543d4f5f 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -249,7 +249,7 @@ pub async fn compact_run( // the engine on that slot counts as this conversation's only if it writes // the same store. Resolved here rather than from `compact_config` because a // reused engine never builds one, and the store must be known either way. - let store = requested_store(session, workspace?=payload.workspace) + let store = requested_store(payload.workspace) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before anything else — even with a live engine. A start between @@ -351,7 +351,7 @@ pub async fn goal_run( @session_store.ensure_not_archived(session) // The goal is durable state of one record, so the store is resolved before // the slot is read — see `compact_run`. - let store = requested_store(session, workspace?=payload.workspace) + let store = requested_store(payload.workspace) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before looking at the engine, exactly as compact_run does: this op @@ -902,7 +902,7 @@ async test "start echoes the client submission id in Started" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }), ) let mut echoed : String? = None @@ -979,7 +979,7 @@ async test "a goal is delivered only once its command is on the wire" { auto: None, model: None, max_steps: None, - workspace: Some(workspace), + workspace, }) assert_true(delivered.delivered) // The reply already means the bytes left this host, so the child has them @@ -1108,7 +1108,7 @@ async test "setup failure retires the writer before an immediate next start" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }) assert_true( @async.with_timeout_opt(3000, () => { @@ -1134,7 +1134,7 @@ async test "setup failure retires the writer before an immediate next start" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }) assert_eq(second.status, "accepted") assert_true(@fsx.exists(old_done)) @@ -1231,7 +1231,7 @@ async test "a prompt bigger than the pipe still settles exactly once" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }) assert_eq(reply.status, "accepted") assert_eq(started.val, Some(reply.run_id)) @@ -1327,7 +1327,7 @@ async test "named session retries after its first prompt creates no record" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }) assert_eq(first.status, "accepted") assert_eq(started.val, Some(first.run_id)) @@ -1367,7 +1367,7 @@ async test "named session retries after its first prompt creates no record" { model: None, max_steps: None, session, - workspace: Some(workspace), + workspace, }) assert_eq(second.status, "accepted") assert_true( @@ -1579,7 +1579,7 @@ async test "workspace detach refuses a running conversation without hiding it" { model: None, max_steps: None, session: "s-detach-running", - workspace: Some(workspace.to_string()), + workspace: workspace.to_string(), }) assert_eq(reply.status, "accepted") let committed : Ref[Bool] = Ref(false) @@ -1665,7 +1665,7 @@ async test "workspace detach drains an idle writer and its follower first" { model: None, max_steps: None, session: "s-detach-idle", - workspace: Some(workspace.to_string()), + workspace: workspace.to_string(), }) assert_true( @async.with_timeout_opt(3000, () => { @@ -1718,7 +1718,7 @@ async test "a goal request is refused before it can reach the wrong engine" { auto: Some(true), model: None, max_steps: None, - workspace: None, + workspace: "", }).contains("autonomous goal continuation is not supported yet"), ) // Manual, named: past the refusal and on to the engine, which is where a @@ -1730,7 +1730,7 @@ async test "a goal request is refused before it can reach the wrong engine" { auto: None, model: None, max_steps: None, - workspace: None, + workspace: "", }).contains("not supported yet"), ) } diff --git a/desktop/internal/engine/settings.mbt b/desktop/internal/engine/settings.mbt index eaa2292e2..1701ac507 100644 --- a/desktop/internal/engine/settings.mbt +++ b/desktop/internal/engine/settings.mbt @@ -741,7 +741,7 @@ async test "run config resolves the endpoint from the stored provider" { model: None, max_steps: None, session: "s-settings-default", - workspace: Some(workspace), + workspace, }) // The official endpoint is the engine default: no URL override. assert_true(config.api_url is None) @@ -758,7 +758,7 @@ async test "run config resolves the endpoint from the stored provider" { model: None, max_steps: None, session: "s-settings-custom", - workspace: Some(workspace), + workspace, }) assert_true( custom.api_url is Some("https://proxy.example/chat/completions"), @@ -785,7 +785,7 @@ async test "a custom provider without a URL refuses to start" { model: None, max_steps: None, session: "s-settings-missing-url", - workspace: None, + workspace: "", }) catch { EngineError(message) => message diff --git a/desktop/internal/engine/store_identity_wbtest.mbt b/desktop/internal/engine/store_identity_wbtest.mbt index 8d14d3326..9179faef2 100644 --- a/desktop/internal/engine/store_identity_wbtest.mbt +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -89,7 +89,7 @@ async test "a start cannot open a second record for an id another store owns" { model: None, max_steps: None, session: "s-dup", - workspace: Some(ws.to_string()), + workspace: ws.to_string(), }), ) "no error" @@ -111,18 +111,27 @@ async test "a start cannot open a second record for an id another store owns" { model: None, max_steps: None, session: "s-fresh", - workspace: Some(ws.to_string()), + workspace: ws.to_string(), }) assert_eq(fresh.status, "accepted") + // Now the project holds `s-dup` too — copied in by the CLI, or carried in + // with the project directory. The Scratch conversation continues its own + // record: the store it named places the run, and the id it shares with a + // project record decides nothing. + @fsx.ensure_dir(@workspaces.store_root(ws).join("sessions/s-dup").to_path()) let owned = start_run(sink, manager, { task: "continue", submission_id: None, model: None, max_steps: None, session: "s-dup", - workspace: Some(owner.to_string()), + workspace: owner.to_string(), }) assert_eq(owned.status, "accepted") + guard sessions.get("s-dup") is Some(slot) && slot.engine is Some(live) else { + fail("the accepted start left no engine on the slot") + } + assert_true(live.writes_store(@workspaces.store_root(owner))) group.return_immediately(()) }) @fs.rmdir(root.to_string(), recursive=true) @@ -173,7 +182,7 @@ async test "a command addressed to one store never reaches a same-id twin's engi model: None, max_steps: None, session: "s-busy", - workspace: Some(owner.to_string()), + workspace: owner.to_string(), }) assert_eq(started.status, "accepted") let detail = try { @@ -182,7 +191,7 @@ async test "a command addressed to one store never reaches a same-id twin's engi session: "s-busy", model: None, max_steps: None, - workspace: Some(other.to_string()), + workspace: other.to_string(), }), ) "no error" @@ -208,7 +217,7 @@ async test "a command addressed to one store never reaches a same-id twin's engi auto: None, model: None, max_steps: None, - workspace: Some(owner.to_string()), + workspace: owner.to_string(), }), ) ignore( @@ -218,7 +227,7 @@ async test "a command addressed to one store never reaches a same-id twin's engi auto: None, model: None, max_steps: None, - workspace: Some(other.to_string()), + workspace: other.to_string(), }), ) guard sessions.get("s-idle") is Some(idle) && idle.engine is Some(rebound) else { @@ -260,7 +269,7 @@ async test "archiving one store's record ignores another store's live engine" { model: None, max_steps: None, session: "s-dup", - workspace: Some(ws.to_string()), + workspace: ws.to_string(), }) assert_eq(reply.status, "accepted") // A record with the same id in the other project, idle — the CLI could diff --git a/desktop/internal/engine/worktree_seam_wbtest.mbt b/desktop/internal/engine/worktree_seam_wbtest.mbt index 035543397..a795f10e0 100644 --- a/desktop/internal/engine/worktree_seam_wbtest.mbt +++ b/desktop/internal/engine/worktree_seam_wbtest.mbt @@ -202,7 +202,7 @@ async test "a bound session routes every start through its checkout" { model: None, max_steps: None, session: "s-bind", - workspace: Some(ws.to_string()), + workspace: ws.to_string(), }) assert_eq( first.cwd.map(dir => dir.to_string()), @@ -213,7 +213,8 @@ async test "a bound session routes every start through its checkout" { first.session_root.unwrap().to_string(), ws.join(".openseek").to_string(), ) - // A hint-less resume goes through the durable record and the mapping. + // A resume names the same store and goes through the registry again: the + // binding is the registry's, not the first start's. @fsx.ensure_dir(ws.join(".openseek/sessions/s-bind").to_path()) let resumed = run_config(manager, { task: "resume", @@ -221,7 +222,7 @@ async test "a bound session routes every start through its checkout" { model: None, max_steps: None, session: "s-bind", - workspace: None, + workspace: ws.to_string(), }) assert_eq( resumed.cwd.map(dir => dir.to_string()), @@ -292,7 +293,7 @@ async test "worktree remove refuses a running conversation without unmapping it" model: None, max_steps: None, session: "s-wt-run", - workspace: Some(ws.to_string()), + workspace: ws.to_string(), }) assert_eq(reply.status, "accepted") let committed : Ref[Bool] = Ref(false) @@ -403,7 +404,7 @@ async test "a checkout deleted outside the app refuses runs and is repairable" { model: None, max_steps: None, session: "s-vanish", - workspace: Some(repo.to_string()), + workspace: repo.to_string(), }), ) "no error" @@ -413,26 +414,6 @@ async test "a checkout deleted outside the app refuses runs and is repairable" { error => "\{error}" } assert_true(hinted_refusal.contains("wt") && hinted_refusal.contains("gone")) - let resumed_refusal = try { - ignore( - run_config(manager, { - task: "resume", - submission_id: None, - model: None, - max_steps: None, - session: "s-vanish", - workspace: None, - }), - ) - "no error" - } catch { - EngineError(detail) => detail - error if @async.is_being_cancelled() => raise error - error => "\{error}" - } - assert_true( - resumed_refusal.contains("wt") && resumed_refusal.contains("gone"), - ) // Recreating repairs the SAME environment: same name, same branch, and the // branch's commit is back in the working tree. let repaired = @worktree.create( diff --git a/desktop/internal/protocol/desktop_messages.mbt b/desktop/internal/protocol/desktop_messages.mbt index b8335ceee..22ef70441 100644 --- a/desktop/internal/protocol/desktop_messages.mbt +++ b/desktop/internal/protocol/desktop_messages.mbt @@ -7,6 +7,15 @@ // request channel, so scheme and authority never appear in these strings. // UNC paths are outside this contract. Workspace-relative fields remain // slash-separated relative strings. +// +// Session ids are unique only within one durable store, so every payload that +// addresses a record names the store beside the id. The store is spelled as +// the host-reported project resource path, or `""` for the global Scratch +// store — the same encoding `session.changed` and the sidebar's listings +// report. The host still validates project paths against its registry rather +// than trusting a client to name an arbitrary directory, and it selects that +// store exactly instead of searching, so a same-id record in another store can +// never be read, written, or moved by mistake. ///| pub(all) struct StartPayload { @@ -19,11 +28,11 @@ pub(all) struct StartPayload { // Every desktop run belongs to a durable conversation. The engine rejects // a blank id before it acquires that conversation's record lease. session : String - // The registered workspace directory this conversation works in. A - // session bound to one of that workspace's worktrees (`worktree.create` - // binds at creation) runs in that checkout — the start itself never names - // or mutates worktrees. - workspace : String? + // The store this run's record lives in: the registered workspace directory + // this conversation works in. A session bound to one of that workspace's + // worktrees (`worktree.create` binds at creation) runs in that checkout — + // the start itself never names or mutates worktrees. + workspace : String } derive(Debug, Eq, FromJson, ToJson) ///| @@ -53,20 +62,23 @@ pub(all) struct RunsPayload { ///| pub(all) struct CompactPayload { - // Compaction addresses a durable conversation rather than a live run. + // Compaction addresses a durable conversation rather than a live run: the + // record it rewrites is `(workspace, session)`. session : String + workspace : String // A resumed conversation may need a new engine process with these settings. model : String? max_steps : Int? - workspace : String? } derive(Debug, Eq, FromJson, ToJson) ///| pub(all) struct GoalPayload { // Like compaction, a standing goal addresses a durable conversation rather // than a live run: the serve engine accepts one in any phase (idle append, - // mid-turn steer, queued behind a compaction). + // mid-turn steer, queued behind a compaction). Like compaction, the goal is + // durable state of the record `(workspace, session)`. session : String + workspace : String // The new standing goal; absent clears it. text : String? // Arms the engine's autonomous continuation for this goal. Optional, and @@ -76,16 +88,15 @@ pub(all) struct GoalPayload { // A resumed conversation may need a new engine process with these settings. model : String? max_steps : Int? - workspace : String? } derive(Debug, Eq, FromJson, ToJson) // Session ids are unique only within one durable store, so every op that -// addresses an existing record names the store beside the id: the -// host-reported project resource path, the same spelling `session.changed` -// and the sidebar's listings report. The host validates it against its -// registry rather than trusting a client to name an arbitrary directory, and -// selects that store exactly instead of searching, so a same-id record in -// another store can never be read or moved by mistake. +// addresses a record names the store beside the id: the host-reported project +// resource path, the same spelling `session.changed` and the sidebar's +// listings report. The host validates it against its registry rather than +// trusting a client to name an arbitrary directory, and selects that store +// exactly instead of searching, so a same-id record in another store can +// never be read, written, or moved by mistake. ///| /// `session.load` / `session.load_archived` read one durable record in one diff --git a/desktop/internal/protocol/pkg.generated.mbti b/desktop/internal/protocol/pkg.generated.mbti index 7bc7aa100..1a88cf8d7 100644 --- a/desktop/internal/protocol/pkg.generated.mbti +++ b/desktop/internal/protocol/pkg.generated.mbti @@ -370,9 +370,9 @@ pub impl @json.FromJson for CodexTurnSteerPayload pub(all) struct CompactPayload { session : String + workspace : String model : String? max_steps : Int? - workspace : String? } derive(Eq, ToJson, @debug.Debug, @json.FromJson) pub(all) struct CompactReply { @@ -551,11 +551,11 @@ pub(all) struct GitPullRequestReply { pub(all) struct GoalPayload { session : String + workspace : String text : String? auto : Bool? model : String? max_steps : Int? - workspace : String? } derive(Eq, ToJson, @debug.Debug, @json.FromJson) pub(all) struct GoalReply { @@ -975,7 +975,7 @@ pub(all) struct StartPayload { model : String? max_steps : Int? session : String - workspace : String? + workspace : String } derive(Eq, ToJson, @debug.Debug, @json.FromJson) pub(all) struct StartReply { diff --git a/docs/remote-protocol.md b/docs/remote-protocol.md index 00feb3457..9efc337c0 100644 --- a/docs/remote-protocol.md +++ b/docs/remote-protocol.md @@ -260,11 +260,11 @@ echoes. | method | params | result | |---|---|---| -| `agent.start` | `{task, session, submission_id?, model?, max_steps?, workspace?}` — `session` is a required non-blank durable conversation id. No credentials or store path are accepted: the host resolves settings and durable placement; `workspace` is honored only when registered. A session bound to one of the workspace's worktrees (`worktree.create` binds at creation) runs in that checkout — the start never names or mutates worktrees | `{run_id, status, …}` — `accepted` after the complete prompt command is written; a post-`started` write failure returns `failed`, while pre-`started` failures use the error response | +| `agent.start` | `{task, session, workspace, submission_id?, model?, max_steps?}` — `(workspace, session)` is the record this run writes: a required non-blank conversation id, and the store holding it (see *Naming the store* below). No credentials or store root are accepted — the host resolves settings, and honors `workspace` only while registered. A session bound to one of the workspace's worktrees (`worktree.create` binds at creation) runs in that checkout — the start never names or mutates worktrees | `{run_id, status, …}` — `accepted` after the complete prompt command is written; a post-`started` write failure returns `failed`, while pre-`started` failures use the error response | | `agent.cancel` | `{run_id?}` (absent = the latest run) | cancel outcome | | `agent.steer` | `{text, run_id?, submission_id?}` | steer outcome | -| `agent.compact` | `{session, model?, max_steps?, workspace?}` — `agent.start` minus `task`: a conversation resumed after a restart has no live process, and compacting spawns one with these settings | compaction outcome | -| `agent.goal` | `{session, text?, auto?, model?, max_steps?, workspace?}` — sets the session's standing goal to `text`, or clears it when `text` is absent; the engine settings match `agent.compact`'s, and a blank `session` is refused before engine lookup. `auto` arms the engine's autonomous continuation and is **currently rejected**: serve announces the turns it starts with `goal_continue`, which this host does not yet fold into a run's lifecycle, so an autonomous turn would leave the engine looking idle to `agent.start` | `{delivered}` — delivery, not durability: the command reached a live engine's stdin. The goal itself is confirmed by the `[goal]` / `[goal cleared]` runtime-notice arriving as a `session.event` commit, which is also what clients should render from; the engine's `goal_updated` stream event duplicates it | +| `agent.compact` | `{session, workspace, model?, max_steps?}` — `agent.start` minus `task`: the same required record identity, and a conversation resumed after a restart has no live process, so compacting spawns one with these settings | compaction outcome | +| `agent.goal` | `{session, workspace, text?, auto?, model?, max_steps?}` — sets the record's standing goal to `text`, or clears it when `text` is absent; the record identity and engine settings match `agent.compact`'s, and a blank `session` is refused before engine lookup. `auto` arms the engine's autonomous continuation and is **currently rejected**: serve announces the turns it starts with `goal_continue`, which this host does not yet fold into a run's lifecycle, so an autonomous turn would leave the engine looking idle to `agent.start` | `{delivered}` — delivery, not durability: the command reached a live engine's stdin. The goal itself is confirmed by the `[goal]` / `[goal cleared]` runtime-notice arriving as a `session.event` commit, which is also what clients should render from; the engine's `goal_updated` stream event duplicates it | | `agent.runs` | `{known?: [{session, run_id?, submission_id?}]}` — each selector must carry a run or submission id; `{}` remains valid | `{runs: […], settled: […]}` — every in-flight run's `agent.started` params plus selector-matched `{run_id, session, submission_id?, status, exit_code?, durable_sequence?}` lifecycle settlements. Normal Terminal-backed statuses are immediately replayable; abnormal statuses appear only with an exact durable sequence. Active and settled state are captured atomically | Notifications: @@ -291,15 +291,16 @@ Notifications: #### Naming the store -A session id is unique only within one durable store, so every op above that -addresses an existing record carries `workspace` beside the id: the -host-reported project resource path, the same spelling `session.changed` and -both listings report. The host +A session id is unique only within one durable store, so every op that +addresses a record carries `workspace` beside the id — the `session.*` ops +above, and `agent.start` / `agent.compact` / `agent.goal`, which write one. +It is the host-reported project resource path, the same spelling +`session.changed` and both listings report. The host validates a project path against its registry (a detached workspace is refused by name) and then selects that store exactly. It never falls back to -searching, so a same-id record in another store can be neither read nor moved -by mistake, and a client that guesses gets an error instead of the wrong -conversation. +searching, so a same-id record in another store can be neither read, written, +nor moved by mistake, and a client that guesses gets an error instead of the +wrong conversation. This matters because `session.list` really can return two rows with one id — one per attached project — and a client that keeps only `session` is @@ -319,9 +320,13 @@ project directory), which is why clients must still handle them rather than assume they cannot occur. Because a host runs at most one engine per session id, a live engine is also -filed under the id alone. Every lifecycle op compares its store before -treating one as its own, so archiving a Scratch record is not refused by — and -does not close — a project conversation that merely shares the id. +filed under the id alone. Every op compares its store before treating one as +its own, so archiving a Scratch record is not refused by — and does not close +— a project conversation that merely shares the id, and a compaction or goal +is never written to an engine serving the other store's record. When that +engine is idle, the command replaces it (the conversation loses only a warm +process); when it is mid-turn, the command is refused, naming the store that +holds it. Notifications: From c5ac204dcc4e16ec8ef1cc0302e640d4656a447f Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sat, 15 Aug 2026 23:13:53 +0800 Subject: [PATCH 13/17] fix(desktop): let a record's own store decide whether it is archived MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `@session_store.archived_root_of` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886. --- desktop/internal/engine/ops.mbt | 12 +-- .../internal/engine/store_identity_wbtest.mbt | 75 ++++++++++++++++++- .../internal/session_store/pkg.generated.mbti | 4 +- desktop/internal/session_store/store.mbt | 33 ++++---- desktop/internal/worktree/worktree.mbt | 7 +- docs/remote-protocol.md | 9 ++- 6 files changed, 107 insertions(+), 33 deletions(-) diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index 0543d4f5f..71ad23432 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -20,9 +20,9 @@ pub async fn start_run( let mut placement_locked = true defer (if placement_locked { manager.workspace_lifecycle_lock.release() }) // Acquire the persistent record lease from the request's canonical key - // before run_config probes live/workspace stores. Otherwise an unarchive - // can move the record between that probe and the archived check, leaving a - // stale global session_root that forks the conversation. + // before the record is read at all: without it an archive could move this + // record between the check below and the prompt, and the turn would append + // to a record no list still shows. let key = payload.session.trim().to_owned() // The record lease is acquired before `run_config`, so reject a blank key // here as well as at that standalone config boundary. There is no shared @@ -33,7 +33,7 @@ pub async fn start_run( let record_guard = manager.begin_record_op(key) defer manager.end_record_op(key, record_guard) let config = run_config(manager, payload) - @session_store.ensure_not_archived(key) + @session_store.ensure_not_archived(key, config.session_root) ensure_new_record_is_unique(key, selected?=config.session_root) let sessions = manager.serving() let slot = slot_for(sessions, key) @@ -244,12 +244,12 @@ pub async fn compact_run( let session = payload.session.trim().to_owned() let record_guard = manager.begin_record_op(session) defer manager.end_record_op(session, record_guard) - @session_store.ensure_not_archived(session) // Which record this compaction rewrites, resolved before the slot is read: // the engine on that slot counts as this conversation's only if it writes // the same store. Resolved here rather than from `compact_config` because a // reused engine never builds one, and the store must be known either way. let store = requested_store(payload.workspace) + @session_store.ensure_not_archived(session, Some(store)) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before anything else — even with a live engine. A start between @@ -348,10 +348,10 @@ pub async fn goal_run( } let record_guard = manager.begin_record_op(session) defer manager.end_record_op(session, record_guard) - @session_store.ensure_not_archived(session) // The goal is durable state of one record, so the store is resolved before // the slot is read — see `compact_run`. let store = requested_store(payload.workspace) + @session_store.ensure_not_archived(session, Some(store)) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before looking at the engine, exactly as compact_run does: this op diff --git a/desktop/internal/engine/store_identity_wbtest.mbt b/desktop/internal/engine/store_identity_wbtest.mbt index 9179faef2..be387430b 100644 --- a/desktop/internal/engine/store_identity_wbtest.mbt +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -1,16 +1,18 @@ // A session id names a record only inside one store, so two attached projects // can hold the same id — the `openseek` CLI takes `--session ` verbatim // under any `--session-root`, and a project's store travels inside the project -// directory. Two host behaviors follow, and neither can be read off the id: +// directory. Three host behaviors follow, and none can be read off the id: // // * the desktop never *adds* to that population — a start opens a record in // the store it named or continues one already there, never a second twin; // * an operation that addresses one store — moving its record, compacting // it, setting its goal — ignores live engines and followers serving -// another, even though both are filed under the id. +// another, even though both are filed under the id; +// * a record's own store decides whether it is archived, so an archived +// twin elsewhere never holds a live conversation hostage. // -// Both are exercised against a real pump: the refusals under test are the -// ones a live host issues, and a start that slips past them must show up as a +// All are exercised against a real pump: the refusals under test are the ones +// a live host issues, and a start that slips past them must show up as a // spawned engine rather than as a stalled request. ///| @@ -137,6 +139,71 @@ async test "a start cannot open a second record for an id another store owns" { @fs.rmdir(root.to_string(), recursive=true) } +///| +#cfg(not(platform="windows")) +async test "an archived record in another store does not block a live one" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + let (root, actor) = store_identity_host("openseek-archived-twin-") + defer restore_session_root_env(previous) + let manager = actor.manager() + let other = root.join("other") + let ws = root.join("ws") + @fsx.ensure_dir(ws.to_path()) + @fsx.ensure_dir(other.to_path()) + let sink = EventSink(fn(_) { }) + @async.with_task_group(group => { + group.spawn_bg(no_wait=true, allow_failure=true, () => { + actor.run(sink, fn(_) { }) + }) + while manager.pump is Stopped { + @async.sleep(1) + } + ignore(@workspaces.add(ws.to_string(), fn(_) { })) + ignore(@workspaces.add(other.to_string(), fn(_) { })) + // The other project archived its `s-twin`; this project's `s-twin` is a + // different conversation and is live. + @fsx.ensure_dir( + @workspaces.store_root(other).join("archived/sessions/s-twin").to_path(), + ) + @fsx.ensure_dir( + @workspaces.store_root(ws).join("sessions/s-twin").to_path(), + ) + let reply = start_run(sink, manager, { + task: "continue", + submission_id: None, + model: None, + max_steps: None, + session: "s-twin", + workspace: ws.to_string(), + }) + assert_eq(reply.status, "accepted") + // The store that did archive it still refuses: this is a scope, not a + // removal of the defense. + let detail = try { + ignore( + start_run(sink, manager, { + task: "resurrect", + submission_id: None, + model: None, + max_steps: None, + session: "s-twin", + workspace: other.to_string(), + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(detail.contains("archived")) + group.return_immediately(()) + }) + @fs.rmdir(root.to_string(), recursive=true) +} + ///| #cfg(not(platform="windows")) async test "a command addressed to one store never reaches a same-id twin's engine" { diff --git a/desktop/internal/session_store/pkg.generated.mbti b/desktop/internal/session_store/pkg.generated.mbti index 0aa4275d6..338c893ec 100644 --- a/desktop/internal/session_store/pkg.generated.mbti +++ b/desktop/internal/session_store/pkg.generated.mbti @@ -9,11 +9,9 @@ import { // Values pub fn archived_root(@pathx.Absolute) -> @pathx.Absolute -pub async fn archived_root_of(String) -> @pathx.Absolute? - pub fn deleting_root(@pathx.Absolute) -> @pathx.Absolute -pub async fn ensure_not_archived(String) -> Unit +pub async fn ensure_not_archived(String, @pathx.Absolute?) -> Unit pub async fn family_in(@pathx.Absolute, String) -> Array[String] diff --git a/desktop/internal/session_store/store.mbt b/desktop/internal/session_store/store.mbt index 750f67097..b9cddb1c2 100644 --- a/desktop/internal/session_store/store.mbt +++ b/desktop/internal/session_store/store.mbt @@ -138,23 +138,22 @@ pub fn StoreSelection::of_workspace( } ///| -/// Any store whose `archived` twin holds this id. Unlike the ops that move or -/// read one named record, the archived-correlation defense guarding a start -/// has no store to go on: it guards a request that carries only an id, so it -/// must search. -pub async fn archived_root_of(session : String) -> @pathx.Absolute? { - guard !session.is_empty() else { return None } - for root in known_roots() { - if @fsx.is_dir(archived_root(root).join("sessions").join(session).to_path()) { - return Some(root) - } - } - None -} - -///| -pub async fn ensure_not_archived(session : String) -> Unit { - if archived_root_of(session) is Some(_) { +/// Refuse to write a record its own store has archived. An archived record is +/// a read-only snapshot, and appending to one would resurrect a conversation +/// the user put away without it reappearing in any list. +/// +/// The check is scoped to the store the request named, because an archived +/// record in another store is a different conversation: searching every store +/// would make a live conversation impossible to continue — its prompts, +/// compactions, and goals all refused as "archived" — until an unrelated twin +/// was restored or deleted. +pub async fn ensure_not_archived( + session : String, + store : @pathx.Absolute?, +) -> Unit { + guard !session.is_empty() && store is Some(store) else { return } + let archived = archived_root(store).join("sessions").join(session) + if @fsx.is_dir(archived.to_path()) { raise StoreError("session is archived; unarchive it first") } } diff --git a/desktop/internal/worktree/worktree.mbt b/desktop/internal/worktree/worktree.mbt index 8cc9be15c..a79bd848f 100644 --- a/desktop/internal/worktree/worktree.mbt +++ b/desktop/internal/worktree/worktree.mbt @@ -810,7 +810,12 @@ pub async fn create( } require_main_worktree(dir) if owner is OpenSeekSession(session) { - @session_store.ensure_not_archived(session) + // The checkout binds to a record in this project's store; an archived + // record elsewhere under the same id is a different conversation. + @session_store.ensure_not_archived( + session, + Some(@workspaces.store_root(dir)), + ) } // Snapshot this workspace's setting once for the lifecycle operation. A // concurrent page edit applies to the next create or repair instead of diff --git a/docs/remote-protocol.md b/docs/remote-protocol.md index 9efc337c0..e8dd435ca 100644 --- a/docs/remote-protocol.md +++ b/docs/remote-protocol.md @@ -309,6 +309,11 @@ anything you can act on later; a client that persists a workspace session across a host restart must send that workspace back, because the record is unreachable without it once the store is no longer the default. +An archived record is read-only, so a write to one is refused ("session is +archived") — but only when the store the request named archived it. An +archived record under the same id in another store is a different +conversation and does not hold this one back. + The host does not add to that population. `agent.start` opens a record in the store its `workspace` selects, or continues one already there; a start that would file a *second* record for an id another store owns is refused, naming @@ -321,8 +326,8 @@ assume they cannot occur. Because a host runs at most one engine per session id, a live engine is also filed under the id alone. Every op compares its store before treating one as -its own, so archiving a Scratch record is not refused by — and does not close -— a project conversation that merely shares the id, and a compaction or goal +its own, so archiving one project's record is not refused by — and does not +close — another project's conversation that merely shares the id, and a compaction or goal is never written to an engine serving the other store's record. When that engine is idle, the command replaces it (the conversation loses only a warm process); when it is mid-turn, the command is refused, naming the store that From 003a943f4f9d14acf00f6056d153237dedf4397f Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Sun, 16 Aug 2026 01:42:23 +0800 Subject: [PATCH 14/17] refactor(desktop): define the codecs of every payload this change touched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `desktop/AGENTS.md` requires an explicit `FromJson`/`ToJson` for anything crossing the JSON boundary, and requires replacing a derived codec in the same change that modifies its type. This change gave `agent.start`, `agent.compact`, `agent.goal`, `session.load`(`_archived`) and `session.archive`/`unarchive` a required `workspace`, and all five still derived theirs. The five codecs now say what the derive left implicit, and say it the same way the neighbouring payloads in this file already do. A required field must be present and of its type; `null` is not a value for one, so it is refused rather than read as absence — which for `workspace` is the point, since reading it as an absent field would leave the host guessing which store was meant. An optional field reads `null` and absence alike as `None` and refuses any other type, so a `null` `text` still clears a goal rather than setting an empty one. Unknown fields are ignored, which is what lets a host accept and drop a client-authored `session_root`. The encoder writes every required field and omits an absent optional instead of emitting `null`. Reported by Codex on #886. --- desktop/internal/api/payload.mbt | 79 +++++ .../internal/protocol/desktop_messages.mbt | 300 +++++++++++++++++- desktop/internal/protocol/pkg.generated.mbti | 20 +- 3 files changed, 385 insertions(+), 14 deletions(-) diff --git a/desktop/internal/api/payload.mbt b/desktop/internal/api/payload.mbt index 629ef1e3a..9df51a755 100644 --- a/desktop/internal/api/payload.mbt +++ b/desktop/internal/api/payload.mbt @@ -195,6 +195,85 @@ test "a run payload without a store is not a valid request" { assert_true(goal is None) } +///| +test "a required field is not satisfied by null or a wrong type" { + // `null` is not a store: reading it as an absent field would leave the host + // guessing which store the request meant, the silent misplacement the + // required field exists to prevent. + let null_store : @protocol.SessionPayload? = Some( + @json.from_json({ "session": "s-move", "workspace": Json::null() }), + ) catch { + _ => None + } + assert_true(null_store is None) + let wrong_type : @protocol.LoadSessionPayload? = Some( + @json.from_json({ "session": "s-load", "workspace": 7 }), + ) catch { + _ => None + } + assert_true(wrong_type is None) +} + +///| +test "an absent optional reads the same as an explicit null" { + let absent : @protocol.StartPayload = @json.from_json({ + "task": "hello", + "session": "s-start", + "workspace": "", + }) + let explicit : @protocol.StartPayload = @json.from_json({ + "task": "hello", + "session": "s-start", + "workspace": "", + "model": Json::null(), + "max_steps": Json::null(), + "submission_id": Json::null(), + }) + assert_true( + absent.model is None && + absent.max_steps is None && + absent.submission_id is None, + ) + assert_true( + explicit.model is None && + explicit.max_steps is None && + explicit.submission_id is None, + ) + // A malformed optional is still refused: absence is a value, a number is a + // client defect. + let malformed : @protocol.StartPayload? = Some( + @json.from_json({ + "task": "hello", + "session": "s-start", + "workspace": "", + "model": 7, + }), + ) catch { + _ => None + } + assert_true(malformed is None) +} + +///| +test "an absent optional is encoded as an absent key, never as null" { + let payload : @protocol.GoalPayload = { + session: "s-goal", + workspace: "", + text: None, + auto: None, + model: None, + max_steps: None, + } + guard payload.to_json() is Object(fields) else { + fail("goal payload is a JSON object") + } + assert_eq(fields.get("session"), Some(Json::string("s-goal"))) + assert_eq(fields.get("workspace"), Some(Json::string(""))) + for name in ["text", "auto", "model", "max_steps"] { + assert_true(fields.get(name) is None) + } +} + ///| test "goal payload distinguishes set from clear by the text field" { let set : @protocol.GoalPayload = @json.from_json({ diff --git a/desktop/internal/protocol/desktop_messages.mbt b/desktop/internal/protocol/desktop_messages.mbt index 22ef70441..7337f0f30 100644 --- a/desktop/internal/protocol/desktop_messages.mbt +++ b/desktop/internal/protocol/desktop_messages.mbt @@ -9,10 +9,9 @@ // slash-separated relative strings. // // Session ids are unique only within one durable store, so every payload that -// addresses a record names the store beside the id. The store is spelled as -// the host-reported project resource path, or `""` for the global Scratch -// store — the same encoding `session.changed` and the sidebar's listings -// report. The host still validates project paths against its registry rather +// addresses a record names the store beside the id: the host-reported project +// resource path, the same encoding `session.changed` and the sidebar's +// listings report. The host still validates project paths against its registry rather // than trusting a client to name an arbitrary directory, and it selects that // store exactly instead of searching, so a same-id record in another store can // never be read, written, or moved by mistake. @@ -33,7 +32,88 @@ pub(all) struct StartPayload { // worktrees (`worktree.create` binds at creation) runs in that checkout — // the start itself never names or mutates worktrees. workspace : String -} derive(Debug, Eq, FromJson, ToJson) +} derive(Debug, Eq) + +///| +/// The record-addressing payloads below answer the same questions the same +/// way. A required field must be present and of its type; `null` is not a +/// value for one, so it is refused rather than read as absence. An optional +/// field reads `null` and absence alike as `None`, and refuses any other type. +/// Unknown fields are ignored, which is what lets a host accept and drop a +/// client-authored `session_root`. The encoder writes every required field and +/// omits an absent optional instead of emitting `null`. +/// +/// `workspace` is required, so a client that omits it is rejected here rather +/// than leaving the host to guess which store it meant. +pub impl FromJson for StartPayload with fn from_json(json, path) { + guard json is Object(fields) else { + raise JsonDecodeError((path, "expected agent start payload")) + } + let task = match fields.get("task") { + Some(String(value)) => value + Some(_) => raise JsonDecodeError((path.add_key("task"), "expected string")) + None => + raise JsonDecodeError((path.add_key("task"), "missing required field")) + } + let submission_id = match fields.get("submission_id") { + Some(String(value)) => Some(value) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError( + (path.add_key("submission_id"), "expected string or null"), + ) + } + let model = match fields.get("model") { + Some(String(value)) => Some(value) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("model"), "expected string or null")) + } + let max_steps = match fields.get("max_steps") { + Some(Number(value, ..)) => Some(value.to_int()) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError( + (path.add_key("max_steps"), "expected integer or null"), + ) + } + let session = match fields.get("session") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("session"), "expected string")) + None => + raise JsonDecodeError((path.add_key("session"), "missing required field")) + } + let workspace = match fields.get("workspace") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("workspace"), "expected string")) + None => + raise JsonDecodeError( + (path.add_key("workspace"), "missing required field"), + ) + } + { task, submission_id, model, max_steps, session, workspace } +} + +///| +pub impl ToJson for StartPayload with fn to_json(self) { + let fields : Map[String, Json] = { + "task": Json::string(self.task), + "session": Json::string(self.session), + "workspace": Json::string(self.workspace), + } + if self.submission_id is Some(submission_id) { + fields["submission_id"] = Json::string(submission_id) + } + if self.model is Some(model) { + fields["model"] = Json::string(model) + } + if self.max_steps is Some(max_steps) { + fields["max_steps"] = Json::number(max_steps.to_double()) + } + Json::object(fields) +} ///| pub(all) struct CancelPayload { @@ -69,7 +149,60 @@ pub(all) struct CompactPayload { // A resumed conversation may need a new engine process with these settings. model : String? max_steps : Int? -} derive(Debug, Eq, FromJson, ToJson) +} derive(Debug, Eq) + +///| +pub impl FromJson for CompactPayload with fn from_json(json, path) { + guard json is Object(fields) else { + raise JsonDecodeError((path, "expected agent compact payload")) + } + let session = match fields.get("session") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("session"), "expected string")) + None => + raise JsonDecodeError((path.add_key("session"), "missing required field")) + } + let workspace = match fields.get("workspace") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("workspace"), "expected string")) + None => + raise JsonDecodeError( + (path.add_key("workspace"), "missing required field"), + ) + } + let model = match fields.get("model") { + Some(String(value)) => Some(value) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("model"), "expected string or null")) + } + let max_steps = match fields.get("max_steps") { + Some(Number(value, ..)) => Some(value.to_int()) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError( + (path.add_key("max_steps"), "expected integer or null"), + ) + } + { session, workspace, model, max_steps } +} + +///| +pub impl ToJson for CompactPayload with fn to_json(self) { + let fields : Map[String, Json] = { + "session": Json::string(self.session), + "workspace": Json::string(self.workspace), + } + if self.model is Some(model) { + fields["model"] = Json::string(model) + } + if self.max_steps is Some(max_steps) { + fields["max_steps"] = Json::number(max_steps.to_double()) + } + Json::object(fields) +} ///| pub(all) struct GoalPayload { @@ -88,7 +221,82 @@ pub(all) struct GoalPayload { // A resumed conversation may need a new engine process with these settings. model : String? max_steps : Int? -} derive(Debug, Eq, FromJson, ToJson) +} derive(Debug, Eq) + +///| +pub impl FromJson for GoalPayload with fn from_json(json, path) { + guard json is Object(fields) else { + raise JsonDecodeError((path, "expected agent goal payload")) + } + let session = match fields.get("session") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("session"), "expected string")) + None => + raise JsonDecodeError((path.add_key("session"), "missing required field")) + } + let workspace = match fields.get("workspace") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("workspace"), "expected string")) + None => + raise JsonDecodeError( + (path.add_key("workspace"), "missing required field"), + ) + } + // Absent text is the clear, so `null` and absence must stay the same value: + // a goal that decoded `null` into an empty string would set an empty goal + // instead of clearing the one already there. + let text = match fields.get("text") { + Some(String(value)) => Some(value) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("text"), "expected string or null")) + } + let auto = match fields.get("auto") { + Some(True) => Some(true) + Some(False) => Some(false) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("auto"), "expected boolean or null")) + } + let model = match fields.get("model") { + Some(String(value)) => Some(value) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("model"), "expected string or null")) + } + let max_steps = match fields.get("max_steps") { + Some(Number(value, ..)) => Some(value.to_int()) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError( + (path.add_key("max_steps"), "expected integer or null"), + ) + } + { session, workspace, text, auto, model, max_steps } +} + +///| +pub impl ToJson for GoalPayload with fn to_json(self) { + let fields : Map[String, Json] = { + "session": Json::string(self.session), + "workspace": Json::string(self.workspace), + } + if self.text is Some(text) { + fields["text"] = Json::string(text) + } + if self.auto is Some(auto) { + fields["auto"] = Json::boolean(auto) + } + if self.model is Some(model) { + fields["model"] = Json::string(model) + } + if self.max_steps is Some(max_steps) { + fields["max_steps"] = Json::number(max_steps.to_double()) + } + Json::object(fields) +} // Session ids are unique only within one durable store, so every op that // addresses a record names the store beside the id: the host-reported project @@ -104,7 +312,36 @@ pub(all) struct GoalPayload { pub(all) struct LoadSessionPayload { session : String workspace : String -} derive(Debug, Eq, FromJson, ToJson) +} derive(Debug, Eq) + +///| +pub impl FromJson for LoadSessionPayload with fn from_json(json, path) { + guard json is Object(fields) else { + raise JsonDecodeError((path, "expected session load payload")) + } + let session = match fields.get("session") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("session"), "expected string")) + None => + raise JsonDecodeError((path.add_key("session"), "missing required field")) + } + let workspace = match fields.get("workspace") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("workspace"), "expected string")) + None => + raise JsonDecodeError( + (path.add_key("workspace"), "missing required field"), + ) + } + { session, workspace } +} + +///| +pub impl ToJson for LoadSessionPayload with fn to_json(self) { + { "session": self.session, "workspace": self.workspace } +} ///| /// `session.archive` / `session.unarchive` move one durable record between @@ -115,7 +352,52 @@ pub(all) struct SessionPayload { session : String workspace : String force : Bool? -} derive(Debug, Eq, FromJson, ToJson) +} derive(Debug, Eq) + +///| +pub impl FromJson for SessionPayload with fn from_json(json, path) { + guard json is Object(fields) else { + raise JsonDecodeError((path, "expected session move payload")) + } + let session = match fields.get("session") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("session"), "expected string")) + None => + raise JsonDecodeError((path.add_key("session"), "missing required field")) + } + let workspace = match fields.get("workspace") { + Some(String(value)) => value + Some(_) => + raise JsonDecodeError((path.add_key("workspace"), "expected string")) + None => + raise JsonDecodeError( + (path.add_key("workspace"), "missing required field"), + ) + } + // Absence is the default, not a third state: a client that has not shown its + // discard dialog simply omits `force`. + let force = match fields.get("force") { + Some(True) => Some(true) + Some(False) => Some(false) + Some(Null) | None => None + Some(_) => + raise JsonDecodeError((path.add_key("force"), "expected boolean or null")) + } + { session, workspace, force } +} + +///| +pub impl ToJson for SessionPayload with fn to_json(self) { + let fields : Map[String, Json] = { + "session": Json::string(self.session), + "workspace": Json::string(self.workspace), + } + if self.force is Some(force) { + fields["force"] = Json::boolean(force) + } + Json::object(fields) +} ///| /// Permanent deletion addresses one archived record in one exact store. diff --git a/desktop/internal/protocol/pkg.generated.mbti b/desktop/internal/protocol/pkg.generated.mbti index 1a88cf8d7..5a4767a06 100644 --- a/desktop/internal/protocol/pkg.generated.mbti +++ b/desktop/internal/protocol/pkg.generated.mbti @@ -373,7 +373,9 @@ pub(all) struct CompactPayload { workspace : String model : String? max_steps : Int? -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) +} derive(Eq, @debug.Debug) +pub impl ToJson for CompactPayload +pub impl @json.FromJson for CompactPayload pub(all) struct CompactReply { compacting : Bool @@ -556,7 +558,9 @@ pub(all) struct GoalPayload { auto : Bool? model : String? max_steps : Int? -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) +} derive(Eq, @debug.Debug) +pub impl ToJson for GoalPayload +pub impl @json.FromJson for GoalPayload pub(all) struct GoalReply { delivered : Bool @@ -606,7 +610,9 @@ pub(all) struct ListAppsReply { pub(all) struct LoadSessionPayload { session : String workspace : String -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) +} derive(Eq, @debug.Debug) +pub impl ToJson for LoadSessionPayload +pub impl @json.FromJson for LoadSessionPayload pub(all) struct LoadSessionReply { session : SessionDocument @@ -906,7 +912,9 @@ pub(all) struct SessionPayload { session : String workspace : String force : Bool? -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) +} derive(Eq, @debug.Debug) +pub impl ToJson for SessionPayload +pub impl @json.FromJson for SessionPayload pub(all) struct SessionRuntimeNotice { content : String @@ -976,7 +984,9 @@ pub(all) struct StartPayload { max_steps : Int? session : String workspace : String -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) +} derive(Eq, @debug.Debug) +pub impl ToJson for StartPayload +pub impl @json.FromJson for StartPayload pub(all) struct StartReply { run_id : String From aa201b9ea711f3974ea4ec654dbd59d1a9ef7ac3 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Mon, 17 Aug 2026 15:00:32 +0800 Subject: [PATCH 15/17] fix(desktop): refuse a step count that is not an integer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The explicit codecs this change introduced accepted every JSON number for `max_steps` and truncated it: `{"max_steps": 1.9}` ran with a limit of 1 while the codec's own contract promised a malformed value would be refused. That is looser than the derive it replaced, and the divergence is silent — the client never learns the run it asked for is not the run it got. The three run payloads now decode the field through one `decoded_max_steps`, which refuses a number with a fractional part and keeps every other answer as it was: `null` and absence are still both `None`, and a non-number is still refused by type. Reported by Codex on #886. --- desktop/internal/api/payload.mbt | 78 +++++++++++++++---- .../internal/protocol/desktop_messages.mbt | 55 +++++++------ 2 files changed, 93 insertions(+), 40 deletions(-) diff --git a/desktop/internal/api/payload.mbt b/desktop/internal/api/payload.mbt index 9df51a755..91fb9c0bf 100644 --- a/desktop/internal/api/payload.mbt +++ b/desktop/internal/api/payload.mbt @@ -114,7 +114,7 @@ test "start and steer payloads preserve submission ids" { let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", "submission_id": "submission-start", }) assert_eq(start.submission_id, Some("submission-start")) @@ -131,7 +131,7 @@ test "start and steer payloads may omit submission ids" { let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", }) assert_true(start.submission_id is None) let steer : @protocol.SteerPayload = @json.from_json({ "text": "more detail" }) @@ -146,24 +146,24 @@ test "client-authored session roots are ignored" { let start : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", "session_root": "/client/chosen/store", }) assert_eq(start.session, "s-start") - assert_eq(start.workspace, "") + assert_eq(start.workspace, "/work/project") let compact : @protocol.CompactPayload = @json.from_json({ "session": "s-compact", - "workspace": "", + "workspace": "/work/project", "session_root": "/client/chosen/store", }) assert_eq(compact.session, "s-compact") - assert_eq(compact.workspace, "") + assert_eq(compact.workspace, "/work/project") } ///| test "start payload requires a session" { let decoded : @protocol.StartPayload? = Some( - @json.from_json({ "task": "hello", "workspace": "" }), + @json.from_json({ "task": "hello", "workspace": "/work/project" }), ) catch { _ => None } @@ -219,12 +219,12 @@ test "an absent optional reads the same as an explicit null" { let absent : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", }) let explicit : @protocol.StartPayload = @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", "model": Json::null(), "max_steps": Json::null(), "submission_id": Json::null(), @@ -245,7 +245,7 @@ test "an absent optional reads the same as an explicit null" { @json.from_json({ "task": "hello", "session": "s-start", - "workspace": "", + "workspace": "/work/project", "model": 7, }), ) catch { @@ -254,11 +254,57 @@ test "an absent optional reads the same as an explicit null" { assert_true(malformed is None) } +///| +test "a step count that is not an integer is refused, not truncated" { + // `1.9` is a malformed request, not a request for one step: running a + // different limit than the client sent is the silent divergence the + // explicit codec exists to prevent. + let start : @protocol.StartPayload? = Some( + @json.from_json({ + "task": "hello", + "session": "s-start", + "workspace": "/work/project", + "max_steps": 1.9, + }), + ) catch { + _ => None + } + assert_true(start is None) + let compact : @protocol.CompactPayload? = Some( + @json.from_json({ + "session": "s-compact", + "workspace": "/work/project", + "max_steps": 1.9, + }), + ) catch { + _ => None + } + assert_true(compact is None) + let goal : @protocol.GoalPayload? = Some( + @json.from_json({ + "session": "s-goal", + "workspace": "/work/project", + "max_steps": 1.9, + }), + ) catch { + _ => None + } + assert_true(goal is None) + // A whole number written as JSON still decodes; only the fractional part is + // the defect. + let whole : @protocol.CompactPayload = @json.from_json({ + "session": "s-compact", + "workspace": "/work/project", + "max_steps": 1000, + }) + assert_eq(whole.max_steps, Some(1000)) +} + ///| test "an absent optional is encoded as an absent key, never as null" { let payload : @protocol.GoalPayload = { session: "s-goal", - workspace: "", + workspace: "/work/project", text: None, auto: None, model: None, @@ -268,7 +314,7 @@ test "an absent optional is encoded as an absent key, never as null" { fail("goal payload is a JSON object") } assert_eq(fields.get("session"), Some(Json::string("s-goal"))) - assert_eq(fields.get("workspace"), Some(Json::string(""))) + assert_eq(fields.get("workspace"), Some(Json::string("/work/project"))) for name in ["text", "auto", "model", "max_steps"] { assert_true(fields.get(name) is None) } @@ -278,7 +324,7 @@ test "an absent optional is encoded as an absent key, never as null" { test "goal payload distinguishes set from clear by the text field" { let set : @protocol.GoalPayload = @json.from_json({ "session": "s-goal", - "workspace": "", + "workspace": "/work/project", "text": "ship the feature", "model": "deepseek-v4-pro", }) @@ -286,7 +332,7 @@ test "goal payload distinguishes set from clear by the text field" { assert_eq(set.model, Some("deepseek-v4-pro")) let clear : @protocol.GoalPayload = @json.from_json({ "session": "s-goal", - "workspace": "", + "workspace": "/work/project", }) assert_true(clear.text is None) } @@ -295,10 +341,10 @@ test "goal payload distinguishes set from clear by the text field" { test "session move payload has one canonical id and a required store" { let payload : SessionPayload = @json.from_json({ "session": " s-canonical ", - "workspace": "", + "workspace": "/work/project", }) assert_eq(payload.canonical_session(), "s-canonical") - assert_eq(payload.workspace, "") + assert_eq(payload.workspace, "/work/project") // The store is half the record's identity, so a payload without it is not // a valid move request — it would leave the host searching again. let storeless = try { diff --git a/desktop/internal/protocol/desktop_messages.mbt b/desktop/internal/protocol/desktop_messages.mbt index 7337f0f30..41585146c 100644 --- a/desktop/internal/protocol/desktop_messages.mbt +++ b/desktop/internal/protocol/desktop_messages.mbt @@ -34,6 +34,34 @@ pub(all) struct StartPayload { workspace : String } derive(Debug, Eq) +///| +/// The step limit as the client sent it, shared by the three run payloads that +/// carry one. A non-integral number is refused rather than truncated: `1.9` is +/// a malformed request, not a request for one step, and silently running a +/// different limit than the client asked for is exactly what an explicit codec +/// exists to prevent. +fn decoded_max_steps( + fields : Map[String, Json], + path : @json.JsonPath, +) -> Int? raise @json.JsonDecodeError { + match fields.get("max_steps") { + Some(Number(value, ..)) => { + let steps = value.to_int() + guard steps.to_double() == value else { + raise JsonDecodeError( + (path.add_key("max_steps"), "expected an integer step count"), + ) + } + Some(steps) + } + Some(Null) | None => None + Some(_) => + raise JsonDecodeError( + (path.add_key("max_steps"), "expected integer or null"), + ) + } +} + ///| /// The record-addressing payloads below answer the same questions the same /// way. A required field must be present and of its type; `null` is not a @@ -69,14 +97,7 @@ pub impl FromJson for StartPayload with fn from_json(json, path) { Some(_) => raise JsonDecodeError((path.add_key("model"), "expected string or null")) } - let max_steps = match fields.get("max_steps") { - Some(Number(value, ..)) => Some(value.to_int()) - Some(Null) | None => None - Some(_) => - raise JsonDecodeError( - (path.add_key("max_steps"), "expected integer or null"), - ) - } + let max_steps = decoded_max_steps(fields, path) let session = match fields.get("session") { Some(String(value)) => value Some(_) => @@ -178,14 +199,7 @@ pub impl FromJson for CompactPayload with fn from_json(json, path) { Some(_) => raise JsonDecodeError((path.add_key("model"), "expected string or null")) } - let max_steps = match fields.get("max_steps") { - Some(Number(value, ..)) => Some(value.to_int()) - Some(Null) | None => None - Some(_) => - raise JsonDecodeError( - (path.add_key("max_steps"), "expected integer or null"), - ) - } + let max_steps = decoded_max_steps(fields, path) { session, workspace, model, max_steps } } @@ -266,14 +280,7 @@ pub impl FromJson for GoalPayload with fn from_json(json, path) { Some(_) => raise JsonDecodeError((path.add_key("model"), "expected string or null")) } - let max_steps = match fields.get("max_steps") { - Some(Number(value, ..)) => Some(value.to_int()) - Some(Null) | None => None - Some(_) => - raise JsonDecodeError( - (path.add_key("max_steps"), "expected integer or null"), - ) - } + let max_steps = decoded_max_steps(fields, path) { session, workspace, text, auto, model, max_steps } } From 00903a4d847a99c7aad048a3f7c995f4564e9db4 Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Mon, 17 Aug 2026 15:04:14 +0800 Subject: [PATCH 16/17] fix(desktop): close the two ways a command still made a same-id twin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The population guard promised the app never adds to the duplicate id population, and two paths could still walk past it. `ensure_new_record_is_unique` probed only each other store's live `sessions/`. A start naming a store whose neighbour holds that id *archived* was therefore treated as new and opened a record — and the duplicate appeared later, when the user restored what they had put away. Another store's archived record is one unarchive away from standing beside this one, so it counts, and its refusal says which state it found. `agent.goal` and `agent.compact` never asked at all. Both spawn an engine when the conversation has no live one, and a spawn writes the record, so either could file the second twin that `agent.start` refuses to. A goal is reachable before the first prompt — the composer offers one on a conversation that has never run — which is exactly when the id is new to its store and old to another. Reported by Codex on #886. --- desktop/internal/engine/archive.mbt | 15 ++++ desktop/internal/engine/ops.mbt | 7 ++ .../internal/engine/store_identity_wbtest.mbt | 83 +++++++++++++++++++ 3 files changed, 105 insertions(+) diff --git a/desktop/internal/engine/archive.mbt b/desktop/internal/engine/archive.mbt index d1da20104..37d7b3620 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -73,6 +73,11 @@ async fn EngineManager::sweep_archived_deletions(self : EngineManager) -> Unit { /// not a creation, and passes untouched — including for an id that is /// duplicated on disk, because refusing to continue an existing record would /// strand the user with a conversation they can open but never answer. +/// +/// Another store's *archived* record counts too. It is one unarchive away from +/// being live beside this one, so treating the id as free here would create +/// the very duplicate this guard promises the app never makes — and the user +/// would meet it only later, when restoring the record they had put away. async fn ensure_new_record_is_unique( session : String, selected? : @pathx.Absolute, @@ -88,6 +93,16 @@ async fn ensure_new_record_is_unique( "this conversation already has a record in \{root}; start a new one here", ) } + if @fsx.is_dir( + @session_store.archived_root(root) + .join("sessions") + .join(session) + .to_path(), + ) { + raise EngineError( + "this conversation has an archived record in \{root}; start a new one here", + ) + } } } diff --git a/desktop/internal/engine/ops.mbt b/desktop/internal/engine/ops.mbt index 71ad23432..7f5072ed5 100644 --- a/desktop/internal/engine/ops.mbt +++ b/desktop/internal/engine/ops.mbt @@ -250,6 +250,10 @@ pub async fn compact_run( // reused engine never builds one, and the store must be known either way. let store = requested_store(payload.workspace) @session_store.ensure_not_archived(session, Some(store)) + // A compaction with no live engine spawns one, and a spawn writes the + // record: without this the command would open a second record for an id + // another store already owns, which `start_run` refuses. + ensure_new_record_is_unique(session, selected=store) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before anything else — even with a live engine. A start between @@ -352,6 +356,9 @@ pub async fn goal_run( // the slot is read — see `compact_run`. let store = requested_store(payload.workspace) @session_store.ensure_not_archived(session, Some(store)) + // A goal is reachable before the first prompt, and its spawn writes the + // record just as a start's does — see `compact_run`. + ensure_new_record_is_unique(session, selected=store) let sessions = manager.serving() let slot = slot_for(sessions, session) // Claim before looking at the engine, exactly as compact_run does: this op diff --git a/desktop/internal/engine/store_identity_wbtest.mbt b/desktop/internal/engine/store_identity_wbtest.mbt index be387430b..f31b91fca 100644 --- a/desktop/internal/engine/store_identity_wbtest.mbt +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -139,6 +139,89 @@ async test "a start cannot open a second record for an id another store owns" { @fs.rmdir(root.to_string(), recursive=true) } +///| +#cfg(not(platform="windows")) +async test "no command opens a second record for an id another store owns" { + ambient_env_test_lock.acquire() + defer ambient_env_test_lock.release() + let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT") + let (root, actor) = store_identity_host("openseek-unique-commands-") + defer restore_session_root_env(previous) + let manager = actor.manager() + let owner = root.join("owner") + let ws = root.join("ws") + @fsx.ensure_dir(ws.to_path()) + @fsx.ensure_dir(owner.to_path()) + let sink = EventSink(fn(_) { }) + @async.with_task_group(group => { + group.spawn_bg(no_wait=true, allow_failure=true, () => { + actor.run(sink, fn(_) { }) + }) + while manager.pump is Stopped { + @async.sleep(1) + } + ignore(@workspaces.add(owner.to_string(), fn(_) { })) + ignore(@workspaces.add(ws.to_string(), fn(_) { })) + // The first project owns `s-live`. A goal is reachable before the first + // prompt, and its spawn writes a record, so it must refuse for the same + // reason a start does rather than filing the second twin itself. + @fsx.ensure_dir( + @workspaces.store_root(owner).join("sessions/s-live").to_path(), + ) + let goal_refusal = try { + ignore( + goal_run(manager, { + session: "s-live", + workspace: ws.to_string(), + text: Some("ship it"), + auto: None, + model: None, + max_steps: None, + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(goal_refusal.has_suffix("; start a new one here")) + guard manager.pump is Serving(sessions~) else { fail("pump stopped") } + assert_true(sessions.get("s-live") is None) + // An *archived* record in the other store counts as well: it is one + // unarchive away from standing beside whatever this start would create. + @fsx.ensure_dir( + @workspaces.store_root(owner) + .join("archived/sessions/s-put-away") + .to_path(), + ) + let start_refusal = try { + ignore( + start_run(sink, manager, { + task: "fork me", + submission_id: None, + model: None, + max_steps: None, + session: "s-put-away", + workspace: ws.to_string(), + }), + ) + "no error" + } catch { + EngineError(detail) => detail + error if @async.is_being_cancelled() => raise error + error => "\{error}" + } + assert_true(start_refusal.contains("archived record")) + assert_true( + start_refusal.contains(@workspaces.store_root(owner).to_string()), + ) + assert_true(sessions.get("s-put-away") is None) + group.return_immediately(()) + }) + @fs.rmdir(root.to_string(), recursive=true) +} + ///| #cfg(not(platform="windows")) async test "an archived record in another store does not block a live one" { From 4db1eac1a1cab7a17e7186176e6d890b67d0b13e Mon Sep 17 00:00:00 2001 From: Haoxiang Fei Date: Mon, 17 Aug 2026 15:09:23 +0800 Subject: [PATCH 17/17] fix(desktop): read both listings before calling a placement unambiguous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `listed_stores` indexed only the live list, so one remaining row for an id was read as an unambiguous placement and moved the conversation onto it. That is wrong whenever the conversation's own record simply left the live list: a record never changes store, so its absence there means it was archived where it already was, and the row elsewhere belongs to a different conversation that happens to share the id. The consequence was worse than a mislabelled row. A project conversation archived while this client was away came back bound to the other project's record: `session.list_archived` then no longer recognized it as the archived one, so its cached transcript stayed open and writable through requests naming a store that never held it. Indexing the archived rows beside the live ones answers the question the index was always asking — which stores hold a record under this id — and two answers stay ambiguous, which is where the conversation's own binding already wins. Reported by Codex on #886. --- desktop/frontend/update.mbt | 85 ++++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 11 deletions(-) diff --git a/desktop/frontend/update.mbt b/desktop/frontend/update.mbt index 7c03c47ff..b7eb4d0ed 100644 --- a/desktop/frontend/update.mbt +++ b/desktop/frontend/update.mbt @@ -101,13 +101,17 @@ priv enum ListedStore { } ///| -/// Index a listing by session id, so placement reconciliation is one lookup -/// per conversation rather than a scan of the whole listing. +/// Index the listings by session id, so placement reconciliation is one lookup +/// per conversation rather than a scan of the whole listing. Live and archived +/// rows go in together: both say a store holds a record under that id, which +/// is the only question this index answers. fn listed_stores( items : Array[@transcript.SessionListItem], + archived : Array[@transcript.SessionListItem], ) -> Map[String, ListedStore] { items .iter() + .concat(archived.iter()) .fold(init=Map([]), (index, item) => { index[item.id] = match index.get(item.id) { None => OneStore(item.workspace) @@ -121,14 +125,20 @@ fn listed_stores( ///| /// Where `session.list` says this conversation's record lives. /// -/// The list is the authority on placement only while it is unambiguous. A -/// conversation materialized from a broadcast before the list arrived carries -/// a provisional store, and the single row bearing its id names the real one. -/// But the list really can carry two rows for one id — Scratch and a project -/// — and then the conversation's own binding is the better answer: it came -/// from the row the user clicked or from the store root a commit named, -/// whereas picking a row by id alone would rebind the conversation to a -/// record it never opened. +/// The listings are the authority on placement only while they are +/// unambiguous. A conversation materialized from a broadcast before they +/// arrived carries a provisional store, and the single row bearing its id +/// names the real one. But a host really can hold two records under one id — +/// one per attached project — and then the conversation's own binding is the +/// better answer: it came from the row the user clicked or from the store root +/// a commit named, whereas picking a row by id alone would rebind the +/// conversation to a record it never opened. +/// +/// Both listings count, because a record that left the live list did not move +/// stores — records never do — it was archived where it already was. Reading +/// the live list alone would see one remaining row, call that unambiguous, and +/// move the conversation onto a stranger's record while its own sat in its +/// store's archived twin. fn listed_project( index : Map[String, ListedStore], conv : Conversation, @@ -4936,7 +4946,7 @@ fn update_device_msg( // store this page believes in is the one every request names, so a // provisional Scratch placement would send the next prompt to a Scratch // record instead of the project one the conversation came from. - let stores = listed_stores(items) + let stores = listed_stores(items, dev.archived) let conversations = model.conversations.map(conv => { if conv.device == channel { { @@ -9400,6 +9410,59 @@ test "same-id session loads are fenced by channel owner" { inspect(loaded.messages[0].content, content="remote loaded") } +///| +test "an archived twin keeps its conversation from adopting the other store" { + let dispatch = test_dispatch() + let mine = @interop.ChannelId::Local.test_project() + let other = @interop.ChannelId::Local.test_resource("/work/alpha") + // Archived while this page was away: the conversation is bound to its own + // project, whose record now sits in that store's archived twin, and the + // live list that comes back holds only the other project's same-id row. + let model = test_model() + .replace_conversation({ + ..test_conversation(), + session_id: "desktop-shared", + workspace: Project(root=mine), + messages: [ + { kind: User, content: "my transcript", ts: None, sequence: None }, + ], + }) + .with_dev(dev => { + ..dev, + projects: [mine, other], + archived: [ + { + id: "desktop-shared", + title: Some("my transcript"), + workspace: mine, + workspace_name: "work", + updated_at_ms: None, + }, + ], + }) + let (_, listed) = update( + dispatch, + sessions_loaded([ + { + id: "desktop-shared", + title: Some("their copy"), + workspace: other, + workspace_name: "alpha", + updated_at_ms: None, + }, + ]), + model, + ) + guard listed.find_conversation(@interop.ChannelId::Local, "desktop-shared") + is Some(conv) else { + fail("the open conversation disappeared") + } + // One live row is not a relocation: records never change store, so the + // conversation stays on its own — where the archived reply can still + // recognize it — instead of writing through the other project's record. + assert_eq(conv.workspace, Project(root=mine)) +} + ///| test "a session list holding one id twice never rebinds an open conversation" { let dispatch = test_dispatch()