Skip to content

fix: isolate model selection per chat to prevent cross-chat leakage#8093

Open
octo-patch wants to merge 1 commit into
janhq:mainfrom
octo-patch:fix/issue-8083-isolate-model-selector-per-chat
Open

fix: isolate model selection per chat to prevent cross-chat leakage#8093
octo-patch wants to merge 1 commit into
janhq:mainfrom
octo-patch:fix/issue-8083-isolate-model-selector-per-chat

Conversation

@octo-patch
Copy link
Copy Markdown
Contributor

Fixes #8083

Problem

When multiple chats are open simultaneously and MCP tool chains are running,
switching the model selector in one chat tab affects in-flight requests in
other tabs. This happens because CustomChatTransport.sendMessages() always
reads selectedModel and selectedProvider from the global Zustand store at
the time of each call. If the user switches to another tab and picks a
different model mid-chain, the next MCP tool-result follow-up in the original
tab uses the newly selected model instead of the one that started the
conversation.

Solution

Added _chainModelId and _chainProviderId private fields to
CustomChatTransport. At the start of every sendMessages() call the code
detects whether we are beginning a new user turn (last message is not an
assistant message with tool-invocation parts) or continuing an existing MCP
tool chain. On a new user turn the fields are snapshotted from global state;
on a tool-chain continuation the previously locked values are reused.

The same guard is applied in refreshTools() so the capabilities check for
tool support also uses the chain-locked model.

Intentional within-thread model changes still take effect normally: they are
picked up on the next user message submission, which resets the chain
snapshot.

Testing

  1. Open two chats simultaneously.
  2. In Chat A select Model X and send a message that triggers several MCP tool
    calls (e.g. a long multi-step task).
  3. While Chat A is mid-chain, switch to Chat B and select Model Y.
  4. Observe that Chat A continues using Model X for all remaining tool-result
    follow-ups.

Previously step 4 would silently switch Chat A's backend requests to Model Y.

…eakage (fixes janhq#8083)

Each CustomChatTransport now locks onto the model/provider that was active
when the user submitted a message and reuses it for the entire MCP tool-call
chain.  Previously, switching the model selector in a second chat tab while
an MCP tool chain was running in the first tab would cause all subsequent
tool-result follow-up requests in chat 1 to use the model selected in chat 2.

The lock is snapshotted at the start of every new user turn (detected by the
last message being an assistant message without tool-invocation parts) and
held for all downstream sendMessages() calls within that chain.  It updates
naturally when the user sends a fresh message, so intentional within-thread
model changes still take effect on the next turn.
@tokamak-pm
Copy link
Copy Markdown

tokamak-pm Bot commented Apr 28, 2026

Review: isolate model selection per chat

Good fix for a real user-facing bug. The approach of snapshotting the model/provider at the start of each user turn and reusing it for MCP tool-chain continuations is clean and correctly targeted. The implementation reads well and the fallbacks are sensible.

Requested Changes

  1. Add unit tests for the chain-locking behavior. This is the main gap. The tool-chain detection heuristic (checking lastMsg.role === 'assistant' with tool-invocation parts) is non-trivial and should be covered. Suggested test cases:
    • A fresh user message (last message is user role) should snapshot from the global store.
    • A tool-chain continuation (last message is assistant with tool-invocation parts) should reuse the previously snapshotted model/provider, even if the global store has changed.
    • After a chain completes and a new user message is sent, the snapshot should update to the current global values.

Minor

  1. The (p as { type: string }).type cast in parts.some() could use the actual part type from the AI SDK if one is available, to improve type safety. Not blocking.

  2. Consider adding a brief inline comment on the ?? '' fallback in getProviderByName(providerId ?? '') explaining why it is safe (provider lookup returns undefined, caught downstream). This aids future readers.

The fix is solid and addresses the reported issue correctly. With tests added, this is ready to merge.

Recommendation: improve needed

@tokamak-pm
Copy link
Copy Markdown

tokamak-pm Bot commented May 1, 2026

Review: fix: isolate model selection per chat to prevent cross-chat leakage

Summary: Fixes a bug where switching the model selector in one chat tab could alter in-flight MCP tool-chain requests in another tab. The fix snapshots modelId and providerId into private _chain* fields at the start of each user turn, and reuses them for all subsequent tool-chain follow-up calls.

Positive observations

  • Addresses a real and serious bug: cross-chat model leakage during MCP tool chains could produce silently wrong results.
  • The chain-locking approach is sound -- snapshot on new user turn, reuse on tool-chain continuation.
  • The detection heuristic (checking if the last message is an assistant message with tool-invocation parts) is reasonable and aligned with how the AI SDK structures MCP flows.
  • The same guard is applied in refreshTools(), which is a good catch -- tool capability checks also need to use the locked model.

Issues and observations

  1. No tests. This is a behavioral fix in critical inference path code. The CustomChatTransport class should have at least a unit test verifying that _chainModelId/_chainProviderId are snapshotted on the first call and preserved on tool-chain continuation calls. Without tests, regressions are likely.
  2. Fallback to global state. The pattern this._chainModelId ?? useModelProvider.getState().selectedModel?.id means that if _chainModelId is null for any reason, the code silently falls back to global state, which is exactly the bug being fixed. Consider making the chain fields non-nullable after the initial snapshot, or at least logging a warning when the fallback is hit.
  3. Chain fields are never explicitly reset. _chainModelId and _chainProviderId are set when !isToolChainContinuation || !this._chainModelId, but they are never cleared when a conversation ends or the transport is reused. If the same CustomChatTransport instance is reused across conversations (unclear from this diff alone), the stale chain values could persist. Verify the transport lifecycle.
  4. getProviderByName(providerId ?? '') with empty string. Passing an empty string to getProviderByName will return undefined, which then falls through to the error check. This works but is semantically misleading -- consider checking for nullish providerId explicitly before calling the method.
  5. lastMsg.parts type assertion. The cast (p as { type: string }).type is fragile. If the parts structure changes, this will silently stop matching. Consider using a more specific type guard.

Recommendation: fix needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

bug: Model selector changes are not isolated, and may affect other (already running) chats.

1 participant