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. 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..604398a49 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 } @@ -1039,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, @@ -1112,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() { @@ -1133,7 +1133,7 @@ fn start_payload( model: Some(selected_model.wire()), max_steps: parsed_max_steps(max_steps), session, - workspace: Some(workspace.path), + workspace, } } @@ -1202,13 +1202,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 +1256,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 +1289,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 +1304,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 { @@ -1393,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(() => { @@ -1405,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 { @@ -1448,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(() => { @@ -1476,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 @@ -1491,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/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/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 6216902d1..bca6c1f98 100644 --- a/desktop/frontend/model.mbt +++ b/desktop/frontend/model.mbt @@ -905,17 +905,36 @@ 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) +} 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 ArchiveRecordKey::matches( - self : ArchiveRecordKey, +fn RecordKey::matches( + self : RecordKey, item : @transcript.SessionListItem, ) -> Bool { item.id == self.session && item.workspace == self.workspace @@ -925,22 +944,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 +982,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 +1619,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 +1712,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 +1893,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 +2011,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 +2026,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 +2652,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 +2670,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 +2878,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 +2894,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 +2907,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 } @@ -2896,8 +2945,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/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 93a4dba34..b7eb4d0ed 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 { @@ -87,17 +91,80 @@ 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 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) + Some(OneStore(store)) if store == item.workspace => OneStore(store) + Some(_) => SeveralStores + } + index + }) +} + +///| +/// Where `session.list` says this conversation's record lives. +/// +/// 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, +) -> @common.Uri { + match index.get(conv.session_id) { + Some(OneStore(store)) => store + Some(SeveralStores) | None => conv.workspace.project() + } +} + ///| /// 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 +199,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 +231,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 +557,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 +3319,7 @@ fn update_msg( archive_session( dispatch, confirm.channel, - confirm.session, + confirm.key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3555,9 +3615,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 +3627,7 @@ fn update_msg( archive_session( dispatch, channel, - session_id, + key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3578,8 +3639,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 +3650,7 @@ fn update_msg( unarchive_session( dispatch, channel, - session_id, + key, dev.connection_generation, sessions_request_generation, archived_request_generation, @@ -3641,18 +3703,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 +3722,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 +3732,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) @@ -4880,21 +4942,17 @@ 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, dev.archived) 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(stores, conv), conv.session_id, conv.workspace, ), @@ -4903,12 +4961,16 @@ 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(stores, active)) + } else if stores.get(model.active_session) is Some(OneStore(store)) { + active_project = Some(store) } } let mut next = { @@ -4922,7 +4984,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 +5321,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 +5345,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 +5385,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) } @@ -5548,9 +5604,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, @@ -5701,7 +5760,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 +6005,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 +7247,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 { { @@ -7394,13 +7462,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 } @@ -8510,7 +8581,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 +8609,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 +8819,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 +9208,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 +9221,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 +9292,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 +9360,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 +9410,208 @@ 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() + 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() + 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 +9629,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 +9644,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 +9693,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 +9711,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 +9733,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 +9794,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 +9814,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 +9862,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 +9888,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 +10149,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 +10214,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 +13359,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 +16261,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 +17276,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 +19303,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 +20394,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..9e2404044 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, @@ -215,18 +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~ : @hashset.HashSet[RecordKey], + archived~ : @hashset.HashSet[RecordKey], ) -> 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() { + 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.contains(key) || + archived.contains(key) { continue } // A fresh worktree conversation knows its binding locally; once the @@ -256,19 +260,17 @@ fn Model::push_group_component_inputs( } else { "New chat" }, + workspace, nested=true, worktree?, - root~, ), ), ) } - let durable : Array[@transcript.SessionListItem] = [] - for item in dev.sessions { - if item.workspace == workspace && !archived(item.id) { - 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 @@ -276,7 +278,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 +308,9 @@ fn Model::push_group_component_inputs( dev.channel, child.id, child_label, + workspace, nested=true, subrun=true, - root~, ), ) } @@ -323,6 +326,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 +334,6 @@ fn Model::push_group_component_inputs( archivable=true, nested=true, worktree?=worktree_name_for(dev, workspace, tree.item.id), - root~, children~, ), ), @@ -395,11 +398,14 @@ 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 = 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 = 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 @@ -506,7 +512,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({ @@ -686,7 +696,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 +2782,128 @@ 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() + 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..502262e24 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) @@ -183,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, @@ -327,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, @@ -1110,12 +1120,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 +1498,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 +1624,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 +1635,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 +1653,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 +1668,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 @@ -1657,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/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..91fb9c0bf 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": "/work/project", "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": "/work/project", }) assert_true(start.submission_id is None) let steer : @protocol.SteerPayload = @json.from_json({ "text": "more detail" }) @@ -138,48 +140,218 @@ 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": "/work/project", "session_root": "/client/chosen/store", }) assert_eq(start.session, "s-start") - assert_true(start.workspace is None) + assert_eq(start.workspace, "/work/project") let compact : @protocol.CompactPayload = @json.from_json({ "session": "s-compact", + "workspace": "/work/project", "session_root": "/client/chosen/store", }) assert_eq(compact.session, "s-compact") - assert_true(compact.workspace is None) + assert_eq(compact.workspace, "/work/project") } ///| test "start payload requires a session" { let decoded : @protocol.StartPayload? = Some( - @json.from_json({ "task": "hello" }), + @json.from_json({ "task": "hello", "workspace": "/work/project" }), ) 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 "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": "/work/project", + }) + let explicit : @protocol.StartPayload = @json.from_json({ + "task": "hello", + "session": "s-start", + "workspace": "/work/project", + "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": "/work/project", + "model": 7, + }), + ) catch { + _ => None + } + 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: "/work/project", + 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("/work/project"))) + 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({ "session": "s-goal", + "workspace": "/work/project", "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": "/work/project", + }) assert_true(clear.text is None) } ///| -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": "/work/project", }) assert_eq(payload.canonical_session(), "s-canonical") + 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 { + 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..37d7b3620 100644 --- a/desktop/internal/engine/archive.mbt +++ b/desktop/internal/engine/archive.mbt @@ -57,6 +57,55 @@ 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. +/// +/// 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, +) -> 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", + ) + } + 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", + ) + } + } +} + ///| /// 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 @@ -64,16 +113,12 @@ async fn EngineManager::sweep_archived_deletions(self : EngineManager) -> Unit { /// its siblings. 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. - workspace_token : String + // 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. + store : @session_store.StoreSelection members : Array[String] guards : Array[(String, SessionRecordGuard)] claims : Array[PendingClaim] @@ -94,15 +139,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 +168,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 +217,6 @@ 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) - } for session_id in members { let record_state = manager.claim_record_move(session_id) guards.push((session_id, record_state)) @@ -223,8 +237,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( @@ -239,12 +260,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) } } @@ -252,16 +274,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 } } ///| @@ -275,12 +288,12 @@ 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.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) } } @@ -295,8 +308,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 { @@ -349,7 +364,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) } }) } @@ -382,10 +397,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 +415,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,19 +433,26 @@ 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 // (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.store.workspace, on_committed?=on_worktree_changed, ) if refusal is Some(refusal) { @@ -441,19 +467,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 +493,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 +543,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, @@ -517,7 +554,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) } ///| @@ -532,8 +569,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 { @@ -586,7 +623,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) } }) } @@ -665,16 +702,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 +744,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 +759,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 +793,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 +809,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 +829,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 +1020,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 +1051,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 +1065,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/config.mbt b/desktop/internal/engine/config.mbt index b2d06cb48..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,13 +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 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)) } ///| @@ -143,46 +166,26 @@ async fn session_command_config( /// 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 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) { - 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) - 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 { - 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, @@ -235,7 +238,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, @@ -317,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") @@ -336,7 +339,7 @@ async test "run config rejects a blank session" { model: None, max_steps: None, session, - workspace: None, + workspace: "", }), ) "no error" @@ -369,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) @@ -378,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()), - Some(workspace + "/.openseek"), + 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" @@ -417,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.map(root => root.to_string()), - Some(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/engine.mbt b/desktop/internal/engine/engine.mbt index c42e97516..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. @@ -374,6 +374,22 @@ 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. +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 +943,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 +986,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 +994,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 +1027,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 +1044,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 +1062,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 +1087,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) } } @@ -1540,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 } @@ -2309,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 => { @@ -2321,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 87fecc3b8..f9825deb8 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 @@ -492,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], @@ -1488,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 f7cf7bef4..7f5072ed5 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,8 @@ 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) // Busy-ness is a property of the handle, not of admission: a condemned @@ -169,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. @@ -212,7 +244,16 @@ 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)) + // 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 @@ -224,7 +265,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) @@ -311,7 +352,13 @@ 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)) + // 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 @@ -321,7 +368,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) @@ -569,23 +616,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 +632,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 +675,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 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(live.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) @@ -888,7 +909,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 @@ -965,7 +986,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 @@ -1094,7 +1115,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, () => { @@ -1120,7 +1141,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)) @@ -1217,7 +1238,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)) @@ -1313,7 +1334,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)) @@ -1353,7 +1374,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( @@ -1411,9 +1432,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 +1444,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 +1465,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 +1479,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 +1515,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" @@ -1565,7 +1586,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) @@ -1651,7 +1672,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, () => { @@ -1704,7 +1725,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 @@ -1716,7 +1737,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/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/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 new file mode 100644 index 000000000..f31b91fca --- /dev/null +++ b/desktop/internal/engine/store_identity_wbtest.mbt @@ -0,0 +1,453 @@ +// 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. 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; +// * a record's own store decides whether it is archived, so an archived +// twin elsewhere never holds a live conversation hostage. +// +// 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. + +///| +/// 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) + 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 :; 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: 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: 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: 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) +} + +///| +#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" { + 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" { + 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: owner.to_string(), + }) + assert_eq(started.status, "accepted") + let detail = try { + ignore( + compact_run(manager, { + session: "s-busy", + model: None, + max_steps: None, + workspace: 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: owner.to_string(), + }), + ) + ignore( + goal_run(manager, { + session: "s-idle", + text: Some("other goal"), + auto: None, + model: None, + max_steps: None, + workspace: 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" { + 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: 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/engine/worktree_seam_wbtest.mbt b/desktop/internal/engine/worktree_seam_wbtest.mbt index 32669086e..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()), @@ -210,10 +210,11 @@ 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. + // 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,15 +222,15 @@ 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()), 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) } @@ -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( @@ -515,7 +496,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 +513,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 +554,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 +587,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..41585146c 100644 --- a/desktop/internal/protocol/desktop_messages.mbt +++ b/desktop/internal/protocol/desktop_messages.mbt @@ -7,6 +7,14 @@ // 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 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. ///| pub(all) struct StartPayload { @@ -19,12 +27,114 @@ 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? -} derive(Debug, Eq, FromJson, ToJson) + // 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) + +///| +/// 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 +/// 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 = decoded_max_steps(fields, path) + 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 { @@ -53,20 +163,69 @@ 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) +} 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 = decoded_max_steps(fields, path) + { 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 { // 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,31 +235,179 @@ 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) +} 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 = decoded_max_steps(fields, path) + { 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 +// 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 +/// exact store. pub(all) struct LoadSessionPayload { session : String - // The workspace holding the session, when the client already knows it. - workspace : String? -} derive(Debug, Eq, FromJson, ToJson) + workspace : String +} derive(Debug, Eq) ///| -/// `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. +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 +/// 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) +} 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. -/// `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..5a4767a06 100644 --- a/desktop/internal/protocol/pkg.generated.mbti +++ b/desktop/internal/protocol/pkg.generated.mbti @@ -370,10 +370,12 @@ 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) +} derive(Eq, @debug.Debug) +pub impl ToJson for CompactPayload +pub impl @json.FromJson for CompactPayload pub(all) struct CompactReply { compacting : Bool @@ -551,12 +553,14 @@ 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) +} derive(Eq, @debug.Debug) +pub impl ToJson for GoalPayload +pub impl @json.FromJson for GoalPayload pub(all) struct GoalReply { delivered : Bool @@ -605,8 +609,10 @@ pub(all) struct ListAppsReply { pub(all) struct LoadSessionPayload { session : String - workspace : String? -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) + workspace : String +} derive(Eq, @debug.Debug) +pub impl ToJson for LoadSessionPayload +pub impl @json.FromJson for LoadSessionPayload pub(all) struct LoadSessionReply { session : SessionDocument @@ -904,8 +910,11 @@ pub impl @json.FromJson for SessionListEntry 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 @@ -974,8 +983,10 @@ pub(all) struct StartPayload { model : String? max_steps : Int? session : String - workspace : String? -} derive(Eq, ToJson, @debug.Debug, @json.FromJson) + workspace : String +} derive(Eq, @debug.Debug) +pub impl ToJson for StartPayload +pub impl @json.FromJson for StartPayload pub(all) struct StartReply { run_id : String diff --git a/desktop/internal/session_store/pkg.generated.mbti b/desktop/internal/session_store/pkg.generated.mbti index 9b222dfe7..338c893ec 100644 --- a/desktop/internal/session_store/pkg.generated.mbti +++ b/desktop/internal/session_store/pkg.generated.mbti @@ -9,18 +9,14 @@ 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] 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 +29,13 @@ 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 +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 65a0deadb..b9cddb1c2 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,68 +100,60 @@ 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") } + 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 } } ///| -/// The store root whose archived `sessions/` holds this conversation's -/// durable record. -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 -} - -///| -/// 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(_) { +/// 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/pkg.generated.mbti b/desktop/internal/worktree/pkg.generated.mbti index 7b2b890fa..bd2921d32 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]? @@ -42,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 9e01ed638..a79bd848f 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 } ///| @@ -418,21 +421,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 +445,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 +481,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 +549,41 @@ 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. +pub async fn archive_bound_checkout( + session : String, + force : Bool, + workspace : @pathx.Absolute, + on_committed? : (String, Array[WorktreeInfo]) -> Unit, +) -> ArchiveNeedsForceReply? { + 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 @@ -768,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 @@ -1140,7 +1187,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 { @@ -1207,17 +1254,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 24a63fd6f..e8dd435ca 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: @@ -282,12 +282,56 @@ 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 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, 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 +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. + +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 +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 op compares its store before treating one as +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 +holds it. Notifications: @@ -435,12 +479,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.*