feature: add granular builtin tool enablement #14525
feature: add granular builtin tool enablement #14525ashwinnathan-openai wants to merge 13 commits intomainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
ae06690 to
8f52730
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 149d68af53
ℹ️ 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".
f379010 to
443dce3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 443dce3c35
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
37561c6 to
7d31d45
Compare
|
@codex review |
codex-rs/app-server-protocol/schema/json/v2/ConfigReadResponse.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d31d45e75
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0acaecfbfd
ℹ️ 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".
| pub shell: Option<bool>, | ||
| pub filesystem: Option<bool>, | ||
| pub javascript: Option<bool>, | ||
| pub agents: Option<bool>, | ||
| pub agent_jobs: Option<bool>, | ||
| pub planning: Option<bool>, | ||
| pub user_input: Option<bool>, | ||
| pub web_search: Option<bool>, | ||
| pub image_generation: Option<bool>, | ||
| pub document_generation: Option<bool>, |
There was a problem hiding this comment.
I feel like these should have their own types with the associated configuration specific to each tool. None can mean "not enabled" so the corresponding configuration struct doesn't have to have its own enabled field perhaps?
There was a problem hiding this comment.
@pakrym-oai I'm particularly interested to get your thoughts on this bit!
There was a problem hiding this comment.
iiuc we can't do None = not enabled because in the disable_defaults = false case, if a thread override doesn't include tools.shell, we'd resolve that as tool_feature_overrides.shell = None and incorrectly treat it as disabled instead of inheriting from the lower layer.
I think we need Option<bool> for the three states:
None= inherit / no explicit overrideSome(true)= enabledSome(false)= disabled
Separately though open to having separate types with the associated config specific to each tool but don't know if that's premature atm.
| #[schemars(deny_unknown_fields)] | ||
| pub struct ToolsToml { | ||
| pub disable_defaults: Option<bool>, | ||
| pub shell: Option<ToolFeatureToml>, |
There was a problem hiding this comment.
Likewise, this should have the specific TOML shape we want to parse?
There was a problem hiding this comment.
As in create a specific shape for each tool like ShellFeatureToml? For now I just kept it generic as ToolFeatureToml which has enabled and if we end up adding feature specific configuration we should be able to evolve easily right?
codex-rs/core/src/tools/spec.rs
Outdated
| push_builtin_tool_spec_if_enabled( | ||
| &mut builder, | ||
| config, | ||
| "local_shell", |
There was a problem hiding this comment.
I also don't understand how this and others are not causing #14652 to fail....
19bc70c to
3e62ad3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb3dcc8da
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 800ffbe6e8
ℹ️ 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".
Why
This PR adds a new config surface for controlling built-in tool availability from
config.toolsand from per-thread config overrides.The final design uses grouped feature tables under
[tools.<feature>]. This composes cleanly with layered TOML, keeps per-feature config colocated with enablement, and gives SDK/app-server callers an explicit opt-in mode when they want to start from no built-in tools.What Changed
shellfilesystemjavascriptagentsagent_jobsplanninguser_inputweb_searchimage_generationdocument_generationtools.disable_defaultsas the top-level switch for explicit grouped mode.ToolFeature*wrappers withenabledWebSearchFeature*wrappers withenabledplus existing web-search configtools.view_imageweb_search = "cached" | "live" | "disabled"[tools] web_search = true|falseremains accepted and ignored, and the schema reflects thatInvariants
tools.disable_defaultsis omitted orfalse, Codex preserves the existing/default tool behavior and shipped legacy fallback knobs continue to participate.tools.disable_defaults = true, grouped[tools.<feature>]entries are the only source of truth for built-in tool exposure.web_search, top-level mode config controlsCached/Live/Disabled, but exposure still comes from[tools.web_search]whendisable_defaults = true.Testing
just write-config-schemajust write-app-server-schemajust fmtcargo test -p codex-app-server-protocolcargo test -p codex-corecargo test