Repository navigation
[WRONG BRANCH] fix(catalog): normalize the custom-provider model-discovery join (#4724) - #4773
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(#4773)은 이슈 #4724를 닫는 패치다. 커스텀(비레지스트리) 프로바이더의 이 PR이 하는 일은 좁고 분명하다. 스택 위치도 중요하다. 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: 9984bca2a8
ℹ️ 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".
| export function providerModelsUrl(baseUrl: string): string { | ||
| const trimmed = baseUrl.trim().replace(TRAILING_SLASHES, ""); | ||
| const withoutEndpoint = trimmed.replace(TRAILING_MODELS, ""); | ||
| return `${withoutEndpoint}/models`; |
There was a problem hiding this comment.
This changes both src/providers/ and src/oauth/, but the commit updates only structure/runtime.md. The source map also assigns these areas to structure/transports/inventory.md, structure/providers/xai-grok.md, and, for providers, structure/subagents.md; update each mapped document to reconcile or explicitly preserve its contract rather than leaving the required architecture review incomplete.
AGENTS.md reference: structure/AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
68b5c73 to
70129ac
Compare
9984bca to
a9eb794
Compare
d75657e to
3d74681
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>
3d74681 to
217fe01
Compare
|
Cascading downward. A custom provider base URL with a trailing slash no longer produces a doubled path segment. The first attempt guarded a 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 |
6a8f754
into
codex/pw3-kimi-tool-adjacency
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
…eurl-join fix(catalog): normalize the custom-provider model-discovery join (lidge-jun#4724)
1 participant
Summary
A custom (non-registry) provider whose
baseUrlends in/produced a doubled model-discovery path —https://gateway.example.com/v1//models. Gateways that treat the doubled path as a distinct route reject it (the report observedHTTP 403), so discovery failed and the catalog silently fell back to the configured models, with the provider's discovery status reading{"status":"failed","reason":"http","httpStatus":403}.The send paths were normalized already —
openaiChatCompletionsUrl(#1582) andopenaiResponsesUrl— but discovery was not:buildModelsRequest()insrc/oauth/index.tsappended the endpoint verbatim as\${baseUrl}/models.resolveProviderModelDiscoveryUrl()insrc/providers/model-discovery.tsreturns that default unchanged for a provider with no registry spec — precisely the custom case.Registry providers escaped the bug because
new URL(spec.path, base)collapses the doubled slash.providerModelsUrl()mirrors the existingopenaiChatCompletionsUrlprecedent rather than inventing a second normalization policy — same trim, same strip-if-already-pasted, same single append:It lives beside the other discovery logic rather than being shared across the module boundary, because the send-path helper belongs to the chat adapter while this belongs to provider discovery. Both production callers of the default discovery URL now use it: catalog discovery (
buildModelsRequest) and API-key validation (validateApiKey).Scope of behaviour change, enumerated rather than assumed:
baseUrlcarries outer whitespace, one or more trailing slashes, or an already-pasted/models.baseUrlalready written ashttps://gw.example.com/v1; any registryspec.path; absolute endpoint overrides; relative endpoint overrides; and the Google CCA, AI Studio and Anthropic branches.https://gw.example.com/api/openai/v1keeps/api/openaiand is not collapsed to the origin.Workaround for anyone on a released build: configure
baseUrlwithout the trailing slash.Closes #4724
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:
resolveProviderModelDiscoveryUrland thediscoveryUrlclosure inbuildModelsRequestbefore editing, then enumerated the changed vs. byte-identical input classes listed above./modelsURL, so no third caller keeps the old join.src/adapters/openai-chat-url.tsto confirm the normalization rule is the same one the send path already establishes, rather than a new policy invented for discovery.git diff --checkclean.Regression coverage added to
tests/providers/provider-model-discovery-contract.test.ts(the existing owner of this contract, so no new file):normalizes custom model discovery URLs without collapsing path prefixes (#4724)— no slash, one slash, multiple slashes, nested prefix, longer prefix, pasted/models.keeps registry path discovery independent of default URL normalization (#4724)keeps absolute and relative discovery endpoint overrides unchanged (#4724)normalizes a custom API-key validation discovery URL (#4724)— covers the second production caller.structure/runtime.md, which ownssrc/providers/, is updated so the documented custom-provider discovery rule states the normalization instead of implying a verbatim join.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/runtime.md; the user-facing fix is that a previously broken configuration now works, so no configuration documentation changes)validateApiKeyis touched only to reuse the same URL join; no change to auth headers, token handling, or destination trust — the helper cannot move a request to a different origin because it only trims and appends a path)