fix: tighten config and recovery review follow-ups

Author: ndycode

docs/reference/storage-paths.md

@@ -52,6 +52,12 @@ Backup metadata:
 - `settings.json.bak` stores the last valid unified settings snapshot before each write and is used as a recovery fallback when `settings.json` is unreadable.
 - Flagged-account backup recovery is suppressed whenever the flagged reset marker is still present, so partial clears cannot revive previously cleared flagged entries.
 
+Upgrade note:
+
+- Restore workflows now distinguish between unreadable state and intentionally cleared state. `settings.json.bak` is only used when `settings.json` exists but cannot be read, while flagged-account backups stay suppressed whenever the reset marker survives a partial clear.
+- Operators validating a restore or clear flow should use `codex auth verify-flagged`, `codex auth fix --dry-run`, and `codex auth doctor --fix` to confirm what will be recovered, what stays cleared, and whether manual repair is still needed.
+- Maintainers validating the on-disk upgrade behavior can run `npm run build` plus `npm test -- --run test/unified-settings.test.ts test/storage-recovery-paths.test.ts test/storage-flagged.test.ts` before shipping backup or restore changes.
+
 ---
 
 ## Project-Scoped Account Paths

lib/config.ts

@@ -320,11 +320,18 @@ function sanitizePluginConfigRecord(
 	return sanitized as Partial<PluginConfig>;
 }
 
+/**
+ * Sanitize a stored plugin-config record while preserving unknown keys.
+ *
+ * Known plugin-config keys are schema-validated before they are merged back
+ * into runtime state. Unknown keys are kept so legacy and forward-compatible
+ * settings survive env-path saves.
+ */
 function sanitizeStoredPluginConfigRecord(
 	data: unknown,
-): Record<string, unknown> {
+): Record<string, unknown> | null {
 	if (!isRecord(data)) {
-		return {};
+		return null;
 	}
 
 	const sanitized: Record<string, unknown> = {};
@@ -551,10 +558,11 @@ export async function savePluginConfig(
 
 	const unifiedPath = getUnifiedSettingsPath();
 	await withConfigSaveLock(unifiedPath, async () => {
+		const unifiedConfigRecord = loadUnifiedPluginConfigSync();
 		const unifiedConfig = sanitizeStoredPluginConfigRecord(
-			loadUnifiedPluginConfigSync(),
+			unifiedConfigRecord,
 		);
-		const legacyPath = unifiedConfig ? null : resolvePluginConfigPath();
+		const legacyPath = unifiedConfigRecord ? null : resolvePluginConfigPath();
 		const legacyConfig = legacyPath
 			? sanitizeStoredPluginConfigRecord(readConfigRecordFromPath(legacyPath))
 			: null;
@@ -631,7 +639,11 @@ function resolveBooleanSetting(
 ): boolean {
 	const rawEnvValue = process.env[envName];
 	const envValue = parseBooleanEnv(rawEnvValue);
-	if (rawEnvValue !== undefined && envValue === undefined) {
+	if (
+		rawEnvValue !== undefined &&
+		rawEnvValue.trim().length > 0 &&
+		envValue === undefined
+	) {
 		logConfigWarnOnce(
 			`Ignoring invalid boolean env ${envName}=${JSON.stringify(rawEnvValue)}. Expected 0/1, true/false, or yes/no.`,
 		);
@@ -659,7 +671,11 @@ function resolveNumberSetting(
 ): number {
 	const rawEnvValue = process.env[envName];
 	const envValue = parseNumberEnv(rawEnvValue);
-	if (rawEnvValue !== undefined && envValue === undefined) {
+	if (
+		rawEnvValue !== undefined &&
+		rawEnvValue.trim().length > 0 &&
+		envValue === undefined
+	) {
 		logConfigWarnOnce(
 			`Ignoring invalid numeric env ${envName}=${JSON.stringify(rawEnvValue)}. Expected a finite number.`,
 		);

lib/storage/flagged-storage-io.ts

@@ -2,6 +2,9 @@ import { existsSync, promises as fs } from "node:fs";
 import { dirname } from "node:path";
 import type { FlaggedAccountStorageV1 } from "../storage.js";
 
+/**
+ * Return the ordered backup paths consulted for flagged-account recovery.
+ */
 function getFlaggedBackupPaths(path: string): string[] {
 	return [`${path}.bak`, `${path}.bak.1`, `${path}.bak.2`];
 }
@@ -28,6 +31,9 @@ export async function loadFlaggedAccountsState(params: {
 				const backupContent = await fs.readFile(backupPath, "utf-8");
 				const backupData = JSON.parse(backupContent) as unknown;
 				const recovered = params.normalizeFlaggedStorage(backupData);
+				if (existsSync(params.resetMarkerPath)) {
+					return empty;
+				}
 				params.logInfo("Recovered flagged account storage from backup", {
 					from: backupPath,
 					to: params.path,

lib/unified-settings.ts

@@ -19,7 +19,9 @@ export const UNIFIED_SETTINGS_VERSION = 1 as const;
 const UNIFIED_SETTINGS_PATH = join(getCodexMultiAuthDir(), "settings.json");
 const UNIFIED_SETTINGS_BACKUP_PATH = `${UNIFIED_SETTINGS_PATH}.bak`;
 const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]);
+const TRANSIENT_READ_FS_CODES = new Set(["EBUSY", "EAGAIN"]);
 let settingsWriteQueue: Promise<void> = Promise.resolve();
+let backupDerivedReadPending = false;
 
 function isRetryableFsError(error: unknown): boolean {
 	const code = (error as NodeJS.ErrnoException | undefined)?.code;
@@ -55,13 +57,22 @@ function parseSettingsRecord(content: string): JsonRecord {
 	return parsed;
 }
 
+/**
+ * Read a unified-settings record from a specific path.
+ *
+ * Returns `null` when the file is absent and throws when the file exists but
+ * cannot be parsed into the expected object shape.
+ */
 function readSettingsRecordSyncFromPath(filePath: string): JsonRecord | null {
 	if (!existsSync(filePath)) {
 		return null;
 	}
 	return parseSettingsRecord(readFileSync(filePath, "utf8"));
 }
 
+/**
+ * Async variant of `readSettingsRecordSyncFromPath`.
+ */
 async function readSettingsRecordAsyncFromPath(
 	filePath: string,
 ): Promise<JsonRecord | null> {
@@ -71,6 +82,12 @@ async function readSettingsRecordAsyncFromPath(
 	return parseSettingsRecord(await fs.readFile(filePath, "utf8"));
 }
 
+/**
+ * Best-effort backup reader for sync callers.
+ *
+ * Backup corruption is treated as an unavailable backup so callers can keep
+ * their legacy null-on-unavailable behavior.
+ */
 function readSettingsBackupSync(): JsonRecord | null {
 	try {
 		return readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
@@ -79,6 +96,9 @@ function readSettingsBackupSync(): JsonRecord | null {
 	}
 }
 
+/**
+ * Best-effort backup reader for async callers.
+ */
 async function readSettingsBackupAsync(): Promise<JsonRecord | null> {
 	try {
 		return await readSettingsRecordAsyncFromPath(UNIFIED_SETTINGS_BACKUP_PATH);
@@ -87,7 +107,40 @@ async function readSettingsBackupAsync(): Promise<JsonRecord | null> {
 	}
 }
 
+/**
+ * Decide whether a primary settings read should fall back to the backup file.
+ *
+ * Missing primaries stay missing so callers do not merge stale backup state.
+ * Corrupt or unreadable primaries may fall back, while transient lock errors
+ * are rethrown so callers can retry.
+ */
+function shouldFallbackToSettingsBackup(
+	primaryExists: boolean,
+	error: unknown,
+): boolean {
+	if (!primaryExists) {
+		return false;
+	}
+	const code = (error as NodeJS.ErrnoException | undefined)?.code;
+	if (code === "ENOENT") {
+		return false;
+	}
+	if (typeof code === "string" && TRANSIENT_READ_FS_CODES.has(code)) {
+		return false;
+	}
+	return true;
+}
+
+/**
+ * Snapshot the primary settings file into `settings.json.bak` for sync writes.
+ *
+ * When the current in-memory state was recovered from backup, snapshotting is
+ * skipped so a corrupt primary cannot overwrite the only known-good backup.
+ */
 function trySnapshotUnifiedSettingsBackupSync(): void {
+	if (backupDerivedReadPending) {
+		return;
+	}
 	if (!existsSync(UNIFIED_SETTINGS_PATH)) {
 		return;
 	}
@@ -104,7 +157,13 @@ function trySnapshotUnifiedSettingsBackupSync(): void {
 	}
 }
 
+/**
+ * Async variant of `trySnapshotUnifiedSettingsBackupSync`.
+ */
 async function trySnapshotUnifiedSettingsBackupAsync(): Promise<void> {
+	if (backupDerivedReadPending) {
+		return;
+	}
 	if (!existsSync(UNIFIED_SETTINGS_PATH)) {
 		return;
 	}
@@ -132,20 +191,25 @@ async function trySnapshotUnifiedSettingsBackupAsync(): Promise<void> {
  * - Sensitive data: this function performs no token or secret redaction; any sensitive values present in the file are returned as-is and callers are responsible for redaction before logging or external exposure.
  */
 function readSettingsRecordSync(): JsonRecord | null {
+	const primaryExists = existsSync(UNIFIED_SETTINGS_PATH);
 	try {
 		const primaryRecord = readSettingsRecordSyncFromPath(UNIFIED_SETTINGS_PATH);
 		if (primaryRecord) {
 			return primaryRecord;
 		}
 	} catch (error) {
+		if (!shouldFallbackToSettingsBackup(primaryExists, error)) {
+			throw error;
+		}
 		const backupRecord = readSettingsBackupSync();
 		if (backupRecord) {
+			backupDerivedReadPending = true;
 			return backupRecord;
 		}
 		throw error;
 	}
 
-	return readSettingsBackupSync();
+	return null;
 }
 
 /**
@@ -156,6 +220,7 @@ function readSettingsRecordSync(): JsonRecord | null {
  * @returns The parsed settings record as an object clone, or `null` if unavailable or invalid.
  */
 async function readSettingsRecordAsync(): Promise<JsonRecord | null> {
+	const primaryExists = existsSync(UNIFIED_SETTINGS_PATH);
 	try {
 		const primaryRecord = await readSettingsRecordAsyncFromPath(
 			UNIFIED_SETTINGS_PATH,
@@ -164,14 +229,18 @@ async function readSettingsRecordAsync(): Promise<JsonRecord | null> {
 			return primaryRecord;
 		}
 	} catch (error) {
+		if (!shouldFallbackToSettingsBackup(primaryExists, error)) {
+			throw error;
+		}
 		const backupRecord = await readSettingsBackupAsync();
 		if (backupRecord) {
+			backupDerivedReadPending = true;
 			return backupRecord;
 		}
 		throw error;
 	}
 
-	return readSettingsBackupAsync();
+	return null;
 }
 
 /**
@@ -219,6 +288,7 @@ function writeSettingsRecordSync(record: JsonRecord): void {
 			try {
 				renameSync(tempPath, UNIFIED_SETTINGS_PATH);
 				moved = true;
+				backupDerivedReadPending = false;
 				return;
 			} catch (error) {
 				if (!isRetryableFsError(error) || attempt >= 4) {
@@ -271,6 +341,7 @@ async function writeSettingsRecordAsync(record: JsonRecord): Promise<void> {
 			try {
 				await fs.rename(tempPath, UNIFIED_SETTINGS_PATH);
 				moved = true;
+				backupDerivedReadPending = false;
 				return;
 			} catch (error) {
 				if (!isRetryableFsError(error) || attempt >= 4) {

test/config-save.test.ts

@@ -161,6 +161,35 @@ describe("plugin config save paths", () => {
     ).toBe(1);
   });
 
+  it("does not warn for blank numeric env overrides", async () => {
+    process.env.CODEX_AUTH_PARALLEL_PROBING_MAX_CONCURRENCY = "";
+    vi.resetModules();
+    const logWarnMock = vi.fn();
+
+    vi.doMock("../lib/logger.js", async () => {
+      const actual =
+        await vi.importActual<typeof import("../lib/logger.js")>(
+          "../lib/logger.js",
+        );
+      return {
+        ...actual,
+        logWarn: logWarnMock,
+      };
+    });
+
+    try {
+      const { getParallelProbingMaxConcurrency } = await import(
+        "../lib/config.js"
+      );
+      expect(
+        getParallelProbingMaxConcurrency({ parallelProbingMaxConcurrency: 4 }),
+      ).toBe(4);
+      expect(logWarnMock).not.toHaveBeenCalled();
+    } finally {
+      vi.doUnmock("../lib/logger.js");
+    }
+  });
+
   it("normalizes fallback chain and drops invalid entries", async () => {
     const { getUnsupportedCodexFallbackChain } =
       await import("../lib/config.js");

test/plugin-config.test.ts

@@ -663,11 +663,11 @@ describe('Plugin Configuration', () => {
 
 		it('should ignore invalid env values and fall back to config/default', () => {
 			process.env.CODEX_MODE = 'maybe';
-			const config: PluginConfig = { codexMode: true };
+			const config: PluginConfig = { codexMode: false };
 
 			const result = getCodexMode(config);
 
-			expect(result).toBe(true);
+			expect(result).toBe(false);
 		});
 
 		it('should use config codexMode=true when explicitly set', () => {
@@ -1036,6 +1036,7 @@ describe('Plugin Configuration', () => {
 			process.env.CODEX_AUTH_FETCH_TIMEOUT_MS = '';
 			expect(getFetchTimeoutMs({ fetchTimeoutMs: 90000 })).toBe(90000);
 			delete process.env.CODEX_AUTH_FETCH_TIMEOUT_MS;
+			expect(getFetchTimeoutMs({})).toBe(60000);
 		});
 
 		it('should read stream stall timeout from env', () => {

test/storage-flagged.test.ts

@@ -341,6 +341,43 @@ describe("flagged account storage", () => {
 		readSpy.mockRestore();
 	});
 
+	it("honors the reset marker even when it appears during backup recovery", async () => {
+		const backupPath = `${getFlaggedAccountsPath()}.bak`;
+		const resetMarkerPath = `${getFlaggedAccountsPath()}.reset-intent`;
+		await fs.writeFile(
+			backupPath,
+			JSON.stringify({
+				version: 1,
+				accounts: [
+					{
+						refreshToken: "backup-race",
+						flaggedAt: 1,
+						addedAt: 1,
+						lastUsed: 1,
+					},
+				],
+			}),
+			"utf8",
+		);
+
+		const originalReadFile = fs.readFile.bind(fs);
+		const readSpy = vi.spyOn(fs, "readFile").mockImplementation(async (...args) => {
+			const [targetPath] = args;
+			const result = await originalReadFile(...args);
+			if (targetPath === backupPath) {
+				await fs.writeFile(resetMarkerPath, "reset", "utf8");
+			}
+			return result;
+		});
+
+		try {
+			const flagged = await loadFlaggedAccounts();
+			expect(flagged.accounts).toHaveLength(0);
+		} finally {
+			readSpy.mockRestore();
+		}
+	});
+
 	it("clears discovered flagged backup artifacts so manual snapshots cannot revive after clear", async () => {
 		await saveFlaggedAccounts({
 			version: 1,