test(skills): cover additional rescan paths in skill_commands cache (#14536)#19042
Closed
Tranquil-Flow wants to merge 1 commit into
Closed
Conversation
…ousResearch#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds three regression tests on top of the rescan-on-platform-change fix
that landed in #18739, covering code paths in
_resolve_skill_commands_platformandget_skill_commandsthat theexisting
test_get_skill_commands_rescans_when_platform_scope_changestest does not exercise.
Suggested by @teknium1 in #15375 (closed in favor of #18739) — the
original tests on #15375 were tied to the per-platform-cache approach
and don't translate, so this PR adds fresh tests targeting the merged
rescan-based implementation instead.
Refs #14536, #18739.
Coverage gaps addressed
HERMES_SESSION_PLATFORMvia ContextVar) —_resolve_skill_commands_platformconsultsget_session_envafterHERMES_PLATFORM, and the gateway sets that variable throughset_session_vars(acontextvars.ContextVar), notos.environ.The new test uses
set_session_vars/clear_session_varsto drivethe actual gateway signal, and the disabled-skill stub reads via
get_session_envso the cache rescan is proven against the realContextVar path — not the legacy env-var fallback. A regression that
swapped
get_session_envfor plainos.getenvwould still pass anenv-var-based test but break concurrent gateway sessions, which is
the bug the ContextVar plumbing exists to prevent.
session) — verifies that unsetting
HERMES_PLATFORMafter atelegram-scoped scan triggers a fresh unfiltered scan rather than
re-serving the telegram view.
scan_skill_commandswith a spy after the cache is primed and asserts
call_count == 0across three consecutive
get_skill_commands()calls under the sameplatform. Guards against a regression where the rescan condition
simplifies to "always re-resolve" — a gateway serving consecutive
telegram requests must not pay the scan cost per request.
Tests
tests/agent/test_skill_commands.py::TestScanSkillCommands.scripts/run_tests.sh tests/agent/test_skill_commands.py.Type of change