studio: keep each chat's composer pills and settings with the chat - #8686
Conversation
7a8adeb to
f847de7
Compare
The composer pills, the permission level and the retrieval controls were
installation-wide, so switching chats carried one conversation's modes into the
next, and reopening an old chat showed whatever the defaults happened to be.
Sixteen of those settings now travel with the thread. chat_threads gains a
settings_json column, ChatThreadSettings pins the contract PATCH
/api/chat/threads/{id} accepts, and thread-scoped-settings.ts re-validates every
value against the same literals and ranges before it is sent.
Editing one of them with a chat open writes the snapshot onto that thread and
leaves the installation defaults alone. Editing with no chat open still moves
those defaults, which every chat without a snapshot follows, so a fresh install
behaves exactly as before. A chat that stored nothing is pinned on first open,
so a later change to the defaults cannot rewrite its modes. Full access stays
session-only: the sanitizer drops it, and a write made while it is active
carries through the level the chat already held.
resolveToolsEnabledOnLoad, setBypassPermissions, setDeepResearchEnabled and both
external-provider effects in chat-page.tsx read the open chat's value before the
installation one, so a model load or a model switch no longer re-applies the
defaults over the pills a chat is running with. A thread that stores no value
for a setting falls back to the defaults rather than to the outgoing chat's.
upsert_chat_thread COALESCEs the column, so the writers that rebuild a thread
record cannot clear it; fork_chat_thread copies it; and list_chat_threads leaves
it out, since the sidebar lists every thread and only an opened one reads it.
normalizeStoredPermissionMode moves the legacy confirm-toggle migration out of
the store so it can be pinned directly. Once the level is mirrored to
/api/chat/settings the first hydration seeds it there, so the mapping can no
longer be driven through the UI more than once per installation.
for more information, see https://pre-commit.ci
…on a switch
Two fixes on top of the thread-scoped settings work.
A pill clicked between opening a saved chat and its GET /api/chat/threads/{id}
returning was written to the installation defaults, because captureThreadScopedEdit
needs activeThreadId to equal threadScopedSettingsThreadId and the pairing is only
set when that read resolves. The click moved every snapshot-less chat's default and
was then overwritten by the arriving snapshot, so it leaked and appeared to do
nothing. Those edits are now held for the duration of the read: if the chat turns
out to own a row they win over what came back and are stored on it, and if it does
not (a new chat's runtime id, a legacy row, a failed read) they are replayed to the
defaults exactly as before. On loopback the read wins the race, so this shows up on
remote and tunnelled sessions.
buildThreadScopedSnapshot carried the stored permissionMode forward when apply()
held it back under Full access, but not deepResearchEnabled, which apply() holds
back the same way for external models and incognito. Toggling any other pill in
such a chat erased the true it had stored. Carried forward the same way.
1d0f9df to
562c6b6
Compare
…ents only Two problems found while simulating upgrades, downgrades and the three browser engines against this branch. settings_json is the first strictly validated nested model Studio builds out of the database rather than off the wire, and a stored snapshot outlives the build that wrote it. A newer Studio adding a setting, widening an enum or raising a bound writes a blob this one rejects, and refusing it 500s the chat on open, on fork and on patch, and takes GET /api/chat/export down for every chat, not just the affected one. _json_loads already shrugs off JSON that will not parse, so JSON that parses but postdates this build now gets the same treatment: rows go through thread_from_row, which drops what it cannot read. Only the read is forgiving. Both wire models stay extra = forbid, so a client still cannot invent a setting, and the row is left untouched, so upgrading again restores it. The unload flush also ran on visibilitychange, which is not terminal: it fires on every tab switch and the page carries on afterwards. That path used the keepalive beacon, which PATCHes the row directly and answers 404 for a thread whose row has not been created yet, while having already consumed the pending write. Terminal events keep the beacon; visibilitychange takes the normal path, which creates the row first and which the page is still alive to await. tests/studio/sim_thread_settings_portability.py covers the migration against a populated pre-existing database, idempotency, the COALESCE preservation, unicode and emoji and Windows-shaped paths through the column, WAL, and the SQLite floor. It is stdlib only, so it runs on Windows and macOS as well as Linux.
for more information, see https://pre-commit.ci
…r-broad guards Six items from the review, all reproduced against the code at head first. The composer is live while a chat's settings are being read, and until they land the store still holds the OUTGOING chat's values. snapshotQueuedChatRunSettings captures permissionMode at send time and sends it as permission_mode for the whole run, so a send in that window really does run under the previous chat's level, and a rejected read left those values up indefinitely. The store now drops to the installation defaults for the duration of the read, which is the only honest thing to show for a chat whose own settings are not known yet. Edits made during that window were still held, but the switch-away path replayed them into the installation defaults, which is the leak the holding exists to prevent. They are now committed to the thread they were made in; the values are still in the store at that point because the incoming chat's read has not resolved, and a chat with no row writes nothing, so an unsaved chat stays on the defaults. activeThreadScopedSettings was only refreshed when the debounce built its payload, so for 400ms after an edit neither it nor localStorage carried the new value. A model status poll landing there reverted the pill and the pending write persisted the revert. threadScopedOverride now prefers the live store while a captured write is pending; a pending pin keeps its own snapshot. Snapshot writes are chained per thread. The backend replaces settings_json rather than merging it, so two unordered writes do not merge, they pick a winner. Two guards were too broad. A chat first opened under Full access was pinned with no permission level at all and then followed the installation one forever; it now records the level underneath. And the deep research guard refused every external checkpoint, while externalCheckpointRefusesDeepResearch already exists and treats openai_codex as supported, which is the rule the composer follows.
…on window, and two restore constraints - await this chat's in-flight snapshot PATCH before reading it back, so a chat edited, left and re-entered does not get its pre-edit snapshot applied over the values the user just set - keep the chat paired after a failed thread read, with a bounded retry, so later edits in it stay thread-scoped instead of moving the installation defaults - give each per-thread write a ticket so a queued write cannot land after the unload beacon and restore the older snapshot - start holding a chat's edits as soon as its id is known, not once /api/chat/settings has hydrated; the composer is interactive in between - keep Search and Thinking mutually exclusive on Kimi when restoring a snapshot - keep a stored Search/Code preference that a tool-less model has clamped off
…write races - send only the edited fields, merged onto the row's own snapshot, when a chat's read never landed; the full replace was erasing settings the user never touched - release a held edit as soon as the first read says the row does not exist, instead of waiting for an unrelated history event - keep global hydration off a field whose edit is still held for its chat - keep a held edit out of the captured installation defaults - preserve a clamped Images and Fetch preference too, not just Search and Code - abort a settings PATCH that is already out when the unload beacon supersedes it
…that waits for them - PATCH gains settingsPatch, which applies only the fields it names. The unload path and the commit-on-leave path use it: both know what changed but not what else the row holds, and a replacement built from the defaults on screen was erasing the rest of the chat's snapshot - a replacement now keeps whatever the writing client could not read, so an older Studio opening a newer database no longer deletes the settings it had to drop - flush an edit held during pairing on a terminal event, keepalive, instead of relying on effect cleanup that unload does not guarantee - a legacy Dexie fallback means the backend read FAILED, so retry and keep the chat paired rather than releasing its edits to the installation defaults - park a send while the chat's own settings are still on their way, reusing the existing wait-and-send path, so a chat stored as ask cannot run on a global off
for more information, see https://pre-commit.ci
…and stop parking sends forever - give up openly when the thread read is out of retries: staying paired held every send behind a wait with nothing left to resolve it, so the chat now falls back to the installation defaults and says so - record the installation defaults when pairing begins. Deleting a held field from the capture left it with no fallback, and on the session's first pairing the edited value then stayed live into the next chat, which is the same leak - add settings_seq and refuse a snapshot write older than the one already stored: aborting a fetch does not stop a handler the server has already started - do the read, merge and write of a snapshot in one transaction, so two tabs cannot both build a replacement from the same stale row - re-send with keepalive on a terminal event anything an earlier visibilitychange flushed normally but has not landed - settle the source snapshot before forking, or the fork copies the modes the chat had before the pill the user just clicked
…first The Search and Code pills are disabled while a model that cannot run tools is loaded, and this test drives both. The CI job it was added to leaves a small GGUF resident from an earlier step, so every pill click timed out on a disabled button: the test failed for a reason that has nothing to do with what it checks. Unload first, which is also the state the test is about, since with nothing loaded the pills stay pre-selectable. Found by running the job on a staging repo while the org queue was backed up; this step has never actually completed on this PR.
- release a chat's pairing gate only for that chat. The previous round held them per chat but resolved every one on any settle, so a run started for A was still freed by B's pairing ending. Leaving a chat mid-read now leaves its gate shut, since its snapshot never arrived, and the wait is bounded so a run cannot hang on it - clear a thread's replay entry once a write of this session's lands. The entry carries the previous session's writer id, which the server will not order, so a retry could revert settings changed since - bound each replay request. Every settings write waits on them, so one socket that never settled left the session unable to persist anything - keep a failed write tracked, so a terminal event still beacons it. Dropping it left nothing to resend and the edit came back reverted - let a failed snapshot write reach the fork, which now stops rather than making a fork that carries the pre-edit modes Six more invariants pinned, all six failing on the previous revision.
An unsaved chat carries an assistant-ui runtime id (__LOCALID_...), which no row exists for, so its settings read can only 404. Holding edits behind that certain failure meant a pill or permission level chosen on a fresh /chat did not reach the installation defaults until the round trip came back, and playwright_chat_ui asserts it is there immediately: 'Run automatically persisted ask, expected off'. That test is untouched by this PR and passed at an earlier revision of it, so this is a regression the branch introduced. Found by running the suite on a staging repo while the org queue was backed up.
Four fixes from review: - A run whose pairing wait ran out no longer proceeds. The wait only expires for a chat left mid-read, whose gate is held shut on purpose, so the store describes a different chat by then; the run is refused instead. The wait is also raised past the read's own retry budget so an ordinary slow read never reaches it. - A fork now fails when the flushed write for the chat it is copying failed. The replacement path reports failure by resolving false, which the settle helper ignored, so the copy took the pre-edit snapshot silently. - A replay clears only the body it sent. A terminal event in this session can store a newer body for the same thread while an older replay is out. - A provider constraint no longer rewrites what a chat stored. Kimi's builtin search cannot run with thinking, so the composer moves the other pill with persist: false, deliberately bypassing the capture path; the next full snapshot then saved the provider's value over the user's.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 868492f3c8
ℹ️ 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".
| if settings_write is not None: | ||
| if _write_chat_thread_settings_in_conn(conn, id, **settings_write) is None: | ||
| conn.rollback() | ||
| return None |
There was a problem hiding this comment.
Roll back metadata when rejecting a stale settings write
When a PATCH combines metadata with settings and its settingsSeq is stale for the writer, _write_chat_thread_settings_in_conn returns False, but this condition only handles None. The metadata assignments are therefore committed while the settings portion is silently rejected, violating the intended all-or-nothing transaction and leaving callers with a partially applied PATCH. Handle False separately and roll back the metadata changes as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I do not think this one holds, so I have left it as is and documented why. The all-or-nothing here is about failure: a rejected metadata precondition must not leave the settings committed, and a missing row must write nothing. A refusal is neither. False means this same writer has already landed a newer snapshot, so the row holds what the writer wanted whether or not this write applies, and rolling the metadata back with it would silently drop a rename the client sent in the same PATCH and got a 200 for. The docstring said together-or-not-at-all without that distinction, which is fair to read the way you did; 7bf2a5b spells it out. Happy to revisit if there is a caller where a stale seq should void the metadata too.
The UI workflow boots this server on the Studio home the chat-ui and cross-browser permission tests have already driven, so the install is shared rather than fresh and arrives carrying whatever level they left in the mirrored settings. Every assertion in the file names a literal level, so the run failed on staging with the composer showing Run automatically where Approve for me was expected, before a single per-chat assertion had been made. Set the default from the unsaved chat the test already starts on, where no chat is open and the edit is the installation's, and print it.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3207292384
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| applyThreadScopedSettings(null, null); | ||
| releaseHeldThreadScopedEdits(); |
There was a problem hiding this comment.
Release the exhausted thread's pairing gate
When all thread-settings read attempts fail, commitHeldThreadScopedEditsToTheirThread() has already cleared pendingPairingThreadId but deliberately leaves the per-thread gate unresolved. These calls therefore cannot close pairingSettledByThreadId[activeThreadId]; the composer appears ready and the toast says defaults are usable, yet every subsequent run waits 30 seconds in awaitThreadScopedPairing() and is then refused until the chat is reopened. Explicitly settle the active thread's gate when falling back after retry exhaustion.
Useful? React with 👍 / 👎.
| } | ||
| writeStorageValue(key, raw); | ||
| }; | ||
| if (mirrored && captureThreadScopedEdit(mirrored.field, writeGlobal)) return; |
There was a problem hiding this comment.
Keep compatibility clamps out of explicit thread edits
When an open chat has deepResearchEnabled: true and switches to an external checkpoint that cannot run Deep Research, the existing setCheckpoint() path calls saveBool(..., false) as a compatibility clamp. This new generic capture path marks that call as an explicit thread edit, so buildThreadScopedSnapshot() skips its external-model preservation branch and permanently replaces the chat's stored value with false; switching back to a local model no longer restores the chat's preference. Compatibility-derived writes need to bypass the explicit-edit marker, as the non-persisting reasoning/tool constraint paths already do.
Useful? React with 👍 / 👎.
Four fixes from review: - Each pairing read now carries its own deadline and controller. bounded: true is the 30 second WRITE timeout, so the losing side of the 8 second race stayed open while the retry opened the next one, up to three per chat during an outage, and leaving the chat cancelled none of them. - The ensure step in front of a settings write is bounded and takes the caller's signal. It runs before the write, so neither the signal nor the write timeout reached it, and a stall there left the per-thread chain pending for the life of the page with reopening and forking that chat waiting on it. - A default hydration skipped because its field was held is kept and used when the pairing window closes. The restore was falling back to the pre-hydration copy, so snapshot-less chats followed this browser's stale value rather than the server's until a reload. - Tab-close snapshots are replayed even when /api/chat/settings fails to hydrate. They are rows' own settings and have nothing to do with that endpoint; the replay is guarded so it still runs once. Also documents why a refused settings write does not roll the metadata in the same PATCH back: a refusal means this writer already landed something newer, not that the write failed.
|
@codex review |


The composer pills, the permission level and the retrieval controls are installation-wide, so switching chats carries one conversation's modes into the next, and reopening an old chat shows whatever the defaults happen to be. This gives sixteen of those settings to the thread instead.
Stacked on #8656, which this branch is based on.
What changed
chat_threadsgains asettings_jsoncolumn with an idempotentALTER TABLEfor existing databases.ChatThreadSettingspins the contractPATCH /api/chat/threads/{id}accepts, andthread-scoped-settings.tsre-validates every value against the same literals and ranges before it is sent, matching how the mirrored settings already work.Per chat:
reasoningEnabled,reasoningEffort, the Search, Code, Images, Fetch, Deep Research, Artifacts and MCP pills,permissionMode, and the six RAG controls. Left global: Canvas visibility, artifact collapse and network access, the RAG ingest options, the research website policy, the tool-call limits, the model-load preferences and the quantization display flags.How it behaves
Editing a setting with a chat open writes the snapshot onto that thread and leaves the installation defaults alone. Editing with no chat open still moves those defaults, which every chat without a snapshot follows, so a fresh install behaves exactly as it does today. A chat that stored nothing is pinned on first open, so a later change to the defaults cannot rewrite its modes. A thread that stores no value for a setting falls back to the defaults rather than to the outgoing chat's.
Full access stays session-only. The sanitizer drops it, and a write made while it is active carries through the level the chat already held, so enabling it cannot erase that.
resolveToolsEnabledOnLoad,setBypassPermissions,setDeepResearchEnabledand both external-provider effects inchat-page.tsxread the open chat's value before the installation one, so a model load or a model switch no longer re-applies the defaults over the pills a chat is running with.upsert_chat_threadCOALESCEs the column so the writers that rebuild a thread record cannot clear it,fork_chat_threadcopies it, andlist_chat_threadsleaves it out since the sidebar lists every thread and only an opened one reads it.normalizeStoredPermissionModemoves the legacy confirm-toggle migration out of the store so it can be pinned directly. Once the level is mirrored to/api/chat/settingsthe first hydration seeds it there, so the mapping can no longer be driven through the UI more than once per installation.Verification
Fully verified by hand in Unsloth Studio, in addition to the automated checks below.
tests/studio/playwright_thread_scoped_settings.pydrives the real UI end to end against a live backend and SQLite, and is wired intostudio-ui-smoke.yml. It covers an unsaved chat still editing the defaults, two chats keeping their own pills and permission levels across a sidebar switch, a full reload and a switch back, a new chat and a snapshot-less chat opening on the defaults, the stored snapshots read back over the API, and the thread listing staying free of them. It needs no model, since the Search and Code pills stay clickable with nothing loaded.studio/backend/tests/test_chat_thread_settings.pycovers the payload contract, thesettings_jsonmigration against a database created before this change, snapshot survival across a record rewrite, replace-not-merge on patch, the listing omission and fork inheritance.studio/frontend/tests/thread-scoped-settings.test.tspins what the client may send and which settings may become per-chat;mirrored-chat-settings.test.tspins the permission-level mapping.2036 frontend tests, 103 backend chat tests,
tsc -b, biome and ruff all pass, andplaywright_chat_ui.py --permission-onlypasses.