feat: Add pluggable HTTP transport adapter

[sgilrodriguez]

Description

Add an injectable HTTP transport so consumers can supply a custom adapter via Client.new(http_adapter:) instead of being hard-wired to Net::HTTP. Purely additive: the default path is unchanged and there are no breaking changes for consumers.

• Recurly::HTTP::Adapter base documenting the contract #call(method, url, headers, body) -> AdapterResponse.
• Recurly::HTTP::DefaultHttpAdapter wrapping Net::HTTP and the shared connection pool.
• The client retains URL building, auth headers, idempotency, retry, and error mapping; the adapter owns only transport, translating transport exceptions into a neutral TransportError.

Testing

Automated Test Coverage:

• Run: bundle exec rspec (ruby 2.7.8)
• Result: 906 examples, 0 failures, 1 pending (integration spec is env-gated on RECURLY_INTEGRATION=1 + RECURLY_API_KEY and skips otherwise)

Manual:

1. Inject a custom adapter via Client.new(http_adapter:) and confirm requests route through it.
   Expected: adapter receives (method, url, headers, body).
2. Construct a Client with no adapter.
   Expected: default Net::HTTP path behaves identically.

[epagerecurly]

Adversarial review notes

• interface compatibility — now passes an AdapterResponse where it previously passed a Net::HTTP::Response. The #code shim handles the common dispatch pattern. However, if Errors::APIError.from_response (in errors.rb, not in this diff) uses / checks against Net::HTTP subclasses (e.g. Net::HTTPSuccess, Net::HTTPServerError), those would silently return nil on an AdapterResponse, causing all non-JSON error responses to fall through to the wrong error class. Worth confirming from_response only touches .code before merge. (lib/recurly/client.rb)

• Blank reason phrase for uncommon status codes — HTTP_STATUS_MESSAGES covers ~20 common codes. For anything not in the map (418, 421, 425, 451, 507, 511, etc.), reason_phrase returns nil, producing error messages like "418: " with a trailing blank. Low risk in practice but a minor regression for custom adapter authors. (lib/recurly/client.rb)

Clean otherwise — retry counter sharing, 5xx inline re-issue, header case-insensitivity, timeout defaults, and the transport error hierarchy all look correct.

---

Code quality review (recurly:review-pr + marvin:code-reviewer-code-quality)

Blocking:

• Security test removed without replacement — the old client spec asserted expect(net_http).not_to receive(:set_debug_output) to prevent plaintext credential leaks. Net::HTTP moved into DefaultHttpAdapter but no equivalent guard was added to default_http_adapter_spec.rb. (spec/recurly/client_spec.rb removed ~line 1004)

Advisory:

• TransportError#cause shadows Exception#cause — Ruby's built-in Exception#cause is auto-populated on raise inside rescue and is read by Sentry, logging frameworks, and pp. Rename to original_exception or remove. (lib/recurly/errors/network_errors.rb:452)
• Misleading "Retrying" log — fires before the if retries < MAX_RETRIES guard; when budget is exhausted it logs but doesn't retry. Move the log inside the conditional. (lib/recurly/client.rb, run_request)
• timeout: param should be read_timeout: — DefaultHttpAdapter#initialize accepts timeout: but stores it as @read_timeout. (lib/recurly/http/default_http_adapter.rb:538)
• Unit inconsistency in timeout params — constructor takes ms (timeout: 60_000), per-request overrides in #call are in seconds. Not documented.
• Dead logger: param — stored as @logger, never read. (lib/recurly/http/default_http_adapter.rb:544)
• Redundant body param in run_request — duplicated in HTTP::Request#body; can silently diverge. (lib/recurly/client.rb:175)
• AdapterResponse#code leaks a Net::HTTP artifact — from_response should accept an integer status_code directly. (lib/recurly/http/response.rb:697)
• HTTP_STATUS_MESSAGES placed after private — constants are unaffected by private in Ruby; false impression of encapsulation. (lib/recurly/client.rb:348)

---

Test coverage review (marvin:test-reviewer-coverage + marvin:test-reviewer-calibration)

Blocking:

• 5xx on non-GET verbs never tested — every 5xx path test uses get_account. The guard method == HTTP::HttpMethod::GET prevents POST/PUT/DELETE from being re-issued on 5xx, but no test verifies this. A POST returning 500 should call the adapter exactly once. Add tests for POST and DELETE asserting adapter.calls.length == 1 on a 500.
• Status boundary 299/300 not tested — handle_response! uses .between?(200, 299) (replacing Net::HTTPSuccess). Neither 299 nor 300 is exercised. An off-by-one change would silently absorb redirects as successes.
• HTTP_STATUS_MESSAGES fallback never reached — every error test passes an explicit reason_phrase. The nil fallback branch is untested. Add a test with reason_phrase: nil.

Advisory:

• raise_api_error! nil content-type path (new &. nil-safe) has no test.
• Per-request timeout overrides not verified at the adapter level.
• NET_VERBS.fetch unsupported-verb ArgumentError has no test.
• :connection kind tested only for Errno::ECONNREFUSED; SocketError, Errno::ECONNRESET etc. untested.
• Redundant test context ("5xx-GET re-issue never exceeds MAX_RETRIES") duplicates parent assertions.

---

Integration seam review (marvin:test-reviewer-integration)

Advisory:

• [5xx, TransportError, *] counter ordering untested — the "invariant B" test covers [transport, transport, 5xx]. The inverse — 5xx on first call (bumps retries to 1), then TransportError on the inline re-issue — exercises a different counter mutation order. A change to retry logic could break one path without breaking the other. Suggested fixtures: [500, TransportError(:connection), 500] → raises ConnectionFailedError, 3 calls; [500, TransportError(:connection), 200] → returns account, 3 calls.
• Non-2xx with Content-Type: application/json and empty body raises wrong exception — raise_api_error! calls JSONParser.parse(self, response.body) without guarding for nil body. HTTP::Response sets @body = nil when the body string is empty. This raises TypeError/JSON::ParserError instead of a typed Recurly::Errors::APIError. Reproducer: AdapterResponse.new(status_code: 422, headers: {"content-type" => "application/json"}, body: "").
• 200 with no Content-Type header raises InvalidContentTypeError — custom adapters that omit the header get an unexpected error. The adapter contract docs don't require a response content-type. Reproducer: AdapterResponse.new(status_code: 200, headers: {}, body: '{"object":"account"}').
• Unrecognized status code with reason_phrase: nil — HTTP_STATUS_MESSAGES covers 21 codes; anything outside (408, 425, 451, etc.) produces "408: ". No test covers this path.

Seams with adequate coverage: all four TransportError#kind values tested; AdapterResponse#code string dispatch; DefaultHttpAdapter exception mapping including Net::ReadTimeout < Timeout::Error ordering; invariant B [transport, transport, 5xx]; Idempotency-Key stability; absolute-URL construction.

---

AC verification (marvin:code-reviewer-ac) — All 5 criteria: met.

Criterion	Verdict
Client.new(http_adapter:) injectable transport	✅ met
Recurly::HTTP::Adapter base class with contract	✅ met
DefaultHttpAdapter wrapping Net::HTTP + shared pool	✅ met
Client retains URL/auth/idempotency/retry/error mapping; adapter owns transport only	✅ met
Purely additive — default path unchanged, no breaking changes	✅ met

---

Automated review by Claude.

[sgilrodriguez]

Addressed in cc417df:

Fixed (blocking):

• Restored the set_debug_output security guard, moved to default_http_adapter_spec.rb where Net::HTTP now lives.
• Added 5xx-on-POST/DELETE tests asserting the adapter is called exactly once (no re-issue on non-GET).
• Added 299/300 boundary tests.
• Added a reason_phrase: nil fallback test.

Fixed (bug found while verifying the interface-compatibility note):

• A non-2xx response with Content-Type: application/json and an empty body was calling JSONParser.parse(self, nil), raising a raw TypeError instead of a typed APIError. Added a nil-body guard in raise_api_error! plus a regression test.

Fixed (advisory):

• TransportError#cause → renamed reader to original_exception; Ruby's built-in Exception#cause is no longer shadowed (both are now tested).
• Moved the "Retrying" log inside the retries < MAX_RETRIES guard.
• timeout: → read_timeout: on DefaultHttpAdapter#initialize; documented the ms-vs-seconds unit split against the per-request override.
• Removed the dead logger: param from DefaultHttpAdapter.
• Removed the redundant body arg from run_request (reads request.body now).
• Moved HTTP_STATUS_MESSAGES above private (constants aren't affected by it, but it was misleading).
• Reason-phrase formatting is now nil-safe: an unmapped status code with no adapter-supplied phrase renders as e.g. "418" instead of "418: ".

Confirmed as non-issues:

• Errors::APIError.from_response only touches .code — verified in errors.rb. Safe with AdapterResponse#code's string shim.

Deliberately deferred:

• AdapterResponse#code → integer refactor: ERROR_MAP lives in lib/recurly/errors/api_errors.rb, which is auto-generated by the OpenAPI tooling and keyed by string status codes. Changing the type would fight the generator — better addressed in that pipeline than in this PR.
• 200-with-no-Content-Type strictness: this is pre-existing behavior, not a regression introduced by the adapter seam. Left as-is.

Full suite (919 examples) passes.

[epagerecurly]

Re-review after fix commit — all blocking issues resolved

Ran adversarial, code-quality, and test-coverage passes on the updated diff. Summary:

All 7 claimed fixes verified. Previous blocking concerns are addressed:

• set_debug_output security test correctly re-homed in DefaultHttpAdapter spec (not client_spec)
• TransportError#cause renamed to original_exception — built-in Exception#cause no longer shadowed
• Dead body and logger params dropped from run_request / DefaultHttpAdapter
• Misleading "Retrying" log moved inside the retries < MAX_RETRIES guard
• Blank reason phrase for unmapped status codes handled via HTTP_STATUS_MESSAGES fallback
• HTTP_STATUS_MESSAGES consolidated to one definition

Remaining items are advisory only:

• DefaultHttpAdapter constructor takes read_timeout/open_timeout in milliseconds while per-request overrides in #call are in seconds — units diverge. No production path hits this today (only defaults are used at construction), but it's a footgun for direct consumers.
• Constructor keyword param is named cause: which overlaps semantically with Exception#cause; the stored attribute is original_exception. Renaming the param to original_exception: would be cleaner.
• PUT 5xx not-re-issued path is untested (POST and DELETE have explicit specs; PUT is in the same equivalence class but absent).
• A few DefaultHttpAdapter defensive branches have no coverage (unsupported HTTP verb ArgumentError path, nil content-type header on error response).

None of these are blocking. The PR is in good shape.

[epagerecurly]

All blocking issues from the initial review are resolved. Ran adversarial, code-quality, and test-coverage passes on the updated diff — zero blocking findings across all three. A few advisories remain (timeout unit mismatch in the constructor, keyword naming, a couple untested defensive branches) but none are blocking.

[epagerecurly]

All blocking issues from the initial review are resolved. Ran adversarial, code-quality, and test-coverage passes on the updated diff — zero blocking findings across all three. A few advisories remain (timeout unit mismatch in the DefaultHttpAdapter constructor, cause: keyword naming, a couple untested defensive branches) but none are blocking.

[epagerecurly]

Timeout fix in c67f11f is correct — constructor now stores values as-is (seconds), consistent with the per-request override unit. Unit test pins it. No further concerns; the broader adapter seam is clean.