Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 2b803d2ed5
ℹ️ 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".
| # @param [Mixed] Data to be matched against the list of oneOf items | ||
| # @return [Object] Returns the model or the data itself | ||
| def build(data) | ||
| discriminator_value = data[openapi_discriminator_name] |
There was a problem hiding this comment.
When CreateFrontendCustomDomainRequest.build_from_hash receives a normal JSON-parsed hash, the nested TLS object has a string key such as "mode", but this lookup uses only the symbol :mode. Both managed and BYOC inputs therefore produce nil, after which the request model's required tls setter raises ArgumentError; normalize the nested keys or check both string and symbol forms before dispatching to the concrete TLS model.
Useful? React with 👍 / 👎.
| example: handler | ||
| is_public: | ||
| type: boolean | ||
| default: false |
There was a problem hiding this comment.
When this upsert operation targets an existing public function and the caller omits is_public, the repository's request validator injects schema defaults, so this declaration turns the omitted field into false and can make the function private during an otherwise ordinary code redeploy. The operation explicitly updates existing functions, and the generated Ruby client leaves this optional field out unless supplied; remove the schema default and apply false only on the create path so updates can distinguish omission from an explicit visibility change.
Useful? React with 👍 / 👎.
2b803d2 to
c48f716
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48f716d2f
ℹ️ 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".
| freeze_oauth_api_data(data) | ||
| rescue KeyError, NoMethodError | ||
| raise TypeError, 'Expected OAuth provider API response data' | ||
| raise TypeError, 'Expected OAuth provider API response data' unless response_body.is_a?(Hash) |
There was a problem hiding this comment.
When the provider returns a top-level JSON array—as the documented GitHub /user/repos endpoint normally does—this Hash-only check raises TypeError for custom transports, while the generated transport's new Hash<String, Object> return type converts the array into a malformed hash with array elements as keys and nil values. The previous implementation deliberately returned and recursively froze any JSON value from data; the raw-response contract and facade must likewise support arrays rather than requiring an object.
Useful? React with 👍 / 👎.
| if attributes.key?(:'openapi_spec') | ||
| if (value = attributes[:'openapi_spec']).is_a?(Hash) | ||
| self.openapi_spec = value | ||
| end |
There was a problem hiding this comment.
When a caller follows the schema's instruction to clear existing metadata with UpdateFunctionRequest.new(openapi_spec: nil), this Hash guard never assigns @openapi_spec. The model's to_hash intentionally omits nullable attributes whose instance variable was never defined, so the request serializes as {} instead of {openapi_spec: nil} and the server retains the old OpenAPI document. Explicit nil must be assigned so omission remains distinguishable from clearing.
Useful? React with 👍 / 👎.
| - is_public | ||
| - invocation_mode | ||
| - http_auth_mode | ||
| - openapi_spec |
There was a problem hiding this comment.
The same schema says that function list responses omit the OpenAPI document, but PaginatedFunctions.data references Function and this required entry makes every such item invalid unless it contains an openapi_spec key (even when its value is null). Response validators and stricter generated clients will therefore reject the documented list payload; use a list-item schema without the document or make this property optional.
Useful? React with 👍 / 👎.
| description: | | ||
| Names what to restore. Supply exactly one of `backup_name` or | ||
| `restore_to`. | ||
| properties: |
There was a problem hiding this comment.
Although the request contract requires exactly one target, the schema makes both backup_name and restore_to independently optional and supplies no oneOf constraint. Consequently the generated CreateDatabaseRestoreRequest#valid? accepts both {} and requests containing both fields, even though the endpoint documents that either input is guaranteed to return 400; encode the exclusivity in the schema so generated clients can construct and validate only usable restore requests.
Useful? React with 👍 / 👎.
c48f716 to
7da01ec
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ 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". |
7da01ec to
8f90941
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f90941819
ℹ️ 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".
| required: | ||
| - mode |
There was a problem hiding this comment.
When a project-config apply includes only one half of a BYOC rotation, such as {mode: "byoc", certificate_pem: "..."}, this schema and the generated BYOCProjectConfigFrontendCustomDomainTLSConfig#valid? accept it even though a usable rotation requires the corresponding private key. Mode-only must remain valid for secret-free exports and unchanged existing domains, but the schema should constrain certificate material to either be entirely absent or include both certificate_pem and private_key_pem, so clients reject partial rotations before the server returns a validation error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9bf0ba. The project-config schema now rejects partial certificate material while still allowing mode-only exports, and the Ruby adapter enforces the same pair invariant because the generator does not emit conditional-schema validation.
| Given('a managed custom-domain TLS request') do | ||
| generated = Volcano.const_get(:Generated, false) | ||
| tls = generated::ManagedFrontendCustomDomainTLSConfig.new(mode: 'managed') | ||
| @contract.managed_tls_request = generated::CreateFrontendCustomDomainRequest.new( |
There was a problem hiding this comment.
When the new managed-TLS feature is actually executed rather than only checked with Cucumber's --dry-run, this assignment immediately raises NoMethodError because VolcanoContract::World defines no managed_tls_request= accessor; the subsequent managed_tls_wire and managed_tls_response assignments are missing accessors as well. Add these fields to World or keep this scenario's state in step-local instance variables so the new contract scenario can run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9bf0ba. The contract world now owns all three managed TLS scenario fields; the focused scenario executes successfully, not only in dry-run discovery.
8f90941 to
d9bf0ba
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
This branch has not been deployed
1 participant
Summary
Verification
bin/check-openapibundle exec rubocop --parallelbundle exec rspecgem build volcano-sdk.gemspecPaired with https://github.com/Kong/volcano-hosting/pull/958
VOL-314