fix: address latest review follow-ups

Author: ndycode

lib/storage/flagged-storage-io.ts

@@ -106,6 +106,7 @@ export async function loadFlaggedAccountsState(params: {
 							to: params.path,
 							error: String(persistError),
 						});
+						return recovered;
 					}
 				}
 				params.logInfo("Recovered flagged account storage from backup", {

lib/unified-settings.ts

@@ -53,10 +53,16 @@ function cloneRecord(value: unknown): JsonRecord | null {
 	return { ...value };
 }
 
+class InvalidSettingsRecordError extends Error {
+	override readonly name = "InvalidSettingsRecordError";
+}
+
 function parseSettingsRecord(content: string): JsonRecord {
 	const parsed = cloneRecord(JSON.parse(content));
 	if (!parsed) {
-		throw new Error("Unified settings must contain a JSON object at the root.");
+		throw new InvalidSettingsRecordError(
+			"Unified settings must contain a JSON object at the root.",
+		);
 	}
 	return parsed;
 }
@@ -157,10 +163,7 @@ function isInvalidSettingsRecordError(error: unknown): boolean {
 	if (error instanceof SyntaxError) {
 		return true;
 	}
-	return (
-		error instanceof Error &&
-		error.message === "Unified settings must contain a JSON object at the root."
-	);
+	return error instanceof InvalidSettingsRecordError;
 }
 
 /**

scripts/codex.js

@@ -32,6 +32,8 @@ let shadowHomeCleanupPreflightReadBusyFailuresRemaining = Number.parseInt(
 	process.env.CODEX_MULTI_AUTH_TEST_SHADOW_PREFLIGHT_READ_BUSY_FAILURES ?? "0",
 	10,
 );
+const shadowHomeCleanupRetryMarkerDir =
+	(process.env.CODEX_MULTI_AUTH_TEST_SHADOW_RETRY_MARKER_DIR ?? "").trim();
 
 function isRetryableShadowHomeCleanupError(error) {
 	const code = error && typeof error === "object" && "code" in error ? error.code : undefined;
@@ -246,6 +248,25 @@ function maybeThrowSimulatedShadowHomePreflightReadBusyError() {
 	}
 }
 
+function writeShadowHomeCleanupRetryMarker(destinationPath, attempt) {
+	if (shadowHomeCleanupRetryMarkerDir.length === 0) {
+		return;
+	}
+	try {
+		mkdirSync(shadowHomeCleanupRetryMarkerDir, { recursive: true });
+		writeFileSync(
+			join(
+				shadowHomeCleanupRetryMarkerDir,
+				`${basename(destinationPath)}.retry-${attempt + 1}`,
+			),
+			`${attempt + 1}\n`,
+			"utf8",
+		);
+	} catch {
+		// Best-effort test hook only.
+	}
+}
+
 function ensureShadowHomeDestinationMatchesSnapshot(destinationPath, expectedState) {
 	if (!expectedState) {
 		return;
@@ -277,6 +298,7 @@ function renameFileWithRetry(sourcePath, destinationPath, expectedDestinationSta
 			) {
 				throw error;
 			}
+			writeShadowHomeCleanupRetryMarker(destinationPath, attempt);
 			sleepSync(SHADOW_HOME_CLEANUP_BACKOFF_MS[attempt]);
 		}
 	}

test/codex-bin-wrapper.test.ts

@@ -548,28 +548,41 @@ describe("codex bin wrapper", () => {
 
 	it("does not clobber sync-back files that change during rename retry backoff", () => {
 		const fixtureRoot = createWrapperFixture();
+		const retryMarkerDir = join(fixtureRoot, "retry-markers");
+		const accountsRetryMarker = join(retryMarkerDir, "accounts.json.retry-1");
 		const fakeBin = createCustomFakeCodexBin(fixtureRoot, [
 			"#!/usr/bin/env node",
 			'const { spawn } = require("node:child_process");',
 			'const fs = require("node:fs");',
 			'const path = require("node:path");',
 			'const home = process.env.CODEX_HOME ?? "";',
+			'const retryMarker = process.env.CODEX_MULTI_AUTH_TEST_RETRY_MARKER ?? "";',
 			'const originalHome = process.env.CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME ?? "";',
-			'fs.writeFileSync(path.join(home, "auth.json"), \'{"token":"shadow"}\\n\', "utf8");',
 			'fs.writeFileSync(path.join(home, "accounts.json"), \'{"accounts":["shadow"]}\\n\', "utf8");',
 			'fs.writeFileSync(path.join(home, ".codex-global-state.json"), \'{"last":"shadow"}\\n\', "utf8");',
-			"if (originalHome) {",
+			"if (originalHome && retryMarker) {",
 			"  const mutateScript = [",
 			'    \'const fs = require("node:fs");\',',
 			'    \'const path = require("node:path");\',',
-			'    \'const target = process.argv[1];\',',
-			'    \'setTimeout(() => {\',',
+			'    \'const markerPath = process.argv[1];\',',
+			'    \'const target = process.argv[2];\',',
+			'    \'const startedAt = Date.now();\',',
+			'    \'const waitForMarker = () => {\',',
+			'    \'  if (fs.existsSync(markerPath)) {\',',
 			'    \'  fs.writeFileSync(path.join(target, \"accounts.json\"), \"{\\\\\"accounts\\\\\":[\\\\\"external-during-retry\\\\\"]}\\\\n\", \"utf8\");\',',
 			'    \'  fs.writeFileSync(path.join(target, \".codex-global-state.json\"), \"{\\\\\"last\\\\\":\\\\\"external-during-retry\\\\\"}\\\\n\", \"utf8\");\',',
 			'    \'  process.exit(0);\',',
-			'    \'}, 40);\',',
+			'    \'    return;\',',
+			'    \'  }\',',
+			'    \'  if (Date.now() - startedAt > 5000) {\',',
+			'    \'    process.exit(2);\',',
+			'    \'    return;\',',
+			'    \'  }\',',
+			'    \'  setTimeout(waitForMarker, 5);\',',
+			'    \'};\',',
+			'    \'waitForMarker();\',',
 			"  ].join(\"\\n\");",
-			"  const mutator = spawn(process.execPath, [\"-e\", mutateScript, originalHome], {",
+			"  const mutator = spawn(process.execPath, [\"-e\", mutateScript, retryMarker, originalHome], {",
 			"    detached: true,",
 			'    stdio: "ignore",',
 			"  });",
@@ -581,6 +594,7 @@ describe("codex bin wrapper", () => {
 		const controlledTmp = join(fixtureRoot, "tmp");
 		mkdirSync(originalHome, { recursive: true });
 		mkdirSync(controlledTmp, { recursive: true });
+		mkdirSync(retryMarkerDir, { recursive: true });
 		writeFileSync(join(originalHome, "auth.json"), '{"token":"original"}\n', "utf8");
 		writeFileSync(join(originalHome, "accounts.json"), '{"accounts":["original"]}\n', "utf8");
 		writeFileSync(join(originalHome, ".codex-global-state.json"), '{"last":"original"}\n', "utf8");
@@ -593,6 +607,8 @@ describe("codex bin wrapper", () => {
 				CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin,
 				CODEX_HOME: originalHome,
 				CODEX_MULTI_AUTH_TEST_EXTERNAL_HOME: originalHome,
+				CODEX_MULTI_AUTH_TEST_RETRY_MARKER: accountsRetryMarker,
+				CODEX_MULTI_AUTH_TEST_SHADOW_RETRY_MARKER_DIR: retryMarkerDir,
 				TMP: controlledTmp,
 				TEMP: controlledTmp,
 				TMPDIR: controlledTmp,
@@ -601,7 +617,7 @@ describe("codex bin wrapper", () => {
 		);
 
 		expect(result.status).toBe(0);
-		expect(readFileSync(join(originalHome, "auth.json"), "utf8").trim()).toBe('{"token":"shadow"}');
+		expect(readFileSync(join(originalHome, "auth.json"), "utf8").trim()).toBe('{"token":"original"}');
 		expect(readFileSync(join(originalHome, "accounts.json"), "utf8").trim()).toBe(
 			'{"accounts":["external-during-retry"]}',
 		);

test/storage-flagged.test.ts

@@ -333,12 +333,12 @@ describe("flagged account storage", () => {
 				return originalReadFile(...args);
 			});
 
-	const flagged = await loadFlaggedAccounts();
-	expect(flagged.accounts).toHaveLength(1);
-	expect(flagged.accounts[0]?.refreshToken).toBe("primary-flagged");
-	expect(existsSync(flaggedPath)).toBe(true);
+		const flagged = await loadFlaggedAccounts();
+		expect(flagged.accounts).toHaveLength(1);
+		expect(flagged.accounts[0]?.refreshToken).toBe("primary-flagged");
+		expect(existsSync(flaggedPath)).toBe(true);
 
-	readSpy.mockRestore();
+		readSpy.mockRestore();
 	});
 
 	it("retries transient flagged primary read errors before falling back to backup", async () => {
@@ -716,4 +716,73 @@ describe("flagged storage extracted helpers", () => {
 			expect.objectContaining({ path: "flagged.json" }),
 		);
 	});
+
+	it("does not log successful backup recovery when persisting the recovery fails", async () => {
+		const { loadFlaggedAccountsState } = await import(
+			"../lib/storage/flagged-storage-io.js"
+		);
+		const fixtureRoot = join(
+			tmpdir(),
+			`codex-flagged-io-${Math.random().toString(36).slice(2)}`,
+		);
+		const flaggedPath = join(fixtureRoot, "flagged.json");
+		const resetMarkerPath = `${flaggedPath}.reset`;
+		const logError = vi.fn();
+		const logInfo = vi.fn();
+
+		try {
+			await fs.mkdir(fixtureRoot, { recursive: true });
+			await fs.writeFile(
+				`${flaggedPath}.bak`,
+				JSON.stringify(
+					{
+						version: 1,
+						accounts: [
+							{
+								refreshToken: "backup-token",
+								flaggedAt: 1,
+								addedAt: 1,
+								lastUsed: 1,
+							},
+						],
+					},
+					null,
+					2,
+				),
+				"utf8",
+			);
+
+			await expect(
+				loadFlaggedAccountsState({
+					path: flaggedPath,
+					legacyPath: `${flaggedPath}.legacy`,
+					resetMarkerPath,
+					normalizeFlaggedStorage: (data) => data as never,
+					persistRecoveredBackup: vi.fn(async () => {
+						throw new Error("persist failed");
+					}),
+					saveFlaggedAccounts: vi.fn(async () => {}),
+					logError,
+					logInfo,
+				}),
+			).resolves.toEqual({
+				version: 1,
+				accounts: [
+					{
+						refreshToken: "backup-token",
+						flaggedAt: 1,
+						addedAt: 1,
+						lastUsed: 1,
+					},
+				],
+			});
+			expect(logError).toHaveBeenCalledWith(
+				"Failed to persist recovered flagged account storage",
+				expect.objectContaining({ from: `${flaggedPath}.bak`, to: flaggedPath }),
+			);
+			expect(logInfo).not.toHaveBeenCalled();
+		} finally {
+			await removeWithRetry(fixtureRoot, { recursive: true, force: true });
+		}
+	});
 });