fix: validate log containers before generated normalization - #228
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. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 15f785cd08
ℹ️ 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".
| transport = FakeLogsTransport() | ||
| payload = {"data": data, "limit": 25, "has_more": False, "total": 0} | ||
| transport.search_response = FakeResponse(200, payload, {}) | ||
| transport.activity_response = FakeResponse(200, payload, {}) | ||
| logs = logs_client(transport).logs |
There was a problem hiding this comment.
When the generated HTTP adapter receives {"data": {}}, both generated response models iterate the empty mapping as a zero-element collection and serialize it back as [], so the facade accepts the malformed response. This test injects the payload after generated deserialization via FakeLogsTransport, allowing its {} case to pass while the real client remains broken; run the row cases through make_client as well, as the metadata test does.
Useful? React with 👍 / 👎.
| transport = FakeLogsTransport() | ||
| transport.search_response = FakeResponse(200, payload, {}) | ||
| transport.activity_response = FakeResponse(200, payload, {}) | ||
| logs = logs_client(transport).logs | ||
| read = logs.search if operation == "search" else logs.activity |
There was a problem hiding this comment.
For an actual 200 response whose JSON body is [] or "invalid", the generated model's from_dict raises KeyError or ValueError before _response_values runs, rather than the asserted TypeError with Expected a complete log response. Injecting these values through FakeLogsTransport bypasses that parser and makes the test claim a consistent failure mode that real SDK users do not receive; exercise these cases with make_client and normalize the generated parse failures.
Useful? React with 👍 / 👎.
swkeever
changed the title
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2611d23f1
ℹ️ 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".
| def _validate_log_data(response: _RawHTTPResponse) -> None: | ||
| if response.status_code == HTTP_OK: | ||
| payload = GeneratedTransport._raw_response(response).payload | ||
| _ = response_data(response_values(payload)) |
There was a problem hiding this comment.
When a 200 response has a valid data list but omits limit or has_more for search, or total for activity, this check succeeds and the generated model then raises KeyError from its required-field pop() before the facade can produce the documented TypeError("Expected a complete log response"). Validate these required metadata fields at this raw-response boundary as well so malformed envelopes have the promised, consistent failure mode.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
The generated HTTP adapter could turn an invalid
data: {}response into an empty list, or raise inconsistent errors for missing metadata. Validate response envelopes, row collections, and required metadata before generated deserialization, using the same checks as the facade. Retain generated model normalization for valid responses, including timestamps.Run 40 malformed-response cases through both the facade fixture and real HTTP adapter (80 tests), including each missing required field. Non-object bodies, invalid row containers, and malformed metadata consistently raise
TypeError. Document the response requirements.Validation: 22 container cases and three missing-metadata cases failed through the native HTTP adapter before their fixes.
uv run poe qualitypassed with 1,436 tests, strict typing, generator comparison, contract-binding checks, and wheel/sdist consumer tests. The targeted suite passes 108 tests with 100% line and branch coverage for the logs facade and shared response validation. Hosting's contract/harness unit tests passed; shared scenarios are unchanged.