[agent] fix: refuse lossy media conversion at the final adapter boundary - #4562
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (21)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds shared detection for untranslated audio and file inputs. Translated adapters and Chat projection now reject unsupported media explicitly, while native Responses, Azure, and Chat paths preserve existing media handling. ChangesInput media admission
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatInbound
participant RegisteredAdapter
participant Upstream
Client->>ChatInbound: submit request with media
ChatInbound->>ChatInbound: inspect translatable content
ChatInbound-->>Client: return sanitized 400 for lossy media
Client->>RegisteredAdapter: submit translated request
RegisteredAdapter->>RegisteredAdapter: inspect original _rawBody
RegisteredAdapter-->>Client: return unsupported_input_modality
Client->>Upstream: submit native Responses or Chat request
Upstream-->>Client: preserve native media handling
Merge Risk: ⚪ Minimal · up to Translated routes now explicitly reject unsupported media while native passthrough behavior remains preserved. No actionable merge-blocking issue is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 6 files. (15 skipped: 15 unsupported.)
✨ Finishing Touches 💡 1📝 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. |
리뷰 · 우선순위 62 / 80이 PR은 provider-parity 스택의 다섯 번째 층입니다. 지금 구체적으로 테스트는 라인 45 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
c5a66bb to
a98db76
Compare
fff3aa4 to
c8e7370
Compare
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: c8e737004b
ℹ️ 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".
| } else if (item.type === "message" || item.type === undefined) { | ||
| const kind = contentMedia(item.content); | ||
| if (kind) return kind; |
There was a problem hiding this comment.
When a Responses request carries a Codex agent_message whose content contains input_audio, input_file, or a file-ID-only input_image, this branch skips it because its type is neither message nor undefined. The explicit agent_message path in src/responses/parser.ts:215-225 then passes that content through inputContentParts, which reduces populated attachments to presence text, allowing a translated adapter to return success without the attachment. Include agent_message among the content-bearing item types and add a regression covering this established replay shape.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| const build = adapter.buildRequest.bind(adapter); | ||
| adapter.buildRequest = (parsed, incoming) => { | ||
| const kind = untranslatedResponsesInputMedia(parsed._rawBody); | ||
| if (kind) throw new Error(untranslatedInputMediaMessage(kind)); |
There was a problem hiding this comment.
When the same request activates the web-search or image bridge, those loops call this guarded builder at src/web-search/loop.ts:467 and src/images/loop.ts:575 before the normal core build-error catch. Because the guard throws a plain Error, both loops classify it as 502 Provider unreachable at lines 607 and 702; the runTurn image path similarly discards the emitted 400 status at src/images/loop.ts:758. A known nonretryable client input error is therefore reported as an upstream outage, inviting retries and misleading diagnostics. Use a shared typed client-input failure that the bridges preserve, or perform admission before entering them.
AGENTS.md reference: src/AGENTS.md:L17-L19
Useful? React with 👍 / 👎.
|
Maintainer integration per MAINTAINERS.md: landing this maintainer-authored PR on dev without a second maintainer approval.\n\nExact-head verification on c8e7370 (restacked onto the #4539 squash commit; tree identical to reviewed head fff3aa4): 25 checks pass, 2 skipped, 0 failed/cancelled. Run set: 34794607350 plus metadata workflows.\n\nLocal suite/typecheck/build: NOT RUN (hosted exact-head CI is the evidence). |
1 participant
Summary
Close the remaining silent-success part of the provider input audit: recognized audio/file attachments must not become placeholder text and then be reported as a successful translated request.
This layer was implemented directly by ChatGPT in a separate worktree. It does not add native audio/file transport, fetch attachment URLs, decode content, change credentials or enable vendor CLI tools.
buildRequestfollows the existing 400 response path;runTurnemits one typed, nonretryableunsupported_input_modalityerror before transport;localTerminaldeclines its success shortcut so the guarded builder can reject.function-image regression: native eligibility diverts it, but the converter does not implement legacy call/result pairing. Return an explicit migration error instead of silently discarding that result.Stack — review and merge bottom-up
devagent/provider-parity-04-modalityThis is a manual dependent PR chain, not native GitHub stack registration. No merge or automatic merge is requested.
Deliberate limits
The change closes silent-success behavior, not native media transport. Translated video markers remain adapter-specific. Native wire/model support remains upstream-dependent. Legacy image results must use modern
tool_calls/role: "tool"/tool_call_id; this does not invent a legacy function-call bridge. The full CodeBuddy/Qoder client-tool bridge and unverified gateway vision declarations remain outside this patch.Verification
Connected-Mac product checks: NOT RUN BY USER INSTRUCTION. No local test suite, typecheck, build, lint, structure/privacy validation or pre-push suite was executed for this layer. Commits and pushes use
--no-verify; no repository workflow, dependency or global Git configuration was changed.This is a draft pending exact-head hosted CI and the completed independent static review described below. Neither a written test nor a pending/skipped job is claimed as passing.
Separate ChatGPT Linux container: copied the exact two new production modules, verified their SHA-256 against commit
c1b34d4532c8e7e55cfdf0f8cc73b4ac2d82f3fc, transpiled them with the container's existing TypeScript compiler, then rannode --test guard.test.mjs: 12 pass, 0 fail. This is scanner/guard runtime proof, not the full Bun suite or repository-wide type checking. A negative control removed the compiled guard and made 5 tests fail (exit 1); restoring it returned 12 pass, 0 fail. The original production TypeScript remained unchanged. No connected-Mac product command ran.Hosted evidence at that head:
gatespassed; all 10 newadapter-input-media-guard.test.tscases and all 5 newchat-media-translation.test.tscases passed, including real registry/Azure passthrough and the real HTTP zero-upstream-send regression. Shards 1 and 3 completed successfully. Fulltest 4/4failed later in the inheritedrelease-version-line.test.ts: the stack carries package version2.54.0, and tagv2.54.0was published after the parent stack was prepared. Currentdevcarries2.55.0. No test was disabled and no artificial version bump was made in this feature patch. That historical version-line failure is not current-head proof: the stack has now been synchronized with currentdev; new-head hosted jobs are pending.Desired regressions added:
tests/adapters/adapter-input-media-guard.test.ts: content-only scanning; file-ID-only images; ordinary text/arguments/schema exclusion; builder and local-terminal ordering; typed runTurn failure without underlying execution; actual registry guards; raw Responses and Azure preservation using the same request after a rejected translated attempt.tests/responses/chat-media-translation.test.ts: user/tool audio and files rejected before conversion; native Chat messages unchanged; legacy function images explicitly rejected; real HTTP translated Responses requests return 400 with no upstream sends and no payload in the error.Both files are registered in the existing layout map and expected fixture. Source/diff inspection and hook configuration inspection were performed without executing product validation. The independent read-only Aside review completed with no blocking bugs found in the inspected scope. It inspected the scanner, guard, registry and Chat projection plus both test files and the narrow error/stream owners. It ran no product code. Optional additional cases were identified, not represented as executed tests.
Checklist
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:
fff3aa4047f3b0e0eea0df08cd983f3776721cfa. Current base:agent/provider-parity-04-modality.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.Independent review result
The Aside reviewer completed its source-only review of the direct media-guard implementation with no blocking bugs found. It independently confirmed final-wire alias handling, native-Chat exclusion, preprojection rejection, legacy-image explicit failure, build/runTurn/local-terminal ownership and content-free errors. Optional coverage suggestions remain distinct from proven regressions; the review is not a live-provider or full new-head CI result.
The two directly authored scanner/guard modules retain exactly the same SHA-256 at synchronized head
fff3aa4047f3b0e0eea0df08cd983f3776721cfaas the hash-checked container-tested implementation. Integration with the newly incorporated dev changes still requires that new head's hosted CI. No product tests ran on the connected Mac.Summary by CodeRabbit
Bug Fixes
Documentation
Tests