Repository navigation
fix(model/openaimodel): stop replaying prior-turn reasoning to the API - #1395
Conversation
|
The drop is the right call, and the conversion holds where it matters: no part marked as a thought contributes text to any emitted item, on any role and in any position, and a thought-marked call or response still survives with its pairing intact. Exercising it turned up one consequence worth a decision before this lands, and three smaller things after it. The run loop stops terminating on a reasoning-only turn. Driving the test those two runs come from — drop it in
|
Review of google#1395 found three shapes the drop got wrong and one sentence of package doc it made untrue. A thought carrying reasoning text on top of an image or code matched no arm at all: the text was suppressed, the call and response arms did not apply, and the rejection arm was gated on part.Text == "", which was false. The media left the request with no error, against the promise the same change had just made. The arm now keys on whether the part contributed text rather than on whether it had any, so suppressing the text cannot suppress the rejection with it. replayedReasoning listed the fields that disqualified a part, which left ToolCall, ToolResponse and AudioTranscription out and dropped them silently where main reported an unsupported part. The test is inverted: clear the reasoning, its signature, and the metadata that only qualifies media held in another field, and a part is droppable when what remains is the zero value. A field added by a later genai release is then rejected by default instead of vanishing, and a field walk over genai.Part in the tests keeps that honest. An unsupported role stopped being reported when a turn's only text was a thought: nothing buffered, so flushText returned before normalizeRole ran. The role is now checked wherever text is dropped, so the error is raised identically with the marker on or off. The doc paragraph claimed reasoning is never sent back on a later turn, unqualified. In a multi-agent run the shared flow rebuilds a peer's event as plain user text with the thought marker unset, so a peer's reasoning still arrives as ordinary content; the paragraph now says so and says it is not this package's to fix. The unbounded run loop the review also raised is closed on main by google#1290, which bounds consecutive thought-only turns; driving runner.Run against a stub that only ever reasons now ends after 10 calls with the request body flat at 382 bytes, where before this change it grew until the provider refused it.
…art carries Review of the previous commit found the rejection still keyed on what the part contributed, which only moved the hole. A thought carrying text on top of an image was reported, but the same image riding on ordinary text, or on a function call, still matched a sendable arm first and left the request with no error at all. That predates this branch, and it is what made the promise the branch had just written down untrue. The check is now made once per part, before anything is emitted, and asks the opposite question: not "which fields disqualify this part" but "is anything set that convertContents does not account for". Text, the thought marker and its signature, a call, a response, and the three metadata fields that only qualify media carried elsewhere are cleared from a copy; whatever is left over is named in the error. So an image reaches the caller as "unsupported content part: InlineData" no matter what it travels with, and a field added to genai.Part by a later release is reported rather than silently dropped, because it is unaccounted for by construction. replayedReasoning now shares that test instead of restating it, and the arm that used to serve both jobs is left with the one it is for: a part that contributed nothing and is not reasoning worth dropping now says it carries nothing to send, rather than borrowing the unsupported-part wording. On the tests: describeInput rendered an assistant input message and an assistant output message identically, so the table could not have seen a replayed assistant turn go out as "input_text" — the shape the Responses API rejects. They now render as in/ and out/. The genai.Part field walk is stated against the accounted-for list and fails loudly on an unexported field instead of panicking, and it runs against unsupportedPayload as well as the drop. Two tests drive the public GenerateContent surface, blocking and streaming, asserting on the bytes that leave the process: the reasoning is absent and the answer is not, and a request the drop empties never reaches the model. Each of the seven guards was reverted individually and the suite re-run; every one is caught by a named test.
…just the predicate Second review round. The field walk asserted what unsupportedPayload returns, which says nothing about whether convertContents passes it on: a mutation that swallowed the error for one field would leave the walk green. Each generated case now also builds the request and requires the field's name in the error, so the unit and the caller are held to the same answer. An empty part had no case either. buildContentsDefault filters those out upstream, so it only reaches convertContents from a caller assembling contents itself, which is exactly why the contract is worth pinning: it is reported, not quietly skipped. The "carries nothing to send" error is built with errors.New rather than a verbless fmt.Errorf, matching errors.go.
9dd6a6f to
ee104ef
Compare
|
Thanks — all four landed, and the fifth needed nothing. Rebased onto
Two more fell out of the rest of the review:
Verified: each of the seven guards reverted individually and the suite re-run, every one caught by a named test; gates green in both modules; and exercised live on One behavior change worth your eye, also in the PR body: a part carrying an unsendable field alongside sendable text or a call now errors instead of building a request with that field silently discarded — it never reached the API either way, but a caller who was getting a 200 now gets an error. |
… on the drop Review found the two of them gated on the wrong thing. The wrap keyed on the request having contents, so a Content carrying no parts at all reported "every part was dropped as replayed reasoning or blank text" when nothing had been dropped, and turned the bare sentinel into a wrapped one — err == ErrNoContents flipped from true to false for a request that behaves exactly as it did before this branch. convertContents now reports whether it dropped anything and the wrap follows that, so the bare sentinel survives for a request that arrived empty, blank text included, and the message no longer has to hedge about which of the two emptied it. The role check keyed on the part having text, so it ran for a thought carrying reasoning and not for one carrying only the marker or a bare signature — which main rejected as an unsupported part, and this branch had made silent. It now runs wherever a part is dropped, so an unsupported role is reported on any of the three. Also pinned, because neither the drop nor the pairing was ever the subject of a test here: text riding on a function response that pairs with nothing, and on a call with no name. Both used to be discarded because the text arm matched first, and are reported now that a part's fields are read independently.
|
Second pass over my own follow-up caught three more, all introduced by it rather than by the original commit. Pushed as
Gates green in both modules, the ten guards each reverted individually and caught by a named test, and the live suite's failures are identical to |
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
All five landed, and I re-ran the one that mattered most rather than taking it on trust. Driving runner.Run against a stub that only ever reasons, with the same body-size budget as before: this branch makes 10 model calls with the request body flat at 382 bytes and ends on its own, where main before the drop grew the body every turn until the stub refused it. The bound is maxConsecutiveThoughtOnlyTurns at base_flow.go:120, so the cap question is closed on main and needs nothing here.
the two runs, from the same test I posted in August
before the fix (main) model calls: 6
request body sizes: [196 509 636 763 890 1017]
outcome: TERMINATED BY ERROR: 400 context_length_exceeded
with the PR model calls: 10
request body sizes: [196 382 382 382 382 382 382 382 382 382]
outcome: RAN TO COMPLETION ON ITS OWN
The other four check out against a run rather than a reading. I built ~25 part shapes and put each through buildOpenAIParams at both revisions: an image or a ToolCall or an AudioTranscription riding on text now errors naming the field where main dropped it silently, a bare thought or a lone signature on an invalid role now reports the role where main reported an unsupported part, and go doc -all differs from main by exactly the qualified paragraph. Two shapes are worth calling out because they show the inversion earning its keep beyond what I asked for: a part carrying reasoning text, a call and an image had both the image and the call dropped on main, and a well-formed call riding on text used to be discarded, which then orphaned its own response and failed the whole conversion — that shape now converts correctly.
Nothing here blocks. Two sentences in the description claim more than the code does, and both are a clause away from being true.
err == ErrNoContents does not survive a blank thought. The description says the identity holds "for every request that was empty on arrival, including blank/whitespace-only text". For {Text: " ", Thought: true} it does not: main returns the bare sentinel, this branch returns the wrapped one. The code is behaving exactly as request.go:54-64 says it will, since a part genuinely was dropped as reasoning — it is the sentence that is wrong, and the fix is to drop "including blank/whitespace-only text" or to trim before setting the flag.
&model.LLMRequest{Contents: []*genai.Content{modelTurn(&genai.Part{Text: " ", Thought: true})}}
// main: err == ErrNoContents true
// this branch: err == ErrNoContents false (errors.Is still true)"Reported by name, never silently discarded" is not true of the four accounted qualifier fields. ThoughtSignature, VideoMetadata, MediaResolution and PartMetadata are zeroed at request.go:242-249, so a part carrying one alongside text is converted with the field gone, and a part carrying one alone gets content part carries nothing to send — which names nothing and is not quite true either, since the part does carry something. Both are identical to main, so nothing regresses. I would narrow the claim to fields outside the accounted set rather than change the code, since three of those four genuinely have nowhere to go in a Responses item.
Four smaller things are on the lines they belong to, all of them non-blocking.
One thing outside this PR that the doc paragraph is right to flag: ConvertForeignEvent at contents_processor.go:596-599 rebuilds a peer's text part with the marker unset, so a sub-agent's reasoning still reaches every model package as ordinary user text. Worth its own issue against internal/llminternal rather than holding this.
Gates are green on both modules apart from telemetry, which fails the same way on main in the same sandbox because it dials the metadata server. No exported surface moves.
Review found the empty-request wrap firing for a part whose reasoning text was only whitespace. The drop is real there, but the text would have been skipped by newMessage anyway, so the drop is not what emptied the request — and wrapping turned err == ErrNoContents from true to false for a shape that behaves exactly as it does on main. The flag is now set only for reasoning that carried text the model would otherwise have seen, which also gives the bare sentinel back to a bare thought and a lone signature. The rejection for a part that reaches nothing keeps the unsupported-content-part prefix the single message used before this branch, so a caller string-matching on that phrase still matches the shapes that now land there — an empty part, and one carrying only VideoMetadata, MediaResolution or PartMetadata. The comment above the payload check claimed a part is read field by field. Text is, against a call or a response, but a call and a response on the same part are still arms of one switch and the response is dropped, as on main. The comment now says what the code does rather than what the phrase implies. Two test gaps the review named: the identity table gains the blank-reasoning case the claim turns on, plus the bare thought and lone signature, and the genai.Part field walk now puts each unsendable field on text, on reasoning text and on a function call rather than only on a part of its own, since the riding shape is the one this change exists to fix.
…the doc Review round found the flag's accumulation untested: writing it as an assignment rather than a conditional set passed the whole suite, so a blank thought following a real one could have talked the emptied-request report back down. Two cases pin it, within a turn and across turns, and one more pins the blank-skip the trim rule leans on — that a blank text item is dropped beside a real one rather than only when it is alone. The package doc claimed a single exception to reasoning never being sent back. There are three: a peer agent's reply folded into context, a compacted transcript, and a memory recall all render reasoning as ordinary text upstream of any model package. It now says that rather than naming one.
…across turns Second review round. The across-turns case put the real thought in the last turn, so a flag reset per content block would have passed it; the real turn now comes first, which is the order that catches it. The blank skip was pinned only on the assistant path through newOutputMessage, leaving the user path through newMessage untested both beside real text and on its own. The field walk asserted the error contains the field name, which a shorter name that prefixes a longer one would satisfy; it now asserts the suffix. Three doc comments ran past the two-sentence limit this package keeps.
|
Both description claims fixed, and the four line notes are answered on their threads.
Since Gates green in both modules. |
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Both description claims from the last round are closed, and I re-ran the one that mattered: {Text: " ", Thought: true} now returns the bare ErrNoContents on this branch exactly as it does on main, and so do a bare thought and a lone signature. Taking the trim over the wording was the better of the two options I offered, since it fixes the behavior instead of documenting it. The four line notes are answered on their threads. adk-python's Responses backend does drop replayed reasoning for the reason the description gives, at _openai_responses_llm.py:307-323, dispatched from :436-438. Nothing here blocks.
Two sentences in the description are contradicted by measurement. Both are a clause away from being true and neither needs a code change.
"No shape that succeeded on the wire now fails" is falsified by the table two rows above it. Putting the corpus through the conversion at the merge base and at this branch, three shapes move from a built request to a hard error: an InlineData blob riding on plain text, a ToolCall riding on text, and text riding on a FunctionResponse that pairs with nothing. Each of those returned a 200 on main with the field quietly discarded. The table already says so for the first two — "succeeded, field silently discarded" becomes "error naming the field" — and then the prose says the opposite. Narrowing it to shapes whose payload actually reached the provider would keep the claim you can defend.
"Sixteen guards reverted individually ... every one is caught by a named test" does not hold for one of them. Deleting part.Text != "" || from request.go:155 leaves go test ./model/openaimodel/ green, and changes what a caller sees through the public surface:
req := &model.LLMRequest{Contents: []*genai.Content{{
Role: "assistant",
Parts: []*genai.Part{{
Thought: true,
Text: "scratch",
FunctionCall: &genai.FunctionCall{Name: "lookup", ID: "c1"},
}},
}}}
for _, err := range llm.GenerateContent(ctx, req, false) { ... }
// this branch: err = openai: unsupported role "assistant" provider calls: 0
// guard deleted: err = <nil> provider calls: 1With the guard gone the request reaches the provider under a role the package cannot send. main errors on that shape too, so this is a coverage hole rather than a regression — but it is the line that keeps the role check honest when reasoning text rides on a call, which is the case the round-2 fix was for. The four role cases at request_test.go:503-542 all use parts carrying nothing but reasoning, and thought_text_riding_on_a_call uses modelTurn, so nothing exercises the disjunct. The shape is unreachable from ADK itself, which only ever builds user and model roles, so this is about the claim rather than about a user hitting it.
One small thing, and I would leave it alone if the churn is not worth it. The wrapped message at request.go:56-61 says "every part was dropped as replayed reasoning" for a request where only some part was: [{Text: "scratch", Thought: true}, {Text: " "}] gets the wrap, and the blank sibling was buffered and then skipped downstream, not dropped as reasoning. Diagnostic wording only, and rewording it costs two pinned test expectations.
One thing to leave alone. The two guards in replayedReasoning that look unobservable — the signature-on-an-answer early return, and the unsupportedPayload conjunct — are reachable only through the predicate's own tests, but removing either breaks TestReplayedReasoning_EveryUnaccountedFieldDisqualifies, which is the executable form of the forward-compatibility rule we settled last round.
Gates are green on both modules, the merge into current main is clean, and no exported surface moves. Two open PRs of yours, #1392 and #1389, conflict with this one on the same three files — whichever lands first, the other needs a rebase.
Superseded by the approving review. The items this review named are verified closed at the current head.
Brings in google#1392, which landed while this branch was in review. The only conflict is doc.go, where both add a paragraph at the same anchor: google#1392's table of how every GenerateContentConfig field is treated, and this branch's account of why reasoning is not replayed. They describe different things, so both are kept, the landed table first and this branch's paragraph after it. go doc renders both. Nothing else conflicts, and the overlap is narrower than the shared file list suggests. Both branches rewrote request.go, but in disjoint functions — this one in convertContents, replayedReasoning and unsupportedPayload, google#1392 in applyGenerationConfig and the tables it drives — and no test function in request_test.go or openai_test.go was touched by both, so neither side's cases inherit a field the other added. Checked rather than assumed: every function each side added to request.go is present; the reflection guards on both sides still run and pass (TestReplayedReasoning_EveryUnaccountedFieldDisqualifies, TestEveryUnsupportedHTTPOptionFieldIsDriven and the three accounting tests); and neutering replayedReasoning or rejectUnsupportedConfigFields each turns the package red, so the merge left both sides driven.
karolpiotrowicz
left a comment
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Nothing blocks. I re-checked the branch merged with current main (d64e113), and the drop still holds there: no reasoning text reaches any request body, blocking or streaming, and a function call with its output, an answer and a refusal all replay intact on the turns after them.
The fix matters more now than when this was opened. At the time openaimodel never asked for reasoning, so a history carrying thought parts needed the raw-options escape hatch. Since #1392, ThinkingConfig{IncludeThoughts: true} sends reasoning.summary=auto (request.go:536-537), the summaries come back as thought parts (response.go:184), and on main the next turn sends them to the API as assistant text. Two turns through runner.Run against a stub show it:
main without this PR: turn 2 replays the scratchpad: true
main with this PR: turn 2 replays the scratchpad: false
the test — drop it in model/openaimodel and run go test -run TestThinkingSummaryReplay -v ./model/openaimodel/
package openaimodel_test
import (
"context"
"io"
"net/http"
"net/http/httptest"
"strings"
"testing"
"google.golang.org/genai"
"google.golang.org/adk/v2/agent"
"google.golang.org/adk/v2/agent/llmagent"
"google.golang.org/adk/v2/model/openaimodel"
"google.golang.org/adk/v2/runner"
"google.golang.org/adk/v2/session"
)
func TestThinkingSummaryReplay(t *testing.T) {
const turn = `{"id":"resp_1","object":"response","created_at":1,"status":"completed","model":"m",
"output":[{"type":"reasoning","id":"rs_1","summary":[{"type":"summary_text","text":"PRIVATE-SCRATCHPAD"}]},
{"type":"message","id":"msg_1","role":"assistant","status":"completed",
"content":[{"type":"output_text","text":"4","annotations":[]}]}],
"usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2}}`
var bodies []string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
b, _ := io.ReadAll(r.Body)
bodies = append(bodies, string(b))
w.Header().Set("Content-Type", "application/json")
io.WriteString(w, turn)
}))
defer srv.Close()
ctx := context.Background()
m, _ := openaimodel.NewModel(ctx, "m", &openaimodel.ClientConfig{APIKey: "k", BaseURL: srv.URL})
a, _ := llmagent.New(llmagent.Config{Name: "assistant", Model: m,
GenerateContentConfig: &genai.GenerateContentConfig{
ThinkingConfig: &genai.ThinkingConfig{IncludeThoughts: true}}})
r, _ := runner.New(runner.Config{AppName: "app", Agent: a,
SessionService: session.InMemoryService(), AutoCreateSession: true})
for _, q := range []string{"what is 2+2?", "and 3+3?"} {
for _, err := range r.Run(ctx, "u", "s", genai.NewContentFromText(q, genai.RoleUser), agent.RunConfig{}) {
if err != nil {
t.Fatal(err)
}
}
}
t.Logf("turn 1 asks for summaries: %v", strings.Contains(bodies[0], `"reasoning":{"summary":"auto"}`))
t.Logf("turn 2 replays the scratchpad: %v", strings.Contains(bodies[1], "PRIVATE-SCRATCHPAD"))
}The two hand-resolved conflicts in doc.go keep both sides: the generation-config table and the strict-tools paragraph from main are intact, and the reasoning paragraph follows them once.
One non-blocking thing. The live e2e run in the Testing Plan was made on an earlier head (a3de3b5) with openai-go v3.54.0, and main has since moved to openai-go v3.61.0 and genai v1.71.0. A re-run on the current head would keep that section describing what actually merges.
…model-subpackages Brings in main through google#1395, which changed request.go after this branch split it. Its reasoning drop and unsupported-payload check are applied to internal/responses/request.go, and its tests merged into internal/responses under the renamed identifiers. The request corpus is identical on both branches.
…a/FileData parts convertContents only handled Text, FunctionCall and FunctionResponse parts; any genai.Part.InlineData or FileData hit the default branch and failed with "openai: unsupported content part *genai.Part". Convert both into the Responses API's input_image / input_file content items, mirroring adk-python's _inline_data_part_to_response_content and _file_data_part_to_response_content: - InlineData: image MIME -> input_image with a data:<mime>;base64,<...> URL; anything else -> input_file carrying the same data URL in file_data. - FileData: image MIME -> input_image with the URI passed through; a URI shaped like an OpenAI file id (file-...) -> input_file via FileID; any other URI -> input_file via FileURL. - A message mixing text and media is emitted as a single message whose content list preserves part order, replacing the text-only accumulator. - A replayed assistant turn carrying media now fails loudly instead of silently dropping it, since the output-message content type is text-only. Rebased onto main, which has since landed google#1395, google#1614, google#1616, google#1389, google#1392 and google#1359 in this package. The conflict was semantic rather than textual, so the resolution is spelled out here: - convertContents keeps main's (items, droppedReasoning, error) signature and its unsupportedPayload guard; the text accumulator it flushes is the ordered content list this change introduced. - Media is read independently of text, the way main already reads text independently of a call, and is appended after the text of the same part so the content list keeps the order the parts arrived in. - unsupportedPayload now zeroes InlineData and FileData: it reports fields the package cannot send, and it can send these two now. accountedForFields, the restated spec in the test, gains the same two entries. - replayedReasoning no longer treats a thought-marked part carrying media as reasoning that can be dropped, because dropping it would lose content. - The carries-nothing arm accounts for media, so a part holding only an image is not reported as carrying nothing to send. Four cases in TestBuildOpenAIParams_DropsReplayedThoughts pinned the previous contract, that media is unsendable, and now pin the new one. Three become positive cases; media on a model turn stays a rejection, but for the narrower reason that an output message carries output_text only, so the message changes. TestBuildOpenAIParams_UnsupportedPart keeps main's ExecutableCode body, with its t.Fatalf text corrected from "inline data part" to the part it actually exercises. Fixes google#1333 Assisted-by: Claude Code/claude-opus-5[1m] Machine: MacBook-Anton Account: a Operator: robot:git-s29-anchor-ladder Signed-off-by: Anton Dziatkovskii <194927794+tonydzi@users.noreply.github.com>
google#1395) * fix(model/openaimodel): stop replaying prior-turn reasoning to the API * fix(model/openaimodel): close the review gaps in the reasoning drop Review of google#1395 found three shapes the drop got wrong and one sentence of package doc it made untrue. A thought carrying reasoning text on top of an image or code matched no arm at all: the text was suppressed, the call and response arms did not apply, and the rejection arm was gated on part.Text == "", which was false. The media left the request with no error, against the promise the same change had just made. The arm now keys on whether the part contributed text rather than on whether it had any, so suppressing the text cannot suppress the rejection with it. replayedReasoning listed the fields that disqualified a part, which left ToolCall, ToolResponse and AudioTranscription out and dropped them silently where main reported an unsupported part. The test is inverted: clear the reasoning, its signature, and the metadata that only qualifies media held in another field, and a part is droppable when what remains is the zero value. A field added by a later genai release is then rejected by default instead of vanishing, and a field walk over genai.Part in the tests keeps that honest. An unsupported role stopped being reported when a turn's only text was a thought: nothing buffered, so flushText returned before normalizeRole ran. The role is now checked wherever text is dropped, so the error is raised identically with the marker on or off. The doc paragraph claimed reasoning is never sent back on a later turn, unqualified. In a multi-agent run the shared flow rebuilds a peer's event as plain user text with the thought marker unset, so a peer's reasoning still arrives as ordinary content; the paragraph now says so and says it is not this package's to fix. The unbounded run loop the review also raised is closed on main by google#1290, which bounds consecutive thought-only turns; driving runner.Run against a stub that only ever reasons now ends after 10 calls with the request body flat at 382 bytes, where before this change it grew until the provider refused it. * fix(model/openaimodel): reject unsendable payload whatever else the part carries Review of the previous commit found the rejection still keyed on what the part contributed, which only moved the hole. A thought carrying text on top of an image was reported, but the same image riding on ordinary text, or on a function call, still matched a sendable arm first and left the request with no error at all. That predates this branch, and it is what made the promise the branch had just written down untrue. The check is now made once per part, before anything is emitted, and asks the opposite question: not "which fields disqualify this part" but "is anything set that convertContents does not account for". Text, the thought marker and its signature, a call, a response, and the three metadata fields that only qualify media carried elsewhere are cleared from a copy; whatever is left over is named in the error. So an image reaches the caller as "unsupported content part: InlineData" no matter what it travels with, and a field added to genai.Part by a later release is reported rather than silently dropped, because it is unaccounted for by construction. replayedReasoning now shares that test instead of restating it, and the arm that used to serve both jobs is left with the one it is for: a part that contributed nothing and is not reasoning worth dropping now says it carries nothing to send, rather than borrowing the unsupported-part wording. On the tests: describeInput rendered an assistant input message and an assistant output message identically, so the table could not have seen a replayed assistant turn go out as "input_text" — the shape the Responses API rejects. They now render as in/ and out/. The genai.Part field walk is stated against the accounted-for list and fails loudly on an unexported field instead of panicking, and it runs against unsupportedPayload as well as the drop. Two tests drive the public GenerateContent surface, blocking and streaming, asserting on the bytes that leave the process: the reasoning is absent and the answer is not, and a request the drop empties never reaches the model. Each of the seven guards was reverted individually and the suite re-run; every one is caught by a named test. * test(model/openaimodel): prove the rejection reaches the caller, not just the predicate Second review round. The field walk asserted what unsupportedPayload returns, which says nothing about whether convertContents passes it on: a mutation that swallowed the error for one field would leave the walk green. Each generated case now also builds the request and requires the field's name in the error, so the unit and the caller are held to the same answer. An empty part had no case either. buildContentsDefault filters those out upstream, so it only reaches convertContents from a caller assembling contents itself, which is exactly why the contract is worth pinning: it is reported, not quietly skipped. The "carries nothing to send" error is built with errors.New rather than a verbless fmt.Errorf, matching errors.go. * fix(model/openaimodel): key the empty-request wrap and the role check on the drop Review found the two of them gated on the wrong thing. The wrap keyed on the request having contents, so a Content carrying no parts at all reported "every part was dropped as replayed reasoning or blank text" when nothing had been dropped, and turned the bare sentinel into a wrapped one — err == ErrNoContents flipped from true to false for a request that behaves exactly as it did before this branch. convertContents now reports whether it dropped anything and the wrap follows that, so the bare sentinel survives for a request that arrived empty, blank text included, and the message no longer has to hedge about which of the two emptied it. The role check keyed on the part having text, so it ran for a thought carrying reasoning and not for one carrying only the marker or a bare signature — which main rejected as an unsupported part, and this branch had made silent. It now runs wherever a part is dropped, so an unsupported role is reported on any of the three. Also pinned, because neither the drop nor the pairing was ever the subject of a test here: text riding on a function response that pairs with nothing, and on a call with no name. Both used to be discarded because the text arm matched first, and are reported now that a part's fields are read independently. * fix(model/openaimodel): let a blank thought keep the bare ErrNoContents Review found the empty-request wrap firing for a part whose reasoning text was only whitespace. The drop is real there, but the text would have been skipped by newMessage anyway, so the drop is not what emptied the request — and wrapping turned err == ErrNoContents from true to false for a shape that behaves exactly as it does on main. The flag is now set only for reasoning that carried text the model would otherwise have seen, which also gives the bare sentinel back to a bare thought and a lone signature. The rejection for a part that reaches nothing keeps the unsupported-content-part prefix the single message used before this branch, so a caller string-matching on that phrase still matches the shapes that now land there — an empty part, and one carrying only VideoMetadata, MediaResolution or PartMetadata. The comment above the payload check claimed a part is read field by field. Text is, against a call or a response, but a call and a response on the same part are still arms of one switch and the response is dropped, as on main. The comment now says what the code does rather than what the phrase implies. Two test gaps the review named: the identity table gains the blank-reasoning case the claim turns on, plus the bare thought and lone signature, and the genai.Part field walk now puts each unsendable field on text, on reasoning text and on a function call rather than only on a part of its own, since the riding shape is the one this change exists to fix. * test(model/openaimodel): pin the drop flag across parts, and correct the doc Review round found the flag's accumulation untested: writing it as an assignment rather than a conditional set passed the whole suite, so a blank thought following a real one could have talked the emptied-request report back down. Two cases pin it, within a turn and across turns, and one more pins the blank-skip the trim rule leans on — that a blank text item is dropped beside a real one rather than only when it is alone. The package doc claimed a single exception to reasoning never being sent back. There are three: a peer agent's reply folded into context, a compacted transcript, and a memory recall all render reasoning as ordinary text upstream of any model package. It now says that rather than naming one. * test(model/openaimodel): cover the user-turn blank skip and the flag across turns Second review round. The across-turns case put the real thought in the last turn, so a flag reset per content block would have passed it; the real turn now comes first, which is the order that catches it. The blank skip was pinned only on the assistant path through newOutputMessage, leaving the user path through newMessage untested both beside real text and on its own. The field walk asserted the error contains the field name, which a shorter name that prefixes a longer one would satisfy; it now asserts the suffix. Three doc comments ran past the two-sentence limit this package keeps. --------- Co-authored-by: wojtasjan <wojtasjan@google.com> Co-authored-by: Jan Wojtas <56300703+wojtas000@users.noreply.github.com>
google#1395) * fix(model/openaimodel): stop replaying prior-turn reasoning to the API * fix(model/openaimodel): close the review gaps in the reasoning drop Review of google#1395 found three shapes the drop got wrong and one sentence of package doc it made untrue. A thought carrying reasoning text on top of an image or code matched no arm at all: the text was suppressed, the call and response arms did not apply, and the rejection arm was gated on part.Text == "", which was false. The media left the request with no error, against the promise the same change had just made. The arm now keys on whether the part contributed text rather than on whether it had any, so suppressing the text cannot suppress the rejection with it. replayedReasoning listed the fields that disqualified a part, which left ToolCall, ToolResponse and AudioTranscription out and dropped them silently where main reported an unsupported part. The test is inverted: clear the reasoning, its signature, and the metadata that only qualifies media held in another field, and a part is droppable when what remains is the zero value. A field added by a later genai release is then rejected by default instead of vanishing, and a field walk over genai.Part in the tests keeps that honest. An unsupported role stopped being reported when a turn's only text was a thought: nothing buffered, so flushText returned before normalizeRole ran. The role is now checked wherever text is dropped, so the error is raised identically with the marker on or off. The doc paragraph claimed reasoning is never sent back on a later turn, unqualified. In a multi-agent run the shared flow rebuilds a peer's event as plain user text with the thought marker unset, so a peer's reasoning still arrives as ordinary content; the paragraph now says so and says it is not this package's to fix. The unbounded run loop the review also raised is closed on main by google#1290, which bounds consecutive thought-only turns; driving runner.Run against a stub that only ever reasons now ends after 10 calls with the request body flat at 382 bytes, where before this change it grew until the provider refused it. * fix(model/openaimodel): reject unsendable payload whatever else the part carries Review of the previous commit found the rejection still keyed on what the part contributed, which only moved the hole. A thought carrying text on top of an image was reported, but the same image riding on ordinary text, or on a function call, still matched a sendable arm first and left the request with no error at all. That predates this branch, and it is what made the promise the branch had just written down untrue. The check is now made once per part, before anything is emitted, and asks the opposite question: not "which fields disqualify this part" but "is anything set that convertContents does not account for". Text, the thought marker and its signature, a call, a response, and the three metadata fields that only qualify media carried elsewhere are cleared from a copy; whatever is left over is named in the error. So an image reaches the caller as "unsupported content part: InlineData" no matter what it travels with, and a field added to genai.Part by a later release is reported rather than silently dropped, because it is unaccounted for by construction. replayedReasoning now shares that test instead of restating it, and the arm that used to serve both jobs is left with the one it is for: a part that contributed nothing and is not reasoning worth dropping now says it carries nothing to send, rather than borrowing the unsupported-part wording. On the tests: describeInput rendered an assistant input message and an assistant output message identically, so the table could not have seen a replayed assistant turn go out as "input_text" — the shape the Responses API rejects. They now render as in/ and out/. The genai.Part field walk is stated against the accounted-for list and fails loudly on an unexported field instead of panicking, and it runs against unsupportedPayload as well as the drop. Two tests drive the public GenerateContent surface, blocking and streaming, asserting on the bytes that leave the process: the reasoning is absent and the answer is not, and a request the drop empties never reaches the model. Each of the seven guards was reverted individually and the suite re-run; every one is caught by a named test. * test(model/openaimodel): prove the rejection reaches the caller, not just the predicate Second review round. The field walk asserted what unsupportedPayload returns, which says nothing about whether convertContents passes it on: a mutation that swallowed the error for one field would leave the walk green. Each generated case now also builds the request and requires the field's name in the error, so the unit and the caller are held to the same answer. An empty part had no case either. buildContentsDefault filters those out upstream, so it only reaches convertContents from a caller assembling contents itself, which is exactly why the contract is worth pinning: it is reported, not quietly skipped. The "carries nothing to send" error is built with errors.New rather than a verbless fmt.Errorf, matching errors.go. * fix(model/openaimodel): key the empty-request wrap and the role check on the drop Review found the two of them gated on the wrong thing. The wrap keyed on the request having contents, so a Content carrying no parts at all reported "every part was dropped as replayed reasoning or blank text" when nothing had been dropped, and turned the bare sentinel into a wrapped one — err == ErrNoContents flipped from true to false for a request that behaves exactly as it did before this branch. convertContents now reports whether it dropped anything and the wrap follows that, so the bare sentinel survives for a request that arrived empty, blank text included, and the message no longer has to hedge about which of the two emptied it. The role check keyed on the part having text, so it ran for a thought carrying reasoning and not for one carrying only the marker or a bare signature — which main rejected as an unsupported part, and this branch had made silent. It now runs wherever a part is dropped, so an unsupported role is reported on any of the three. Also pinned, because neither the drop nor the pairing was ever the subject of a test here: text riding on a function response that pairs with nothing, and on a call with no name. Both used to be discarded because the text arm matched first, and are reported now that a part's fields are read independently. * fix(model/openaimodel): let a blank thought keep the bare ErrNoContents Review found the empty-request wrap firing for a part whose reasoning text was only whitespace. The drop is real there, but the text would have been skipped by newMessage anyway, so the drop is not what emptied the request — and wrapping turned err == ErrNoContents from true to false for a shape that behaves exactly as it does on main. The flag is now set only for reasoning that carried text the model would otherwise have seen, which also gives the bare sentinel back to a bare thought and a lone signature. The rejection for a part that reaches nothing keeps the unsupported-content-part prefix the single message used before this branch, so a caller string-matching on that phrase still matches the shapes that now land there — an empty part, and one carrying only VideoMetadata, MediaResolution or PartMetadata. The comment above the payload check claimed a part is read field by field. Text is, against a call or a response, but a call and a response on the same part are still arms of one switch and the response is dropped, as on main. The comment now says what the code does rather than what the phrase implies. Two test gaps the review named: the identity table gains the blank-reasoning case the claim turns on, plus the bare thought and lone signature, and the genai.Part field walk now puts each unsendable field on text, on reasoning text and on a function call rather than only on a part of its own, since the riding shape is the one this change exists to fix. * test(model/openaimodel): pin the drop flag across parts, and correct the doc Review round found the flag's accumulation untested: writing it as an assignment rather than a conditional set passed the whole suite, so a blank thought following a real one could have talked the emptied-request report back down. Two cases pin it, within a turn and across turns, and one more pins the blank-skip the trim rule leans on — that a blank text item is dropped beside a real one rather than only when it is alone. The package doc claimed a single exception to reasoning never being sent back. There are three: a peer agent's reply folded into context, a compacted transcript, and a memory recall all render reasoning as ordinary text upstream of any model package. It now says that rather than naming one. * test(model/openaimodel): cover the user-turn blank skip and the flag across turns Second review round. The across-turns case put the real thought in the last turn, so a flag reset per content block would have passed it; the real turn now comes first, which is the order that catches it. The blank skip was pinned only on the assistant path through newOutputMessage, leaving the user path through newMessage untested both beside real text and on its own. The field walk asserted the error contains the field name, which a shorter name that prefixes a longer one would satisfy; it now asserts the suffix. Three doc comments ran past the two-sentence limit this package keeps. --------- Co-authored-by: wojtasjan <wojtasjan@google.com> Co-authored-by: Jan Wojtas <56300703+wojtas000@users.noreply.github.com>
3 participants
Problem
model/openaimodelreports model reasoning to the caller as thought parts andthen sends those parts straight back to the API on the next turn as ordinary
assistant text.
convertContentsmatched every part onpart.Text != ""withno guard on
part.Thought, and nothing upstream strips thought parts. Thereasoning accumulated into the same
textPartsbuffer as the real answer andflushed with it into a single assistant message, so nothing separated the
model's private scratchpad from what it had actually said, and the following
turn was answered against it. A turn that produced only reasoning replayed as an
assistant message the model never sent.
Solution
Drop replayed reasoning rather than send it. The Responses API takes it back
only as an input item referencing the id of the item that produced it, and ADK
carries no such ids, so there is no correct way to replay it; adk-python's
Responses backend drops it for the same reason. Text and call/response are now
read independently rather than as arms of one switch, because a single part can
carry both — choosing between them either put the reasoning on the wire or
dropped the call and stranded its response in
callTracker. A thought-markedcall or response therefore survives while its reasoning text does not.
Two adjacent holes close with it. A thought carrying only a signature used to
reach the
defaultarm and fail the whole conversion, and is now dropped, sincea Responses request has nowhere to put one either way; and a part marked as a
thought that carries media or code is still rejected as an unsupported part
rather than vanishing from the request unannounced. A request left empty by the
drop now reports that, instead of
ErrNoContentsas though the caller had sentnothing. No exported API changes — the only new declaration is the unexported
replayedReasoning.Update after review.
Rebased onto
main; the conflict wasnewMessage's signature, which #1427 changed under this branch.A part carrying an unsendable field — media, code, server-side tool traffic — is now rejected wherever it appears, because the previous check keyed on what the part contributed: reasoning text riding on an image matched no arm at all and the image left the request unannounced, and so did the same image riding on ordinary text or on a function call.
unsupportedPayloadruns once per part before anything is emitted and asks the opposite question — is anything set thatconvertContentsdoes not account for — so the field is named in the error,unsupported content part: InlineData, whatever it travels with.replayedReasoningis stated the same way rather than as a list of the fields that disqualify a part, which had omittedToolCall,ToolResponseandAudioTranscriptionand dropped them silently; a field genai adds later is now reported rather than vanishing, and a reflection walk overgenai.Partholds both predicates to it.An unsupported role is reported again on a turn whose only text was a thought — nothing buffers, so
flushTextreturned beforenormalizeRoleever ran.A part that contributed nothing now says it carries nothing to send, rather than borrowing the unsupported-part wording it no longer fits.
The package doc no longer claims without qualification that reasoning is never sent back on a later turn: in a multi-agent run the shared flow rebuilds a peer's event as plain user text with the marker unset, so a peer's reasoning still arrives as ordinary content, which is not this package's to fix.
The unbounded run loop raised in review is closed on
mainby #1290 and needs nothing here — drivingrunner.Runagainst a stub that only ever reasons ends after 10 calls with the request body flat at 382 bytes, where before the drop it grew until the provider refused it.A second round found three more, all of them mine rather than the original commit's.
The empty-request wrap keyed on the request having contents rather than on a part having been dropped, so a
Contentcarrying no parts reported that every part was dropped when none had been, and turned the bareErrNoContentsinto a wrapped one —err == ErrNoContentsflipped true to false for a request that behaves exactly as it did before this branch.convertContentsnow reports whether it dropped anything and the wrap follows that, so a request that arrived empty, blank text included, still returns the bare sentinel.The restored role check keyed on the part having text, so it ran for a thought carrying reasoning and not for one carrying only the marker or a bare signature — which
mainrejected as an unsupported part and this branch had made silent. It now runs wherever a part is dropped.Pairing is enforced where it previously was not, which is the widest behavior change here and belongs in the note below rather than buried: text riding on a function response that pairs with nothing, or on a call with no name, used to be discarded because the text arm matched first and the call or response was never read.
A third round, from the review on
baff7e4b, closed the two claims the description overstated and the four smaller notes.err == ErrNoContentsnow survives a blank thought.{Text: " ", Thought: true}was being wrapped: the drop is real, but the text would have been skipped downstream anyway, so the drop is not what emptied the request. The flag is set only for reasoning carrying text the model would otherwise have seen, which also gives the bare sentinel back to a bare thought and a lone signature.The rejection for a part that reaches nothing keeps the
unsupported content partprefix the single message used before this branch, so a caller string-matching that phrase still matches the shapes that now land there — an empty part, and one carrying onlyVideoMetadata,MediaResolutionorPartMetadata.The comment claiming a part is read field by field now says what the code does: text is read independently of a call or a response, but a call and a response on the same part remain arms of one switch and the response is dropped, exactly as on
main.The package doc claimed a single exception to reasoning never being replayed. There are three — a peer agent's reply folded into context, a compacted transcript, and a memory recall all render reasoning as ordinary text upstream of any model package — so it now says that instead of naming one.
Behavioral changes. A 16-shape corpus was run through the public API against
mainand this branch and diffed outcome-by-outcome; 11 shapes differ. Nothing that previously reached the OpenAI API stops reaching it, and no shape that succeeded on the wire now fails.main{Text:"scratch", Thought:true}aloneErrNoContents, wrapped, naming the dropFileData,ToolCall,AudioTranscription,CodeExecutionResult,InlineData,ExecutableCode,ToolResponse) riding on text or a call{Thought:true}, loneThoughtSignatureunsupported content partErrNoContentsVideoMetadata/MediaResolution/PartMetadataunsupported content part *genai.Partunsupported content part: carries nothing to sendVideoMetadatariding on texterrors.Is(err, ErrNoContents)holds throughout.err == ErrNoContentsholds for every request that was empty on arrival; it is false only where the drop suppressed text that would otherwise have been sent, a shape that returned no error at all onmain. No exported identifier changes.Known gaps, deliberately not fixed here.
main, nothing in the tree builds that part; the comment no longer implies otherwise.mainhad none either. AnErrUnsupportedContentPartsentinel would be an exported-API addition and wants a maintainer's call first.ConvertForeignEvent(internal/llminternal) rebuilds a peer's reasoning as plain user text, so it still reaches every model package. Out of scope here and worth its own issue, as the review notes.flattenContentText(the system-instruction path) has no thought filter. No in-repo caller can put a marked thought part there; noted as defense-in-depth rather than a live leak.Testing Plan.
go build -mod=readonly workgo test -race -mod=readonly -count=1 -shuffle=on workgolangci-lint runandgo mod tidy -diff— clean in both modulesnewMessageand fromnewOutputMessage.adk-openai-e2e), which is the only evidence that exists for this package — it ships no httprr cassettes, so its unit tests assert serialization against expectation and nothing in CI proves OpenAI accepts the payload:Scenario 16 is held out: it is written against the unmerged #1392 and does not compile against
maineither. The same suite onmain(a6114727) is likewise 29/0, so this branch is at parity.Review loop. Three rounds of a six-lens panel (security, api-compat, correctness, testing, idioms, parity). Round 1: one blocking correctness finding (an unsendable field riding on text or a call escaped the rejection) and two blocking testing gaps. Round 2: four testing gaps and three over-length comments; one blocking correctness claim — that a dropped
ThoughtSignatureshould wrap the sentinel — was disputed and adjudicated in favour of the current rule by an independent pass, on the grounds that a signature has nowhere to go in a Responses request under any circumstance. Round 3: clean.