diff --git a/api/routes.py b/api/routes.py index b0c69425..f25e80c5 100644 --- a/api/routes.py +++ b/api/routes.py @@ -14196,6 +14196,29 @@ def _validate_session_toolsets_shape(toolsets): raise ValueError("each toolset must be a non-empty string") return toolsets + +def _resolve_new_session_workspace(body, visible_prev_session_id): + """Resolve a new-session workspace, recovering only verified inheritance.""" + candidate = body.get("workspace") + if not candidate: + return None + if ( + body.get("workspace_inherited_from_prev_session") is not True + or not visible_prev_session_id + ): + return str(resolve_trusted_workspace(candidate)) + try: + previous_session = get_session(visible_prev_session_id, metadata_only=True) + except KeyError: + return str(resolve_trusted_workspace(candidate)) + if str(getattr(previous_session, "workspace", None) or "") != str(candidate): + return str(resolve_trusted_workspace(candidate)) + workspace, _recovered = resolve_implicit_workspace_with_recovery( + candidate, + get_last_workspace, + ) + return str(workspace) + def handle_post(handler, parsed) -> bool: """Handle all POST routes. Returns True if handled, False for 404.""" diag = RequestDiagnostics.maybe_start("POST", parsed.path, logger=logger, print_fn=getattr(handler, '_safe_webui_print', None)) @@ -14529,8 +14552,13 @@ def handle_post(handler, parsed) -> bool: ) if parsed.path == "/api/session/new": + workspace_prev_session_id = body.get("prev_session_id") + if workspace_prev_session_id and not _session_id_visible_to_request_profile( + handler, workspace_prev_session_id, emit_error=False + ): + workspace_prev_session_id = None try: - workspace = str(resolve_trusted_workspace(body.get("workspace"))) if body.get("workspace") else None + workspace = _resolve_new_session_workspace(body, workspace_prev_session_id) except (TypeError, ValueError) as e: return bad(handler, str(e)) worktree_info = None diff --git a/static/sessions.js b/static/sessions.js index 8cf39905..b7a59359 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -1409,15 +1409,20 @@ async function newSession(flash, options={}){ _messagesTruncated=false; _oldestIdx=0; clearLiveToolCards(); - // One-shot profile-switch workspace wins first; otherwise prefer the profile default. + // Explicit profile switch wins, then the current conversation, then the profile default. + // Provenance lets the server recover only a deleted inherited path; explicit paths stay strict. const switchWs=S._profileSwitchWorkspace; S._profileSwitchWorkspace=null; - const inheritWs=switchWs||(S._profileDefaultWorkspace||null)||(S.session?S.session.workspace:null); + const sessionWs=(!switchWs&&S.session)?S.session.workspace:null; + const inheritWs=switchWs||sessionWs||(S._profileDefaultWorkspace||null); const reqBody={ workspace:inheritWs, profile:S.activeProfile||'default', }; - if(S.session&&S.session.session_id) reqBody.prev_session_id=S.session.session_id; + if(S.session&&S.session.session_id){ + reqBody.prev_session_id=S.session.session_id; + if(sessionWs) reqBody.workspace_inherited_from_prev_session=true; + } // Three-value worktree contract (#6022): explicit true/false is forwarded // verbatim; an ABSENT key lets the server apply the agent's config-level // `worktree:` default. Auto-bind paths pass worktree:false explicitly so a @@ -1453,7 +1458,7 @@ async function newSession(flash, options={}){ } if(newModelState&&newModelState.model){ reqBody.model=newModelState.model; - // Cold-start / picker-without-provider fallback: when the dropdown option's + // Cold-start / picker-without-provider fallback (#2518): when the dropdown option's // data-provider is empty/'default' or the persisted state predates provider // tracking, newModelState.model_provider is null. POST /api/session/new's // fast path in _resolve_compatible_session_model_state requires both model diff --git a/tests/test_issue4755_profile_default_workspace.py b/tests/test_issue4755_profile_default_workspace.py index a5ecf44f..2b211e9c 100644 --- a/tests/test_issue4755_profile_default_workspace.py +++ b/tests/test_issue4755_profile_default_workspace.py @@ -120,7 +120,11 @@ def _new_session_driver(session_workspace: str, default_workspace: str, switch_w function loadDir(){{return Promise.resolve();}} {new_session} newSession().then(()=>{{ - process.stdout.write(JSON.stringify({{captured,switchWorkspace:S._profileSwitchWorkspace}})); + process.stdout.write(JSON.stringify({{ + captured, + switchWorkspace:S._profileSwitchWorkspace, + profileDefaultWorkspace:S._profileDefaultWorkspace, + }})); }}).catch(err=>{{ console.error(err && err.stack || err); process.exit(1); @@ -130,7 +134,33 @@ def _new_session_driver(session_workspace: str, default_workspace: str, switch_w @node_test -def test_new_session_prefers_profile_default_over_current_session_workspace(): +def test_new_session_prefers_current_session_workspace_over_profile_default(): + """Deliberate divergence from the precedence introduced in #4755. + + #4755 made the "profile default" win over the current conversation's workspace. + That precedence assumes the profile default is a stable, configured value. It is + not, for a structural reason: + + ``api/workspace.py::get_profile_default_workspace()`` reads ``last_workspace.txt`` + BEFORE the configured value, and for the ``default`` profile that file is the + GLOBAL one. ``set_last_workspace()`` rewrites it on every ``/api/chat/start`` and + ``/api/session/update`` from ANY conversation. + + So ``S._profileDefaultWorkspace`` — hydrated from ``/api/profile/active`` — is not a + configured default: it is a volatile shared pointer. "profile default wins over the + current conversation" therefore degrades into "the workspace last used by ANOTHER + conversation wins over the conversation I am actually in". With several + conversations open on different workspaces, opening a new chat from conversation A + created it on conversation B's workspace. + + Precedence used here: one-shot profile switch -> current conversation -> profile + default. The profile default remains the blank-page fallback (#804/#5169) and is + never consumed (#823). + + MAINTAINER NOTE: this intentionally diverges from #4755 and will conflict if that + precedence is restored. Please do not "fix" it by reinstating the previous order + without first addressing the volatility of ``get_profile_default_workspace()``. + """ payload = _run_node(_new_session_driver( session_workspace="/current-workspace", default_workspace="/profile-default", @@ -138,8 +168,29 @@ def test_new_session_prefers_profile_default_over_current_session_workspace(): )) assert payload["captured"]["path"] == "/api/session/new" - assert payload["captured"]["body"]["workspace"] == "/profile-default" + assert payload["captured"]["body"]["workspace"] == "/current-workspace" assert payload["captured"]["body"]["prev_session_id"] == "previous-session" + assert payload["captured"]["body"]["workspace_inherited_from_prev_session"] is True + assert payload["profileDefaultWorkspace"] == "/profile-default" + + +@node_test +def test_new_session_blank_page_falls_back_to_profile_default(): + """Sans conversation chargée, le défaut de profil reste le repli (#804/#5169).""" + driver = _new_session_driver( + session_workspace="/current-workspace", + default_workspace="/profile-default", + switch_workspace=None, + ).replace( + "session:{session_id:'previous-session',workspace:\"/current-workspace\"},", + "session:null,", + ) + payload = _run_node(driver) + + assert payload["captured"]["body"]["workspace"] == "/profile-default" + assert "workspace_inherited_from_prev_session" not in payload["captured"]["body"] + assert payload["profileDefaultWorkspace"] == "/profile-default" + @node_test @@ -151,7 +202,28 @@ def test_new_session_one_shot_switch_workspace_still_wins_and_clears(): )) assert payload["captured"]["body"]["workspace"] == "/explicit-switch" + assert "workspace_inherited_from_prev_session" not in payload["captured"]["body"] assert payload["switchWorkspace"] is None + assert payload["profileDefaultWorkspace"] == "/profile-default" + + +@node_test +def test_parallel_conversations_each_inherit_their_own_workspace(): + first = _run_node(_new_session_driver( + session_workspace="/workspace-a", + default_workspace="/shared-profile-pointer", + switch_workspace=None, + )) + second = _run_node(_new_session_driver( + session_workspace="/workspace-b", + default_workspace="/shared-profile-pointer", + switch_workspace=None, + )) + + assert first["captured"]["body"]["workspace"] == "/workspace-a" + assert second["captured"]["body"]["workspace"] == "/workspace-b" + assert first["captured"]["body"]["workspace_inherited_from_prev_session"] is True + assert second["captured"]["body"]["workspace_inherited_from_prev_session"] is True @node_test diff --git a/tests/test_workspace_stale_recovery.py b/tests/test_workspace_stale_recovery.py index d103957c..7eb8ea71 100644 --- a/tests/test_workspace_stale_recovery.py +++ b/tests/test_workspace_stale_recovery.py @@ -119,6 +119,169 @@ def test_resolve_chat_workspace_with_recovery_preserves_explicit_errors(monkeypa assert saved["count"] == 0 +def _post_new_session_with_workspace( + monkeypatch, + body, + previous_session, + fallback, + *, + previous_visible=True, +): + import api.session_lifecycle as session_lifecycle + + captured = {"new": [], "fallback": 0} + + class _NewSession: + session_id = "new-session" + profile = "default" + messages = [] + + def __init__(self, workspace): + self.workspace = workspace + + def compact(self): + return { + "session_id": self.session_id, + "profile": self.profile, + "workspace": self.workspace, + } + + def get_fallback(): + captured["fallback"] += 1 + return str(fallback) + + def create_session(**kwargs): + captured["new"].append(kwargs) + return _NewSession(kwargs["workspace"]) + + monkeypatch.setattr(routes, "read_body", lambda _handler: body) + monkeypatch.setattr(routes, "_check_csrf", lambda _handler: True) + monkeypatch.setattr(routes, "_csrf_exempt_path", lambda _path: False) + monkeypatch.setattr(routes, "_handle_extension_sidecar_proxy", lambda *_a, **_k: False) + monkeypatch.setattr(routes, "_guard_request_session_visibility", lambda *_a, **_k: True) + monkeypatch.setattr( + routes, + "_session_id_visible_to_request_profile", + lambda *_a, **_k: previous_visible, + ) + monkeypatch.setattr( + routes, + "get_session", + lambda sid, metadata_only=False: previous_session + if sid == body.get("prev_session_id") + else (_ for _ in ()).throw(KeyError(sid)), + ) + monkeypatch.setattr(routes, "get_last_workspace", get_fallback) + monkeypatch.setattr(routes, "_session_model_state_from_request", lambda *_a: (None, None)) + monkeypatch.setattr(routes, "new_session", create_session) + monkeypatch.setattr( + session_lifecycle, + "_register_background_commit_thread", + lambda _thread: False, + ) + monkeypatch.setattr( + routes, + "j", + lambda _handler, payload, status=200, **_kwargs: captured.setdefault( + "response", (status, payload) + ), + ) + monkeypatch.setattr( + routes, + "bad", + lambda _handler, message, status=400, **_kwargs: captured.setdefault( + "error", (status, message) + ), + ) + + handler = SimpleNamespace(command="POST", headers={}) + routes.handle_post(handler, urlparse("/api/session/new")) + return captured + + +def test_session_new_recovers_deleted_workspace_inherited_from_visible_previous_session( + monkeypatch, tmp_path +): + fallback = tmp_path / "fallback" + fallback.mkdir() + stale = tmp_path / "deleted-workspace" + previous = SimpleNamespace( + session_id="previous-session", + profile="default", + workspace=str(stale), + ) + body = { + "workspace": str(stale), + "workspace_inherited_from_prev_session": True, + "prev_session_id": previous.session_id, + "profile": "default", + "worktree": False, + } + + monkeypatch.setattr(workspace, "_home_path", lambda: tmp_path) + monkeypatch.setattr(workspace, "load_workspaces", lambda: []) + captured = _post_new_session_with_workspace( + monkeypatch, + body, + previous, + fallback, + ) + + assert "error" not in captured + assert captured["response"][0] == 200 + assert captured["response"][1]["session"]["workspace"] == str(fallback.resolve()) + assert captured["new"][0]["workspace"] == str(fallback.resolve()) + assert captured["fallback"] == 1 + + +@pytest.mark.parametrize( + ("mark_inherited", "previous_visible", "stored_workspace"), + [ + pytest.param(False, True, "requested", id="explicit-request"), + pytest.param(True, False, "requested", id="foreign-previous-session"), + pytest.param(True, True, "different", id="stored-workspace-mismatch"), + ], +) +def test_session_new_keeps_deleted_unverified_workspace_strict( + monkeypatch, + tmp_path, + mark_inherited, + previous_visible, + stored_workspace, +): + fallback = tmp_path / "fallback" + fallback.mkdir() + stale = tmp_path / "deleted-workspace" + previous = SimpleNamespace( + session_id="previous-session", + profile="default", + workspace=(str(stale) if stored_workspace == "requested" else str(tmp_path / "other")), + ) + body = { + "workspace": str(stale), + "prev_session_id": previous.session_id, + "profile": "default", + "worktree": False, + } + if mark_inherited: + body["workspace_inherited_from_prev_session"] = True + + monkeypatch.setattr(workspace, "_home_path", lambda: tmp_path) + monkeypatch.setattr(workspace, "load_workspaces", lambda: []) + captured = _post_new_session_with_workspace( + monkeypatch, + body, + previous, + fallback, + previous_visible=previous_visible, + ) + + assert captured["error"][0] == 400 + assert "does not exist" in captured["error"][1].lower() + assert captured["new"] == [] + assert captured["fallback"] == 0 + + def test_chat_recovery_preserves_existing_implicit_trust_error(monkeypatch, tmp_path): home = tmp_path / "home" fallback = home / "fallback"