-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(chat): preserve external screenshot tool results and image detail #3586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
78e0144
docs: plan external-client image roundtrip repair
invalid-email-address d752746
fix(chat): preserve image detail and screenshot tool results
invalid-email-address 9e80ee1
test(chat): cover user image forms and ship their contract
invalid-email-address 4a9a125
docs(chat): scope tool image placement to its adapter
invalid-email-address File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| # External-client image round trips | ||
|
|
||
| ## Loop specification | ||
|
|
||
| - Class: C3 protocol compatibility repair; spec-satisfaction, no optimization race. | ||
| - Trigger: external clients report missing screenshots through OpenAI routes. | ||
| - Goal: preserve supported image bytes, URLs, ordering and detail across translation. | ||
| - Non-goals: no new uploader, provider settings, auth changes, live-service restart, | ||
| release, image synthesis, or unrelated adapter refactor. | ||
| - Verifier: standalone converter/parser/adapter body inspection, TypeScript, exact-head | ||
| GitHub CI. ALL local test suites are forbidden by the user, including focused suites. | ||
| - Stop: reviewed two-layer stack merged bottom-up to dev with green CI and ancestry. | ||
| - Memory: this unit and the session-bound goalplan/ledger. | ||
| - Outcomes: DONE only with proof; external dependencies may be BLOCKED/NEEDS_HUMAN; | ||
| unsafe expansion is UNSAFE. No implementation-success claim from docs-only work. | ||
| - Scope: this managed checkout, read-only Aside official docs, GitHub stack/CI/admin | ||
| merge. Maximum four concurrent agents; reassess after 90 minutes; no token cap set. | ||
| - Escalation: reclaim a lane after two distinct failed agents; any delegated writes | ||
| must be planned with disjoint paths before B. No production credentials in artifacts. | ||
|
|
||
| ## Measured baseline and hypotheses | ||
|
|
||
| H1: normal user images disappear in Responses serialization. Falsifier: compare the | ||
| synthetic URL in final request JSON. REJECTED: direct Chat converter -> parseRequest -> | ||
| canonical forward buildRequest preserves the input_image, as does openai-chat. | ||
| H2: Chat ingress drops image metadata or tool-result images. Falsifier: compare role:user | ||
| and role:tool with identical image_url parts. CONFIRMED: user detail is absent and tool | ||
| output becomes just `Read this`. Source: src/chat/inbound.ts:80 and :284. | ||
| H3: external route/native forwarding or Claude ingress drops otherwise preserved | ||
| images. Independent read-only investigation pending; don't assume a token count alone | ||
| identifies a serializer. Plain Claude image and tool-result paths have dedicated mapping. | ||
|
|
||
| Baseline command: standalone `bun -e` importing chat/inbound, responses/parser, | ||
| openai-responses, openai-chat and createTranslatorBudget. Exit 0; direct source imports | ||
| observe the actual owners, no bun:test import and no network. User image retained in | ||
| both wire formats; identical tool image absent from both. Original typecheck could not | ||
| resolve bun-types in this fresh worktree; frozen-lockfile dependency install (scripts | ||
| disabled) completed, with no manifest/lock edits. CI remains the test-suite authority. | ||
|
|
||
| No-code options: do nothing leaves demonstrated loss; deletion/configuration cannot | ||
| restore discarded payloads. Reuse userContentToBlocks and existing downstream image | ||
| serialization. Do not add a generic image helper or patch correct Responses code. | ||
|
|
||
| ## Dependency-ordered roadmap | ||
|
|
||
| 1. wp0: docs-only roadmap and independent audit (this cycle). | ||
| 2. wp1 / 010: preserve Chat image detail and structured tool output; lower PR to dev. | ||
| 3. wp2 / 020: cross-protocol wire regressions and public contract; child PR to lower | ||
| branch, then CI/review/admin-merge bottom-up, retarget child and verify again. | ||
|
|
||
| Existing placement is reused: src/chat/, tests/responses/, public reference/proxy-formats, | ||
| structure/04_transports-and-sidecars.md. No new package, runtime module, or config. | ||
| The user explicitly requested stacking; the upper layer consumes the corrected | ||
| converter and protects the integrated contract independently of unit-level assertions. | ||
|
|
||
| ## Continuity | ||
|
|
||
| Roadmap audit: independent gpt-6-astra high reviewer returned GO-WITH-FIXES, | ||
| two medium findings. Both folded: exact no-suite typecheck/push commands and actual | ||
| Claude converter export. Direct node tsc exits 0. Standalone reproduction at | ||
| `.tmp/external-image-probe.ts` exits 1 before production edits with imageRetained=false | ||
| and detailRetained=false. No local suite ran. Aside opened official Chat docs confirming | ||
| image_url.url and nested auto/low/high detail; Anthropic tool-result docs confirm | ||
| nested image content. No upload handler is necessary for data URLs. | ||
|
|
||
| Roadmap initially recorded against freshly fetched origin/dev. Never claim the whole | ||
| reported model-specific outage fixed merely because a converter fix lands. Preserve | ||
| the negative result for ordinary user images in the final report. |
55 changes: 55 additions & 0 deletions
55
devlog/_plan/260905_external_image_roundtrip/010_chat_image_parts.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| # Chat image-part foundation | ||
|
|
||
| Depends on wp0. One full PABCD cycle, one lower PR. | ||
|
|
||
| ## MODIFY src/chat/inbound.ts | ||
|
|
||
| Keep imageUrlFromPart's current object/string URL support. In userContentToBlocks, | ||
| extend the input_image push with detail from the nested image_url object, or the part | ||
| for the already-supported string shorthand. Preserve only auto/low/high detail. | ||
|
|
||
| ```diff | ||
| - blocks.push({ type: "input_image", image_url: imageUrl }); | ||
| + const detail = isRec(raw.image_url) ? raw.image_url.detail : raw.detail; | ||
| + blocks.push({ type: "input_image", image_url: imageUrl, | ||
| + ...(detail === "auto" || detail === "low" || detail === "high" ? { detail } : {}) }); | ||
| ``` | ||
|
|
||
| For role:tool, reuse the existing content converter, retaining the original text-only | ||
| string behavior when no valid image is present. Responses function_call_output accepts | ||
| input_text/input_image, not input_video; don't newly forward video tool blocks. | ||
|
|
||
| ```diff | ||
| - const output = typeof msg.content === "string" ? msg.content : contentToText(msg.content); | ||
| + const blocks = userContentToBlocks(msg.content); | ||
| + const output = blocks.some(part => part.type === "input_image") | ||
| + ? blocks.filter(part => part.type === "input_text" || part.type === "input_image") | ||
| + : contentToText(msg.content); | ||
| ``` | ||
|
|
||
| Keep output_text tool parts supported: extend the reusable converter's text recognition | ||
| to output_text (already accepted by contentToText) so mixed arrays lose no old text. | ||
| Field chain: nested Chat detail -> input_image.detail -> parser image.detail -> existing | ||
| Chat image_url.detail; raw Responses preserves detail. No new type/enum/config. | ||
|
|
||
| ## MODIFY tests/responses/chat-completions-endpoint.test.ts | ||
|
|
||
| Add converter-level cases beside the existing conversion tests: | ||
| - user image: remote/data URL, nested detail, no detail, string shorthand; | ||
| - tool image: function_call plus mixed text/image output retains exact order; | ||
| - image-only tool output stays a nonempty array; | ||
| - text-only string/array and invalid image keep existing text behavior; | ||
| - mixed output_text/image preserves text; video does not enter function output. | ||
| Use explicit expected objects, not converter-derived expectations. No removed assertions. | ||
|
|
||
| ## Acceptance and delivery | ||
|
|
||
| Repeat the baseline standalone invocation: tool output must now contain input_image and | ||
| both final wire bodies must contain the synthetic URL; user detail must survive. Run | ||
| `node node_modules/typescript/bin/tsc --noEmit` (not a suite), add tests but execute | ||
| them only in CI. Review source and tests, commit, `git push --no-verify origin | ||
| codex/external-image-parts`, and open templated PR to dev. The user explicitly forbids | ||
| local suites; the installed pre-push hook runs package.json prepush including the full | ||
| suite, so that hook must be bypassed for this authorized push. No persistent hook | ||
| configuration change. Existing large files | ||
| are extended narrowly to avoid an unrelated split. No new exports or upload handler. |
60 changes: 60 additions & 0 deletions
60
devlog/_plan/260905_external_image_roundtrip/020_wire_contract.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # External wire contract and stack delivery | ||
|
|
||
| Depends on wp1 and its corrected Chat converter. One full PABCD cycle. | ||
|
|
||
| ## MODIFY tests/responses/openai-responses-passthrough.test.ts | ||
|
|
||
| Import real chatCompletionsToResponsesBody, anthropicToResponsesBody, | ||
| parseRequest, and createOpenAIChatAdapter wrapped with the | ||
| existing withTestTranslatorBudget. Add a table-driven regression for each ingress: | ||
| Chat user image, Chat tool screenshot (depends on wp1), Claude user image, Claude | ||
| tool_result image. Use data and HTTPS URL fixtures, two ordered images, and image-only | ||
| tool output. Build each through public API-key Responses, canonical ChatGPT forward, | ||
| and Chat adapter; assert exact image payloads in the actual serialized body, original | ||
| input immutability, and tool call/result adjacency. Add orphan tool-result case using | ||
| the existing repair path; don't modify production adapters unless evidence demands it. | ||
| Do not claim these body tests prove upstream model OCR or live route selection. | ||
|
|
||
| ## MODIFY docs-site/src/content/docs/reference/proxy-formats.md | ||
|
|
||
| After the Chat intro add: | ||
|
|
||
| ```diff | ||
| + Image URLs and base64 data URLs use Chat `image_url` content parts. Translation | ||
| + preserves supported `detail` values (`auto`, `low`, `high`). OpenCodex also accepts | ||
| + image-bearing tool-result arrays as a compatibility extension: Responses routes | ||
| + retain structured output, while Chat adapters send tool images in a following user | ||
| + message because the upstream Chat tool role is text-only. Plain text results remain | ||
| + strings. Native passthrough follows its upstream contract. | ||
| ``` | ||
|
|
||
| No locale currently contradicts this additive contract; inspect sibling translated | ||
| sections before deciding whether an amendment is needed. Document no model entitlement. | ||
|
|
||
| ## MODIFY tests/responses/chat-completions-endpoint.test.ts | ||
|
|
||
| Reuse mockDualWireUpstream (line 113) and dualWireConfig (line 2764), beside the | ||
| existing Chat-to-Responses HTTP regression (line 2834). POST a user image with high | ||
| detail and a paired tool screenshot to mock/grok-4.5; consume the stream and assert | ||
| one captured /responses body with unchanged ordered image parts. This is real HTTP | ||
| route proof in CI, not real-model OCR or canonical account authentication. | ||
|
|
||
| ## MODIFY structure/04_transports-and-sidecars.md | ||
|
|
||
| Add one short paragraph beside the Chat inbound responsibility: its converter owns | ||
| detail and tool-image preservation; adapters own target-specific image placement. | ||
| Retain all existing transport/security/sidecar policy. | ||
|
|
||
| ## Acceptance / delivery | ||
|
|
||
| Run `node node_modules/typescript/bin/tsc --noEmit`; public documentation build in CI | ||
| or local build (not tests); focused | ||
| regressions and full OS suites in GitHub CI only. A fresh independent patch audit | ||
| checks each wire assertion and absence of secret/logging changes. Publish child branch | ||
| codex/external-image-wire-contract against the open lower branch using `git push | ||
| --no-verify` (same explicit no-local-suite override as 010). Record admin bypass | ||
| authorization in both PR bodies; merge lower only when exact-head full CI is green, | ||
| don't delete parent branch. Prefer merge commits to preserve stack ancestry; retarget | ||
| the child to dev and refresh CI/review. If squash is used, restack and reverify its new | ||
| HEAD before merge. Fetch origin/dev and prove both merge SHAs ancestors. Archive this | ||
| unit only after the completed outcome is public. No restart/deployment is authorized. |
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
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.