Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d202f90fa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds explicit install vs uninstall handling to backend “skills” management so users can remove skill setups cleanly, while preserving existing install behavior where backend-specific handlers exist.
Changes:
- Extend
ai-code-install-backend-skillsto prompt for an install/uninstall action and route uninstall through a dedicated fallback prompt. - Refactor the existing install fallback into a shared install/uninstall fallback helper with action-specific prompts.
- Update the transient menu label and expand ERT coverage for the new action choice and uninstall path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ai-code-backends.el |
Adds install/uninstall action selection and a shared fallback prompt helper; updates backend specs. |
ai-code.el |
Updates transient menu label to reflect (un)install capability. |
test/test_ai-code-backends.el |
Updates existing install tests to account for the new action prompt and adds an uninstall fallback test. |
| (defun ai-code-install-backend-skills () | ||
| "Install skills for the currently selected backend. | ||
| If the backend defines an :install-skills property, use it: | ||
| - string: run as a shell command via `compile'. | ||
| - symbol: call the function. | ||
| Otherwise fall back to prompting the AI session to install from a | ||
| skills repository URL." | ||
| ;; DONE: firstly ask user if it is install or uninstall, then ask for the repo URL if needed, and finally run install / uninstall using corresponding prompt. | ||
| (interactive) | ||
| (let* ((spec (ai-code--backend-spec ai-code-selected-backend))) |
There was a problem hiding this comment.
ai-code-install-backend-skills now supports both install and uninstall via completing-read, but the docstring still describes install-only behavior, and the inline ;; DONE: comment reads like leftover implementation notes. Please update the docstring to reflect the new (un)install flow and remove the DONE note to keep the function documentation accurate and maintainable.
ai-code-backends.el
Outdated
| (defun ai-code--manage-backend-skills-fallback (label action) | ||
| "Fallback backend skills management for LABEL and ACTION. | ||
| ACTION should be the symbol `install' or `uninstall'." | ||
| (let* ((action-name (if (eq action 'uninstall) "uninstall" "install")) |
There was a problem hiding this comment.
ai-code--manage-backend-skills-fallback silently treats any ACTION other than uninstall as install (via action-name defaulting to "install"). Since the docstring says ACTION should be install or uninstall, it would be safer to validate ACTION explicitly (e.g., pcase/memq) and signal a user-error for unexpected values to avoid incorrect prompts if this helper is called incorrectly in the future.
| (let* ((action-name (if (eq action 'uninstall) "uninstall" "install")) | |
| (let* ((action-name | |
| (pcase action | |
| ('install "install") | |
| ('uninstall "uninstall") | |
| (_ (user-error | |
| "Invalid backend skills action: %S; expected `install' or `uninstall'" | |
| action)))) |
| url label)) | ||
| (if (eq action 'uninstall) | ||
| (format | ||
| "Please read the README of %s and uninstall/remove the skills described there for %s. Follow the repository instructions to remove any installed skill files and cleanup related configuration." |
There was a problem hiding this comment.
In the uninstall fallback prompt, "cleanup related configuration" should be "clean up related configuration" (verb form). This prompt is user/AI-facing text, so tightening grammar here helps avoid ambiguity.
| "Please read the README of %s and uninstall/remove the skills described there for %s. Follow the repository instructions to remove any installed skill files and cleanup related configuration." | |
| "Please read the README of %s and uninstall/remove the skills described there for %s. Follow the repository instructions to remove any installed skill files and clean up related configuration." |
Summary
Backend skill management only handled install flows, which made it awkward to remove skill setups cleanly.
This branch adds an explicit install/uninstall choice to
ai-code-install-backend-skills, keeps install behavior using the existing backend-specific handler when available, and routes uninstall through a dedicated fallback prompt so Codex can remove skill files and related configuration consistently.Verification
Previously verified during implementation with targeted and full ERT runs for the touched backend flow; no tests were run as part of this PR creation request.