[agent] fix: normalize inbound Chat images before route selection and preserve an explicit reasoning disable - #4534
Conversation
… preserve an explicit reasoning disable
The native Chat fast path recognized only `image_url` parts, while the translated
path also understood Pi/MCP `{type:"image", data, mimeType}` and Anthropic-shaped
`{type:"image", source}` parts. Two failures followed from that single gap: a
text-only routed model kept an image-bearing body because `isNativeChatRouteEligible`
could not see the image, and the native whitelist passthrough forwarded the foreign
part verbatim to an OpenAI-compatible upstream that does not accept it.
Recognition now lives once in `src/chat/image-parts.ts`, and
`normalizeChatImageParts` runs before `routeModel` so the diversion decision and the
forwarded wire observe the same parts. A body with no foreign image part is returned
by reference and stays byte-identical. A remote reference is recognized and
rewritten, never fetched.
Separately, the Chat inbound effort allowlist dropped `none`. That is the runtime's
disable sentinel, not an unknown value: `src/reasoning-effort.ts` maps it to omitting
the wire parameter and the Pi export maps Pi's `off` level onto it. Dropping it let a
provider default re-enable thinking the caller had turned off, which is not neutral
for Anthropic families that think by default.
Audit findings F1 and F7 (2026-09-14).
Local verification NOT RUN BY USER INSTRUCTION.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe pull request adds provider-parity planning documents and implements shared Chat image recognition and normalization. It also accepts ChangesProvider parity planning
Chat ingress normalization
Reasoning disablement
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
…omain Hosted CI caught this: tests/test-layout-tooling.test.ts reported "chat-inbound-reasoning-none.test.ts: seed responses != server" for both new files. scripts/test-layout/layout.json seeds a `chat-` prefix to the `responses` domain, which is where the sibling Chat-translation tests already live, so registering them under `server` contradicted the seed. Moves both files to tests/responses/ and registers them there. Import depth is unchanged, so no import edits were needed. Local verification NOT RUN BY USER INSTRUCTION.
리뷰 · 우선순위 74 / 80이 PR은 2026-09-14 프로바이더/PI 호환 감사의 1층(F1·F7) 입니다. 지금 첫째, 운영자가 텍스트만 받는 모델로 라우팅해도, 본문에 Pi/Anthropic 모양 이미지가 있으면 같은 PR의 F7은 Chat inbound effort 허용 목록에 스택으로 보면 이 PR이 바닥입니다. base는 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
… variant Independent review found this comment now states a false fact about this stack's own change. It said the chat ingress allowlist OUTPUT_CONFIG_EFFORTS "has no none", which stopped being true in 63fbe66 when F7 added the disable sentinel to that allowlist. The filter itself is kept, narrowly and on a stated basis: emitting the variant would change what this exporter writes into a user's opencode config, and whether opencode's picker round-trips reasoningEffort "none" back to a wire this proxy reads has not been verified. Re-enabling it is a scoped follow-up needing that check, not a side effect of an ingress fix. MCode and ZCode filter none for their own separate reasons, which remain accurate at their call sites. Comment-only; no behavior change. Local verification NOT RUN BY USER INSTRUCTION.
|
Maintainer integration per MAINTAINERS.md: landing this maintainer-authored PR on dev without a second maintainer approval.\n\nExact-head verification on c7e863b: 28 checks pass, 2 skipped (macos control, windows matrix placeholder), 0 failed/cancelled. Run set: 34790779603 plus metadata workflows (enforce-target, label, resolve-pr, hygiene) — all green on this head.\n\nLocal suite/typecheck/build: NOT RUN (hosted exact-head CI is the evidence). |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7e863b9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const message of messages) { | ||
| if (!isRec(message) || !Array.isArray(message.content)) continue; | ||
| for (const part of message.content) { | ||
| if (isRec(part) && chatImageUrlFromPart(part) !== null) return true; |
There was a problem hiding this comment.
When a text-only route receives a usable text part alongside {type:"image_url", image_url:""}, this predicate now returns false because the URL is empty. The previous predicate matched the image_url type itself and diverted the request to translation, where the unusable image was discarded and the text could proceed; normalization now leaves the malformed part unchanged and native passthrough forwards it, which can turn that previously accepted text turn into an upstream 400 on providers that validate content parts. Preserve type-based diversion for malformed image parts, or reject/strip them before native eligibility is evaluated.
Useful? React with 👍 / 👎.
| @@ -247,3 +247,33 @@ Translated Chat request construction uses the [inline-image budget](../transport | |||
| The [explicit model-capability contract](../config.md#explicit-per-model-capability-declarations) preserves operator declarations through provider storage and catalog capture; it does not infer upstream capability or change this surface's routing behavior. | |||
|
|
|||
| Provider-scoped approval reviewer settings are projected by the [catalog owner](../catalog.md#provider-scoped-approval-reviewer); this surface retains its existing routing, transport and account-selection behavior. | |||
There was a problem hiding this comment.
This is the only structure document updated even though the commit changes contracts in both src/chat/ and src/server/; structure/INDEX.md maps src/chat/ to runtime.md and transports/inventory.md as well, and maps src/server/ to several additional documents. Update every listed owner with the new contract, or narrow the manifest ownership if those documents are not actually owners.
AGENTS.md reference: structure/AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
| // Pi's "off" thinking level onto it (src/clients/config-export.ts). Dropping it here let a | ||
| // provider default re-enable thinking the caller had explicitly turned off — and for the | ||
| // Anthropic families that think by default, omission is not the same as disabled. | ||
| const OUTPUT_CONFIG_EFFORTS = new Set(["none", "minimal", "low", "medium", "high", "xhigh", "max", "ultra"]); |
There was a problem hiding this comment.
Accepting reasoning_effort: "none" changes the public Chat Completions contract, including causing supported default-on Anthropic models to receive an explicit thinking disable, but the public Chat section in docs-site/src/content/docs/reference/proxy-formats.md still only says that reasoning effort is translated and does not identify none or its disable semantics. Add this behavior to the English reference and keep the translated locales consistent.
AGENTS.md reference: AGENTS.md:L380-L381
Useful? React with 👍 / 👎.
1 participant
Summary
The native Chat fast path recognized only
image_urlcontent parts, while the translated path also understood Pi/MCP{type:"image", data, mimeType}parts (Asideread_filetool results) and Anthropic-shaped{type:"image", source}parts. Two user-visible failures followed from that one gap:isNativeChatRouteEligiblecould not see the image and so never diverted the request to the Responses pipeline that describes or strips it.Recognition now lives once in
src/chat/image-parts.ts, andnormalizeChatImagePartsruns inhandleChatCompletionsWithBudgetimmediately after routing-body validation and beforerouteModel, so the diversion decision and the forwarded wire observe the same parts.Identity is preserved deliberately: a body with no foreign image part is returned by reference and stays byte-identical, as is one whose images are already
image_url. Only themessagesarray, the messages holding a rewritten part, and theircontentarrays are rebuilt — the native path is a whitelist passthrough, so an incidental deep clone would itself be a behavior change. A remotesource.type:"url"reference is recognized and rewritten, never fetched; this PR adds no outbound request.Separately, the Chat inbound effort allowlist dropped
none. That is the runtime's disable sentinel, not an unknown value:src/reasoning-effort.tsaccepts it and maps it to omitting the wire parameter, andsrc/clients/config-export.tsmaps Pi'soffthinking level onto it. Dropping it let a provider default re-enable thinking the caller had explicitly turned off — not neutral for the Anthropic families that think by default and require an explicitthinking:{type:"disabled"}to stop (src/adapters/anthropic.ts:960-966).Addresses audit findings F1 and F7 from the 2026-09-14 provider/PI compatibility audit.
Stack (merge bottom-up)
Base is
dev. Layer 2 genuinely depends on this one — both changesrc/server/chat-completions.tsandsrc/chat/inbound.ts. Review this PR's diff only.The plan unit is
devlog/_plan/260914_provider_parity_stack/;003_blocker_corrections.mdis authoritative over the per-layer docs and records the corrections independent review required.Verification
Local verification NOT RUN BY USER INSTRUCTION. The repository owner directed that no local product check execute on this machine for this work. No
bun test,bun run test,typecheck,build,lint,structure:check,privacy:scanor prepush script was run by the authoring session, and none is claimed as passing, provisional, or assumed. This PR is therefore opened as a draft.Because red-first execution is impossible under that restriction, the regressions below were written to assert the desired behavior and reviewed statically rather than driven red first.
What real evidence exists:
c7e863b9e5fff52c224574981d7b6860a34ce0f2) is the gate for this layer. Results are not pre-judged here.df7dc1be53, before this unit's changes —bun run typecheckexit 0,bun run structure:checkexit 0,bun run privacy:scanexit 0. That is a baseline of unmodified source and is not coverage of anything this PR adds.Regression coverage added (not executed locally):
tests/server/chat-native-image-normalization.test.ts— recognizer accepts both OpenAI spellings, Pidata/mimeType, and both Anthropicsourceforms; returns null for a part with no usable reference. Normalization rewrites a Pi part, preserves adetailhint, handles image-only and tool-message content, returns the identical object reference when there is no image and when images are alreadyimage_url, and leaves other body fields untouched. Text-only diversion is asserted for Pi, Anthropic base64, Anthropic remote-url and tool-carried images; a text-only body still takes the native path and a vision-capable route still keeps an image body on it.tests/server/chat-inbound-reasoning-none.test.ts— flat and nestednonespellings survive intoreasoning.effort, the produced body still validates againstresponsesRequestSchema, every other ladder value is unchanged, and an unknown effort is still ignored rather than forwarded.Both files are registered in
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json(2-line insertions each, no reordering).Checklist
structure/data-planes/inbound-compat.mdgains the two sections that own this behavior, perstructure/INDEX.mdownership forsrc/chat/andsrc/server/.Current stack synchronization
The manual stack was synchronized bottom-up with
dev@246b5cab432b03cbec1766c2faffac13d6e39321.The repository's existing
2.55.0version change came from that parent; no artificial feature-branch version bump or release-test suppression was used.Order: #4534 → #4535 → #4536 → #4539 → #4562.
Current head:
c7e863b9e5fff52c224574981d7b6860a34ce0f2. Current base:dev.Each parent is an ancestor of its child. All five branches were pushed using
git push --no-verify.Fresh hosted CI is required at these new heads. Earlier green jobs or the historical
2.54.0release-line failure are not represented as new-head results. The PR remains draft; no merge or release was performed. No product validation ran on the connected Mac.Summary by CodeRabbit
New Features
reasoning_effort: "none".Documentation
Tests