fix!: migrate HTTP client from requests to httpx #52
Conversation
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
Replace the synchronous HTTP client library from `requests` to `httpx` for improved async support potential and modern API design. Changes include: - Replace `requests>=2.32.3` with `httpx>=0.28.0` in dependencies - Update `stackone_ai/models.py` to use httpx.request() instead of requests.request() - Refactor exception handling to use httpx-specific exceptions: - httpx.HTTPStatusError for HTTP error responses - httpx.RequestError for connection/network errors - Replace test mocking library from `responses` to `respx` - Remove `types-requests` type stubs (no longer needed) - Update all test files to use respx decorators and httpx.Response BREAKING CHANGE: Error handling now uses httpx exceptions instead of requests exceptions. Code catching RequestException should be updated to catch httpx.HTTPStatusError or httpx.RequestError.
ryoppippi
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the HTTP client library from requests to httpx to enable future async support and leverage a more modern API design. The migration includes updating all HTTP request handling, exception management, and test infrastructure.
Key Changes:
- Replace
requestslibrary withhttpxfor HTTP operations - Update exception handling to use httpx-specific exceptions (
HTTPStatusErrorandRequestError) - Migrate test mocking from
responsestorespxlibrary
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated dependencies: replaced requests>=2.32.3 with httpx>=0.28.0, responses>=0.25.8 with respx>=0.22.0, and removed types-requests |
| stackone_ai/models.py | Refactored HTTP client to use httpx.request() and updated exception handling to catch httpx-specific exceptions |
| tests/test_tool_calling.py | Migrated test mocks from responses to respx, updating mock setup and request body assertions |
| tests/test_models.py | Updated patched module from requests.request to httpx.request and added required mock response attributes |
| tests/test_meta_tools.py | Converted test mocking from responses to respx format |
| tests/test_feedback.py | Refactored all test mocks to use respx, simplified mock setup, and split combined test into separate test methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
256
to
269
| except httpx.HTTPStatusError as exc: | ||
| status = "error" | ||
| error_message = str(exc) | ||
| response_body = None | ||
| if exc.response.text: | ||
| try: | ||
| response_body = exc.response.json() | ||
| except json.JSONDecodeError: | ||
| response_body = exc.response.text | ||
| raise StackOneAPIError( | ||
| str(exc), | ||
| exc.response.status_code, | ||
| response_body, | ||
| ) from exc |
There was a problem hiding this comment.
The variables status and error_message are assigned but never used before raising the exception. These assignments should be removed to avoid confusion.
| assert error_result["status"] == "error" | ||
| assert "401 Client Error: Unauthorized" in error_result["error"] | ||
| error_result = next(r for r in result["results"] if r["account_id"] == "acc_unauthorized") | ||
| assert error_result["status"] == "error" |
There was a problem hiding this comment.
The error message assertion was removed from the test. The test should verify that the error result contains the expected error information to ensure proper error handling and reporting.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/test_feedback.py">
<violation number="1" location="tests/test_feedback.py:202">
P2: Test coverage reduced: the assertion verifying error message content was removed. The original test checked `assert "401 Client Error: Unauthorized" in error_result["error"]` to ensure error details were properly captured. Consider adding an assertion to verify that `error_result["error"]` is non-empty or contains expected error information.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| assert error_result["status"] == "error" | ||
| assert "401 Client Error: Unauthorized" in error_result["error"] | ||
| error_result = next(r for r in result["results"] if r["account_id"] == "acc_unauthorized") | ||
| assert error_result["status"] == "error" |
There was a problem hiding this comment.
P2: Test coverage reduced: the assertion verifying error message content was removed. The original test checked assert "401 Client Error: Unauthorized" in error_result["error"] to ensure error details were properly captured. Consider adding an assertion to verify that error_result["error"] is non-empty or contains expected error information.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_feedback.py, line 202:
<comment>Test coverage reduced: the assertion verifying error message content was removed. The original test checked `assert "401 Client Error: Unauthorized" in error_result["error"]` to ensure error details were properly captured. Consider adding an assertion to verify that `error_result["error"]` is non-empty or contains expected error information.</comment>
<file context>
@@ -55,165 +56,150 @@ def test_multiple_account_ids_validation(self) -> None:
- assert error_result["status"] == "error"
- assert "401 Client Error: Unauthorized" in error_result["error"]
+ error_result = next(r for r in result["results"] if r["account_id"] == "acc_unauthorized")
+ assert error_result["status"] == "error"
def test_tool_integration(self) -> None:
</file context>
✅ Addressed in 9b023ed
- Remove unused error_message variable in exception handlers - Restore error assertion in test_multiple_account_ids_mixed_success
glebedel
approved these changes
Dec 22, 2025
Resolved conflicts in: - pyproject.toml: Added new ty dependency from main - tests/test_meta_tools.py: Updated test URLs to use hibob while keeping httpx/respx approach - uv.lock: Regenerated to incorporate changes from both branches All tests passing after merge.
This was referenced Dec 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Migrate the HTTP client library from
requeststohttpxfor improved async support potential and modern API design. Also updates the test mocking library fromresponsestorespx.What Changed
requests>=2.32.3withhttpx>=0.28.0stackone_ai/models.pyto usehttpx.request()httpx.HTTPStatusErrorfor HTTP error responses (4xx/5xx)httpx.RequestErrorfor connection/network errorsresponsestorespxlibrarytypes-requests(no longer needed)Why
httpxprovides a more modern API with built-in async supportrespxintegrates seamlessly withhttpxfor testingBreaking Change
Error handling now uses httpx exceptions instead of requests exceptions. Code catching
RequestExceptionshould be updated to catchhttpx.HTTPStatusErrororhttpx.RequestError.Summary by cubic
Migrated the HTTP client from requests to httpx to enable async-ready usage and clearer error handling. Tests now use respx for HTTP mocking.
Refactors
Migration
Written for commit 9367d1d. Summary will update automatically on new commits.