From 026e3e84ea8d652734975dc661f54fab8438a6e9 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 04:00:55 -0700 Subject: [PATCH] fix: reject unavailable desktop profile and session targets --- .../desktop_bug_campaign/persistence_live.py | 24 ++++++++++- tests/test_tui_gateway_server.py | 10 ++++- .../test_profile_rebuild_commit.py | 10 +++++ .../test_profile_target_unavailable.py | 40 +++++++++++++++++++ tui_gateway/methods_tools.py | 6 ++- tui_gateway/server.py | 13 +++--- .../docs/developer-guide/session-storage.md | 4 ++ 7 files changed, 95 insertions(+), 12 deletions(-) create mode 100644 tests/tui_gateway/test_profile_target_unavailable.py diff --git a/evals/desktop_bug_campaign/persistence_live.py b/evals/desktop_bug_campaign/persistence_live.py index d372a48c61..037f09b72f 100644 --- a/evals/desktop_bug_campaign/persistence_live.py +++ b/evals/desktop_bug_campaign/persistence_live.py @@ -18,6 +18,7 @@ def main(): parser.add_argument('--repo', type=Path, required=True) parser.add_argument('--out', type=Path, required=True) parser.add_argument('--port', type=int, default=18020) + parser.add_argument('--invalid-targets', action='store_true') parser.add_argument('--compress', action='store_true') parser.add_argument('--reset', action='store_true') parser.add_argument('--failure', action='store_true', help='inject one model-config preparation failure') @@ -94,7 +95,7 @@ def main(): receipt.write(json.dumps(row) + '\n') print(str(row)[:250], flush=True) return row - def rpc(method, params): + def rpc(method, params, allow_error=False): nonlocal counter counter += 1 ws.send(json.dumps({'jsonrpc': '2.0', 'id': counter, 'method': method, 'params': params})) @@ -102,6 +103,8 @@ def main(): row = receive() if row.get('id') == counter: if 'error' in row: + if allow_error: + return row raise RuntimeError(row) return row['result'] def turn(sid, text): @@ -137,7 +140,9 @@ def main(): raise result['expected_failure'] = str(exc) result['launch_config_unchanged'] = (home / 'config.yaml').read_bytes() == launch_before - result['worker_tools'] = yaml.safe_load((profile / 'config.yaml').read_text())['platform_toolsets']['cli'] + sys.path.insert(0, str(args.repo)) + from hermes_cli.config import read_user_config_raw + result['worker_tools'] = read_user_config_raw(profile / 'config.yaml')['platform_toolsets']['cli'] else: (profile / 'SOUL.md').write_text('Capabilities changed for the worker.\n') if observe and args.reset: @@ -150,6 +155,16 @@ def main(): with sqlite3.connect(p / 'state.db') as db: result['stores'][name] = db.execute('SELECT role, content, active FROM messages WHERE session_id=? ORDER BY id', (key,)).fetchall() result['wrong_profile_writes'] = any('PERSISTENCE_AFTER_REBUILD' in str(row) for row in result['stores']['launch']) + if args.invalid_targets: + result['target_controls'] = {str(p): rpc('session.list', {'profile': p}) for p in (None, 'default', 'worker')} + rpc('session.close', {'session_id': sid}) + before = (home / 'config.yaml').read_bytes() + result['stale_session'] = rpc('tools.configure', {'session_id': sid, 'action': 'enable', 'names': ['terminal']}, allow_error=True) + profile.rename(home / 'removed-worker') + result['invalid_profiles'] = {name: {method: rpc(method, dict(params, profile=name), allow_error=True) for method, params in [('session.list', {}), ('config.get', {'key': 'full'}), ('config.set', {'key': 'busy', 'value': 'steer'})]} for name in ('worker', 'unknown')} + result['invalid_config_unchanged'] = before == (home / 'config.yaml').read_bytes() + result['global_tools'] = rpc('tools.configure', {'action': 'enable', 'names': ['terminal']}) + result['global_config_changed'] = before != (home / 'config.yaml').read_bytes() if observe: rpc('session.close', {'session_id': sid}) result['ownership_after_close'] = rpc('probe.ownership', {'session_id': sid}) @@ -169,6 +184,11 @@ def main(): print(json.dumps({k: result[k] for k in ('sha', 'home', 'wrong_profile_writes', 'duplicate_active_groups') if k in result}, indent=2)) assert not result['wrong_profile_writes'], 'rebuild wrote the next turn to launch state.db' assert any('PERSISTENCE_AFTER_REBUILD' in str(row) for row in result['stores']['worker']) + if args.invalid_targets: + assert 'error' in result['stale_session'], 'stale session changed launch config' + assert all('error' in reply for replies in result['invalid_profiles'].values() for reply in replies.values()) + assert result['invalid_config_unchanged'] + assert result['global_config_changed'] and not result['global_tools']['reset'] if args.reset: assert result['launch_config_unchanged'], 'tools.configure changed launch config' assert 'web' in result['worker_tools'], 'tools.configure did not change worker config' diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 07ee399e92..0fca69f3e8 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -15451,6 +15451,7 @@ def test_session_branch_writes_to_parent_profile_db(monkeypatch, tmp_path): """session.branch must copy history into the parent's profile state.db.""" profile_home = tmp_path / "profiles" / "mlperf" profile_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) seen: dict = {"msgs": []} class LaunchDB: @@ -15889,6 +15890,7 @@ def test_session_branch_installs_parent_profile_secret_scope(monkeypatch, tmp_pa profile_home = tmp_path / "profiles" / "mlperf" profile_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) (profile_home / ".env").write_text( "PROXMOX_TOKEN=mlperf-secret\n", encoding="utf-8" ) @@ -15981,6 +15983,7 @@ def test_session_branch_uses_persisted_display_history_after_compaction(monkeypa """A live branch must copy the complete visible transcript, not the compacted model tail.""" profile_home = tmp_path / "profiles" / "mlperf" profile_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) seen: dict = {"msgs": []} display_history = [ @@ -20663,7 +20666,7 @@ def test_prompt_submit_passes_persist_user_message_to_agent(monkeypatch): server._sessions.pop("sid", None) -def test_prompt_submit_releases_old_history_before_heap_trim(monkeypatch): +def test_prompt_submit_releases_old_history_before_heap_trim(monkeypatch, tmp_path): """The trim boundary must not retain the just-pruned history snapshots.""" observed = {} cleanup_order = [] @@ -20702,7 +20705,10 @@ def test_prompt_submit_releases_old_history_before_heap_trim(monkeypatch): observed["run_kwargs"] = caller_locals.get("run_kwargs") session = _session(agent=_Agent()) - session["profile_home"] = "/tmp/test-profile" + profile_home = tmp_path / "profiles" / "worker" + profile_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + session["profile_home"] = str(profile_home) session["history"] = [ {"role": "tool", "tool_call_id": "old", "content": "x" * 20_000} ] diff --git a/tests/tui_gateway/test_profile_rebuild_commit.py b/tests/tui_gateway/test_profile_rebuild_commit.py index ff8fb98727..2825f1f375 100644 --- a/tests/tui_gateway/test_profile_rebuild_commit.py +++ b/tests/tui_gateway/test_profile_rebuild_commit.py @@ -32,6 +32,16 @@ def test_tools_configure_uses_live_session_profile(tmp_path, monkeypatch, explic assert "terminal" not in yaml.safe_load((profile / "config.yaml").read_text())["platform_toolsets"]["cli"] assert seen == [profile] assert get_hermes_home() == home + worker_before = (profile / "config.yaml").read_bytes() + monkeypatch.delitem(server._sessions, "profile-tools") + response = server._methods["tools.configure"](2, params) + assert response["error"]["code"] == 4001 + assert (home / "config.yaml").read_bytes() == launch_before + assert (profile / "config.yaml").read_bytes() == worker_before + response = server._methods["tools.configure"](3, {"action": "disable", "names": ["terminal"]}) + assert "error" not in response and not response["result"]["reset"] + assert (home / "config.yaml").read_bytes() != launch_before + assert (profile / "config.yaml").read_bytes() == worker_before @pytest.mark.parametrize("path", ["reset", "capabilities"]) diff --git a/tests/tui_gateway/test_profile_target_unavailable.py b/tests/tui_gateway/test_profile_target_unavailable.py new file mode 100644 index 0000000000..08509f488a --- /dev/null +++ b/tests/tui_gateway/test_profile_target_unavailable.py @@ -0,0 +1,40 @@ +"""An unavailable explicit target must never become the launch profile.""" +from pathlib import Path + +import pytest + + +def test_explicit_profile_target_never_falls_back(tmp_path, monkeypatch): + from tui_gateway import server + from hermes_state import SessionDB + + home = tmp_path / ".hermes" + worker = home / "profiles" / "worker" + worker.mkdir(parents=True) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr(server, "_hermes_home", home) + for path, marker in ((home, "launch"), (worker, "worker")): + (path / "config.yaml").write_text(f"terminal:\n cwd: /{marker}\n") + with SessionDB(db_path=path / "state.db") as db: + db.create_session(marker, "tui") + for name, marker in ((None, "launch"), ("default", "launch"), ("DEFAULT", "launch"), ("worker", "worker")): + with server._profile_db({"profile": name}) as db: + assert db.get_session(marker) + response = server._methods["config.get"](1, {"profile": name, "key": "full"}) + assert response["result"]["config"]["terminal"]["cwd"] == f"/{marker}" + before = (home / "config.yaml").read_bytes() + worker.rename(worker.with_name("gone")) + for name in ("worker", "unknown"): + with pytest.raises(FileNotFoundError): + with server._profile_db({"profile": name}): + pytest.fail("unavailable profile reached a database") + with pytest.raises(FileNotFoundError): + server._methods["config.set"](2, {"profile": name, "key": "busy", "value": "steer"}) + assert (home / "config.yaml").read_bytes() == before + # A real resolution I/O failure must propagate, too (no predicate patch). + profiles = home / "profiles" + profiles.rename(home / "saved-profiles") + profiles.symlink_to("profiles") + with pytest.raises((OSError, RuntimeError)): + server._profile_home("worker") diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index 31a10dabd9..ed57a1f51e 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -981,7 +981,11 @@ def _(rid, params: dict) -> dict: @_rpc("tools.configure", 5035) def _(rid, params: dict) -> dict: sid = params.get("session_id", "") - session = _sessions.get(sid) + session = None + if sid: + session, err = _sess_nowait(params, rid) + if err: + return err # The client sends session_id, not profile; the live session is authoritative. home = (session or {}).get("profile_home") scopes = _bind_build_profile_scopes(home) if home else None diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 39f8043916..60fd932aa2 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -460,13 +460,12 @@ def _profile_home(profile: str | None) -> Path | None: """Resolve a named profile's home on THIS host, or None for the launch profile.""" if not (name := (profile or "").strip()): return None - try: - from hermes_cli import profiles as profiles_mod - home = Path(profiles_mod.get_profile_dir(name)) - except Exception: - return None - if home.resolve() == Path(_hermes_home).resolve() or not home.exists(): - return None # already the launch profile (no override needed), or no such profile + from hermes_cli import profiles as profiles_mod + home = Path(profiles_mod.get_profile_dir(name)) + if not home.is_dir(): + raise FileNotFoundError(f"Profile '{name}' does not exist.") + if home.resolve() == Path(_hermes_home).resolve(): + return None # already the launch profile (no override needed) _served_profile_homes.add(home) # the change watcher must stat every served sibling store too return home diff --git a/website/docs/developer-guide/session-storage.md b/website/docs/developer-guide/session-storage.md index 9fb7da80f7..5329d862b6 100644 --- a/website/docs/developer-guide/session-storage.md +++ b/website/docs/developer-guide/session-storage.md @@ -17,6 +17,10 @@ Releasing the outgoing agent must not close the handle inherited by its replacem even when the client supplies only `session_id`. Rebuilds prepare model configuration before allocating a replacement, then install the agent and transfer ownership together; preparation failure leaves the existing agent responsible for teardown. +Explicit profiles that cannot be resolved or whose directory has disappeared fail +before accessing launch configuration or history. A stale `tools.configure` +session ID likewise returns `session not found` without changing configuration; +omitting the session ID still supports the global settings operation. In-place compaction archives old rows with `active=0` and inserts the retained context as `active=1` rows. A protected message can therefore legitimately appear