fix: keep GET /api/session side-effect free for stale models — v0.50.149 (PR #848 by @franksong2702)
Replace _normalize_session_model_in_place() on the GET /api/session read path with a read-only _resolve_effective_session_model_for_display() that returns the effective display model without writing it back to disk or the session index. Closes #845. Tests: 1856 passing.
This commit is contained in:
@@ -268,6 +268,18 @@ def _normalize_session_model_in_place(session) -> str:
|
|||||||
return effective_model
|
return effective_model
|
||||||
|
|
||||||
|
|
||||||
|
def _resolve_effective_session_model_for_display(session) -> str:
|
||||||
|
"""Resolve the model a session should display without mutating persisted state.
|
||||||
|
|
||||||
|
`GET /api/session` should stay side-effect free. If a stale persisted model
|
||||||
|
needs normalization for the current provider configuration, return the
|
||||||
|
effective model for the response payload only and leave disk state alone.
|
||||||
|
"""
|
||||||
|
original_model = getattr(session, "model", None) or ""
|
||||||
|
effective_model, _changed = _resolve_compatible_session_model(original_model or None)
|
||||||
|
return effective_model or original_model
|
||||||
|
|
||||||
|
|
||||||
from api.models import (
|
from api.models import (
|
||||||
Session,
|
Session,
|
||||||
get_session,
|
get_session,
|
||||||
@@ -604,7 +616,7 @@ def handle_get(handler, parsed) -> bool:
|
|||||||
return j(handler, {"error": "session_id is required"}, status=400)
|
return j(handler, {"error": "session_id is required"}, status=400)
|
||||||
try:
|
try:
|
||||||
s = get_session(sid)
|
s = get_session(sid)
|
||||||
_normalize_session_model_in_place(s)
|
effective_model = _resolve_effective_session_model_for_display(s)
|
||||||
raw = s.compact() | {
|
raw = s.compact() | {
|
||||||
"messages": s.messages,
|
"messages": s.messages,
|
||||||
"tool_calls": getattr(s, "tool_calls", []),
|
"tool_calls": getattr(s, "tool_calls", []),
|
||||||
@@ -613,6 +625,8 @@ def handle_get(handler, parsed) -> bool:
|
|||||||
"pending_attachments": getattr(s, "pending_attachments", []),
|
"pending_attachments": getattr(s, "pending_attachments", []),
|
||||||
"pending_started_at": getattr(s, "pending_started_at", None),
|
"pending_started_at": getattr(s, "pending_started_at", None),
|
||||||
}
|
}
|
||||||
|
if effective_model:
|
||||||
|
raw["model"] = effective_model
|
||||||
return j(handler, {"session": redact_session_data(raw)})
|
return j(handler, {"session": redact_session_data(raw)})
|
||||||
except KeyError:
|
except KeyError:
|
||||||
# Not a WebUI session -- try CLI store
|
# Not a WebUI session -- try CLI store
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ import json
|
|||||||
import pathlib
|
import pathlib
|
||||||
import re
|
import re
|
||||||
import urllib.request
|
import urllib.request
|
||||||
|
from tests.conftest import TEST_STATE_DIR
|
||||||
|
|
||||||
REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve()
|
REPO_ROOT = pathlib.Path(__file__).parent.parent.resolve()
|
||||||
from tests._pytest_port import BASE
|
from tests._pytest_port import BASE
|
||||||
@@ -23,6 +24,15 @@ def _read(rel_path: str) -> str:
|
|||||||
return (REPO_ROOT / rel_path).read_text(encoding="utf-8")
|
return (REPO_ROOT / rel_path).read_text(encoding="utf-8")
|
||||||
|
|
||||||
|
|
||||||
|
def _post(path, body=None):
|
||||||
|
data = json.dumps(body or {}).encode()
|
||||||
|
req = urllib.request.Request(
|
||||||
|
BASE + path, data=data, headers={"Content-Type": "application/json"}
|
||||||
|
)
|
||||||
|
with urllib.request.urlopen(req, timeout=10) as r:
|
||||||
|
return json.loads(r.read()), r.status
|
||||||
|
|
||||||
|
|
||||||
# ── 1. streaming.py: auth error detection ───────────────────────────────────
|
# ── 1. streaming.py: auth error detection ───────────────────────────────────
|
||||||
|
|
||||||
class TestStreamingAuthErrorDetection:
|
class TestStreamingAuthErrorDetection:
|
||||||
@@ -348,7 +358,7 @@ def test_google_active_provider_keeps_valid_gemini_session_model(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
def test_session_model_normalizer_persists_corrected_model(monkeypatch):
|
def test_session_model_normalizer_persists_corrected_model(monkeypatch):
|
||||||
"""GET /api/session should persist the corrected model back to disk/state."""
|
"""Write-path normalization should still persist corrected models."""
|
||||||
import api.routes as routes
|
import api.routes as routes
|
||||||
|
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
@@ -377,6 +387,65 @@ def test_session_model_normalizer_persists_corrected_model(monkeypatch):
|
|||||||
assert save_calls == [False]
|
assert save_calls == [False]
|
||||||
|
|
||||||
|
|
||||||
|
def test_session_model_display_resolver_is_read_only(monkeypatch):
|
||||||
|
"""Read-path model resolution must not mutate or save the session."""
|
||||||
|
import api.routes as routes
|
||||||
|
|
||||||
|
monkeypatch.setattr(
|
||||||
|
routes,
|
||||||
|
"get_available_models",
|
||||||
|
lambda: {
|
||||||
|
"active_provider": "openai-codex",
|
||||||
|
"default_model": "gpt-5.4-mini",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
save_calls = []
|
||||||
|
|
||||||
|
class DummySession:
|
||||||
|
def __init__(self):
|
||||||
|
self.model = "gemini-3.1-pro-preview"
|
||||||
|
|
||||||
|
def save(self, touch_updated_at=True):
|
||||||
|
save_calls.append(touch_updated_at)
|
||||||
|
|
||||||
|
session = DummySession()
|
||||||
|
effective = routes._resolve_effective_session_model_for_display(session)
|
||||||
|
|
||||||
|
assert effective == "gpt-5.4-mini"
|
||||||
|
assert session.model == "gemini-3.1-pro-preview"
|
||||||
|
assert save_calls == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_api_session_is_side_effect_free_for_stale_models():
|
||||||
|
"""GET /api/session must not rewrite the session file on first open (#845)."""
|
||||||
|
created, status = _post("/api/session/new", {})
|
||||||
|
assert status == 200
|
||||||
|
sid = created["session"]["session_id"]
|
||||||
|
|
||||||
|
session_path = TEST_STATE_DIR / "sessions" / f"{sid}.json"
|
||||||
|
session_data = json.loads(session_path.read_text(encoding="utf-8"))
|
||||||
|
stale_model = "google/gemini-3.1-pro-preview"
|
||||||
|
session_data["model"] = stale_model
|
||||||
|
before = json.dumps(session_data, ensure_ascii=False, indent=2)
|
||||||
|
session_path.write_text(before, encoding="utf-8")
|
||||||
|
|
||||||
|
with urllib.request.urlopen(
|
||||||
|
BASE + f"/api/session?session_id={sid}", timeout=10
|
||||||
|
) as r:
|
||||||
|
payload = json.loads(r.read())
|
||||||
|
|
||||||
|
after = session_path.read_text(encoding="utf-8")
|
||||||
|
assert payload["session"]["model"], "response should still expose an effective display model"
|
||||||
|
assert payload["session"]["model"] != stale_model, (
|
||||||
|
"response model should be compatibility-normalized on the read path"
|
||||||
|
)
|
||||||
|
assert after == before, (
|
||||||
|
"GET /api/session must return an effective model for display without "
|
||||||
|
"rewriting the session file on disk"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
# ── Model switch toast (#419) ─────────────────────────────────────────────────
|
# ── Model switch toast (#419) ─────────────────────────────────────────────────
|
||||||
|
|
||||||
class TestModelSwitchToast:
|
class TestModelSwitchToast:
|
||||||
|
|||||||
Reference in New Issue
Block a user