Expose explicit dynamic tool namespaces in thread start#27371
Conversation
1765bf7 to
acca13f
Compare
0501a07 to
e0bb3c7
Compare
| #[derive(Serialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] |
There was a problem hiding this comment.
lmk if this is an antipattern and we'd rather define the types in app-server and protocol, i know we duplicate other types.
|
|
||
| /// Exercises the full dynamic tool call path (server request, client response, model output). | ||
| #[tokio::test] | ||
| async fn dynamic_tool_call_round_trip_sends_text_content_items_to_model() -> Result<()> { |
There was a problem hiding this comment.
main happy path integration test; updated with new types
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f2b5e0a6f
ℹ️ 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 this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f2b5e0a6f
ℹ️ 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".
ab8c1e5 to
c856ed2
Compare
9f2b5e0 to
8ed9281
Compare
8ed9281 to
b2bee78
Compare
jif-oai
left a comment
There was a problem hiding this comment.
Can we add a thread/start integration test for duplicate namespace containers? Just to lock the behaviour explicitly
b2bee78 to
1b4b9a6
Compare
Follow-up to #27356. ## Stack note This PR changes Codex's internal dynamic-tool shape while leaving `thread/start` unchanged. App-server therefore converts the existing per-tool input into explicit functions and namespaces before passing it to core. [#27371](#27371) updates `thread/start` to use the same explicit shape and removes this temporary conversion. ## Why Dynamic tools repeat namespace metadata on every function. Core should keep one explicit namespace with its member tools so descriptions and membership stay consistent across sessions and runtime planning. ## What changed - Represent dynamic tools as top-level functions or explicit namespaces in protocol and session state. - Read old flat rollout metadata and write the canonical hierarchy. - Flatten namespace members only when registering callable tools. - Keep `thread/start.dynamicTools` flat for now and normalize it at the app-server boundary. New builds can read old rollout metadata. Older builds cannot read newly written hierarchical metadata. ## Test plan - `just test -p codex-app-server thread_start_normalizes_legacy_dynamic_tools_into_model_request` - `just test -p codex-protocol session_meta_normalizes_legacy_dynamic_tools` - `just test -p codex-core resume_restores_dynamic_tools_from_rollout_with_sqlite_enabled` - `just test -p codex-core tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call` - `just test -p codex-core code_mode_can_call_hidden_dynamic_tools` - `just test -p codex-tools`
1b4b9a6 to
fe2b08e
Compare
Stacked on #27365.
Stack note
#27365 kept
thread/startunchanged and converted its input inthread_processor. This PR updatesthread/startto accept explicit functions and namespaces directly.Legacy per-tool arrays are still accepted and converted while reading the request. As a result,
thread_processorcan validate and pass the tools through directly, which is why some code added in #27365 is removed here.Why
thread/start.dynamicToolsstill repeats namespace data on each function even though core now stores explicit namespace groups. The request API should use the same shape so each namespace has one description and one member list.What changed
dynamicTools.exposeToContext.Test plan
just test -p codex-app-server-protocoljust test -p codex-app-server dynamic_tool_call_round_trip_sends_text_content_items_to_modeljust test -p codex-app-server thread_start_normalizes_legacy_dynamic_tools_into_model_requestjust test -p codex-app-server thread_start_rejects_mixed_dynamic_tool_formatsjust test -p codex-app-server thread_start_rejects_hidden_dynamic_tools_without_namespace