-
Notifications
You must be signed in to change notification settings - Fork 6.6k
fix: remove serde(flatten) annotation for TurnError #7499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// For all other responses and notifications returning a Turn, | ||
| /// the items field will be an empty list. | ||
| pub items: Vec<ThreadItem>, | ||
| #[serde(flatten)] | ||
| pub status: TurnStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep Turn.status flat when serializing
The Turn struct now stores status as a normal field while TurnStatus is still internally tagged with status; without the previous #[serde(flatten)], any v2 Turn serialization will nest a second status key (e.g., { "status": { "status": "completed" }, "error": null }) instead of the expected top-level string shown in the commit description. That breaks the v2 API schema for every Turn response/notification, changing status from a string to an object and likely invalidating clients deserializing existing payloads.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(tag = "status", rename_all = "camelCase")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think codex' review is valid. you want to remove the tag in this line & below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codex the implementer has failed me
Hmm, is this a good choice for |
@bolinfest This is a fine contract for RPC calls, but this Turn object gets serialized out in notifications (and RPC calls like |
The problem with using
serde(flatten)on Turn status is that it conditionally serializes theerrorfield, which is not the pattern we want in API v2 where all fields on an object should always be returned.serializes to:
Instead we want: