From 5f638a16944815e42b63675624ed4bb66129ec8a Mon Sep 17 00:00:00 2001 From: Rui Xue Date: Mon, 13 Jul 2026 17:58:05 +0000 Subject: [PATCH] feat(sessions): honor the agent's worktree config default in /api/session/new /api/session/new treated an absent 'worktree' key as false, so the agent's config-level 'worktree: true' default was never consulted for WebUI sessions even though the WebUI reuses the agent's own worktree helper. Both callers should agree on same-agent/same-repo isolation. Three-value model: - body 'worktree' absent -> agent config default applies (profile-aware via get_config_for_profile_home, so a user with worktree:true in one profile but not another gets per-profile behavior) - explicit true/false -> honored verbatim; explicit always wins Non-git workspaces: a config-default request degrades to a plain session and surfaces 'worktree_skipped' in the payload; an explicit worktree:true keeps the hard 400. Frontend (same change, load-bearing): newSession() forwards explicit true/false and omits the key otherwise; the boot-time auto-bind (_maybeBindFreshDefaultWorkspaceSession) and the onboarding session send worktree:false explicitly so a config default can never mint a worktree + branch from merely opening the UI. Deliberate 'New Chat' sends nothing and inherits the default. Tests: route matrix (config on/off x body absent/true/false), non-git fallback vs explicit 400, explicit-false-beats-config-true, config helper (ambient/named-profile/fail-soft), static UI assertions that auto-bind and onboarding opt out and New Chat paths don't pin a value. Fixes #6022 --- api/routes.py | 66 ++++- static/boot.js | 5 +- static/onboarding.js | 4 +- static/sessions.js | 6 +- tests/test_issue1955_worktree_ui_static.py | 4 +- ...sue4877_fresh_blank_boot_workspace_bind.py | 5 +- .../test_issue6022_worktree_config_default.py | 232 ++++++++++++++++++ tests/test_issue6022_worktree_ui_static.py | 55 +++++ 8 files changed, 366 insertions(+), 11 deletions(-) create mode 100644 tests/test_issue6022_worktree_config_default.py create mode 100644 tests/test_issue6022_worktree_ui_static.py diff --git a/api/routes.py b/api/routes.py index 511af492..72f6a921 100644 --- a/api/routes.py +++ b/api/routes.py @@ -7574,6 +7574,35 @@ def _rescale_threshold_tokens_for_context_window( return max(1, int(threshold * new_window / old_window)) +def _worktree_default_from_config(profile: str | None) -> bool: + """Return the agent's config-level ``worktree:`` default for *profile*. + + The agent CLI honors ``worktree: true`` in config.yaml for every session + it creates (``use_worktree = worktree or w or CLI_CONFIG.get("worktree", + False)``). /api/session/new consults this only when the request body has + no explicit ``worktree`` key, so both entry points to the same repo agree + on isolation (#6022). Explicit body values always win. + + Profile-aware on purpose: the WebUI serves multiple profiles from one + process, and a user with ``worktree: true`` in one profile but not another + expects per-profile behavior. ``get_config_for_profile_home`` handles the + ambient/common case via the mtime-tracked cache and reads a diverging + profile's config.yaml directly off disk (see #3294). + """ + try: + if profile: + from api.profiles import get_hermes_home_for_profile + + cfg_dict = get_config_for_profile_home(get_hermes_home_for_profile(profile)) + else: + cfg_dict = get_config_for_profile_home(None) + return bool((cfg_dict or {}).get("worktree", False)) + except Exception: + # Config resolution must never break session creation. + logger.warning("failed to read worktree config default", exc_info=True) + return False + + def _session_model_state_from_request( model: str | None, requested_provider: str | None, @@ -13877,10 +13906,22 @@ def handle_post(handler, parsed) -> bool: except (TypeError, ValueError) as e: return bad(handler, str(e)) worktree_info = None - worktree_requested = ( - body.get("worktree") is True - or str(body.get("worktree")).strip().lower() in {"1", "true", "yes", "on"} - ) + worktree_skipped = None + # Three-value worktree model (#6022): an explicit body value always + # wins; an ABSENT key falls back to the agent's config-level + # ``worktree:`` default so WebUI sessions and CLI sessions agree on + # isolation for the same repo. Clients that must never create a + # worktree (e.g. the boot-time auto-bind) send ``worktree: false`` + # explicitly. + raw_worktree = body.get("worktree") + worktree_explicit = raw_worktree is not None + if worktree_explicit: + worktree_requested = ( + raw_worktree is True + or str(raw_worktree).strip().lower() in {"1", "true", "yes", "on"} + ) + else: + worktree_requested = _worktree_default_from_config(body.get("profile") or None) if worktree_requested: try: from api.worktrees import create_worktree_for_workspace @@ -13890,7 +13931,15 @@ def handle_post(handler, parsed) -> bool: worktree_info = create_worktree_for_workspace(base_workspace) workspace = worktree_info["path"] except (TypeError, ValueError) as e: - return bad(handler, str(e), status=400) + # Explicit requests keep the hard failure. A config-default + # request on a non-git workspace (create_worktree_for_workspace + # raises ValueError) degrades to a plain session instead — + # otherwise `worktree: true` in config.yaml would 400 every + # session in every non-git directory. + if worktree_explicit: + return bad(handler, str(e), status=400) + worktree_info = None + worktree_skipped = str(e) except Exception as e: logger.exception("failed to create worktree-backed session") return bad(handler, f"Failed to create worktree: {e}", status=500) @@ -13973,7 +14022,12 @@ def handle_post(handler, parsed) -> bool: profile=getattr(s, "profile", None), session_id=getattr(s, "session_id", None), ) - return j(handler, {"session": s.compact() | {"messages": s.messages}}) + payload = {"session": s.compact() | {"messages": s.messages}} + if worktree_skipped: + # Config-default worktree was skipped (non-git workspace); tell the + # client the session is plain so the UI doesn't assume isolation. + payload["worktree_skipped"] = worktree_skipped + return j(handler, payload) if parsed.path == "/api/session/compression-recovery/start": return _handle_session_compression_recovery_start(handler, body) diff --git a/static/boot.js b/static/boot.js index ac5d4fdd..0a8f7774 100644 --- a/static/boot.js +++ b/static/boot.js @@ -303,7 +303,10 @@ async function _maybeBindFreshDefaultWorkspaceSession(prefillIntent=null){ if(_workspacePanelMode!=='browse') return false; if(!S._profileDefaultWorkspace) return false; try{ - await newSession(false, {awaitWorkspaceLoad: true}); + // worktree:false is explicit and load-bearing — this auto-bind runs on + // page load, and a config-level worktree default must never leak a fresh + // worktree + branch from simply opening the UI (#6022). + await newSession(false, {awaitWorkspaceLoad: true, worktree: false}); return true; }catch(e){ console.warn('[hermes] failed to bind fresh default workspace session', e); diff --git a/static/onboarding.js b/static/onboarding.js index 9a43e252..5ed3574f 100644 --- a/static/onboarding.js +++ b/static/onboarding.js @@ -527,7 +527,9 @@ async function _finishOnboarding(){ await loadWorkspaceList(); if(typeof renderSessionList==='function') await renderSessionList(); if(!S.session && typeof newSession==='function'){ - await newSession(true); + // System-minted session (no deliberate New Chat intent) — like the boot + // auto-bind, it must not inherit a config-level worktree default (#6022). + await newSession(true, {worktree: false}); await renderSessionList(); } } diff --git a/static/sessions.js b/static/sessions.js index c485185e..48830f4c 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -1362,7 +1362,11 @@ async function newSession(flash, options={}){ profile:S.activeProfile||'default', }; if(S.session&&S.session.session_id) reqBody.prev_session_id=S.session.session_id; - if(options&&options.worktree) reqBody.worktree=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 + // config default can never mint a worktree (+ branch) on mere page load. + if(options&&Object.prototype.hasOwnProperty.call(options,'worktree')) reqBody.worktree=!!options.worktree; if(Object.prototype.hasOwnProperty.call(options,'project_id')){ reqBody.project_id=options.project_id; } else if(_activeProject&&_activeProject!==NO_PROJECT_FILTER){ diff --git a/tests/test_issue1955_worktree_ui_static.py b/tests/test_issue1955_worktree_ui_static.py index d160d5a8..83c01a61 100644 --- a/tests/test_issue1955_worktree_ui_static.py +++ b/tests/test_issue1955_worktree_ui_static.py @@ -18,7 +18,9 @@ def test_session_new_route_accepts_worktree_flag_and_uses_worktree_info(): def test_new_session_request_can_include_worktree_flag(): src = read("static/sessions.js") assert "async function newSession(flash, options={})" in src - assert "reqBody.worktree=true" in src + # Three-value contract (#6022): explicit true/false forwarded verbatim, + # absent key omitted so the server applies the config default. + assert "reqBody.worktree=!!options.worktree" in src def test_workspace_dropdown_exposes_new_worktree_conversation_action(): diff --git a/tests/test_issue4877_fresh_blank_boot_workspace_bind.py b/tests/test_issue4877_fresh_blank_boot_workspace_bind.py index e15b0bee..6d109acb 100644 --- a/tests/test_issue4877_fresh_blank_boot_workspace_bind.py +++ b/tests/test_issue4877_fresh_blank_boot_workspace_bind.py @@ -116,7 +116,10 @@ def test_blank_boot_open_panel_auto_binds_default_workspace_session(): result = _helper_driver("browse", "/default-workspace") assert result["bound"] is True - assert result["calls"] == [{"arg0": False, "arg1": {"awaitWorkspaceLoad": True}}] + # worktree:false is explicit and load-bearing (#6022): the boot auto-bind + # must opt out so a config-level worktree default can't mint a worktree + # from merely opening the page. + assert result["calls"] == [{"arg0": False, "arg1": {"awaitWorkspaceLoad": True, "worktree": False}}] @node_test diff --git a/tests/test_issue6022_worktree_config_default.py b/tests/test_issue6022_worktree_config_default.py new file mode 100644 index 00000000..1006737c --- /dev/null +++ b/tests/test_issue6022_worktree_config_default.py @@ -0,0 +1,232 @@ +"""Tests for the config-level worktree default in /api/session/new (Issue #6022). + +Three-value contract: + - body ``worktree`` ABSENT -> agent config ``worktree:`` default applies + - body ``worktree`` explicit -> honored verbatim (explicit always wins) + +Config-default requests degrade to a plain session (+ ``worktree_skipped`` in +the payload) on non-git workspaces; explicit requests keep the hard 400. +""" + +from types import SimpleNamespace + +import pytest + +import api.models as models +import api.routes as routes +import api.worktrees as worktrees +from api.models import SESSIONS + + +@pytest.fixture(autouse=True) +def _isolate_sessions(tmp_path, monkeypatch): + session_dir = tmp_path / "sessions" + session_dir.mkdir() + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + monkeypatch.setenv("GIT_CEILING_DIRECTORIES", str(tmp_path.parent.resolve())) + SESSIONS.clear() + yield session_dir + SESSIONS.clear() + + +def _post_session_new(tmp_path, monkeypatch, body, *, config_default, workspace_dir=None, fake_worktree=None): + """Drive POST /api/session/new with a stubbed transport and worktree factory.""" + monkeypatch.setattr(routes, "_check_csrf", lambda handler: True) + monkeypatch.setattr(routes, "read_body", lambda handler: body) + monkeypatch.setattr( + routes, "_worktree_default_from_config", lambda profile: config_default + ) + if workspace_dir is not None: + monkeypatch.setattr( + routes, "resolve_trusted_workspace", lambda raw: workspace_dir + ) + if fake_worktree is not None: + monkeypatch.setattr( + worktrees, "create_worktree_for_workspace", lambda workspace: fake_worktree + ) + captured = {} + monkeypatch.setattr( + routes, + "j", + lambda handler, payload, status=200, extra_headers=None: captured.update( + payload=payload, status=status + ) + or True, + ) + import api.helpers as helpers + + monkeypatch.setattr( + helpers, + "j", + lambda handler, payload, status=200, extra_headers=None: captured.update( + payload=payload, status=status + ) + or True, + ) + assert routes.handle_post(object(), SimpleNamespace(path="/api/session/new")) is True + return captured + + +def _fake_worktree_info(repo, worktree): + return { + "path": str(worktree), + "branch": "hermes/hermes-6022", + "repo_root": str(repo), + "created_at": 321.0, + } + + +def _mk_repo_dirs(tmp_path): + repo = tmp_path / "repo" + worktree = repo / ".worktrees" / "hermes-6022" + repo.mkdir() + worktree.mkdir(parents=True) + return repo, worktree + + +# ── Route matrix: absent key ───────────────────────────────────────────────── + + +def test_absent_key_with_config_default_off_creates_plain_session(tmp_path, monkeypatch): + repo, _ = _mk_repo_dirs(tmp_path) + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(repo), "profile": "default"}, + config_default=False, + workspace_dir=repo, + ) + assert captured["status"] == 200 + session = captured["payload"]["session"] + assert session.get("worktree_path") in (None, "") + assert "worktree_skipped" not in captured["payload"] + + +def test_absent_key_with_config_default_on_creates_worktree_session(tmp_path, monkeypatch): + repo, worktree = _mk_repo_dirs(tmp_path) + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(repo), "profile": "default"}, + config_default=True, + workspace_dir=repo, + fake_worktree=_fake_worktree_info(repo, worktree), + ) + assert captured["status"] == 200 + session = captured["payload"]["session"] + assert session["workspace"] == str(worktree.resolve()) + assert session["worktree_path"] == str(worktree.resolve()) + assert session["worktree_branch"] == "hermes/hermes-6022" + + +# ── Route matrix: explicit wins ────────────────────────────────────────────── + + +def test_explicit_false_beats_config_default_true(tmp_path, monkeypatch): + repo, worktree = _mk_repo_dirs(tmp_path) + + def _fail(workspace): + pytest.fail("worktree must not be created when body says worktree=false") + + monkeypatch.setattr(worktrees, "create_worktree_for_workspace", _fail) + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(repo), "profile": "default", "worktree": False}, + config_default=True, + workspace_dir=repo, + ) + assert captured["status"] == 200 + session = captured["payload"]["session"] + assert session.get("worktree_path") in (None, "") + + +def test_explicit_true_with_config_default_off_creates_worktree(tmp_path, monkeypatch): + repo, worktree = _mk_repo_dirs(tmp_path) + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(repo), "profile": "default", "worktree": True}, + config_default=False, + workspace_dir=repo, + fake_worktree=_fake_worktree_info(repo, worktree), + ) + assert captured["status"] == 200 + session = captured["payload"]["session"] + assert session["worktree_path"] == str(worktree.resolve()) + + +# ── Route matrix: non-git workspace ────────────────────────────────────────── + + +def test_config_default_on_non_git_workspace_falls_back_to_plain_session(tmp_path, monkeypatch): + non_git = tmp_path / "plain" + non_git.mkdir() + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(non_git), "profile": "default"}, + config_default=True, + workspace_dir=non_git, + # real create_worktree_for_workspace -> find_git_repo_root raises ValueError + ) + assert captured["status"] == 200 + session = captured["payload"]["session"] + assert session.get("worktree_path") in (None, "") + assert "not inside a git repository" in captured["payload"]["worktree_skipped"] + + +def test_explicit_true_on_non_git_workspace_still_hard_400(tmp_path, monkeypatch): + non_git = tmp_path / "plain" + non_git.mkdir() + captured = _post_session_new( + tmp_path, + monkeypatch, + {"workspace": str(non_git), "profile": "default", "worktree": True}, + config_default=False, + workspace_dir=non_git, + ) + assert captured["status"] == 400 + assert "not inside a git repository" in captured["payload"].get("error", "") + + +# ── Config helper ──────────────────────────────────────────────────────────── + + +def test_worktree_default_reads_ambient_config_when_no_profile(monkeypatch): + monkeypatch.setattr( + routes, "get_config_for_profile_home", lambda home: {"worktree": True} + ) + assert routes._worktree_default_from_config(None) is True + monkeypatch.setattr( + routes, "get_config_for_profile_home", lambda home: {"worktree": False} + ) + assert routes._worktree_default_from_config(None) is False + monkeypatch.setattr(routes, "get_config_for_profile_home", lambda home: {}) + assert routes._worktree_default_from_config(None) is False + + +def test_worktree_default_resolves_named_profile_home(monkeypatch, tmp_path): + import api.profiles as profiles + + seen = {} + monkeypatch.setattr( + profiles, "get_hermes_home_for_profile", lambda name: tmp_path / name + ) + + def fake_cfg(home): + seen["home"] = home + return {"worktree": True} + + monkeypatch.setattr(routes, "get_config_for_profile_home", fake_cfg) + assert routes._worktree_default_from_config("work") is True + assert seen["home"] == tmp_path / "work" + + +def test_worktree_default_is_fail_soft_on_config_errors(monkeypatch): + def boom(home): + raise RuntimeError("config unavailable") + + monkeypatch.setattr(routes, "get_config_for_profile_home", boom) + assert routes._worktree_default_from_config(None) is False diff --git a/tests/test_issue6022_worktree_ui_static.py b/tests/test_issue6022_worktree_ui_static.py new file mode 100644 index 00000000..e57b2f81 --- /dev/null +++ b/tests/test_issue6022_worktree_ui_static.py @@ -0,0 +1,55 @@ +"""Static assertions for the frontend half of Issue #6022. + +The three-value worktree contract only holds if system-minted sessions +(boot-time auto-bind, onboarding) explicitly opt OUT — otherwise a config +``worktree: true`` default would leak a fresh worktree + branch on every page +load, with nothing to reap it (#6023). +""" + +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] + + +def read(path): + return (ROOT / path).read_text(encoding="utf-8") + + +def test_new_session_forwards_explicit_worktree_and_omits_absent(): + src = read("static/sessions.js") + # Explicit true/false forwarded verbatim; absent key stays absent so the + # server can apply the config default. + assert ( + "if(options&&Object.prototype.hasOwnProperty.call(options,'worktree')) " + "reqBody.worktree=!!options.worktree;" in src + ) + # The old shape (only-true is ever sent) must be gone. + assert "if(options&&options.worktree) reqBody.worktree=true;" not in src + + +def test_boot_auto_bind_sends_explicit_worktree_false(): + src = read("static/boot.js") + bind = src[src.index("async function _maybeBindFreshDefaultWorkspaceSession") :] + bind = bind[: bind.index("\n}\n")] + assert "worktree: false" in bind + + +def test_onboarding_session_sends_explicit_worktree_false(): + src = read("static/onboarding.js") + finish = src[src.index("async function _finishOnboarding") :] + finish = finish[: finish.index("\n}\n")] + assert "worktree: false" in finish + + +def test_deliberate_new_chat_paths_do_not_pin_worktree(): + # Sidebar "New Chat" and command paths must NOT pass an explicit worktree + # value — they inherit the server-side config default by design. + boot = read("static/boot.js") + for line_no in ( + i + for i, line in enumerate(boot.splitlines(), 1) + if "await newSession();await renderSessionList();closeMobileSidebar();" in line + ): + line = boot.splitlines()[line_no - 1] + assert "worktree" not in line