fix: echo empty reasoning_content on tool-bearing DeepSeek requests - #8
Merged
Merged
Conversation
DeepSeek's thinking mode requires the reasoning_content key on every replayed assistant turn once the request carries tools. A missing key is a 400 -- "The reasoning_content in the thinking mode must be passed back to the API" -- and it recurs on every later request in the loop, not just the offending turn, so one bad turn kills the session at an arbitrary iteration. The provider elides reasoning for some turns (a live-provider behaviour the e2e probes already note), so such a turn arrives with nothing to replay and oaMessage's omitempty dropped the key outright. DeepSeek's docs and sample code pass the key back unconditionally, empty value included. oaMessage.ReasoningContent becomes *string so "present and empty" is expressible, and a new Quirks.EchoReasoningWithTools flag (enabled for deepseek) makes tool-bearing requests echo the key on every assistant message. Without tools the key stays absent: DeepSeek ignores it there, and providers that do not know the field never see it. Tests, RED first: empty echo with tools, key absent without tools, and no echo for providers without the quirk. gofmt and vet clean, golangci-lint reports 0 issues, go test -race -count=1 green, buildOpenAIRequest at 100% statement coverage. Docs updated in the same commit: README provider notes and Message field, AGENTS.md probe guidance, Message doc comment.
…und 1) Adversarial review of the first commit found three things worth fixing: the flag that actually reaches production was pinned by no test (deleting it from the registry kept the whole suite green), the docs claimed DeepSeek AND GLM coverage while zai carries no such flag, and the echo's blast radius was described more narrowly than it behaves. - provider_test.go: pin EchoReasoningWithTools on deepseek and assert it stays off for the other five providers. Verified by experiment: with the flag removed from the registry the suite now FAILS on this assertion, where before the change it passed regardless. Every other test hand-builds its own ProviderConfig, so only this check can see the registry at all. - openai_test.go: TestBuildOpenAIRequest_EchoFollowsToolsNotThinking pins the echo as a function of the tools, not the thinking setting, on every assistant turn (enabled / disabled / unset / high), making the wider blast radius a decision rather than a side effect. - Docs: scope the requirement to DeepSeek everywhere (openai.go, message.go, README). README no longer implies GLM coverage, states that a custom provider id inherits no quirks, and drops wire framing from the ChatResult field comment. AGENTS.md records that the flag is off for every provider except deepseek and that empty-value acceptance is measured, not asserted. - responses.go: comment the builder asymmetry - the /responses path replays reasoning as an encrypted item, so this quirk has nothing to act on there. - e2e_deepseek_echo_test.go: tag-gated probe that measures the one open premise against the live API (present-but-empty vs omitted) and reports which fix layer the answer implies, keeping the caller-side fallback documented. It never builds without the e2e tag, and reads a key only when invoked deliberately. gofmt clean, go vet clean (with and without the e2e tag), golangci-lint 0 issues, go test -race -count=1 green.
…round 2) Round-2 adversarial review found the flag's end-to-end path still untested, a misleading failure verdict in the new probe, and remaining scope wording that overstated what the code does. - openai_test.go: TestChatClient_DeepSeekRegistryEchoReachesWire drives the built-in deepseek registry entry (FromEnv) against an httptest server and asserts the empty echo reaches the outbound body. Every other wire test hand-builds its ProviderConfig, so registry and serializer were only ever tested apart. Verified by experiment: removing the registry flag now fails this test AND the registry pin, where before only the latter existed. - openai_test.go: TestWithProvider_QuirksReplaceDropsEcho pins the replacement semantics of WithQuirks observably - re-registering the built-in id with explicit quirks drops the echo. The behaviour is a documented decision; making WithQuirks merge must fail here first. - openai_test.go: drop the stale "DeepSeek/GLM" bundling from a failure message (last instance of the round-1 overstatement). - e2e_deepseek_echo_test.go: classify outcomes instead of treating every error as a contract rejection. Only a 400 naming reasoning_content counts as REJECTED; transport, timeout, rate-limit and auth failures report INCONCLUSIVE so a provider hiccup can never be dressed up as evidence about the replay contract. The probe also sweeps thinking enabled / disabled / unset, since the widest new wire state is the non-thinking one that an odek tool loop actually produces. - Wording: the echo applies to every assistant turn of a chat-completions request that carries tools (including turns that made no tool call), and a request diverted to /responses never reaches that builder - corrected in openai.go, AGENTS.md and README. gofmt clean, go vet clean with and without the e2e tag, golangci-lint 0 issues, go test -race -count=1 green.
Live evidence from a failing odek session relocated the enforcement trigger. The transcript (282 messages, 143 assistant turns) shows DeepSeek eliding reasoning_content on 36 of 143 turns - 17 in the first half, 19 in the second, newest three at 9451/7448/11318 characters. That is interleaved, so it is provider elision, not odek's stripOldReasoning trim (which would zero every turn except the newest keepRecentReasoning=2). 25 of those elided turns carry tool calls, each followed by a tool result: the request is a tool-loop continuation, and that is the shape that 400s. The previous probe ended its history with a USER message - a terminal request - and measured both the echoed and the omitted key as accepted. That was a false negative: it never exercised the shape odek actually sends. probeMessages now takes a continuation flag and the test measures both shapes (continuation and terminal) x (echoed, omitted), so the two verdicts can be compared directly and the shape that enforces the contract is named in the output. Thinking stays enabled throughout: the enforcement question is about continuation, not about the thinking preset. Still tag-gated (never builds without e2e), still asserts nothing, and still classifies non-contract errors as INCONCLUSIVE rather than as rejections. gofmt clean, go vet -tags e2e clean, unit suite green.
Contributor
Author
Live verdict — the empty echo is the correct layer (controlled experiment)The tag-gated probe settles the one open premise. Four live calls, thinking enabled, tools advertised, identical history — only the shape and the echo vary:
What this proves
This also closes the round-1 hypothesis that omission on older turns must have been tolerated (inferred from the session reaching iteration ~16). It was not tolerated; that session simply had not yet hit a continuation whose history carried an elided turn — and once it did, the 400 recurred on every later request, exactly as reported. Still true, unchanged
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DeepSeek thinking mode returns HTTP 400 on tool loops:
It appears at an arbitrary iteration and then recurs on every later request, so one bad turn kills the rest of the session.
Root cause
DeepSeek requires
reasoning_contentto be replayed on every assistant turn of a request that carriestools— including turns where the provider itself returned no reasoning, which it does on some turns. The assistant message was taggedjson:"reasoning_content,omitempty", so such a turn serialized with no key at all.Enforcement is shape-dependent: a request whose history continues a tool loop (ends with a tool result) is rejected when the key is missing, while a terminal request is accepted. That is why the failure surfaces at an arbitrary iteration rather than a fixed one, and why it then persists.
Fix
oaMessage.ReasoningContent→*string, so "key absent" and "key present and empty" are both expressible on the wire.Quirks.EchoReasoningWithTools, enabled fordeepseek— an explicit provider flag, never URL sniffing.Blast radius
zaicarries no flag: no z.ai documentation confirms the same requirement.WithQuirksreplaces the struct rather than merging into it — both documented.Verification
gofmt,go vet(with and without thee2etag),golangci-lint,go test -race -count=1, and CI are all green.Notes
README.mdandAGENTS.mdare updated in the same commits, along with theopenai.goandresponses.godoc comments.