feat: make outbound HTTP timeouts configurable via spring.http.clients

[tmseidel]

No description provided.

[max-california]

🤖 Agentic Code Review

Summary

Nice direction overall: switching provider/client construction from RestClient.builder() to an injected RestClient.Builder is the right way to let Spring apply shared HTTP client configuration such as timeouts. Most of the touched providers look consistent.

I found one remaining call site that still bypasses the shared builder and therefore does not get the configured timeouts. That makes the PR incomplete relative to its stated goal.

Findings

Blocking

• Bitbucket PR diff fetch still ignores the new configurable timeouts
  • File: src/main/java/org/remus/giteabot/bitbucket/BitbucketApiClient.java:70
  • The PR claims outbound HTTP timeouts are configurable via spring.http.clients, but BitbucketApiClient#getPullRequestDiff(...) still creates its own client with RestClient.builder() and a custom JdkClientHttpRequestFactory.
  • Because this bypasses the Spring-managed RestClient.Builder, the new timeout properties will not apply to Bitbucket diff downloads. For large/slow responses, this is exactly the kind of path where a read timeout matters.
  • This is not just a documentation mismatch: Bitbucket reviews can now behave differently from all the other outbound calls, which is a correctness/operability gap.
  • Suggested fix: inject/use the shared RestClient.Builder (or otherwise centralize request factory customization) here as well, while preserving redirect-following behavior. If the redirect-following request factory must remain custom, make sure its connect/read timeouts are explicitly propagated from configuration too.

Non-blocking

• The application properties comment overstates current coverage
  • File: src/main/resources/application.properties:82
  • The comment says this applies to “all RestClient instances,” but as noted above, BitbucketApiClient currently constructs one manually.
  • If you keep the current implementation, the comment should be narrowed; otherwise fixing the Bitbucket path will make the comment true.

Decision

---

Read-only agentic review by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary

Nice improvement overall: switching provider/client construction from RestClient.builder() to the Spring-managed RestClient.Builder is the right way to let spring.http.clients.* apply centrally.

I found one blocking regression in the Bitbucket path and one notable maintainability/test gap around the OpenAI-specific request factory override.

Blocking

1. Bitbucket diff redirect handling is no longer guaranteed and likely breaks tests/runtime assumptions

Files:

• src/main/java/org/remus/giteabot/bitbucket/BitbucketApiClient.java:60-67
• src/main/java/org/remus/giteabot/repository/BitbucketProviderMetadata.java:147-151

Previously, getPullRequestDiff() explicitly built a JdkClientHttpRequestFactory with HttpClient.Redirect.NORMAL to follow Bitbucket Cloud’s 302 from the diff endpoint. This PR removes that special handling and now relies on the auto-configured client:

String diff = restClient.get() ...

The comment says the auto-configured HttpComponents client follows redirects by default, but that’s not established anywhere in this codebase, and more importantly it’s inconsistent with the previous implementation’s deliberate workaround. If the managed RestClient.Builder ends up using a different request factory in some environments/tests, getPullRequestDiff() may stop working.

This is especially risky because BitbucketProviderMetadata.buildRestClient() only sets headers/base URL and no explicit request factory, so redirect behavior is now implicit and environment-dependent.

Actionable fix: preserve the explicit redirect-following behavior for the Bitbucket diff flow, either by:

• configuring the Bitbucket RestClient with a request factory that is known to follow redirects, or
• keeping a dedicated client/factory just for getPullRequestDiff() if the global builder cannot express that behavior reliably.

At minimum, add/adjust a test that exercises the redirect case so this behavior is pinned down.

Non-blocking

2. OpenAI still bypasses the managed HTTP client settings for the actual request factory

File: src/main/java/org/remus/giteabot/ai/openai/OpenAiProviderMetadata.java:58-63

Although this class now obtains a Spring-managed RestClient.Builder, it immediately overrides the request factory with:

var httpClient = HttpClients.createDefault();
HttpComponentsClientHttpRequestFactory factory =
        new HttpComponentsClientHttpRequestFactory(httpClient);
...
.requestFactory(factory)

That means the new spring.http.clients.connect-timeout / read-timeout configuration may not actually apply to OpenAI, which undercuts the PR’s stated goal (“make outbound HTTP timeouts configurable via spring.http.clients”). It may also diverge from any future Boot-level client customizations.

I’m not marking this blocking because I can’t verify Spring Boot 4’s exact builder customization semantics from the repo alone, but it deserves attention.

Suggestion: if OpenAI no longer needs a custom request factory, remove the override and rely fully on the injected builder. If it does need HttpComponents specifically, configure that client/factory with the same timeout values or wire it through Spring so the central HTTP client configuration still applies.

Positives

• The migration to injected RestClient.Builder is consistent across AI and repository providers.
• Tests were updated to construct providers with an ObjectProvider<RestClient.Builder>, which keeps unit coverage compiling.
• Centralized timeout properties in application.properties are sensible defaults.

---

Read-only agentic review by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary

The overall direction makes sense: moving all RestClient creation onto Spring Boot’s auto-configured RestClient.Builder is the right way to pick up centralized timeout/redirect settings, and the constructor/test updates look consistent.

I found one likely blocking regression in the Bitbucket path, plus one maintainability concern around the dependency change.

Blocking

1. Bitbucket diff redirect handling now depends on an assumption that is not true for all configured request factories

Files: src/main/java/org/remus/giteabot/bitbucket/BitbucketApiClient.java:60-67, src/main/resources/application.properties:82-88

Previously, BitbucketApiClient#getPullRequestDiff explicitly created a JDK HttpClient with followRedirects(HttpClient.Redirect.NORMAL) and wired it into a dedicated JdkClientHttpRequestFactory. That guaranteed Bitbucket Cloud’s 302-based diff endpoint would work.

This PR removes that explicit request factory and relies on the shared auto-configured client instead:

String diff = restClient.get()
    .uri("/repositories/{workspace}/{repo}/pullrequests/{pr_id}/diff", ...)
    .header("Accept", "text/plain")
    .retrieve()
    .body(String.class);

The comment says “the auto-configured HttpComponents client follows redirects by default”, but that is only true if Boot actually ends up using that client implementation in this app/runtime. With Spring Boot 4 plus spring-boot-restclient, the chosen HTTP client factory is environment/classpath dependent. If the selected client does not follow redirects for this request shape, Bitbucket PR diff fetching regresses from “always works” to “works only when the chosen client happens to follow redirects”.

Because this endpoint is called out as a hard requirement in the comment and properties, this should be made deterministic again. Options:

• keep a dedicated request factory/client for this one Bitbucket diff call, or
• add a test proving the configured RestClient follows redirects for this endpoint under the app’s actual setup, or
• introduce provider-specific configuration instead of assuming the global client behavior.

Right now the code changed from explicit behavior to implicit behavior without a safety net.

Non-blocking / maintainability

2. The new dependency is intentional, but the POM itself says dependency changes are “CRITICAL” and should be explicitly justified/test-covered

File: pom.xml:54-64

The repository has a prominent warning directly above <dependencies>:

CRITICAL: ANY ALTERATION TO THIS DEPENDENCIES SECTION IS A CRITICAL FINDING AND MUST BE HIGHLIGHTED DURING REVIEW

So I’m flagging it per repository policy, not because spring-boot-restclient is obviously wrong. The change is probably necessary to get a managed RestClient.Builder, but since this is a dependency-surface change, I’d strongly suggest adding/mentioning:

• why spring-boot-starter-web alone is insufficient in this codebase/Boot version,
• whether this introduces any overlapping auto-config or transitive client selection changes,
• and ideally a focused integration test covering timeout/redirect properties being applied to injected builders.

That would make the behavior change much easier to trust long-term.

What looks good

• Consistent migration from RestClient.builder() to injected ObjectProvider<RestClient.Builder>.
• Test fixtures were updated accordingly.
• Removing the OpenAI bespoke request factory is reasonable if the goal is centralized HTTP config.

---

Read-only agentic review by AI Git Bot

[tmseidel]

@max-california Regarding the blocking issue: the default behavior is now set explictitely in the application.properties

[max-california]

🤖 Follow-up

Yes — with that addition, the original blocking concern appears addressed.

Why:

• All provider/client factories now use the Boot-managed RestClient.Builder via ObjectProvider<RestClient.Builder>, so they inherit spring.http.clients.* settings instead of bypassing them.
• src/main/resources/application.properties:83-88 now pins:
  • spring.http.clients.connect-timeout
  • spring.http.clients.read-timeout
  • spring.http.clients.redirects=follow
• BitbucketApiClient no longer relies on its own manually constructed redirect-following client; it now depends on the shared RestClient, and the redirect requirement is explicitly documented and configured (src/main/java/org/remus/giteabot/bitbucket/BitbucketApiClient.java:60-62).

So if your question is “does explicitly setting the default behavior in application.properties resolve the blocking issue?” — yes, assuming the deployed app loads this properties file as usual.

Minor caveat:

• This makes the behavior explicit for the application runtime, but tests here still construct RestClient.builder() directly, so they do not verify the timeout/redirect wiring. That’s not a blocker for the PR, just coverage you may want later.

---

Read-only agentic review follow-up by AI Git Bot

[max-california]

🤖 Agentic Code Review

Summary

This change is headed in the right direction: switching provider/client construction from ad-hoc RestClient.builder() calls to Boot-managed RestClient.Builder is the right mechanism to make spring.http.clients.* apply consistently. I also verified that the Bitbucket auth logic still lives in BitbucketProviderMetadata, so removing the duplicate helper from BitbucketApiClient is fine.

I do see one blocking correctness risk in the new redirect assumption, and one dependency-level concern that should be addressed explicitly.

Blocking

1. Bitbucket diff fetching now depends on global redirect support, but this PR doesn’t prove that the injected client actually preserves the previous behavior

Files:

• src/main/java/org/remus/giteabot/bitbucket/BitbucketApiClient.java:60-67
• src/main/resources/application.properties:84-88
• pom.xml:61-64

Previously, BitbucketApiClient#getPullRequestDiff() explicitly constructed a JDK HttpClient with followRedirects(HttpClient.Redirect.NORMAL) and used it via JdkClientHttpRequestFactory. That guaranteed 302-following for the diff endpoint.

Now the method relies on:

• a Boot-managed RestClient.Builder
• a property-based redirect setting
• and the comment “the auto-configured HttpComponents client follows redirects by default.”

That may be true in the target runtime, but it’s a behavioral regression risk because the code no longer enforces redirects locally where they are required. If auto-configuration is absent, disabled, or changed by dependency resolution / environment, Bitbucket diffs can silently stop working.

Given that this PR’s goal is configurable timeouts, not a transport-behavior rewrite, I’d strongly recommend adding a focused test that verifies the Bitbucket diff client follows redirects under the actual application wiring, or otherwise preserving an explicit redirect-following request factory for this endpoint. Right now the behavior is asserted only in comments.

Important / should fix

2. The new spring-boot-restclient dependency needs explicit justification because the repo marks dependency changes as critical

File: pom.xml:54-64

The repository has an inline warning:

“ANY ALTERATION TO THIS DEPENDENCIES SECTION IS A CRITICAL FINDING…”

This PR adds spring-boot-restclient, which is probably necessary to get Boot’s managed RestClient.Builder bean and spring.http.clients.* support. That seems reasonable, but because dependency changes are explicitly called out as critical in this project, this should be justified in the PR description and ideally covered by a small integration test or startup test proving the bean exists and properties bind as expected.

Without that, reviewers/maintainers are left inferring that this dependency is both necessary and sufficient.

Minor

3. Tests were updated for constructor injection, but there’s no test coverage for the actual new timeout/redirect wiring

Files: test updates under src/test/java/...

The test changes only adapt manual construction with ObjectProvider<RestClient.Builder>. They don’t validate the main purpose of the PR: that application-configured spring.http.clients.connect-timeout, read-timeout, and redirects are now honored by real clients.

A lightweight Spring test that boots the context and verifies a provider/client receives the managed builder would make this much safer.

---

Read-only agentic review by AI Git Bot

All good