feat: add --disable-commands flag to hide and disable slash commands in TUI#2913
Conversation
| if c == "" { | ||
| continue | ||
| } | ||
| if !strings.HasPrefix(c, "/") { |
There was a problem hiding this comment.
[LOW] WithDisabledCommands is case-sensitive — /Cost silently fails to disable /cost
The function normalizes leading slashes (e.g. "cost" → "/cost") and trims whitespace, but does not fold to lowercase. If a programmatic caller passes "/Cost" or "/COST", the map lookup m.disabledCommands[item.SlashCommand] will not match the actual slash command (which is stored in lowercase). The command will silently remain enabled.
The doc comment currently only mentions the slash-prefix normalization. In practice, all defined slash commands use lowercase, so interactive users of the CLI flag are unlikely to encounter this. But the WithDisabledCommands public API gives no indication that casing matters.
Suggestion: either add a note to the doc comment (// Case-sensitive: entries must match the exact casing of the slash command.) or normalize with strings.ToLower.
aheritier
added
area/cli
dgageot
force-pushed
the
board/a7b0512f057b0683
branch
from
a208313 to
f7b7e70
Compare
|
Addressed review feedback:
Rebased on top of |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding in the newly added filter. The guard condition for empty categories and the redundant outer guard in are clean. Flag wiring through , normalization logic, and test coverage are all correct.
| for _, cat := range categories { | ||
| items := make([]commands.Item, 0, len(cat.Commands)) | ||
| for _, item := range cat.Commands { | ||
| if m.disabledCommands[item.SlashCommand] { |
There was a problem hiding this comment.
[MEDIUM] Case-insensitive filtering silently fails for dynamic commands with uppercase names
WithDisabledCommands normalises user input to lowercase before storing in the disabledCommands map (e.g. "MyTool" → "/mytool"), but the lookup in commandCategories() uses item.SlashCommand verbatim — with no strings.ToLower applied to the registered value.
For built-in commands this is harmless (they're hardcoded as lowercase strings like "/cost", "/eval"). However, dynamically-registered entries — agent commands and skill commands — are built as "/" + name where name comes directly from the runtime registry:
// pkg/tui/commands/commands.go (existing, not in diff)
SlashCommand: "/" + name, // agent command — name not lowercased
SlashCommand: "/" + skillName, // skill command — skillName not lowercasedIf any such name contains uppercase letters (e.g. "MySkill" → "/MySkill"), passing --disable-commands=/myskill adds "/myskill" to the map, but the lookup m.disabledCommands["/MySkill"] misses it. The PR's claim of case-insensitive matching only holds for the user-input side, not for the registered value side.
Fix: lowercase item.SlashCommand at the point of lookup:
if m.disabledCommands[strings.ToLower(item.SlashCommand)] {
aheritier
removed
the
status/needs-rebase
5 participants
Users sometimes want to limit which slash commands are available in the TUI—for example, to hide commands that shouldn't be used in a particular environment or workflow. This change adds a
--disable-commandsflag todocker agent runanddocker agent execthat lets you specify which commands to hide.The flag accepts a comma-separated list of slash command names, like
--disable-commands="/cost,/eval,/model". Leading slashes are optional and matching is case-insensitive (/Cost,cost,/costall disable/cost). When set, the specified commands are completely hidden and disabled: they're stripped from the command palette (Ctrl+K), the slash-command parser, and the editor completion popup. The filtering happens in a singlecommandCategories()chokepoint, so all three surfaces inherit the behavior consistently.