Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2281b13a7f
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5d570761d
ℹ️ 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".
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Nice: I'm excited to get this in!
Similar to what @sayan-oai did in #8956 for `config.schema.json`, this PR updates the repo so that it includes the output of `codex app-server generate-json-schema` and `codex app-server generate-ts` and adds a test to verify it is in sync with the current code. Motivation: - This makes any schema changes introduced by a PR transparent during code review. - In particular, this should help us catch PRs that would introduce a non-backwards-compatible change to the app schema (eventually, this should also be enforced by tooling). - Once #10231 is in to formalize the notion of "experimental" fields, we can work on ensuring the non-experimental bits are backwards-compatible. `codex-rs/app-server-protocol/tests/schema_fixtures.rs` was added as the test and `just write-app-server-schema` can be use to generate the vendored schema files. Incidentally, when I run: ``` rg _ codex-rs/app-server-protocol/schema/typescript/v2 ``` I see a number of `snake_case` names that should be `camelCase`.
jif-oai
force-pushed
the
jif/experimental
branch
from
34ca142 to
bb0c923
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ 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". |
2 participants
## Problem being solved
Right now that’s easy to drift or miss when done ad-hoc.
How to declare experimental methods and fields
#[experimental("method/name")]to theClientRequestvariant inclient_request_definitions!.ExperimentalApiand annotate the field with#[experimental("method/name.field")]+ setinspect_params: truefor the method variant soClientRequest::experimental_reason()inspects params for experimental fields.How the macro solves it
codex-rs/codex-experimental-api-macros/src/lib.rsand is used via#[derive(ExperimentalApi)]plus#[experimental("reason")]attributes.ExperimentalApi::experimental_reason(&self)that checks only annotated fields.Option<T>:is_some_and(...)recursively checks inner.Vec/HashMap/BTreeMap: must be non-empty.bool: must betrue.true).inventorywith(type_name, serialized field name, reason)and exposesEXPERIMENTAL_FIELDSfor that type. Field names are converted fromsnake_casetocamelCasefor schema/TS filtering.matchreturningSome(reason)for annotated variants andNoneotherwise (no wildcard arm).ExperimentalApi::experimental_reason()incodex-rs/app-server/src/message_processor.rsto reject requests unlessInitializeParams.capabilities.experimental_api == true.EXPERIMENTAL_CLIENT_METHODSfromclient_request_definitions!to strip experimental methods/fields whenexperimental_apiis false.