fix(auth): address round-3 review blockers

- Fix CI: add missing useProviderUpdates mock in AppContainer.test.tsx
  that caused TypeError breaking React effects (title/height tests)
- Fix half-rollback: snapshot settings + modelProviders before install,
  restore in-memory state (not just disk) on refreshAuth failure
- Fix .orig backup reuse: always create fresh backup (overwrite stale),
  cleanup on success, unlink after restore to prevent data loss
- Fix cross-package key consistency: VS Code settingsWriter now writes
  to providerMetadata namespace matching CLI's new structure
- Fix validateApiKey: remove baseUrl guard so sk-sp- prefix check
  applies to both China and Global Coding Plan endpoints
This commit is contained in:
pomelo-nwu 2026-05-07 21:39:16 +08:00
parent 4f935b37f7
commit 91fa309420
10 changed files with 85 additions and 11 deletions

View file

@ -13,6 +13,7 @@ import type { ProviderInstallPlan } from '../types.js';
vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
vi.mock('../../config/modelProvidersScope.js', () => ({
@ -20,12 +21,18 @@ vi.mock('../../config/modelProvidersScope.js', () => ({
}));
function createSettings(modelProviders = {}) {
const settingsObj = {
settings: {},
originalSettings: {},
path: '/tmp/settings.json',
};
return {
merged: {
modelProviders,
},
setValue: vi.fn(),
forScope: vi.fn(() => ({ path: '/tmp/settings.json' })),
forScope: vi.fn(() => settingsObj),
recomputeMerged: vi.fn(),
};
}

View file

@ -8,6 +8,7 @@ import type { ModelProvidersConfig } from '@qwen-code/qwen-code-core';
import { getPersistScopeForModelSelection } from '../../config/modelProvidersScope.js';
import {
backupSettingsFile,
cleanupSettingsBackup,
restoreSettingsFromBackup,
} from '../../utils/settingsUtils.js';
import type {
@ -61,6 +62,14 @@ export async function applyProviderInstallPlan(
backupSettingsFile(settingsFile.path);
const previousEnvValues = new Map<string, string | undefined>();
const previousSettingsSnapshot = structuredClone(settingsFile.settings);
const previousOriginalSnapshot = structuredClone(
settingsFile.originalSettings,
);
const previousModelProviders: ModelProvidersConfig = {
...((settings.merged.modelProviders as ModelProvidersConfig | undefined) ??
{}),
};
try {
for (const [key, value] of Object.entries(plan.env ?? {})) {
@ -133,12 +142,23 @@ export async function applyProviderInstallPlan(
await config.refreshAuth(plan.authType);
}
cleanupSettingsBackup(settingsFile.path);
return {
persistScope,
updatedModelProviders,
};
} catch (error) {
restoreSettingsFromBackup(settingsFile.path);
// Restore in-memory settings state
settingsFile.settings = previousSettingsSnapshot;
settingsFile.originalSettings = previousOriginalSnapshot;
settings.recomputeMerged();
// Restore in-memory config state
config.reloadModelProvidersConfig(previousModelProviders);
for (const [key, prev] of previousEnvValues) {
if (prev === undefined) {
delete process.env[key];

View file

@ -83,8 +83,8 @@ export const codingPlanProvider: ProviderConfig = {
? 'ModelStudio Coding Plan for Global/Intl'
: 'ModelStudio Coding Plan',
apiKeyPlaceholder: 'sk-sp-...',
validateApiKey: (key, baseUrl) =>
baseUrl === CODING_PLAN_CHINA_BASE_URL && !key.startsWith('sk-sp-')
validateApiKey: (key) =>
!key.startsWith('sk-sp-')
? 'Invalid API key. Coding Plan API keys start with "sk-sp-". Please check.'
: null,
ownsModel: (model) =>

View file

@ -48,6 +48,8 @@ vi.mock('../../config/config.js', () => ({
vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: mockBackupSettingsFile,
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
vi.mock('../../config/modelProvidersScope.js', () => ({

View file

@ -438,6 +438,10 @@ export class LoadedSettings {
saveSettings(settingsFile, createSettingsUpdate(key, value));
}
recomputeMerged(): void {
this._merged = this.computeMergedSettings();
}
/**
* Get user-level hooks from user settings (not merged with workspace).
* These hooks should always be loaded regardless of folder trust.

View file

@ -79,6 +79,12 @@ vi.mock('./hooks/useIdeTrustListener.js');
vi.mock('./hooks/useMessageQueue.js');
vi.mock('./hooks/useAutoAcceptIndicator.js');
vi.mock('./hooks/useGitBranchName.js');
vi.mock('./hooks/useProviderUpdates.js', () => ({
useProviderUpdates: vi.fn(() => ({
providerUpdateRequest: undefined,
dismissProviderUpdate: vi.fn(),
})),
}));
vi.mock('./contexts/VimModeContext.js');
vi.mock('./contexts/SessionContext.js');
vi.mock('./contexts/AgentViewContext.js', () => ({

View file

@ -28,6 +28,8 @@ vi.mock('../hooks/useQwenAuth.js', () => ({
vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
vi.mock('../../config/modelProvidersScope.js', () => ({

View file

@ -25,6 +25,8 @@ import {
vi.mock('../../utils/settingsUtils.js', () => ({
backupSettingsFile: vi.fn(),
restoreSettingsFromBackup: vi.fn(),
cleanupSettingsBackup: vi.fn(),
}));
const chinaTemplate = buildProviderTemplate(

View file

@ -624,7 +624,7 @@ export function getEffectiveDisplayValue(
/**
* Backup a settings file before modification.
* Creates a backup with `.orig` suffix if the file exists and backup doesn't already exist.
* Always creates a fresh backup with `.orig` suffix (overwrites any stale backup).
* @param filePath - Path to the settings file to backup
* @returns boolean indicating whether a backup was created
*/
@ -632,10 +632,8 @@ export function backupSettingsFile(filePath: string): boolean {
try {
if (fs.existsSync(filePath)) {
const backupPath = `${filePath}.orig`;
if (!fs.existsSync(backupPath)) {
fs.renameSync(filePath, backupPath);
return true;
}
fs.copyFileSync(filePath, backupPath);
return true;
}
} catch (_e) {
// Ignore backup errors, proceed without backup
@ -645,6 +643,7 @@ export function backupSettingsFile(filePath: string): boolean {
/**
* Restore a settings file from its `.orig` backup created by {@link backupSettingsFile}.
* Removes the backup file after a successful restore.
* @param filePath - Path to the settings file to restore
* @returns boolean indicating whether the restore succeeded
*/
@ -653,6 +652,7 @@ export function restoreSettingsFromBackup(filePath: string): boolean {
const backupPath = `${filePath}.orig`;
if (fs.existsSync(backupPath)) {
fs.copyFileSync(backupPath, filePath);
fs.unlinkSync(backupPath);
return true;
}
} catch (_e) {
@ -661,4 +661,19 @@ export function restoreSettingsFromBackup(filePath: string): boolean {
return false;
}
/**
* Remove the `.orig` backup after a successful operation.
* @param filePath - Path to the settings file whose backup should be removed
*/
export function cleanupSettingsBackup(filePath: string): void {
try {
const backupPath = `${filePath}.orig`;
if (fs.existsSync(backupPath)) {
fs.unlinkSync(backupPath);
}
} catch (_e) {
// Ignore cleanup errors — non-critical
}
}
export const TEST_ONLY = { clearFlattenedSchema };

View file

@ -145,8 +145,14 @@ export function writeCodingPlanConfig(
}));
providers[AuthType.USE_OPENAI] = [...planModels, ...nonCodingPlan];
// Coding Plan metadata
settings.codingPlan = { region: codingRegion, version: planConfig.version };
// Coding Plan metadata — write to the providerMetadata namespace that
// the CLI now reads from. Remove legacy top-level key if present.
const providerMetadata = ensureNestedObject(settings, 'providerMetadata');
providerMetadata['coding-plan'] = {
region: codingRegion,
version: planConfig.version,
};
delete settings.codingPlan;
// Default model
const defaultModelId = planConfig.template[0]?.id ?? 'qwen3.5-plus';
@ -214,6 +220,11 @@ export function writeModelProvidersConfig(params: {
for (const plan of SUBSCRIPTION_PLAN_OPTIONS) {
delete settings[plan.metadataKey];
}
const pm = settings.providerMetadata as Record<string, unknown> | undefined;
if (pm) {
delete pm['coding-plan'];
delete pm['token-plan'];
}
writeSettings(settings);
}
@ -297,10 +308,15 @@ export function clearPersistedAuth(): void {
delete env['OPENAI_API_KEY'];
}
// Remove subscription plan metadata
// Remove subscription plan metadata (legacy + new namespace)
for (const plan of SUBSCRIPTION_PLAN_OPTIONS) {
delete settings[plan.metadataKey];
}
const pm = settings.providerMetadata as Record<string, unknown> | undefined;
if (pm) {
delete pm['coding-plan'];
delete pm['token-plan'];
}
writeSettings(settings);
} catch (error) {