refactor(auth): unify provider config in core, simplify /auth as "Connect a Provider"#4287
Open
pomelo-nwu wants to merge 29 commits into
Open
refactor(auth): unify provider config in core, simplify /auth as "Connect a Provider"#4287pomelo-nwu wants to merge 29 commits into
pomelo-nwu wants to merge 29 commits into
Conversation
…xports Move all ProviderConfig definitions, registry (ALL_PROVIDERS), and utility functions (buildInstallPlan, resolveBaseUrl, etc.) from packages/cli/src/auth/ into packages/core/src/providers/ so both CLI and VSCode can share the same provider system. - Add core providers module with types, presets, install logic - Rewrite VSCode AuthMessageHandler to dynamically generate provider choices from ALL_PROVIDERS instead of hardcoding 3 providers - Add applyProviderInstallPlanToFile in VSCode settingsWriter using the ProviderSettingsAdapter abstraction - Delete 11 CLI re-export wrapper files, update ~20 import sites - Keep CLI-specific applyProviderInstallPlan (uses LoadedSettings) and openrouterOAuth.ts (CLI-only OAuth runtime) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
OpenRouter now uses the standard API-key flow under "Third-party Providers" (issue #4108). The whole OpenRouter OAuth implementation (PKCE, callback server, model auto-install) and the /manage-models command (only OpenRouter was wired in; /auth Step 2 already covers model selection) are removed. /auth is renamed around the "Connect a Provider" mental model: - Dialog title is now "Connect a Provider"; the OAuth main entry is gone - handleAuthSelect (mixed close + auth trigger) is split into a single-purpose closeAuthDialog; legacy wrappers (handleSubscriptionPlanSubmit, handleApiKeyProviderSubmit, handleCustomApiKeySubmit, ...) are dropped in favor of the unified handleProviderSubmit Core: openRouterProvider switches to authMethod='input', uiGroup='third-party', ships with two recommended free models, and is reordered to the end of the third-party list to keep DeepSeek as the default highlight. Net diff: 34 files, +124 / -3835. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
CLI and vscode now share core's applyProviderInstallPlan instead of keeping two parallel implementations. The CLI-only env rollback (snapshot process.env, restore on error) is folded into the core version so vscode also benefits from it. CLI ships a LoadedSettingsAdapter that maps LoadedSettings to core's ProviderSettingsAdapter contract. Backup/restore is layered: write a .orig file, structuredClone settings + originalSettings, then recomputeMerged() on restore — same guarantees as before, just routed through the adapter. Tests for the install logic are migrated to core and rewritten against the adapter mock (more focused than the previous LoadedSettings/Config mocks). packages/cli/src/auth/ is gone entirely. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Every preset has had authMethod='input' since OpenRouter switched to the
standard API-key flow, making the field a dead dimension. Removing it
cleans up three never-taken branches and aligns the type with reality:
connecting a provider always means entering an API key.
- core: remove ProviderConfig.authMethod; shouldShowStep('apiKey') is
now unconditionally true; drop authMethod from 9 presets
- vscode AuthMessageHandler: drop the OAuth branch in handleAuthInteractive
- vscode WebViewProvider: simplify the apiKey-required guard
- tests: update provider-config.test and custom-provider.test
If a future provider needs a browser-based flow, the field can be
re-introduced; for now the smaller surface is worth more.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Rename coding-plan.{ts,test.ts} → alibaba-coding-plan.{ts,test.ts} and
token-plan.{ts,test.ts} → alibaba-token-plan.{ts,test.ts} so the file
names line up with the existing alibaba-standard preset and make it
obvious at a glance which presets belong to Alibaba ModelStudio.
Export names (codingPlanProvider, tokenPlanProvider, TOKEN_PLAN_*,
CODING_PLAN_*) are unchanged — only the file paths and the two
imports in all-providers.ts / index.ts move.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…er-config-to-core # Conflicts: # packages/cli/src/ui/components/ManageModelsDialog.test.tsx # packages/cli/src/ui/components/ManageModelsDialog.tsx # packages/cli/src/utils/apiPreconnect.ts # packages/core/src/providers/all-providers.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
pomelo-nwu
force-pushed
the
refactor/unify-provider-config-to-core
branch
from
ae4fcc0 to
efcfce5
Compare
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
The dotted-key writer in createFileSettingsAdapter walked through any segment, including __proto__/constructor/prototype, which would let a malicious or malformed ProviderInstallPlan reach Object.prototype. Refuse to write paths containing reserved segments and use hasOwnProperty when traversing intermediate objects so that inherited properties cannot redirect the walk. Addresses CodeQL alert #226 surfaced on PR #4287. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
In the /auth Custom Provider advanced-config step, "Enable modality" should default to Image + Video only. Audio was on by default, which implied the model accepts audio input even though most providers people configure here don't. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
In Custom Provider Step 2/6 (and on protocol switch), the base URL input started with the protocol's default URL pre-filled. Users who wanted a non-default endpoint had to manually clear the field first. Switch to placeholder semantics: the input starts empty, the default URL is shown as a hint, and submitting blank falls back to that default (then writes it back to baseUrl so downstream steps see a real value). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The old description ("Configure authentication information for login")
implied a Qwen-account login. After the /auth refactor it's really
about picking an LLM provider and entering credentials, so the menu
entry should say that.
Also add 'connect' as an alt-name alongside the existing 'login' so
users can type /connect when 'auth' feels wrong. Keep 'login' for
muscle-memory compatibility.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Strict-parity locales (zh, zh-TW) require every built-in command description to be translated; the renamed /auth description was falling back to English and breaking the must-translate test. Add translations for zh / zh-TW (required) and refresh the other seven locales (en, ru, de, ja, fr, ca, pt) so the old "Configure authentication information for login" key is removed everywhere rather than left as a dangling dictionary entry. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
pomelo-nwu
changed the title
wenshao
requested changes
May 18, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] packages/core/src/providers/install.ts:40 — The isSameModelIdentity fallback merge path in applyModelProvidersPatch (reached when patch.ownsModel is not provided) has zero test coverage. Every test in __tests__/install.test.ts explicitly provides ownsModel. The type allows omitting it, so a future caller would exercise an untested branch with subtle baseUrl normalization. Consider adding a test that omits ownsModel on a prepend-and-remove-owned patch and asserts that only models matching both id and baseUrl are removed.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
Critical: applyProviderInstallPlanToFile fired the install plan with `void`, so any rejection (EACCES from persist(), prototype-pollution guard throw, etc.) was silently swallowed and WebViewProvider proceeded to disconnect/reconnect the agent as if the write had succeeded. Make the wrapper `async` and `await` it in the only caller. Tests added: - core/install.test: isSameModelIdentity fallback path (prepend-and-remove-owned with no ownsModel) — verifies models are matched on id+baseUrl, not just id. - vscode/AuthMessageHandler.test: happy-path with a fixed-baseUrl third-party provider, validateApiKey error branch, and BaseUrlOption picker presentation. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
vscode AuthMessageHandler (Critical): - Add the missing protocol-selection step so custom-provider users can pick Anthropic/Gemini instead of being silently locked to OpenAI. - Validate free-form base URL with the same /^https?:\/\// check the CLI uses; reject file:/javascript: schemes. vscode AuthMessageHandler (Suggestion): - Stop filtering separator entries from the provider QuickPick so groups (Alibaba Cloud / Third Party / Custom) actually show as headers instead of a flat list. - Treat a null authInteractiveHandler as an error: surface an authError + cancellation notification instead of silently dropping the user's input. - Call notifyAuthCancelled when validateApiKey rejects so the webview state resets and the user can retry. core/providers/presets/openrouter.ts (Critical): - Replace the substring includes() in ownsModel with a URL-hostname match so paths like https://api.example.com/openrouter.ai/v1 stop being misidentified as OpenRouter models (and getting removed on re-install). vscode/services/settingsWriter.ts (Critical): - stripTrailingCommas() so JSONC files with trailing commas (VSCode's default style) parse instead of silently returning {} and then overwriting the entire settings file. - readSettings() distinguishes ENOENT (return {}) from parse errors (log + rethrow) so a malformed file never gets clobbered. - writeSettings() writes through a temp file + fs.renameSync atomic rename, eliminating the half-written file window on EACCES / disk-full / crash. - setValue() refuses to overwrite a scalar at an intermediate path segment (would have silently destroyed e.g. {"env": "legacy-string"}). core/providers/install.ts (Suggestion): - Move settings.backup?.() inside the try block so a backup failure still triggers the env-rollback path in catch. cli/config/loadedSettingsAdapter.ts (Suggestion): - Add the same UNSAFE_KEY_PARTS guard the vscode adapter has, so __proto__/constructor/prototype segments are rejected before reaching the underlying setNestedPropertySafe walker. Defense in depth: not exploitable today but the utility has no built-in guard. vscode/webview/providers/WebViewProvider.ts (Suggestion): - Hoist buildInstallPlan / applyProviderInstallPlanToFile to static imports (both modules already top-level imported); drops two per-call await import() round-trips. cli/utils/doctorChecks.ts (Suggestion): - Whitespace nit before the comma in the qwen-code-core import. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
wenshao
requested changes
May 19, 2026
Critical: - settingsWriter: stripTrailingCommas now uses a char-by-char scanner so literal ",]" inside a string value is preserved (the previous regex silently corrupted it). - install.ts: wrap settings.restore() in try/catch so a restore failure doesn't mask the original error or skip the env-rollback loop. - install.ts: snapshot the runtime ModelProvidersConfig before applying patches and reload it in the catch path, so an in-flight refreshAuth() failure doesn't leave the live session holding providers that were never successfully installed. - AuthMessageHandler: custom-provider Base URL is now a placeholder instead of a pre-filled value, with the default selected by the user's chosen protocol (openai/anthropic/gemini). Empty input falls back to the protocol-appropriate URL, preventing the pick-Anthropic-but-keep-OpenAI-URL footgun. Suggestion: - AuthDialog: replace the isCurrentlyCodingPlan misnomer with a uiGroup check — resolveMetadataKey returns config.id for *any* provider with a static models[], so the old guard made DeepSeek/MiniMax/OpenRouter users land on the Alibaba tab instead of Third-party Providers. - AuthMessageHandler: guard against modelIds being [] after splitting comma input (matches the CLI's "Model IDs cannot be empty."). - WebViewProvider: restore the explanatory comment for the authState === true success-toast guard that the previous diff accidentally dropped. Tests: - settingsWriter.test: new applyProviderInstallPlanToFile suite covering happy path, prototype-pollution guard (built via Object.defineProperty to bypass __proto__ literal semantics), intermediate-scalar rejection, malformed-file no-clobber, JSONC-with-trailing-commas parsing (including a string containing ",]"), and the atomic-write tmp-file cleanup. - loadedSettingsAdapter.test: new file — forwarding, UNSAFE_KEY_PARTS rejection, getValue against merged settings, backup/restore round-trip, cleanupBackup semantics. - provider-config.test: added findProviderByCredentials and getAllProviderBaseUrls coverage (preset hits, unknown-key misses, BaseUrlOption[] preset expansion). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
CI's `tsc --build` (with emit) enforced two strict checks that
`tsc --noEmit` had been letting through:
- `noPropertyAccessFromIndexSignature` flagged `file.settings['env']`
reads against `Record<string, unknown>`. Switched the test fixture
shape to a named `SettingsShape` interface with explicit `env` and
`modelProviders` keys (plus an index signature for setValue's
arbitrary writes), so dot access on the known keys is no longer
"through" the index signature.
- Calling optional methods via `adapter.backup?.()` produced TS2722
(`Cannot invoke an object which is possibly 'undefined'`) under the
build flags. createLoadedSettingsAdapter always installs
backup/restore/cleanupBackup, so the tests now assert
`toBeTypeOf('function')` first and then call via non-null assertion,
which both documents the invariant and makes the call typesafe.
- Dropped the `({} as Record<string, unknown>)['polluted']` sanity
check; `expect(setValue).not.toHaveBeenCalled()` already proves the
guard short-circuits before any write reaches LoadedSettings.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…test CodeQL flagged the mock setValue's recursive property assignment as a prototype-pollution sink. Add UNSAFE_KEY_PARTS check at the top of the mock to align with the real setNestedPropertySafe contract. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…iser CodeQL re-flagged the mock setValue write even after the Set.has guard added in 2e6adf8 — the scanner only recognises inline literal === comparisons as prototype-pollution sanitisers, not Set lookups. Reworked the mock to (1) merge the guard into the loop so every current[part] write is preceded by a literal === check against '__proto__'/'constructor'/'prototype', and (2) collapse the dual leaf/branch logic into a single loop body. Runtime behaviour is identical; CodeQL should now treat the write as sanitised. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
wenshao
requested changes
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (outside diff hunks)
[Critical] resolveBaseUrl crashes on empty baseUrl array — provider-config.ts:261: config.baseUrl[0].url crashes with TypeError when config.baseUrl is []. Any future provider or edge case with an empty array triggers an unhandled crash during install. Fix: guard config.baseUrl.length === 0 and return selectedBaseUrl ?? ''.
[Critical] providerMatchesCredentials never matches custom provider — provider-config.ts:310: returns false when config.envKey is not a string. Custom provider has envKey: generateCustomEnvKey (a function). findProviderByCredentials never matches it, making custom provider users invisible to /doctor and system info diagnostics.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
Critical: - useAuth: handleProviderSubmit now calls setPendingAuthType at the start of the try, so handleAuthFailure can record the AuthEvent telemetry on applyProviderInstallPlan rejection (previously dropped silently because pendingAuthType was undefined). - settingsWriter: readQwenSettingsForVSCode wraps readSettings in try/catch so a malformed settings.json no longer crashes the VSCode extension on activation; the write paths (writeCodingPlanConfig, writeModelProvidersConfig) deliberately keep propagating to avoid silently overwriting a corrupt file with partial data. Suggestions: - settingsWriter.setValue: intermediate-segment guard now also rejects arrays (typeof [] === 'object' previously slipped through and would let us set string keys on an array). Loop restructured so the literal-=== prototype-pollution guard runs at every step, satisfying CodeQL's sanitiser detector on both the leaf and intermediate writes. - settingsWriter atomic write: SETTINGS_FILE_MODE = 0o600 + SETTINGS_DIR_MODE = 0o700 + best-effort chmod on existing files. API keys persisted into env.* are no longer world-readable on multi-user systems. - loadedSettingsAdapter: switched its prototype-pollution guard to the same inline literal === pattern so the two adapters stay symmetric and CodeQL recognises both as sanitisers (Comment 6 — explicit 'keep in sync' comment + same shape rather than a shared helper that CodeQL wouldn't trace through). - AuthMessageHandler: protocol QuickPick now shows 'OpenAI Compatible' / 'Anthropic' / 'Gemini' instead of the raw AuthType enum values. - WebViewProvider: authInteractive log now records only the parsed hostname, not the full inputs.baseUrl, so credentials embedded in userinfo or query strings don't leak into extension-host logs. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…n + useAuth failure path Addresses the missing-coverage points in the latest review pass: every deliberately-engineered rollback path in install.ts and the visible side effects of handleAuthFailure now have a regression test, so a future refactor that 'simplifies' these paths can't silently break them. applyProviderInstallPlan (install.test.ts, +4 cases): - restores runtime model providers when refreshAuth rejects after reloadModelProviders ran (asserts the second reloadModelProviders call receives the pre-install snapshot). - still rolls back env vars when backup() throws before persist (pins the 'backup inside try' invariant added in 38a214d). - continues env rollback even when settings.restore itself throws (pins the nested try/catch around restore added in 38a214d). - continues throw + env rollback when the rollback-time reloadModelProviders itself throws (the original error must still surface; env vars must still revert). useAuth (useAuth.test.ts, +1 case): - surfaces install-plan rejection as an auth error and records telemetry — refreshAuth throws, the test asserts authError is set, the dialog reopens, isAuthenticating clears, no success toast is added, and pendingAuthType is populated (which is what the new setPendingAuthType call lets handleAuthFailure key the AuthEvent on). - createSettings now mocks recomputeMerged + forScope.settings so the loaded-settings-adapter restore() path doesn't emit a noisy stderr. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Critical:
- settingsWriter JSONC scanner: \uXXXX is a 6-char escape, not 2.
The previous stripJsonComments / stripTrailingCommas used j+=2 for
every backslash, so a value containing \u0022 would let the embedded
quote terminate the string early — turning a single string value into
multiple top-level keys after the strip passes. That's a parser
differential vs JSON.parse and enables settings.json key injection
(e.g. an attacker-controlled API_KEY string could inject env.NODE_OPTIONS).
Now we branch on text[j+1] === 'u' and skip 6, satisfying both scanners.
- resolveBaseUrl no longer crashes on an empty baseUrl array. The
previous config.baseUrl[0].url threw 'Cannot read undefined.url' on []
and brought down the whole install flow. Falls back to selectedBaseUrl
or '' instead.
- providerMatchesCredentials now resolves function-typed envKey by
calling it with (protocol, baseUrl). The previous typeof-string gate
made the custom provider invisible to findProviderByCredentials —
/doctor and system-info diagnostics couldn't see custom-provider users.
Catches the function call so a misbehaving custom envKey can't crash
the matcher.
Suggestions:
- AuthDialog: defaultMainIndex now also returns 2 for uiGroup === 'custom'
so a custom-provider user lands on the Custom Provider tab instead of
Alibaba ModelStudio.
- install.ts: env-var rollback loop is now wrapped in try/catch matching
the same shape as the settings.restore() and reloadModelProviders
rollbacks. A process.env write throwing (custom property descriptors,
some sandboxes) won't skip the runtime-providers rollback below.
- readSettings: SyntaxError is now wrapped in an actionable Error
('Cannot parse ~/.qwen/settings.json ($name: $message). Standard
JSONC is supported... Please fix or delete $path...') so users facing
a corrupt file get a clear message instead of a bare SyntaxError. The
cause is preserved via Error.cause.
Tests:
- settingsWriter: new \u0022 injection regression — asserts that a
string containing \u0022 stays a single string and no injected key
lands at the top level.
- provider-config: new edge-case suite for resolveBaseUrl with [] and
providerMatchesCredentials with function-typed envKey (matching path,
wrong-key path, function-throws path). Re-imports via the relative
source path so the new behaviour is exercised even before dist/ is
rebuilt.
Not addressed:
- handleProviderSubmit error-path test (Comment 3264567491) was already
added in 7d8b478 — same test, same surface (refreshAuth rejection
+ authError set + dialog reopen + isAuthenticating false + no success
toast + pendingAuthType populated).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Re. the Additional findings (outside diff hunks) in your latest review — both addressed in 93199a4:
|
AuthMessageHandler now references AuthType.USE_OPENAI etc. as enum values (for the protocolLabels map added in cdc17cb), but the import was 'import type AuthType' which strips the runtime binding. TS1361 fired in CI's emitting build even though --noEmit was happy locally. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
审查总结 — PR #4287
架构方向正确，大规模精简（+1650/-5161），但存在若干需关注的问题。
Critical（需合并前修复）
-
Custom provider 对
findProviderByCredentials不可见 —provider-config.ts中providerMatchesCredentials在config.envKey为 function 或baseUrl为 undefined 时立即返回 false。Custom provider 使用generateCustomEnvKey（function 类型），永远无法被匹配。影响systemInfoFields.ts（provider 标签缺失）、doctorChecks.ts、AuthDialog.tsx（defaultMainIndex错误）。 -
Install 流程冗余磁盘写入 —
LoadedSettings.setValue()每次调用都saveSettings()写盘。典型安装触发 5+ 次独立写入 +computeMergedSettings()重算。loadedSettingsAdapter.persist()是空操作但被调用，架构不一致。 -
useProviderSetupFlow.ts（518 行）零测试覆盖 —/auth核心状态机无测试文件。 -
modelscope.test.ts迁移丢失 — 60 行 / 4 个 test case 从 CLI 删除后未在 core 包重建。 -
openrouter.tsownsModel仅检查 hostname — 不验证envKey，用户自定义 model 可能被错误覆盖。 -
Model 前缀迁移风险 —
useProviderUpdates.ts中getInstalledOwnedModelIds使用当前命名前缀，未来更改将产生重复 model。 -
32 个 tsc TS4111 错误 —
settingsWriter.ts和WebViewProvider.ts中 index signature 属性访问使用.prop而非['prop']。
Suggestion
resolveBaseUrl对空 baseUrl 数组无 null guarduseAuth.tsauth 失败遥测静默丢失（setPendingAuthType永远 undefined）useProviderSetupFlow.tsimport 顺序违规settingsWriter.tswriteModelProvidersConfig死代码loadedSettingsAdapter.tspersist()空操作架构不一致loadedSettingsAdapter.tsrestore()忽略restoreSettingsFromBackup()返回值useProviderSetupFlow.tsmodalityAudio默认 false 未文档化useDialogClose.ts无测试（143 行，11 种 dialog 类型）install.tsreloadModelProviders同步 throw 路径未测试
Verdict: Request Changes
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Two findings from the latest /review pass that survived earlier rounds: 1. modelscope.test.ts was deleted in the move-from-CLI step (60 lines / 4 cases under packages/cli/src/auth/providers/thirdParty/) but never recreated in core's preset test folder. Re-added a 3-case suite (config shape, install plan with per-model metadata for known IDs, graceful fallback for unknown IDs) so the third-party preset coverage is symmetric again. Also exported modelscopeProvider from packages/core/src/providers/index.ts so the public API matches the other presets. 2. openrouter.ts ownsModel previously claimed any model on an openrouter.ai hostname, which would silently delete a user's hand-added entry that happened to route through openrouter.ai under a different envKey (e.g. a personal gateway). Now requires both model.envKey === OPENROUTER_ENV_KEY AND the openrouter.ai hostname match. Existing openrouter.test.ts updated and extended to cover: matching path, envKey mismatch path, host mismatch path, missing/malformed baseUrl. The remaining findings in that /review were either already addressed in earlier rounds (custom provider visibility / resolveBaseUrl empty array / useAuth telemetry / TS4111 errors — verified 0 locally) or architectural concerns beyond this PR's scope (LoadedSettings.setValue's per-call saveSettings). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@wenshao 谢谢这一轮 deepseek-v4-pro 的总结审查。我按 已修 / 仍需修 / PR 范围外 三类逐条回复： ✅ 已在更早 commits 修复（review 看到的是 stale 状态）
✅ 本轮 commit
|
| Review 点 | 理由 |
|---|---|
| Install 流程冗余磁盘写入 | LoadedSettings.setValue 的既有行为（CLI 侧本来就如此），属跨 PR 的架构改造。loadedSettingsAdapter.persist() 是空操作正是为了 配合 这个既有行为避免双写。可在后续 PR 引入 batch-set / lazy-persist 模式 |
useProviderSetupFlow.ts 整体测试覆盖（518 行） |
独立工作量；本 PR 已 88 文件 +1.6k/-5.2k，开测试套不利于审查。建议 follow-up issue |
useDialogClose.ts 测试（143 行 11 种 dialog） |
与本 PR 无 diff 关联，同样建议 follow-up |
| 模型前缀迁移风险 | speculative future risk；目前无改前缀计划，且 ownsModel 已经在做去重控制 |
writeModelProvidersConfig 死代码、modalityAudio 文档化、restore() 返回值忽略 |
Minor，建议通过后单独清理（避免再扩大 diff） |
整体仍欢迎继续 push back；critical / 阻塞性的我立刻修。
wenshao
requested changes
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] generateCustomEnvKey in custom-provider.ts:24 — URL normalization replaces all non-alphanumeric characters with _, then collapses. api.example.com, api-example.com, and api_example.com all normalize to API_EXAMPLE_COM, producing identical env keys. Two structurally different custom provider URLs would collide and overwrite each other's API key. Consider appending a short hash of the raw URL to disambiguate.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
Critical: - provider-config.ts providerMatchesCredentials: iterate config.protocolOptions when resolving a function-typed envKey instead of relying on the default config.protocol. A custom provider configured under USE_ANTHROPIC or USE_GEMINI persists an envKey derived from THAT protocol, not from USE_OPENAI — without iteration the matcher silently misses them and custom-provider users disappear from /doctor + AppHeader + systemInfoFields + AuthDialog.defaultMainIndex. - provider-config.test.ts: the existing test asserting 'returns false for function-typed envKey' was holding on the old broken behaviour. Flipped to assert toBe(true) for the matching path, and routed it through the relative source import so it doesn't run against stale dist. Suggestions: - settingsWriter.clearPersistedAuth: now wipes every preset's string envKey (iterates ALL_PROVIDERS, plus the existing subscription-plan loop kept for explicitness) and every QWEN_CUSTOM_API_KEY_* key by prefix match. Previously DeepSeek / MiniMax / Z.AI / IdeaLab / ModelScope / OpenRouter / custom keys lingered on disk after clearing auth. - custom-provider.ts generateCustomEnvKey: the readable-only normalization collapsed 'api.example.com', 'api-example.com', and 'api_example.com' into the same env key, so two structurally different custom providers would overwrite each other's API key. Now appends a 6-hex-char SHA-256 suffix derived from (protocol, baseUrl-with-trailing-slash-stripped). The trailing-slash invariant from the prior implementation is preserved (api/v1 and api/v1/ still hash equal). Suffix collision probability at 6 hex chars is ~1/16M per pair — fine for an interactive flow. Tests: - provider-config.test.ts: added a 'iterates protocolOptions' case that configures a custom-style provider, derives the key under USE_ANTHROPIC, and asserts the matcher finds it. - custom-provider.test.ts: regex-matches the new readable+hash format for the deterministic / special-character / empty-string cases, and a new 'disambiguates structurally distinct URLs that normalize identically' case that pins down the collision fix (api.example.com vs api-example.com vs api_example.com all differ). Not addressed: - TS1361 'type AuthType' import — already fixed in 8f94b01 - modelscope re-export — already fixed in 7228d73 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Re. the generateCustomEnvKey collision finding (the changes-requested summary): fixed in 6b50ca0. The function now appends a 6-hex-char SHA-256 suffix derived from |
CodeQL alerts 225 + 232 flagged `/_+/g`, `/^_+|_+$/g`, and `/\/+$/` in generateCustomEnvKey as polynomial regex on user input. V8 handles these patterns linearly in practice, but the scanner can't see that and any baseUrl with many '_' or '/' would be flagged as a theoretical worst case. Replaced both passes with single-pass character scans: - normalizeEnvSegment: walks the string once, emits alphanumerics verbatim, collapses any non-alphanumeric run to a single '_', then trims leading/trailing underscores via charCodeAt index walks. Equivalent to the prior three regexes but with no quantifier backtracking surface. - stripTrailingSlashes: walks backwards from the end while charCodeAt === 47, then slices. Equivalent to `replace(/\/+$/, '')`. All 11 custom-provider tests still pass — output format and invariants (trailing-slash equivalence, hash suffix, protocol/URL disambiguation) are unchanged. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
Critical: - i18n: 9 locale files updated to replace orphaned 'Select Authentication Method' / 'You must select an auth method...' keys with the new 'Connect a Provider' / 'You must connect a provider...' keys the AuthDialog actually references. Non-English users no longer see the English fallback for the main heading + exit-prevention warning. - settingsWriter.writeSettings: renameSync is now wrapped in try/catch that unlinks the temp file on failure (EPERM/EBUSY on Windows from watchers/AV would otherwise orphan a secret-bearing .tmp file in ~/.qwen on every failed write). - settingsWriter.restore(): write to disk FIRST, then update in-memory data. The previous order left memory clean while disk retained the failed install's partial state if writeSettings threw. Now matches the CLI adapter's order. - AuthMessageHandler custom-provider tests: added 4 cases covering protocol picker → free-form URL → API key → comma-split model IDs → advanced config (one happy path), plus the http(s) scheme guard, the protocol-aware blank-URL fallback, and the whitespace-only model IDs guard. Previously the entire custom path through runProviderSetupFlow had zero coverage. - settingsWriter clearPersistedAuth tests: added cases for the expanded preset/custom/subscription cleanup (asserts NODE_OPTIONS survives, every QWEN_CUSTOM_API_KEY_* is wiped, providerMetadata entries for every preset are gone) plus a no-settings-file no-op. Suggestions: - loadedSettingsAdapter.restore(): now checks restoreSettingsFromBackup's boolean return value and logs an explicit warning when on-disk rollback fails (EACCES / missing .orig). Previously the failure was silent and the next CLI restart would read a corrupted file. - generateCustomEnvKey: hash suffix lengthened from 6 → 12 hex chars (24 → 48 bits). Brings collision search out of milliseconds-range enumeration; offline 'pick a URL that collides' attack is no longer practical at interactive setup time. - getDefaultBaseUrlForProtocol: new shared helper in core consumed by both the CLI (useProviderSetupFlow) and VS Code (AuthMessageHandler) flows. Removes the duplicated DEFAULT_BASE_URLS map; one source of truth for the OpenAI/Anthropic/Gemini placeholder URLs. - settingsWriter.clearPersistedAuth: providerMetadata cleanup now iterates ALL_PROVIDERS with resolveMetadataKey instead of hardcoding coding-plan/token-plan. Stale metadata for deepseek/minimax/zai/ idealab/modelscope/openrouter no longer lingers after logout. - resolveMetadataKey: explicit guard against provider ids containing '.'. A dotted id would split into multiple nested objects under providerMetadata, silently corrupting the settings tree. Now throws loudly at registration time. - customProvider: added explicit ownsModel that prefix-matches against QWEN_CUSTOM_API_KEY_*. Reinstalling a custom provider under a different baseUrl now reliably replaces (not accumulates) the old entries. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously approved these changes
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
reviewed
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (terminal-only — no diff-line mapping):
- Test coverage gaps (Suggestion): Three new code paths lack tests: (1)
loadedSettingsAdapter.restore()return-value check forrestoreSettingsFromBackup— no test assertsconsole.erroris called when restore fails; (2)settingsWriter.writeSettingsrenameSyncerror path with temp-file cleanup — no test mocksrenameSyncto throw; (3)resolveMetadataKeydot validation — no test asserts the throw forconfig.id.includes('.'). - Stale comment (Nice to have):
custom-provider.ts:19still reads "A 6-hex-char suffix" while the code and the same paragraph use 12 chars. - Missing error details (Nice to have):
loadedSettingsAdapter.ts:85logs thatrestoreSettingsFromBackupfailed but not why —restoreSettingsFromBackupswallows the error in acatch(_e)and returnsfalse.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 19, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: manual review environment could not run bundled presubmit; submitting as comment instead of approval. — gpt-5.5 via Qwen Code /review
wenshao
reviewed
May 19, 2026
Suggestions: - clearPersistedAuth metadata cleanup loop: per-iteration try/catch around resolveMetadataKey so a future dotted-id provider can't abort the loop and leave secrets on disk. - VS Code AuthMessageHandler: removed the hardcoded || 'https://api.openai.com/v1' fallback after getDefaultBaseUrlForProtocol — defaults must live in core. The CLI flow has no such fallback, and the silent OpenAI default would mask a new AuthType core hadn't been taught about. - settingsWriter restore() comment: clarified the deliberate divergence from the CLI adapter's trade-off (disk-fail-throws here, disk-fail- logs-and-continues there) so the comment doesn't read 'same order'. - useAuth handleAuthFailure: closure staleness — setPendingAuthType queues an async React update, so handleAuthFailure's pendingAuthType read could see undefined when a synchronous throw beats the next render. Added an optional protocolForTelemetry argument that the new handleProviderSubmit passes explicitly; closure fallback kept for legacy callers. AuthEvent error telemetry is no longer silently dropped. - install.ts: track currentStep before each phase (backup → env → modelProviders → authType → legacyCredentials → modelSelection → providerState → persist → reloadModelProviders → syncAuthState → refreshAuth → cleanupBackup) and annotate the rethrown error with the failing step + authType. Original error preserved via Error.cause so callers matching on err.code still work. - custom-provider.ts: stale '6-hex-char' comment updated to 12. Added a migration note explaining that old 6-char keys persist as harmless orphan disk state until clear-auth. - settingsUtils.restoreSettingsFromBackup: was swallowing fs errors with catch(_e); now logs the underlying cause so the adapter's on-disk-rollback-failed warning has something specific to point at. Tests: - useAuth: new cancelAuthentication case asserts isAuthenticating clears, externalAuthState clears, dialog opens, authError clears. - provider-config: new resolveMetadataKey suite — normal id, no-models → undefined, dotted id → throws. - install: new case asserting the rethrown error names the failing step ('refreshAuth') + authType and preserves the original error via Error.cause. Not addressed: - 6→12 hash backward compat (Comment 3267562667): The 6-char keys are orphan disk state — never read by applyProviderInstallPlan (the new model provider entries reference the new 12-char key), so no security or correctness issue, just disk noise that clears on next sign-out. Documented in custom-provider.ts. A full clean-up pass would need a new ProviderSettingsAdapter delete API + a migration scan — better as its own PR. - writeSettings renameSync error path test + loadedSettingsAdapter restore-failure log test (terminal-only findings): adding these requires fs mocking surgery that's worth its own PR. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Re. the Additional findings (terminal-only) in the latest review pass — partially addressed in Done:
Deferred:
Also FYI: the 6→12 hash backward-compat concern (Comment |
wenshao
requested changes
May 19, 2026
All four are Critical-tagged formatter / docs issues caught by the latest /review pass:
- AppHeader.tsx: `AuthType ,` (stray space before comma) → standard newline-after-{ form. Was breaking CI Lint.
- useProviderUpdates.test.ts: same `AuthType ,` pattern → standard form.
- apiPreconnect.ts: double blank line after the closing `}` of the
import block (left behind when getAllProviderBaseUrls was removed
from the old auth/allProviders path) → single blank line.
- types.ts (Suggestion): JSDoc for `modelsEditable` said
"false → skip model step; use models as-is (e.g. Coding Plan)" but
codingPlanProvider actually sets modelsEditable: true (every preset
in the registry does), so the example contradicts the registry.
Dropped the parenthetical.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Windows CI flaked on `standalone release packaging > rejects unexpected dist assets` with a 5000ms timeout. The test shells out to `node scripts/create-standalone-package.js` which produces a tar.gz; observed real runtimes from sibling tests in the same run: 4780ms / 1666ms / 1079ms — the 4.8s case is already at vitest's default 5s limit, so a slightly slower subprocess startup (antivirus inspection, contended runner) tips it over. Pre-existing test (added 2026-05-11 in cb7059f), unrelated to this PR's auth refactor. Bumped the suite-wide testTimeout to 30s in scripts/tests/vitest.config.ts — the tests still complete in seconds when subprocess startup is healthy; the headroom only kicks in to cover Windows-slow variance. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
requested changes
May 19, 2026
wenshao
reviewed
May 20, 2026
Critical:
- WebViewProvider.handleAuthInteractive: roll back bad credentials when the
agent reconnect rejects them. applyProviderInstallPlanToFile commits the
key + calls cleanupBackup before the disconnect/reconnect runs, so the
plan's own rollback can't cover an authState=false outcome. Now snapshot
settings before the write (snapshotSettingsForRollback) and restore it
(restoreSettingsSnapshot) on both the authState!=true branch and the
catch branch. Without this a rejected key persisted and every VS Code
restart retried it. Two new helpers added to settingsWriter; never-throw
snapshot so a malformed pre-state degrades to a no-op restore.
Suggestions:
- AuthMessageHandler: trim the API key before validateApiKey + persistence,
matching the CLI flow (useProviderSetupFlow trims in two places). A key
pasted with trailing whitespace no longer causes silent auth failures or
VS-Code-only validateApiKey rejections.
- install.ts: the annotated rethrow no longer bakes 'step "persist"' into
the user-facing message. Step + authType are now structured properties on
a new exported ProviderInstallError (message stays the underlying error
text, cause preserved). Callers can show a clean message and log
err.step/err.authType to the dev console.
- provider-config.ts: providerMatchesCredentials no longer swallows a throw
from a function-typed envKey — console.warn surfaces the programming
error so a custom provider silently vanishing from /doctor has a trace.
- types.ts: documented that ProviderSettingsAdapter.setValue MAY flush to
disk eagerly (the CLI LoadedSettings adapter does) and that persist() can
be a no-op for such adapters — so future authors don't insert pre-persist
steps assuming atomicity.
- settingsWriter: moved the orphaned stripJsonComments JSDoc off
jsonEscapeLength (the \u-escape helper inserted between the doc and its
function) back onto stripJsonComments itself.
Tests:
- settingsWriter: snapshot/restore round-trip, malformed→null→no-op-restore,
no-file→{} snapshot.
- install: updated the step-annotation test to assert err.step/err.authType
structured properties + clean message instead of the embedded string.
- WebViewProvider.test: settingsWriter mock extended with
applyProviderInstallPlanToFile/snapshotSettingsForRollback/
restoreSettingsSnapshot.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
#4108 (drop OpenRouter OAuth) gave us a chance to fix the
/authsurface in one pass. Four kinds of accumulated complexity were ready to come out together:/authand/manage-modelsboth edited the same provider config. The 1.2k-lineManageModelsDialogonly existed because/authStep 2 used to be inadequate at model selection — it isn't anymore.'input'authMethodfield.applyProviderInstallPlan. Drift had already started: CLI had env-var rollback, VS Code silently didn't. One implementation is enough.alibaba-standard.tswas prefixed; sibling Alibaba presets weren't.The mental model we want:
/authis "Connect a Provider". Pick one, type a key, pick a model. That's it.What changed
Five focused refactor commits, each removable independently:
44f77d0arefactor(providers): unify provider config into corecli/src/authintocore/src/providers. CLI re-exports gone.423e4530refactor(cli): drop OpenRouter OAuth + /manage-models, simplify /auth/manage-models(command + 1.2k-line dialog), drops theOAUTHmain menu entry. Menu is now exactly 3 options: Alibaba ModelStudio / Third-party Providers / Custom Provider.6804a18frefactor(auth): unify applyProviderInstallPlan in coreProviderSettingsAdapter. CLI's env-rollback now applies to VS Code too.packages/cli/src/auth/is gone entirely.94319666refactor(providers): drop unused authMethod fieldb4aa4b4frefactor(providers): prefix Alibaba plan presets with alibaba-Plus three follow-up fixes surfaced by real-user testing:
c7f01627fix(vscode): guard ProviderSettingsAdapter against prototype pollution0d8fe738fix(auth): default Audio modality off in provider advanced configed08f729fix(auth): show base URL default as placeholder, not prefilled valueMerge with main (121 commits) was clean except for porting #4150 (modelscope provider) into
packages/core/src/providers/presets/modelscope.ts.Total: 88 files, +1,405 / −5,155.
Reviewer focus
If you only have 10 minutes, read these three files in order:
packages/core/src/providers/install.ts— the single source of truth for install/uninstall. Note env-rollback (previously CLI-only) is now folded in so VS Code benefits.packages/cli/src/config/loadedSettingsAdapter.ts— new ~80-line adapter mappingLoadedSettingsto the coreProviderSettingsAdaptercontract. Preserves on-disk.origbackup, in-memorystructuredClonesnapshot, andrecomputeMergedon restore.packages/cli/src/ui/auth/AuthDialog.tsx+useAuth.ts— the action-surface trim:handleAuthSelectis split into single-purposecloseAuthDialog; 5 legacy wrapper handlers are gone.Validation
npm run typecheck— all 4 workspaces cleannpm run lint— clean/authflow with the tmux-real-user-testing skill.Quickest reviewer verification:
npm run dev -- --approval-mode yolo→ type/auth→ walk through Third-party Providers → OpenRouter. You should land onOpenRouter · Step 1/2 · API Keywith no browser tab opening.Evidence:
Risk / breaking changes
OPENROUTER_API_KEYset/manage-modelscommand/auth(re-run setup to swap model IDs) or edit~/.qwen/settings.jsondirectly.ProviderConfig.authMethodfieldProviderConfigneeds to drop the field.AuthController['actions']shapehandleAuthSelect,handleOpenRouterSubmit, and 5 legacy wrappers gone. New entry:closeAuthDialog.handleProviderSubmitis the only programmatic install entry.Not covered
packages/core/src/qwen/qwenOAuth2.tsare intentionally preserved (will be restored to UI in a later PR) even though the OAuth main menu entry is gone.itWhenTuiInputReliableAuthDialog tests time out locally — pre-existing flake from feat(cli): readline Ctrl+P/N for history and selection navigation #4082 (readline selection navigation); they areCI=truegated, so CI is unaffected. Confirmed against backup branch before the merge.Screenshots / Video Demo
Testing Matrix
Validated on macOS (Darwin 25.3) with
npm run devreal-user walkthrough; Windows/Linux not exercised directly. Changes are TS/UI-only with no platform-sensitive code paths.Linked Issues / Bugs
Closes #4108
🤖 Generated with Qwen Code