fix: allow disable-model-invocation skills to be loaded via systematic_skill tool#80
Merged
Conversation
…c_skill tool Skills with disable-model-invocation: true were filtered from both the tool description and the execute path, making them completely unreachable. Split getSystematicSkills() into getAllSkills() (for execution) and getDiscoverableSkills() (for description/hints) so hidden skills can still be loaded by explicit name while remaining absent from AI-visible context.
Collaborator
Code Review SummaryVerdict: ✅ APPROVEDThe PR correctly fixes the bug where skills with were unreachable via the tool. What Changed
AnalysisLogic Correctness: ✅ The fix properly separates concerns:
Test Coverage: ✅ New tests verify:
Edge Cases: ✅ Properly handled:
Verification Results
Minor ObservationThe PR is well-structured with clear separation of concerns. The naming ( vs ) accurately describes their different purposes. No further improvements needed. |
Owner
Author
|
@fro-bot approve the PR |
Collaborator
✅ PR ApprovedThe PR has been approved and is ready for merge. |
marcusrbrown
deleted the
fix/update-disable-model-invocation-handling
branch
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
disable-model-invocation: truewere completely unreachable via thesystematic_skilltool. The singlegetSystematicSkills()function filtered them out for both description building (correct) and execution (incorrect), causing "Skill not found" errors when explicitly requested by name.getAllSkills()(used byexecute()— any non-disabled skill is loadable) andgetDiscoverableSkills()(used bybuildDescription()andbuildParameterHint()— hidesdisableModelInvocationskills from AI context). Error messages only suggest discoverable skills to avoid leaking hidden skill names.orchestrating-swarms,file-todos,compound-docsBehavior After Fix
disable-model-invocation: truesystematic_skillby exact nameTests Added
excludes disableModelInvocation skills from descriptionloads disableModelInvocation skill when explicitly requesteddoes not list disableModelInvocation skills in error suggestionsAll 296 unit tests pass. Typecheck and lint clean.