Open
Conversation
RafaelJohn9
requested changes
Mar 7, 2026
Member
There was a problem hiding this comment.
Hey @watersRand can we make the Commit message more informative of what we want to be achieved ?
The PR was never merged thus is hard to understand what this commit does without referencing a GitHub PR
And we also have some failing checks
Member
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Just let me know! 🐇 |
Member
|
@coderabbitai Review the PR |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughAdded retry and idempotency handling to the async HTTP client, expanded Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant AsyncClient as MpesaAsyncHttpClient
participant Retry as Tenacity (retry wrapper)
participant HTTPX as httpx.AsyncClient
participant MPESA as Mpesa API
Client->>AsyncClient: post(url, json, headers?, timeout, idempotent?)
alt idempotent == true
AsyncClient->>Retry: _retryable_post(...)
Retry->>HTTPX: _raw_post (X-Idempotency-Key ensured)
HTTPX->>MPESA: HTTP POST
MPESA-->>HTTPX: response
HTTPX-->>Retry: response
Retry-->>AsyncClient: response
else
AsyncClient->>HTTPX: _raw_post(...)
HTTPX->>MPESA: HTTP POST
MPESA-->>HTTPX: response
HTTPX-->>AsyncClient: response
end
AsyncClient->>AsyncClient: handle_request_error(response)
AsyncClient-->>Client: response.json() or raise MpesaApiException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/http_client/test_mpesa_async_http_client.py (1)
162-174: Assert the retry attempt count in these GET failure tests.These assertions only pin the final translated exception. They still pass if
_raw_get()stops retrying but preserves the last error mapping. Add anawait_count == 3assertion to matchstop_after_attempt(3)so the new retry contract is actually covered.Also applies to: 178-190, 194-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/http_client/test_mpesa_async_http_client.py` around lines 162 - 174, The test only checks the final mapped MpesaApiException but not that retries occurred; update test_get_timeout (and the other similar tests at the referenced ranges) to assert the mocked request was awaited the expected number of times by adding an assertion like async_client._client.get.await_count == 3 after the existing exception assertions to match stop_after_attempt(3); apply the same await_count == 3 check in the two other GET failure tests mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 50-57: The POST retry policy risks duplicate transactions; update
the retry behavior in _raw_post (and its decorator/usage) so POST requests do
not automatically retry on network timeouts unless the caller explicitly opts
into idempotency/deduplication; specifically, change the retry predicate used on
_raw_post to only retry for safe/idempotent methods or when an idempotency
token/header is provided, and leave _raw_get unchanged; ensure callers like the
STK Push flow (stk_push.py processrequest call) either disable retries or pass
an idempotency key before allowing retries.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 164-183: The test is calling MpesaHttpClient.post("/test")
directly which bypasses the StkPush flow; instead construct a StkPush (and
AsyncStkPush for the other test) using a mocked token manager and the real
client, patch the underlying httpx.Client.post used by StkPush to return the
mocked response, then loop 100 times calling StkPush.push() so the token lookup,
Authorization header construction, request serialization and service-layer reuse
are exercised; ensure you reference StkPush.push (and AsyncStkPush.push) and the
token manager mock when updating the test and assert both the push success count
and the mocked httpx post call_count == 100.
---
Nitpick comments:
In `@tests/unit/http_client/test_mpesa_async_http_client.py`:
- Around line 162-174: The test only checks the final mapped MpesaApiException
but not that retries occurred; update test_get_timeout (and the other similar
tests at the referenced ranges) to assert the mocked request was awaited the
expected number of times by adding an assertion like
async_client._client.get.await_count == 3 after the existing exception
assertions to match stop_after_attempt(3); apply the same await_count == 3 check
in the two other GET failure tests mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: acaf4488-58de-4e2b-9858-ce73371b628f
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/http_client/test_mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
79-91: Remove duplicated POST transport logic in_retryable_post.
_retryable_post()duplicates_raw_post()and can drift over time. Delegate to_raw_post()so retry behavior wraps a single transport implementation.♻️ Proposed refactor
async def _retryable_post( self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10, ) -> httpx.Response: - return await self._client.post( - url, - json=json, - headers=headers, - timeout=timeout, - ) + return await self._raw_post(url=url, json=json, headers=headers, timeout=timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, _refiable_post currently duplicates the HTTP transport by directly calling self._client.post; change it to delegate to the single transport implementation _raw_post so retries wrap one place. Replace the body of _retryable_post to call and return await self._raw_post(url=url, json=json, headers=headers, timeout=timeout) (preserve the same parameters and return type httpx.Response) so all POST logic lives in _raw_post and retry behavior uses that single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 113-117: The code mutates the caller-provided headers dict when
adding an idempotency key, which can leak the key into subsequent requests; fix
by making a shallow copy of headers before any modification (e.g., assign
headers = dict(headers) when headers is not None or headers = {} otherwise) and
then add the "X-Idempotency-Key" (using uuid.uuid4()) to that copy so the
original dict passed into the method remains unchanged; update the logic around
the idempotent check and "X-Idempotency-Key" assignment (referencing the headers
variable, the idempotent flag, and uuid.uuid4()) to use the copied headers.
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 164-210: The test test_stk_push_multiple_times should ensure
MpesaHttpClient is closed even on early failures: wrap the loop/body that
constructs MpesaHttpClient(env="sandbox", use_session=use_session) and uses stk
= StkPush(http_client=client, token_manager=mock_token_manager) in a try/finally
(or use a context manager) and call client.close() (and if use_session=True
ensure any underlying session is closed) in the finally block so that the httpx
client/session is cleaned up regardless of assertion errors; apply the same
pattern to the other similar test at lines 341-386.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: _refiable_post currently duplicates the HTTP transport by
directly calling self._client.post; change it to delegate to the single
transport implementation _raw_post so retries wrap one place. Replace the body
of _retryable_post to call and return await self._raw_post(url=url, json=json,
headers=headers, timeout=timeout) (preserve the same parameters and return type
httpx.Response) so all POST logic lives in _raw_post and retry behavior uses
that single implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67c4458d-11a3-4571-b9b8-d15414df130c
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/http_client/test_mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/http_client/test_mpesa_async_http_client.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)
79-91: Have_retryable_post()delegate to_raw_post().The retryable and non-retryable POST paths currently duplicate the same low-level request construction. Reusing
_raw_post()keeps that behavior in one place and avoids the two paths drifting.Suggested refactor
async def _retryable_post( self, url: str, json: Dict[str, Any], headers: Dict[str, str], timeout: int = 10, ) -> httpx.Response: - return await self._client.post( - url, - json=json, - headers=headers, - timeout=timeout, - ) + return await self._raw_post(url, json, headers, timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, _refactor:_retryable_post currently duplicates the low-level request logic; change it to call and return self._raw_post(...) instead of calling self._client.post directly. Pass the same parameters (url, json, headers, timeout) through to _raw_post so the retryable path reuses the centralized request construction in _raw_post (ensure the return type remains httpx.Response).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 115-116: The branch that sets an idempotency key can add a
duplicate header if the caller provided a differently-cased key; change the
check in the idempotent handling (where `idempotent` and `headers` are used in
mpesa_async_http_client.py) to look for any header whose lowercased name equals
"x-idempotency-key" (e.g., any(k.lower() == "x-idempotency-key" for k in
headers)) and only generate and insert a new `X-Idempotency-Key` when no such
key exists; keep the canonical "X-Idempotency-Key" casing when inserting so
downstream servers see a consistent header name.
In `@mpesakit/mpesa_express/schemas.py`:
- Line 92: The schema contains a literal Base64-looking password
"bXlwYXNzd29yZA==" with a "#nosec B105" suppression; remove the suppression and
replace the value for the "Password" field with a clearly fake placeholder (e.g.
"BASE64_PASSWORD_PLACEHOLDER" or "<PASSWORD_PLACEHOLDER>") so it no longer
resembles credentials, and make the same replacement for the other occurrence
referenced (the second "Password" example). Ensure you do not reintroduce any
secret-suppression comments.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: _refactor:_retryable_post currently duplicates the
low-level request logic; change it to call and return self._raw_post(...)
instead of calling self._client.post directly. Pass the same parameters (url,
json, headers, timeout) through to _raw_post so the retryable path reuses the
centralized request construction in _raw_post (ensure the return type remains
httpx.Response).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0433efa-38e7-494e-8986-09d0ba850e8a
📒 Files selected for processing (3)
mpesakit/http_client/mpesa_async_http_client.pympesakit/mpesa_express/schemas.pytests/unit/mpesa_express/test_stk_push.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/mpesa_express/test_stk_push.py
| "example": { | ||
| "BusinessShortCode": 654321, | ||
| "Password": "bXlwYXNzd29yZA==", | ||
| "Password": "bXlwYXNzd29yZA==", #nosec B105 |
Contributor
There was a problem hiding this comment.
Replace the secret-looking sample instead of suppressing the warning.
These literals still look like credentials to secret scanners, so #nosec B105 only hides the Bandit finding while Betterleaks keeps flagging both examples. Use a clearly fake placeholder here instead so the schema docs stay useful without normalizing secret-leak suppressions.
Suggested change
- "Password": "bXlwYXNzd29yZA==", `#nosec` B105
+ "Password": "<generated-from-shortcode-passkey-timestamp>",Also applies to: 502-502
🧰 Tools
🪛 Betterleaks (1.1.1)
[high] 92-92: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mpesakit/mpesa_express/schemas.py` at line 92, The schema contains a literal
Base64-looking password "bXlwYXNzd29yZA==" with a "#nosec B105" suppression;
remove the suppression and replace the value for the "Password" field with a
clearly fake placeholder (e.g. "BASE64_PASSWORD_PLACEHOLDER" or
"<PASSWORD_PLACEHOLDER>") so it no longer resembles credentials, and make the
same replacement for the other occurrence referenced (the second "Password"
example). Ensure you do not reintroduce any secret-suppression comments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
mpesakit/http_client/mpesa_async_http_client.py (2)
79-91: Inconsistent indentation in method signature and body.The method parameters and return statement have non-standard indentation that differs from the rest of the file.
♻️ Suggested formatting fix
async def _retryable_post( - self, - url: str, - json: Dict[str, Any], - headers: MutableMapping[str, str], - timeout: int = 10, -) -> httpx.Response: - return await self._raw_post( + self, + url: str, + json: Dict[str, Any], + headers: MutableMapping[str, str], + timeout: int = 10, + ) -> httpx.Response: + return await self._raw_post( url, json, headers, timeout, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 79 - 91, The _retryable_post method has inconsistent indentation for its signature and the return statement; adjust the function signature and body to match the project's style (align parameters under the opening parenthesis or use a single-line signature) and indent the return statement one level inside the method body, making the call to _raw_post consistently formatted (refer to _retryable_post and the call to _raw_post) so the parameters and closing parenthesis line up with the rest of the file.
3-24: Redundant exception types in retry condition.
httpx.ConnectTimeoutandhttpx.ReadTimeoutare subclasses ofhttpx.TimeoutException, so listing them explicitly is redundant. Including onlyhttpx.TimeoutExceptionandhttpx.ConnectErrorwould be sufficient and clearer.♻️ Suggested simplification
mpesa_retry_condition = retry_if_exception_type( - (httpx.ConnectError, httpx.ConnectTimeout,httpx.TimeoutException ,httpx.ReadTimeout) + (httpx.ConnectError, httpx.TimeoutException) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpesakit/http_client/mpesa_async_http_client.py` around lines 3 - 24, The retry condition mpesa_retry_condition currently lists httpx.ConnectTimeout and httpx.ReadTimeout along with httpx.TimeoutException, which is redundant because those classes subclass httpx.TimeoutException; simplify the retry_if_exception_type call used to build mpesa_retry_condition to include only httpx.ConnectError and httpx.TimeoutException (remove explicit ConnectTimeout and ReadTimeout) so the intent is clearer and behavior remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/mpesa_express/test_stk_push.py`:
- Around line 344-386: The test redundantly calls await client.aclose() after
using the context manager; remove the explicit await client.aclose() call at the
end of the test so the MpesaAsyncHttpClient context manager (its __aexit__)
handles closing automatically—locate the test block that creates
MpesaAsyncHttpClient(), AsyncStkPush, and the loop (references:
MpesaAsyncHttpClient, AsyncStkPush, mock_async_token_manager) and delete the
final await client.aclose() statement.
---
Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 79-91: The _retryable_post method has inconsistent indentation for
its signature and the return statement; adjust the function signature and body
to match the project's style (align parameters under the opening parenthesis or
use a single-line signature) and indent the return statement one level inside
the method body, making the call to _raw_post consistently formatted (refer to
_retryable_post and the call to _raw_post) so the parameters and closing
parenthesis line up with the rest of the file.
- Around line 3-24: The retry condition mpesa_retry_condition currently lists
httpx.ConnectTimeout and httpx.ReadTimeout along with httpx.TimeoutException,
which is redundant because those classes subclass httpx.TimeoutException;
simplify the retry_if_exception_type call used to build mpesa_retry_condition to
include only httpx.ConnectError and httpx.TimeoutException (remove explicit
ConnectTimeout and ReadTimeout) so the intent is clearer and behavior remains
the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c3a8dac-3760-47f6-97db-b076d9ba3188
📒 Files selected for processing (2)
mpesakit/http_client/mpesa_async_http_client.pytests/unit/mpesa_express/test_stk_push.py
Comment on lines
+344
to
+386
| async with MpesaAsyncHttpClient() as client: | ||
| stk = AsyncStkPush(http_client =client,token_manager=mock_async_token_manager) | ||
|
|
||
| request_data = StkPushSimulateRequest( | ||
| BusinessShortCode=174379, | ||
| Amount=1, | ||
| PhoneNumber="254700000000", | ||
| CallBackURL="https://example.com/callback", | ||
| AccountReference="TestRef", | ||
| TransactionDesc="TestDesc", | ||
| TransactionType="CustomerPayBillOnline", | ||
| PartyA="254700000000", | ||
| PartyB="174379", | ||
| Timestamp="20231010120000", | ||
| Password="base64_encoded_password" | ||
| ) | ||
|
|
||
|
|
||
| mock_response = MagicMock(spec=httpx.Response) | ||
| mock_response.is_success = True | ||
| mock_response.status_code = 200 | ||
| mock_response.json.return_value = { | ||
| "MerchantRequestID": "12345", | ||
| "CheckoutRequestID":"ws_CO_260520211133524545", | ||
| "ResponseDescription":"Test Description", | ||
| "CustomerMessage": "Success", | ||
| "ResponseCode": "0"} | ||
|
|
||
| with patch.object(mock_async_token_manager, "get_token", AsyncMock(return_value="mock_token")): | ||
| with patch.object(httpx.AsyncClient, "send", AsyncMock(return_value=mock_response)) as mock_send: | ||
| success_count = 0 | ||
|
|
||
| for _ in range(100): | ||
| result = await stk.push( | ||
| request=request_data | ||
| ) | ||
| if str(result.ResponseCode) == "0": | ||
| success_count += 1 | ||
|
|
||
| assert success_count == 100 | ||
| assert mock_send.call_count == 100 | ||
|
|
||
| await client.aclose() |
Contributor
There was a problem hiding this comment.
Remove redundant aclose() call inside async with block.
The async with MpesaAsyncHttpClient() context manager already calls aclose() in __aexit__. The explicit call on line 386 is redundant and results in double-closing the client.
🐛 Proposed fix
assert success_count == 100
assert mock_send.call_count == 100
- await client.aclose()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/mpesa_express/test_stk_push.py` around lines 344 - 386, The test
redundantly calls await client.aclose() after using the context manager; remove
the explicit await client.aclose() call at the end of the test so the
MpesaAsyncHttpClient context manager (its __aexit__) handles closing
automatically—locate the test block that creates MpesaAsyncHttpClient(),
AsyncStkPush, and the loop (references: MpesaAsyncHttpClient, AsyncStkPush,
mock_async_token_manager) and delete the final await client.aclose() statement.
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
2 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.
Description
This PR is a follow-up on PR #127.
It aligns the
MpesaAsyncHttpClientwith the synchronous client to support unified retry functionality using thetenacitylogic.Key Changes:
@retrydecorators inMpesaAsyncHttpClientmatching the sync implementation.requests.SessionPerformance test when and without using it. #116: Added simulation for 100 STK pushes to verify performance and session handling._raw_postand_raw_getmethods to handle low-level exceptions.Type of Change
How Has This Been Tested?
tests/unit/http_client/pass.httpx.AsyncClientsession.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation