fix: don't advertise skills the user disabled - #386
Draft
Ruijian-Zha wants to merge 1 commit into
Draft
Conversation
`buildAvailableCommands()` iterated every skill returned by `skills/list` without checking `skill.enabled`, so a skill disabled via `[[skills.config]] enabled = false` in `~/.codex/config.toml` was still published to the client as a `$`-command. The `/skills` builtin listed them for the same reason. A client that renders the advertised command list had no way to tell the skill was already turned off, so its UI ended up contradicting the user's own config. Fixes agentclientprotocol#385
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.
Fixes #385.
buildAvailableCommands()iterated every skill returned byskills/listwithout checkingskill.enabled, so a skill disabled via[[skills.config]] enabled = falsein~/.codex/config.tomlwas still published to the client as a$-command. The/skillsbuiltin listed them for the same reason.A client that renders the advertised command list had no way to tell the skill was already turned off, so its UI ended up contradicting the user's own config — a user who disables a skill and reopens the session still sees it offered.
Changes
buildAvailableCommands(): skip entries withenabled === false/skillslisting: same filter, so the two surfaces agreeTest
Added
should not advertise skills the user disabledinCodexAcpClient.test.ts— publishes askills/listresponse with one enabled and one disabled skill and snapshots the resultingavailable_commands_update(data/available-commands-skills-disabled.json):$buildis present,$deployis not.npm run typecheckclean; full suite 401 passed / 28 skipped.One thing worth knowing before merging
enabledis not the whole story for model visibility. Codex has a second, independent filter —<skill-dir>/agents/openai.yamlwithpolicy.allow_implicit_invocation: false— and a skill with that set is absent from the model-visible prompt whileskills/liststill reportsenabled: true(the bundledreview-agentships exactly this). So after this PR a policy-disabled skill is still advertised. That would need the policy state surfaced on theskills/listentry first, so I left it out of this change — happy to follow up if you'd like it tracked separately.Verifying any of this locally costs no model turn:
codex debug prompt-inputrenders the model-visible prompt list as JSON.