From eee72f942ec29dc8d0765f9d6d3dbf1b4281f6cd Mon Sep 17 00:00:00 2001 From: ruizanthony Date: Fri, 21 Aug 2026 01:36:39 +0200 Subject: [PATCH] fix(workspace): new chat inherits the current conversation's workspace (#7180) * fix(workspace): new chat inherits the current conversation's workspace `newSession()` resolved the workspace of a new chat as: switchWs || S._profileDefaultWorkspace || S.session.workspace `S._profileDefaultWorkspace` is global to the profile, hydrated from `/api/profile/active`. Preferring it over the current conversation meant a new chat opened from a MES conversation landed on the profile-global value instead of MES. With several conversations running in parallel on different workspaces, the context the user was actually in was systematically lost. The precedence is worse than it looks, because that "profile default" is not a stable configured value: `get_profile_default_workspace()` reads `last_workspace.txt` BEFORE the configured key (api/workspace.py ~419-427), 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` of ANY conversation. So "profile default wins over the current session" degrades in practice to "whatever workspace another conversation last used wins over the conversation I am in" -- observed live: the pointer flipped mid-operation because a concurrent turn wrote to it. New precedence: switchWs || S.session.workspace || S._profileDefaultWorkspace Preserved invariants: - the one-shot profile-switch flag still wins first and is still consumed (#823); - a blank page (no loaded session) still falls back to the profile default, which remains required for the empty-composer workspace chip (#804/#5169); - `_profileDefaultWorkspace` is never nulled by `newSession()` (#823). This deliberately diverges from upstream #4755, which made the profile default win. That decision assumed a stable configured default; on a shared/global `last_workspace.txt` it inverts its own intent. The upstream test is rewritten with the rationale inline so a future rebase does not silently restore the bug, plus a new test that evaluates the real expression under node and proves the blank-page fallback still holds. Pre-existing failures in tests/test_workspace_stale_recovery.py (3) were verified identical with and without this change. * fix(workspace): recover stale inherited workspace --------- Co-authored-by: Anthony Ruiz --- api/routes.py | 30 +++- static/sessions.js | 13 +- ...est_issue4755_profile_default_workspace.py | 78 ++++++++- tests/test_workspace_stale_recovery.py | 163 ++++++++++++++++++ 4 files changed, 276 insertions(+), 8 deletions(-) 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"