Skip to content

fix(tui): reject stale session model switches - #56083

Open
Success6666 wants to merge 1 commit into
NousResearch:mainfrom
Success6666:fix-config-set-missing-session
Open

fix(tui): reject stale session model switches#56083
Success6666 wants to merge 1 commit into
NousResearch:mainfrom
Success6666:fix-config-set-missing-session

Conversation

@Success6666

@Success6666 Success6666 commented Jul 1, 2026

Copy link
Copy Markdown

Summary

  • reject config.set updates when a non-empty session_id no longer maps to a live gateway session
  • keep no-session calls available for intentional global/default config updates
  • add regression coverage so stale desktop/TUI session requests cannot fall through to key-specific global paths
  • keep the fix scoped to the gateway guard while validating the existing desktop model controls path

Fixes #56058

Tests

  • uv run --extra dev pytest tests/test_tui_gateway_server.py -k config_set -q
  • npm --workspace apps/desktop run test:ui -- src/app/session/hooks/use-model-controls.test.tsx
  • uv run --extra dev ruff check tui_gateway/server.py tests/test_tui_gateway_server.py
  • git diff --check origin/main...HEAD

Copilot AI review requested due to automatic review settings July 1, 2026 05:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a TUI gateway edge case where config.set model changes could incorrectly fall back to the global/default model switch path when the caller provided a stale session_id that no longer maps to a live session. This aligns desktop/TUI model picker behavior with the intended “session-scoped override unless explicitly global” contract.

Changes:

  • Reject config.set requests for key=="model" when a non-empty session_id is provided but not found in _sessions.
  • Preserve the intentional global model-switch behavior when session_id is empty/omitted.
  • Add a regression test ensuring stale session model switch requests do not hit the global model switch path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tui_gateway/server.py Rejects stale session_id model updates instead of falling back to global model switching.
tests/test_tui_gateway_server.py Adds coverage to prevent missing-session requests from invoking the global model switch path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_tui_gateway_server.py Outdated
@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P2 Medium — degraded but workaround exists labels Jul 1, 2026
@Success6666
Success6666 force-pushed the fix-config-set-missing-session branch 3 times, most recently from f4bf168 to 91db1d4 Compare July 2, 2026 10:48
@Success6666

Copy link
Copy Markdown
Author

Update: rebased this branch onto current main and reran the focused validation.

Validation:

  • uv run --extra dev pytest tests/test_tui_gateway_server.py -k config_set -q — 32 passed, 272 deselected
  • npm --workspace apps/desktop run test:ui -- src/app/session/hooks/use-model-controls.test.tsx — 1 file passed, 5 tests passed
  • uv run --extra dev ruff check tui_gateway/server.py tests/test_tui_gateway_server.py — passed
  • git diff --check origin/main...HEAD — passed

Current head: 91db1d4862626490bc5080e8285b1148891371ad.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has the reported fallthrough: tui_gateway/server.py:10243 looks up the requested session without distinguishing a supplied stale ID from an omitted ID, and tui_gateway/server.py:10285-10293 routes that case through _apply_model_switch("", {"agent": None}, ...). _apply_model_switch() can persist the resolved model globally (tui_gateway/server.py:2852-2855, 3007-3008).

The PR's early 4001 guard distinguishes those cases while retaining the intentional empty-ID global path, and its added regression covers model plus the other mutating config.set keys. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@Success6666
Success6666 force-pushed the fix-config-set-missing-session branch from 91db1d4 to 63f0fdc Compare July 16, 2026 05:16
@Success6666

Copy link
Copy Markdown
Author

Rebased the branch onto the latest main and pushed the updated head 63f0fdc. The patch remains limited to rejecting stale session-scoped config.set model updates while preserving the explicit global path.\n\nValidation on the rebased head:\n- uv run --extra dev pytest tests/test_tui_gateway_server.py -k config_set -q - 34 passed, 290 deselected\n-
pm --workspace apps/desktop run test:ui -- src/app/session/hooks/use-model-controls.test.tsx - 6 tests passed\n- focused Ruff check - passed\n- git diff --check origin/main...HEAD - passed

@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Desktop chat model picker persists to global config instead of session-scoped override

4 participants