fix(auth): second round of PR #4287 review fixes

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>

packages/cli/src/config/loadedSettingsAdapter.test.ts

@@ -0,0 +1,148 @@
+/**
+ * @license
+ * Copyright 2025 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { describe, expect, it, vi } from 'vitest';
+import { createLoadedSettingsAdapter } from './loadedSettingsAdapter.js';
+import { SettingScope } from './settings.js';
+
+// settingsUtils makes real fs calls in backup/restore — stub them out so the
+// tests can focus on adapter behavior without touching disk.
+vi.mock('../utils/settingsUtils.js', async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import('../utils/settingsUtils.js')>();
+  return {
+    ...actual,
+    backupSettingsFile: vi.fn(),
+    restoreSettingsFromBackup: vi.fn(),
+    cleanupSettingsBackup: vi.fn(),
+  };
+});
+
+interface MutableSettingsFile {
+  settings: Record<string, unknown>;
+  originalSettings: Record<string, unknown>;
+  path: string;
+}
+
+function makeSettings(initial: Record<string, unknown> = {}) {
+  const file: MutableSettingsFile = {
+    settings: structuredClone(initial),
+    originalSettings: structuredClone(initial),
+    path: '/tmp/qwen-test-settings.json',
+  };
+  const setValue = vi.fn(
+    (_scope: SettingScope, key: string, value: unknown) => {
+      const parts = key.split('.');
+      let current: Record<string, unknown> = file.settings;
+      for (let i = 0; i < parts.length - 1; i++) {
+        const part = parts[i]!;
+        if (!current[part] || typeof current[part] !== 'object') {
+          current[part] = {};
+        }
+        current = current[part] as Record<string, unknown>;
+      }
+      current[parts[parts.length - 1]!] = value;
+      file.originalSettings = structuredClone(file.settings);
+    },
+  );
+  const recomputeMerged = vi.fn(() => {
+    /* merged() is computed lazily via the getter below */
+  });
+  const settings = {
+    get merged() {
+      return file.settings;
+    },
+    forScope: vi.fn(() => file),
+    setValue,
+    recomputeMerged,
+  };
+  return { settings, file, setValue, recomputeMerged };
+}
+
+describe('createLoadedSettingsAdapter', () => {
+  it('forwards setValue to LoadedSettings.setValue with the resolved scope', () => {
+    const { settings, setValue } = makeSettings();
+    const adapter = createLoadedSettingsAdapter(
+      settings as never,
+      SettingScope.User,
+    );
+    adapter.setValue('env.MY_KEY', 'val');
+    expect(setValue).toHaveBeenCalledWith(
+      SettingScope.User,
+      'env.MY_KEY',
+      'val',
+    );
+  });
+
+  it('rejects prototype-pollution keys before reaching LoadedSettings', () => {
+    const { settings, setValue } = makeSettings();
+    const adapter = createLoadedSettingsAdapter(
+      settings as never,
+      SettingScope.User,
+    );
+    expect(() => adapter.setValue('__proto__.polluted', 'x')).toThrow(
+      /reserved segment/,
+    );
+    expect(() => adapter.setValue('foo.constructor.bar', 'x')).toThrow(
+      /reserved segment/,
+    );
+    expect(() => adapter.setValue('prototype.x', 'x')).toThrow(
+      /reserved segment/,
+    );
+    expect(setValue).not.toHaveBeenCalled();
+    expect(({} as Record<string, unknown>).polluted).toBeUndefined();
+  });
+
+  it('getValue reads from settings.merged via dotted key', () => {
+    const { settings } = makeSettings({
+      env: { MY_KEY: 'from-merged' },
+      modelProviders: { openai: [{ id: 'gpt' }] },
+    });
+    const adapter = createLoadedSettingsAdapter(
+      settings as never,
+      SettingScope.User,
+    );
+    expect(adapter.getValue('env.MY_KEY')).toBe('from-merged');
+    expect(adapter.getValue('modelProviders.openai')).toEqual([{ id: 'gpt' }]);
+    expect(adapter.getValue('missing.path')).toBeUndefined();
+  });
+
+  it('backup() snapshots in-memory state; restore() reverts and recomputes merged', async () => {
+    const { settings, file, recomputeMerged } = makeSettings({
+      env: { ORIGINAL: '1' },
+    });
+    const adapter = createLoadedSettingsAdapter(
+      settings as never,
+      SettingScope.User,
+    );
+
+    adapter.backup();
+
+    // Simulate mutations that would happen during an install plan apply.
+    adapter.setValue('env.NEW_KEY', 'new-value');
+    expect(file.settings.env).toEqual({ ORIGINAL: '1', NEW_KEY: 'new-value' });
+
+    adapter.restore();
+
+    expect(file.settings).toEqual({ env: { ORIGINAL: '1' } });
+    expect(file.originalSettings).toEqual({ env: { ORIGINAL: '1' } });
+    expect(recomputeMerged).toHaveBeenCalled();
+  });
+
+  it('cleanupBackup() clears the in-memory snapshot so a later restore is a no-op', () => {
+    const { settings, file } = makeSettings({ env: { K: 'v1' } });
+    const adapter = createLoadedSettingsAdapter(
+      settings as never,
+      SettingScope.User,
+    );
+    adapter.backup();
+    adapter.setValue('env.K', 'v2');
+    adapter.cleanupBackup();
+    // restore after cleanup should not bring v1 back
+    adapter.restore();
+    expect(file.settings.env).toEqual({ K: 'v2' });
+  });
+});

packages/cli/src/ui/auth/AuthDialog.tsx

@@ -22,7 +22,6 @@ import {
   customProvider,
   ALIBABA_PROVIDERS,
   THIRD_PARTY_PROVIDERS,
-  resolveMetadataKey,
   type ProviderConfig,
 } from '@qwen-code/qwen-code-core';
 import { useProviderSetupFlow } from './useProviderSetupFlow.js';
@@ -206,15 +205,15 @@ export function AuthDialog(): React.JSX.Element {
     contentGenConfig?.baseUrl,
     contentGenConfig?.apiKeyEnvKey,
   );
-  const isCurrentlyCodingPlan = !!(
-    matchedProvider && resolveMetadataKey(matchedProvider)
-  );
 
+  // Land on the tab that matches the active provider's uiGroup so a DeepSeek
+  // / MiniMax / OpenRouter user opens Third-party Providers, not Alibaba.
+  // (resolveMetadataKey returns config.id for *any* provider with a static
+  // models[], so it can't be used to detect "Alibaba" specifically.)
   const defaultMainIndex = useMemo(() => {
-    if (isCurrentlyCodingPlan) return 0;
     if (matchedProvider?.uiGroup === 'third-party') return 1;
     return 0;
-  }, [isCurrentlyCodingPlan, matchedProvider]);
+  }, [matchedProvider]);
 
   // -- Handlers -------------------------------------------------------------
 

packages/core/src/providers/__tests__/provider-config.test.ts

@@ -10,6 +10,8 @@ import {
   buildInstallPlan,
   buildProviderTemplate,
   computeModelListVersion,
+  findProviderByCredentials,
+  getAllProviderBaseUrls,
   getDefaultModelIds,
   resolveBaseUrl,
   shouldShowStep,
@@ -481,3 +483,58 @@ describe('buildProviderTemplate', () => {
     expect(template[0]?.name).toBe('[Intl] m');
   });
 });
+
+describe('findProviderByCredentials', () => {
+  it('finds a preset by its env key + base URL', () => {
+    const found = findProviderByCredentials(
+      'https://api.deepseek.com',
+      'DEEPSEEK_API_KEY',
+    );
+    expect(found?.id).toBe('deepseek');
+  });
+
+  it('returns undefined for an unknown env key', () => {
+    expect(
+      findProviderByCredentials('https://api.deepseek.com', 'NOT_A_REAL_KEY'),
+    ).toBeUndefined();
+  });
+
+  it('returns undefined for a known env key but mismatched base URL', () => {
+    expect(
+      findProviderByCredentials(
+        'https://wrong.example.com/v1',
+        'DEEPSEEK_API_KEY',
+      ),
+    ).toBeUndefined();
+  });
+
+  it('matches a multi-baseUrl preset against any of its registered URLs', () => {
+    // coding-plan ships both China and Singapore endpoints under the same env key.
+    const china = findProviderByCredentials(
+      'https://coding.dashscope.aliyuncs.com/v1',
+      'BAILIAN_CODING_PLAN_API_KEY',
+    );
+    const intl = findProviderByCredentials(
+      'https://coding-intl.dashscope.aliyuncs.com/v1',
+      'BAILIAN_CODING_PLAN_API_KEY',
+    );
+    expect(china?.id).toBe('coding-plan');
+    expect(intl?.id).toBe('coding-plan');
+  });
+});
+
+describe('getAllProviderBaseUrls', () => {
+  it('returns a non-empty list including known preset URLs', () => {
+    const urls = getAllProviderBaseUrls();
+    expect(urls.length).toBeGreaterThan(0);
+    expect(urls).toContain('https://api.deepseek.com');
+    expect(urls).toContain('https://openrouter.ai/api/v1');
+  });
+
+  it('expands BaseUrlOption[] presets into each option URL', () => {
+    const urls = getAllProviderBaseUrls();
+    // coding-plan has China + Singapore options
+    expect(urls).toContain('https://coding.dashscope.aliyuncs.com/v1');
+    expect(urls).toContain('https://coding-intl.dashscope.aliyuncs.com/v1');
+  });
+});

packages/core/src/providers/install.ts

@@ -88,6 +88,13 @@ export async function applyProviderInstallPlan(
   } = options;
 
   const previousEnvValues = new Map<string, string | undefined>();
+  // Snapshot the runtime providers map *before* any setValue/reload so we can
+  // restore in-memory state if a callback later in the flow rejects (e.g.
+  // refreshAuth() against a bad endpoint). Without this the live session
+  // could be left holding providers that the plan failed to install.
+  const previousRuntimeProviders: ModelProvidersConfig = {
+    ...settings.getModelProviders(),
+  };
 
   try {
     // backup() inside the try so a failure here (e.g. structuredClone on a
@@ -103,7 +110,7 @@ export async function applyProviderInstallPlan(
 
     // Apply model providers patches
     let updatedModelProviders: ModelProvidersConfig = {
-      ...settings.getModelProviders(),
+      ...previousRuntimeProviders,
     };
 
     for (const patch of plan.modelProviders ?? []) {
@@ -159,7 +166,17 @@ export async function applyProviderInstallPlan(
 
     return { updatedModelProviders };
   } catch (error) {
-    settings.restore?.();
+    // Best-effort rollback. Each step is wrapped so a failure in one
+    // doesn't mask the original error or skip the later steps.
+    try {
+      settings.restore?.();
+    } catch (restoreErr) {
+      // eslint-disable-next-line no-console -- best-effort rollback path
+      console.error(
+        '[applyProviderInstallPlan] settings.restore failed during rollback:',
+        restoreErr,
+      );
+    }
     for (const [key, prev] of previousEnvValues) {
       if (prev === undefined) {
         delete process.env[key];
@@ -167,6 +184,17 @@ export async function applyProviderInstallPlan(
         process.env[key] = prev;
       }
     }
+    // Restore in-memory runtime providers — reloadModelProviders may have run
+    // before the failure (e.g. before a refreshAuth rejection).
+    try {
+      reloadModelProviders?.(previousRuntimeProviders);
+    } catch (reloadErr) {
+      // eslint-disable-next-line no-console -- best-effort rollback path
+      console.error(
+        '[applyProviderInstallPlan] reloadModelProviders failed during rollback:',
+        reloadErr,
+      );
+    }
     throw error;
   }
 }

packages/vscode-ide-companion/src/services/settingsWriter.test.ts

@@ -25,9 +25,10 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => {
   };
 });
 
-import { AuthType } from '@qwen-code/qwen-code-core';
+import { AuthType, type ProviderInstallPlan } from '@qwen-code/qwen-code-core';
 import { CODING_PLAN_ENV_KEY } from './subscriptionPlanDefinitions.js';
 import {
+  applyProviderInstallPlanToFile,
   readQwenSettingsForVSCode,
   writeCodingPlanConfig,
   writeModelProvidersConfig,
@@ -103,4 +104,131 @@ describe('settingsWriter', () => {
       codingPlanRegion: 'china',
     });
   });
+
+  describe('applyProviderInstallPlanToFile', () => {
+    it('writes env, auth selection, and model providers to settings.json', async () => {
+      const plan: ProviderInstallPlan = {
+        providerId: 'test',
+        authType: AuthType.USE_OPENAI,
+        env: { TEST_API_KEY: 'sk-test' },
+        modelSelection: { modelId: 'gpt-4o' },
+        modelProviders: [
+          {
+            authType: AuthType.USE_OPENAI,
+            models: [{ id: 'gpt-4o', envKey: 'TEST_API_KEY' }],
+            mergeStrategy: 'prepend-and-remove-owned',
+            ownsModel: (m) => m.envKey === 'TEST_API_KEY',
+          },
+        ],
+      };
+
+      await applyProviderInstallPlanToFile(plan);
+
+      const written = JSON.parse(fs.readFileSync(settingsPath, 'utf-8'));
+      expect(written.env.TEST_API_KEY).toBe('sk-test');
+      expect(written.security.auth.selectedType).toBe(AuthType.USE_OPENAI);
+      expect(written.model.name).toBe('gpt-4o');
+      expect(written.modelProviders[AuthType.USE_OPENAI]).toEqual([
+        { id: 'gpt-4o', envKey: 'TEST_API_KEY' },
+      ]);
+    });
+
+    it('rejects __proto__ in install-plan env keys (prototype-pollution guard)', async () => {
+      // {__proto__: 'x'} literal sets the object's prototype rather than a
+      // real property, so build the env via defineProperty to land an actual
+      // "__proto__" own-property that survives Object.entries.
+      const env: Record<string, string> = {};
+      Object.defineProperty(env, '__proto__', {
+        value: 'polluted',
+        enumerable: true,
+        writable: true,
+        configurable: true,
+      });
+      const plan: ProviderInstallPlan = {
+        providerId: 'evil',
+        authType: AuthType.USE_OPENAI,
+        env,
+      };
+
+      await expect(applyProviderInstallPlanToFile(plan)).rejects.toThrow(
+        /reserved segment/,
+      );
+      // Ensure prototype was not polluted by the failed call
+      expect(({} as Record<string, unknown>).polluted).toBeUndefined();
+    });
+
+    it('rejects writes that would overwrite an intermediate scalar segment', async () => {
+      // Hand-edited settings with `env` as a string (legacy / mistake).
+      fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
+      fs.writeFileSync(
+        settingsPath,
+        JSON.stringify({ env: 'legacy-string' }),
+        'utf-8',
+      );
+      const plan: ProviderInstallPlan = {
+        providerId: 'test',
+        authType: AuthType.USE_OPENAI,
+        env: { NEW_KEY: 'value' },
+      };
+
+      await expect(applyProviderInstallPlanToFile(plan)).rejects.toThrow(
+        /segment "env" is a string/,
+      );
+      // Original scalar must be untouched
+      const after = JSON.parse(fs.readFileSync(settingsPath, 'utf-8'));
+      expect(after.env).toBe('legacy-string');
+    });
+
+    it('throws on malformed settings file instead of silently overwriting it', async () => {
+      fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
+      // Note the broken bracket — neither comments nor trailing commas fix it.
+      fs.writeFileSync(settingsPath, '{ "broken": [1, 2', 'utf-8');
+      const plan: ProviderInstallPlan = {
+        providerId: 'test',
+        authType: AuthType.USE_OPENAI,
+        env: { K: 'v' },
+      };
+
+      await expect(applyProviderInstallPlanToFile(plan)).rejects.toThrow();
+      // Bad file is preserved, not silently clobbered with {}
+      expect(fs.readFileSync(settingsPath, 'utf-8')).toBe('{ "broken": [1, 2');
+    });
+
+    it('parses JSONC with trailing commas (and preserves comma inside strings)', async () => {
+      fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
+      // Comments + trailing commas + a string containing a literal ",]".
+      const jsonc = `{
+  // hand-edited
+  "preserveMe": ",]",
+  "list": [1, 2,],
+}`;
+      fs.writeFileSync(settingsPath, jsonc, 'utf-8');
+      const plan: ProviderInstallPlan = {
+        providerId: 'test',
+        authType: AuthType.USE_OPENAI,
+        env: { K: 'v' },
+      };
+
+      await applyProviderInstallPlanToFile(plan);
+
+      const after = JSON.parse(fs.readFileSync(settingsPath, 'utf-8'));
+      expect(after.preserveMe).toBe(',]'); // literal preserved, not corrupted
+      expect(after.list).toEqual([1, 2]);
+      expect(after.env.K).toBe('v');
+    });
+
+    it('writes atomically — no .tmp residue on success', async () => {
+      const plan: ProviderInstallPlan = {
+        providerId: 'test',
+        authType: AuthType.USE_OPENAI,
+        env: { K: 'v' },
+      };
+      await applyProviderInstallPlanToFile(plan);
+      const dir = path.dirname(settingsPath);
+      const leftovers = fs
+        .readdirSync(dir)
+        .filter((f) => f.startsWith('settings.json.') && f.endsWith('.tmp'));
+      expect(leftovers).toEqual([]);
+    });
+  });
 });

packages/vscode-ide-companion/src/services/settingsWriter.ts

@@ -102,12 +102,47 @@ function stripJsonComments(text: string): string {
 
 /**
  * Strip trailing commas inside arrays/objects (`,]` / `,}`) — VSCode's own
- * settings.json allows them and they crash strict JSON.parse. Operates on the
- * already-comment-stripped text, so a literal `,]` inside a string is safe
- * (string-literal copy in stripJsonComments preserves contents).
+ * settings.json allows them and they crash strict JSON.parse. Uses a
+ * character-by-character scanner so that the `,]` substring inside a string
+ * literal (e.g. `"MY_VAR": ",]"`) is preserved unchanged — a regex would
+ * silently rewrite it and corrupt the value.
  */
 function stripTrailingCommas(text: string): string {
-  return text.replace(/,(\s*[}\]])/g, '$1');
+  let result = '';
+  let i = 0;
+  while (i < text.length) {
+    const ch = text[i];
+    if (ch === '"') {
+      // Copy the entire string literal (including escapes) verbatim.
+      let j = i + 1;
+      while (j < text.length) {
+        if (text[j] === '\\') {
+          j += 2;
+          continue;
+        }
+        if (text[j] === '"') {
+          j++;
+          break;
+        }
+        j++;
+      }
+      result += text.slice(i, j);
+      i = j;
+      continue;
+    }
+    if (ch === ',') {
+      // Drop comma only if the next non-whitespace char is } or ].
+      let j = i + 1;
+      while (j < text.length && /\s/.test(text[j]!)) j++;
+      if (j < text.length && (text[j] === ']' || text[j] === '}')) {
+        i++;
+        continue;
+      }
+    }
+    result += ch;
+    i++;
+  }
+  return result;
 }
 
 /**

packages/vscode-ide-companion/src/webview/handlers/AuthMessageHandler.ts

@@ -275,15 +275,27 @@ export class AuthMessageHandler extends BaseMessageHandler {
         if (!selected) return;
         baseUrl = selected;
       } else {
-        // Free-form URL input
+        // Free-form URL input. Show a protocol-specific default as
+        // placeholder (NOT pre-filled value) so picking Anthropic/Gemini
+        // doesn't silently write the OpenAI endpoint when the user hits
+        // Enter on the OpenAI default.
+        const effectiveProtocol = protocol ?? provider.protocol;
+        const defaultByProtocol: Record<string, string> = {
+          openai: 'https://api.openai.com/v1',
+          anthropic: 'https://api.anthropic.com/v1',
+          gemini: 'https://generativelanguage.googleapis.com',
+        };
+        const placeholder =
+          defaultByProtocol[String(effectiveProtocol)] ??
+          'https://api.openai.com/v1';
         const urlInput = await this.input({
           title: `${flowTitle}: Base URL`,
           prompt: 'Enter API base URL',
-          placeHolder: 'https://api.openai.com/v1',
-          value: 'https://api.openai.com/v1',
+          placeHolder: placeholder,
+          value: '',
         });
         if (urlInput === undefined) return;
-        baseUrl = urlInput;
+        baseUrl = urlInput.trim() || placeholder;
         if (!/^https?:\/\//i.test(baseUrl)) {
           this.sendToWebView({
             type: 'authError',
@@ -338,6 +350,15 @@ export class AuthMessageHandler extends BaseMessageHandler {
         .split(',')
         .map((id) => id.trim())
         .filter(Boolean);
+      if (modelIds.length === 0) {
+        // E.g. user typed only whitespace/commas like ", , ,".
+        this.sendToWebView({
+          type: 'authError',
+          data: { message: 'Model IDs cannot be empty.' },
+        });
+        this.notifyAuthCancelled();
+        return;
+      }
     } else {
       modelIds = getDefaultModelIds(provider);
     }

packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts

@@ -1336,6 +1336,9 @@ export class WebViewProvider {
       await this.doInitializeAgentConnection({ autoAuthenticate: false });
 
       // Only emit authSuccess when the reconnection actually authenticated.
+      // doInitializeAgentConnection sets this.authState via sendMessageToWebView
+      // — when credentials are rejected (wrong key / bad endpoint) it stays
+      // false, and showing a success toast then would mislead the user.
       if (this.authState === true) {
         this.sendMessageToWebView({
           type: 'authSuccess',