Skip to content

A followup to pr #127#128

Open
watersRand wants to merge 5 commits intoByte-Barn:masterfrom
watersRand:test/stk-push-limit
Open

A followup to pr #127#128
watersRand wants to merge 5 commits intoByte-Barn:masterfrom
watersRand:test/stk-push-limit

Conversation

@watersRand
Copy link
Copy Markdown
Contributor

@watersRand watersRand commented Feb 25, 2026

Description

This PR is a follow-up on PR #127.
It aligns the MpesaAsyncHttpClient with the synchronous client to support unified retry functionality using the tenacity logic.

Key Changes:

Type of Change

  • Refactor (aligned async retry logic with sync client)
  • Tests (added 100 STK push simulation and retry path tests)

How Has This Been Tested?

  • Unit Tests: All existing tests in tests/unit/http_client/ pass.
  • Simulation: Verified 100 successful STK pushes using a persistent httpx.AsyncClient session.

Checklist

  • My code follows the project's coding style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features

    • Idempotent POSTs now support automatic retries with idempotency-key injection; GET/POST accept optional headers and configurable timeouts; responses returned as parsed JSON.
    • Centralized HTTP error/retry handling shared across clients.
  • Bug Fixes

    • No retries for client (4xx) errors; clearer mapping of connection/timeout failures to explicit error messages.
  • Tests

    • Stress tests for repeated STK Push; tests for idempotency persistence, retry limits, and updated error-message expectations.
  • Documentation

    • Example payloads updated to use a placeholder for generated password.

Copy link
Copy Markdown
Member

@RafaelJohn9 RafaelJohn9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RafaelJohn9
Copy link
Copy Markdown
Member

@coderabbitai

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 7, 2026

Tip

For best results, initiate chat on the files or code changes.

@RafaelJohn9 It looks like your message may have been sent without the full content. How can I assist you? Here are a few things I can help with:

  • 🔍 Review the PR — I can perform a full review of the changes.
  • 🐛 Investigate a specific concern — Share the details and I'll dig in.
  • 📋 Summarize the changes — I can regenerate the PR summary.
  • 🛠️ Help with code changes — Describe what you'd like updated.

Just let me know! 🐇

@RafaelJohn9
Copy link
Copy Markdown
Member

@coderabbitai Review the PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 8, 2026

@RafaelJohn9 Sure! I'll perform a full review of PR #128 now. 🐇

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 8, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added Tenacity-based retry and idempotency handling to the async HTTP client, expanded post/get signatures (optional headers, timeout, idempotent flag), factored raw/retryable request helpers, consolidated error-to-exception mapping, moved shared retry/error utilities to a common module, and added/updated tests and schema examples.

Changes

Cohort / File(s) Summary
Async HTTP client
mpesakit/http_client/mpesa_async_http_client.py
Added module logger, UUID idempotency key injection, _raw_post/_raw_get helpers, _retryable_post (Tenacity wrapper), mpesa_retry_condition/retry usage, expanded post/get signatures (optional headers, timeout, idempotent), and consolidated exception handling; calls handle_request_error before returning JSON.
Shared HTTP utilities
mpesakit/http_client/http_client.py
Extracted/added handle_request_error(response), handle_retry_exception(retry_state), and retry_enabled(enabled) to centralize HTTP error mapping and Tenacity retry callback/predicate logic.
Sync HTTP client imports
mpesakit/http_client/mpesa_http_client.py
Removed local retry/error helper definitions and now imports handle_request_error, handle_retry_exception, and retry_enabled from the shared http_client.py.
Unit tests: async client
tests/unit/http_client/test_mpesa_async_http_client.py
Added tests verifying idempotency-key persistence across retries, retry/no-retry behavior on specific status codes, retry stop limit behavior; updated expectations for error message substrings.
Integration tests: STK push
tests/unit/mpesa_express/test_stk_push.py
Added sync and async tests that perform 100 repeated STK pushes to validate client reuse/session behavior and call counts.
Schemas (examples only)
mpesakit/mpesa_express/schemas.py
Replaced hardcoded base64 Password example with a placeholder string in json_schema_extra examples; no runtime or validation 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(...) (ensures X-Idempotency-Key)
    Retry->>HTTPX: _raw_post(...)
    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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Develop #117: Modifies the same HTTP retry/error utilities and retryable request behavior; likely overlaps implementation and tests.
  • Feat/async #122: Touches async HTTP client retry/idempotency logic and related helper functions.
  • Migrate Dev => Develop #89: Prior changes to shared retry/error handling used by HTTP clients; strong overlap in centralized utilities.

"I nibble logs and hop with glee,
A UUID carrot for idempotency.
Retries bounce like springtime hops,
Tests clap paws for all my props.
— 🐰"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and generic, using 'followup to pr #127' which doesn't convey meaningful information about the actual changes. Replace with a descriptive title like 'Align MpesaAsyncHttpClient with sync client for unified retry functionality' that clearly summarizes the main change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description covers all required sections with clear context about the changes, testing approach, and completed checklist items.
Docstring Coverage ✅ Passed Docstring coverage is 90.48% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 an await_count == 3 assertion to match stop_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

📥 Commits

Reviewing files that changed from the base of the PR and between 40042d9 and 22e5468.

📒 Files selected for processing (3)
  • mpesakit/http_client/mpesa_async_http_client.py
  • tests/unit/http_client/test_mpesa_async_http_client.py
  • tests/unit/mpesa_express/test_stk_push.py

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 22e5468 and 37157b4.

📒 Files selected for processing (3)
  • mpesakit/http_client/mpesa_async_http_client.py
  • tests/unit/http_client/test_mpesa_async_http_client.py
  • tests/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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 37157b4 and 97eb1d0.

📒 Files selected for processing (3)
  • mpesakit/http_client/mpesa_async_http_client.py
  • mpesakit/mpesa_express/schemas.py
  • tests/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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ConnectTimeout and httpx.ReadTimeout are subclasses of httpx.TimeoutException, so listing them explicitly is redundant. Including only httpx.TimeoutException and httpx.ConnectError would 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

📥 Commits

Reviewing files that changed from the base of the PR and between 97eb1d0 and 35a463e.

📒 Files selected for processing (2)
  • mpesakit/http_client/mpesa_async_http_client.py
  • tests/unit/mpesa_express/test_stk_push.py

before_sleep_log,
)

from .mpesa_http_client import retry_enabled, handle_request_error, handle_retry_exception
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be wise for an async client depending on functions found in a sync client.

It would be better to store them in the common base file http_client.py as methods or functions.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
mpesakit/http_client/mpesa_async_http_client.py (1)

93-100: Mirror these new parameters in AsyncHttpClient.

MpesaAsyncHttpClient.post()/get() now expose timeout and idempotent, but AsyncHttpClient in mpesakit/http_client/http_client.py still only advertises (url, json, headers) and (url, params, headers). Callers typed against the abstraction won't see the new knobs, and static checks will keep rejecting valid calls.

Also applies to: 187-193

🤖 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 93 - 100, The
abstract interface AsyncHttpClient must mirror the new parameters introduced on
MpesaAsyncHttpClient: add timeout: int = 10 and idempotent: bool = False to the
AsyncHttpClient.post(...) signature (which currently lists url, json, headers)
and likewise add timeout and idempotent to AsyncHttpClient.get(...) (currently
url, params, headers); update the method type hints/docs to include these
parameters so callers typed against AsyncHttpClient can pass them, and ensure
any other concrete implementations of AsyncHttpClient implement the new
parameters as well to preserve compatibility with MpesaAsyncHttpClient.post/get.
🤖 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/http_client.py`:
- Around line 71-83: The code assumes response.json() returns a dict and calls
response_data.get(...), which will raise AttributeError for non-object JSON
(list/string/null); update the error handling in the response parsing block that
assigns response_data to first check isinstance(response_data, dict) before
calling .get(), and for non-dict values convert/serialize them to a string
(e.g., use response.text or str(response_data)) to populate
MpesaError.error_message and raw_response so that raise
MpesaApiException(MpesaError(...)) always runs safely; touch the block around
response.json(), response_data, MpesaApiException and MpesaError to implement
this guard and fallback.

---

Nitpick comments:
In `@mpesakit/http_client/mpesa_async_http_client.py`:
- Around line 93-100: The abstract interface AsyncHttpClient must mirror the new
parameters introduced on MpesaAsyncHttpClient: add timeout: int = 10 and
idempotent: bool = False to the AsyncHttpClient.post(...) signature (which
currently lists url, json, headers) and likewise add timeout and idempotent to
AsyncHttpClient.get(...) (currently url, params, headers); update the method
type hints/docs to include these parameters so callers typed against
AsyncHttpClient can pass them, and ensure any other concrete implementations of
AsyncHttpClient implement the new parameters as well to preserve compatibility
with MpesaAsyncHttpClient.post/get.
🪄 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: d3fa35c1-7a88-4620-8474-6344dda92aad

📥 Commits

Reviewing files that changed from the base of the PR and between 35a463e and d61f4e6.

📒 Files selected for processing (4)
  • mpesakit/http_client/http_client.py
  • mpesakit/http_client/mpesa_async_http_client.py
  • mpesakit/http_client/mpesa_http_client.py
  • mpesakit/mpesa_express/schemas.py
✅ Files skipped from review due to trivial changes (1)
  • mpesakit/mpesa_express/schemas.py

Comment on lines +71 to +83
try:
response_data = response.json()
except ValueError:
response_data = {"errorMessage": response.text.strip() or ""}

error_message = response_data.get("errorMessage", "")
raise MpesaApiException(
MpesaError(
error_code=f"HTTP_{response.status_code}",
error_message=error_message,
status_code=response.status_code,
raw_response=response_data,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find the repository structure
git ls-files | head -20

Repository: Byte-Barn/mpesakit

Length of output: 629


🏁 Script executed:

#!/bin/bash
# Locate and read the specific file mentioned in the review
find . -name "http_client.py" -type f | head -10

Repository: Byte-Barn/mpesakit

Length of output: 99


🏁 Script executed:

#!/bin/bash
# Read the handle_request_error function in http_client.py
cat -n mpesakit/http_client/http_client.py | sed -n '60,90p'

Repository: Byte-Barn/mpesakit

Length of output: 1259


🏁 Script executed:

#!/bin/bash
# Check the full context of the error handling
cat -n mpesakit/http_client/http_client.py | sed -n '50,100p'

Repository: Byte-Barn/mpesakit

Length of output: 2051


🏁 Script executed:

#!/bin/bash
# Check where handle_request_error is called
rg "handle_request_error" mpesakit/http_client/ -n

Repository: Byte-Barn/mpesakit

Length of output: 794


🏁 Script executed:

#!/bin/bash
# Read the mpesa_http_client.py to see how handle_request_error is called
cat -n mpesakit/http_client/mpesa_http_client.py | head -150

Repository: Byte-Barn/mpesakit

Length of output: 6218


🌐 Web query:

httpx.Response.json() return type Any

💡 Result:

In HTTPX, Response.json() is explicitly typed to return typing.Any:

  • def json(self, **kwargs: typing.Any) -> typing.Any: [1]

That’s because decoded JSON can be a dict, list, str, int, float, bool, or None depending on the payload.

Source(s)
[1] https://raw.githubusercontent.com/encode/httpx/def4778d/httpx/_models.py


🏁 Script executed:

#!/bin/bash
# Check MpesaError class to see what type raw_response should be
rg "class MpesaError" -A 20 mpesakit/

Repository: Byte-Barn/mpesakit

Length of output: 1236


🏁 Script executed:

#!/bin/bash
# Get more context around the caller on line 92 to see how response.json() is used after handle_request_error
sed -n '88,95p' mpesakit/http_client/mpesa_http_client.py

Repository: Byte-Barn/mpesakit

Length of output: 385


🏁 Script executed:

#!/bin/bash
# Verify the proposed fix logic - check if there's any issue with how raw_response is assigned
# The fix assigns response_data directly to raw_response in both branches
# This should preserve the original parsed JSON for dict case and the string/None for non-dict case
cat -n mpesakit/errors.py | head -20

Repository: Byte-Barn/mpesakit

Length of output: 858


Handle non-object JSON error bodies before calling .get().

httpx.Response.json() returns typing.Any, which can be a list, string, null, or other JSON values in addition to objects. Calling response_data.get(...) unconditionally will raise AttributeError for non-dict payloads, causing both clients to leak that exception instead of MpesaApiException. Guard with isinstance(response_data, dict) and fall back to string handling for non-dict responses.

Proposed fix
     try:
         response_data = response.json()
     except ValueError:
-        response_data = {"errorMessage": response.text.strip() or ""}
-
-    error_message = response_data.get("errorMessage", "")
+        response_data = response.text.strip() or ""
+
+    if isinstance(response_data, dict):
+        error_message = str(response_data.get("errorMessage", "") or "")
+        raw_response = response_data
+    else:
+        error_message = str(response_data or "")
+        raw_response = response_data
+
     raise MpesaApiException(
         MpesaError(
             error_code=f"HTTP_{response.status_code}",
             error_message=error_message,
             status_code=response.status_code,
-            raw_response=response_data,
+            raw_response=raw_response,
         )
     )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpesakit/http_client/http_client.py` around lines 71 - 83, The code assumes
response.json() returns a dict and calls response_data.get(...), which will
raise AttributeError for non-object JSON (list/string/null); update the error
handling in the response parsing block that assigns response_data to first check
isinstance(response_data, dict) before calling .get(), and for non-dict values
convert/serialize them to a string (e.g., use response.text or
str(response_data)) to populate MpesaError.error_message and raw_response so
that raise MpesaApiException(MpesaError(...)) always runs safely; touch the
block around response.json(), response_data, MpesaApiException and MpesaError to
implement this guard and fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants