-
Notifications
You must be signed in to change notification settings - Fork 6.6k
feat: Support listing and selecting skills via $ or /skills #7506
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
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| mod footer; | ||
| mod list_selection_view; | ||
| mod prompt_args; | ||
| mod skill_popup; |
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.
Added skill_popup module missing implementation
The new mod skill_popup; and SkillPopup references introduce a required module, but there is no skill_popup.rs (nor a directory module) anywhere under codex-rs/tui/src/bottom_pane, so the crate no longer compiles (file not found for module skill_popup). A repo-wide search for skill_popup.rs turns up nothing, so every build will fail at module resolution until the file is added or the import is removed.
Useful? React with 👍 / 👎.
|
@codex review again. |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
codex-rs/core/prompt.md
Outdated
| - Do not use one-letter variable names unless explicitly requested. | ||
| - NEVER output inline citations like "【F:README.md†L5-L14】" in your outputs. The CLI is not able to render these so they will just be broken in the UI. Instead, if you output valid filepaths, users will be able to click on them to open the files in their editor. | ||
|
|
||
| ## Skill mentions |
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.
Can we inject this as part of the skills list message or condition on the skills feature being enabled?
codex-rs/tui/src/app.rs
Outdated
| // One-shot suppression of the next world-writable scan after user confirmation. | ||
| skip_world_writable_scan_once: bool, | ||
|
|
||
| pub(crate) skills: Vec<SkillMetadata>, |
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.
can skills be Optional<> and that be a signal that they are enabled? Don't feel strongly.
codex-rs/tui/src/slash_command.rs
Outdated
| SlashCommand::Quit | SlashCommand::Exit => "exit Codex", | ||
| SlashCommand::Diff => "show git diff (including untracked files)", | ||
| SlashCommand::Mention => "mention a file", | ||
| SlashCommand::Skills => "Use skills to improve how Codex performs specific tasks", |
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.
nit: casing
codex-rs/tui/src/slash_command.rs
Outdated
| pub enum SlashCommand { | ||
| // DO NOT ALPHA-SORT! Enum order is presentation order in the popup, so | ||
| // more frequently used commands should be listed first. | ||
| Skills, |
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.
Do we want skills before models and approvals?
| | SlashCommand::Logout => false, | ||
| SlashCommand::Diff | ||
| | SlashCommand::Mention | ||
| | SlashCommand::Skills |
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.
Do we filter out the slash command when skills are not available?
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review again |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| match_indices: indices | ||
| .map(|v| v.into_iter().map(|i| i.saturating_add(1)).collect()), |
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.
Fix skill popup match highlight offset
The skill popup offsets the fuzzy-match indices by one even though the rendered skill label has no leading prefix. When filtering skills via $ or /skills, the highlighted characters are shifted right and no longer correspond to the matched letters, which makes the fuzzy results misleading. Use the indices from fuzzy_match directly so highlighting aligns with the displayed skill names.
Useful? React with 👍 / 👎.
| } else { | ||
| built_in_slash_commands() | ||
| .into_iter() | ||
| .filter(|(_, cmd)| *cmd != SlashCommand::Skills) |
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.
can we simplify
| .filter(|(_, cmd)| *cmd != SlashCommand::Skills) | |
| .filter(|(_, cmd)| skills_enabled || *cmd != SlashCommand::Skills) |
and remove the if
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.
but also, should we use the existing is_visible method for this?
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.
is_visible only handles debug-only commands; teaching it about the skills flag would mean threading skills_enabled through its callers
pakrym-oai
left a comment
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.
Consider using is_visible to hide the command,
|
@codex review again |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| let selected = popup.selected_skill().map(|skill| skill.name.clone()); | ||
| if let Some(name) = selected { | ||
| self.insert_selected_skill(&name); | ||
| } | ||
| self.active_popup = ActivePopup::None; |
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.
Avoid hijacking $ tokens as skill mentions
handle_key_event_with_skill_popup consumes Enter/Tab by inserting the currently selected skill and returning InputResult::None rather than submitting whenever the skill popup is active. Because the popup opens for any $-prefixed token (via current_skill_token in sync_popups), typing a normal shell command like echo $HOME with skills enabled triggers the popup and pressing Enter replaces $HOME with the first skill instead of sending the message. This blocks submitting messages containing $… unless the user manually dismisses the popup first.
Useful? React with 👍 / 👎.
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.
It’s a valid concern but a functional trade-off
List/Select skills with $-mention or /skills