Skip to content

fix(relay): return None on non-object media upload response - #72933

Open
wuisabel-gif wants to merge 1 commit into
NousResearch:mainfrom
wuisabel-gif:fix/relay-media-upload-non-object
Open

fix(relay): return None on non-object media upload response#72933
wuisabel-gif wants to merge 1 commit into
NousResearch:mainfrom
wuisabel-gif:fix/relay-media-upload-non-object

Conversation

@wuisabel-gif

Copy link
Copy Markdown

What does this PR do?

RelayMediaClient.upload() (gateway/relay/media.py) documents that it "returns None on any failure (callers fall back to their pre-media behaviour, media delivery is best-effort by design)." Its except clause catches (urllib.error.URLError, ValueError, OSError).

But after a successful HTTP response it does body = json.loads(...) and then body.get("id"). If the connector returns a valid JSON value that is not an object (null, [], a string, a number), json.loads succeeds and body.get("id") raises AttributeError, which is not in the caught tuple. So it escapes upload(), propagates through _send_media() and the send_image_file / send_voice / send_video / send_document senders, and bypasses the best-effort fallback those lanes rely on ("never a regression when the connector predates the op").

This guards the parsed body with isinstance(body, dict) before .get, so a non-object response degrades to None like every other failure.

Related Issue

No open issue. Found by reading the Phase 2 media code (PR #71363, commit 689b51b).

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/relay/media.py: in upload()'s _post(), return None when the parsed JSON body is not a dict, before calling .get("id").
  • tests/gateway/relay/test_relay_media.py: add a parametrized test (null, [], "ok", 42) asserting upload() returns None instead of raising.

How to Test

scripts/run_tests.sh tests/gateway/relay/test_relay_media.py -q
  • With the fix: 19 passing.
  • Reverting just the isinstance guard makes the 4 new cases fail with AttributeError: '...' object has no attribute 'get', confirming the test catches the bug.

Checklist

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've added a test that fails without the fix and passes with it
  • Cross-platform impact considered: pure stdlib logic, platform-independent

RelayMediaClient.upload() documents 'returns None on any failure', and its
except catches (URLError, ValueError, OSError). But a valid-JSON non-object
response (null, [], a string) reached body.get('id') and raised AttributeError,
which is outside that tuple — so it escaped upload() and broke the best-effort
fallback the send_* lanes rely on.

Guard the parsed body with isinstance(body, dict) before .get. Add a
parametrized test (null/[]/"ok"/42) proving upload() returns None instead of
raising.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. The premise is confirmed on current main: gateway/relay/media.py:143-144 parses JSON and calls .get("id") without verifying the result is an object, while _send_media() relies on a falsey upload() result to select the fallback at gateway/relay/adapter.py:1369-1371.

The proposed guard preserves that contract, and the parametrized cases cover the relevant valid-JSON non-object values. Current main retains the PR diff's production-file preimage, so this is mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants