-
Notifications
You must be signed in to change notification settings - Fork 11
feat: add CLI command /model-delete + add prevention #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new /model-delete CLI command that allows users to remove custom saved models, with built-in safeguards to prevent deletion of active models and Kolosal Cloud models.
- Implements
/model-deletecommand with validation to prevent deletion of active or Kolosal-managed models - Adds interactive confirmation dialog for model deletion
- Integrates delete functionality into the existing model selection dialog via 'd' key
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/ui/hooks/slashCommandProcessor.ts | Adds handler for new model_delete dialog action |
| packages/cli/src/ui/components/ModelSelectionDialog.tsx | Adds delete functionality via 'd' key and tracks selected model index |
| packages/cli/src/ui/components/ModelDeleteConfirmationDialog.tsx | New confirmation dialog component for model deletion |
| packages/cli/src/ui/commands/types.ts | Adds model_delete dialog type |
| packages/cli/src/ui/commands/modelDeleteCommand.ts | Implements the core /model-delete command logic with validation |
| packages/cli/src/ui/commands/modelDeleteCommand.test.ts | Test coverage for the model delete command |
| packages/cli/src/ui/App.tsx | Integrates delete dialog and handlers into main app |
| packages/cli/src/test-utils/mockCommandContext.ts | Adds missing sessionId field to mock context |
| packages/cli/src/services/BuiltinCommandLoader.ts | Registers the new model delete command |
| packages/cli/src/config/savedModels.ts | Adds removeSavedModelEntry utility function |
| packages/cli/src/config/savedModels.test.ts | Test coverage for removeSavedModelEntry function |
| kolosal-server | Subproject commit update |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Open model selection dialog filtered to deletable models | ||
| setIsModelSelectionDialogOpen(true); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleModelDeleteOpen function opens the model selection dialog, but this conflicts with the normal model selection flow. When a user invokes /model-delete, they should see only deletable models, but this opens the same dialog used for model switching. This will confuse users as the dialog doesn't filter to show only deletable models in the delete context.
| // Open model selection dialog filtered to deletable models | |
| setIsModelSelectionDialogOpen(true); | |
| // Open model delete dialog, which will show only deletable models | |
| setIsModelDeleteDialogOpen(true); |
|
|
||
| const handleModelDeleteOpen = useCallback(() => { | ||
| // Open model selection dialog filtered to deletable models | ||
| setIsModelSelectionDialogOpen(true); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model selection dialog is being reused for two different purposes (selection and deletion) without a clear way to distinguish the context. Consider passing a mode parameter or using a separate state variable to indicate whether the dialog is in 'select' or 'delete' mode, which would allow for different filtering and UI behavior.
| export function removeSavedModelEntry( | ||
| existing: SavedModelEntry[] | undefined, | ||
| entry: SavedModelEntry, | ||
| ): SavedModelEntry[] { |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removeSavedModelEntry function is defined but never used in the codebase. The deletion logic in App.tsx (lines 2241-2253) manually filters the array instead of calling this utility function, resulting in code duplication and inconsistency.
| } else if (key.name === 'd' && onDelete) { | ||
| const selectedModel = availableModels[selectedIndex]; | ||
| if (selectedModel && selectedModel.savedModel) { | ||
| // Let handleModelDeleteRequest handle the current model check and show error | ||
| onDelete(selectedModel); | ||
| } |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The delete key handler silently does nothing when a model is not a saved custom model. Consider providing user feedback (e.g., a brief error message) when 'd' is pressed on a non-deletable model to avoid confusion about why the action didn't trigger.
Remove the model by using
/model-deletefrom custom, and also we add delete preventation if it's active