refactor(openaimodel): split Chat Completions and Responses endpoints into sub-packages by wojtas000 · Pull Request #1643 · google/adk-go

wojtas000 · 2026-09-24T20:04:21Z

Link to Issue or Description of Change

Related: Support Third-Party Providers Using OpenAI-Compatible Format #341
Stacked on feat(openaimodel): support the Chat Completions API #1642, which should be reviewed and merged first; this PR stays a draft until then. Its base is feat(openaimodel): support the Chat Completions API #1642's branch, so the diff shows only the refactor commit.

Problem: Both endpoints live in one package, so every helper needs an endpoint prefix (buildChatParams beside buildOpenAIParams), and nothing keeps one path out of the other's internals.

Solution: Split into internal/responses and internal/completions, which use the same short names, and internal/shared for shared helpers and the error sentinels. model/openaimodel becomes a thin facade that aliases the sentinels, so errors.Is still matches.

Behavior change

None. The exported API is unchanged, and requests sent through the public API produce identical request bodies and errors before and after the split. Only %T of the returned model.LLM changes, since the unexported concrete types now live in the internal packages.

Testing Plan

go test -race, golangci-lint and go mod tidy -diff are clean.
Tests moved with their code (194 before, 196 after). Facade tests now check, through the real NewModel, endpoint selection, all 22 sentinels, that HTTPClient and Options are forwarded, and that HTTPOptions.Headers never reach the provider.
The OpenAI live checks from feat(openaimodel): support the Chat Completions API #1642 pass unchanged on this branch.

karolpiotrowicz

One thing needs fixing before merge, and it is in the tests rather than the code: after the move, no test fails if NewModel stops forwarding ClientConfig.HTTPClient.

The move itself holds up. Compared against the head of #1642 (8636ead), apidiff reports no change to the public package, every moved function body matches its original apart from renames, and build, vet and the full suite pass on this head and on its merge with the current #1642 head.

The HTTPClient forwarding in NewModel is no longer tested. On the #1642 head (test setup), TestModel_GenerateStream_StopsWhenTheConsumerStops built its model through NewModel with a spy transport, so it failed if the client was dropped. After the move the same test builds its model through newTestModel (call site), a copy of NewModel's option assembly, and the chat tests use newCompletionsModel. The facade tests do pass HTTPClient: server.Client() (here), but the server is plain HTTP on loopback, which the default client reaches just as well. I replaced the forwarding at openaimodel.go#L92-L94 with a no-op and ran go test ./model/openaimodel/...: all four packages pass on this head, while the same edit on the #1642 head fails TestModel_GenerateStream_StopsWhenTheConsumerStops. The property worth restoring is that some test driving the real NewModel fails when HTTPClient is not forwarded. APIKey, BaseURL and the API switch are still pinned through NewModel.

A few references went stale in the move. These are small and can go in the same push:

The Model doc comment links [NewModel], [ClientConfig] and [APIChatCompletions], which don't exist in package completions, so go doc cannot link them.
The RequestTimeout comment refers to applyGenerationConfig, which now lives in the two sibling packages.
A test comment still says callTracker.

The red apidiff check on this PR is not from this change. Its only incompatible entry is the removal of examples/openai, which happens in #1642.

wolo-lab

Two things to fix before merge: the new test helpers leak a socket on every call, and several names in the new packages still reflect where the code used to live.

The two loopback test helpers leak a listening socket per call. httptest.NewUnstartedServer already binds a listener, and both helpers replace server.Listener without closing it, so server.Close() only closes the replacement: openaimodel_test.go#L210-L215 and completions/model_test.go#L493-L498. Twenty newLoopbackServer subtests leave twenty sockets open, and closing the original listener before the assignment brings that to zero. The pattern predates this PR in responses/openai_test.go, but this change adds two more copies, and the two new ones also differ in name and in who registers the cleanup.

Names. The PR's own goal is short names that the package path qualifies, and the production code in completions and responses follows it. A few places do not yet:
- openaicommon repeats its path, since it already lives under model/openaimodel/internal/ (common.go#L22). Rename it to shared. I would avoid common, because common/common.go tends to become a catch-all, and for the same reason it would help to split the 672-line common.go into files named after what they hold (schema handling, config-field rejection, server-text clipping).
- The internal responses package shares its name with github.com/openai/openai-go/v3/responses, which all ten of its files import without an alias. Inside the package responses.X means the SDK, while in openaimodel.go responses.New means ours. Aliasing the SDK import there (say oairesponses) keeps the completions/responses pair symmetric.
- responses/openai.go and openai_test.go kept their old names. The Chat Completions side went from chat.go to model.go, so model.go and model_test.go would match.
- The completions tests still carry the chat prefix the production code dropped: chatRig, newChatRig, chatJSON, chatSSE, askChat and newCompletionsModel in model_test.go#L39-L83 and #L505, plus chatWire and chatMessages in request_test.go#L34-L51.
The packages are internal, so all of this is free outside model/openaimodel, and it is cheapest now, while every importing file is already in this diff.

wojtas000 · 2026-09-29T11:15:36Z

Updated. The branch is now eleven commits on #1642's head (63f23ed), one per change, so each fix below names the commit to open:

b205d5b: the split exactly as reviewed at b40848c, re-parented. Only doc.go differs, carrying feat(openaimodel): support the Chat Completions API #1642's text.
992330b: feat(openaimodel): support the Chat Completions API #1642's later Chat Completions fixes (57d37fc, 63f23ed) carried into internal/completions. They need three helpers from the Responses path, which move to the shared package with their bodies unchanged.
eaf52cb: @karolpiotrowicz's review.
1b87c57, 1c12b31, 4f13b0e, 56a5c4e, 25b1269, f60b519, 9d7dfe9: @wolo-lab's review, one commit per point.
fe87126: comments that still described code from where it used to live.

Through the public API, 22 requests on each API, 6 streamed replies and 2 blocking replies produce the same wire bodies, errors and responses as on 63f23ed.

@karolpiotrowicz, both in eaf52cb:

HTTPClient: TestNewModel_UsesTheConfiguredClient answers each API from an in-memory transport while BaseURL points at a loopback server that returns 418, so a model on the default client fails it. It checks Options the same way. Turning either forwarding line in NewModel into a no-op fails the test.
Stale references: the completions.Model doc, the RequestTimeout comment and the test comment no longer name identifiers from before the split.

@wolo-lab:

Socket leak, fixed in 25b1269: each package's newLoopbackServer is now httptest.NewServer plus t.Cleanup. httptest already binds 127.0.0.1 before trying IPv6, so replacing its listener bought nothing. Twenty calls of the old helper left 20 extra file descriptors open, and twenty of the new one leave none.
openaicommon is now internal/shared, split into calls.go, config.go, contents.go, response.go, schema.go, servertext.go and tools.go with their tests beside them (1b87c57). The SDK's shared is imported as oaishared wherever it meets ours.
responses/openai*.go are now model*.go (1c12b31).
Inside internal/responses the SDK package is imported as oairesponses (4f13b0e).
The completions test helpers dropped the chat prefix (56a5c4e).

wolo-lab

LGTM, thanks!

wojtas000 · 2026-09-30T08:52:05Z

@karolpiotrowicz @wolo-lab
I modified the docs in code to be provider-neutral (as some providers that were mentioned already support both Chat Completions and Responses and the docs were untruthy).

Need your reapproval.

The merge-base changed after approval.

The package held both conversion paths in one namespace, so every identifier either path wanted needed a prefix to avoid colliding with its twin: buildOpenAIParams beside buildChatParams, finishReason beside chatFinishReason, and about forty more. Give each endpoint a directory instead. internal/responses and internal/completions now use the same short names -- buildParams, convertContents, applyGenerationConfig, finishReason, convertUsage, streamTranslator -- and the import path tells them apart. Helpers neither endpoint owns, and the error sentinels, move to internal/openaicommon; the facade aliases the sentinels, so errors.Is is unaffected. Nothing exported changes: apidiff reports no difference against the previous commit. Tests move with the code they exercise. Tests of the public contract -- endpoint selection, sentinel identity, headers never reaching the provider -- run in the facade against the real NewModel. The moved files are as reviewed at b40848c. The Chat Completions fixes #1642 gained since then are carried into internal/completions by the next commit, so this one alone predates them. For: b/544805254

…/completions #1642 gained two commits after the split was written. 57d37fc makes history conversion read each part field on its own, drop parts holding only a thought signature and reject calls on non-model turns, and makes streaming pin chunk ids, fail a refused tool-call chunk and keep the snapshot's calls. 63f23ed stops a completion returning a call without a name. This applies both, with their tests, to the moved code under its new names. They use three helpers the split left in internal/responses, so partsWithoutCalls, replayedReasoning and unsupportedPayload move to internal/openaicommon, exported, with their bodies unchanged. The Responses path now calls them there.

After the split, the tests that exercised HTTPClient forwarding built their models through package-local helpers, so NewModel could drop ClientConfig.HTTPClient and every test still passed. A facade test now answers each API from an in-memory transport while BaseURL points at a failing server, so a model on the default client fails it. It checks ClientConfig.Options the same way, the other client setting only the facade handles. The headers test serves a completed response again and fails if the blocking call errors, as it did before the move. Also drops the Responses test helper's branches and field no caller reached, and fixes comments and test messages that still named identifiers from before the split.

…topic The package already sits under model/openaimodel, so the openai prefix repeated its path. It is now internal/shared, and its 758-line common.go is split into files named for what they hold: calls, config, contents, response, schema, servertext and tools, with their tests beside them. Declarations move verbatim. The SDK's own shared package is imported as oaishared wherever it meets ours, so shared.X always means this package.

The Chat Completions side already went from chat.go to model.go, so the Responses side and its test now match.

…nses Inside internal/responses, responses.X meant the SDK, while in the facade responses.New means our package. Aliasing the SDK import keeps the name for ours, matching how completions reads.

The production code in internal/completions dropped the chat prefix in the split; its test helpers now follow: testRig, newTestRig, serveJSON, serveSSE, ask, newTestModel, requestWire and wireMessages.

Each helper built its server with httptest.NewUnstartedServer, which has already bound a listener, then replaced that listener without closing it, so Close only closed the replacement and every call left a socket open. httptest binds 127.0.0.1 before trying IPv6 anyway, so the replacement bought nothing. The helper in each of the three packages is now newLoopbackServer on httptest.NewServer, and it registers its own cleanup, so the callers that closed the server themselves no longer need to.

The exported Model type and its Name method had no doc comment in internal/responses, and Name had none in internal/completions.

preserveSchemaNumbers and lowercaseSchemaTypes are called only from within internal/shared, so they no longer need to be exported.

Several comments moved into internal/shared still said "this package" or "Responses" for code both endpoints now use, and two test helpers called the facade the parent package. They now name what they mean.

The samples index listed seven providers as implementing only Chat Completions, but most of them now serve /v1/responses too. Which API a provider serves changes faster than these files, so the index, the Chat Completions sample README and the package doc now describe the choice with OpenAI settings and placeholders instead of naming DeepSeek, Groq or others. The warning about the OPENAI_API_KEY fallback stays.

… into sub-packages (google#1643) * refactor(openaimodel): split the two endpoints into sub-packages The package held both conversion paths in one namespace, so every identifier either path wanted needed a prefix to avoid colliding with its twin: buildOpenAIParams beside buildChatParams, finishReason beside chatFinishReason, and about forty more. Give each endpoint a directory instead. internal/responses and internal/completions now use the same short names -- buildParams, convertContents, applyGenerationConfig, finishReason, convertUsage, streamTranslator -- and the import path tells them apart. Helpers neither endpoint owns, and the error sentinels, move to internal/openaicommon; the facade aliases the sentinels, so errors.Is is unaffected. Nothing exported changes: apidiff reports no difference against the previous commit. Tests move with the code they exercise. Tests of the public contract -- endpoint selection, sentinel identity, headers never reaching the provider -- run in the facade against the real NewModel. The moved files are as reviewed at b40848c. The Chat Completions fixes google#1642 gained since then are carried into internal/completions by the next commit, so this one alone predates them. For: b/544805254 * refactor(openaimodel): carry the Chat Completions fixes into internal/completions google#1642 gained two commits after the split was written. 57d37fc makes history conversion read each part field on its own, drop parts holding only a thought signature and reject calls on non-model turns, and makes streaming pin chunk ids, fail a refused tool-call chunk and keep the snapshot's calls. 63f23ed stops a completion returning a call without a name. This applies both, with their tests, to the moved code under its new names. They use three helpers the split left in internal/responses, so partsWithoutCalls, replayedReasoning and unsupportedPayload move to internal/openaicommon, exported, with their bodies unchanged. The Responses path now calls them there. * test(openaimodel): pin that NewModel forwards the configured client After the split, the tests that exercised HTTPClient forwarding built their models through package-local helpers, so NewModel could drop ClientConfig.HTTPClient and every test still passed. A facade test now answers each API from an in-memory transport while BaseURL points at a failing server, so a model on the default client fails it. It checks ClientConfig.Options the same way, the other client setting only the facade handles. The headers test serves a completed response again and fails if the blocking call errors, as it did before the move. Also drops the Responses test helper's branches and field no caller reached, and fixes comments and test messages that still named identifiers from before the split. * refactor(openaimodel): rename openaicommon to shared and split it by topic The package already sits under model/openaimodel, so the openai prefix repeated its path. It is now internal/shared, and its 758-line common.go is split into files named for what they hold: calls, config, contents, response, schema, servertext and tools, with their tests beside them. Declarations move verbatim. The SDK's own shared package is imported as oaishared wherever it meets ours, so shared.X always means this package. * refactor(openaimodel): rename responses/openai.go to model.go The Chat Completions side already went from chat.go to model.go, so the Responses side and its test now match. * refactor(openaimodel): import the SDK's responses package as oairesponses Inside internal/responses, responses.X meant the SDK, while in the facade responses.New means our package. Aliasing the SDK import keeps the name for ours, matching how completions reads. * test(openaimodel): drop the chat prefix from completions test helpers The production code in internal/completions dropped the chat prefix in the split; its test helpers now follow: testRig, newTestRig, serveJSON, serveSSE, ask, newTestModel, requestWire and wireMessages. * test(openaimodel): stop the loopback test helpers leaking a socket Each helper built its server with httptest.NewUnstartedServer, which has already bound a listener, then replaced that listener without closing it, so Close only closed the replacement and every call left a socket open. httptest binds 127.0.0.1 before trying IPv6 anyway, so the replacement bought nothing. The helper in each of the three packages is now newLoopbackServer on httptest.NewServer, and it registers its own cleanup, so the callers that closed the server themselves no longer need to. * docs(openaimodel): document Model and Name in both endpoint packages The exported Model type and its Name method had no doc comment in internal/responses, and Name had none in internal/completions. * refactor(openaimodel): unexport the two schema helpers only shared calls preserveSchemaNumbers and lowercaseSchemaTypes are called only from within internal/shared, so they no longer need to be exported. * docs(openaimodel): name the package or endpoint stale comments mean Several comments moved into internal/shared still said "this package" or "Responses" for code both endpoints now use, and two test helpers called the facade the parent package. They now name what they mean. * docs(openaimodel): keep the OpenAI docs and samples provider-neutral The samples index listed seven providers as implementing only Chat Completions, but most of them now serve /v1/responses too. Which API a provider serves changes faster than these files, so the index, the Chat Completions sample README and the package doc now describe the choice with OpenAI settings and placeholders instead of naming DeepSeek, Groq or others. The warning about the OPENAI_API_KEY fallback stays.

This was referenced Sep 24, 2026

feat(openaimodel): support the Chat Completions API #1642

Merged

refactor(openaimodel): split Chat Completions and Responses endpoints into sub-packages #1626

Closed

wojtas000 marked this pull request as ready for review September 24, 2026 20:09

wojtas000 requested review from a team and karolpiotrowicz September 24, 2026 20:10

wojtas000 mentioned this pull request Sep 25, 2026

feat(model/openaimodel): support image/file input via genai InlineData/FileData parts #1543

Open

7 of 8 tasks

wojtas000 added this pull request to stack #1645 September 25, 2026 07:48

wojtas000 added the breaking-change label Sep 25, 2026

wojtas000 requested a review from wolo-lab September 27, 2026 21:11

karolpiotrowicz requested changes Sep 28, 2026

View reviewed changes

Comment thread model/openaimodel/internal/responses/openai_test.go Outdated

Comment thread model/openaimodel/openaimodel_test.go Outdated

wojtas000 removed the request for review from a team September 28, 2026 09:42

wolo-lab requested changes Sep 28, 2026

View reviewed changes

wojtas000 force-pushed the refactor/openaimodel-subpackages branch from b40848c to fe87126 Compare September 29, 2026 11:15

wolo-lab previously approved these changes Sep 29, 2026

View reviewed changes

wojtas000 requested a review from karolpiotrowicz September 30, 2026 07:12

karolpiotrowicz previously approved these changes Sep 30, 2026

View reviewed changes

wojtas000 dismissed stale reviews from karolpiotrowicz and wolo-lab via a8f9571 September 30, 2026 08:38

wojtas000 requested review from karolpiotrowicz and wolo-lab September 30, 2026 08:56

wolo-lab previously approved these changes Sep 30, 2026

View reviewed changes

wojtas000 removed the request for review from karolpiotrowicz September 30, 2026 09:29

wojtas000 dismissed wolo-lab’s stale review via 53c58c0 September 30, 2026 09:31

wojtas000 force-pushed the refactor/openaimodel-subpackages branch from a8f9571 to 53c58c0 Compare September 30, 2026 09:31

wolo-lab previously approved these changes Sep 30, 2026

View reviewed changes

Base automatically changed from feat/openaimodel-chat-completions to main September 30, 2026 09:46

wojtas000 added 11 commits September 30, 2026 11:46

refactor(openaimodel): rename responses/openai.go to model.go

44f7372

The Chat Completions side already went from chat.go to model.go, so the Responses side and its test now match.

refactor(openaimodel): import the SDK's responses package as oairespo…

be536be

…nses Inside internal/responses, responses.X meant the SDK, while in the facade responses.New means our package. Aliasing the SDK import keeps the name for ours, matching how completions reads.

test(openaimodel): drop the chat prefix from completions test helpers

83b99b8

The production code in internal/completions dropped the chat prefix in the split; its test helpers now follow: testRig, newTestRig, serveJSON, serveSSE, ask, newTestModel, requestWire and wireMessages.

docs(openaimodel): document Model and Name in both endpoint packages

bcac987

The exported Model type and its Name method had no doc comment in internal/responses, and Name had none in internal/completions.

refactor(openaimodel): unexport the two schema helpers only shared calls

024d883

preserveSchemaNumbers and lowercaseSchemaTypes are called only from within internal/shared, so they no longer need to be exported.

docs(openaimodel): name the package or endpoint stale comments mean

6ed6878

Several comments moved into internal/shared still said "this package" or "Responses" for code both endpoints now use, and two test helpers called the facade the parent package. They now name what they mean.

wojtas000 force-pushed the refactor/openaimodel-subpackages branch from 53c58c0 to 12d40ca Compare September 30, 2026 09:46

wolo-lab approved these changes Sep 30, 2026

View reviewed changes

wojtas000 merged commit 7c4ccf7 into main Sep 30, 2026
14 checks passed

Navigation Menu

Repository navigation

refactor(openaimodel): split Chat Completions and Responses endpoints into sub-packages - #1643

refactor(openaimodel): split Chat Completions and Responses endpoints into sub-packages#1643
wojtas000 merged 12 commits into
mainfrom
refactor/openaimodel-subpackages

wojtas000 commented Sep 24, 2026 •

edited

Loading

Uh oh!

karolpiotrowicz left a comment

Uh oh!

Uh oh!

Uh oh!

wolo-lab left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wojtas000 commented Sep 29, 2026

Uh oh!

wolo-lab left a comment

Uh oh!

wojtas000 commented Sep 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wojtas000 commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to Issue or Description of Change

Behavior change

Testing Plan

Uh oh!

karolpiotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wolo-lab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wojtas000 commented Sep 29, 2026

Uh oh!

wolo-lab left a comment

Choose a reason for hiding this comment

Uh oh!

wojtas000 commented Sep 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wojtas000 commented Sep 24, 2026 •

edited

Loading