feat(config): map provider id to SDK protocol via providerProtocol (#5758)#5793
feat(config): map provider id to SDK protocol via providerProtocol (#5758)#5793qqqys wants to merge 3 commits into
Conversation
…wenLM#5758) Decouple provider identity from SDK routing in a backward-compatible way (issue QwenLM#5758, Approach A), without re-introducing the modelProviders structural change that got QwenLM#5089 reverted. - Add a `providerProtocol` settings dictionary mapping a provider id to the built-in protocol (AuthType) that should route it. `modelProviders` stays a Record<providerId, ModelConfig[]> array; old versions ignore the new key. - ModelRegistry resolves each provider id to a protocol (explicit map entry, or the id itself when built-in) and registers custom ids under that protocol, merging providers that share one protocol. Unmapped unknown ids are skipped (typo guard); the resolver is a pure function. - Honor a custom provider's envKey/metadata: auth pre-flight and CLI model resolution now look up models by resolved protocol across all provider ids, not just the protocol key. - Surface skipped providers (no mapping / unknown protocol) through the visible CLI warnings path instead of debug-only logging. - reloadModels preserves the protocol map when omitted (undefined) and replaces it when given; ACP workspaceReload threads providerProtocol so long-lived sessions pick up changes. End-to-end providerId identity and the ACP/VSCode wire format are deferred to a follow-up. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@zzhenyao 你是 #5089 的作者,也是 #5758 这个讨论的发起人,最了解 provider/protocol 解耦的来龙去脉,能否帮忙 review 一下这个 PR?🙏 这个 PR 实现的是 #5758 里讨论的 Approach A:新增一个 特别想听听你对这几点的看法:
@zzhenyao As the author of #5089 and the one who opened the #5758 discussion, you have the most context here — would you mind helping review? This implements Approach A from #5758 (a separate |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @qqqys — thanks for picking up Approach A from #5758. Before I dive into code review, the PR body is missing most of the required sections from the PR template. Please restructure so reviewers get the info they need at a glance.
Missing required headings (currently in the body as custom sections):
| Required heading | What's there instead |
|---|---|
## What this PR does |
## Summary |
## Why it's needed |
(motivation is spread across the Summary and the #5758 link) |
## Reviewer Test Plan → ### How to verify |
## Testing (no Reviewer Test Plan wrapper, no ### How to verify sub-heading) |
### Evidence (Before & After) |
missing — N/A is fine since this is non-user-visible, but the heading is still required |
### Tested on OS table |
missing |
## Risk & Scope (with the three bullets) |
## Scope only |
## Linked Issues |
Relates to #5758. as a trailing line, under no heading |
<details>中文说明</details> |
missing entirely |
The quickest fix is to copy the template, paste your existing content under each heading, and fill N/A where it doesn't apply (Evidence, Environment). The Chinese translation is required per the project convention.
Quick read on direction while you restructure — not a full review yet:
- Direction looks aligned. The problem in #5758 is real (ACP/VSCode pass
providerId + modelId, CLI usesmodelId + baseUrl), and Approach A (additiveproviderProtocoldictionary, keepmodelProvidersshape untouched) is exactly what was discussed as the least-disruptive path. Claude Code's CHANGELOG has many fixes around third-party provider sessions (Bedrock/Vertex/Foundry routing, banner labels, env inheritance), confirming the area is actively user-facing. - Scope feels disciplined for a first cut. The PR explicitly defers provider identity and the ACP/VSCode wire format to a follow-up, which keeps this one reviewable and revertable.
- One approach-level question for you: the registry still buckets by protocol/AuthType after this change, so two custom providers mapping to the same protocol merge (first-wins on
id+baseUrlper your description). Is that the right default when a user actually wants two distinct endpoints under the same protocol, or should merging be opt-in rather than automatic? Happy to discuss in #5758 or in the next revision.
Once the template is filled in, I'll come back for the full code review. 🙏
中文说明
@qqqys 感谢你承接 #5758 里的 Approach A。在进入代码评审之前,PR 正文缺少了 PR 模板 里的大部分必填小节,需要按模板结构重新组织一下。
缺少的必填标题(目前用的是自定义小节名):
| 必填标题 | 当前写法 |
|---|---|
## What this PR does |
## Summary |
## Why it's needed |
动机分散在 Summary 和 #5758 链接里 |
## Reviewer Test Plan → ### How to verify |
## Testing(没有 Reviewer Test Plan 外层,也没有 ### How to verify 子标题) |
### Evidence (Before & After) |
缺失 — 这是非用户可见改动可以写 N/A,但标题仍需保留 |
### Tested on 操作系统表格 |
缺失 |
## Risk & Scope(含三个 bullet) |
只有 ## Scope |
## Linked Issues |
Relates to #5758. 作为末尾一行,没有放在任何标题下 |
<details>中文说明</details> |
完全缺失 |
最快的修法:把模板复制一份,把现有内容搬到对应标题下,不适用的地方填 N/A(Evidence、Environment)。中文说明按项目惯例是必填的。
关于方向的简短判断(不是完整评审):
- 方向一致。 #5758 讨论的问题是真实存在的(ACP/VSCode 传
providerId + modelId,CLI 用modelId + baseUrl),Approach A(新增一个providerProtocol字典、不碰modelProviders结构)确实是当时讨论出的最小改动路径。Claude Code 的 CHANGELOG 里也有大量围绕第三方 provider session 的修复(Bedrock/Vertex/Foundry 路由、banner 标签、env 继承),说明这个方向在用户侧是活跃的。 - 第一刀的范围比较克制。 PR 明确把 provider identity 和 ACP/VSCode wire format 推迟到 follow-up,便于独立 review 和回滚。
- 方案层面的一个问题: 改动后 registry 仍按 protocol/AuthType 分桶,所以两个自定义 provider 映射到同一 protocol 时会自动合并(按你的描述是
id+baseUrl先到先得)。如果用户确实想在同一 protocol 下挂两个不同 endpoint,这样的默认行为是否合适?还是应该把合并改成显式 opt-in?欢迎在 #5758 或下一版里继续讨论。
模板补齐后我会回来做完整的代码评审。🙏
— Qwen Code · qwen3.7-max
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Solid implementation of the provider protocol mapping feature — the design is clean, backward compatible, and well-tested at the core level (202 tests passing). A few suggestions below for edge cases in the ACP hot-reload path and downstream consumers.
Suggestions
-
ACP
refreshAuthskipped onproviderProtocol-only changes (acpAgent.ts~line 6891): The reload path at line 6858 correctly updates the model registry whenproviderProtocolchanges, but the subsequentrefreshAuthblock only fires whenchanged.has('modelProviders') || envChanged. AproviderProtocol-only change leaves the content generator's live auth state (API key, base URL) stale against the remapped registry. Consider addingchanged.has('providerProtocol')to therefreshAuthcondition. -
workspace-providers-status.tskey mismatch for custom providers:buildExplicitModelBaseUrlsbuilds keys using the raw provider id frommodelProviders(e.g.,"idealab"), but the consumer looks up using the resolved protocol (e.g.,"openai"). For custom providers mapped viaproviderProtocol, the keys never match, somatchesCurrentBaseUrlreceivesmodelHasExplicitBaseUrl: false, potentially causing incorrect current-model detection. Consider resolving throughproviderProtocolinbuildExplicitModelBaseUrlsto align the key namespaces. -
ModelsConfig.reloadModelProvidersConfigreload path untested with two args (modelsConfig.test.ts): All existing tests pass only the first argument. TheproviderProtocolConfig wiringtests cover the constructor path, but the reload path with the second argument is only exercised throughModelRegistry.reloadModelsdirectly. A test likemc.reloadModelProvidersConfig(newProviders, { idealab: 'openai' })would guard the delegation wiring.
Test gaps (Nice to have)
- No test coverage for the ACP
providerProtocolchange detection (acpAgent.ts:6859-6868). Typical for ACP integration code but worth noting. findModelConfiginauth.tsgainedproviderProtocolbut has no direct test (consistent with the file's historical coverage).
— qwen3.7-max via Qwen Code /review
| config.reloadModelProvidersConfig(newMerged.modelProviders); | ||
| config.reloadModelProvidersConfig( | ||
| newMerged.modelProviders, | ||
| newMerged.providerProtocol, |
There was a problem hiding this comment.
[Suggestion] When providerProtocol is removed from settings entirely, newMerged.providerProtocol is undefined. Per ModelRegistry.reloadModels semantics, undefined means "preserve the existing map" — so removing the mapping from settings.json does not actually unregister the custom providers. They persist in the registry until a full process restart.
Pass newMerged.providerProtocol ?? {} instead, which maps "absent from settings" to "empty mapping" (clear). The JSDoc on reloadModels explicitly warns about this footgun: "Callers that want to preserve must omit the argument, not pass settings.providerProtocol ?? {}."
| newMerged.providerProtocol, | |
| newMerged.providerProtocol ?? {}, |
— qwen3.7-max via Qwen Code /review
What this PR does
This PR adds a backward-compatible provider protocol mapping so a custom provider id can route through an existing SDK protocol while keeping provider identity separate from transport behavior. The mapping is optional: built-in providers continue to route automatically, unmapped custom providers keep the existing warning-and-skip behavior, and mapped custom providers can reuse an existing protocol while retaining their own credentials and model metadata.
Why it's needed
Issue #5758 needs a way to support custom provider names that speak a built-in protocol without repeating the reverted shape change from #5089. Keeping the provider list shape stable avoids a settings migration while still allowing users to identify providers by their own names and route requests through the correct SDK implementation.
Reviewer Test Plan
How to verify
Reviewers should confirm that a custom provider id can be mapped to an existing protocol, that built-in provider ids still work without explicit mapping, that unmapped unknown ids still warn and skip, and that reload paths preserve or replace the mapping according to the incoming settings. Also confirm that the generated VS Code settings schema includes the new top-level mapping and the clarified provider description.
Evidence (Before & After)
Before: custom provider ids were treated as unknown routing keys unless they matched a built-in protocol, so they could not clearly separate identity from SDK protocol. After: a custom provider id can explicitly map to a built-in protocol, shares the protocol route, and keeps its provider-specific credentials and metadata. This is a non-TUI configuration behavior change, so screenshot evidence is N/A.
Tested on
Environment (optional)
Local validation used package unit tests, TypeScript checks, ESLint, and the settings schema generator. Cross-platform validation is left to CI.
Risk & Scope
Linked Issues
Relates to #5758.
中文说明
What this PR does
本 PR 增加了向后兼容的 provider protocol 映射能力,让自定义 provider id 可以通过现有 SDK protocol 路由,同时把 provider 身份和传输行为分开。该映射是可选的:内置 provider 仍会自动路由,未映射的自定义 provider 仍保持现有的警告并跳过行为,已映射的自定义 provider 可以复用现有 protocol,同时保留自己的凭证和模型元数据。
Why it's needed
#5758 需要一种方式支持“自定义 provider 名称 + 内置 protocol”的组合,同时不能重复 #5089 中已回滚的结构变更。保持 provider 列表结构稳定可以避免 settings 迁移,又能让用户用自己的名称标识 provider,并把请求路由到正确的 SDK 实现。
Reviewer Test Plan
How to verify
Reviewer 应确认自定义 provider id 可以映射到现有 protocol,内置 provider id 不需要显式映射仍可工作,未映射的未知 id 仍会警告并跳过,reload 路径会按传入 settings 保留或替换映射。同时确认生成的 VS Code settings schema 包含新的顶层映射项,以及更新后的 provider 描述。
Evidence (Before & After)
Before:自定义 provider id 除非正好匹配内置 protocol,否则会被当作未知路由 key,无法清晰地区分身份和 SDK protocol。After:自定义 provider id 可以显式映射到内置 protocol,共用该 protocol 路由,并保留 provider 自己的凭证和元数据。这是非 TUI 配置行为变更,因此截图证据为 N/A。
Tested on
Environment (optional)
本地验证使用了包级单测、TypeScript 检查、ESLint,以及 settings schema 生成器。跨平台验证交由 CI。
Risk & Scope
Linked Issues
Relates to #5758.