Support disabling tool suggest for specific tools.#20072
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 712d4ce659
ℹ️ 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 let Some(response) = response.as_ref() { | ||
| maybe_persist_tool_suggest_disable(&session, &turn, &tool, response).await; | ||
| } |
There was a problem hiding this comment.
what happens if the elicitation action is Accept and persist = "always"? should we gate on action Decline?
There was a problem hiding this comment.
good catch, we don't need to persist accept.
I've also refactored the config shape since codex found out that creating ghost entries for apps and plugins can cause more side effects than I thought. Right now consolidating the disabled tool list under [tool_suggest]
There was a problem hiding this comment.
gotcha. is the list only created in user-level config, or does it map back to the config layer the tool comes from?
codex says: if the list always writes to user config, a list defined in a higher-precedence layer (like project-level) would clobber the user config list because they don't merge.
There was a problem hiding this comment.
That makes sense! I think it's more of a "how to merge" issue rather than config location thing, added logic to support merging the disabled tool list from higher precedence.
|
Made some small edits so that we can hot-reload any disabled_tools changes after the tool suggest elicitation. |
Summary
disable_tool_suggestto app and plugin config, schema, and TypeScript outputconfig.tomlTesting