diff --git a/api/routes.py b/api/routes.py index 5d39c95f..9dab8331 100644 --- a/api/routes.py +++ b/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 diff --git a/tests/test_issue5420_profile_switch_session_new.py b/tests/test_issue5420_profile_switch_session_new.py new file mode 100644 index 00000000..4df40477 --- /dev/null +++ b/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" diff --git a/tests/test_session_active_profile_authorization.py b/tests/test_session_active_profile_authorization.py index 1c9d9b9c..83d13d42 100644 --- a/tests/test_session_active_profile_authorization.py +++ b/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):