From d4de7d41792c84ec09f55848914613ff1289edcd Mon Sep 17 00:00:00 2001 From: Tranquil-Flow Date: Sun, 3 May 2026 11:14:44 +1000 Subject: [PATCH] test(skills): cover additional rescan paths in skill_commands cache (#14536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rescan-on-platform-change fix landed in #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. --- tests/agent/test_skill_commands.py | 131 +++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/tests/agent/test_skill_commands.py b/tests/agent/test_skill_commands.py index bdea17385c..bbecd5c43f 100644 --- a/tests/agent/test_skill_commands.py +++ b/tests/agent/test_skill_commands.py @@ -177,6 +177,137 @@ class TestScanSkillCommands: assert "/telegram-only" not in telegram_again assert "/discord-only" in telegram_again + def test_get_skill_commands_rescans_when_session_platform_changes(self, tmp_path): + """``HERMES_SESSION_PLATFORM`` from the gateway session context must + also trigger a rescan, not just ``HERMES_PLATFORM`` (#14536). + + Exercises the real ContextVar path: the gateway sets the active + adapter via ``set_session_vars(platform=...)`` and the resolver + reads it via ``get_session_env``. Setting ``HERMES_SESSION_PLATFORM`` + in ``os.environ`` would only test ``get_session_env``'s legacy + env-var fallback — a regression that swapped ``get_session_env`` + for plain ``os.getenv`` would still pass while breaking concurrent + gateway sessions, which is the bug the ContextVar plumbing exists + to prevent in the first place. + """ + import agent.skill_commands as sc_mod + from agent.skill_commands import get_skill_commands + from gateway.session_context import ( + clear_session_vars, + get_session_env, + set_session_vars, + ) + + def _disabled_skills(): + platform = ( + os.getenv("HERMES_PLATFORM") + or get_session_env("HERMES_SESSION_PLATFORM") + ) + if platform == "telegram": + return {"telegram-only"} + if platform == "discord": + return {"discord-only"} + return set() + + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch("tools.skills_tool._get_disabled_skill_names", side_effect=_disabled_skills), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_platform", None), + ): + _make_skill(tmp_path, "shared") + _make_skill(tmp_path, "telegram-only") + _make_skill(tmp_path, "discord-only") + + # First simulated gateway request: telegram handler. + tokens = set_session_vars(platform="telegram") + try: + telegram_commands = dict(get_skill_commands()) + finally: + clear_session_vars(tokens) + + assert "/shared" in telegram_commands + assert "/discord-only" in telegram_commands + assert "/telegram-only" not in telegram_commands + + # Second simulated gateway request: discord handler. The cache + # was just populated for telegram; the rescan trigger must fire + # off the ContextVar change, not just an env-var change. + tokens = set_session_vars(platform="discord") + try: + discord_commands = dict(get_skill_commands()) + finally: + clear_session_vars(tokens) + + assert "/shared" in discord_commands + assert "/telegram-only" in discord_commands + assert "/discord-only" not in discord_commands + + def test_get_skill_commands_rescans_when_leaving_platform_scope(self, tmp_path, monkeypatch): + """Returning to no-platform-scope (CLI / cron / RL) after a gateway + session must rescan so the unfiltered view is repopulated (#14536). + + A long-lived process running both gateway sessions and bare CLI + invocations would otherwise stay stuck on whichever platform's + filter was last applied. + """ + import agent.skill_commands as sc_mod + from agent.skill_commands import get_skill_commands + + def _disabled_skills(): + if os.getenv("HERMES_PLATFORM") == "telegram": + return {"telegram-only"} + return set() + + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch("tools.skills_tool._get_disabled_skill_names", side_effect=_disabled_skills), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_platform", None), + ): + _make_skill(tmp_path, "shared") + _make_skill(tmp_path, "telegram-only") + + monkeypatch.setenv("HERMES_PLATFORM", "telegram") + telegram_commands = dict(get_skill_commands()) + assert "/telegram-only" not in telegram_commands + + # Drop back to no platform scope — bare CLI / cron / RL rollouts. + monkeypatch.delenv("HERMES_PLATFORM", raising=False) + bare_commands = dict(get_skill_commands()) + + assert "/telegram-only" in bare_commands + assert sc_mod._skill_commands_platform is None + + def test_get_skill_commands_does_not_rescan_when_platform_unchanged(self, tmp_path): + """Same-platform back-to-back calls must hit the cache, not rescan. + + The rescan trigger is *change* in platform scope, not "always + re-resolve." A gateway serving consecutive telegram requests must + not pay the scan cost for each one. + """ + import agent.skill_commands as sc_mod + from agent.skill_commands import get_skill_commands + + with ( + patch("tools.skills_tool.SKILLS_DIR", tmp_path), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_platform", None), + patch.dict(os.environ, {"HERMES_PLATFORM": "telegram"}), + ): + _make_skill(tmp_path, "shared") + # Prime the cache. + get_skill_commands() + # Spy on rescans during the subsequent same-platform calls. + with patch( + "agent.skill_commands.scan_skill_commands", + wraps=sc_mod.scan_skill_commands, + ) as scan_spy: + get_skill_commands() + get_skill_commands() + get_skill_commands() + assert scan_spy.call_count == 0 + def test_special_chars_stripped_from_cmd_key(self, tmp_path): """Skill names with +, /, or other special chars produce clean cmd keys."""