Skip to content

fix(model/openaimodel): stop replaying prior-turn reasoning to the API - #1395

Open
hanorik wants to merge 1 commit into
google:mainfrom
hanorik:fix/openaimodel-replayed-thoughts
Open

fix(model/openaimodel): stop replaying prior-turn reasoning to the API#1395
hanorik wants to merge 1 commit into
google:mainfrom
hanorik:fix/openaimodel-replayed-thoughts

Conversation

@hanorik

@hanorik hanorik commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Problem

model/openaimodel reports model reasoning to the caller as thought parts and
then sends those parts straight back to the API on the next turn as ordinary
assistant text. convertContents matched every part on part.Text != "" with
no guard on part.Thought, and nothing upstream strips thought parts. The
reasoning accumulated into the same textParts buffer as the real answer and
flushed 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-marked
call 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 default arm and fail the whole conversion, and is now dropped, since
a 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 ErrNoContents as though the caller had sent
nothing. No exported API changes — the only new declaration is the unexported
replayedReasoning.

@hanorik
hanorik requested review from a team and karolpiotrowicz August 23, 2026 22:32
@karolpiotrowicz

Copy link
Copy Markdown
Contributor

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. base_flow.go:120 deliberately does not end the run when the model's last turn is all thought parts — it calls the model again — and nothing bounds that path. MaxLLMCalls occurs exactly twice in the tree, as a field at agent/live.go:48 and an assignment at runtime.go:304, and is never read anywhere. Until now each retry replayed the previous turn's reasoning, so the request grew and a real endpoint eventually refused it. With request.go:128 dropping those parts, the request is byte-identical every iteration, so nothing ends the run at all.

Driving runner.Run against a stub that always answers with reasoning only, and rejects an over-budget body with context_length_exceeded the way the real API does:

main    0a51e15   model calls: 6
                  request body sizes: [196 487 592 697 802 907]
                  outcome: TERMINATED BY PROVIDER ERROR: 400 context_length_exceeded

branch  9dd6a6f   model calls: 40
                  request body sizes: [196 382 382 382 ... 382]
                  outcome: STOPPED BY TEST CAP - did not terminate on its own
the test those two runs come from — drop it in model/openaimodel and run go test -run TestBackstop -v ./model/openaimodel/
package openaimodel_test

import (
	"context"
	"io"
	"net/http"
	"net/http/httptest"
	"testing"
	"time"

	"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"
)

const reasoningOnly = `{
  "id":"resp_1","object":"response","created_at":1,"status":"incomplete","model":"test-model",
  "incomplete_details":{"reason":"max_output_tokens"},
  "output":[{"type":"reasoning","id":"rs_1","summary":[{"type":"summary_text","text":"still thinking about it"}]}],
  "usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2,"output_tokens_details":{"reasoning_tokens":1}}
}`

func TestBackstop(t *testing.T) {
	const budget = 900 // bytes of request body, standing in for a token budget

	var sizes []int
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		b, _ := io.ReadAll(r.Body)
		sizes = append(sizes, len(b))
		if len(b) > budget {
			w.WriteHeader(http.StatusBadRequest)
			io.WriteString(w, `{"error":{"message":"maximum context length exceeded","type":"invalid_request_error","code":"context_length_exceeded"}}`)
			return
		}
		w.Header().Set("Content-Type", "application/json")
		io.WriteString(w, reasoningOnly)
	}))
	defer srv.Close()

	ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
	defer cancel()

	m, _ := openaimodel.NewModel(ctx, "test-model", &openaimodel.ClientConfig{APIKey: "test", BaseURL: srv.URL})
	a, _ := llmagent.New(llmagent.Config{Name: "assistant", Model: m, Instruction: "be helpful"})
	r, _ := runner.New(runner.Config{AppName: "backstop", Agent: a,
		SessionService: session.InMemoryService(), AutoCreateSession: true})

	events, outcome := 0, ""
	for _, err := range r.Run(ctx, "u", "s", genai.NewContentFromText("q", genai.RoleUser), agent.RunConfig{}) {
		if err != nil {
			outcome = "TERMINATED BY PROVIDER ERROR: " + err.Error()
			break
		}
		if events++; events >= 40 {
			outcome = "STOPPED BY TEST CAP - did not terminate on its own"
			break
		}
	}
	t.Logf("model calls: %d", len(sizes))
	t.Logf("request body sizes: %v", sizes)
	t.Logf("outcome: %s", outcome)
}

The bound this removes was the bug's own symptom, so preserving it is not an option and the drop is not what is wrong here. It is worth saying plainly how narrow the trigger is, because it decides how urgent the cap is. The turn has to consist only of thought parts, and on the default configuration that cannot happen: openaimodel never sets Reasoning on the request, so a real gpt-5-nano call truncated by max_output_tokens returns one reasoning item with empty content and empty summary, response.go:107 returns ErrNoTextOrToolContent, and the run ends after a single call. The same call with "reasoning":{"summary":"auto"} returns two summary entries and no message item, which response.go:100 maps to all-thought parts and does spin. That is reachable today only by passing the option through the escape hatch at openai.go:70.

So the missing cap belongs in base_flow.go:120 rather than in this package, and I would land this first and track the cap separately rather than hold a confidentiality fix behind it.

A thought-marked part that also carries media or code vanishes without the error. The rejection arm at request.go:152 is gated on part.Text == "", so a part with reasoning text riding on an image matches no arm at all — the text is suppressed by request.go:128, the call and response arms do not apply, and part.Text == "" is false. The comment at request.go:188-189 says such a part goes on "to be converted, or rejected as unsupported, on their merits", which holds only while the part has no text, and request_test.go:304 covers just that variant. The media is dropped on main too, so nothing regresses — what is new is the promise to reject it.

{Thought: true, Text: "scratch", InlineData: &genai.Blob{MIMEType: "image/png", Data: []byte{1}}}
// no error, one input item emitted, the image gone. Same for FileData, ExecutableCode, CodeExecutionResult.

{Thought: true, InlineData: &genai.Blob{MIMEType: "image/png", Data: []byte{1}}}
// openai: unsupported content part *genai.Part   <- the only variant the table covers

replayedReasoning does not cover every genai.Part field, so three more shapes drop silently. request.go:190-192 names six fields and omits ToolCall, ToolResponse and AudioTranscription, along with VideoMetadata, MediaResolution and PartMetadata, which carry nothing sendable on their own. A part marked as a thought carrying any of the first three returns true from the predicate and disappears, where main returned openai: unsupported content part. Nothing in the repo populates those fields today, so this is about the next genai bump rather than about now. Inverting the polarity, so the predicate is true only when text and signature are the only things set, would make a newly added field fail loudly by default instead of quietly.

An invalid role stops being reported when a turn's only text is a thought. Nothing buffers, so flushText returns before normalizeRole ever runs.

{Role: "assistant", Parts: []*genai.Part{{Text: "x"}}}
// main and this branch: openai: unsupported role "assistant"

{Role: "assistant", Parts: []*genai.Part{{Text: "x", Thought: true}}}
// main:        openai: unsupported role "assistant"
// this branch: nil

The package doc promises more than the package can deliver. doc.go:24-29 says reasoning "is not sent back on a later turn", unqualified. In a multi-agent run a peer's event goes through ConvertForeignEvent at contents_processor.go:108, which rebuilds each text part as a brand-new &genai.Part{Text: fmt.Sprintf("[%s] said: %s", ...)} at contents_processor.go:580-582 with Thought left at its zero value, so the guard passes it straight through as user text. That rebuild is shared flow code and out of scope here — one qualifying clause in the paragraph would keep the doc true in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants