[sisantaChhatoi]

Description

The Chat Completions, Responses and Anthropic/Bedrock clients each carried their own copy of the same setup code. This extracts the shared pieces into helpers. The behavior should not change. Outside tests the diff is +93/−148, and the only new comments are the ones that explain a constraint.

• applyClientDefaults: the timeout and session-key defaults repeated in all four constructors.
• sdkMiddlewares: the retry-codes → raw → retry-observer middleware list. Both SDKs' option.Middleware alias the same function type, so one list serves every constructor. The raw-before-observer ordering rule, which three constructors pointed back to with "see NewOpenAIClient", now lives in one place. In the Bedrock client, the middleware is still installed before bedrock.WithConfig, so request signing still runs innermost.
• newOpenAISDK: the OpenAI SDK options shared by the Chat Completions and Responses clients. Callers pass the retry count and HTTP client, so the Responses constructor keeps its ChatGPT-plan setup (no retries, no redirect following) and the Chat client passes the same defaults it used before. URL normalization stays in each constructor, because the two keep different trailing-slash contracts on cfg.URL (see TestNewOpenAIClient_URLNormalization).
• openAIRequestOptions: per-request extra_headers/extra_body expansion. Each client passes its own skipped keys (stream and stream_options for Chat Completions, stream for Responses), and each explanatory comment stays at its call site. In ChatGPT-plan mode the Responses client still sends its plan token first. The Anthropic loop keeps its own body handling because of its thinking rules, and only shares the session-key lookup (requestSessionKey).
• finalizeOnExit: the deferred finalize-and-recover closure copied into all three CompletionsWithCtx methods. It is deferred directly, so recover still takes effect, and it applies describeTimeout to the returned error before finalizing, as each copy did. The reason a panic has to finalize stays documented on errRequestPanicked.

The request builders and response mappers are untouched, because that is where the Chat Completions and Responses wire formats actually differ.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

make check passes and make coverage reports 92.1%. New tests for finalizeOnExit: a panic is recorded as a failed request and still propagates, and the deferred call reads the error the function actually returned, not the value err held when the defer line ran. Until now the panic path was only tested at the finalizeRequest level.

TestNewOpenAIResponsesClientPlanModeSDKSettings builds the Responses client in API-key and ChatGPT-plan mode against a local server and checks the retry count and redirect handling. The existing ChatGPT tests replace the SDK client, so the settings the constructor passes to newOpenAISDK were otherwise untested. TestRequestTimeoutAndSDKRetryBehavior covers describeTimeout through all three clients.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA
• I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI disclosure: prepared with Claude Code (Claude Opus 5.5). I reviewed every line and can explain each change.

Related Issues

None.

🤖 Generated with Claude Code

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

[Qiyuanqiii]

I did a second pass including documentation impact, comments, naming, and maintainability.

The extracted helpers preserve the existing client behavior: protocol-specific URL normalization remains at the individual constructors, the middleware order is unchanged, per-request session-key expansion still follows the same precedence, and finalizeOnExit preserves both the named return error and panic propagation.

This is an internal refactor, so I don't see a user-facing documentation update that is required.

The comments are concise and useful. One minor naming debt is that sdkMiddlewares returns []retryObserver even though that common function type is also used for retry-code and raw-capture middleware. A future rename of the alias to something like sdkMiddleware would describe its role more accurately, but that alias predates this change and I would not block this refactor on it.

One merge-order note: #1565 changes the same client-construction area. If #1565 lands first, please make sure the rebase preserves its key-failover middleware and ordering; I would re-review the resulting head after that conflict is resolved.

For the current head against the current main, I have no blocking findings. Approved.

[sisantaChhatoi]

Heads up on the latest push: it's a rebase onto current main with conflict resolution. main reworked the Responses client in the meantime (ChatGPT plan support, timeout descriptions), so newOpenAISDK now takes the retry count and HTTP client to keep plan mode's no-retry/no-redirect setup, and finalizeOnExit applies describeTimeout like each copy did. I also added TestNewOpenAIResponsesClientPlanModeSDKSettings to cover those plan-mode settings. Details are in the description. The red test/cross-compile checks stop at "Verify generated provider presets" on the self-hosted runners, same as other PRs right now; they pass locally.