Repository navigation
[WRONG BRANCH] fix(providers): give Kimi the Responses tool-result adjacency repair (#4726) - #4770
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. |
리뷰 · 우선순위 77 / 80이 PR은 Kimi를 Responses 와이어에 올렸을 때 나는 고질병을 고친다. Codex Desktop의 LSP 훅이 opencodex 쪽에는 이미 같은 모양을 고치는 수리기가 있다. 이 PR이 하는 일은 단순하다. 테스트 스택은 라인 - 없음(치명 버그). 아래는 운영·스택 관찰이다. 헤드 커밋 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 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.
68b5c73 to
70129ac
Compare
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>
…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)
|
Cascading downward. Kimi declares the adjacency capability so a hook-injected developer message no longer separates a tool call from its result. The middle message is preserved and the result is repositioned, rather than dropped to silence the 400. 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 |
aa31ed4
into
codex/pw2-deepseek-reasoning-replay
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
…adjacency fix(providers): give Kimi the Responses tool-result adjacency repair (lidge-jun#4726)
1 participant
Summary
Kimi's Code Plan Responses endpoint (
https://api.kimi.com/coding/v1) requires a tool result to follow its call immediately. When Codex Desktop's LSP hook injects a developer message between a code-modeexeccall and its output, Kimi rejects the whole request withHTTP 400, naming thetool_call_idthat "did not have response messages". Because the affected row replays its full history every turn, the session then fails on every subsequent turn rather than once.opencodex already implements exactly this repair —
normalizeResponsesToolResultAdjacency()insrc/adapters/openai-responses/tool-output-recovery.ts— butpassthrough.tsgates it onprovider.requiresAdjacentResponsesToolResults === true, and only DeepSeek's registry entry seeded that flag. Kimi inherited no normalization, so a user who configures Kimi onto the Responses wire gets a permanently broken session.This seeds the flag on both
kimi(entries-core.ts) andkimi-code(entries-extended.ts). The existing fill-only derivation inproviderConfigSeed,enrichProviderFromRegistryandroutedProviderConfigcarries 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. That distinction is the point of the change. The intervening developer message is preserved and moves after the batch, so this cannot be mistaken for silencing the 400 by discarding hook context. I read
normalizeResponsesToolResultAdjacencyline by line to confirm it before relying on it: it keeps every intervening non-tool item in original relative order, preserves function/custom tool-type pairing, and deliberately leaves duplicate, missing, or backwards pairs for the upstream to reject rather than guessing.Two things the issue asserted that current source does not support, checked rather than assumed:
statelessResponses; that belongs to the reporter's own configuration. The defect and the fix are unaffected.tool_callsmust be exactly the matchingrole=toolmessages, which is consistent with the observed behaviour.Closes #4726
Verification
Static source review only, plus hosted CI. No local suite, typecheck, 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/providers/derive.ts(providerConfigSeed, the registry enrichment merge, and the stale-row backfill),src/router.ts, then the gate insrc/adapters/openai-responses/passthrough.ts.normalizeResponsesToolResultAdjacencyin full to confirm it preserves rather than drops intervening items, and that it refuses ambiguous duplicate or reversed pairs instead of reordering them.scripts/test-layout/layout.json(explicit) andtests/fixtures/test-layout-expected.json, whichtests/test-layout.test.tsandtests/test-layout-tooling.test.tsenforce.git diff --checkclean.Regression coverage in
tests/providers/kimi-responses-adjacency.test.ts, mirroring the assertion style oftests/providers/deepseek-inbound-wire.test.ts:both Kimi registry entries seed the adjacency capabilitya stale persisted Kimi row is backfilled and activates adjacency repair on replaymoves a result next to its call while preserving an intervening developer messagekeeps call_id pairing and all interleaved history with two outstanding replayed callsleaves an already-adjacent call and result input untouchedstructure/providers/chat-compat.mdis updated: the adjacency pass is documented as gated by the capability rather than by provider name, with the Kimi evidence and the reason it is not a wire-wide default.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/providers/chat-compat.md; no user-facing configuration changed — the capability is seeded, not user-set)