[app-server] remove serde(skip_serializing_if = "Option::is_none") annotations #5939
Conversation
owenlin0
force-pushed
the
owen/clean_up_skip_serializing_if_none
branch
from
c2ac3f8 to
24a8b03
Compare
owenlin0
changed the title
owenlin0
force-pushed
the
owen/clean_up_skip_serializing_if_none
branch
from
02b292f to
534c5d1
Compare
owenlin0
force-pushed
the
owen/clean_up_skip_serializing_if_none
branch
from
534c5d1 to
c8186a4
Compare
| "params": { | ||
| "pageSize": null, | ||
| "cursor": null | ||
| } |
There was a problem hiding this comment.
FYI app-server can still take this as a valid request:
"method": "model/list",
"id": 6,
"params": {}
Even though this is what the struct serializes to in this test, this is independent from how codex app-server deserializes the incoming request.
| return RustProp(prop_name, serde_str) | ||
|
|
||
| if is_optional and serde_str: | ||
| ts_str = "#[ts(optional)]" |
There was a problem hiding this comment.
This actually makes our generated MCP protocol types match https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts
| id: String, | ||
| summary: Vec<ReasoningItemReasoningSummary>, | ||
| #[serde(default, skip_serializing_if = "should_serialize_reasoning_content")] | ||
| #[ts(optional = nullable)] |
There was a problem hiding this comment.
What is the difference between #[ts(optional = nullable)] and #[ts(optional)]? Just want to make sure this change was intended.
There was a problem hiding this comment.
#[ts(optional = nullable)] generates field?: T | null
whereas #[ts(optional)] generates field: T | null
| id: Option<String>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional = nullable)] | ||
| #[ts(optional)] |
There was a problem hiding this comment.
I'm curious why this field gets a #[ts(optional)] but other Option<String> fields in the same struct do not. Just want to make sure that's intended.
There was a problem hiding this comment.
oh this is a really good question
just checked, it looks like fields marked #[serde(skip_serializing)] will still be rendered in the TS definition, which is wrong... we need to omit those fields. I'll look into this. Good catch!
There was a problem hiding this comment.
(I only added #[ts(optional)] for fields with skip_serializing_if = "Option::is_none")
There was a problem hiding this comment.
updated so that fields with #[serde(skip_serializing)] also have #[ts(skip)]
We had this annotation everywhere in app-server APIs which made it so that fields get serialized as
field?: T, meaning if the field asNonewe would omit the field in the payload. Removing this annotation changes it so that we returnfield: T | nullinstead, which makes codex app-server's API more aligned with the convention of public OpenAI APIs like Responses.Separately, remove the
#[ts(optional_fields = nullable)]annotations that were recently added which made all the TS types becomefield?: T | nullwhich is not great since clients need to handle undefined and null.I think generally it'll be best to have optional types be either:
field: T | null(preferred, aligned with public OpenAI APIs)field?: Twhere we have to, such as types generated from the MCP schema: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/schema/2025-06-18/schema.ts (see changes tomcp-types/)I updated @etraut-openai's unit test to check that all generated TS types are one or the other, not both (so will error if we have a type that has
field?: T | null). I don't think there's currently a good use case for that - but we can always revisit.