Skip to content

fix(skills): preserve command cache on scan setup failure - #18668

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/skill-command-scan-cache-preserve-18659
Open

fix(skills): preserve command cache on scan setup failure#18668
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/skill-command-scan-cache-preserve-18659

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • build skill slash commands in a local map before replacing the global cache
  • keep the previous _skill_commands mapping available when scan setup or directory traversal fails
  • add a regression test for preserving cached commands when scan setup raises

Fixes #18659.

Verification

  • scripts/run_tests.sh tests/agent/test_skill_commands.py::TestScanSkillCommands::test_preserves_cached_commands_when_scan_setup_fails tests/agent/test_skill_commands.py
  • git diff --check origin/main...HEAD

Overlap check

@wesleysimplicio

Copy link
Copy Markdown
Contributor

Closed my competing PR (#22514) in favor of this one — same swap-on-success pattern for scan_skill_commands(). Per @alt-glitch's note, #18720 was already deferring to this PR. Looks ready; the only friction is the 12-day gap since the last push, so a rebase against current main might revive review attention. Happy to send one off your branch if useful.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the unconditional-clear failure path. The bug is still present on current main: scan_skill_commands() clears _skill_commands before scan setup at agent/skill_commands.py:327-332, then swallows setup failures at agent/skill_commands.py:385-386.

Problems

  • Current main added the platform-scoped cache marker in c73594fe4. scan_skill_commands() sets _skill_commands_platform before setup (agent/skill_commands.py:327), while get_skill_commands() trusts it to decide whether to rescan (agent/skill_commands.py:397-401). A command-map-only swap would preserve an old platform's mapping but record the requested platform after failure, suppressing the required retry.
  • The supplied test covers only _skill_commands, not _skill_commands_platform.

Suggested changes

  • Stage and publish the command map and platform marker together only after a successful scan; retain both prior values on outer failure.
  • Add a cross-platform setup-failure regression that exercises the marker and subsequent rescan behavior.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: scan_skill_commands unconditionally clears _skill_commands before try block, silently loses all skills on scan failure

4 participants