Merge #5423: unblock session/new after profile switch (#5420) [nankingjing]

api/routes.py

@@ -454,10 +454,12 @@ def _visible_pinned_lineage_ids(session_rows) -> set[str]:
 from api.profiles import (  # noqa: F401, E402  (re-export)
     _profiles_match,
     _is_isolated_profile_mode,
+    _is_root_profile,
     _SKILLS_STATS_CACHE,
     get_active_profile_name,
     get_active_profile_name as _get_active_profile_name,
     get_active_hermes_home,
+    list_profiles_api,
 )
 
 
@@ -12015,7 +12017,8 @@ def handle_get(handler, parsed) -> bool:
     if parsed.path == "/api/sessions":
         diag = RequestDiagnostics.maybe_start("GET", parsed.path, logger=logger)
         try:
-            from api.profiles import get_active_profile_name
+            from api import profiles as profiles_api
+
             diag.stage("load_settings")
             settings = load_settings()
             show_cli_sessions = bool(settings.get("show_cli_sessions"))
@@ -12026,7 +12029,7 @@ def handle_get(handler, parsed) -> bool:
             show_cron_sessions = bool(settings.get("show_cron_sessions"))
             show_webhook_sessions = bool(settings.get("show_webhook_sessions"))
             agent_session_source_filter = settings.get("agent_session_source_filter")
-            active_profile = get_active_profile_name()
+            active_profile = profiles_api.get_active_profile_name()
             all_profiles = _all_profiles_enabled(parsed)
             include_archived = _query_flag(parsed, "include_archived")
             exclude_hidden = _query_flag(parsed, "exclude_hidden")
@@ -12087,8 +12090,9 @@ def handle_get(handler, parsed) -> bool:
         # ── Profile scoping (#1614) ────────────────────────────────────────
         # Default: filter to the active profile. ?all_profiles=1 returns the
         # aggregate list so settings/admin UIs can still see everything.
-        from api.profiles import get_active_profile_name
-        active_profile = get_active_profile_name()
+        from api import profiles as profiles_api
+
+        active_profile = profiles_api.get_active_profile_name()
         all_projects = load_projects()
         isolated_profile_mode = _is_isolated_profile_mode()
         all_profiles = _all_profiles_enabled(parsed)
@@ -12509,28 +12513,21 @@ def handle_get(handler, parsed) -> bool:
 
     # ── Profile API (GET) ──
     if parsed.path == "/api/profiles":
-        from api.profiles import (
-            get_active_profile_name,
-            list_profiles_api,
-        )
+        from api import profiles as profiles_api
 
         return j(
             handler,
             {
-                "profiles": list_profiles_api(),
-                "active": get_active_profile_name(),
+                "profiles": profiles_api.list_profiles_api(),
+                "active": profiles_api.get_active_profile_name(),
                 "single_profile_mode": _is_isolated_profile_mode(),
             },
         )
 
     if parsed.path == "/api/profile/active":
-        from api.profiles import (
-            _is_root_profile,
-            get_active_hermes_home,
-            get_active_profile_name,
-        )
+        from api import profiles as profiles_api
 
-        active_profile_name = get_active_profile_name()
+        active_profile_name = profiles_api.get_active_profile_name()
         # Resolve the ACTIVE PROFILE's configured workspace so a cold boot with a
         # profile cookie shows the right composer workspace chip on a blank
         # new-chat page (#5169). Use get_profile_default_workspace() (NOT
@@ -12548,8 +12545,8 @@ def handle_get(handler, parsed) -> bool:
             handler,
             {
                 "name": active_profile_name,
-                "path": str(get_active_hermes_home()),
-                "is_default": _is_root_profile(active_profile_name),
+                "path": str(profiles_api.get_active_hermes_home()),
+                "is_default": profiles_api._is_root_profile(active_profile_name),
                 "default_workspace": _profile_default_workspace,
             },
         )
@@ -13018,19 +13015,29 @@ def handle_post(handler, parsed) -> bool:
         # ── Memory lifecycle: commit the previous session before starting a new one ──
         prev_session_id = body.get("prev_session_id")
         if prev_session_id:
-            if not _session_id_visible_to_request_profile(handler, prev_session_id):
-                return True
-            try:
-                from api.session_lifecycle import commit_session_memory
-                from api.config import SESSION_AGENT_CACHE, SESSION_AGENT_CACHE_LOCK
-                prev_agent = None
-                with SESSION_AGENT_CACHE_LOCK:
-                    _cached = SESSION_AGENT_CACHE.get(prev_session_id)
-                    if _cached:
-                        prev_agent = _cached[0]
-                commit_session_memory(prev_session_id, agent=prev_agent)
-            except Exception:
-                logger.debug("Lifecycle commit for prev_session %s failed", prev_session_id, exc_info=True)
+            if not _session_id_visible_to_request_profile(
+                handler, prev_session_id, emit_error=False
+            ):
+                # Cross-profile hand-off after a profile switch: skip memory
+                # commit for the previous profile's session, but still create
+                # the new session (#5420).
+                prev_session_id = None
+            if prev_session_id:
+                try:
+                    from api.session_lifecycle import commit_session_memory
+                    from api.config import SESSION_AGENT_CACHE, SESSION_AGENT_CACHE_LOCK
+                    prev_agent = None
+                    with SESSION_AGENT_CACHE_LOCK:
+                        _cached = SESSION_AGENT_CACHE.get(prev_session_id)
+                        if _cached:
+                            prev_agent = _cached[0]
+                    commit_session_memory(prev_session_id, agent=prev_agent)
+                except Exception:
+                    logger.debug(
+                        "Lifecycle commit for prev_session %s failed",
+                        prev_session_id,
+                        exc_info=True,
+                    )
         s = new_session(
             workspace=workspace,
             model=model,
@@ -14604,7 +14611,6 @@ def handle_post(handler, parsed) -> bool:
         # #1614: refuse moves into a project owned by another profile.
         target_pid = body.get("project_id") or None
         if target_pid:
-            from api.profiles import get_active_profile_name
             # Use the session's own profile for authorization, not the global
             # active profile. A session belongs to a specific profile set at
             # creation; projects from that profile should always be assignable,
@@ -14654,7 +14660,6 @@ def handle_post(handler, parsed) -> bool:
         except ValueError as e:
             return bad(handler, str(e))
         import re as _re
-        from api.profiles import get_active_profile_name
 
         name = body["name"].strip()[:128]
         if not name:
@@ -14689,7 +14694,6 @@ def handle_post(handler, parsed) -> bool:
         except ValueError as e:
             return bad(handler, str(e))
         import re as _re
-        from api.profiles import get_active_profile_name
 
         projects = load_projects()
         proj = next(
@@ -14715,7 +14719,6 @@ def handle_post(handler, parsed) -> bool:
             require(body, "project_id")
         except ValueError as e:
             return bad(handler, str(e))
-        from api.profiles import get_active_profile_name
         projects = load_projects()
         proj = next(
             (p for p in projects if p["project_id"] == body["project_id"]), None

tests/test_issue5420_profile_switch_session_new.py

@@ -0,0 +1,116 @@
+"""Regression tests for #5420 profile-switch /api/session/new failures."""
+
+from __future__ import annotations
+
+import ast
+import inspect
+from unittest.mock import MagicMock
+from urllib.parse import urlparse
+
+import api.routes as routes
+
+
+def _post_session_new(body: dict, monkeypatch):
+    cap = {}
+
+    def _j(_handler, payload, *_, **__):
+        cap["ok"] = payload
+        return True
+
+    def _bad(_handler, msg, code=400):
+        cap["bad"] = (msg, code)
+        return True
+
+    handler = MagicMock()
+    handler.command = "POST"
+    handler.headers = {}
+
+    monkeypatch.setattr(routes, "read_body", lambda _h: body)
+    monkeypatch.setattr(routes, "_check_csrf", lambda _h: True)
+    monkeypatch.setattr(routes, "_csrf_exempt_path", lambda _p: False)
+    monkeypatch.setattr(routes, "_guard_request_session_visibility", lambda *_a, **_k: True)
+    monkeypatch.setattr(routes, "j", _j)
+    monkeypatch.setattr(routes, "bad", _bad)
+
+    routes.handle_post(handler, urlparse("/api/session/new"))
+    return cap
+
+
+def test_handle_post_does_not_shadow_get_active_profile_name():
+    """handle_post must not re-import get_active_profile_name locally (#5420)."""
+    src = inspect.getsource(routes.handle_post)
+    tree = ast.parse(src)
+    for node in ast.walk(tree):
+        if isinstance(node, ast.ImportFrom) and node.module == "api.profiles":
+            for alias in node.names:
+                assert alias.name != "get_active_profile_name", (
+                    "local import shadows module-level get_active_profile_name"
+                )
+
+
+def test_session_new_succeeds_with_cross_profile_prev_session_id(monkeypatch):
+    created = {}
+
+    class _Session:
+        def __init__(self):
+            self.session_id = "new123"
+            self.profile = "work"
+            self.messages = []
+
+        def compact(self):
+            return {"session_id": self.session_id, "profile": self.profile}
+
+    def _new_session(**_kwargs):
+        s = _Session()
+        created["session"] = s
+        return s
+
+    monkeypatch.setattr(routes, "new_session", _new_session)
+    monkeypatch.setattr(
+        routes,
+        "_session_id_visible_to_request_profile",
+        lambda *_a, **_k: False,
+    )
+
+    cap = _post_session_new(
+        {"profile": "work", "prev_session_id": "old-from-default"},
+        monkeypatch,
+    )
+
+    assert "bad" not in cap
+    assert "ok" in cap
+    assert cap["ok"]["session"]["session_id"] == "new123"
+    assert created["session"].profile == "work"
+
+
+def test_session_new_still_commits_same_profile_prev_session_id(monkeypatch):
+    class _Session:
+        def __init__(self):
+            self.session_id = "new456"
+            self.profile = "default"
+            self.messages = []
+
+        def compact(self):
+            return {"session_id": self.session_id}
+
+    monkeypatch.setattr(routes, "new_session", lambda **_k: _Session())
+    monkeypatch.setattr(
+        routes,
+        "_session_id_visible_to_request_profile",
+        lambda *_a, **_k: True,
+    )
+
+    commit_calls = []
+
+    def _commit(prev_session_id, **kwargs):
+        commit_calls.append(prev_session_id)
+
+    import api.session_lifecycle as lifecycle
+
+    monkeypatch.setattr(lifecycle, "commit_session_memory", _commit)
+
+    cap = _post_session_new({"prev_session_id": "same-profile-old"}, monkeypatch)
+
+    assert "bad" not in cap
+    assert commit_calls == ["same-profile-old"]
+    assert cap["ok"]["session"]["session_id"] == "new456"

tests/test_session_active_profile_authorization.py

@@ -463,25 +463,39 @@ def test_chat_stream_allows_unknown_dead_stream_fallback_replay_path(monkeypatch
     assert calls["replay"] == 0
 
 
-def test_session_new_blocks_prev_session_from_other_profile(monkeypatch):
+def test_session_new_skips_prev_session_commit_from_other_profile(monkeypatch):
+    """Cross-profile prev_session_id after a profile switch must not 404 (#5420)."""
     import api.session_lifecycle as session_lifecycle
     handler = _FakeHandler()
     foreign = _SimpleSession("foreign_session", profile="other")
     calls = {"commit": 0, "new": 0}
 
+    class _NewSession:
+        def __init__(self):
+            self.session_id = "new_session"
+            self.messages = []
+
+        def compact(self):
+            return {"session_id": self.session_id}
+
     monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True)
     monkeypatch.setattr(routes, "read_body", lambda _handler: {"prev_session_id": "foreign_session"})
     monkeypatch.setattr(routes, "_get_active_profile_name", lambda: "default")
     monkeypatch.setattr(routes, "get_session", lambda sid, metadata_only=False: foreign if sid == "foreign_session" else (_ for _ in ()).throw(KeyError("Session not found")))
     monkeypatch.setattr(session_lifecycle, "commit_session_memory", lambda sid, agent=None: calls.__setitem__("commit", calls["commit"] + 1))
-    monkeypatch.setattr(routes, "new_session", lambda **_kwargs: (_ for _ in ()).throw(AssertionError("new session should not be created")))
+    monkeypatch.setattr(
+        routes,
+        "new_session",
+        lambda **_kwargs: calls.__setitem__("new", calls["new"] + 1) or _NewSession(),
+    )
 
     cap = _capture(monkeypatch)
     routes.handle_post(handler, urlparse("/api/session/new"))
 
     assert calls["commit"] == 0
-    assert calls["new"] == 0
-    assert cap["bad"] == ("Session not found", 404)
+    assert calls["new"] == 1
+    assert "bad" not in cap
+    assert cap["ok"]["session"]["session_id"] == "new_session"
 
 
 def test_session_new_keeps_prev_session_commit_for_same_profile(monkeypatch):