Skip to content

fix: treat 2xx responses as successful - #253

Open
saidM wants to merge 1 commit into
PostHog:mainfrom
saidM:fix/accept-all-2xx-responses
Open

fix: treat 2xx responses as successful#253
saidM wants to merge 1 commit into
PostHog:mainfrom
saidM:fix/accept-all-2xx-responses

Conversation

@saidM

@saidM saidM commented Aug 19, 2026

Copy link
Copy Markdown

💡 Motivation and Context

Only 200 responses were considered successful, so other successful responses such as 201, 202, and 204 triggered on_error.

This updates both the sync client and background worker to accept any 2xx response.

It fixes issue #187

💚 How did you test it?

Added specs covering 201, 202, and 204 responses in both code paths.

  • Full test suite: 670 examples, 0 failures
  • RuboCop: no offenses
  • rake public_api:check: passed

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Used GPT-5.6 in Codex to help implement and test the change. I reviewed the final code.

@saidM
saidM requested a review from a team as a code owner August 19, 2026 19:54
@saidM
saidM force-pushed the fix/accept-all-2xx-responses branch from 4739528 to 12dbd5c Compare August 19, 2026 19:55
@saidM saidM changed the title Treat 2xx responses as successful fix: treat 2xx responses as successful Aug 19, 2026

@dustinbyrne dustinbyrne left a comment

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.

thanks for putting this together @saidM! one small bug described below:

Comment on lines +281 to +283
allow_any_instance_of(PostHog::Transport).to(
receive(:send).and_return(PostHog::Response.new(status))
)

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.

a real Net::HTTPNoContent (204) response has a nil body and we JSON parse the body in the transport (lib/posthog/transport.rb), so in this case, it'd actually raise an exception

we should fix / test this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hey @dustinbyrne, good catch! going to push a fix shortly

@dustinbyrne

Copy link
Copy Markdown
Contributor

one more thing: check RELEASING.md for release prerequisites. verified commits are also required for us to be able to merge this change.

@saidM
saidM force-pushed the fix/accept-all-2xx-responses branch from 12dbd5c to 2f228a8 Compare August 19, 2026 22:04
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