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

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>

packages/core/src/providers/__tests__/install.test.ts

@@ -443,10 +443,16 @@ describe('applyProviderInstallPlan', () => {
     }
 
     expect(caught).toBeInstanceOf(Error);
-    const err = caught as Error & { cause?: Error };
-    expect(err.message).toContain('step "refreshAuth"');
-    expect(err.message).toContain('authType=openai');
-    expect(err.message).toContain('endpoint unreachable');
+    const err = caught as Error & {
+      step?: string;
+      authType?: string;
+      cause?: Error;
+    };
+    // Step + authType are structured properties (not baked into the
+    // user-facing message, which stays the underlying error text).
+    expect(err.step).toBe('refreshAuth');
+    expect(err.authType).toBe('openai');
+    expect(err.message).toBe('endpoint unreachable');
     // Original error preserved via cause so callers matching on err.code
     // (NodeJS.ErrnoException) still work.
     expect(err.cause).toBeInstanceOf(Error);

packages/core/src/providers/index.ts

@@ -75,4 +75,5 @@ export {
   applyProviderInstallPlan,
   type ApplyProviderInstallPlanOptions,
   type ApplyProviderInstallPlanResult,
+  type ProviderInstallError,
 } from './install.js';

packages/core/src/providers/install.ts

@@ -75,6 +75,18 @@ export interface ApplyProviderInstallPlanResult {
   updatedModelProviders: ModelProvidersConfig;
 }
 
+/**
+ * Error thrown by {@link applyProviderInstallPlan} when a step fails. The
+ * message is the underlying error's message (safe to surface to users); the
+ * `step` and `authType` properties carry diagnostic context, and `cause`
+ * preserves the original error (so callers matching on `err.code` still work
+ * via the chain).
+ */
+export interface ProviderInstallError extends Error {
+  step: string;
+  authType: AuthType;
+}
+
 export async function applyProviderInstallPlan(
   plan: ProviderInstallPlan,
   options: ApplyProviderInstallPlanOptions,
@@ -220,16 +232,16 @@ export async function applyProviderInstallPlan(
         reloadErr,
       );
     }
-    // Annotate the rethrow with the step that failed so a downstream catch
-    // (handleAuthFailure, /doctor) can tell EACCES-from-persist apart from a
-    // refreshAuth rejection. Preserve the original via `cause`.
+    // Attach the failing step + authType as *structured properties* rather
+    // than baking them into the message. Keeps the user-facing message clean
+    // (callers show error.message verbatim) while letting devs read
+    // error.step / error.authType and the original error.cause off the chain.
     const errMsg = error instanceof Error ? error.message : String(error);
-    const annotated = new Error(
-      `applyProviderInstallPlan failed at step "${currentStep}" for authType=${plan.authType}: ${errMsg}`,
-      { cause: error instanceof Error ? error : undefined },
-    );
-    // Preserve the original name (e.g. NodeJS.ErrnoException) so callers
-    // matching on err.code still work via the `cause` chain.
+    const annotated = new Error(errMsg, {
+      cause: error instanceof Error ? error : undefined,
+    }) as ProviderInstallError;
+    annotated.step = currentStep;
+    annotated.authType = plan.authType;
     throw annotated;
   }
 }

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

@@ -365,8 +365,15 @@ export function providerMatchesCredentials(
           configEnvKey = derived;
           break;
         }
-      } catch {
-        /* try next protocol */
+      } catch (err) {
+        // A throw here is a programming error in the provider's envKey fn,
+        // not an expected "no match" — surface it so a custom provider
+        // silently vanishing from /doctor / system-info has a trace.
+        // eslint-disable-next-line no-console -- diagnostic for a misconfigured provider
+        console.warn(
+          `[providerMatchesCredentials] envKey(${proto}, ${baseUrl}) threw; skipping this protocol:`,
+          err,
+        );
       }
     }
   }

packages/core/src/providers/types.ts

@@ -168,11 +168,26 @@ export interface ProviderInstallPlan {
 export interface ProviderSettingsAdapter {
   /** Get a value by dotted key path (e.g. 'security.auth.selectedType'). */
   getValue(key: string): unknown;
-  /** Set a value by dotted key path. */
+  /**
+   * Set a value by dotted key path.
+   *
+   * IMPORTANT: implementations MAY flush to disk on every call (the CLI's
+   * LoadedSettings-backed adapter does — each setValue triggers a
+   * saveSettings). Callers must therefore NOT assume the on-disk file is
+   * untouched until `persist()`; if the process crashes mid-sequence, disk
+   * can hold a partial write. `backup()`/`restore()` are the rollback path
+   * for that, not deferred persistence. Don't insert new pre-persist steps
+   * assuming atomicity.
+   */
   setValue(key: string, value: unknown): void;
   /** Get the current model providers config. */
   getModelProviders(): ModelProvidersConfig;
-  /** Flush changes to disk. */
+  /**
+   * Flush changes to disk. NOTE: this may be a no-op for adapters whose
+   * `setValue` already persists eagerly (see the warning on `setValue`).
+   * It remains in the contract as the explicit commit point for adapters
+   * that *do* buffer (e.g. the VS Code file adapter writes here).
+   */
   persist(): void;
   /** Create a backup before making changes (for rollback on error). */
   backup?(): void;

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

@@ -31,6 +31,8 @@ import {
   applyProviderInstallPlanToFile,
   clearPersistedAuth,
   readQwenSettingsForVSCode,
+  restoreSettingsSnapshot,
+  snapshotSettingsForRollback,
   writeCodingPlanConfig,
   writeModelProvidersConfig,
 } from './settingsWriter.js';
@@ -313,4 +315,53 @@ describe('settingsWriter', () => {
       expect(() => clearPersistedAuth()).not.toThrow();
     });
   });
+
+  describe('snapshotSettingsForRollback / restoreSettingsSnapshot', () => {
+    it('round-trips: snapshot → mutate → restore brings the old state back', () => {
+      fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
+      const original = {
+        env: { OPENAI_API_KEY: 'sk-good' },
+        security: { auth: { selectedType: 'openai' } },
+      };
+      fs.writeFileSync(
+        settingsPath,
+        JSON.stringify(original, null, 2),
+        'utf-8',
+      );
+
+      const snapshot = snapshotSettingsForRollback();
+      expect(snapshot).not.toBeNull();
+
+      // Simulate a bad-credential install writing over the file.
+      fs.writeFileSync(
+        settingsPath,
+        JSON.stringify({ env: { OPENAI_API_KEY: 'sk-bad' } }, null, 2),
+        'utf-8',
+      );
+
+      restoreSettingsSnapshot(snapshot);
+
+      const after = JSON.parse(fs.readFileSync(settingsPath, 'utf-8'));
+      expect(after).toEqual(original);
+    });
+
+    it('snapshot returns null on a malformed file and restore is then a no-op', () => {
+      fs.mkdirSync(path.dirname(settingsPath), { recursive: true });
+      fs.writeFileSync(settingsPath, '{ "broken": [1, 2', 'utf-8');
+
+      const snapshot = snapshotSettingsForRollback();
+      expect(snapshot).toBeNull();
+
+      // No-op restore must not throw and must not clobber the file.
+      expect(() => restoreSettingsSnapshot(snapshot)).not.toThrow();
+      expect(fs.readFileSync(settingsPath, 'utf-8')).toBe('{ "broken": [1, 2');
+    });
+
+    it('snapshot returns {} (not null) when no settings file exists', () => {
+      // ENOENT → readSettings returns {}, so we get a valid empty snapshot
+      // that restore can write (creating the file).
+      const snapshot = snapshotSettingsForRollback();
+      expect(snapshot).toEqual({});
+    });
+  });
 });

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

@@ -66,11 +66,6 @@ export interface QwenSettingsForVSCode {
 // Low-level read/write helpers
 // ---------------------------------------------------------------------------
 
-/**
- * Strip single-line (`//`) and multi-line comments from JSONC content
- * so that `JSON.parse` can handle ~/.qwen/settings.json files that
- * contain comments (common when hand-edited or generated by CLI).
- */
 /**
  * Length of a JSON escape sequence starting with `\\`. Handles the six-char
  * `\\uXXXX` form — otherwise a value like `"sk-abc\\u0022,..."` would let the
@@ -81,6 +76,11 @@ function jsonEscapeLength(text: string, backslashIdx: number): number {
   return text[backslashIdx + 1] === 'u' ? 6 : 2;
 }
 
+/**
+ * Strip single-line (`//`) and multi-line comments from JSONC content
+ * so that `JSON.parse` can handle ~/.qwen/settings.json files that
+ * contain comments (common when hand-edited or generated by CLI).
+ */
 function stripJsonComments(text: string): string {
   let result = '';
   let i = 0;
@@ -529,6 +529,37 @@ export async function applyProviderInstallPlanToFile(
   await applyProviderInstallPlan(plan, { settings });
 }
 
+/**
+ * Capture a deep-cloned snapshot of the current on-disk settings for rollback,
+ * or `null` if the file is missing/unreadable.
+ *
+ * Unlike `readSettings`, this never throws — callers use it to checkpoint
+ * before `applyProviderInstallPlanToFile` so that a *later* step the install
+ * plan can't see (e.g. the agent reconnect rejecting a bad API key) can be
+ * undone via {@link restoreSettingsSnapshot}. `applyProviderInstallPlan`'s own
+ * backup/restore only covers failures *inside* the plan; the
+ * disconnect/reconnect in WebViewProvider runs after `cleanupBackup`, so the
+ * caller owns that rollback window.
+ */
+export function snapshotSettingsForRollback(): Record<string, unknown> | null {
+  try {
+    return structuredClone(readSettings());
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * Write a snapshot captured by {@link snapshotSettingsForRollback} back to
+ * disk. No-op when the snapshot is `null` (nothing safe to restore).
+ */
+export function restoreSettingsSnapshot(
+  snapshot: Record<string, unknown> | null,
+): void {
+  if (snapshot === null) return;
+  writeSettings(snapshot);
+}
+
 // ---------------------------------------------------------------------------
 // Read: ~/.qwen/settings.json → VSCode Settings
 // ---------------------------------------------------------------------------

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

@@ -318,13 +318,19 @@ export class AuthMessageHandler extends BaseMessageHandler {
     }
 
     // Step 2: API Key
-    const apiKey = await this.input({
+    const apiKeyInput = await this.input({
       title: `${flowTitle}: API Key`,
       prompt: 'Enter your API key',
       placeHolder: provider.apiKeyPlaceholder ?? 'sk-...',
       password: true,
       required: true,
     });
+    if (!apiKeyInput) return;
+    // Trim before validation and persistence — a key pasted with trailing
+    // whitespace would otherwise be stored as-is and cause silent auth
+    // failures, and validateApiKey could reject in VS Code what the CLI
+    // (which trims) accepts.
+    const apiKey = apiKeyInput.trim();
     if (!apiKey) return;
 
     // Validate API key if provider has validation

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

@@ -166,6 +166,9 @@ vi.mock('../../services/settingsWriter.js', () => ({
   writeModelProvidersConfig: mockWriteModelProvidersConfig,
   readQwenSettingsForVSCode: mockReadQwenSettingsForVSCode,
   clearPersistedAuth: mockClearPersistedAuth,
+  applyProviderInstallPlanToFile: vi.fn().mockResolvedValue(undefined),
+  snapshotSettingsForRollback: vi.fn().mockReturnValue(null),
+  restoreSettingsSnapshot: vi.fn(),
 }));
 
 vi.mock('../../services/qwenAgentManager.js', () => ({

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

@@ -29,6 +29,8 @@ import { isAuthenticationRequiredError } from '../../utils/authErrors.js';
 import { getErrorMessage } from '../../utils/errorMessage.js';
 import {
   applyProviderInstallPlanToFile,
+  snapshotSettingsForRollback,
+  restoreSettingsSnapshot,
   writeCodingPlanConfig,
   readQwenSettingsForVSCode,
   clearPersistedAuth,
@@ -1326,6 +1328,13 @@ export class WebViewProvider {
       `[WebViewProvider] authInteractive: provider=${providerConfig.id}, host=${baseUrlHost}`,
     );
 
+    // Snapshot the pre-write settings so we can roll back bad credentials if
+    // the reconnect below rejects them. applyProviderInstallPlanToFile's own
+    // backup/restore only covers failures *inside* the plan; the
+    // disconnect/reconnect runs after the plan commits (cleanupBackup), so
+    // without this a rejected key would persist and every VS Code restart
+    // would keep retrying it.
+    const rollbackSnapshot = snapshotSettingsForRollback();
     try {
       // Use core's buildInstallPlan to create a standardized install plan,
       // then apply it via the VSCode settings adapter.
@@ -1355,6 +1364,9 @@ export class WebViewProvider {
           data: { message: 'Provider configured successfully!' },
         });
       } else {
+        // Auth failed against the live backend — roll the bad credentials
+        // back off disk so a restart doesn't keep retrying them.
+        restoreSettingsSnapshot(rollbackSnapshot);
         this.sendMessageToWebView({
           type: 'authError',
           data: {
@@ -1366,6 +1378,11 @@ export class WebViewProvider {
     } catch (error) {
       const errorMsg = getErrorMessage(error);
       console.error('[WebViewProvider] authInteractive failed:', error);
+      // A throw can land here after the plan committed but before/while
+      // reconnecting — restore the snapshot so partial/bad state doesn't
+      // linger. (Redundant but harmless if the plan's own rollback already
+      // ran: it just rewrites the same pre-state.)
+      restoreSettingsSnapshot(rollbackSnapshot);
       this.sendMessageToWebView({
         type: 'authError',
         data: { message: `Configuration failed: ${errorMsg}` },