Repository navigation
[WRONG BRANCH] fix(responses): refuse a combo failover that cannot replay mandatory reasoning (#4696) - #4769
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
리뷰 · 우선순위 76 / 80이 PR은 콤보 페일오버가 공식 DeepSeek로 넘어갈 때 나오던 고치는 방식은 추측해서 plaintext를 만들어 넣지 않고 fail-closed입니다. plaintext reasoning 리플레이가 필수인 타깃은, 라우트 변경으로 그 reasoning이 이미 사라졌다고 증명되면 후보에서 빠집니다. 페일오버는 다음 타깃으로 가고, 후보가 다 떨어지면 코드로 보면 세 층입니다. (1) 테스트는 추가만 했습니다. 현재 라인 ~41+ (passthrough.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
…4726) [skip ci] Kimi's Code Plan Responses endpoint requires a tool result to follow its call immediately. When the desktop LSP hook injects a developer message between a code-mode exec call and its output, Kimi rejects the whole request with HTTP 400 naming the unanswered tool_call_id. Because the row replays full history every turn, the session then fails permanently rather than once. opencodex already implements exactly this repair in normalizeResponsesToolResultAdjacency, but passthrough gates it on requiresAdjacentResponsesToolResults, which only DeepSeek's entry seeded. Kimi inherited no normalization, so a user who configures Kimi onto the Responses wire hits the 400 on every affected turn. Seed the flag on both kimi and kimi-code. The existing fill-only derivation in providerConfigSeed, enrichProviderFromRegistry and routedProviderConfig carries it into new and already-persisted rows without overriding an explicit user value, and the flag is inert while these presets use the Chat wire. The repair reorders; it does not delete. The intervening developer message is preserved and moves after the batch, so the fix cannot be mistaken for silencing the 400 by dropping hook context. Coverage pins that, plus call_id pairing with two outstanding calls and interleaved noise, and an already-adjacent input being left untouched. No upstream specification documents the requirement; the evidence is the reported 400 and DeepSeek's identical failure shape under #1292. Upstream Codex deliberately leaves an intervening developer message where it is, so this stays a per-provider capability rather than a wire-wide default.
…reasoning (#4696) [skip ci] Official DeepSeek rejects a tool-bearing continuation whose prior reasoning is not replayed, with 400 invalid_request_error: the reasoning_text in the thinking mode must be passed back to the API. A combo failover reached that state. The reported shape is narrow and still reproduces. When the previous successful turn's reasoning exists only as opaque encrypted_content minted by a different provider, account or model, the replay identity no longer matches, so the sanitizer strips the foreign blob. Stripping is correct — that blob is not decodable by the new route — but it left a reasoning item carrying neither encrypted_content nor reasoning_text, and forwarded that hollow shell to a provider whose thinking mode requires the real thing. A history that already carries plaintext reasoning_text was never affected and is unchanged: preserveResponsesReasoningContent keeps it, and each combo target clones the original request body rather than reusing a previous target's converted one. Fail closed instead of guessing. A target that requires plaintext reasoning replay is ineligible for a history proven to have lost it across a route change; failover continues to the next target, and an exhausted combo returns 400 target_incompatible through the same core-errors factory convention as unreadable_encrypted_agent_task. Reasoning is never fabricated, and an opaque blob belonging to another provider or account is never forwarded. A target that cannot be routed at all stays eligible here, so an unrelated routing failure still surfaces as 503 combo_unavailable rather than being misreported as a reasoning incompatibility. requiresPlaintextReasoningReplay() names the provider contract in one place. It currently derives from preserveResponsesReasoningContent plus requiresAdjacentResponsesToolResults because official DeepSeek is the only entry setting both; that derivation is documented and should become an explicit capability as soon as a second provider needs it.
862eef4 to
1eccd0f
Compare
…) [skip ci] A custom provider whose baseUrl ends in a slash produced a doubled discovery path: https://gateway.example.com/v1//models. Gateways that route the doubled path as a distinct route reject it — the report observed HTTP 403 — so discovery failed and the catalog silently fell back to the configured models. The send paths were normalized already, by openaiChatCompletionsUrl and openaiResponsesUrl, but discovery was not. buildModelsRequest appended the endpoint verbatim, and resolveProviderModelDiscoveryUrl returns that default unchanged for a provider with no registry spec, which is exactly the custom case. Registry providers escaped it because new URL(spec.path, base) collapses the doubled slash. providerModelsUrl mirrors openaiChatCompletionsUrl rather than inventing a second policy: trim outer whitespace and trailing slashes, drop an already pasted /models, then append exactly one. Both production callers of the default discovery URL use it — catalog discovery and API-key validation. An existing path prefix is preserved, so /api/openai/v1 is not collapsed to the origin. Registry spec.path, absolute endpoint overrides and relative endpoint overrides all resolve exactly as before; a baseUrl already written without a trailing slash is byte-identical to its previous output.
The CodeBuddy route launches the vendor CLI with --tools "" and --strict-mcp-config, so the routed model has no native tool channel and writes its call as prose. The shared coding-agent projection forwards text_delta unrepaired, so that markup reached the client as an ordinary assistant answer. Qoder's guard does not match it. The leaked tags are wrapped in FULLWIDTH VERTICAL LINE (U+FF5C), which none of the shipped UNREPAIRABLE_MARKERS cover, so this needed a signature of its own rather than a port. Refusal requires the observed two-line grammar: a calls control line at column zero, outside a Markdown fence, immediately followed by an invoke line naming a functions.* tool. A lone tag, a quoted or inline-code literal, a fenced example, a blockquote, indented source, or prose discussing the markup all carry extra syntax before the tag and are forwarded untouched. Matching the marker alone would refuse a legitimate answer that merely explains this protocol, which is why the detector is narrower than the marker spelling. A detected leak preserves the answer text already proven safe, emits one non-retryable vendor_scaffold_detected error, and suppresses the vendor's later success terminal so the client never sees a completed turn. Markers split across streamed deltas are caught by holding only a bounded suffix that could still complete a control sequence or a fence; unrelated pending text is released at the next mismatch or terminal. The reasoning channel is guarded independently. Leaked prose is never promoted into a real tool call. The text channel carries no authenticated call envelope and no validated arguments, so converting it would manufacture execution authority out of model output. Kept CodeBuddy-owned rather than lifted into the shared coding-agent path, the same containment #4234 chose for Qoder: the contract observed here is this vendor's, and #4190's lane packet asked for a report rather than symmetry. Co-authored-by: Ingwannu <ingwannu@users.noreply.github.com>
…#4679) [skip ci] Command Code's gateway rejects a request outright with 400 name must be at most 64 characters, got 66. Codex Desktop built-in app tools flatten to <namespace>__<name> past that bound, a user cannot exclude them, and Responses-Lite catalogs bundle every declared tool, so the surface cannot be shrunk from configuration. The bound belongs to the adapter, not to the shared name helper. Three adapters already solve this for themselves: Kiro normalizes to its own charset with a deterministic 8-hex suffix, Google compiles and restores names in its wire compiler, and Meta Muse aliases names on api.meta.ai. The translated openai-chat path is the only one with no answer, and it is the path Command Code uses. A request-scoped registry now owns one collision domain per translated Chat Completions request, following Kiro's shape. A namespaced name whose flattened spelling exceeds 64 characters becomes a charset-safe alias derived purely from the native identity, so it is stable across processes, catalog order and catalog membership. Declarations, replayed assistant tool calls and tool_choice all pass through the same registry, and both the streaming and buffered parsers restore the echoed alias before tool_call_start, so the existing bridge map still hands the client its native {namespace, name}. The registry is seeded from the union of the current catalog and the structured tool calls still present in replay history, because a historical call can keep its namespace without being redeclared; seeding from the catalog alone would let exactly the reported over-limit name reach the gateway again on a later turn. Nothing else changes. Names at or under 64 characters and bare names are byte-identical on the wire, and Kiro, Google and Muse still receive the raw flattened name and run their own normalization. 64 is the Chat Completions function-name limit and a strict-gateway compatibility concern, not OpenAI Responses parity: upstream Codex raised its own MCP ceiling to 128 bytes in openai/codex#39594 because native Responses accepts 128. Applying it on this wire is correct for that wire alone. Carried from #4715. That PR placed the bound in the shared namespacedToolName helper and was provisionally accepted there. Hosted CI then showed twice that the shared point intercepts adapters which already had an answer: it broke Google's wire-compiler restore, and after that was narrowed it broke Kiro's normalizer. The problem statement and issue analysis are the original author's; only the placement changed. Co-authored-by: Hulian Buligon <205309211+HulianBuligon@users.noreply.github.com>
# Conflicts: # structure/transports/responses.md
…guard fix(codebuddy): refuse leaked vendor tool-call scaffolding (#4596)
…ames fix(openai-chat): bound flattened tool wire names for strict gateways (#4679)
fix(catalog): normalize the custom-provider model-discovery join (#4724)
fix(providers): give Kimi the Responses tool-result adjacency repair (#4726)
|
Cascading downward. A DeepSeek failover whose history carries only another route's opaque reasoning is refused as target-incompatible instead of dispatched with an empty placeholder that the provider rejects anyway. Evidence at the verified tip 49f815d (tree
Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate into |
53455e0
into
codex/pw1-vision-sidecar-truncation
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
…easoning-replay fix(responses): refuse a combo failover that cannot replay mandatory reasoning (lidge-jun#4696)
1 participant
Summary
A combo failover to official DeepSeek returned
400 invalid_request_error: "Thereasoning_textin the thinking mode must be passed back to the API." The first target failed withupstream_server_error, the ladder moved to DeepSeek, and the replayed history was missing the reasoning DeepSeek requires.Reproduction status first, because the report was filed against 2.54.0: it partially reproduces on current
dev, and the surviving half is narrow.Not reproducing: a history that already carries plaintext
reasoning_text.preserveResponsesReasoningContentkeeps it, and combo failover never reuses a previous target's converted body —previous_response_idis expanded once before the ladder and each targetstructuredClones the same original request. Neither commit since 2.54.0 changes this (b42d573331is an ancestor of the reported build, and369be813c4only backfillssummary: []), so there is no basis to close this as already-fixed.Still reproducing: when the previous successful turn's reasoning exists only as opaque
encrypted_contentminted by a different provider, account or model. The replay identity (provider + destination + adapter + model + credential) no longer matches, so the sanitizer strips the foreign blob. Stripping is right — that blob is not decodable by the new route — but it left a reasoning item carrying neitherencrypted_contentnorreasoning_text, and forwarded that hollow shell to a provider whose thinking mode requires the real thing. That is exactly the shape of the reported 400.The fix fails closed rather than guessing. A target that requires plaintext reasoning replay is ineligible for a history proven to have lost it across a route change; failover continues to the next target; an exhausted combo returns
400 target_incompatible. Reasoning is never fabricated, and an opaque blob belonging to another provider or account is never forwarded — the serializer additionally strips any encrypted blob for such a provider regardless of provenance.Two deliberate details:
503 combo_unavailable.requiresPlaintextReasoningReplay()names the contract in one place. It derives frompreserveResponsesReasoningContentandrequiresAdjacentResponsesToolResultsbecause official DeepSeek is the only registry entry setting both. That derivation is incidental rather than meaningful, so it is documented as such and should become an explicit capability the moment a second provider needs it. (Verified against this stack: the sibling layer addingrequiresAdjacentResponsesToolResultstokimi/kimi-codedoes not trip it, because neither Kimi entry setspreserveResponsesReasoningContent.)The refusal uses the existing
core-errors.tsfactory convention rather than a second error shape — same form asunreadable_encrypted_agent_task, which is the same class of failure: a history the selected provider cannot consume. It fires only before a response has started, so it is a clean refusal and never needs a mid-stream terminal variant.Upstream Codex has no multi-provider inference failover at all, so this is not reinventing an upstream mechanism; its closest analogue is
comp_hash-guarded compaction, which fails closed on incompatible opaque state the same way.Closes #4696
Verification
Static source review only, plus hosted CI. No local test suite,
typecheck,test:changed, or build was run — the repository owner prohibits local suite execution in this lane after a past local run deleted real~/.opencodexdata.Static checks performed:
src/responses/reasoning-replay-cache.ts(identity includes credential),src/server/responses/core-replay.ts(_stripReasoningEncryptedContenton mismatch),src/adapters/openai-responses/reasoning.ts(strips the blob, never synthesizes plaintext).tests/providers/deepseek-reasoning-replay.test.tsand the body-cloning insrc/combos/request.ts/core-combo.ts.kimi(entries-core.ts:426) andkimi-code(entries-extended.ts:894) set only the adjacency flag;preserveResponsesReasoningContentappears on neither.git diff --numstatontests/shows additions only).git diff --checkclean.Regression coverage:
upstream_server_error failover to strict Responses preserves existing reasoning_texta foreign opaque-only replay skips the strict target without forwarding or fabricationan exhausted combo reports target_incompatible when mandatory plaintext is unavailablea route switch never forwards foreign opaque reasoning or invents plaintext503 combo_unavailable, not the new refusal.structure/transports/responses.mddocuments the new target-eligibility rule, since this changes observable routing behaviour.Hosted CI: this is a non-tip layer of a stacked lane and carries
[skip ci]under the maintainer-approved DEV-STACK-08 tip-only policy. The lane's CI gate runs on the tip branch, which contains this commit.Checklist
structure/transports/responses.md; no user-facing configuration changed)Update after the first hosted CI run
The first push of this layer failed the file-size ratchet:
tests/server/server-combo-failover-e2e.test.tsreached 4,322 lines against its committed cap of 4,166 (verdict: GREW).The cap was not raised — that guard exists precisely to stop this, and the tooling only ever lowers caps. The tests this layer adds now live in a new file,
tests/server/server-combo-reasoning-replay-eligibility.test.ts(300 lines), registered in bothscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. The original file is back to 4,156 lines with content byte-identical to its pre-change state, and the test-name multiset across the two files is unchanged at 101.The regression coverage listed above is unchanged in substance; it simply resides in the new file.