fix(skill_commands): preserve cache when scan fails#22514
Closed
wesleysimplicio wants to merge 1 commit into
Closed
fix(skill_commands): preserve cache when scan fails#22514wesleysimplicio wants to merge 1 commit into
wesleysimplicio wants to merge 1 commit into
Conversation
Problem: scan_skill_commands() cleared `_skill_commands` and `_skill_lower_to_canonical` at the start of the function. If a later import or scan step raised, the function returned the wiped dicts and left module state empty — every previously cached skill command was silently lost until the next successful rescan. Root cause: in-place clear of module-level dicts before the work that could fail. Fix: build a local `new_commands` dict and only commit to module state once the full scan succeeds. On exception, return the existing `_skill_commands` (cache preserved). Tests: TestScanPreservesCacheOnFailure covers (1) cache preserved when the scan raises mid-flight and (2) successful rescan still replaces the cache normally. Closes NousResearch#18659
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in agent.skill_commands.scan_skill_commands() where an exception during scanning could previously wipe the process-global skill command cache, leaving no slash commands available until a later successful rescan.
Changes:
- Refactor
scan_skill_commands()to build results into a local dict and only commit to module state after a successful scan; on failure it returns the existing cache. - Add regression tests to ensure scan failures preserve the previously populated cache and successful scans still replace it.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agent/skill_commands.py | Builds skill command mapping locally and commits only on success; preserves existing cache on scan/import failure. |
| tests/agent/test_skill_commands.py | Adds regression coverage for preserving cache on scan failure and replacing cache on successful rescan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
305
to
+308
| except Exception: | ||
| pass | ||
| # Top-level scan failed (e.g. ImportError from skills_tool / | ||
| # skill_utils). Preserve the existing cache rather than wiping it. | ||
| return _skill_commands |
Collaborator
Contributor
Author
|
Closing in favor of #18668 per @alt-glitch's note — same scan_skill_commands() cache preservation fix. |
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
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.
Problem
scan_skill_commands()clears_skill_commandsand_skill_lower_to_canonicalat the start of the function. If a later import or scan step raises, the function returns the wiped dicts and leaves module state empty — every previously cached skill command is silently lost until the next successful rescan.Root cause
In-place clear of module-level dicts before the work that could fail.
Fix
Build a local
new_commandsdict and only commit to module state once the full scan succeeds. On exception, return the existing_skill_commands(cache preserved).Tests
TestScanPreservesCacheOnFailure:test_scan_failure_keeps_previous_cache— patches a downstream import to raise mid-scan, asserts the previously populated cache is preserved.test_successful_scan_replaces_cache— confirms a successful rescan still replaces the cache normally.Stash-verify confirmed the new test flips red without the fix.
Closes #18659