Skip to content

Commit 77edc30

Browse files
ndycodendycode
authored
Merge pull request #95 from ndycode/fix/storage-identity-contracts
fix: tighten storage identity and persistence contracts
2 parents 8336f16 + 4e2666a commit 77edc30

2 files changed

Lines changed: 95 additions & 76 deletions

File tree

‎lib/storage.ts‎

Lines changed: 60 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { promises as fs, existsSync } from "node:fs";
22
import { randomBytes } from "node:crypto";
3-
import { dirname, join } from "node:path";
3+
import { basename, dirname, join } from "node:path";
44
import { ACCOUNT_LIMITS } from "./constants.js";
55
import { createLogger } from "./logger.js";
66
import { MODEL_FAMILIES, type ModelFamily } from "./prompts/codex.js";
@@ -84,6 +84,12 @@ export interface ImportAccountsResult {
8484
backupError?: string;
8585
}
8686

87+
export interface ImportPreviewResult {
88+
imported: number;
89+
total: number;
90+
skipped: number;
91+
}
92+
8793
/**
8894
* Custom error class for storage operations with platform-aware hints.
8995
*/
@@ -1199,31 +1205,47 @@ async function readAndNormalizeImportFile(filePath: string): Promise<{
11991205
return { resolvedPath, normalized };
12001206
}
12011207

1208+
function analyzeImportedAccounts(
1209+
existingAccounts: AccountMetadataV3[],
1210+
importedAccounts: AccountStorageV3["accounts"],
1211+
): ImportPreviewResult & { accounts: AccountMetadataV3[] } {
1212+
const merged = [...existingAccounts, ...importedAccounts];
1213+
const accounts = deduplicateAccountsForStorage(merged);
1214+
if (accounts.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1215+
throw new Error(
1216+
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${accounts.length})`,
1217+
);
1218+
}
1219+
const imported = Math.max(0, accounts.length - existingAccounts.length);
1220+
const skipped = Math.max(0, importedAccounts.length - imported);
1221+
return {
1222+
accounts,
1223+
imported,
1224+
total: accounts.length,
1225+
skipped,
1226+
};
1227+
}
1228+
1229+
/**
1230+
* Import preview/apply analysis is pure in-memory work: it does not touch disk
1231+
* and it does not log token or workspace values. The surrounding
1232+
* `withAccountStorageTransaction` caller keeps Windows lock-retry and
1233+
* serialized read-modify-write behavior; see `test/storage.test.ts` for the
1234+
* overlapping transaction regression and pre-import backup lock coverage.
1235+
*/
1236+
12021237
export async function previewImportAccounts(
12031238
filePath: string,
1204-
): Promise<{ imported: number; total: number; skipped: number }> {
1239+
): Promise<ImportPreviewResult> {
12051240
const { normalized } = await readAndNormalizeImportFile(filePath);
12061241

12071242
return withAccountStorageTransaction((existing) => {
12081243
const existingAccounts = existing?.accounts ?? [];
1209-
const merged = [...existingAccounts, ...normalized.accounts];
1210-
1211-
if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1212-
const deduped = deduplicateAccountsForStorage(merged);
1213-
if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1214-
throw new Error(
1215-
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`,
1216-
);
1217-
}
1218-
}
1219-
1220-
const deduplicatedAccounts = deduplicateAccountsForStorage(merged);
1221-
const imported = deduplicatedAccounts.length - existingAccounts.length;
1222-
const skipped = normalized.accounts.length - imported;
1244+
const analysis = analyzeImportedAccounts(existingAccounts, normalized.accounts);
12231245
return Promise.resolve({
1224-
imported,
1225-
total: deduplicatedAccounts.length,
1226-
skipped,
1246+
imported: analysis.imported,
1247+
total: analysis.total,
1248+
skipped: analysis.skipped,
12271249
});
12281250
});
12291251
}
@@ -1294,6 +1316,7 @@ export async function importAccounts(
12941316
let backupStatus: ImportBackupStatus = "skipped";
12951317
let backupPath: string | undefined;
12961318
let backupError: string | undefined;
1319+
let backupLogError: string | undefined;
12971320
if (backupMode !== "none" && existingAccounts.length > 0) {
12981321
backupPath = createTimestampedBackupPath(backupPrefix);
12991322
try {
@@ -1302,28 +1325,26 @@ export async function importAccounts(
13021325
} catch (error) {
13031326
backupStatus = "failed";
13041327
backupError = error instanceof Error ? error.message : String(error);
1328+
const backupCode = (error as NodeJS.ErrnoException)?.code;
1329+
backupLogError = backupCode
1330+
? `pre-import backup failed (${backupCode})`
1331+
: "pre-import backup failed";
13051332
if (backupMode === "required") {
1306-
throw new Error(`Pre-import backup failed: ${backupError}`);
1333+
throw new Error(
1334+
backupCode
1335+
? `Pre-import backup failed (${backupCode})`
1336+
: "Pre-import backup failed",
1337+
);
13071338
}
13081339
log.warn("Pre-import backup failed; continuing import apply", {
1309-
path: backupPath,
1310-
error: backupError,
1340+
backupFile: backupPath ? basename(backupPath) : undefined,
1341+
error: backupLogError,
13111342
});
13121343
}
13131344
}
13141345

1315-
const merged = [...existingAccounts, ...normalized.accounts];
1316-
1317-
if (merged.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1318-
const deduped = deduplicateAccountsForStorage(merged);
1319-
if (deduped.length > ACCOUNT_LIMITS.MAX_ACCOUNTS) {
1320-
throw new Error(
1321-
`Import would exceed maximum of ${ACCOUNT_LIMITS.MAX_ACCOUNTS} accounts (would have ${deduped.length})`
1322-
);
1323-
}
1324-
}
1325-
1326-
const deduplicatedAccounts = deduplicateAccountsForStorage(merged);
1346+
const analysis = analyzeImportedAccounts(existingAccounts, normalized.accounts);
1347+
const deduplicatedAccounts = analysis.accounts;
13271348

13281349
const mappedActiveIndex = (() => {
13291350
if (deduplicatedAccounts.length === 0) return 0;
@@ -1359,12 +1380,10 @@ export async function importAccounts(
13591380

13601381
await persist(newStorage);
13611382

1362-
const imported = deduplicatedAccounts.length - existingAccounts.length;
1363-
const skipped = normalized.accounts.length - imported;
13641383
return {
1365-
imported,
1366-
total: deduplicatedAccounts.length,
1367-
skipped,
1384+
imported: analysis.imported,
1385+
total: analysis.total,
1386+
skipped: analysis.skipped,
13681387
backupStatus,
13691388
backupPath,
13701389
backupError,
@@ -1377,8 +1396,8 @@ export async function importAccounts(
13771396
skipped: skippedCount,
13781397
total,
13791398
backupStatus,
1380-
backupPath,
1381-
backupError,
1399+
backupFile: backupPath ? basename(backupPath) : undefined,
1400+
backupError: backupError ? "available on command result" : undefined,
13821401
});
13831402

13841403
return {

‎test/storage.test.ts‎

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
22
import { promises as fs, existsSync } from "node:fs";
33
import { basename, dirname, join } from "node:path";
44
import { tmpdir } from "node:os";
5-
import {
5+
import {
66
deduplicateAccounts,
77
deduplicateAccountsByEmail,
88
normalizeAccountStorage,
@@ -25,11 +25,6 @@ import {
2525
withFlaggedAccountStorageTransaction,
2626
} from "../lib/storage.js";
2727

28-
// Mocking the behavior we're about to implement for TDD
29-
// Since the functions aren't in lib/storage.ts yet, we'll need to mock them or
30-
// accept that this test won't even compile/run until we add them.
31-
// But Task 0 says: "Tests should fail initially (RED phase)"
32-
3328
describe("storage", () => {
3429
describe("getWorkspaceIdentityKey", () => {
3530
it("uses organizationId and accountId when both are present", () => {
@@ -141,56 +136,43 @@ describe("storage", () => {
141136
});
142137

143138
it("should export accounts to a file", async () => {
144-
// @ts-ignore - exportAccounts doesn't exist yet
145-
const { exportAccounts } = await import("../lib/storage.js");
146-
147139
const storage = {
148140
version: 3,
149141
activeIndex: 0,
150142
accounts: [{ accountId: "test", refreshToken: "ref", addedAt: 1, lastUsed: 2 }]
151143
};
152-
// @ts-ignore
153144
await saveAccounts(storage);
154-
155-
// @ts-ignore
145+
156146
await exportAccounts(exportPath);
157-
147+
158148
expect(existsSync(exportPath)).toBe(true);
159149
const exported = JSON.parse(await fs.readFile(exportPath, "utf-8"));
160150
expect(exported.accounts[0].accountId).toBe("test");
161151
});
162152

163153
it("should fail export if file exists and force is false", async () => {
164-
// @ts-ignore
165-
const { exportAccounts } = await import("../lib/storage.js");
166154
await fs.writeFile(exportPath, "exists");
167-
168-
// @ts-ignore
155+
169156
await expect(exportAccounts(exportPath, false)).rejects.toThrow(/already exists/);
170157
});
171158

172159
it("should import accounts from a file and merge", async () => {
173-
// @ts-ignore
174-
const { importAccounts } = await import("../lib/storage.js");
175-
176160
const existing = {
177161
version: 3,
178162
activeIndex: 0,
179163
accounts: [{ accountId: "existing", refreshToken: "ref1", addedAt: 1, lastUsed: 2 }]
180164
};
181-
// @ts-ignore
182165
await saveAccounts(existing);
183-
166+
184167
const toImport = {
185168
version: 3,
186169
activeIndex: 0,
187170
accounts: [{ accountId: "new", refreshToken: "ref2", addedAt: 3, lastUsed: 4 }]
188171
};
189172
await fs.writeFile(exportPath, JSON.stringify(toImport));
190-
191-
// @ts-ignore
173+
192174
await importAccounts(exportPath);
193-
175+
194176
const loaded = await loadAccounts();
195177
expect(loaded?.accounts).toHaveLength(2);
196178
expect(loaded?.accounts.map(a => a.accountId)).toContain("new");
@@ -222,6 +204,32 @@ describe("storage", () => {
222204
expect(loaded?.accounts[0]?.accountId).toBe("existing");
223205
});
224206

207+
it("keeps preview and apply counts aligned for the same import fixture", async () => {
208+
await saveAccounts({
209+
version: 3,
210+
activeIndex: 0,
211+
accounts: [{ accountId: "existing", refreshToken: "ref1", addedAt: 1, lastUsed: 2 }],
212+
});
213+
214+
await fs.writeFile(
215+
exportPath,
216+
JSON.stringify({
217+
version: 3,
218+
activeIndex: 0,
219+
accounts: [{ accountId: "preview", refreshToken: "ref2", addedAt: 3, lastUsed: 4 }],
220+
}),
221+
);
222+
223+
const preview = await previewImportAccounts(exportPath);
224+
const applied = await importAccounts(exportPath);
225+
226+
expect(preview).toMatchObject({
227+
imported: applied.imported,
228+
skipped: applied.skipped,
229+
total: applied.total,
230+
});
231+
});
232+
225233
it("creates timestamped backup paths in storage backups directory", () => {
226234
const path = createTimestampedBackupPath();
227235
const expectedBackupDir = join(dirname(testStoragePath), "backups");
@@ -625,9 +633,6 @@ describe("storage", () => {
625633
});
626634

627635
it("should enforce MAX_ACCOUNTS during import", async () => {
628-
// @ts-ignore
629-
const { importAccounts } = await import("../lib/storage.js");
630-
631636
const manyAccounts = Array.from({ length: 21 }, (_, i) => ({
632637
accountId: `acct${i}`,
633638
refreshToken: `ref${i}`,
@@ -641,31 +646,26 @@ describe("storage", () => {
641646
accounts: manyAccounts
642647
};
643648
await fs.writeFile(exportPath, JSON.stringify(toImport));
644-
645-
// @ts-ignore
649+
646650
await expect(importAccounts(exportPath)).rejects.toThrow(/exceed maximum/);
647651
});
648652

649653
it("should fail export when no accounts exist", async () => {
650-
const { exportAccounts } = await import("../lib/storage.js");
651654
setStoragePathDirect(testStoragePath);
652655
await expect(exportAccounts(exportPath)).rejects.toThrow(/No accounts to export/);
653656
});
654657

655658
it("should fail import when file does not exist", async () => {
656-
const { importAccounts } = await import("../lib/storage.js");
657659
const nonexistentPath = join(testWorkDir, "nonexistent-file.json");
658660
await expect(importAccounts(nonexistentPath)).rejects.toThrow(/Import file not found/);
659661
});
660662

661663
it("should fail import when file contains invalid JSON", async () => {
662-
const { importAccounts } = await import("../lib/storage.js");
663664
await fs.writeFile(exportPath, "not valid json {[");
664665
await expect(importAccounts(exportPath)).rejects.toThrow(/Invalid JSON/);
665666
});
666667

667668
it("should fail import when file contains invalid format", async () => {
668-
const { importAccounts } = await import("../lib/storage.js");
669669
await fs.writeFile(exportPath, JSON.stringify({ invalid: "format" }));
670670
await expect(importAccounts(exportPath)).rejects.toThrow(/Invalid account storage format/);
671671
});
@@ -756,7 +756,7 @@ describe("storage", () => {
756756
preImportBackupPrefix: "codex-pre-import-backup",
757757
backupMode: "required",
758758
}),
759-
).rejects.toThrow(/Pre-import backup failed: Timed out writing file/);
759+
).rejects.toThrow("Pre-import backup failed");
760760
} finally {
761761
writeSpy.mockRestore();
762762
}

0 commit comments

Comments
 (0)