From 4bb294181ca90efad1409a09fdf760c8f3169c00 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 7 Oct 2026 22:26:46 +0200 Subject: [PATCH 1/2] feat(eval): add --keep-conversations flag A conversation is deleted as soon as its item finishes, so the AI Interaction Intelligence endpoints -- GET .../conversations/{id}/steps and .../items -- have nothing to read once a run ends. --keep-conversations keeps every conversation, passed or failed, so a diagnostic batch can be inspected afterwards. Off by default; also settable via GOODDATA_EVAL_KEEP_CONVERSATIONS. Enforced inside ChatClient.delete_conversation rather than at each call site. The thirteen agentic evaluators build their own clients deep in the call tree and clean up by hand in their finally blocks -- twenty call sites that never pass through ask() -- so a constructor kwarg would have reached the single-turn path only. set_keep_conversations follows set_default_turn_timeout, which exists for the same reason, but applies at deletion rather than construction so a client that already exists honours it too. That also closes a gap in --preserve-failed, which has only ever applied to the single-turn path: every agentic kind deletes unconditionally today, so a failed agentic conversation could not be inspected even with the flag set. run_agentic_conversation was written before --preserve-failed landed and was never wired into it. Two structural guards: no agentic module may issue its own DELETE, and the modules must still clean up by default. The first is discovered by scanning the package, so a fourteenth kind is covered the day it lands. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/cli/main.py | 22 ++++- .../src/gooddata_eval/core/chat/sse_client.py | 30 ++++++ .../src/gooddata_eval/core/config.py | 3 + .../tests/test_agentic_runner.py | 36 +++++++ packages/gooddata-eval/tests/test_cli.py | 57 +++++++++++ .../gooddata-eval/tests/test_sse_client.py | 96 +++++++++++++++++++ 6 files changed, 242 insertions(+), 2 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index a724a8bac..91fad9538 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -15,7 +15,12 @@ from rich.table import Table from gooddata_eval.cli.agentic_runner import AGENTIC_TEST_KINDS, UNGATED_AGENTIC_TEST_KINDS, run_agentic_items -from gooddata_eval.core.chat.sse_client import ChatClient, set_default_item_timeout, set_default_turn_timeout +from gooddata_eval.core.chat.sse_client import ( + ChatClient, + set_default_item_timeout, + set_default_turn_timeout, + set_keep_conversations, +) from gooddata_eval.core.config import ( DEFAULT_GATE, DEFAULT_JUDGE_MODEL, @@ -169,7 +174,17 @@ def _build_parser() -> argparse.ArgumentParser: "--preserve-failed", action="store_true", dest="preserve_failed", - help="Keep failed conversations on the server for post-mortem inspection.", + help="Keep failed conversations on the server for post-mortem inspection. Single-turn " + "kinds only; use --keep-conversations to cover the agentic ones.", + ) + run.add_argument( + "--keep-conversations", + action="store_true", + dest="keep_conversations", + help="Keep every conversation on the server, passed or failed, so the AI Interaction " + "Intelligence endpoints can be queried after the run (or set " + "GOODDATA_EVAL_KEEP_CONVERSATIONS=1). Covers the agentic kinds, which --preserve-failed " + "does not. Leaves state behind: only for a diagnostic run.", ) run.add_argument( "--reasoning-effort", @@ -485,6 +500,8 @@ def _run(config: RunConfig) -> int: # Applies to the agentic evaluators' own clients too, which this function never sees. set_default_turn_timeout(config.turn_timeout_s) set_default_item_timeout(config.item_timeout_s) + if config.keep_conversations: + set_keep_conversations(True) if config.log_to_langfuse and config.langfuse_dataset is None: print( "error: --langfuse requires --langfuse-dataset (local datasets have no Langfuse item ids to link to).", @@ -723,6 +740,7 @@ def main(argv: list[str] | None = None) -> int: quiet=args.quiet, kind=args.kind, preserve_failed=args.preserve_failed, + keep_conversations=args.keep_conversations, reasoning_effort=args.reasoning_effort, gate=normalize_gate(args.gate), agent_id=args.agent_id or os.environ.get("GD_EVAL_AGENT_ID"), diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index e086db8ce..21ae2ac83 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -119,6 +119,16 @@ def _float_env(name: str, default: float) -> float: return float(raw) if raw else default +def _bool_env(name: str, default: bool) -> bool: + """Read a flag from the environment, falling back to ``default`` when unset or blank. + + ``0``/``false``/``no``/``off`` are false, anything else present is true -- so the usual + ``VAR=0`` turns a flag off rather than reading as a non-empty, therefore true, string. + """ + raw = os.getenv(name) + return raw.strip().lower() not in ("0", "false", "no", "off") if raw and raw.strip() else default + + # Retry budget defaults, giving a ~2 min worst-case cap per send (5/10/20/40/60s). # Each is overridable via env so CI can retune without cutting a new release -- # read per call rather than at import, so an exported value cannot silently @@ -148,6 +158,13 @@ def _float_env(name: str, default: float) -> float: _TRACE_LABELS_ENV = "GOODDATA_EVAL_TRACE_LABELS" _TRACE_LABEL_KEY = re.compile(r"[A-Za-z0-9_-]+") +# When set, no conversation is deleted -- pass or fail, single-turn or agentic. The +# server-side record is what the Interaction Intelligence endpoints read, so a run meant to +# be inspected afterwards has to leave it behind. Enforced inside delete_conversation rather +# than at each call site: thirteen agentic evaluators call it twenty times between them, and +# a fourteenth kind would have been one more place to remember. +_KEEP_CONVERSATIONS = _bool_env("GOODDATA_EVAL_KEEP_CONVERSATIONS", False) + @functools.cache def _warn_skipped_trace_label(key: str) -> None: @@ -194,6 +211,17 @@ def set_default_item_timeout(seconds: float | None) -> None: _ITEM_TIMEOUT_S = seconds or 0.0 +def set_keep_conversations(keep: bool) -> None: + """Keep every conversation this process creates, whatever its outcome. + + Lands here for the same reason the timeout defaults do -- the agentic evaluators build + their own clients deep in the call tree -- but takes effect at deletion rather than at + construction, so a client already built honours it too. + """ + global _KEEP_CONVERSATIONS + _KEEP_CONVERSATIONS = keep + + T = TypeVar("T") @@ -550,6 +578,8 @@ def _do() -> str: return conversation_id def delete_conversation(self, conversation_id: str) -> None: + if _KEEP_CONVERSATIONS: + return try: self._client.delete(f"{self._base}/{conversation_id}", headers=self._auth) except httpx.HTTPError: diff --git a/packages/gooddata-eval/src/gooddata_eval/core/config.py b/packages/gooddata-eval/src/gooddata_eval/core/config.py index b665b74be..db4223c1f 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/config.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/config.py @@ -90,6 +90,9 @@ class RunConfig: quiet: bool = False kind: str = "visualization" preserve_failed: bool = False + keep_conversations: bool = False + """Keep every conversation, pass or fail. Superset of ``preserve_failed``; applies to the + agentic kinds too, which ``preserve_failed`` never reached.""" reasoning_effort: ReasoningEffort | None = None agent_id: str | None = None turn_timeout_s: float | None = None diff --git a/packages/gooddata-eval/tests/test_agentic_runner.py b/packages/gooddata-eval/tests/test_agentic_runner.py index 23586b563..09c162734 100644 --- a/packages/gooddata-eval/tests/test_agentic_runner.py +++ b/packages/gooddata-eval/tests/test_agentic_runner.py @@ -1,9 +1,11 @@ # (C) 2026 GoodData Corporation. All rights reserved. # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise import importlib +import re import threading import time from concurrent.futures import ThreadPoolExecutor +from pathlib import Path from typing import Any from unittest.mock import patch @@ -855,3 +857,37 @@ def test_run_agentic_binds_the_user_context_to_its_chat_client( with patch.object(module, "ChatClient", side_effect=_Stop) as mock_client, pytest.raises(_Stop): getattr(module, run_fn)("https://h", "tok", "ws1", "q", expected, user_context=user_context) assert mock_client.call_args.kwargs["user_context"] == user_context + + +def _agentic_module_sources() -> dict[str, str]: + """Every agentic evaluator module's source, keyed by name. Discovered by scanning the + package so a new kind is covered the day it lands rather than when someone remembers.""" + package = importlib.import_module("gooddata_eval.core.agentic") + directory = Path(package.__file__).parent + return { + path.stem: path.read_text(encoding="utf-8") + for path in sorted(directory.glob("*.py")) + if not path.stem.startswith("_") + } + + +@pytest.mark.parametrize("module_name", sorted(_agentic_module_sources())) +def test_every_agentic_module_deletes_through_the_client_method(module_name: str) -> None: + """--keep-conversations is enforced inside ChatClient.delete_conversation, so a module + that issues its own DELETE would quietly ignore it. Thirteen modules delete twenty times + between them; one bypass is enough to lose the conversation a diagnostic run needed.""" + source = _agentic_module_sources()[module_name] + # Only HTTP deletes: `created.delete(sdk, workspace_id)` is workspace-object cleanup, + # which has nothing to do with conversation retention. + bypasses = [ + line.strip() for line in source.splitlines() if re.search(r"\b(httpx|requests|_client)\.delete\(", line) + ] + assert not bypasses, f"{module_name} issues its own DELETE; call client.delete_conversation instead: {bypasses}" + + +def test_the_agentic_modules_do_delete_conversations() -> None: + """Guards the guard: an assertion that only forbids a pattern still passes once the + cleanup it is protecting has been deleted outright.""" + sources = _agentic_module_sources() + deleting = {name for name, src in sources.items() if "delete_conversation(" in src} + assert len(deleting) >= 12, f"expected most agentic kinds to clean up, found {sorted(deleting)}" diff --git a/packages/gooddata-eval/tests/test_cli.py b/packages/gooddata-eval/tests/test_cli.py index 1d23293bf..77221f833 100644 --- a/packages/gooddata-eval/tests/test_cli.py +++ b/packages/gooddata-eval/tests/test_cli.py @@ -683,6 +683,63 @@ def _fake_run(items, backend, *, runs, model, workspace_id, **kw): assert captured_kwargs.get("preserve_failed") is True +def test_cli_keep_conversations_flag_applies_process_wide(monkeypatch, fixtures_dir): + """--keep-conversations lands on the module-level switch, not on the ChatClient kwargs. + + The agentic evaluators build their own clients deep in the call tree, so a constructor + kwarg would reach the single-turn path only -- which is the gap --preserve-failed has. + """ + monkeypatch.setattr(cli_main, "resolve_connection", lambda host, token, profile: ("https://h", "tok")) + applied: list[bool] = [] + + class _FakeController: + def __init__(self, *a, **k): ... + def get_active(self): + return ActiveLlmProvider(provider_id="p", default_model_id="gpt-5.2") + + def resolve_and_activate(self, requested, provider=None): + return ResolvedModel(provider_id="p", model_id="gpt-5.2", switched=False, provider_name="P") + + def restore(self, original): ... + def close(self): ... + + monkeypatch.setattr(cli_main, "WorkspaceModelController", _FakeController) + monkeypatch.setattr(cli_main, "set_keep_conversations", applied.append) + monkeypatch.setattr(cli_main, "ChatClient", lambda **kwargs: object()) + + def _fake_run(items, backend, *, runs, model, workspace_id, **kw): + return EvalReport( + model=model, + workspace_id=workspace_id, + items=[ + ItemReport(id="i1", dataset_name="d", test_kind="visualization", question="q", pass_at_k=True, runs=1) + ], + ) + + monkeypatch.setattr(cli_main, "run_items", _fake_run) + + argv = [ + "run", + "--host", + "https://h", + "--token", + "tok", + "--workspace", + "ws1", + "--dataset", + str(fixtures_dir / "sample_dataset"), + "--quiet", + ] + assert cli_main.main([*argv, "--keep-conversations"]) == 0 + assert applied == [True] + + # Absent, the switch is left alone rather than forced off: GOODDATA_EVAL_KEEP_CONVERSATIONS + # is the other way to ask for this, and a bare `run` must not overwrite it. + applied.clear() + assert cli_main.main(argv) == 0 + assert applied == [] + + def test_cli_rejects_negative_concurrency(monkeypatch, fixtures_dir): monkeypatch.setattr(cli_main, "resolve_connection", lambda host, token, profile: ("https://h", "tok")) exit_code = cli_main.main( diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index 3d5d5d574..465df0553 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -806,6 +806,102 @@ def handler(request): assert "delete" not in calls # conversation preserved +@pytest.fixture +def keep_conversations(monkeypatch): + """Turn the process-wide keep flag on for one test, and off again afterwards.""" + monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", True) + + +def _delete_recording_handler(calls, conversation_id, sse_body): + def handler(request): + if request.method == "POST" and request.url.path.endswith("/conversations"): + return httpx.Response(200, json={"conversationId": conversation_id}) + if request.method == "POST" and "messages" in str(request.url): + return httpx.Response(200, content=sse_body) + if request.method == "DELETE": + calls.append("delete") + return httpx.Response(204) + return httpx.Response(404) + + return handler + + +def test_keep_conversations_keeps_the_conversation_on_success(keep_conversations): + """A passing run is kept too -- that is the whole difference from preserve_failed.""" + calls: list[str] = [] + client = _client_with_handler(_delete_recording_handler(calls, "conv-keep-ok", _OK_SSE)) + + item = DatasetItem(id="t1", dataset_name="d", test_kind="visualization", question="q", expected_output={}) + result = client.ask(item) + + assert result.conversation_id == "conv-keep-ok" + assert "delete" not in calls + + +def test_keep_conversations_keeps_the_conversation_on_failure(monkeypatch, keep_conversations): + """Set on its own, with preserve_failed left off, it still keeps a failed conversation.""" + calls: list[str] = [] + monkeypatch.setattr(sse_mod.time, "sleep", lambda s: None) + client = _client_with_handler(_delete_recording_handler(calls, "conv-keep-fail", _NONRETRY_SSE)) + + item = DatasetItem(id="t1", dataset_name="d", test_kind="visualization", question="q", expected_output={}) + with pytest.raises(ChatError): + client.ask(item) + + assert "delete" not in calls + + +def test_keep_conversations_blocks_a_direct_delete_conversation_call(keep_conversations): + """The gate is in delete_conversation itself, which is what the agentic evaluators call. + + Each of them creates its own ChatClient deep in the call tree and deletes by hand in a + finally block -- twenty call sites that never pass through ``ask``. If this stops holding, + --keep-conversations silently covers the single-turn kinds only, which is the gap + --preserve-failed already had. + """ + calls: list[str] = [] + client = _client_with_handler(_delete_recording_handler(calls, "conv-direct", _OK_SSE)) + + client.delete_conversation("conv-direct") + + assert calls == [] + + +def test_set_keep_conversations_affects_an_already_built_client(): + """The agentic clients may already exist when the CLI applies the flag.""" + calls: list[str] = [] + client = _client_with_handler(_delete_recording_handler(calls, "conv-late", _OK_SSE)) + try: + sse_mod.set_keep_conversations(True) + client.delete_conversation("conv-late") + assert calls == [] + finally: + sse_mod.set_keep_conversations(False) + + client.delete_conversation("conv-late") + assert calls == ["delete"] + + +@pytest.mark.parametrize( + ("raw", "expected"), + [("1", True), ("true", True), ("yes", True), ("0", False), ("false", False), ("off", False), ("", False)], +) +def test_keep_conversations_env_var(monkeypatch, raw, expected): + """``GOODDATA_EVAL_KEEP_CONVERSATIONS=0`` turns it off rather than reading as truthy.""" + monkeypatch.setenv("GOODDATA_EVAL_KEEP_CONVERSATIONS", raw) + assert sse_mod._bool_env("GOODDATA_EVAL_KEEP_CONVERSATIONS", False) is expected + + +def test_conversations_are_deleted_when_the_flag_is_off(): + """The default is unchanged: a run that does not ask to keep state leaves none behind.""" + calls: list[str] = [] + client = _client_with_handler(_delete_recording_handler(calls, "conv-default", _OK_SSE)) + + client.delete_conversation("conv-default") + + assert calls == ["delete"] + + def test_ask_without_preserve_failed_deletes_on_error(monkeypatch): """Without preserve_failed, conversations are deleted even on error.""" calls = [] From 5bc7c5753c0fe5114a9d16e0daad71dce9433f6d Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 7 Oct 2026 22:44:06 +0200 Subject: [PATCH 2/2] fix(eval): scope conversation retention to the run that asked for it main() is re-entrant -- under test and as a library call -- and a bare set_keep_conversations(True) stayed set for whatever the process did next. The direction of that leak is the costly one: a later run that asked for nothing would silently leave server-side conversations behind. keep_conversations() is a context manager that restores the previous setting on every exit path, errors included. keep=False means "did not ask" rather than "delete", so an exported GOODDATA_EVAL_KEEP_CONVERSATIONS still survives a run that passes no flag. Also pin the off state explicitly in the tests that assert a DELETE happens. They read _KEEP_CONVERSATIONS as False at import, which is only true when the env var is unset -- run on their own with it exported, they failed. Co-Authored-By: Claude Opus 5 --- .../src/gooddata_eval/cli/main.py | 9 ++-- .../src/gooddata_eval/core/chat/sse_client.py | 23 +++++++++- packages/gooddata-eval/tests/test_cli.py | 20 +++++++-- .../gooddata-eval/tests/test_sse_client.py | 42 +++++++++++++++---- 4 files changed, 77 insertions(+), 17 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/main.py b/packages/gooddata-eval/src/gooddata_eval/cli/main.py index 91fad9538..415e8c83e 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/main.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/main.py @@ -17,9 +17,9 @@ from gooddata_eval.cli.agentic_runner import AGENTIC_TEST_KINDS, UNGATED_AGENTIC_TEST_KINDS, run_agentic_items from gooddata_eval.core.chat.sse_client import ( ChatClient, + keep_conversations, set_default_item_timeout, set_default_turn_timeout, - set_keep_conversations, ) from gooddata_eval.core.config import ( DEFAULT_GATE, @@ -500,8 +500,6 @@ def _run(config: RunConfig) -> int: # Applies to the agentic evaluators' own clients too, which this function never sees. set_default_turn_timeout(config.turn_timeout_s) set_default_item_timeout(config.item_timeout_s) - if config.keep_conversations: - set_keep_conversations(True) if config.log_to_langfuse and config.langfuse_dataset is None: print( "error: --langfuse requires --langfuse-dataset (local datasets have no Langfuse item ids to link to).", @@ -747,7 +745,10 @@ def main(argv: list[str] | None = None) -> int: turn_timeout_s=args.turn_timeout, item_timeout_s=args.item_timeout, ) - return _run(config) + # Scoped to this run: main() is re-entrant under test and as a library call, and a + # leaked True would leave the next run's conversations on the server unasked. + with keep_conversations(config.keep_conversations): + return _run(config) except ( ConnectionError_, ModelResolutionError, diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index 21ae2ac83..df155c8f4 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -13,6 +13,7 @@ protocol, not on this class. """ +import contextlib import functools import json import logging @@ -20,7 +21,7 @@ import re import time from dataclasses import dataclass, field -from typing import Any, Callable, Iterable, TypeVar +from typing import Any, Callable, Iterable, Iterator, TypeVar from urllib.parse import quote import httpx @@ -217,11 +218,31 @@ def set_keep_conversations(keep: bool) -> None: Lands here for the same reason the timeout defaults do -- the agentic evaluators build their own clients deep in the call tree -- but takes effect at deletion rather than at construction, so a client already built honours it too. + + Prefer ``keep_conversations`` for a single run: a bare set leaks into whatever the + process does next, and the direction of the leak is the costly one -- a later run that + asked for nothing would silently leave server-side state behind. """ global _KEEP_CONVERSATIONS _KEEP_CONVERSATIONS = keep +@contextlib.contextmanager +def keep_conversations(keep: bool) -> Iterator[None]: + """Scope conversation retention to one run, restoring the previous setting on exit. + + ``keep=False`` is not "delete": it leaves the setting alone, so an operator who exported + GOODDATA_EVAL_KEEP_CONVERSATIONS still gets it on a run that passes no flag. + """ + previous = _KEEP_CONVERSATIONS + if keep: + set_keep_conversations(True) + try: + yield + finally: + set_keep_conversations(previous) + + T = TypeVar("T") diff --git a/packages/gooddata-eval/tests/test_cli.py b/packages/gooddata-eval/tests/test_cli.py index 77221f833..c1a260846 100644 --- a/packages/gooddata-eval/tests/test_cli.py +++ b/packages/gooddata-eval/tests/test_cli.py @@ -1,4 +1,5 @@ # (C) 2026 GoodData Corporation +import contextlib import io from concurrent.futures import ThreadPoolExecutor, as_completed @@ -683,6 +684,17 @@ def _fake_run(items, backend, *, runs, model, workspace_id, **kw): assert captured_kwargs.get("preserve_failed") is True +def _recording_keep(applied: list[bool]): + """Stand-in for the keep_conversations context manager that records what it was asked for.""" + + @contextlib.contextmanager + def _cm(keep: bool): + applied.append(keep) + yield + + return _cm + + def test_cli_keep_conversations_flag_applies_process_wide(monkeypatch, fixtures_dir): """--keep-conversations lands on the module-level switch, not on the ChatClient kwargs. @@ -704,7 +716,7 @@ def restore(self, original): ... def close(self): ... monkeypatch.setattr(cli_main, "WorkspaceModelController", _FakeController) - monkeypatch.setattr(cli_main, "set_keep_conversations", applied.append) + monkeypatch.setattr(cli_main, "keep_conversations", _recording_keep(applied)) monkeypatch.setattr(cli_main, "ChatClient", lambda **kwargs: object()) def _fake_run(items, backend, *, runs, model, workspace_id, **kw): @@ -733,11 +745,11 @@ def _fake_run(items, backend, *, runs, model, workspace_id, **kw): assert cli_main.main([*argv, "--keep-conversations"]) == 0 assert applied == [True] - # Absent, the switch is left alone rather than forced off: GOODDATA_EVAL_KEEP_CONVERSATIONS - # is the other way to ask for this, and a bare `run` must not overwrite it. + # Scoped per run, so a bare `run` afterwards asks for nothing. keep=False leaves an + # env-set GOODDATA_EVAL_KEEP_CONVERSATIONS alone rather than forcing it off. applied.clear() assert cli_main.main(argv) == 0 - assert applied == [] + assert applied == [False] def test_cli_rejects_negative_concurrency(monkeypatch, fixtures_dir): diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index 465df0553..c10e4904c 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -867,21 +867,44 @@ def test_keep_conversations_blocks_a_direct_delete_conversation_call(keep_conver assert calls == [] -def test_set_keep_conversations_affects_an_already_built_client(): +def test_set_keep_conversations_affects_an_already_built_client(monkeypatch): """The agentic clients may already exist when the CLI applies the flag.""" calls: list[str] = [] + monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", False) client = _client_with_handler(_delete_recording_handler(calls, "conv-late", _OK_SSE)) - try: - sse_mod.set_keep_conversations(True) - client.delete_conversation("conv-late") - assert calls == [] - finally: - sse_mod.set_keep_conversations(False) + sse_mod.set_keep_conversations(True) + client.delete_conversation("conv-late") + assert calls == [] + + sse_mod.set_keep_conversations(False) client.delete_conversation("conv-late") assert calls == ["delete"] +def test_keep_conversations_restores_the_previous_setting(monkeypatch): + """One run asking to keep must not leave the next run keeping too.""" + monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", False) + + with sse_mod.keep_conversations(True): + assert sse_mod._KEEP_CONVERSATIONS is True + assert sse_mod._KEEP_CONVERSATIONS is False + + with pytest.raises(RuntimeError), sse_mod.keep_conversations(True): + raise RuntimeError("run blew up") + assert sse_mod._KEEP_CONVERSATIONS is False + + +def test_keep_conversations_false_does_not_clear_an_env_set_flag(monkeypatch): + """``keep=False`` means "did not ask", not "delete" -- an exported env var survives a + run that passes no flag.""" + monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", True) + + with sse_mod.keep_conversations(False): + assert sse_mod._KEEP_CONVERSATIONS is True + assert sse_mod._KEEP_CONVERSATIONS is True + + @pytest.mark.parametrize( ("raw", "expected"), [("1", True), ("true", True), ("yes", True), ("0", False), ("false", False), ("off", False), ("", False)], @@ -892,9 +915,12 @@ def test_keep_conversations_env_var(monkeypatch, raw, expected): assert sse_mod._bool_env("GOODDATA_EVAL_KEEP_CONVERSATIONS", False) is expected -def test_conversations_are_deleted_when_the_flag_is_off(): +def test_conversations_are_deleted_when_the_flag_is_off(monkeypatch): """The default is unchanged: a run that does not ask to keep state leaves none behind.""" calls: list[str] = [] + # Set explicitly rather than assumed: with GOODDATA_EVAL_KEEP_CONVERSATIONS exported, the + # module initialises to True and this test would fail when run on its own. + monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", False) client = _client_with_handler(_delete_recording_handler(calls, "conv-default", _OK_SSE)) client.delete_conversation("conv-default")