feat: slash commands (built-in, skill, recipe) in acp server#9238
feat: slash commands (built-in, skill, recipe) in acp server#9238lifeizhou-ap wants to merge 18 commits into
Conversation
* main: (66 commits) Switch GH pages deploy to actions/artifact workflow (#9025) fix(summon): re-apply canonical limits when delegate overrides model (#9183) Split code signing from build (#8587) refactor(logging): consolidate logging setup into shared helper in goose crate (#8817) fix(cli): report cumulative total_tokens in stream-json/json output (#8910) plugins: add open plugins (just skills for now) (#9063) fix(providers): refresh GCP metadata server token on expiration (#8929) chore(deps): bump the cargo-minor-and-patch group across 1 directory with 14 updates (#9178) chore(deps): bump bzip2 from 0.5.2 to 0.6.1 (#8964) chore(deps): bump tauri from 2.10.3 to 2.11.1 in /ui/goose2/src-tauri (#9066) chore(deps): bump hono from 4.12.14 to 4.12.18 in /evals/open-model-gym/mcp-harness (#9073) localize hardcoded strings in provider settings UI (#8931) chore(deps): bump @babel/plugin-transform-modules-systemjs from 7.28.5 to 7.29.4 in /documentation (#9122) move settings into app shell (#9047) Add Location column to CLI skills table (#8785) (feat): add routstr as a declarative provider (#9175) Add FuturMix provider (#8840) fix: convert quoted numeric config values to numbers if needed (#8844) fix(ui): keep SSE reconnect loop alive on long disconnects (#8717) (#8846) fix(openai): apply request_params to outgoing API payload (#9151) ...
![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: 218e9c8e8c
ℹ️ 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".
| for c in input.chars() { | ||
| match c { | ||
| '"' if !in_single_quote => in_double_quote = !in_double_quote, | ||
| '\'' if !in_double_quote => in_single_quote = !in_single_quote, |
There was a problem hiding this comment.
split_command_args flips in_single_quote on every ', so ordinary text like O'Reilly is parsed as an unmatched quote and returned as an error. This parser is now used by recipe_slash_command::parse_recipe_args and skills::apply_skill_arguments, so slash-command recipes/skills that accept free-text arguments can fail on common apostrophes even when users are not trying to quote. Please only start single-quote mode when ' begins a quoted token (or otherwise support apostrophes inside unquoted words).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3afe8678fe
ℹ️ 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 known_keys: HashSet<&str> = required | ||
| .iter() | ||
| .chain(optional.iter()) | ||
| .map(|p| p.key.as_str()) |
There was a problem hiding this comment.
parse_recipe_args currently treats both required and optional keys as valid --flag names, but the required-position tracking (required_idx) is only advanced by positional tokens. In a mixed call like --component Button old-lib, the positional token is incorrectly assigned back to component, which overwrites the flagged value and leaves from missing. This produces confusing "missing parameter" failures for otherwise reasonable input; either disallow --<required> entirely or mark required params as consumed when seen as flags.
Useful? React with 👍 / 👎.
| for command in super::recipe_slash_command::commands_from_mappings( | ||
| super::recipe_slash_command::list_commands(), | ||
| ) { | ||
| if reserved_names.insert(command.name.clone()) { | ||
| commands.push(command); |
There was a problem hiding this comment.
The ACP command list only reserves names from recipes that successfully validate in commands_from_mappings, then it exposes same-named skills. Execution still checks raw recipe mappings first and returns a recipe error on invalid files, so users can be shown a skill command that is not actually invokable when an invalid mapped recipe with the same name exists. Reserve names from all configured recipe mappings (or fall back to skill resolution on recipe-validation errors) to keep discovery and execution consistent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cfb00b5e6
ℹ️ 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".
| if value.starts_with("--") { | ||
| return Err(anyhow!("Missing value for --{}", flag)); |
There was a problem hiding this comment.
The flag parser currently rejects any flag value whose token starts with --, so valid quoted inputs like /deploy service --args "--dry-run --force" are treated as Missing value for --args even though a value was provided. This blocks a common recipe pattern where a parameter is meant to carry downstream CLI options, because split_command_args strips quotes before this check and the value still begins with --.
Useful? React with 👍 / 👎.
| fn builtin_input_hint(command: &str) -> Option<&'static str> { | ||
| match command { | ||
| "prompt" => Some("<name> [--info] [key=value...]"), | ||
| "prompts" => Some("[--extension <name>]"), |
There was a problem hiding this comment.
/prompts input hint with actual argument parsing
The ACP command metadata advertises /prompts [--extension <name>], but command execution does not parse a --extension flag (it treats the first token as the raw extension name in handle_prompts_command). Following the published hint therefore fails with an "Extension '--extension' not found" style error instead of filtering prompts, so discovery metadata and runtime behavior diverge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f30d691a5
ℹ️ 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".
| self.send_available_commands_update(cx, &args.session_id) | ||
| .await?; | ||
|
|
||
| Self::send_available_commands_update(cx, &args.session_id, &args.cwd)?; |
There was a problem hiding this comment.
on_load_session builds the available-command list from args.cwd, but slash-skill execution later resolves skills from the session’s stored working directory (handle_skill_command reads it via session_manager.get_session). When those differ (for example clients that call load with a generic cwd like "~"), ACP advertises skills that are not actually runnable in that loaded session, causing /skill commands suggested by discovery to fail at runtime. Build the command update from goose_session.working_dir to keep discovery and execution aligned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think args.cwd is intentional here. on_load_session updates the persisted session working_dir before publishing commands:
self.session_manager
.update(&session_id)
.working_dir(args.cwd.clone())
.apply()
.await?;
Slash-skill execution later reloads the session from session_manager, so it should resolve skills from that same updated cwd.
lifeizhou-ap
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dbcad75a7
ℹ️ 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".
| for c in input.chars() { | ||
| match c { | ||
| '"' if !in_single_quote => in_double_quote = !in_double_quote, | ||
| '\'' if !in_double_quote && (in_single_quote || current.is_empty()) => { |
There was a problem hiding this comment.
The new single-quote guard only enters quote mode when current.is_empty(), so inputs like ENV='two words' cmd or --note='needs review' are tokenized at the internal space instead of being kept as one argument. This is a regression from the previous parser behavior and now affects every caller of split_command_args (notably stdio extension parsing plus recipe/skill slash-argument parsing), causing valid quoted values attached to env/flag prefixes to be misparsed or rejected.
Useful? React with 👍 / 👎.
DOsinga
added
the
needs_human
2 participants
Summary
Wires
builtin,skill, andrecipeslash commands into the ACPavailable_commandsupdate so ACP clients get a unified list, and restructures the slash-command code into a proper module with first-class skill and recipe handlers (incl. argument parsing).Changes
ACP: surface all slash commands
available_commands_updatenow returns builtins + recipes + skills in one list, with name-collision precedencebuiltin > recipe > skill.MentionPopovernow inserts/skill-namefor skill items, matching builtin/recipe behavior (previously inserted prose"Use the X skill to ").New
slash_commands/moduleReplaces the old single-file
crates/goose/src/slash_commands.rswith a module that splits recipe, skill, into their own files and merges them into a unified list for ACP.Skill slash commands
/skill-name [args].$ARGUMENTSblob, positional tokens ($1,$ARGUMENTS[0]), or named args ($component,$from) tied to the skill's declared argument names. Skills with no placeholders still receive the raw args appended so the model can see them.metadata.argument-hintandmetadata.arguments(per the agentskills.io frontmatter spec). Open question whether to promote these to top-level frontmatter to match Claude Code — happy to discuss.argument-hintfrom frontmatter is surfaced as the ACPinput_hint.load_skillMCP tool and the slash-command path. Theload_skilltool also accepts an optionalargsfield.Recipe slash commands
--flag value. Example:/migrate "Button Group" old-lib --to new-lib./weather Melbourne weatherworks without quoting).<required1> <required2> [--optional <optional>]and shown in the ACP command list.build_recipe_from_template_with_positional_paramshelper.slash_commands. Open question whether to move these to a per-project / discoverable location like skills — happy to discuss.Misc cleanup
split_quotedfromgoose-clito sharedgoose::utils::split_command_args; reused by stdio extension parsing, recipe args, and skill args. Now also tolerates apostrophes inside unquoted words (O'Reilly,don't).handle_skills_command(/skillslisting) now delegates toskill_slash_command::format_installed_skills.Testing
Unit Test
Manual testing in Desktop App
Created a simple client locally to test integrate with acp server
Open questions
argument-hint/argumentslive at the top level of frontmatter (Claude Code style) instead of undermetadata:?Next