fix(openai-chat): bound flattened tool wire names for strict gateways (#4679) - #4774
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(#4774)은 이슈 #4679를 고칩니다. Codex 클라이언트가 MCP/앱 도구를 많이 선언하면, 프록시는 채팅 게이트웨이용으로 이름을 지금 비교 기준인 이 PR이 하는 일은 그 생성 함수에 64 UTF-8 바이트 게이트웨이 호환 정책을 넣는 것입니다. 한도를 넘는(또는 예약 철자를 이미 쓰는) 이름은 64바이트는 OpenAI Responses 스키마 상한(128)이 아니라 Chat Completions/엄격 게이트웨이 쪽의 더 짧은 한도입니다. 소스 주석과 스택 위치가 중요합니다. base는 라인 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: cbcd0e20a2
ℹ️ 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".
| const flat = namespace ? `${namespace}__${name}` : name; | ||
| return needsBoundedToolName(flat) ? boundedToolName(namespace, name, flat) : flat; |
There was a problem hiding this comment.
When a client declares a bare tool name longer than 64 UTF-8 bytes—or one matching the reserved alias pattern—this branch replaces it even though namespace is undefined. However, buildToolBridgeMaps only adds reverse mappings inside its if (t.namespace) branch (src/server/responses/collaboration.ts:163-165), so an upstream echo of the new ocx_* name is emitted to the client unchanged in both buffered and streaming responses. The client then cannot match the call to its declared tool; register changed bare names in the reverse map as well, or avoid transforming them here.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
9984bca to
a9eb794
Compare
e3d5e7f to
cf8969f
Compare
lidge-jun
changed the title
a9eb794 to
d75657e
Compare
cf8969f to
a85315a
Compare
d75657e to
3d74681
Compare
a85315a to
441dc16
Compare
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>
3d74681 to
217fe01
Compare
441dc16 to
99c977d
Compare
…guard fix(codebuddy): refuse leaked vendor tool-call scaffolding (#4596)
|
Cascading downward. Bounded tool wire names now apply only in the openai-chat translation path. Kiro, the Google wire compiler and the Meta Muse alias each already own their names, and hooking the shared flattening point intercepted them — CI demonstrated that twice, once through Google and once through Kiro. The 64-character bound is a gateway-compatibility policy, not an upstream Responses requirement: upstream raised that limit to 128 in openai/codex#39594, and the code, description and structure docs all say so now. 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 |
1 participant
Summary
Carries #4715 by @HulianBuligon for #4679, with the fix moved to a different layer — see "Why the placement changed" below.
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 intoadditional_tools, 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:
api.meta.aiA 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_choiceall pass through the same registry, and both the streaming and buffered parsers restore the echoed alias beforetool_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. A historical call can keep its namespace without being redeclared, so 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 all 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 64 on this wire is correct for this wire alone.
Why the placement changed
#4715 placed the bound in the shared
namespacedToolName()helper, and that was provisionally accepted. Hosted CI then showed twice that the shared point intercepts adapters which already had an answer:tests/adapters/google/google-wire-compiler.test.ts— Google received a pre-aliased name and its own restore path broke, returning the syntheticocx_…spelling to the client.tests/providers/kiro/kiro-adapter.test.ts— "long namespaced tool names are normalized to Kiro's <=64-char charset" expected/_[0-9a-f]{8}$/and receivedocx_ent_applications_9ecUBk…, because the shared helper pre-empted Kiro's normalizer.Breaking two different adapters in two rounds is the signal that the bound sat at the wrong layer.
src/types/tools.tsis now byte-identical to the merge base.The problem statement and issue analysis are the original author's; only the placement changed, and the
Co-authored-bytrailer is retained.Closes #4679
Verification
Static source review plus an adversarial static audit, and 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. The verification claims in #4715's description are that author's, not re-asserted here.An adversarial audit of the first draft of this rewrite returned FAIL and found two defects that were fixed before pushing:
A follow-up review also removed two
throws on the request path: distinct identities really can flatten to the same name (namespacea__b+ namecversus namespacea+ nameb__c), and turning that into a hard error would have been a regression beyond this issue's scope. Ambiguous flattened spellings are now simply left out of the replay rewrite map rather than guessed, and map construction iterates in sorted identity order so no outcome depends on catalog order.Regression coverage in
tests/responses/bounded-tool-names.test.ts:bounds and restores the exact reported identity across request, replay, tool_choice, and responsebounds and restores a replay-only historical call absent from the current catalogleaves names at or under 64 characters and ordinary bare names byte-identicalderives deterministic distinct aliases independent of catalog orderkeeps shared naming untouched so Kiro and Google retain adapter-owned normalization— this one fails if anyone reintroduces a bound in the shared helperaliases colliding identities but leaves their ambiguous replay spelling unchangedstructure/providers/chat-compat.mdrecords the adapter-ownership split and the compatibility-limit framing with the upstream reference.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 changes, and ordinary catalogs are unaffected)