Skip to content

Commit 59f7c17

Browse files
committed
Fix stale retrieval note and doctor env leak from review
TEN: reset _last_grounding/_last_sdk_ms at the start of _query_moss so a failed second search in a turn no longer replays the previous hit in the retrieval note, and send the note the current grounding return value. custom-llm: snapshot MOCK and CUSTOM_LLM_API_KEY before the first create_app() and restore both in an outer finally so run_doctor() (called in-process by the tests) stops leaking env into later tests. Add regressions for both.
1 parent da1e5ba commit 59f7c17

4 files changed

Lines changed: 230 additions & 26 deletions

File tree

apps/agora-custom-llm-moss/server/src/llm.py

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -346,26 +346,40 @@ async def health():
346346
def run_doctor() -> None:
347347
from fastapi.testclient import TestClient
348348

349-
os.environ["MOCK"] = "1"
350-
payload = {
351-
"model": "mock",
352-
"stream": True,
353-
"messages": [{"role": "user", "content": "How long do refunds take?"}],
354-
}
355-
for mode in ("ambient", "tool"):
356-
with TestClient(create_app(mode)) as client:
357-
ok = client.post("/chat/completions", json=payload)
358-
if ok.status_code != 200 or "data: [DONE]" not in ok.text:
359-
raise SystemExit(f"doctor {mode} failed: {ok.status_code} {ok.text}")
360-
print(f"doctor {mode}: ok")
361-
os.environ["MOCK"] = "0"
362-
with TestClient(create_app("ambient")) as client:
363-
denied = client.post("/chat/completions", json=payload)
364-
if denied.status_code != 401:
365-
raise SystemExit(f"doctor bearer: expected 401, got {denied.status_code}")
366-
print("doctor bearer: rejected missing Authorization")
367-
saved_key = os.environ.pop("CUSTOM_LLM_API_KEY", None)
349+
# run_doctor runs in-process from the test suite, so snapshot every env var
350+
# it flips and restore the originals on the way out (None means "was absent").
351+
saved_mock = os.environ.get("MOCK")
352+
saved_key = os.environ.get("CUSTOM_LLM_API_KEY")
353+
354+
def _restore(name: str, value: str | None) -> None:
355+
if value is None:
356+
os.environ.pop(name, None)
357+
else:
358+
os.environ[name] = value
359+
368360
try:
361+
os.environ["MOCK"] = "1"
362+
payload = {
363+
"model": "mock",
364+
"stream": True,
365+
"messages": [{"role": "user", "content": "How long do refunds take?"}],
366+
}
367+
for mode in ("ambient", "tool"):
368+
with TestClient(create_app(mode)) as client:
369+
ok = client.post("/chat/completions", json=payload)
370+
if ok.status_code != 200 or "data: [DONE]" not in ok.text:
371+
raise SystemExit(f"doctor {mode} failed: {ok.status_code} {ok.text}")
372+
print(f"doctor {mode}: ok")
373+
os.environ["MOCK"] = "0"
374+
with TestClient(create_app("ambient")) as client:
375+
denied = client.post("/chat/completions", json=payload)
376+
if denied.status_code != 401:
377+
raise SystemExit(f"doctor bearer: expected 401, got {denied.status_code}")
378+
print("doctor bearer: rejected missing Authorization")
379+
# Temporarily remove the key so the next request runs against an unset
380+
# key; create_app reloads .env, so pop again after building the app. The
381+
# outer finally owns the real restore.
382+
os.environ.pop("CUSTOM_LLM_API_KEY", None)
369383
test_app = create_app("ambient")
370384
os.environ.pop("CUSTOM_LLM_API_KEY", None)
371385
with TestClient(test_app) as client:
@@ -378,13 +392,11 @@ def run_doctor() -> None:
378392
raise SystemExit(
379393
f"doctor bearer: unset key should reject any token, got {any_token.status_code}"
380394
)
395+
print("doctor bearer: rejected any token while CUSTOM_LLM_API_KEY is unset")
396+
print("doctor: ok")
381397
finally:
382-
if saved_key is None:
383-
os.environ.pop("CUSTOM_LLM_API_KEY", None)
384-
else:
385-
os.environ["CUSTOM_LLM_API_KEY"] = saved_key
386-
print("doctor bearer: rejected any token while CUSTOM_LLM_API_KEY is unset")
387-
print("doctor: ok")
398+
_restore("MOCK", saved_mock)
399+
_restore("CUSTOM_LLM_API_KEY", saved_key)
388400

389401

390402
if __name__ == "__main__":

apps/agora-custom-llm-moss/tests/test_llm.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,3 +194,23 @@ def _load(_server_dir=None) -> None:
194194
out = capsys.readouterr().out
195195
assert "rejected any token while CUSTOM_LLM_API_KEY is unset" in out
196196
assert "doctor: ok" in out
197+
198+
199+
def test_doctor_restores_preset_env(
200+
monkeypatch: pytest.MonkeyPatch, moss_ok
201+
) -> None:
202+
monkeypatch.setenv("MOCK", "preset")
203+
monkeypatch.setenv("CUSTOM_LLM_API_KEY", "preset-key")
204+
llm.run_doctor()
205+
assert os.environ["MOCK"] == "preset"
206+
assert os.environ["CUSTOM_LLM_API_KEY"] == "preset-key"
207+
208+
209+
def test_doctor_restores_absent_env(
210+
monkeypatch: pytest.MonkeyPatch, moss_ok
211+
) -> None:
212+
monkeypatch.delenv("MOCK", raising=False)
213+
monkeypatch.delenv("CUSTOM_LLM_API_KEY", raising=False)
214+
llm.run_doctor()
215+
assert "MOCK" not in os.environ
216+
assert "CUSTOM_LLM_API_KEY" not in os.environ

apps/ten-moss/tenapp/ten_packages/extension/main_python/extension.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ async def _on_tool_call(self, cmd: Cmd) -> None:
247247
else:
248248
self._moss_tool_calls += 1
249249
grounding = await self._query_moss(query)
250-
await self._send_retrieval_note(self._last_grounding, self._last_sdk_ms)
250+
await self._send_retrieval_note(grounding, self._last_sdk_ms)
251251
else:
252252
self.ten_env.log_error(
253253
f"[MainControlExtension] unknown tool_call name={name!r}"
@@ -261,6 +261,10 @@ async def _on_tool_call(self, cmd: Cmd) -> None:
261261
await self.ten_env.return_result(result)
262262

263263
async def _query_moss(self, user_text: str) -> str:
264+
# Reset per-query state up front so a failed search never replays the
265+
# previous hit's grounding/latency in the retrieval note.
266+
self._last_grounding = ""
267+
self._last_sdk_ms = None
264268
if self.moss is None:
265269
return ""
266270
try:
Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
"""Regression for the retrieval note after a failed second search in a turn.
2+
3+
extension.py imports the TEN runtime, which is not installed offline, so we load
4+
just that one file against light stubs and drive its Moss methods directly.
5+
"""
6+
7+
from __future__ import annotations
8+
9+
import importlib.util
10+
import json
11+
import sys
12+
import types
13+
from pathlib import Path
14+
15+
import pytest
16+
17+
EXTENSION = (
18+
Path(__file__).resolve().parents[1]
19+
/ "tenapp"
20+
/ "ten_packages"
21+
/ "extension"
22+
/ "main_python"
23+
/ "extension.py"
24+
)
25+
26+
27+
def _load_extension_module():
28+
pkg = "main_python"
29+
30+
def stub(name: str) -> types.ModuleType:
31+
mod = types.ModuleType(name)
32+
sys.modules[name] = mod
33+
return mod
34+
35+
runtime = stub("ten_runtime")
36+
for symbol in ("AsyncTenEnv", "Cmd", "Data"):
37+
setattr(runtime, symbol, type(symbol, (), {}))
38+
runtime.AsyncExtension = type(
39+
"AsyncExtension", (), {"__init__": lambda self, name: None}
40+
)
41+
runtime.StatusCode = type("StatusCode", (), {"OK": "ok"})
42+
43+
class _CmdResult:
44+
def __init__(self):
45+
self.content = None
46+
47+
@classmethod
48+
def create(cls, _status, _cmd):
49+
return cls()
50+
51+
def set_property_from_json(self, _key, value):
52+
self.content = value
53+
54+
runtime.CmdResult = _CmdResult
55+
56+
ten_moss = stub("ten_moss")
57+
ten_moss.MossSessionManager = type("MossSessionManager", (), {})
58+
59+
const = stub("ten_ai_base.const")
60+
const.CMD_PROPERTY_RESULT = "result"
61+
ai_types = stub("ten_ai_base.types")
62+
ai_types.LLMToolMetadata = type("LLMToolMetadata", (), {})
63+
ai_types.LLMToolMetadataParameter = type("LLMToolMetadataParameter", (), {})
64+
ai_base = stub("ten_ai_base")
65+
ai_base.const = const
66+
ai_base.types = ai_types
67+
68+
parent = stub(pkg)
69+
parent.__path__ = []
70+
agent_pkg = stub(f"{pkg}.agent")
71+
agent_pkg.__path__ = []
72+
decorators = stub(f"{pkg}.agent.decorators")
73+
decorators.agent_event_handler = lambda *a, **k: (lambda fn: fn)
74+
agent_mod = stub(f"{pkg}.agent.agent")
75+
agent_mod.Agent = type("Agent", (), {})
76+
events = stub(f"{pkg}.agent.events")
77+
for evt in (
78+
"ASRResultEvent",
79+
"LLMResponseEvent",
80+
"ToolRegisterEvent",
81+
"UserJoinedEvent",
82+
"UserLeftEvent",
83+
):
84+
setattr(events, evt, type(evt, (), {}))
85+
helper = stub(f"{pkg}.helper")
86+
helper._send_cmd = helper._send_data = lambda *a, **k: None
87+
helper.parse_sentences = lambda frag, delta: ([], "")
88+
config = stub(f"{pkg}.config")
89+
config.MainControlConfig = type("MainControlConfig", (), {})
90+
91+
spec = importlib.util.spec_from_file_location(f"{pkg}.extension", EXTENSION)
92+
module = importlib.util.module_from_spec(spec)
93+
module.__package__ = pkg
94+
spec.loader.exec_module(module)
95+
return module
96+
97+
98+
extension = _load_extension_module()
99+
100+
101+
class _RecordingEnv:
102+
def __init__(self):
103+
self.results = []
104+
105+
def log_info(self, *_a):
106+
pass
107+
108+
def log_error(self, *_a):
109+
pass
110+
111+
async def return_result(self, result):
112+
self.results.append(result)
113+
114+
115+
class _FlakyMoss:
116+
"""Succeeds once, then raises - a second search failing mid-turn."""
117+
118+
last_time_taken_ms = 12
119+
120+
def __init__(self):
121+
self.calls = 0
122+
123+
async def query_context(self, _text: str) -> str:
124+
self.calls += 1
125+
if self.calls == 1:
126+
return "Refunds land in 3-5 business days."
127+
raise RuntimeError("moss backend unavailable")
128+
129+
130+
class _ToolCallCmd:
131+
"""A tool_call Cmd carrying a search_knowledge_base payload."""
132+
133+
def __init__(self, query: str):
134+
self._raw = json.dumps(
135+
{"name": "search_knowledge_base", "arguments": {"query": query}}
136+
)
137+
138+
def get_property_to_json(self, _key):
139+
return self._raw, None
140+
141+
142+
@pytest.mark.asyncio
143+
async def test_failed_second_search_does_not_replay_first_hit() -> None:
144+
ext = extension.MainControlExtension("main_control")
145+
ext.ten_env = _RecordingEnv()
146+
ext.moss = _FlakyMoss()
147+
148+
notes: list[str] = []
149+
150+
async def capture(role, text, final, stream_id, data_type="text"):
151+
notes.append(text)
152+
153+
ext._send_transcript = capture
154+
155+
# Drive the real tool-call handler so the note reflects production flow.
156+
await ext._on_tool_call(_ToolCallCmd("How long do refunds take?"))
157+
assert "3-5 business days" in notes[0]
158+
assert "3-5 business days" in ext.ten_env.results[0].content
159+
160+
await ext._on_tool_call(_ToolCallCmd("What about exchanges?"))
161+
162+
# Second search failed: the note must not replay the first hit, and the
163+
# tool result is empty - the two must agree.
164+
assert "3-5 business days" not in notes[1]
165+
assert "no match" in notes[1]
166+
assert '"content": ""' in ext.ten_env.results[1].content
167+
assert ext._last_grounding == ""
168+
assert ext._last_sdk_ms is None

0 commit comments

Comments
 (0)