[codex] Add user input client ids#24653
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
9fc5e62 to
3e25979
Compare
| pub enum UserInput { | ||
| Text { | ||
| #[serde(rename = "clientId", default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(rename = "clientId", optional)] |
There was a problem hiding this comment.
let's remove the skip_serializing_if = "Option::is_none" part for types exposed over app-server
There was a problem hiding this comment.
everything else about the annotations look good though
| #[serde(tag = "type", rename_all = "snake_case")] | ||
| pub enum UserInput { | ||
| Text { | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
this looks good in core protocol 👍
3e25979 to
f71bb24
Compare
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e25979d31
ℹ️ 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".
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| client_id: Option<String>, |
There was a problem hiding this comment.
Restore build compatibility for UserInput literals
Adding client_id as a required Rust field on the UserInput variants breaks existing call sites that this commit does not update; for example, codex-rs/cli/src/main.rs:1701 still constructs UserInput::LocalImage { path, detail: None } and :1704 constructs UserInput::Text without client_id, so crates using those literals will fail to compile. Either update all constructors or introduce a constructor/defaulted API that keeps call sites source-compatible.
Useful? React with 👍 / 👎.
| content.push(UserInput::Text { | ||
| client_id: None, |
There was a problem hiding this comment.
Preserve client IDs when replaying user messages
When a v2 turn includes clientId, the live ItemCompleted(UserMessage) carries it, but persisted history replay still ignores user-message item-completed events and rebuilds the user message from the legacy UserMessageEvent; hard-coding None here means thread/read, resume, or refresh drops every client ID after replay. Clients relying on these IDs to reconcile their submitted input items will only see them on the live notification path, so replay should use the structured item or persist the IDs in the legacy event.
Useful? React with 👍 / 👎.
| #[serde(rename = "clientId", default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(rename = "clientId", optional)] |
There was a problem hiding this comment.
Keep v2 clientId fields nullable on the wire
The app-server v2 guidance in AGENTS.md says not to use skip_serializing_if = "Option::is_none" for v2 payload fields; with this annotation, any UserInput returned in ItemStarted/ItemCompleted or thread responses will omit clientId when it is absent instead of serializing the stable nullable field shape used by the rest of the v2 API. Please model this as a nullable field on the wire (and align the TS annotation for request typing) rather than making it disappear.
Useful? React with 👍 / 👎.
| content.push(UserInput::Text { | ||
| client_id: None, |
There was a problem hiding this comment.
Preserve client IDs in live turn snapshots
For live app-server threads, ThreadState::track_current_turn_event also rebuilds the active turn through ThreadHistoryBuilder: the structured ItemCompleted(UserMessage) now has the client_ids, but the builder ignores that item type and later records the legacy UserMessageEvent, which lands here with None. As a result TurnCompletedNotification and other active-turn snapshots drop the client IDs even before any persisted replay, so clients that reconcile from the turn snapshot will lose the IDs despite receiving them on the individual item notification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
sorry, i realized this approach got quite invasive. can we try something like this instead?
// input
TurnStartParams { client_user_message_id: Option<String>, input: Vec<UserInput>, ... }
TurnSteerParams { client_user_message_id: Option<String>, input: Vec<UserInput>, ... }
// output
ThreadItem::UserMessage {
id: String,
client_id: Option<String>,
content: Vec<UserInput>,
}
|
I think the message-level ID is the right one, but there's some funky merging logic going on - we can improve with the fix below:
|
| } | ||
|
|
||
| #[test] | ||
| fn user_message_client_id_uses_camel_case_wire_field() { |
There was a problem hiding this comment.
nit: this kind of test is low value IMO, we can remove
| UserMessage { id: String, content: Vec<UserInput> }, | ||
| UserMessage { | ||
| id: String, | ||
| #[serde(default)] |
There was a problem hiding this comment.
nit: let's remove the serde(default) annotation here, not needed
e6eb304 to
6bb0032
Compare
Summary
Adds an optional
clientIdfield to app-server v2UserInputand carries it through the coreUserInputmodel so clients can correlate echoed user input items without relying on payload equality.Details
client_id: Option<String>to coreUserInputvariants.clientIdon the wire and in generated TypeScript.Validation
just fmtjust write-app-server-schemacargo test -p codex-app-server-protocolcargo test -p codex-protocoljust fix -p codex-app-server-protocoljust fix -p codex-protocolgit diff --check