refactor(auth): unify provider config in core, simplify /auth as "Connect a Provider"#4287
Merged
Merged
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
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>
wenshao
requested changes
May 18, 2026
wenshao
left a comment
Collaborator
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
wenshao
left a comment
Collaborator
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>
wenshao
requested changes
May 20, 2026
wenshao
requested changes
May 20, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings that could not be mapped to a diff line:
[Suggestion] setNestedPropertySafe (settingsUtils.ts) lacks __proto__/constructor/prototype guard. migrateProviderMetadata in useProviderUpdates.ts calls settings.setValue with field values from Object.entries on user-editable settings.json data. JSON.parse preserves __proto__ as an own property, so a crafted settings file could pollute Object.prototype. Consider adding the guard directly inside setNestedPropertySafe rather than relying on every adapter to wrap it.
Critical (both from the previous round's own changes): - WebViewProvider.handleAuthInteractive: restoreSettingsSnapshot → writeSettings can throw (EPERM on Windows renameSync / disk full / EACCES). Both rollback call sites are now routed through a local safeRollback() that try/catches and logs, so a rollback failure can never (a) re-throw out of the else-branch into the outer catch and trigger a second rollback that skips the error message, nor (b) throw out of the catch-branch and leave the webview auth dialog hanging with no feedback. - provider-config.providerMatchesCredentials: the new envKey-throw console.warn logged the full baseUrl, which can embed credentials (https://user:sk-secret@host). Now logs only new URL(baseUrl).hostname (with an [invalid] fallback) and err.message, matching the sanitization WebViewProvider already uses. Tests: - WebViewProvider.test: new 'credential rollback' describe with three cases — (1) authState!==true after reconnect → restoreSettingsSnapshot called with the snapshot, (2) authState===true → restore NOT called, (3) restore throws (EPERM) → handleAuthInteractive still resolves and the authError message is still sent. Hoisted mocks extended with applyProviderInstallPlanToFile / snapshotSettingsForRollback / restoreSettingsSnapshot refs so the scenario is controllable. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Critical:
- AuthMessageHandler: validation-failure paths (bad URL scheme, invalid
API key, empty model IDs, handler-not-set) no longer call
notifyAuthCancelled after sendToWebView({authError}). The webview's
ProviderSetupForm clears the error on authCancelled, so the two
messages raced and the error flashed away before the user could read
it. authCancelled is now reserved for genuine user dismissals (Escape
on a QuickPick/InputBox); authError already clears the connecting state.
- WebViewProvider: after rolling back rejected credentials, also
disconnect the agent. The reconnect spawned a process holding the bad
key in memory; without disconnect a subsequent chat message hit a
stale-credential error unrelated to the original auth failure. Now
agentManager.disconnect() + agentInitialized=false so the next /auth
reconnects cleanly.
Suggestions:
- install.ts: added a DENY_ENV_KEYS denylist (NODE_OPTIONS, NODE_PATH,
LD_PRELOAD, LD_LIBRARY_PATH, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH,
PATH, HOME, TMPDIR), checked case-insensitively before writing any
plan.env entry to settings + process.env. Defense in depth: all callers
go through buildInstallPlan with hardcoded keys today, but
ProviderInstallPlan is exported.
- settingsUtils: setNestedPropertySafe AND setNestedPropertyForce now
refuse __proto__/constructor/prototype path segments (inline literal
=== so CodeQL recognises the sanitiser). migrateProviderMetadata feeds
field names from Object.entries on user settings.json, and JSON.parse
keeps __proto__ as an own property — guarding at the utility protects
every caller, not just the adapters.
Already fixed in f31224b (review ran against 9f45a75):
- restoreSettingsSnapshot throw masking the original error → safeRollback.
- baseUrl logged verbatim in providerMatchesCredentials → hostname only.
Tests:
- install: NODE_OPTIONS rejected + not leaked to process.env/settings;
case-insensitive Path rejection.
- AuthMessageHandler: validation authError is NOT followed by
authCancelled.
- WebViewProvider: rollback path disconnects the agent + clears
agentInitialized.
- settingsUtils: setNestedPropertySafe/Force refuse __proto__/
constructor/prototype and don't pollute Object.prototype.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Re. the Additional finding (setNestedPropertySafe lacks a prototype-pollution guard): fixed in c6316d5. Added a |
This was referenced May 20, 2026
The new setNestedProperty guard tests asserted obj.a.b.c / obj.x.y dot-access on Record<string, unknown>, which trips noPropertyAccessFromIndexSignature (TS4111) under the emitting tsc --build the CI 'Install dependencies' step runs. Local npm run typecheck (--noEmit) had a stale tsbuildinfo and didn't re-check the file. Switched to bracket access (obj['a']['b']['c']) to match the strict option. Behaviour unchanged; 78 settingsUtils tests still pass. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 20, 2026
…ctive All prior rollback tests exercised the else-branch (authState !== true). The outer catch — reached when applyProviderInstallPlanToFile or doInitializeAgentConnection throws (disk errors, partial writes) — had no coverage, and that's the higher-risk path. New test makes doInitializeAgentConnection reject and asserts (1) restoreSettingsSnapshot called with the snapshot, (2) authError sent containing 'Configuration failed', (3) handleAuthInteractive resolves without throwing. Guards against a regression that drops the safeRollback wrapper in the catch. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 20, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI is still pending (Test jobs on macOS, Ubuntu, and Windows are in progress). — gpt-5.5 via Qwen Code /review
The test asserted process.env.NODE_OPTIONS toBeUndefined after the rejected plan, but CI sets NODE_OPTIONS (--max-old-space-size=3072 from the build script), so it failed there while passing locally where NODE_OPTIONS is unset. Snapshot the original value and assert the rejected plan left it UNCHANGED (and specifically not the evil --require value) — that's the actual invariant: the denylist throws before mutating process.env. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 20, 2026
…ock too The else-branch (authState !== true) disconnected the agent after rollback, but the outer catch only rolled back. If doInitializeAgentConnection partially initializes (agentInitialized=true, agent process spawned) then throws — e.g. a disk error during post-connect setup — the stale-credential agent stayed connected. Extracted a disconnectStaleAgent() local helper (alongside safeRollback) and called it in both the else-branch and the catch, so the two paths are symmetric. Extended the outer-catch test to spawn a partial agent before the throw and assert disconnect() is called + agentInitialized cleared. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 20, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI is still pending (CodeQL, Lint, Test on macOS/Ubuntu/Windows). — qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
reviewed
May 20, 2026
All from DeepSeek's pass, all on recent commits:
- settingsUtils: stale comment referenced a non-existent UNSAFE_PATH_SEGMENTS const; the actual guard is pathHasUnsafeSegment(). Fixed both comment sites.
- settingsWriter.snapshotSettingsForRollback: was silently returning null on a readSettings throw (disabling credential rollback with no signal). Now console.warn's the cause so oncall can tie repeated cross-restart auth failures back to a transient unreadable settings file.
- provider-config.providerMatchesCredentials: the envKey-throw warn logged err.message, which a user-defined envKey fn could populate with the API key (new Error(`bad config: ${apiKey}`)). Now logs only err.constructor.name — no message, no URL.
- install.ProviderInstallError: was an interface (erased at compile time → instanceof always false). Converted to a class extending Error so instanceof works at runtime; exported as a value (not type) from the barrel. Construction simplified to new ProviderInstallError(msg, step, authType, { cause }).
- install.DENY_ENV_KEYS: added Windows TMP/TEMP alongside TMPDIR so a crafted plan can't redirect temp-file creation on Windows.
Tests:
- install: assert the thrown error is instanceof ProviderInstallError; new it.each covering TMP/TEMP/tmp rejection (case-insensitive).
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
previously approved these changes
May 20, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified locally on macOS arm64, Node v22.17.0, against worktree HEAD 54a3309ad.
Type & build: npm run typecheck clean across all 4 workspaces (core / cli / sdk / webui). Built core, channel-base, 4 channel adapters, web-templates, and cli — all pass.
Targeted unit tests — 466 pass / 17 skipped / 0 fail under CI=true:
- CLI auth UI + AppContainer + DialogManager + slashCommandProcessor + useProviderUpdates + loadedSettingsAdapter + authCommand + settingsUtils + doctorChecks + BuiltinCommandLoader: 291 / 17
- Core providers (install + 11 preset suites + provider-config): 106 / 0
- VSCode AuthMessageHandler + settingsWriter + WebViewProvider: 69 / 0
The 17 skipped are exactly the itWhenTuiInputReliable flake-gated tests called out in the PR description.
tmux real-user walkthrough (the "quickest reviewer verification" from the PR):
| Step | Expected | Actual |
|---|---|---|
/auth main menu |
"Connect a Provider", 3 options, no OAuth | ✅ Alibaba ModelStudio / Third-party Providers / Custom Provider |
| Third-party Providers → OpenRouter | "Connect with an OpenRouter API key" | ✅ link to openrouter.ai/keys |
| Select OpenRouter | Lands on Step 1/2 · API Key, no browser |
✅ sk-... placeholder, docs link, no OAuth callback |
/manage-models |
Removed | ✅ no autocomplete, "✕ Unknown command" |
| Custom Provider Step 2/6 Base URL | Default as placeholder, not prefilled | ✅ typing a clears the default https://api.openai.com/v1 (validates ed08f729) |
The mental-model trim ("Connect a Provider — pick one, type a key, pick a model") is clearly delivered. Net -3,750 lines with no functional regressions surfaced. Risk areas are well-documented in the PR body. Ship it.
Collaborator
Maintainer Verification ReportReproduced the PR's targeted unit tests + tmux real-user walkthrough end-to-end on a clean worktree. Environment
1. Typecheck
2. BuildBuilt in dependency order on the same worktree: One-time observation: 3. Unit tests — 466 pass / 17 skipped / 0 fail (
|
| PR Claim | Verified by |
|---|---|
| 3-option "Connect a Provider" menu (Commit #2) | tmux step 1 |
| OpenRouter OAuth removed, replaced with API-key path (Commit #2) | tmux step 3 (no browser, Step 1/2 API Key) |
| ~1.5k OAuth code deleted (Commit #2) | Code level: cli/src/auth/providers/oauth/openrouterOAuth* deleted (–1,643 lines) |
/manage-models removed (Commit #2) |
tmux step 4 (Unknown command) |
applyProviderInstallPlan unified into core (Commit #3) |
Core providers tests 106/106 + VSCode AuthMessageHandler 69/69 |
authMethod field dead-coded (Commit #4) |
Core provider-config tests pass with field absent |
| Alibaba preset prefix consistency (Commit #5) | Core preset tests 11/11 pass |
Prototype-pollution guard (c7f01627) |
settingsUtils.test.ts 50 added cases pass |
Audio modality default off (0d8fe738) |
useProviderUpdates / AuthDialog tests pass; not directly clicked through (deep step) |
Base URL placeholder, not prefilled (ed08f729) |
tmux step 5 (typing 'a' clears the default) |
13 itWhenTuiInputReliable AuthDialog flakes are CI-gated |
Reproduced: 13 fail without CI=true, all skip with CI=true |
| Net -3,750 lines refactor with no behavior regressions | 466/466 effective tests pass + UI walkthrough clean |
Verdict
All major PR claims reproduced. The mental-model claim ("/auth is 'Connect a Provider' — pick one, type a key, pick a model") delivered as advertised. Approving.
wenshao
reviewed
May 20, 2026
…ollback Consistency with the err.constructor.name approach applied in provider-config.providerMatchesCredentials. The risk here is lower (the catch is filesystem errors from readSettings/structuredClone, not user-defined functions), but logging only the class name keeps the security stance uniform across the codebase. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
May 20, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (CodeQL, Lint, Test, Post Coverage Comment, Classify PR). — DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
approved these changes
May 20, 2026
LaZzyMan
added a commit
that referenced
this pull request
May 21, 2026
…istoryShallow) Main landed #4286 (replace structuredClone with shallow copy) which: - Reverted #4186's heap-pressure auto-compaction safety net (#4286 removed HEAP_PRESSURE_COMPRESSION_RATIO because the underlying OOM cause was fixed by the shallow-copy refactor) - Reverted #4168's consecutiveFailures ladder back to single-shot hasFailedCompressionAttempt - Introduced getHistoryShallow() / peekLastHistoryEntry() to replace structuredClone-based history access - Added a Chinese-language design doc draft for this exact redesign Resolution strategy: - Take OUR redesign everywhere it conflicts: three-tier threshold ladder, consecutiveFailures circuit breaker, hard-rescue, token estimator, hard-rescue debug log, CompressOptions plumbing for pendingUserMessage / precomputedEffectiveTokens / trigger. - DROP all bypassTokenThreshold / heapPressureCompressionCooldownUntil / HEAP_PRESSURE_* / mockGetHeapStatistics / mockHeapPressure code (heap-pressure mechanism is gone on main; we're not reviving it). - Use main's new getHistoryShallow(true) in chatCompressionService and in the hard-tier rescue estimator path (was getHistory(true) before main's refactor; the shallow path is what other compaction call sites now use). - For chatCompressionService.test.ts inline mockChat objects, alias getHistoryShallow to the same vi.fn() as getHistory so existing .mockReturnValue() calls drive both methods. - For the design doc, keep our resolved Open Question 2 closure rationale and prepend the round-2 blockquote clarifying the Background section describes pre-redesign behavior; take main's slightly more thorough SUMMARY_RESERVE paragraph where it explains both with/without-thinking cases. - Replace the round-2 test that asserted "hard-rescue forwards consecutiveFailures=3" with a test compatible with the post-merge history-access shape (now using getHistoryShallow). 346 core tests passing; CLI typecheck clean for affected files. Pre-existing provider-config typecheck errors from main's #4287 refactor are unrelated to this PR and not touched here.
4 tasks
doudouOUC
added a commit
that referenced
this pull request
May 27, 2026
Auth provider files were removed by #4287 (auth unification) and httpAcpBridge.test.ts was moved to packages/acp-bridge in the F1 test split. These existed in the original orphan branch baseline but were deleted via sync-main commits.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.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
4 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