Conversation
bolinfest
force-pushed
the
pr10349
branch
3 times, most recently
from
136c14d to
223cb55
Compare
bolinfest
force-pushed
the
pr10349
branch
2 times, most recently
from
56c918e to
c14c2ce
Compare
This was referenced Feb 2, 2026
bolinfest
force-pushed
the
pr10349
branch
2 times, most recently
from
cffa4bc to
dbb97b1
Compare
mzeng-openai
reviewed
Feb 2, 2026
codex-rs/core/src/mcp/codex_apps.rs
Outdated
| @@ -0,0 +1,119 @@ | |||
| use std::collections::HashMap; | |||
Contributor
There was a problem hiding this comment.
Can we defer these refactoring to a later PR so the diffs are just about replacing the mcp types?
mzeng-openai
reviewed
Feb 2, 2026
| const MCP_TOOL_NAME_PREFIX: &str = "mcp"; | ||
| const MCP_TOOL_NAME_DELIMITER: &str = "__"; | ||
| pub(crate) const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; | ||
| const CODEX_CONNECTORS_TOKEN_ENV_VAR: &str = "CODEX_CONNECTORS_TOKEN"; |
Contributor
There was a problem hiding this comment.
Same here, can we defer these refactoring to a later PR so the diffs are cleaner?
Collaborator
Author
There was a problem hiding this comment.
Ah, sorry about that. Codex and I had a misunderstanding: I'll back this out!
bolinfest
added a commit
that referenced
this pull request
Feb 2, 2026
Currently, types from our custom `mcp-types` crate are part of some of our APIs: https://github.com/openai/codex/blob/03fcd12e77fedf4fa327af27e2e476e1ebc5f651/codex-rs/app-server-protocol/src/protocol/v2.rs#L43-L46 To eliminate this crate in #10349 by switching to `rmcp`, we need our own wrappers for the `rmcp` types that we can use in our API, which is what this PR does. Note this PR introduces the new API types, but we do not make use of them until #10349. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/10356). * #10357 * #10349 * __->__ #10356
owenlin0
approved these changes
Feb 2, 2026
Collaborator
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
I'll give a heads up to our partners that this is coming, based on the PR description it looks like updating their clients hopefully won't be too arduous
| // this crate exports JSON schema + TS types (`schemars`/`ts-rs`), and the rmcp model types | ||
| // aren't set up to be schema/TS friendly (and would introduce heavier coupling to rmcp's Rust | ||
| // representations). Using `JsonValue` keeps the payload wire-shaped and easy to export. | ||
| pub content: Vec<JsonValue>, |
bolinfest
force-pushed
the
pr10349
branch
4 times, most recently
from
99abcc3 to
b1bfee9
Compare
mzeng-openai
approved these changes
Feb 2, 2026
Contributor
|
LFG! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We started working with MCP in Codex before https://crates.io/crates/rmcp was mature, so we had our own crate for MCP types that was generated from the MCP schema:
https://github.com/openai/codex/blob/8b95d3e082376f4cb23e92641705a22afb28a9da/codex-rs/mcp-types/README.md
Now that
rmcpis more mature, it makes more sense to use their MCP types in Rust, as they handle details (like the_metafield) that our custom version ignored. Though one advantage that our custom types had is that our generated types implementedJsonSchemaandts_rs::TS, whereas the types inrmcpdo not. As such, part of the work of this PR is leveraging the adapters betweenrmcptypes and the serializable types that are API for us (app server and MCP) introduced in #10356.Note this PR results in a number of changes to
codex-rs/app-server-protocol/schema, which merit special attention during review. We must ensure that these changes are still backwards-compatible, which is possible because we have:so
ContentBlockhas been replaced with the more generalJsonValue. Note thatContentBlockwas defined as:so the deletion of those individual variants should not be a cause of great concern.
Similarly, we have the following change in
codex-rs/app-server-protocol/schema/typescript/Tool.ts:so:
annotations?: ToolAnnotations➡️JsonValueinputSchema: ToolInputSchema➡️JsonValueoutputSchema?: ToolOutputSchema➡️JsonValueand two new fields:
icons?: Array<JsonValue>, _meta?: JsonValueStack created with Sapling. Best reviewed with ReviewStack.