-
Notifications
You must be signed in to change notification settings - Fork 6.3k
fix: revert "[app-server] fix account/read response annotation (#5642)" #5796
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
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".
| #[ts(rename = "account/read")] | ||
| GetAccount { | ||
| params: #[ts(type = "undefined")] #[serde(skip_serializing_if = "Option::is_none")] Option<()>, | ||
| response: GetAccountResponse, | ||
| response: Option<Account>, | ||
| }, |
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.
Restore export failure for account/read response
Changing GetAccount back to response: Option<Account> means export_client_responses will again call Option<Account>::export_all_to(...). ts_rs cannot export Option<T> as a standalone type, so the export binary will fail with CannotBeExported before any TypeScript files are generated. The reverted GetAccountResponse wrapper existed solely to represent Account | null while being exportable. With this change, running cargo run -p codex-app-server-protocol --bin export -- --out … will error out, making schema generation unusable.
Useful? React with 👍 / 👎.
Original commit changeset: 190e7eb
|
@codex review please |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Revert #5642 because this generates:
But
Accountis unknown.The unique use of
#[ts(export)]onGetAccountResponseis also suspicious as are the changes tocodex-rs/app-server-protocol/src/export.rssince the existing system has worked fine for quite some time.Though a pure backout of #5642 puts things in a state where, as the PR noted, the following does not work:
So in addition to the backout, this PR adds:
and changes
GetAccount.responseas follows:making it consistent with other types.
With this change, I verified that both of the following work:
The generated TypeScript is as follows:
and
Though while the inconsistency between
"type": "ApiKey"and"type": "chatgpt"is quite concerning, I'm not sure if that format is ever written to disk in any case, but @owenlin0, I would recommend looking into that.Also, it appears that the types in
codex-rs/protocol/src/account.rsare used exclusively by theapp-server-protocolcrate, so perhaps they should just be moved there?