Return the response body for 4xx errors#77
Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom Aug 28, 2025
Merged
Return the response body for 4xx errors#77openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom
openshift-merge-bot[bot] merged 1 commit intoopenshift-assisted:masterfrom
Conversation
In these cases the response should contain information that would be
good for the model to provide back to the user such as permissions or
validation failures.
Before the model would just say that it encountered an error without
more detail. In particular this will allow errors with the cluster name
to show the actual requirements.
Example log:
`2025-08-27 18:43:20,506 - root - ERROR - 140044886067008:5 - API error during create_cluster: Status: 400, Reason: Bad Request, Body: {"code":"400","href":"","id":400,"kind":"Error","reason":"Cluster name format is not valid: 'test_cluster'. Name must start and end with either a letter or number and consist of lower-case letters, numbers, and hyphens. Subdomains in cluster names are only valid with none platform."}`
WalkthroughThe decorator sanitize_exceptions now appends HTTP body details to 4xx ApiException messages. It logs and rethrows as AssistedServiceAPIError with "API error: Status {status}, Details: {body}" when a body exists. Tests were updated to construct ApiException with http_resp and assert the detailed message. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DecoratedFunc as Decorated API Call
participant API as Assisted Service API
participant Logger as Logger
Caller->>DecoratedFunc: invoke()
DecoratedFunc->>API: HTTP request
API-->>DecoratedFunc: 4xx response (body present)
Note over DecoratedFunc: ApiException raised
DecoratedFunc->>Logger: log(status, reason, body)
alt 4xx with body
DecoratedFunc-->>Caller: raise AssistedServiceAPIError("API error: Status {status}, Details: {body}")
else other errors
DecoratedFunc-->>Caller: raise AssistedServiceAPIError("An internal error occurred")
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
Bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service_client/exceptions.py (1)
50-53: Include reason and harden for bytes body / non-int status
- Some SDKs surface
e.bodyas bytes; this patch decodes with fallback to avoid raw bytes appearing in messages.- A defensive cast of
statusguards against non-integer values (e.g., unexpected strings) without altering any existing calls—our grep found only integer statuses passed internally.- Appending
e.reasonimproves diagnostics and won’t break current tests, since they assert on substrings.Apply this diff in service_client/exceptions.py (lines 50–53):
- error_msg = f"API error: Status {e.status}" - if e.status and 400 <= e.status <= 499 and e.body: - error_msg += f", Details: {e.body}" - raise AssistedServiceAPIError(error_msg) from e + status = e.status + try: + status_int = int(status) if status is not None else None + except (TypeError, ValueError): + status_int = None + + error_msg = f"API error: Status {status}" if status is not None else "API error" + if getattr(e, "reason", None): + error_msg += f" ({e.reason})" + + if status_int is not None and 400 <= status_int <= 499 and e.body: + body_text = ( + e.body.decode("utf-8", "replace") + if isinstance(e.body, (bytes, bytearray)) + else str(e.body) + ) + error_msg += f", Details: {body_text}" + raise AssistedServiceAPIError(error_msg) from etests/test_assisted_service_api.py (1)
196-221: Good coverage for 4xx body passthrough — consider adding bytes/5xx cases.
- Current test validates string body. Add one with bytes body to verify decoding.
- Add a 5xx-with-body test to ensure details are not appended outside 4xx.
Additional tests (placed near this block):
@pytest.mark.asyncio async def test_get_cluster_api_exception_bytes_body(client: InventoryClient) -> None: """4xx with bytes body should be decoded and included in the message.""" with patch.object(client, "_installer_api") as mock_installer_api: mock_api = Mock() mock_http_resp = Mock() mock_http_resp.status = 400 mock_http_resp.reason = "Bad Request" mock_http_resp.data = b'{"message":"bad name","code":"INVALID_NAME"}' mock_http_resp.getheaders.return_value = {} mock_api.v2_get_cluster.side_effect = ApiException(http_resp=mock_http_resp) mock_installer_api.return_value = mock_api with pytest.raises(AssistedServiceAPIError) as exc_info: await client.get_cluster("test-id") msg = str(exc_info.value) assert "API error: Status 400" in msg assert 'Details: {"message":"bad name","code":"INVALID_NAME"}' in msg @pytest.mark.asyncio async def test_get_cluster_api_exception_5xx_body_omitted(client: InventoryClient) -> None: """5xx responses should not include response body details.""" with patch.object(client, "_installer_api") as mock_installer_api: mock_api = Mock() mock_http_resp = Mock() mock_http_resp.status = 500 mock_http_resp.reason = "Internal Server Error" mock_http_resp.data = '{"message":"oops"}' mock_http_resp.getheaders.return_value = {} mock_api.v2_get_cluster.side_effect = ApiException(http_resp=mock_http_resp) mock_installer_api.return_value = mock_api with pytest.raises(AssistedServiceAPIError) as exc_info: await client.get_cluster("test-id") msg = str(exc_info.value) assert "API error: Status 500" in msg assert "Details:" not in msg
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
service_client/exceptions.py(2 hunks)tests/test_assisted_service_api.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_assisted_service_api.py (2)
service_client/exceptions.py (1)
AssistedServiceAPIError(15-16)service_client/assisted_service_api.py (1)
get_cluster(119-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (1)
service_client/exceptions.py (1)
29-31: Docstring update matches the new behavior — LGTM.
Clear and accurate about including 4xx response bodies in messages.
keitwb
approved these changes
Aug 27, 2025
|
@keitwb: changing LGTM is restricted to collaborators DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 5a1256e
into
openshift-assisted:master
15 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In these cases the response should contain information that would be good for the model to provide back to the user such as permissions or validation failures.
Before the model would just say that it encountered an error without more detail. In particular this will allow errors with the cluster name to show the actual requirements.
Example log:
2025-08-27 18:43:20,506 - root - ERROR - 140044886067008:5 - API error during create_cluster: Status: 400, Reason: Bad Request, Body: {"code":"400","href":"","id":400,"kind":"Error","reason":"Cluster name format is not valid: 'test_cluster'. Name must start and end with either a letter or number and consist of lower-case letters, numbers, and hyphens. Subdomains in cluster names are only valid with none platform."}Summary by CodeRabbit
Bug Fixes
Tests