Skip to content

Commit 4bb2941

Browse files
Tomkessclaude
andcommitted
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 <noreply@anthropic.com>
1 parent 0f69236 commit 4bb2941

6 files changed

Lines changed: 242 additions & 2 deletions

File tree

‎packages/gooddata-eval/src/gooddata_eval/cli/main.py‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,12 @@
1515
from rich.table import Table
1616

1717
from gooddata_eval.cli.agentic_runner import AGENTIC_TEST_KINDS, UNGATED_AGENTIC_TEST_KINDS, run_agentic_items
18-
from gooddata_eval.core.chat.sse_client import ChatClient, set_default_item_timeout, set_default_turn_timeout
18+
from gooddata_eval.core.chat.sse_client import (
19+
ChatClient,
20+
set_default_item_timeout,
21+
set_default_turn_timeout,
22+
set_keep_conversations,
23+
)
1924
from gooddata_eval.core.config import (
2025
DEFAULT_GATE,
2126
DEFAULT_JUDGE_MODEL,
@@ -169,7 +174,17 @@ def _build_parser() -> argparse.ArgumentParser:
169174
"--preserve-failed",
170175
action="store_true",
171176
dest="preserve_failed",
172-
help="Keep failed conversations on the server for post-mortem inspection.",
177+
help="Keep failed conversations on the server for post-mortem inspection. Single-turn "
178+
"kinds only; use --keep-conversations to cover the agentic ones.",
179+
)
180+
run.add_argument(
181+
"--keep-conversations",
182+
action="store_true",
183+
dest="keep_conversations",
184+
help="Keep every conversation on the server, passed or failed, so the AI Interaction "
185+
"Intelligence endpoints can be queried after the run (or set "
186+
"GOODDATA_EVAL_KEEP_CONVERSATIONS=1). Covers the agentic kinds, which --preserve-failed "
187+
"does not. Leaves state behind: only for a diagnostic run.",
173188
)
174189
run.add_argument(
175190
"--reasoning-effort",
@@ -485,6 +500,8 @@ def _run(config: RunConfig) -> int:
485500
# Applies to the agentic evaluators' own clients too, which this function never sees.
486501
set_default_turn_timeout(config.turn_timeout_s)
487502
set_default_item_timeout(config.item_timeout_s)
503+
if config.keep_conversations:
504+
set_keep_conversations(True)
488505
if config.log_to_langfuse and config.langfuse_dataset is None:
489506
print(
490507
"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:
723740
quiet=args.quiet,
724741
kind=args.kind,
725742
preserve_failed=args.preserve_failed,
743+
keep_conversations=args.keep_conversations,
726744
reasoning_effort=args.reasoning_effort,
727745
gate=normalize_gate(args.gate),
728746
agent_id=args.agent_id or os.environ.get("GD_EVAL_AGENT_ID"),

‎packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,16 @@ def _float_env(name: str, default: float) -> float:
119119
return float(raw) if raw else default
120120

121121

122+
def _bool_env(name: str, default: bool) -> bool:
123+
"""Read a flag from the environment, falling back to ``default`` when unset or blank.
124+
125+
``0``/``false``/``no``/``off`` are false, anything else present is true -- so the usual
126+
``VAR=0`` turns a flag off rather than reading as a non-empty, therefore true, string.
127+
"""
128+
raw = os.getenv(name)
129+
return raw.strip().lower() not in ("0", "false", "no", "off") if raw and raw.strip() else default
130+
131+
122132
# Retry budget defaults, giving a ~2 min worst-case cap per send (5/10/20/40/60s).
123133
# Each is overridable via env so CI can retune without cutting a new release --
124134
# 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:
148158
_TRACE_LABELS_ENV = "GOODDATA_EVAL_TRACE_LABELS"
149159
_TRACE_LABEL_KEY = re.compile(r"[A-Za-z0-9_-]+")
150160

161+
# When set, no conversation is deleted -- pass or fail, single-turn or agentic. The
162+
# server-side record is what the Interaction Intelligence endpoints read, so a run meant to
163+
# be inspected afterwards has to leave it behind. Enforced inside delete_conversation rather
164+
# than at each call site: thirteen agentic evaluators call it twenty times between them, and
165+
# a fourteenth kind would have been one more place to remember.
166+
_KEEP_CONVERSATIONS = _bool_env("GOODDATA_EVAL_KEEP_CONVERSATIONS", False)
167+
151168

152169
@functools.cache
153170
def _warn_skipped_trace_label(key: str) -> None:
@@ -194,6 +211,17 @@ def set_default_item_timeout(seconds: float | None) -> None:
194211
_ITEM_TIMEOUT_S = seconds or 0.0
195212

196213

214+
def set_keep_conversations(keep: bool) -> None:
215+
"""Keep every conversation this process creates, whatever its outcome.
216+
217+
Lands here for the same reason the timeout defaults do -- the agentic evaluators build
218+
their own clients deep in the call tree -- but takes effect at deletion rather than at
219+
construction, so a client already built honours it too.
220+
"""
221+
global _KEEP_CONVERSATIONS
222+
_KEEP_CONVERSATIONS = keep
223+
224+
197225
T = TypeVar("T")
198226

199227

@@ -550,6 +578,8 @@ def _do() -> str:
550578
return conversation_id
551579

552580
def delete_conversation(self, conversation_id: str) -> None:
581+
if _KEEP_CONVERSATIONS:
582+
return
553583
try:
554584
self._client.delete(f"{self._base}/{conversation_id}", headers=self._auth)
555585
except httpx.HTTPError:

‎packages/gooddata-eval/src/gooddata_eval/core/config.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ class RunConfig:
9090
quiet: bool = False
9191
kind: str = "visualization"
9292
preserve_failed: bool = False
93+
keep_conversations: bool = False
94+
"""Keep every conversation, pass or fail. Superset of ``preserve_failed``; applies to the
95+
agentic kinds too, which ``preserve_failed`` never reached."""
9396
reasoning_effort: ReasoningEffort | None = None
9497
agent_id: str | None = None
9598
turn_timeout_s: float | None = None

‎packages/gooddata-eval/tests/test_agentic_runner.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
# (C) 2026 GoodData Corporation. All rights reserved.
22
# SPDX-License-Identifier: LicenseRef-GoodData-Enterprise
33
import importlib
4+
import re
45
import threading
56
import time
67
from concurrent.futures import ThreadPoolExecutor
8+
from pathlib import Path
79
from typing import Any
810
from unittest.mock import patch
911

@@ -855,3 +857,37 @@ def test_run_agentic_binds_the_user_context_to_its_chat_client(
855857
with patch.object(module, "ChatClient", side_effect=_Stop) as mock_client, pytest.raises(_Stop):
856858
getattr(module, run_fn)("https://h", "tok", "ws1", "q", expected, user_context=user_context)
857859
assert mock_client.call_args.kwargs["user_context"] == user_context
860+
861+
862+
def _agentic_module_sources() -> dict[str, str]:
863+
"""Every agentic evaluator module's source, keyed by name. Discovered by scanning the
864+
package so a new kind is covered the day it lands rather than when someone remembers."""
865+
package = importlib.import_module("gooddata_eval.core.agentic")
866+
directory = Path(package.__file__).parent
867+
return {
868+
path.stem: path.read_text(encoding="utf-8")
869+
for path in sorted(directory.glob("*.py"))
870+
if not path.stem.startswith("_")
871+
}
872+
873+
874+
@pytest.mark.parametrize("module_name", sorted(_agentic_module_sources()))
875+
def test_every_agentic_module_deletes_through_the_client_method(module_name: str) -> None:
876+
"""--keep-conversations is enforced inside ChatClient.delete_conversation, so a module
877+
that issues its own DELETE would quietly ignore it. Thirteen modules delete twenty times
878+
between them; one bypass is enough to lose the conversation a diagnostic run needed."""
879+
source = _agentic_module_sources()[module_name]
880+
# Only HTTP deletes: `created.delete(sdk, workspace_id)` is workspace-object cleanup,
881+
# which has nothing to do with conversation retention.
882+
bypasses = [
883+
line.strip() for line in source.splitlines() if re.search(r"\b(httpx|requests|_client)\.delete\(", line)
884+
]
885+
assert not bypasses, f"{module_name} issues its own DELETE; call client.delete_conversation instead: {bypasses}"
886+
887+
888+
def test_the_agentic_modules_do_delete_conversations() -> None:
889+
"""Guards the guard: an assertion that only forbids a pattern still passes once the
890+
cleanup it is protecting has been deleted outright."""
891+
sources = _agentic_module_sources()
892+
deleting = {name for name, src in sources.items() if "delete_conversation(" in src}
893+
assert len(deleting) >= 12, f"expected most agentic kinds to clean up, found {sorted(deleting)}"

‎packages/gooddata-eval/tests/test_cli.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,63 @@ def _fake_run(items, backend, *, runs, model, workspace_id, **kw):
683683
assert captured_kwargs.get("preserve_failed") is True
684684

685685

686+
def test_cli_keep_conversations_flag_applies_process_wide(monkeypatch, fixtures_dir):
687+
"""--keep-conversations lands on the module-level switch, not on the ChatClient kwargs.
688+
689+
The agentic evaluators build their own clients deep in the call tree, so a constructor
690+
kwarg would reach the single-turn path only -- which is the gap --preserve-failed has.
691+
"""
692+
monkeypatch.setattr(cli_main, "resolve_connection", lambda host, token, profile: ("https://h", "tok"))
693+
applied: list[bool] = []
694+
695+
class _FakeController:
696+
def __init__(self, *a, **k): ...
697+
def get_active(self):
698+
return ActiveLlmProvider(provider_id="p", default_model_id="gpt-5.2")
699+
700+
def resolve_and_activate(self, requested, provider=None):
701+
return ResolvedModel(provider_id="p", model_id="gpt-5.2", switched=False, provider_name="P")
702+
703+
def restore(self, original): ...
704+
def close(self): ...
705+
706+
monkeypatch.setattr(cli_main, "WorkspaceModelController", _FakeController)
707+
monkeypatch.setattr(cli_main, "set_keep_conversations", applied.append)
708+
monkeypatch.setattr(cli_main, "ChatClient", lambda **kwargs: object())
709+
710+
def _fake_run(items, backend, *, runs, model, workspace_id, **kw):
711+
return EvalReport(
712+
model=model,
713+
workspace_id=workspace_id,
714+
items=[
715+
ItemReport(id="i1", dataset_name="d", test_kind="visualization", question="q", pass_at_k=True, runs=1)
716+
],
717+
)
718+
719+
monkeypatch.setattr(cli_main, "run_items", _fake_run)
720+
721+
argv = [
722+
"run",
723+
"--host",
724+
"https://h",
725+
"--token",
726+
"tok",
727+
"--workspace",
728+
"ws1",
729+
"--dataset",
730+
str(fixtures_dir / "sample_dataset"),
731+
"--quiet",
732+
]
733+
assert cli_main.main([*argv, "--keep-conversations"]) == 0
734+
assert applied == [True]
735+
736+
# Absent, the switch is left alone rather than forced off: GOODDATA_EVAL_KEEP_CONVERSATIONS
737+
# is the other way to ask for this, and a bare `run` must not overwrite it.
738+
applied.clear()
739+
assert cli_main.main(argv) == 0
740+
assert applied == []
741+
742+
686743
def test_cli_rejects_negative_concurrency(monkeypatch, fixtures_dir):
687744
monkeypatch.setattr(cli_main, "resolve_connection", lambda host, token, profile: ("https://h", "tok"))
688745
exit_code = cli_main.main(

‎packages/gooddata-eval/tests/test_sse_client.py‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,102 @@ def handler(request):
806806
assert "delete" not in calls # conversation preserved
807807

808808

809+
@pytest.fixture
810+
def keep_conversations(monkeypatch):
811+
"""Turn the process-wide keep flag on for one test, and off again afterwards."""
812+
monkeypatch.setattr(sse_mod, "_KEEP_CONVERSATIONS", True)
813+
814+
815+
def _delete_recording_handler(calls, conversation_id, sse_body):
816+
def handler(request):
817+
if request.method == "POST" and request.url.path.endswith("/conversations"):
818+
return httpx.Response(200, json={"conversationId": conversation_id})
819+
if request.method == "POST" and "messages" in str(request.url):
820+
return httpx.Response(200, content=sse_body)
821+
if request.method == "DELETE":
822+
calls.append("delete")
823+
return httpx.Response(204)
824+
return httpx.Response(404)
825+
826+
return handler
827+
828+
829+
def test_keep_conversations_keeps_the_conversation_on_success(keep_conversations):
830+
"""A passing run is kept too -- that is the whole difference from preserve_failed."""
831+
calls: list[str] = []
832+
client = _client_with_handler(_delete_recording_handler(calls, "conv-keep-ok", _OK_SSE))
833+
834+
item = DatasetItem(id="t1", dataset_name="d", test_kind="visualization", question="q", expected_output={})
835+
result = client.ask(item)
836+
837+
assert result.conversation_id == "conv-keep-ok"
838+
assert "delete" not in calls
839+
840+
841+
def test_keep_conversations_keeps_the_conversation_on_failure(monkeypatch, keep_conversations):
842+
"""Set on its own, with preserve_failed left off, it still keeps a failed conversation."""
843+
calls: list[str] = []
844+
monkeypatch.setattr(sse_mod.time, "sleep", lambda s: None)
845+
client = _client_with_handler(_delete_recording_handler(calls, "conv-keep-fail", _NONRETRY_SSE))
846+
847+
item = DatasetItem(id="t1", dataset_name="d", test_kind="visualization", question="q", expected_output={})
848+
with pytest.raises(ChatError):
849+
client.ask(item)
850+
851+
assert "delete" not in calls
852+
853+
854+
def test_keep_conversations_blocks_a_direct_delete_conversation_call(keep_conversations):
855+
"""The gate is in delete_conversation itself, which is what the agentic evaluators call.
856+
857+
Each of them creates its own ChatClient deep in the call tree and deletes by hand in a
858+
finally block -- twenty call sites that never pass through ``ask``. If this stops holding,
859+
--keep-conversations silently covers the single-turn kinds only, which is the gap
860+
--preserve-failed already had.
861+
"""
862+
calls: list[str] = []
863+
client = _client_with_handler(_delete_recording_handler(calls, "conv-direct", _OK_SSE))
864+
865+
client.delete_conversation("conv-direct")
866+
867+
assert calls == []
868+
869+
870+
def test_set_keep_conversations_affects_an_already_built_client():
871+
"""The agentic clients may already exist when the CLI applies the flag."""
872+
calls: list[str] = []
873+
client = _client_with_handler(_delete_recording_handler(calls, "conv-late", _OK_SSE))
874+
try:
875+
sse_mod.set_keep_conversations(True)
876+
client.delete_conversation("conv-late")
877+
assert calls == []
878+
finally:
879+
sse_mod.set_keep_conversations(False)
880+
881+
client.delete_conversation("conv-late")
882+
assert calls == ["delete"]
883+
884+
885+
@pytest.mark.parametrize(
886+
("raw", "expected"),
887+
[("1", True), ("true", True), ("yes", True), ("0", False), ("false", False), ("off", False), ("", False)],
888+
)
889+
def test_keep_conversations_env_var(monkeypatch, raw, expected):
890+
"""``GOODDATA_EVAL_KEEP_CONVERSATIONS=0`` turns it off rather than reading as truthy."""
891+
monkeypatch.setenv("GOODDATA_EVAL_KEEP_CONVERSATIONS", raw)
892+
assert sse_mod._bool_env("GOODDATA_EVAL_KEEP_CONVERSATIONS", False) is expected
893+
894+
895+
def test_conversations_are_deleted_when_the_flag_is_off():
896+
"""The default is unchanged: a run that does not ask to keep state leaves none behind."""
897+
calls: list[str] = []
898+
client = _client_with_handler(_delete_recording_handler(calls, "conv-default", _OK_SSE))
899+
900+
client.delete_conversation("conv-default")
901+
902+
assert calls == ["delete"]
903+
904+
809905
def test_ask_without_preserve_failed_deletes_on_error(monkeypatch):
810906
"""Without preserve_failed, conversations are deleted even on error."""
811907
calls = []

0 commit comments

Comments
 (0)