[staging CI] unslothai/unsloth#8686 - #317
Open
danielhanchen wants to merge 25 commits into
Open
Conversation
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.
…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
Eleven assertions over the gap between opening a saved chat and its snapshot arriving: hold rather than release, capture the defaults up front, merge rather than replace, order every write, end the pairing when the read gives up. Each one has been broken at least once during review, and each break either leaks one chat's settings into another or loses an edit the user watched happen. Source assertions rather than a driven store, matching the sibling store tests: a .tsx barrel sits in the store's import graph, so it cannot be loaded in a bare node test. Checked against the earlier revisions, 10 of the 11 fail on the first of them and 4 on the most recent.
for more information, see https://pre-commit.ci
…clock - settings_seq is now compared only against the same writer's own earlier writes, identified by a per-tab id. Comparing one machine's clock with another's meant the browser that happened to be behind had every edit refused while still being told it had saved, which is worse than the race it was added to fix - carry the snapshot write in the same transaction as the guarded metadata write, or a rejected expectedTitle returns 409 with the settings already committed - remember a tab-close write that could not be confirmed and replay it next session: a chat whose row is still being created answers 404, and the creation that follows knows nothing about the edit - keep a chat's stored reasoningEnabled=false when the model forces thinking on - drop the thread-scoped state in compare mode. Two threads share one composer, so no single snapshot applies; leaving the module pointed at the last single chat let a model load read that chat's pills back into the pair Four more invariants pinned, all five failing on the previous revision.
…nd pending pins - gate the prompt queue on this chat's settings as well. handleSubmit reaches the queue branch before the send guard, so a prompt could be queued snapshotted from the installation defaults still on screen - sample the pre-window defaults once per chat rather than once per read attempt: a retry runs with the held edit already in the store, so re-sampling took that edit for the installation default and leaked it into the next chat - let a pending pin snapshot answer threadScopedOverride. A chat being pinned has no stored snapshot, so the override read null and a model load put the global pills back over the edit the queued write was about to persist Three more invariants pinned, all three failing on the previous revision.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdeee9fcd9
ℹ️ 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".
Comment on lines
+1873
to
+1876
| and writer is not None | ||
| and stored_seq is not None | ||
| and stored_writer == writer | ||
| and seq <= stored_seq |
There was a problem hiding this comment.
When two tabs edit the same thread, this check forgets a writer's last sequence as soon as another writer lands: for example, writes A/5, B/1, then a delayed A/4 are all accepted because the row contains writer B when A/4 is checked. The stale A/4 can therefore overwrite B's later settings and revert A's own newer snapshot, defeating the ordering intended for delayed PATCH or keepalive requests. Preserve the latest sequence independently for each writer rather than storing only the writer associated with the current snapshot.
Useful? React with 👍 / 👎.
for more information, see https://pre-commit.ci
…ll go wrong - sample the pre-window defaults from the installation defaults, not the store. On a switch from one saved chat to another the store still holds the outgoing chat's pills, so its values were becoming the default every snapshot-less chat follows - keep a write watermark per writer. One writer column meant another tab's write replaced it, and the delayed request the ordering exists to refuse was then compared against a watermark that was no longer its own - bound the thread read. It gates sending, and the underlying GET has no timeout, so one that never answered parked every send in the chat with nothing to release it - wait for the chat's settings in the adapter, which every run reaches. Reload, Continue and send-from-edit never touch the composer's guard and could start a run on the installation defaults - keep a replay entry until its PATCH answers ok; authFetch resolves for the 404 this exists to recover from - track a debounce-fired write as unsettled too, so a terminal event can resend it - settle a held edit before forking, not just the debounce and the write chain Seven more invariants pinned, all seven failing on the previous revision.
…r the replay - await the pairing for the RUN's own chat. One promise for all of them meant a run started for A was released by B's pairing ending, and then read B's settings for A - keep last session's replay ahead of this session's writes. It carries the previous writer id, so nothing on the server orders it, and on a slow link a full snapshot from last time could land after an edit made now and revert it - track unsettled writes by identity. Two ordinary edits both carry a null snapshot, so the first request's settle was deleting the second one's tracking - do not resend an older unsettled snapshot for a chat the terminal flush has just beaconed; every beacon takes a higher seq, so the stale one would win - evict writer watermarks by last use, never by counter: every session starts at 1, so comparing across writers threw out the newest tab and kept long-dead ones - stop clearing the defaults sample when committing held edits, which put back the resample-on-retry bug fixed a round earlier - let an explicit clear beat a capability preservation, so enabling Search does not quietly keep Deep Research stored as on - bound the gating read at the fetch, so a stalled GET is aborted and not just raced Nine more invariants pinned, all nine failing on the previous revision.
…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.
danielhanchen
force-pushed
the
pr-8686-xplat-ci
branch
from
August 14, 2026 14:30
fdeee9f to
a6b8f34
Compare
danielhanchen
force-pushed
the
pr-8686-xplat-ci
branch
from
August 14, 2026 14:30
511bb89 to
ae8191c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84bc459837
ℹ️ 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".
Comment on lines
+3455
to
+3460
| const captured = readThreadScopedSettings(state) as Record< | ||
| string, | ||
| unknown | ||
| >; | ||
| const beforeWindow = (pairingWindowDefaults ?? | ||
| globalThreadScopedDefaults) as Record<string, unknown> | null; |
There was a problem hiding this comment.
When the first saved thread is opened after model-capability logic has temporarily clamped a live setting—for example, the global Search preference is true but the current model forces toolsEnabled to false—this captures the clamped store value as globalThreadScopedDefaults. A snapshot-less thread is then pinned with false, and later chats also inherit that value even though the persisted installation default remains true, so switching to a capable model no longer restores the preference. Capture the persisted global values here, or apply the same capability-clamp preservation used when building existing thread snapshots.
Useful? React with 👍 / 👎.
Comment on lines
+2252
to
+2254
| // the open chat's own pills win, or selecting a model would revert them to the global ones. | ||
| const storedToolsEnabled = | ||
| threadScopedOverride("toolsEnabled") ?? |
There was a problem hiding this comment.
When externalProvidersForChat changes after an open thread's snapshot has been applied, such as when provider hydration finishes, this effect reruns and consults the thread override only for the four tool flags. The assignment below still replaces the thread's stored reasoningEffort with the provider default (and can force Kimi's reasoningEnabled on), so the current run no longer uses that chat's saved reasoning choice and a later full-snapshot write can persist the replacement. Resolve the reasoning fields through threadScopedOverride before applying provider capability clamps as well.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disposable CI run for unslothai/unsloth#8686. Do not merge; closed after CI.