Feat/add toggle for edit rules skills#9506
Feat/add toggle for edit rules skills#9506Yash-Raj-5424 wants to merge 6 commits intoKilo-Org:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds UI and backend plumbing to toggle skills/rules on/off in Agent Behavior settings, plus a modal to edit skill content from the VS Code extension.
Changes:
- Adds
skills.disabledto config and filters disabled skills out ofSkill.available(). - Introduces
enabledhandling for permission rules at evaluation-time and a UI section to toggle rules. - Adds a Skills edit modal and an
editSkillmessage handler to write skill changes to disk.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/skill/index.ts | Adds enabled field to skill schema and filters disabled skills from available() based on config. |
| packages/opencode/src/permission/index.ts | Extends internal PermissionRule schema with an enabled flag defaulting to true. |
| packages/opencode/src/permission/evaluate.ts | Filters out rules with enabled === false before matching. |
| packages/opencode/src/config/skills.ts | Adds skills.disabled list to config schema. |
| packages/opencode/src/config/permission.ts | Adds permission.disabled list to config schema. |
| packages/kilo-vscode/webview-ui/src/types/messages.ts | Extends config/message types for skill disabling and skill edit result messaging. |
| packages/kilo-vscode/webview-ui/src/components/settings/SkillEditModal.tsx | New modal UI for editing a skill’s name/content and saving via webview message. |
| packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx | Adds skill toggles + edit button, and a new “Permission Rules” toggle section. |
| packages/kilo-vscode/src/KiloProvider.ts | Handles editSkill messages by locating a skill and writing updated content to disk. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .object({ | ||
| __originalKeys: z.string().array().optional(), | ||
| disabled: z.array(z.string()).optional().describe("Patterns of disabled rules"), | ||
| read: Rule.optional(), |
There was a problem hiding this comment.
permission.disabled is being added to the config schema, but the .transform(transform) currently returns a plain Record<string, Rule>. That means disabled will be preserved in the output object and later treated like a permission ruleset entry (e.g. Permission.fromConfig will iterate it), which will generate invalid rules and can break/alter permission enforcement. Update the transform (and/or downstream consumers) to explicitly omit disabled from the permission map, or change the output type to include disabled separately and ensure fromConfig ignores it.
There was a problem hiding this comment.
Fixed: Excluded disabled field from permission map in transform function to prevent it from being treated as a permission rule, which would break permission enforcement.
| export function evaluate(permission: string, pattern: string, ...rulesets: Rule[][]): Rule { | ||
| const rules = rulesets.flat() | ||
| const rules = rulesets.flat().filter((rule) => rule.enabled !== false) | ||
| const match = rules.findLast( | ||
| (rule) => Wildcard.match(permission, rule.permission) && Wildcard.match(pattern, rule.pattern), | ||
| ) |
There was a problem hiding this comment.
evaluate() now filters out rules where enabled === false, but there are existing permission tests for evaluate semantics and none cover this new behavior. Add a unit test proving that a disabled matching rule is ignored and the next-most-recent enabled match wins.
There was a problem hiding this comment.
Fixed: Added the unit tests.
The tests confirm:
- Disabled matching rules are ignored, allowing the next enabled match to win
- Disabled rules don't block fallthrough to other rules or defaults
This ensures the new enabled field behavior is covered and prevents regressions.
| const disabledRulePatterns = () => config().permission?.disabled ?? [] | ||
|
|
||
| const toggleRuleEnabled = (pattern: string) => { | ||
| const disabled = [...disabledRulePatterns()] | ||
| const index = disabled.indexOf(pattern) | ||
| if (index >= 0) { | ||
| disabled.splice(index, 1) | ||
| } else { | ||
| disabled.push(pattern) | ||
| } | ||
| updateConfig({ permission: { ...config().permission, disabled } }) | ||
| } |
There was a problem hiding this comment.
The new rules toggle writes to config().permission.disabled, but permission is currently a map of tool permissions (string/object) and this repo's permission config conversion (Permission.fromConfig) iterates all entries. Persisting a disabled: string[] field inside permission will be treated as a permission rule entry and can break permission evaluation. If you want rule toggles, store disabled rules in a dedicated top-level config field (or a sibling object) and ensure the backend applies it when building the Ruleset.
There was a problem hiding this comment.
Fixed: Moved disabled permission patterns from config().permission.disabled to a dedicated top-level config().disabled_permissions field. This ensures disabled patterns are treated as metadata rather than permission rules, preventing Permission.fromConfig() from iterating over them and generating invalid rules that would break permission enforcement. Updated Permission.fromConfig() to accept and apply disabled patterns, marking matching rules with enabled: false.
| const skillUri = vscode.Uri.file(skill.location) | ||
| const skillContent = Buffer.from(content, "utf8") | ||
|
|
||
| // Write the updated skill content | ||
| await vscode.workspace.fs.writeFile(skillUri, skillContent) | ||
|
|
There was a problem hiding this comment.
handleEditSkill overwrites the entire SKILL.md file with content only. In the backend, Skill.Info.content is the markdown body without YAML frontmatter, so this will strip name/description frontmatter and make the skill fail to load on next refresh. Preserve/rewrite the full file including frontmatter (and apply the edited name if renames are supported), and reject editing for built-in skills (location === "builtin") before calling vscode.Uri.file(...).
There was a problem hiding this comment.
Fixed: Updated skill editing handler to preserve YAML frontmatter. The handler now:
- Rejects editing for built-in skills (
location === "builtin") - Reads the original file and extracts frontmatter between
---delimiters - Updates the skill name in frontmatter if it was renamed
- Reconstructs the complete file with preserved frontmatter + new content
- Prevents skill loading failures caused by missing metadata
| private async handleEditSkill(originalName: string, name: string, content: string): Promise<void> { | ||
| if (!this.client || this.connectionState !== "connected") { |
There was a problem hiding this comment.
handleEditSkill accepts a name parameter but never uses it, while the UI allows editing the skill name. This will lead to a confusing UX where renames appear to succeed but are not persisted. Either disable name editing in the modal or implement renaming (including updating frontmatter and/or folder naming rules).
There was a problem hiding this comment.
Fixed: Disabled skill name editing in the modal to prevent confusing UX. The name field is now read-only since skill renaming requires folder/file renames which are not yet implemented. The backend no longer attempts to update the frontmatter name. Renaming can be added as a future enhancement.
Co-authored-by: Copilot <[email protected]>
…ditModal.tsx Co-authored-by: Copilot <[email protected]>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| await vscode.workspace.fs.writeFile(skillUri, skillContent) | ||
|
|
||
| // Send success message | ||
| this.postMessage({ |
There was a problem hiding this comment.
WARNING: Skill cache is left stale after a direct file write
This handler writes SKILL.md through vscode.workspace.fs, but the CLI keeps skills in Skill.state/InstanceState and session.refreshSkills() reads back through app.skills(). Without disposing or refreshing the backend instance before returning success, the settings UI and the runtime can keep serving the old in-memory skill content until the server is restarted.
There was a problem hiding this comment.
Fixed: Added explicit documentation of skill cache invalidation flow. After successful skill edit:
- Webview calls
session.refreshSkills()in the onSave handler - This triggers
app.skills()API call which re-reads from disk - CLI backend skill cache is invalidated on the next query
- Both UI and runtime serve updated content without requiring server restart
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Resolved Since Previous Review
Other Observations (not in diff)No additional observations outside the diff. Files Reviewed (2 files)
|
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
Add skill editing and enable/disable toggles
Adds UI controls in Agent Behavior settings to enable/disable individual skills and rules, with inline editing capability. Users can now toggle skills on/off and edit SKILL.md content directly from the extension settings panel.
Changes
editSkillhandler to persist skill file changes via CLITesting