refactor(utils): extract shared HTTP-post helper for delivery modules#952
Conversation
Closes Tracer-Cloud#864. ``app/utils/slack_delivery.py``, ``app/utils/discord_delivery.py``, and ``app/utils/telegram_delivery.py`` each issued ``httpx.post`` directly, applied a per-provider timeout, parsed the response body, and converted exceptions to ``(False, error, ...)`` tuples. The transport pieces were identical; only the success criteria, auth scheme, and error-message extraction differed per provider. Add ``app/utils/delivery_transport.py`` with ``post_json`` plus a ``DeliveryResponse`` dataclass that captures the shared transport behavior: HTTP POST, timeout, optional ``follow_redirects``, exception suppression, and JSON decoding with graceful fallback. The helper deliberately does not decide provider-level success — callers inspect ``status_code`` / ``data`` per their own semantics. Refactored callers: - Slack: ``_call_reactions_api``, ``_post_direct``, ``_post_via_webapp``, ``_post_via_incoming_webhook`` — all four code paths now go through ``post_json``. ``send_slack_report`` orchestration unchanged. - Discord: ``post_discord_message``, ``create_discord_thread`` — same Bearer/Bot header pattern factored into ``_discord_auth_headers``. - Telegram: ``post_telegram_message`` — bot-token redaction in error messages preserved by re-running ``_redact_token`` over ``response.error``. Provider-specific payload building, success criteria (``data["ok"]`` for Slack, status codes for Discord/Telegram), and error extraction all stay in the calling modules. Public function signatures are unchanged. Tests: - ``tests/utils/test_delivery_transport.py`` (new) — 15 tests covering happy path, every transport-failure shape (httpx.ConnectError, ReadTimeout, RequestError, OSError, RuntimeError), JSON-decode fallbacks (non-JSON body, JSON array, empty body), header / timeout / follow_redirects pass-through, and DeliveryResponse frozen-dataclass invariants. - ``tests/utils/test_slack_delivery.py`` (new) — 24 tests covering all four Slack code paths, ``send_slack_report`` orchestration, and fallback chain ``direct → webapp``. - ``tests/utils/test_discord_delivery.py`` (extended) — 3 new tests pinning the helper-delegation contract: module no longer imports ``httpx``, ``post_discord_message`` and ``create_discord_thread`` both go through ``post_json``. - ``tests/utils/test_telegram_delivery.py`` (extended) — 3 new tests pinning the same contract plus a regression test that the bot-token is redacted out of error strings even when the failure originates from the shared transport. - All pre-existing tests in ``test_discord_delivery.py`` and ``test_telegram_delivery.py`` updated to patch ``app.utils.delivery_transport.httpx.post`` (the new transport boundary) instead of the per-module ``httpx.post`` import that no longer exists. Verification: - ``pytest tests/``: 3191 pass, 2 skipped, 1 xfailed, 0 failures. - ``pytest tests/utils/``: 105 pass, 1 skipped (was 32 + 26 = 58 across delivery modules; now 105 with helper + Slack additions). - ``ruff check app/ tests/`` and ``ruff format --check``: clean. - ``mypy app/utils/``: clean on touched modules; the 5 reported errors are pre-existing missing-stub warnings in ``app/integrations/{mariadb,mysql,hf_remote}`` unrelated to this PR.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The previous CI run hit a known transient ImportError in anyio.lowlevel/mcp when opensre integrations list ran on a pytest-xdist worker. Reproduces zero times locally on origin/main and on this branch; pushing an empty commit to re-run with a fresh interpreter pool.
|
Heads up: the |
Greptile SummaryThis PR extracts a shared Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/coverage suggestions with no correctness impact The refactor is mechanically correct: every provider's success semantics, error strings, and public signatures are preserved. The three P2 comments (mutable dict in frozen dataclass, dropped exception-type log field, missing Slack delegation regression test) are quality notes that do not affect runtime behaviour or data correctness. tests/utils/test_slack_delivery.py is the only file worth revisiting to add the TestDelegatesToSharedTransport class that the Discord and Telegram test files already have. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Provider as "slack/discord/telegram delivery"
participant Transport as "delivery_transport.post_json"
participant httpx
Caller->>Provider: public helper call
Provider->>Transport: post_json(url, payload, headers, timeout)
Transport->>httpx: httpx.post(url, json, headers, timeout, follow_redirects)
alt network success
httpx-->>Transport: Response object
Transport-->>Provider: DeliveryResponse ok=True status_code data text
Note over Provider: Apply provider-level check on status_code or data
else network error
httpx-->>Transport: Exception raised
Transport-->>Provider: DeliveryResponse ok=False error=str(exc)
end
Provider-->>Caller: result tuple
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@mayankbharati-ops thank you for the PR. could you pls fix all the review points above? |
|
yeah sure @muddlebee , I'll update it and let you know |
- Wrap `DeliveryResponse.data` in `MappingProxyType` so the frozen dataclass is fully immutable end-to-end. Mutating the parsed body now raises `TypeError` instead of silently succeeding, and caller- passed dicts can no longer leak mutations into the response. - Add `error_type` field on `DeliveryResponse` populated with `type(exc).__name__` on transport failures. `slack_delivery._post_direct` threads it back into the exception log so on-call can distinguish `TimeoutError` from `ConnectionError` at a glance — restores the pre-refactor log shape that Tracer-Cloud#864 had dropped. - Add `TestDelegatesToSharedTransport` to `tests/utils/test_slack_delivery.py`, mirroring the regression class on the discord/telegram test files. Pins that `slack_delivery` does not import `httpx` and that all four code paths (`_call_reactions_api`, `_post_direct`, `_post_via_webapp`, `_post_via_incoming_webhook`) route through `delivery_transport.post_json`. - Add focused tests for the immutable-data, error_type, and slack exc_type-log behaviours (+17 tests; 3208 pass / 2 skipped / 1 xfail). Tighten `_discord_error_from_data` signature to `Mapping[str, Any]` so mypy stays clean against the new read-only `data` type.
|
@muddlebee — pushed 1. 2. Exception type dropped from 3. Missing Test coverage: +17 new tests covering the read-only-data invariant, caller-dict isolation, default-mapping independence, dict-equality preservation, |
| if not response.ok: | ||
| logger.error( | ||
| "[slack] Direct post exception channel=%s thread_ts=%s " | ||
| "exc_type=%s detail=%s (caller may attempt fallback)", |
There was a problem hiding this comment.
Nit: the log key changed from type=%s to exc_type=%s. If any log parser keys off type=, it might miss this. Consider keeping type=%s unless you intentionally want to rename it.
There was a problem hiding this comment.
any particular reason behind this @mayankbharati-ops ? exc_type
| def _discord_auth_headers(bot_token: str) -> dict[str, str]: | ||
| return { | ||
| "Authorization": f"Bot {bot_token}", | ||
| "Content-Type": "application/json; charset=utf-8", |
There was a problem hiding this comment.
Nit: httpx.post(..., json=payload) already injects Content-Type: application/json, so this header is redundant.
| data: Mapping[str, Any] = field(default_factory=dict) | ||
| text: str = "" | ||
| error: str = "" | ||
| error_type: str = "" |
There was a problem hiding this comment.
Nit: consider renaming error_type to exc_type so it matches the Slack log field name.
… drop redundant header, fix CodeQL - slack_delivery `_post_direct` exception log key reverted to ``type=%s`` (matching the exact pre-refactor format ``type=%s channel=%s thread_ts=%s detail=%s``) so existing log parsers that key off ``type=`` keep working. Per @muddlebee's nit on PR Tracer-Cloud#952. - DeliveryResponse field renamed ``error_type`` → ``exc_type``. Pythonic abbreviation for "exception type"; can't be ``type`` because that shadows the builtin. Field/log-key are intentionally distinct: the field is the Python attribute, the log key matches the legacy format. - Drop ``Content-Type: application/json; charset=utf-8`` from ``_slack_bearer_headers`` and ``_discord_auth_headers``. ``httpx.post`` already sets ``Content-Type: application/json`` automatically when the request uses the ``json=`` kwarg, so the explicit header was redundant. No behavioural change for Slack/Discord (UTF-8 is httpx's default encoding and neither provider parses the charset suffix). - Fix CodeQL py/import-and-import-from alerts on tests Tracer-Cloud#503/Tracer-Cloud#504. The ``test_module_does_not_import_httpx`` regression test in each delivery test file was importing the module under test via both ``import X as mod`` and ``from X import Y`` styles. Switched to a single ``from app.utils import <module>`` style so the same module is no longer dual-imported. Test count unchanged (3208 pass / 2 skipped / 1 xfail). Lint, format, and ``mypy app/utils/`` clean.
|
@muddlebee — pushed 1. 2. 3. 4. 5. CodeQL #503 / #504 — Verification: 3208 pass / 2 skipped / 1 xfailed (unchanged). |
…ng without it Live e2e probe against real ``slack.com/api/chat.postMessage`` revealed that dropping the explicit ``Content-Type: application/json; charset=utf-8`` header (per the previous nit-fix commit) caused Slack to add a ``missing_charset`` entry to ``response_metadata.warnings`` on every request. httpx alone sets only the bare ``application/json`` for ``json=`` kwargs, but Slack's docs explicitly recommend the charset suffix (https://api.slack.com/web#posting_json) and emit the warning otherwise. Restored the ``charset=utf-8`` header on ``_slack_bearer_headers`` with an inline comment explaining the reason. ``_discord_auth_headers`` stays auth-only (Discord does not emit this warning, and its docs do not require the charset suffix). Pinned via direct header assertions on the two existing slack tests that already captured headers (``test_sends_correct_url_and_headers`` for the reactions API, ``test_sends_thread_reply_payload`` for chat.postMessage), so a future regression that re-drops the charset will fail loudly. Verified live: - direct ``chat.postMessage`` probe now returns ``warnings=[]`` (was ``['missing_charset']`` before this commit). - 38/38 live checks against real Slack / Discord / Telegram pass. - 3208 unit tests pass; lint / format / mypy clean.
|
@muddlebee — ran a deep live end-to-end probe against the real Slack / Discord / Telegram APIs (no mocks, real HTTPS, real provider error shapes), and the live test caught a real regression introduced by my previous nit-fix commit. Pushed What the live probe found
The regression and the fix (
|
3 participants
Closes #864.
Summary
`app/utils/slack_delivery.py`, `app/utils/discord_delivery.py`, and `app/utils/telegram_delivery.py` each reimplemented the same "POST JSON + parse response + return `(ok, error, id)` style result" pattern. The transport pieces (HTTP POST, timeout, exception suppression, JSON decode) were identical; only the success criteria, auth scheme, and error-extraction differed.
This PR adds `app/utils/delivery_transport.py` and migrates all three modules onto it, per the issue's acceptance criteria.
Design
```python
@DataClass(frozen=True)
class DeliveryResponse:
ok: bool # request did not raise
status_code: int = 0 # HTTP status (0 if request raised)
data: dict = {} # parsed JSON object body, or {} on non-JSON / non-dict / decode error
text: str = "" # raw response body
error: str = "" # exception message when ok=False
def post_json(url, payload, *, headers=None, timeout=15.0, follow_redirects=False) -> DeliveryResponse:
...
```
The helper deliberately does not decide provider-level success. `ok=True` means the HTTP request completed without raising; callers inspect `status_code` and `data` per their own provider semantics:
chat.postMessage/ reactionsRefactored call sites
Public function signatures unchanged. Provider-specific payload building stays in the calling modules per the issue's scope.
Test coverage
Verification
Acceptance criteria audit
Scope