Name the durable store on every record-addressing session op by tonyfettes · Pull Request #886 · moonbitlang/openseek

tonyfettes · 2026-08-15T11:35:41Z

Closes the follow-up recorded on #878: the live path never named which durable store it meant, so an id that exists in both Scratch and a project store could be read, opened, or archived in the wrong one. Codex flagged the three symptoms on that PR (#878 (comment)); they are one change and land here together.

The host selects a store instead of searching for one

session.archive / session.unarchive took {session} alone, and session.load / session.load_archived took the store as an optional hint. Without it the host searched known_session_roots(), which yields the global store first — so archiving the project row could archive the global record, and a Scratch conversation's reload could read a project's transcript.

All four now carry workspace as a required field, spelled the way session.delete_archived already spells it: a host-reported project path, or "" for the global store. @session_store.ArchivedStore becomes StoreSelection (it resolves live stores too), and later in this branch a two-case @session_store.Store — the single place a client's spelling turns into a root. Making it required on claim_session_family_move removes that function's search fallback and its workspace back-inference; @session_store.live_root_of has no callers left.

Refusals now name the store they were about: a detached workspace is refused by name, a record missing from the selected store says so instead of reporting the id as globally absent, and the live/archived twin check looks in that same store rather than any store. Search survives only where the question is genuinely about the population rather than one named record: the whole-host deletion sweep, and the duplicate-record check below.

The sidebar acts on the row it rendered

#878 gave live rows their store in the component id, so two same-id rows keep distinct keys — but everything downstream dropped it. OpenSession, ArchiveSession and UnarchiveSession travelled as (channel, session), and session_project resolved the id against the first same-id listing. The dispatcher now rebuilds the record from the clicked row's own root, so a row acts on the record its group renders. ArchiveRecordKey becomes RecordKey: it is no longer only about archiving but the durable identity (store, session) that opening, archiving, restoring, deleting, and their notifications all carry. The discard-confirmation dialog retains the record the refused attempt named, so its forced retry cannot land elsewhere.

What this deliberately does not change

The page still holds at most one open conversation per (channel, session id) — because the host runs at most one engine per session id (Map[String, SessionSlot]). Making the client hold two would put a second identity through @interop.ConversationKey, which owns file-panel, terminal, symbol and quick-open ownership, to model a state the host cannot produce.

So Model::find_conversation stays the id-keyed lookup that live run events route through, and the store says which record that conversation is bound to. Opening the other store's row rebinds it — exactly what opening an archived twin already did. Everything that asks about a record uses find_record_conversation instead, which is what fixes the shared-row-state symptom: a row reads live state only from a conversation bound to its own store, so the Scratch row stays idle while the project record runs.

Found by E2E: two more id-keyed leftovers

Running the checklist below caught the same defect twice more, both in places
the store-qualification pass had not looked. In each the disk was correct and
the host had done exactly the right thing; the client misread it.

Archiving one row blanked the other. Archiving the project row made the
Scratch row vanish from the live list. The sidebar's two group filters asked
about an id rather than a record (dev.archived.any(item.id == id)), so an
archived record in one store suppressed the live row of a same-id record in
another. The archived section's active check and can_reload_transcript
asked the same half-question.

Unarchiving rebound the wrong conversation. After restoring the project
record, the project group rendered the Scratch conversation — its transcript,
under the project folder. SessionsLoaded reconciles a conversation
materialized from a broadcast before session.list named its store; that
reconcile matched by id and did not stop at the first hit, so the last same-id
row won, and list_sessions orders the global root first. A conversation the
user had opened from Scratch was therefore rebound to the project store, and
would have loaded and sent against a record it never opened. The listing is now
authoritative on placement only while unambiguous: it may name the store of a
conversation whose store is unknown, but it never overrules a binding that came
from a clicked row or a commit's session_root.

Both are worth stating plainly because they are the reason the identity
question needed answering at all: the durable layer had been store-qualified
since #867, and every remaining bug lived in code that still asked "which
session id?" where the answer had to be "which record?".

Found by Codex: the ops that write a record, and the engine they write it to

Rebased onto main (#768's WorktreeOwner, #887's Quick Open package, and #891's split of internal/engine into layered packages). The store selection now lives in internal/session_store beside the lookups it replaces, the worktree cleanup entry is @worktree.archive_bound_checkout, and WorktreeHost::drain_sessions takes the store root beside the ids. Three review findings then closed the last places reasoning by id alone, all on the write path this PR had left as it was.

agent.start / agent.compact / agent.goal took the store as a hint. Without one the host asked workspace_of_session — the first registered project whose store holds that id — so a Scratch conversation sharing an id with a project record submitted into the project: appended to that project's history, run in its checkout, and its compaction and goal sent there too. All three now carry workspace as a required field, spelled exactly as the session.* ops spell it, and SessionStore::from_workspace is the one resolver. Dropping the inference does not reopen the fork it guarded: a start whose named store lacks the record while another store has it is refused by ensure_new_record_is_unique, naming the store that holds it. On the client, RecordKey::workspace_payload already encoded the store of the record a conversation is bound to — these three requests now send it.

A compaction or goal was written to whatever engine held the slot. Slots are keyed by id (the host runs one engine per id), so compact_run and goal_run could hand their command to the engine writing a same-id record in another store — rewriting the wrong history and reporting progress into the conversation that asked. Both resolve their store before reading the slot and reuse the engine only when it writes_store that one. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle (it loses only a warm process, as after a fingerprint change), refused by the store's name while it is mid-turn.

One store's archived record blocked another store's live one. The archived check searched every store, so with s-x archived in Scratch and live in a project, the live conversation could not be prompted, compacted, or given a goal — all refused as "session is archived" — until the unrelated twin was restored or deleted. ensure_record_not_archived reads the named store's archived twin and nothing else; a write to a record its own store archived is still refused. archived_record_root had no other caller and goes with the search.

Testing

moon check --target js / --target native clean; moon fmt, moon info applied
Full suites: 3003 js + 3490 native tests pass
New: archive moves the selected store's record, not the same id elsewhere (host, with both stores holding s-both), a row opens its own store's record beside a same-id conversation, a same-id row lights up only for the store its conversation is bound to, archiving one store's record leaves the same id listed in the other and a session list holding one id twice never rebinds an open conversation (the two E2E regressions above), the discard dialog retries against the store the refusal named, and a decode test that a storeless session.archive payload is rejected
New for the review findings, all against a real pump: a command addressed to one store never reaches a same-id twin's engine (refused mid-turn, rebound when idle), an archived record in another store does not block a live one (and the store that archived it still refuses), the start-uniqueness test extended so the Scratch continuation runs against the Scratch store while the project holds the same id, the request's store places the record, not a search for the id, and a decode test that a storeless agent.start / agent.compact / agent.goal is rejected
The store-selection lookup tests were rewritten rather than deleted: a stale client naming a detached workspace now gets a by-name refusal, and each one additionally asserts the global store does not answer for that id
Each fix was checked against its own test with the fix reverted, so the test provably fails without it

Manual E2E

Create a chat in Scratch and one in a project, then give them the same session id on disk (copy ~/.openseek/sessions/<id> into <project>/.openseek/sessions/), and reconnect.
Both rows appear. Start a run in the project one: only the project row pulses.
Click the Scratch row — its own (empty/other) transcript opens; click back, the project transcript returns.
Archive the project row: only the project record moves; the Scratch row stays live. Send from the Scratch row — it runs, rather than refusing as "session is archived".
Restore it from the Archived group, then delete-archived the Scratch one, and confirm the project record survives.
With the project one mid-turn, /compact the Scratch row: it is refused by name instead of compacting the project's history. Stop the run and repeat — the Scratch compaction now takes the slot and leaves the project record untouched.

🤖 Generated with Claude Code

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93a8598dd3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-15T11:39:36Z

        )
    }
-  } else if model.find_conversation(channel, session_id) is Some(conv) {
+  } else if model.find_record_conversation(channel, key) is Some(conv) {


Keep a running same-id conversation bound to its store

If store A's same-id conversation is running and the user clicks store B's row, this exact-record predicate fails and the fallback replaces the only (channel, session) conversation with a fresh store-B instance, discarding store A's run state while its host engine continues. Subsequent agent.* events are routed by session id to that replacement, so store B can display store A's lifecycle/output and the user loses the correct cancel/steer state. Prevent cross-store rebinding while the existing same-id conversation has an active run, or otherwise preserve and route the active engine owner.

Useful? React with 👍 / 👎.

This one is deliberate, and it is the trade-off recorded in the PR body under What this deliberately does not change: the page holds at most one open conversation per (channel, session id), because the host runs at most one engine per session id (Map[String, SessionSlot]). Holding two would put a second identity through @interop.ConversationKey — which owns file-panel, terminal, symbol and quick-open ownership — to model a state the host cannot produce. Opening the other store's row rebinds the single conversation, exactly as opening an archived twin already did.

What was genuinely unsafe in your report was the consequence, and that is now closed on the host rather than by client bookkeeping:

a prompt, compaction or goal names its store on the wire (053b1a1), so a rebound conversation cannot write store A's record;

compact_run / goal_run compare writes_store before reusing the slot's engine (6ef82ca), so a command for B is never written into A's engine — it replaces the engine when A is idle, and is refused by A's store name while A is mid-turn.

What remains is display-only and bounded: A's engine keeps running host-side, and because live run events route by session id, its events land in whatever record that single conversation is currently bound to. Clicking back rebinds to A and its run reappears. Fixing that properly means either a two-identity client or gating run events on the session_root the host already sends with agent.started — worth doing, but as its own change rather than smuggled into this one. Happy to open a follow-up issue if you think the display cross-wire should not ship as-is.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a963866201

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 03337904c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Archive moved the selected store's record but then asked `archive_bound_worktree_checkout` to find the worktree by session id, and `workspace_of_session` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved, and a Scratch record owns none. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle Scratch record was refused while a project conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: add62abfe1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c58cb3951

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Archive moved the selected store's record but then asked `archive_bound_worktree_checkout` to find the worktree by session id, and `workspace_of_session` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved, and a Scratch record owns none. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle Scratch record was refused while a project conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for the Scratch record was written to the project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886.

`agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `workspace_of_session` — the first registered project whose store holds that id — so a Scratch conversation sharing an id with a project record submitted into the *project*: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path, or `""` for Scratch. `SessionStore::from_workspace` is the single resolver — the wire's empty spelling becomes the Scratch case once, at the boundary — and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional Scratch placement for a project record. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886.

The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `archived_record_root` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a66b42416c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-15T15:18:52Z

+  // (`worktree.create` binds at creation) runs in that checkout — the start
+  // itself never names or mutates worktrees.
+  workspace : String
 } derive(Debug, Eq, FromJson, ToJson)


Replace the derived codecs on modified payloads

The commit changes the JSON contract of StartPayload, CompactPayload, GoalPayload, LoadSessionPayload, and SessionPayload, but all five still derive FromJson/ToJson. This leaves their required, missing, null, malformed, unknown-field, and optional-field omission behavior implicit; replace the derived codecs for every modified payload with explicit implementations as required for Desktop protocol types.

AGENTS.md reference: desktop/AGENTS.md:L15-L25

Useful? React with 👍 / 👎.

Archive moved the selected store's record but then asked `@worktree.archive_bound_checkout` to find the worktree by session id, and `@session_store.workspace_of` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved, and a Scratch record owns none. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle Scratch record was refused while a project conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for the Scratch record was written to the project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886.

`agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `@session_store.workspace_of` — the first registered project whose store holds that id — so a Scratch conversation sharing an id with a project record submitted into the *project*: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path, or `""` for Scratch. `@session_store.Store::from_workspace` is the single resolver — the wire's empty spelling becomes the Scratch case once, at the boundary — and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional Scratch placement for a project record. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886.

The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `@session_store.archived_root_of` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886.

…ched `desktop/AGENTS.md` requires an explicit `FromJson`/`ToJson` for anything crossing the JSON boundary, and requires replacing a derived codec in the same change that modifies its type. This change gave `agent.start`, `agent.compact`, `agent.goal`, `session.load`(`_archived`) and `session.archive`/`unarchive` a required `workspace`, and all five still derived theirs. The five codecs now say what the derive left implicit, and say it the same way the neighbouring payloads in this file already do. A required field must be present and of its type; `null` is not a value for one, so it is refused rather than read as absence — which for `workspace` is the point, since reading it as the empty token would file the record in Scratch. An optional field reads `null` and absence alike as `None` and refuses any other type, so a `null` `text` still clears a goal rather than setting an empty one. Unknown fields are ignored, which is what lets a host accept and drop a client-authored `session_root`. The encoder writes every required field and omits an absent optional instead of emitting `null`. Reported by Codex on #886.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dcec41b6d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-15T18:10:28Z

+    Some(OneStore(store)) => store
+    Some(SeveralStores) | None => conv.workspace.project()


Preserve the bound store when only another live twin remains

When a project-bound conversation is archived while this client is disconnected and a same-ID Scratch record remains live, the reconnecting session.list contains exactly one row for that ID, so this branch rebinds the cached project conversation to Scratch before session.list_archived arrives. The archived reply then no longer recognizes the conversation as the project record, leaving its cached project transcript writable through Scratch-targeted operations. Fresh evidence after the earlier reconciliation fix is that listed_stores indexes only the live list, so an archived twin is misclassified as an unambiguous relocation rather than an absent bound record.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-15T18:10:28Z

+  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()) {


Include archived records in the uniqueness check

When another store owns only an archived record with this ID, this loop finds no conflict because it probes only <root>/sessions/<id>, and agent.start creates a new live record in the selected store. Restoring the archived record later therefore produces the same cross-store duplicate that this newly added guard promises Desktop will not create; probe each other store's archived twin as well before treating the ID as new.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-15T18:10:28Z

+  // The goal is durable state of one record, so the store is resolved before
+  // the slot is read — see `compact_run`.
+  let store = @session_store.Store::from_workspace(payload.workspace)
+  @session_store.ensure_not_archived(session, store)


Apply record uniqueness before spawning for goal commands

When agent.goal names a store with no record while another registered store has the same ID, this scoped archived check passes and the subsequent spawn writes the goal into a newly created record in the requested store. This is reachable for a fresh conversation because the UI permits setting a goal before the first prompt, and unlike start_run this path never calls ensure_new_record_is_unique, so the new store-qualified routing can itself create same-ID records across stores.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-15T18:10:28Z

+  let max_steps = match fields.get("max_steps") {
+    Some(Number(value, ..)) => Some(value.to_int())


Reject fractional max_steps values

A payload such as {"max_steps":1.9} matches Number and is silently truncated by to_int() instead of being rejected as the advertised malformed non-integer. The same replacement logic appears in the new Start, Compact, and Goal codecs, allowing malformed requests to run with a different step limit than the client supplied. Fresh evidence after replacing the derived codecs is that the explicit decoder now accepts every JSON number without checking integrality.

AGENTS.md reference: desktop/AGENTS.md:L19-L22

Useful? React with 👍 / 👎.

Archive moved the selected store's record but then asked `@worktree.archive_bound_checkout` to find the worktree by session id, and `@session_store.workspace_of` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle record was refused while another project's conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for one project's record was written to the other project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886.

`agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `@session_store.workspace_of` — the first registered project whose store holds that id — so a conversation sharing an id with another project's record submitted into *that* project: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path. `@session_store.Store::from_workspace` is the single resolver, and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional placement for a record it never opened. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886.

The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `@session_store.archived_root_of` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886.

…ched `desktop/AGENTS.md` requires an explicit `FromJson`/`ToJson` for anything crossing the JSON boundary, and requires replacing a derived codec in the same change that modifies its type. This change gave `agent.start`, `agent.compact`, `agent.goal`, `session.load`(`_archived`) and `session.archive`/`unarchive` a required `workspace`, and all five still derived theirs. The five codecs now say what the derive left implicit, and say it the same way the neighbouring payloads in this file already do. A required field must be present and of its type; `null` is not a value for one, so it is refused rather than read as absence — which for `workspace` is the point, since reading it as an absent field would leave the host guessing which store was meant. An optional field reads `null` and absence alike as `None` and refuses any other type, so a `null` `text` still clears a goal rather than setting an empty one. Unknown fields are ignored, which is what lets a host accept and drop a client-authored `session_root`. The encoder writes every required field and omits an absent optional instead of emitting `null`. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle record was refused while another project's conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for one project's record was written to the other project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886.

`agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `@session_store.workspace_of` — the first registered project whose store holds that id — so a conversation sharing an id with another project's record submitted into *that* project: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path. `@session_store.Store::from_workspace` is the single resolver, and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional placement for a record it never opened. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886.

The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `@session_store.archived_root_of` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886.

…ched `desktop/AGENTS.md` requires an explicit `FromJson`/`ToJson` for anything crossing the JSON boundary, and requires replacing a derived codec in the same change that modifies its type. This change gave `agent.start`, `agent.compact`, `agent.goal`, `session.load`(`_archived`) and `session.archive`/`unarchive` a required `workspace`, and all five still derived theirs. The five codecs now say what the derive left implicit, and say it the same way the neighbouring payloads in this file already do. A required field must be present and of its type; `null` is not a value for one, so it is refused rather than read as absence — which for `workspace` is the point, since reading it as an absent field would leave the host guessing which store was meant. An optional field reads `null` and absence alike as `None` and refuses any other type, so a `null` `text` still clears a goal rather than setting an empty one. Unknown fields are ignored, which is what lets a host accept and drop a client-authored `session_root`. The encoder writes every required field and omits an absent optional instead of emitting `null`. Reported by Codex on #886.

The explicit codecs this change introduced accepted every JSON number for `max_steps` and truncated it: `{"max_steps": 1.9}` ran with a limit of 1 while the codec's own contract promised a malformed value would be refused. That is looser than the derive it replaced, and the divergence is silent — the client never learns the run it asked for is not the run it got. The three run payloads now decode the field through one `decoded_max_steps`, which refuses a number with a fractional part and keeps every other answer as it was: `null` and absence are still both `None`, and a non-number is still refused by type. Reported by Codex on #886.

The population guard promised the app never adds to the duplicate id population, and two paths could still walk past it. `ensure_new_record_is_unique` probed only each other store's live `sessions/<id>`. A start naming a store whose neighbour holds that id *archived* was therefore treated as new and opened a record — and the duplicate appeared later, when the user restored what they had put away. Another store's archived record is one unarchive away from standing beside this one, so it counts, and its refusal says which state it found. `agent.goal` and `agent.compact` never asked at all. Both spawn an engine when the conversation has no live one, and a spawn writes the record, so either could file the second twin that `agent.start` refuses to. A goal is reachable before the first prompt — the composer offers one on a conversation that has never run — which is exactly when the id is new to its store and old to another. Reported by Codex on #886.

`listed_stores` indexed only the live list, so one remaining row for an id was read as an unambiguous placement and moved the conversation onto it. That is wrong whenever the conversation's own record simply left the live list: a record never changes store, so its absence there means it was archived where it already was, and the row elsewhere belongs to a different conversation that happens to share the id. The consequence was worse than a mislabelled row. A project conversation archived while this client was away came back bound to the other project's record: `session.list_archived` then no longer recognized it as the archived one, so its cached transcript stayed open and writable through requests naming a store that never held it. Indexing the archived rows beside the live ones answers the question the index was always asking — which stores hold a record under this id — and two answers stay ambiguous, which is where the conversation's own binding already wins. Reported by Codex on #886.

chatgpt-codex-connector · 2026-08-17T09:14:41Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

A session id is unique only inside one durable store, and each attached project has one, so two projects may hold records with the same id and the sidebar renders both. The record-addressing ops did not say which one they meant. `session.archive` and `session.unarchive` took `{session}` alone, and `session.load` / `session.load_archived` took the store as an optional hint. Without it the host searched `@session_store.known_roots()`, which yields the global store first — so archiving one project's row could archive another project's record, and loading one conversation could read the other's transcript. All four ops now carry `workspace` as a required field, spelled the way `session.delete_archived` already spells it: a host-reported project path, or `""` for the global store. `@session_store.ArchivedStore` becomes `StoreSelection` because it now resolves live stores too; it is the single place a client's spelling turns into a root, and it is required rather than optional on `claim_session_family_move`, which drops that function's search fallback and its workspace back-inference. `@session_store.live_root_of` has no callers left. Refusals gain the store they were about: naming a detached workspace is refused by name, a record missing from the selected store says so instead of reporting the id as globally absent, and the live/archived twin check now looks in that same store rather than any store. Search remains where a request genuinely carries no store — the archived correlation defense guarding `agent.start`, and the whole-host deletion sweep.

Live rows carried their store in the component id (so two same-id rows keep distinct keys) but dropped it everywhere downstream: `OpenSession`, `ArchiveSession`, and `UnarchiveSession` travelled as `(channel, session)`. Clicking one project's row could therefore open the other's record — `session_project` resolved the id against the first same-id listing — and both rows projected the same conversation's active/running state. `ArchiveRecordKey` becomes `RecordKey`, since it is no longer only about archiving: it is the durable identity `(store, session)` that opening, archiving, restoring, deleting, and their notifications all carry. The sidebar dispatcher rebuilds it from the clicked row's own `root`, so a row acts on the record its group renders. What this deliberately does not change: the page still holds at most one open conversation per `(channel, session id)`, because the host runs at most one engine per session id — `Model::find_conversation` stays the id-keyed lookup that live run events route through. The store says which record that conversation is bound to. Opening the other store's row rebinds it, exactly as opening an archived twin already did, and `find_record_conversation` is what everything asking about a *record* uses instead: a row now reads live state only from a conversation bound to its own store, so the idle row stays idle while the project record runs. `session.load` and the archive ops now always name the store, and the discard-confirmation dialog retains the record the refused attempt named so its forced retry cannot land in a different store.

`session.load`, `session.load_archived`, `session.archive` and `session.unarchive` now take `workspace` as a required field, and the host selects that store exactly instead of searching. A new *Naming the store* section states the encoding once, says why `session.list` can return two rows with one id, and tells clients to persist `(workspace, session)` together.

Found by E2E: archiving one project's row made the other's row disappear from the live list too, even though only the project record moved on disk. The sidebar's two group filters asked about an id, not a record — `dev.sessions.any(item.id == id)` and `dev.archived.any(item.id == id)` — so an archived record in one store suppressed the live row of a same-id record in another. The model layer was already store-qualified; only these predicates and the archived section's `active` check were not. They now take a `RecordKey` and use `RecordKey::matches`, which compares store and id. `can_reload_transcript` asked the same half-question: a fresh conversation whose id matched another store's listing was told it had a durable record, and would then issue a snapshot load naming its own (empty) store.

…ting Found by E2E: after unarchiving the project record, the project group showed the other project's conversation — its transcript, under the wrong folder. `SessionsLoaded` reconciles a conversation materialized from a broadcast before `session.list` named its store. That reconcile matched by id alone and did not stop at the first hit, so the LAST same-id row won — and `list_sessions` orders one group per registered workspace. A conversation the user had open was therefore rebound to the project store: mislabelled in the sidebar, and pointed at a record it had never opened for its next load or send. The listing is now the authority on placement only while it is unambiguous. `listed_project` keeps the conversation's own binding when a row confirms it or when several rows carry the id, and adopts a row's store only when exactly one bears the id — which is the provisional-placement case this reconcile exists for. The active-project follow does the same, preferring the active conversation's own placement. The unarchive handler's `foreground` test asked `session_project` — first same-id row wins — although it already held the conversation bound to the exact record; being the active id is the whole question there.

The store-qualified filters kept the shape of the id-only ones they replaced: a linear scan of the listing per row. The sidebar re-renders on every dispatch and asks twice per row — once against the live listing, once against the archived one — so a device with a few hundred records did tens of thousands of comparisons per frame, and `SessionsLoaded` allocated a filtered array per conversation. `RecordKey` derives `Hash`, so both listings index into a `HashSet[RecordKey]` built once per render and each row's question is a membership test. `@common.Uri` already implements `Hash` and `Eq`; nothing in the editor submodule changes. Placement reconciliation indexes by id into `ListedStore`, which names the single store carrying an id or says several do — the exact distinction `listed_project` needs, so it becomes one lookup with no scan and no first/last-match subtlety left to get wrong. The two accumulation loops this touched are now `filter`/`fold`.

Archive moved the selected store's record but then asked `@worktree.archive_bound_checkout` to find the worktree by session id, and `@session_store.workspace_of` returns the FIRST registered project whose store holds that id. With the same id in two project stores, archiving the later- registered row deleted the other project's checkout — with `force`, its uncommitted changes — while leaving its own record and checkout untouched. The selected workspace now travels into the cleanup: a worktree belongs to the project that owns the record being moved. Reported by Codex on #886.

Session ids are unique only within one durable store. #886 made every op that addresses a record name its store; this closes the two remaining places where the host still reasoned about an id alone. `agent.start` now refuses to open a *second* record for an id another store already owns, the rule `worktree.create` has enforced since it began binding conversations. A start whose selected store already holds the record is a continuation and passes untouched. The app therefore never adds to the duplicate population — every duplicate a user meets came from outside it (the CLI takes `--session <name>` verbatim under any `--session-root`, and a project's store travels with the project), which is why the client still handles them rather than assuming they cannot occur. Session slots and followers are keyed by id alone, because the host runs at most one engine per id. Lifecycle operations that address one named store now compare the engine's `session_root` before treating it as their own, via `ServeEngine::writes_store` and `SessionFollower::tails_store` — the comparison `close_engines_for_workspace` already made inline. Without it, archiving an idle record was refused while another project's conversation with the same id was running, and closed that conversation's engine when it was idle. Worktree removal takes the workspace's store for the same reason. The pending claims stay store-blind on purpose: they last one operation, while an idle engine can hold its slot for hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Several words in this tree are ordinary English already spent on something specific, so a loose second use silently merges two concepts. Write them down. The load-bearing one is that every conversation answers two independent directory questions — where its record lives and where the agent works — and they coincide only for a plain project conversation. A worktree conversation records in its project's store while working in its own checkout, and a Scratch conversation's two directories are in unrelated trees, so neither may ever be derived from the other. "placement" already means the retained worktree registry row and the client's four-case view of a checkout; both are on the checkout axis, so it must never be stretched to cover a store. "root" is qualified everywhere except when it means a store's own root.

A family move was handed three loose values that always travelled together: a root, the project owning it, and `workspace_token` — the protocol spelling of that project. They come from one place, `StoreSelection`, and every consumer had to keep them consistent by hand. `SessionFamilyMove` now carries that selection itself, so the store it moves records in, the owner permanent deletion drops a worktree placement from, and the token every `session.changed` callback carries are one value with one source. Nothing reachable changes.

Session slots are keyed by id alone — the host runs at most one engine per id — so the engine holding a slot may be the one writing a same-id record in another store. `compact_run` and `goal_run` reused any engine that accepted commands, so a compaction requested for one project's record was written to the other project's engine: it rewrote that project's history and reported its progress into the conversation that asked for it. `writes_store` had already reached the operations that move or close a record; these two commands were what was left still addressing an engine by id alone. Both now resolve the store they address before reading the slot, and reuse the live engine only when it writes that store. A mismatch falls through to the spawn path, which replaces the engine: allowed while the other conversation is idle, since it loses only a warm process exactly as after a fingerprint change, and refused by the store's name while it is mid-turn, since destroying work this request never named is not the command's to do. The store resolution moves out of `serve_config` into `requested_store` so that the reuse decision and the spawn decision cannot disagree. Only the store is resolved, not the whole config: a reused engine never builds one, and building it here would raise on a missing worktree checkout that the live engine could still have served. Reported by Codex on #886.

`agent.start`, `agent.compact` and `agent.goal` took `workspace` as a hint. Without one the host asked `@session_store.workspace_of` — the first registered project whose store holds that id — so a conversation sharing an id with another project's record submitted into *that* project: the prompt was appended to that project's history and ran in its checkout, and the same inference sent its compaction and its goal there too. The three ops that write a record were the last ones still recovering the store from the id after #886 made every op that reads or moves one name it. They now carry `workspace` the way `session.archive` and its siblings do: a registered project's resource path. `@session_store.Store::from_workspace` is the single resolver, and `serve_config` uses it instead of a search. A client that omits the field is refused by the decoder rather than silently placed, and a stale client naming a detached project keeps getting the by-name refusal it already got. The client already knew the answer it was not sending: `RecordKey::workspace_payload` encodes the store of the record a conversation is bound to, and now encodes it for these three requests as well, with the same channel check they each open with. A conversation whose store the page has not learned yet is reconciled from `session.list` before any send (#886), so nothing sends a provisional placement for a record it never opened. Dropping the inference does not reopen the fork it guarded against: a start whose named store does not hold the record, while another store does, is refused by `ensure_new_record_is_unique` — naming the store that has it — instead of quietly filing a second record. Reported by Codex on #886.

The archived check searched every registered store, because the ops it guarded carried only an id. They carry their store now, and the search had become a hostage-taking: with one store's record archived and another store's live under the same id, the live conversation could not be prompted, compacted, or given a goal — every write refused as "session is archived" — until the unrelated archived twin was restored or permanently deleted. The duplicate ids this PR exists to support were exactly the case it broke. `ensure_record_not_archived` reads the archived twin of the store the request named, and nothing else. The defense itself is unchanged where it matters: a write to a record its own store archived is still refused, which the test asserts beside the live twin it now lets through. `worktree.create` scopes to the project it is creating in, the only store its binding can address. `@session_store.archived_root_of` had no other caller and goes with the search. The whole-host scan that remains, `ensure_new_record_is_unique`, asks a question about the population rather than about one named record. Reported by Codex on #886.

…ched `desktop/AGENTS.md` requires an explicit `FromJson`/`ToJson` for anything crossing the JSON boundary, and requires replacing a derived codec in the same change that modifies its type. This change gave `agent.start`, `agent.compact`, `agent.goal`, `session.load`(`_archived`) and `session.archive`/`unarchive` a required `workspace`, and all five still derived theirs. The five codecs now say what the derive left implicit, and say it the same way the neighbouring payloads in this file already do. A required field must be present and of its type; `null` is not a value for one, so it is refused rather than read as absence — which for `workspace` is the point, since reading it as an absent field would leave the host guessing which store was meant. An optional field reads `null` and absence alike as `None` and refuses any other type, so a `null` `text` still clears a goal rather than setting an empty one. Unknown fields are ignored, which is what lets a host accept and drop a client-authored `session_root`. The encoder writes every required field and omits an absent optional instead of emitting `null`. Reported by Codex on #886.

The explicit codecs this change introduced accepted every JSON number for `max_steps` and truncated it: `{"max_steps": 1.9}` ran with a limit of 1 while the codec's own contract promised a malformed value would be refused. That is looser than the derive it replaced, and the divergence is silent — the client never learns the run it asked for is not the run it got. The three run payloads now decode the field through one `decoded_max_steps`, which refuses a number with a fractional part and keeps every other answer as it was: `null` and absence are still both `None`, and a non-number is still refused by type. Reported by Codex on #886.

The population guard promised the app never adds to the duplicate id population, and two paths could still walk past it. `ensure_new_record_is_unique` probed only each other store's live `sessions/<id>`. A start naming a store whose neighbour holds that id *archived* was therefore treated as new and opened a record — and the duplicate appeared later, when the user restored what they had put away. Another store's archived record is one unarchive away from standing beside this one, so it counts, and its refusal says which state it found. `agent.goal` and `agent.compact` never asked at all. Both spawn an engine when the conversation has no live one, and a spawn writes the record, so either could file the second twin that `agent.start` refuses to. A goal is reachable before the first prompt — the composer offers one on a conversation that has never run — which is exactly when the id is new to its store and old to another. Reported by Codex on #886.

`listed_stores` indexed only the live list, so one remaining row for an id was read as an unambiguous placement and moved the conversation onto it. That is wrong whenever the conversation's own record simply left the live list: a record never changes store, so its absence there means it was archived where it already was, and the row elsewhere belongs to a different conversation that happens to share the id. The consequence was worse than a mislabelled row. A project conversation archived while this client was away came back bound to the other project's record: `session.list_archived` then no longer recognized it as the archived one, so its cached transcript stayed open and writable through requests naming a store that never held it. Indexing the archived rows beside the live ones answers the question the index was always asking — which stores hold a record under this id — and two answers stay ambiguous, which is where the conversation's own binding already wins. Reported by Codex on #886.

chatgpt-codex-connector · 2026-08-18T02:15:02Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread desktop/internal/engine/archive.mbt Outdated

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread desktop/internal/engine/archive.mbt Outdated

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread desktop/frontend/update.mbt

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread desktop/internal/engine/config.mbt Outdated

Comment thread desktop/internal/engine/archive.mbt Outdated

tonyfettes mentioned this pull request Aug 15, 2026

refactor(desktop): address a conversation by its session id alone #888

Closed

tonyfettes force-pushed the haoxiang/store-qualified-live-path branch from 8c58cb3 to a66b424 Compare August 15, 2026 15:15

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

tonyfettes force-pushed the haoxiang/store-qualified-live-path branch from a66b424 to dcec41b Compare August 15, 2026 18:03

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

tonyfettes force-pushed the haoxiang/store-qualified-live-path branch from dcec41b to 768aa91 Compare August 17, 2026 06:47

tonyfettes force-pushed the haoxiang/store-qualified-live-path branch from f4f41e3 to aed8d2e Compare August 17, 2026 09:14

tonyfettes and others added 17 commits August 18, 2026 09:56

tonyfettes force-pushed the haoxiang/store-qualified-live-path branch from aed8d2e to 4db1eac Compare August 18, 2026 02:14

tonyfettes marked this pull request as draft August 19, 2026 06:48

		Some(OneStore(store)) => store
		Some(SeveralStores) \| None => conv.workspace.project()

		let max_steps = match fields.get("max_steps") {
		Some(Number(value, ..)) => Some(value.to_int())

Uh oh!

Conversation

tonyfettes commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The host selects a store instead of searching for one

The sidebar acts on the row it rendered

What this deliberately does not change

Found by E2E: two more id-keyed leftovers

Found by Codex: the ops that write a record, and the engine they write it to

Testing

Manual E2E

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

tonyfettes Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot commented Aug 17, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tonyfettes commented Aug 15, 2026 •

edited

Loading