fix(skills): skip disabled skills in SkillUseRail incremental loader - #440
fix(skills): skip disabled skills in SkillUseRail incremental loader#440openjiuwen-sync-bot[bot] wants to merge 2 commits into
Conversation
_refresh_skills_incrementally loaded every SKILL.md under skills_dir into the cache, then _filter_skills dropped disabled / not-enabled entries only afterwards. Disabled skills were therefore still scanned, read and cached (wasted IO + cache pollution) even though they never appear in the prompt. Add _is_skill_enabled() mirroring _filter_skills, and skip filtered-out skills in the incremental loader before stat/load. The cache and the exposed skills list now only ever contain enabled skills. Closes #1499. Co-Authored-By: AtomCode (deepseek-v4-flash) <noreply@atomgit.com>
_skill_cache keys are resolved absolute paths, so asserting a bare skill name was always true and never actually verified the disabled skill was excluded from the cache. Use the resolved path of the disabled skill dir instead (as the allow-list test already does). Co-Authored-By: AtomCode (deepseek-v4-flash) <noreply@atomgit.com>
|
|
![openjiuwen-collaboration-bot[bot]](https://avatars.githubusercontent.com/u/264091041?s=60&v=4){kind=small}
| rail = SkillUseRail(str(skills_root), disabled_skills=["disabled_skill"]) | ||
| await rail.reload_skills() | ||
| assert {s.name for s in rail.skills} == {"enabled_skill", "other_skill"} | ||
| # _skill_cache keys are resolved absolute paths, not bare skill names. |
There was a problem hiding this comment.
head_sha: 5651a4fc68d8baaeda33c4eb63bf581ad8dbe331
🟡 Medium Priority
变更行：第 38 行 assert "disabled_skill" not in rail._skill_cache。
_skill_cache 的键格式是 str(item.resolve())（即解析后的绝对路径，如 /tmp/pytest-xxx/.../disabled_skill），而断言检查的是裸字符串 "disabled_skill"。由于该字符串永远不会作为字典键出现，该断言在任何情况下都为 True，无法验证 disabled skill 确实没有被加载到缓存中。如果将来代码回归导致 disabled skill 重新被缓存，此测试不会失败。
相比之下，同文件中第 47 行的 test_enabled_allowlist_limits_loaded_skills 正确地使用了 str((skills_root / "enabled_skill").resolve()) 作为键进行断言，可作为参考。
建议：将断言中的键改为解析后的绝对路径，与 _skill_cache 实际键格式一致，参考同文件第 47 行的写法。
| # _skill_cache keys are resolved absolute paths, not bare skill names. | |
| assert str((skills_root / "disabled_skill").resolve()) not in rail._skill_cache |
2 participants
Paired: GitHub #440 ↔ GitCode !2264
Summary
Fixes #1499: disabled / not-enabled skills were still scanned, read, and cached by
SkillUseRail, even though they never appear in the prompt.Root cause
_refresh_skills_incrementallyloaded everySKILL.mdunderskills_dirinto_skill_cache;_filter_skillsdroppeddisabled_skills/ non-enabled_skillsentries only afterwards, on the exposedskillslist. So filtered-out skills still incurred disk IO + parsing and polluted the cache.Change (
openjiuwen/harness/rails/skills/skill_use_rail.py)_is_skill_enabled(skill_name)mirroring_filter_skills.skillslist only ever contain enabled skills (and a removed skill no longer lingers after a config change, since it's never cached).Tests (
tests/unit_tests/harness/rails/skills/test_skill_use_rail_disabled.py, new)disabled_skills→ excluded fromskillsand never present in_skill_cacheenabled_skillsallow-list → only the listed skill is loaded (cache holds exactly one key)Verification
uv run pytest tests/unit_tests/harness/rails/skills/test_skill_use_rail_disabled.py— 3 passedLinked Closing Issues: