Don't crash when a token refresh fails without an HTTP response

[shime]

refresh_token!'s rescue assumes every OAuth2::Error wraps an HTTP response:

rescue OAuth2::Error => e
  response = e.response.response.env
  Booqable::Error.from_response(response)
end

But oauth2 also raises OAuth2::Error.new({...}) with a plain Hash when the failure never reached the token endpoint — most notably when the stored token has no refresh token (access_token.rb:218 in oauth2 2.0.24). Error#response returns whatever the error was constructed with, so e.response.response dies with:

NoMethodError: undefined method 'response' for an instance of Hash

Reproduced against oauth2 2.0.24. Not a regression — this path has existed as long as the rescue — but the refresh buffer from #55 widens exposure slightly: a refresh-token-less token inside the 60s window used to be sent as-is and now triggers a refresh attempt.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

lib/booqable/middleware/auth/oauth.rb:107

• The YARD @raise tag is now inaccurate: refresh_token! can re-raise OAuth2::Error when there is no HTTP response/env to map. Update the docstring to include OAuth2::Error (and/or clarify the conditional behavior) so callers have an accurate contract.

        # Booqable errors for consistent error handling. OAuth2 errors raised
        # without an HTTP response (e.g. the stored token has no refresh
        # token, so the error carries a plain Hash) propagate as-is — there
        # is no response to map to a Booqable error.
        #
        # @return [OAuth2::AccessToken] The new access token
        # @raise [Booqable::Error] For OAuth-related errors

spec/booqable/oauth_client_spec.rb:477

• This expectation matches the exact exception instance, which is more brittle than necessary and couples the spec to the implementation detail that the same object is re-raised. Prefer asserting on the exception class (and optionally message) to keep the test resilient while still verifying behavior.

        expect { middleware.call(env) }.to raise_error(oauth_error)

spec/booqable/client_spec.rb:584

• Matching a specific OAuth2 gem error message can be brittle across gem versions/locales. If feasible, prefer asserting primarily on the exception class (or a narrower, project-controlled signal) and only match message text if it's essential to the contract being tested.

        expect { client.get("/orders") }
          .to raise_error(OAuth2::Error, /A refresh_token is not available/)

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

lib/booqable/middleware/auth/oauth.rb:107

• The YARD tag @raise [Booqable::Error] is now incomplete/inaccurate because the method can re-raise OAuth2::Error when there’s no HTTP response/env to map. Update the @raise documentation to include OAuth2::Error (or adjust wording to reflect both possible exception types).

        # Booqable errors for consistent error handling. OAuth2 errors raised
        # without an HTTP response (e.g. the stored token has no refresh
        # token, so the error carries a plain Hash) propagate as-is — there
        # is no response to map to a Booqable error.
        #
        # @return [OAuth2::AccessToken] The new access token
        # @raise [Booqable::Error] For OAuth-related errors

lib/booqable/middleware/auth/oauth.rb:118

• This extraction only handles the case where e.response responds to #response and yields an object with #env. If e.response is already an HTTP response object with #env (or otherwise structured differently), this will incorrectly re-raise instead of mapping to Booqable::Error. Consider supporting both shapes by preferring e.response.env when available, otherwise falling back to e.response.response.env, and only re-raising when neither exists.

          http_response = e.response.response if e.response.respond_to?(:response)
          raise unless http_response&.env

          Booqable::Error.from_response(http_response.env)

spec/booqable/client_spec.rb:584

• Matching on /refresh_token/ makes the spec brittle across OAuth2 gem versions/localizations since error messages can change while behavior stays correct. If the intent is to ensure the error propagates as OAuth2::Error (and not a mapped Booqable::Error / not a NoMethodError), consider asserting only the exception class (or asserting it’s not a Booqable error) rather than depending on the message text.

        expect { client.get("/orders") }
          .to raise_error(OAuth2::Error, /refresh_token/)

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

lib/booqable/middleware/auth/oauth.rb:122

• The current extraction only handles error responses shaped like e.response.response.env. If e.response itself is already an HTTP response object that exposes env (but does not respond to response), this will incorrectly skip mapping and re-raise OAuth2::Error. Consider supporting both shapes by also accepting e.response when it responds to env.

          http_response = e.response.response if e.response.respond_to?(:response)
          raise unless http_response&.env

          Booqable::Error.from_response(http_response.env)
          raise

lib/booqable/middleware/auth/oauth.rb:121

• This relies on Booqable::Error.from_response raising internally for mapped errors, and uses a bare raise as a fallback. Making the control flow explicit (e.g., capturing a returned mapped error and raising it when present, otherwise re-raising the original) would clarify intent and avoid coupling to from_response side effects.

          Booqable::Error.from_response(http_response.env)
          raise

spec/booqable/oauth_client_spec.rb:477

• Asserting raise_error(oauth_error) depends on the exact exception instance being re-raised (which is currently true due to and_raise(oauth_error)). To make the test more resilient while still validating behavior, consider asserting on the exception class (and optionally message) rather than object identity.

        expect { middleware.call(env) }.to raise_error(oauth_error)