Skip to content

Commit 3f1ec23

Browse files
Tranquil-Flowgzsiang
authored andcommitted
test(skills): cover additional rescan paths in skill_commands cache (NousResearch#14536)
The rescan-on-platform-change fix landed in NousResearch#18739 ships one regression test that exercises the HERMES_PLATFORM env-var path. Three other code paths in get_skill_commands / _resolve_skill_commands_platform have no direct coverage; this commit adds a regression test for each. - Gateway session context (HERMES_SESSION_PLATFORM via ContextVar): the resolver consults get_session_env after HERMES_PLATFORM, and the gateway sets that variable through set_session_vars (a ContextVar), not os.environ. The test uses set_session_vars / clear_session_vars to drive the actual gateway signal, and the disabled-skill stub reads the same value via get_session_env. A regression that swapped get_session_env for plain os.getenv would still pass an env-var-based test but break concurrent gateway sessions, which is the bug the ContextVar plumbing exists to prevent. - Returning to no-platform-scope (CLI / cron / RL rollouts after a gateway session): the cached telegram view must be dropped and the unfiltered scan repopulated when HERMES_PLATFORM is unset again. - Same-platform cache hit: consecutive calls under the same platform scope must NOT rescan. The rescan trigger is change in scope, not "always re-resolve" — a gateway serving many consecutive telegram requests should pay the scan cost once, not per request. The third test wraps scan_skill_commands with a spy after the cache is primed, so the assertion is on call_count == 0 across three subsequent get_skill_commands() calls. All 39 tests in tests/agent/test_skill_commands.py pass under scripts/run_tests.sh.
1 parent 63c473e commit 3f1ec23

1 file changed

Lines changed: 131 additions & 0 deletions

File tree

tests/agent/test_skill_commands.py

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,137 @@ def _disabled_skills():
177177
assert "/telegram-only" not in telegram_again
178178
assert "/discord-only" in telegram_again
179179

180+
def test_get_skill_commands_rescans_when_session_platform_changes(self, tmp_path):
181+
"""``HERMES_SESSION_PLATFORM`` from the gateway session context must
182+
also trigger a rescan, not just ``HERMES_PLATFORM`` (#14536).
183+
184+
Exercises the real ContextVar path: the gateway sets the active
185+
adapter via ``set_session_vars(platform=...)`` and the resolver
186+
reads it via ``get_session_env``. Setting ``HERMES_SESSION_PLATFORM``
187+
in ``os.environ`` would only test ``get_session_env``'s legacy
188+
env-var fallback — a regression that swapped ``get_session_env``
189+
for plain ``os.getenv`` would still pass while breaking concurrent
190+
gateway sessions, which is the bug the ContextVar plumbing exists
191+
to prevent in the first place.
192+
"""
193+
import agent.skill_commands as sc_mod
194+
from agent.skill_commands import get_skill_commands
195+
from gateway.session_context import (
196+
clear_session_vars,
197+
get_session_env,
198+
set_session_vars,
199+
)
200+
201+
def _disabled_skills():
202+
platform = (
203+
os.getenv("HERMES_PLATFORM")
204+
or get_session_env("HERMES_SESSION_PLATFORM")
205+
)
206+
if platform == "telegram":
207+
return {"telegram-only"}
208+
if platform == "discord":
209+
return {"discord-only"}
210+
return set()
211+
212+
with (
213+
patch("tools.skills_tool.SKILLS_DIR", tmp_path),
214+
patch("tools.skills_tool._get_disabled_skill_names", side_effect=_disabled_skills),
215+
patch.object(sc_mod, "_skill_commands", {}),
216+
patch.object(sc_mod, "_skill_commands_platform", None),
217+
):
218+
_make_skill(tmp_path, "shared")
219+
_make_skill(tmp_path, "telegram-only")
220+
_make_skill(tmp_path, "discord-only")
221+
222+
# First simulated gateway request: telegram handler.
223+
tokens = set_session_vars(platform="telegram")
224+
try:
225+
telegram_commands = dict(get_skill_commands())
226+
finally:
227+
clear_session_vars(tokens)
228+
229+
assert "/shared" in telegram_commands
230+
assert "/discord-only" in telegram_commands
231+
assert "/telegram-only" not in telegram_commands
232+
233+
# Second simulated gateway request: discord handler. The cache
234+
# was just populated for telegram; the rescan trigger must fire
235+
# off the ContextVar change, not just an env-var change.
236+
tokens = set_session_vars(platform="discord")
237+
try:
238+
discord_commands = dict(get_skill_commands())
239+
finally:
240+
clear_session_vars(tokens)
241+
242+
assert "/shared" in discord_commands
243+
assert "/telegram-only" in discord_commands
244+
assert "/discord-only" not in discord_commands
245+
246+
def test_get_skill_commands_rescans_when_leaving_platform_scope(self, tmp_path, monkeypatch):
247+
"""Returning to no-platform-scope (CLI / cron / RL) after a gateway
248+
session must rescan so the unfiltered view is repopulated (#14536).
249+
250+
A long-lived process running both gateway sessions and bare CLI
251+
invocations would otherwise stay stuck on whichever platform's
252+
filter was last applied.
253+
"""
254+
import agent.skill_commands as sc_mod
255+
from agent.skill_commands import get_skill_commands
256+
257+
def _disabled_skills():
258+
if os.getenv("HERMES_PLATFORM") == "telegram":
259+
return {"telegram-only"}
260+
return set()
261+
262+
with (
263+
patch("tools.skills_tool.SKILLS_DIR", tmp_path),
264+
patch("tools.skills_tool._get_disabled_skill_names", side_effect=_disabled_skills),
265+
patch.object(sc_mod, "_skill_commands", {}),
266+
patch.object(sc_mod, "_skill_commands_platform", None),
267+
):
268+
_make_skill(tmp_path, "shared")
269+
_make_skill(tmp_path, "telegram-only")
270+
271+
monkeypatch.setenv("HERMES_PLATFORM", "telegram")
272+
telegram_commands = dict(get_skill_commands())
273+
assert "/telegram-only" not in telegram_commands
274+
275+
# Drop back to no platform scope — bare CLI / cron / RL rollouts.
276+
monkeypatch.delenv("HERMES_PLATFORM", raising=False)
277+
bare_commands = dict(get_skill_commands())
278+
279+
assert "/telegram-only" in bare_commands
280+
assert sc_mod._skill_commands_platform is None
281+
282+
def test_get_skill_commands_does_not_rescan_when_platform_unchanged(self, tmp_path):
283+
"""Same-platform back-to-back calls must hit the cache, not rescan.
284+
285+
The rescan trigger is *change* in platform scope, not "always
286+
re-resolve." A gateway serving consecutive telegram requests must
287+
not pay the scan cost for each one.
288+
"""
289+
import agent.skill_commands as sc_mod
290+
from agent.skill_commands import get_skill_commands
291+
292+
with (
293+
patch("tools.skills_tool.SKILLS_DIR", tmp_path),
294+
patch.object(sc_mod, "_skill_commands", {}),
295+
patch.object(sc_mod, "_skill_commands_platform", None),
296+
patch.dict(os.environ, {"HERMES_PLATFORM": "telegram"}),
297+
):
298+
_make_skill(tmp_path, "shared")
299+
# Prime the cache.
300+
get_skill_commands()
301+
# Spy on rescans during the subsequent same-platform calls.
302+
with patch(
303+
"agent.skill_commands.scan_skill_commands",
304+
wraps=sc_mod.scan_skill_commands,
305+
) as scan_spy:
306+
get_skill_commands()
307+
get_skill_commands()
308+
get_skill_commands()
309+
assert scan_spy.call_count == 0
310+
180311

181312
def test_special_chars_stripped_from_cmd_key(self, tmp_path):
182313
"""Skill names with +, /, or other special chars produce clean cmd keys."""

0 commit comments

Comments
 (0)