fix(skills): preserve _skill_commands cache when scan fails (#18659) - #74511
Open
Tranquil-Flow wants to merge 1 commit into
Open
fix(skills): preserve _skill_commands cache when scan fails (#18659)#74511Tranquil-Flow wants to merge 1 commit into
Tranquil-Flow wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
Collaborator
teknium1
reviewed
Jul 30, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing the real cache-loss path: current main clears the cache before entering the outer scan guard at agent/skill_commands.py:381-383, so an outer failure leaves no skill commands.
Problems
- The proposed publication is not atomic:
agent/skill_commands.py:472-473writes the map and platform marker separately, whileget_skill_commands()reads them separately atagent/skill_commands.py:484-489. Concurrent scans can still retain a map from one platform with another platform's marker, suppressing the required retry. tests/agent/test_skill_commands.py:335-367synchronizes construction but does not force or assert that publication/read interleaving, so it cannot catch this remaining race.
Suggested changes
- Publish/read map and platform as one synchronized snapshot (for example, under a shared lock or immutable snapshot object).
- Add a deterministic concurrent regression asserting the map and platform marker always remain paired.
This is an automated hermes-sweeper review.
| exc_info=True, | ||
| ) | ||
| return _skill_commands | ||
| _skill_commands = new_commands |
Contributor
There was a problem hiding this comment.
This is not an atomic publish with line 473: another scan or get_skill_commands() can run between the two global assignments and observe a new map with the previous platform marker (or vice versa). Publish and read both values through one synchronized snapshot, then add a test that forces this interleaving.
teknium1
added
sweeper:risk-compatibility
Tranquil-Flow
force-pushed
the
fix/18659-skill-command-cache
branch
from
August 4, 2026 22:12
9c5c841 to
e94325e
Compare
…arch#18659) The unconditional ``_skill_commands = {}`` and ``_skill_commands_platform`` assignment at the top of ``scan_skill_commands()`` (and the ``except Exception: pass`` that swallowed the failure) caused a transient scan failure to wipe every skill slash command and trigger a misleading ``/reload-skills`` diff reporting 90+ skills as removed. The ``_skill_commands_platform`` marker (added later for NousResearch#14536) was also written before the scan, so a failed scan for a new platform would have left the old platform-filtered mapping labelled as the requested platform and suppressed the required retry. Build the new map and platform marker in local variables and only commit them to module state when the entire scan completes without exception. On outer failure, return the previous cache unchanged and log a diagnostic warning so the user can see why their skills did not refresh. Add regression tests that exercise: - Outer scan failure preserves the command map and platform marker. - Successful empty scan still replaces the cache. - ``/reload-skills`` on a failed scan does not report survivors as removed. - The diagnostic warning is emitted (and not spammed across retries). - Overlapping scans do not share a partial command map. Fixes NousResearch#18659. Build context: - base: upstream/main @ 4d9541b - layers: 7/7 (failure preservation, mid-scan failure, atomic replacement, platform coherence, reload semantics, concurrent isolation, failure observability) - RED proof: 4 of 5 new tests fail on upstream/main - GREEN: 27/27 tests in tests/agent/test_skill_commands.py pass - regression scope: 107 tests across skill_commands + kanban DB + state DB + session preview modules pass - ruff: clean - excluded pre-existing flaky: test_tui_gateway_server.py ::test_write_json_serializes_concurrent_writes (reproduces 10/10 on upstream/main without the fix; documented as unrelated)
Tranquil-Flow
force-pushed
the
fix/18659-skill-command-cache
branch
from
August 7, 2026 16:18
e94325e to
b192929
Compare
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
3 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
Preserve the last-known-good skill slash-command cache when
scan_skill_commands()encounters an outer scan failure. The new command map and platform marker are built locally and published together only after a successful scan; failed scans retain the prior cache and emit a diagnostic warning. This also prevents overlapping scans from observing another scan's partial map.Fixes #18659.
Verification
/Users/evinova-self/.hermes/hermes-agent/venv/bin/python3 -m pytest tests/agent/test_skill_commands.py -v -o "addopts=" --tb=short— 27 passed/Users/evinova-self/.hermes/hermes-agent/venv/bin/python3 -m pytest tests/gateway/test_stacked_skill_platform_disabled.py tests/agent/test_skill_invocation_description.py tests/agent/test_skill_commands_reload.py tests/test_session_skill_previews.py -v -o "addopts=" --tb=short— 16 passedruff check agent/skill_commands.py tests/agent/test_skill_commands.py— passedgit diff --check upstream/main...HEAD— passedThe branch is one focused commit ahead of current
upstream/main(36f885573c5d054fbb3827e5d45f235cf42fd791).Competitor analysis
Open competitors #18668 and #18720 preserve only the command map and omit atomic staging of
_skill_commands_platform; both therefore can label a stale platform-filtered cache as current after a failed platform transition and suppress the required retry. #18668 also has a failed test check and is dirty. #18735 is a contaminated seven-issue bundle rather than a focused competitor. This PR covers the platform-marker, reload, concurrent-scan, successful-empty-scan, and diagnostic-warning layers with production-path regression tests.Auto-published by Moonsong via Path B automated pipeline.