From 3566d0fee32451a6457f90e1a0480189add6f750 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 08:33:13 +0800 Subject: [PATCH 1/6] fix(storage): share per-project accounts across git worktrees --- docs/reference/storage-paths.md | 6 +- docs/troubleshooting.md | 1 + docs/upgrade.md | 8 ++ lib/storage.ts | 156 ++++++++++++++++++----- lib/storage/paths.ts | 81 +++++++++++- test/paths.test.ts | 111 ++++++++++++++-- test/storage.test.ts | 219 ++++++++++++++++++++++++++++++++ 7 files changed, 541 insertions(+), 41 deletions(-) diff --git a/docs/reference/storage-paths.md b/docs/reference/storage-paths.md index 713231075..323fcacaa 100644 --- a/docs/reference/storage-paths.md +++ b/docs/reference/storage-paths.md @@ -44,7 +44,11 @@ When project-scoped behavior is enabled: - `~/.codex/multi-auth/projects//openai-codex-accounts.json` -`` is derived from normalized project path + short hash. +`` is derived from project identity + short hash. + +- Standard repositories: identity is the project root path. +- Linked Git worktrees: identity is the shared repository root, so all worktrees for the same repo share one account pool. +- Non-Git directories: identity falls back to the detected project path. --- diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 74bf6b633..b8d2ce895 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -27,6 +27,7 @@ codex auth login | Browser opens during login | Expected OAuth behavior | Complete auth and return to terminal | | `codex auth` unrecognized | Wrapper command path conflict | Run `where codex`, then `codex multi auth status` | | Switch says success but wrong account in Codex | Stale Codex auth state sync | Run `codex auth switch `, restart `codex` session | +| Opening a PR worktree asks for login again | Worktree was using a different legacy path key | Run `codex auth list` once in the worktree to trigger migration into repo-shared storage | | `missing field id_token` | Stale auth state payload | Re-login account with `codex auth login` | | `refresh_token_reused` | Token pair already rotated | Re-login that account | | `token_expired` | Token no longer valid | Re-login that account | diff --git a/docs/upgrade.md b/docs/upgrade.md index 2db990283..14627cba3 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -56,6 +56,14 @@ They are not canonical and should not be used for new setup. See [reference/storage-paths.md](reference/storage-paths.md). +### Worktree Storage Migration + +If you used `perProjectAccounts=true` before worktree identity sharing was added, older worktree-keyed account files are migrated automatically on first load: + +- Legacy worktree storage is merged into the canonical repo-shared project file. +- Legacy files are removed only after a successful canonical write. +- If canonical persistence fails, legacy files are retained to avoid data loss. + --- ## Common Upgrade Problems diff --git a/lib/storage.ts b/lib/storage.ts index e53034c10..18270d5bf 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -5,7 +5,14 @@ import { ACCOUNT_LIMITS } from "./constants.js"; import { createLogger } from "./logger.js"; import { MODEL_FAMILIES, type ModelFamily } from "./prompts/codex.js"; import { AnyAccountStorageSchema, getValidationErrors } from "./schemas.js"; -import { getConfigDir, getProjectConfigDir, getProjectGlobalConfigDir, findProjectRoot, resolvePath } from "./storage/paths.js"; +import { + getConfigDir, + getProjectConfigDir, + getProjectGlobalConfigDir, + findProjectRoot, + resolvePath, + resolveProjectStorageIdentityRoot, +} from "./storage/paths.js"; import { migrateV1ToV3, type CooldownReason, @@ -138,6 +145,7 @@ async function ensureGitignore(storagePath: string): Promise { let currentStoragePath: string | null = null; let currentLegacyProjectStoragePath: string | null = null; +let currentLegacyWorktreeStoragePath: string | null = null; let currentProjectRoot: string | null = null; export function setStorageBackupEnabled(enabled: boolean): void { @@ -172,6 +180,7 @@ export function setStoragePath(projectPath: string | null): void { if (!projectPath) { currentStoragePath = null; currentLegacyProjectStoragePath = null; + currentLegacyWorktreeStoragePath = null; currentProjectRoot = null; return; } @@ -179,11 +188,19 @@ export function setStoragePath(projectPath: string | null): void { const projectRoot = findProjectRoot(projectPath); if (projectRoot) { currentProjectRoot = projectRoot; - currentStoragePath = join(getProjectGlobalConfigDir(projectRoot), ACCOUNTS_FILE_NAME); + const identityRoot = resolveProjectStorageIdentityRoot(projectRoot); + currentStoragePath = join(getProjectGlobalConfigDir(identityRoot), ACCOUNTS_FILE_NAME); currentLegacyProjectStoragePath = join(getProjectConfigDir(projectRoot), ACCOUNTS_FILE_NAME); + const previousWorktreeScopedPath = join( + getProjectGlobalConfigDir(projectRoot), + ACCOUNTS_FILE_NAME, + ); + currentLegacyWorktreeStoragePath = + previousWorktreeScopedPath !== currentStoragePath ? previousWorktreeScopedPath : null; } else { currentStoragePath = null; currentLegacyProjectStoragePath = null; + currentLegacyWorktreeStoragePath = null; currentProjectRoot = null; } } @@ -191,6 +208,7 @@ export function setStoragePath(projectPath: string | null): void { export function setStoragePathDirect(path: string | null): void { currentStoragePath = path; currentLegacyProjectStoragePath = null; + currentLegacyWorktreeStoragePath = null; currentProjectRoot = null; } @@ -216,52 +234,126 @@ function getLegacyFlaggedAccountsPath(): string { async function migrateLegacyProjectStorageIfNeeded( persist: (storage: AccountStorageV3) => Promise = saveAccounts, ): Promise { - if ( - !currentStoragePath || - !currentLegacyProjectStoragePath || - currentLegacyProjectStoragePath === currentStoragePath || - !existsSync(currentLegacyProjectStoragePath) - ) { + if (!currentStoragePath) { return null; } - try { - const legacyContent = await fs.readFile(currentLegacyProjectStoragePath, "utf-8"); - const legacyData = JSON.parse(legacyContent) as unknown; - const normalized = normalizeAccountStorage(legacyData); - if (!normalized) return null; + const candidatePaths = [currentLegacyWorktreeStoragePath, currentLegacyProjectStoragePath] + .filter( + (path): path is string => typeof path === "string" && path.length > 0 && path !== currentStoragePath, + ) + .filter((path, index, all) => all.indexOf(path) === index); + + if (candidatePaths.length === 0) { + return null; + } + + let targetStorage = await loadNormalizedStorageFromPath(currentStoragePath, "current account storage"); + let migrated = false; + + for (const legacyPath of candidatePaths) { + if (!existsSync(legacyPath)) { + continue; + } + + const legacyStorage = await loadNormalizedStorageFromPath(legacyPath, "legacy account storage"); + if (!legacyStorage) { + continue; + } + + const mergedStorage = mergeStorageForMigration(targetStorage, legacyStorage); + const fallbackStorage = targetStorage ?? legacyStorage; + + try { + await persist(mergedStorage); + targetStorage = mergedStorage; + migrated = true; + } catch (error) { + targetStorage = fallbackStorage; + log.warn("Failed to persist migrated account storage", { + from: legacyPath, + to: currentStoragePath, + error: String(error), + }); + continue; + } - await persist(normalized); try { - await fs.unlink(currentLegacyProjectStoragePath); - log.info("Removed legacy project account storage file after migration", { - path: currentLegacyProjectStoragePath, + await fs.unlink(legacyPath); + log.info("Removed legacy account storage file after migration", { + path: legacyPath, }); } catch (unlinkError) { const code = (unlinkError as NodeJS.ErrnoException).code; if (code !== "ENOENT") { - log.warn("Failed to remove legacy project account storage file after migration", { - path: currentLegacyProjectStoragePath, + log.warn("Failed to remove legacy account storage file after migration", { + path: legacyPath, error: String(unlinkError), }); } } + log.info("Migrated legacy project account storage", { - from: currentLegacyProjectStoragePath, + from: legacyPath, to: currentStoragePath, - accounts: normalized.accounts.length, + accounts: mergedStorage.accounts.length, }); + } + + if (migrated) { + return targetStorage; + } + if (targetStorage && !existsSync(currentStoragePath)) { + return targetStorage; + } + return null; +} + +async function loadNormalizedStorageFromPath( + path: string, + label: string, +): Promise { + try { + const { normalized, schemaErrors } = await loadAccountsFromPath(path); + if (schemaErrors.length > 0) { + log.warn(`${label} schema validation warnings`, { + path, + errors: schemaErrors.slice(0, 5), + }); + } return normalized; } catch (error) { - log.warn("Failed to migrate legacy project account storage", { - from: currentLegacyProjectStoragePath, - to: currentStoragePath, - error: String(error), - }); + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + log.warn(`Failed to load ${label}`, { + path, + error: String(error), + }); + } return null; } } +function mergeStorageForMigration( + current: AccountStorageV3 | null, + incoming: AccountStorageV3, +): AccountStorageV3 { + if (!current) { + return incoming; + } + + const merged = normalizeAccountStorage({ + version: 3, + activeIndex: current.activeIndex, + activeIndexByFamily: current.activeIndexByFamily, + accounts: [...current.accounts, ...incoming.accounts], + }); + if (!merged) { + return current; + } + return merged; +} + function selectNewestAccount( current: T | undefined, candidate: T, @@ -573,8 +665,12 @@ async function loadAccountsFromJournal(path: string): Promise Promise) | null, ): Promise { + const path = getStoragePath(); + const migratedLegacyStorage = persistMigration + ? await migrateLegacyProjectStorageIfNeeded(persistMigration) + : null; + try { - const path = getStoragePath(); const { normalized, storedVersion, schemaErrors } = await loadAccountsFromPath(path); if (schemaErrors.length > 0) { log.warn("Account storage schema validation warnings", { errors: schemaErrors.slice(0, 5) }); @@ -593,10 +689,8 @@ async function loadAccountsInternal( return normalized; } catch (error) { const code = (error as NodeJS.ErrnoException).code; - const path = getStoragePath(); - if (code === "ENOENT" && persistMigration) { - const migrated = await migrateLegacyProjectStorageIfNeeded(persistMigration); - if (migrated) return migrated; + if (code === "ENOENT" && migratedLegacyStorage) { + return migratedLegacyStorage; } const recoveredFromWal = await loadAccountsFromJournal(path); diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index 2fab17880..a6f94dab7 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -3,7 +3,7 @@ * Extracted from storage.ts to reduce module size. */ -import { existsSync } from "node:fs"; +import { existsSync, readFileSync, statSync } from "node:fs"; import { createHash } from "node:crypto"; import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path"; import { homedir, tmpdir } from "node:os"; @@ -13,6 +13,35 @@ const PROJECT_MARKERS = [".git", "package.json", "Cargo.toml", "go.mod", "pyproj const PROJECTS_DIR = "projects"; const PROJECT_KEY_HASH_LENGTH = 12; +function parseGitDirPointer(pointerContent: string): string | null { + const firstLine = pointerContent.split(/\r?\n/, 1)[0]?.trim(); + if (!firstLine) return null; + const match = /^gitdir:\s*(.+)$/i.exec(firstLine); + if (!match?.[1]) return null; + const value = match[1].trim(); + return value.length > 0 ? value : null; +} + +function readGitCommonDir(gitDirPath: string): string { + const commonDirFile = join(gitDirPath, "commondir"); + if (!existsSync(commonDirFile)) { + return gitDirPath; + } + + try { + const raw = readFileSync(commonDirFile, "utf-8").trim(); + if (!raw) return gitDirPath; + return isAbsolute(raw) ? raw : resolve(gitDirPath, raw); + } catch { + return gitDirPath; + } +} + +function isWorktreeGitDirPath(gitDirPath: string): boolean { + const normalized = normalizeProjectPath(gitDirPath); + return normalized.includes("/.git/worktrees/"); +} + /** * Gets the path to the global Codex multi-auth configuration directory. * @@ -112,6 +141,56 @@ export function getProjectGlobalConfigDir(projectPath: string): string { return join(getConfigDir(), PROJECTS_DIR, getProjectStorageKey(projectPath)); } +/** + * Resolve a stable project identity root for account storage keying. + * + * For standard repositories, this returns `projectRoot` unchanged. + * For linked Git worktrees, this resolves to the shared repository root so + * multiple worktrees use the same per-project account key. + * + * @param projectRoot - Detected project root path (typically from findProjectRoot) + * @returns Identity root used for per-project storage key generation + */ +export function resolveProjectStorageIdentityRoot(projectRoot: string): string { + const gitEntryPath = join(projectRoot, ".git"); + if (!existsSync(gitEntryPath)) { + return projectRoot; + } + + try { + const gitEntryStat = statSync(gitEntryPath); + if (gitEntryStat.isDirectory()) { + return projectRoot; + } + if (!gitEntryStat.isFile()) { + return projectRoot; + } + + const gitPointer = readFileSync(gitEntryPath, "utf-8"); + const gitDirValue = parseGitDirPointer(gitPointer); + if (!gitDirValue) { + return projectRoot; + } + + const gitDirPath = isAbsolute(gitDirValue) + ? gitDirValue + : resolve(projectRoot, gitDirValue); + if (!isWorktreeGitDirPath(gitDirPath)) { + return projectRoot; + } + + const commonGitDir = readGitCommonDir(gitDirPath); + const candidateRepoRoot = dirname(commonGitDir); + if (!existsSync(join(candidateRepoRoot, ".git"))) { + return projectRoot; + } + + return candidateRepoRoot; + } catch { + return projectRoot; + } +} + export function isProjectDirectory(dir: string): boolean { return PROJECT_MARKERS.some((marker) => existsSync(join(dir, marker))); } diff --git a/test/paths.test.ts b/test/paths.test.ts index 67d29ed25..f70b5bd6d 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -4,20 +4,38 @@ import path from "node:path"; vi.mock("node:fs", () => ({ existsSync: vi.fn(), + readFileSync: vi.fn(), + statSync: vi.fn(), })); -import { existsSync } from "node:fs"; +import { existsSync, readFileSync, statSync } from "node:fs"; import { getConfigDir, getProjectConfigDir, getProjectGlobalConfigDir, getProjectStorageKey, + resolveProjectStorageIdentityRoot, isProjectDirectory, findProjectRoot, resolvePath, } from "../lib/storage/paths.js"; const mockedExistsSync = vi.mocked(existsSync); +const mockedReadFileSync = vi.mocked(readFileSync); +const mockedStatSync = vi.mocked(statSync); + +function buildMockStat({ + isDirectory, + isFile, +}: { + isDirectory: boolean; + isFile: boolean; +}): ReturnType { + return { + isDirectory: () => isDirectory, + isFile: () => isFile, + } as unknown as ReturnType; +} describe("Storage Paths Module", () => { const _origCODEX_HOME = process.env.CODEX_HOME; @@ -146,14 +164,91 @@ describe("Storage Paths Module", () => { }); }); - describe("getProjectGlobalConfigDir", () => { - it("returns ~/.codex/multi-auth/projects/", () => { - const projectPath = "/home/user/myproject"; - const result = getProjectGlobalConfigDir(projectPath); - expect(result).toContain(path.join(homedir(), ".codex", "multi-auth", "projects")); - expect(result).toContain("myproject-"); + describe("getProjectGlobalConfigDir", () => { + it("returns ~/.codex/multi-auth/projects/", () => { + const projectPath = "/home/user/myproject"; + const result = getProjectGlobalConfigDir(projectPath); + expect(result).toContain(path.join(homedir(), ".codex", "multi-auth", "projects")); + expect(result).toContain("myproject-"); + }); + }); + + describe("resolveProjectStorageIdentityRoot", () => { + it("returns project root for standard .git directory repos", () => { + const projectRoot = "/repo/main"; + mockedExistsSync.mockImplementation((candidate) => { + return candidate === path.join(projectRoot, ".git"); + }); + mockedStatSync.mockImplementation((candidate) => { + expect(candidate).toBe(path.join(projectRoot, ".git")); + return buildMockStat({ isDirectory: true, isFile: false }); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(projectRoot); + expect(mockedReadFileSync).not.toHaveBeenCalled(); + }); + + it("returns shared repository root for linked git worktrees", () => { + const projectRoot = path.resolve("repo", "worktrees", "pr-8"); + const gitEntry = path.join(projectRoot, ".git"); + const worktreeGitDir = path.resolve("repo", ".git", "worktrees", "pr-8"); + const commondirFile = path.join(worktreeGitDir, "commondir"); + const sharedRepoRoot = path.resolve("repo"); + const sharedGitDir = path.join(sharedRepoRoot, ".git"); + const normalize = (value: string) => + process.platform === "win32" ? value.toLowerCase() : value; + + mockedExistsSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") return false; + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) return true; + if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(sharedGitDir)) return true; + return false; + }); + mockedStatSync.mockImplementation((candidate) => { + expect(normalize(String(candidate))).toBe(normalize(gitEntry)); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") { + throw new Error(`Unexpected read path: ${String(candidate)}`); + } + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) { + return `gitdir: ${worktreeGitDir}\n`; + } + if (normalizedCandidate === normalize(commondirFile)) { + return "../..\n"; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(sharedRepoRoot); + }); + + it("keeps project root when .git file does not point to worktrees", () => { + const projectRoot = "/repo/submodule"; + const gitEntry = path.join(projectRoot, ".git"); + mockedExistsSync.mockImplementation((candidate) => candidate === gitEntry); + mockedStatSync.mockImplementation((candidate) => { + expect(candidate).toBe(gitEntry); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + expect(candidate).toBe(gitEntry); + return "gitdir: /repo/.git/modules/submodule\n"; + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(projectRoot); + }); }); - }); describe("isProjectDirectory", () => { const markers = [".git", "package.json", "Cargo.toml", "go.mod", "pyproject.toml", ".codex"]; diff --git a/test/storage.test.ts b/test/storage.test.ts index 6735ab721..bc5101612 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { promises as fs, existsSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; +import { getConfigDir, getProjectStorageKey } from "../lib/storage/paths.js"; import { deduplicateAccounts, deduplicateAccountsByEmail, @@ -767,6 +768,37 @@ describe("storage", () => { expect(path).toContain(".codex"); expect(path).toContain("projects"); }); + + it("uses the same storage path for main repo and linked worktree", async () => { + const testWorkDir = join(tmpdir(), "codex-worktree-key-" + Math.random().toString(36).slice(2)); + const fakeHome = join(testWorkDir, "home"); + const mainRepo = join(testWorkDir, "repo-main"); + const mainGitDir = join(mainRepo, ".git"); + const worktreeRepo = join(testWorkDir, "repo-pr-8"); + const worktreeGitDir = join(mainGitDir, "worktrees", "repo-pr-8"); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + try { + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + await fs.mkdir(mainGitDir, { recursive: true }); + await fs.mkdir(worktreeGitDir, { recursive: true }); + await fs.mkdir(worktreeRepo, { recursive: true }); + await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); + await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + + setStoragePath(mainRepo); + const mainPath = getStoragePath(); + setStoragePath(worktreeRepo); + const worktreePath = getStoragePath(); + expect(worktreePath).toBe(mainPath); + } finally { + setStoragePathDirect(null); + process.env.HOME = originalHome; + process.env.USERPROFILE = originalUserProfile; + await fs.rm(testWorkDir, { recursive: true, force: true }); + } + }); }); describe("getStoragePath", () => { @@ -1012,6 +1044,193 @@ describe("storage", () => { }); }); + describe("worktree-scoped storage migration", () => { + const testWorkDir = join(tmpdir(), "codex-worktree-migration-" + Math.random().toString(36).slice(2)); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + const originalMultiAuthDir = process.env.CODEX_MULTI_AUTH_DIR; + + type StoredAccountFixture = { + refreshToken: string; + accountId: string; + addedAt: number; + lastUsed: number; + }; + + const now = Date.now(); + const accountFromLegacy: StoredAccountFixture = { + refreshToken: "legacy-refresh", + accountId: "legacy-account", + addedAt: now, + lastUsed: now, + }; + const accountFromCanonical: StoredAccountFixture = { + refreshToken: "canonical-refresh", + accountId: "canonical-account", + addedAt: now + 1, + lastUsed: now + 1, + }; + + async function prepareWorktreeFixture(): Promise<{ + fakeHome: string; + mainRepo: string; + worktreeRepo: string; + }> { + const fakeHome = join(testWorkDir, "home"); + const mainRepo = join(testWorkDir, "repo-main"); + const worktreeRepo = join(testWorkDir, "repo-pr-8"); + const mainGitDir = join(mainRepo, ".git"); + const worktreeGitDir = join(mainGitDir, "worktrees", "repo-pr-8"); + + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + process.env.CODEX_MULTI_AUTH_DIR = join(fakeHome, ".codex", "multi-auth"); + + await fs.mkdir(mainGitDir, { recursive: true }); + await fs.mkdir(worktreeGitDir, { recursive: true }); + await fs.mkdir(worktreeRepo, { recursive: true }); + await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); + await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + + return { fakeHome, mainRepo, worktreeRepo }; + } + + function buildStorage(accounts: StoredAccountFixture[]) { + return { + version: 3 as const, + activeIndex: 0, + activeIndexByFamily: {}, + accounts, + }; + } + + beforeEach(async () => { + await fs.mkdir(testWorkDir, { recursive: true }); + }); + + afterEach(async () => { + setStoragePathDirect(null); + process.env.HOME = originalHome; + process.env.USERPROFILE = originalUserProfile; + process.env.CODEX_MULTI_AUTH_DIR = originalMultiAuthDir; + await fs.rm(testWorkDir, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it("migrates worktree-keyed storage to repo-shared canonical path", async () => { + const { worktreeRepo } = await prepareWorktreeFixture(); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const legacyWorktreePath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + expect(legacyWorktreePath).not.toBe(canonicalPath); + + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(worktreeRepo)), { + recursive: true, + }); + await fs.writeFile( + legacyWorktreePath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + + const loaded = await loadAccounts(); + + expect(loaded).not.toBeNull(); + expect(loaded?.accounts).toHaveLength(1); + expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); + expect(existsSync(canonicalPath)).toBe(true); + expect(existsSync(legacyWorktreePath)).toBe(false); + }); + + it("merges canonical and legacy worktree storage when both exist", async () => { + const { worktreeRepo } = await prepareWorktreeFixture(); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const legacyWorktreePath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(worktreeRepo)), { + recursive: true, + }); + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(join(testWorkDir, "repo-main"))), { + recursive: true, + }); + + await fs.writeFile( + canonicalPath, + JSON.stringify(buildStorage([accountFromCanonical]), null, 2), + "utf-8", + ); + await fs.writeFile( + legacyWorktreePath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + + const loaded = await loadAccounts(); + + expect(loaded).not.toBeNull(); + expect(loaded?.accounts).toHaveLength(2); + const accountIds = loaded?.accounts.map((account) => account.accountId) ?? []; + expect(accountIds).toContain("canonical-account"); + expect(accountIds).toContain("legacy-account"); + expect(existsSync(legacyWorktreePath)).toBe(false); + }); + + it("keeps legacy worktree file when migration persist fails", async () => { + const { worktreeRepo } = await prepareWorktreeFixture(); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const canonicalWalPath = `${canonicalPath}.wal`; + const legacyWorktreePath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(worktreeRepo)), { + recursive: true, + }); + await fs.writeFile( + legacyWorktreePath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + + const originalWriteFile = fs.writeFile.bind(fs); + const writeSpy = vi + .spyOn(fs, "writeFile") + .mockImplementation(async (...args: Parameters) => { + const [targetPath] = args; + if (typeof targetPath === "string" && targetPath === canonicalWalPath) { + const error = new Error("forced write failure") as NodeJS.ErrnoException; + error.code = "EACCES"; + throw error; + } + return originalWriteFile(...args); + }); + + const loaded = await loadAccounts(); + + writeSpy.mockRestore(); + expect(loaded).not.toBeNull(); + expect(loaded?.accounts).toHaveLength(1); + expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); + expect(existsSync(legacyWorktreePath)).toBe(true); + }); + }); + describe("saveAccounts EPERM/EBUSY retry logic", () => { const testWorkDir = join(tmpdir(), "codex-retry-" + Math.random().toString(36).slice(2)); let testStoragePath: string; From 8084ff2b300d7c414958d3a2a270a77b09fdd1b8 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 11:05:34 +0800 Subject: [PATCH 2/6] fix(review): address remaining coderabbit migration comments --- lib/storage.ts | 11 ++-- test/paths.test.ts | 85 ++++++++++++++++++++++++++++ test/storage.test.ts | 132 ++++++++++++++++++++++++++++++++++++++----- 3 files changed, 208 insertions(+), 20 deletions(-) diff --git a/lib/storage.ts b/lib/storage.ts index 18270d5bf..2c990135d 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -248,14 +248,15 @@ async function migrateLegacyProjectStorageIfNeeded( return null; } + const existingCandidatePaths = candidatePaths.filter((legacyPath) => existsSync(legacyPath)); + if (existingCandidatePaths.length === 0) { + return null; + } + let targetStorage = await loadNormalizedStorageFromPath(currentStoragePath, "current account storage"); let migrated = false; - for (const legacyPath of candidatePaths) { - if (!existsSync(legacyPath)) { - continue; - } - + for (const legacyPath of existingCandidatePaths) { const legacyStorage = await loadNormalizedStorageFromPath(legacyPath, "legacy account storage"); if (!legacyStorage) { continue; diff --git a/test/paths.test.ts b/test/paths.test.ts index f70b5bd6d..8eef42997 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -231,6 +231,91 @@ describe("Storage Paths Module", () => { expect(resolved).toBe(sharedRepoRoot); }); + it("resolves relative gitdir pointers to the shared repository root", () => { + const projectRoot = path.resolve("repo", "worktrees", "pr-relative"); + const gitEntry = path.join(projectRoot, ".git"); + const worktreeGitDir = path.resolve("repo", ".git", "worktrees", "pr-relative"); + const commondirFile = path.join(worktreeGitDir, "commondir"); + const sharedRepoRoot = path.resolve("repo"); + const sharedGitDir = path.join(sharedRepoRoot, ".git"); + const normalize = (value: string) => + process.platform === "win32" ? value.toLowerCase() : value; + + mockedExistsSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") return false; + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) return true; + if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(sharedGitDir)) return true; + return false; + }); + mockedStatSync.mockImplementation((candidate) => { + expect(normalize(String(candidate))).toBe(normalize(gitEntry)); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") { + throw new Error(`Unexpected read path: ${String(candidate)}`); + } + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) { + return "gitdir: ../../.git/worktrees/pr-relative\n"; + } + if (normalizedCandidate === normalize(commondirFile)) { + return "../..\n"; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(sharedRepoRoot); + }); + + it("supports Windows-style backslash gitdir pointers", () => { + if (process.platform !== "win32") { + return; + } + + const projectRoot = path.win32.join("C:\\repo", "worktrees", "pr-8"); + const gitEntry = path.win32.join(projectRoot, ".git"); + const worktreeGitDir = path.win32.join("C:\\repo", ".git", "worktrees", "pr-8"); + const commondirFile = path.win32.join(worktreeGitDir, "commondir"); + const sharedRepoRoot = "C:\\repo"; + const sharedGitDir = path.win32.join(sharedRepoRoot, ".git"); + const normalize = (value: string) => path.win32.normalize(value).toLowerCase(); + + mockedExistsSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") return false; + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) return true; + if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(sharedGitDir)) return true; + return false; + }); + mockedStatSync.mockImplementation((candidate) => { + expect(normalize(String(candidate))).toBe(normalize(gitEntry)); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") { + throw new Error(`Unexpected read path: ${String(candidate)}`); + } + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) { + return `gitdir: ${worktreeGitDir}\n`; + } + if (normalizedCandidate === normalize(commondirFile)) { + return "..\\..\\\n"; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(normalize(resolved)).toBe(normalize(sharedRepoRoot)); + }); + it("keeps project root when .git file does not point to worktrees", () => { const projectRoot = "/repo/submodule"; const gitEntry = path.join(projectRoot, ".git"); diff --git a/test/storage.test.ts b/test/storage.test.ts index bc5101612..ceb0c9d88 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { promises as fs, existsSync } from "node:fs"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { tmpdir } from "node:os"; import { getConfigDir, getProjectStorageKey } from "../lib/storage/paths.js"; import { @@ -794,8 +794,10 @@ describe("storage", () => { expect(worktreePath).toBe(mainPath); } finally { setStoragePathDirect(null); - process.env.HOME = originalHome; - process.env.USERPROFILE = originalUserProfile; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; await fs.rm(testWorkDir, { recursive: true, force: true }); } }); @@ -920,8 +922,10 @@ describe("storage", () => { afterEach(async () => { setStoragePathDirect(null); - process.env.HOME = originalHome; - process.env.USERPROFILE = originalUserProfile; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; await fs.rm(testWorkDir, { recursive: true, force: true }); }); @@ -1009,8 +1013,10 @@ describe("storage", () => { afterEach(async () => { setStoragePathDirect(null); - process.env.HOME = originalHome; - process.env.USERPROFILE = originalUserProfile; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; await fs.rm(testWorkDir, { recursive: true, force: true }); }); @@ -1071,16 +1077,20 @@ describe("storage", () => { lastUsed: now + 1, }; - async function prepareWorktreeFixture(): Promise<{ + async function prepareWorktreeFixture(options?: { + pointerStyle?: "default" | "windows"; + worktreeName?: string; + }): Promise<{ fakeHome: string; mainRepo: string; worktreeRepo: string; }> { const fakeHome = join(testWorkDir, "home"); const mainRepo = join(testWorkDir, "repo-main"); - const worktreeRepo = join(testWorkDir, "repo-pr-8"); + const worktreeName = options?.worktreeName ?? "repo-pr-8"; + const worktreeRepo = join(testWorkDir, worktreeName); const mainGitDir = join(mainRepo, ".git"); - const worktreeGitDir = join(mainGitDir, "worktrees", "repo-pr-8"); + const worktreeGitDir = join(mainGitDir, "worktrees", worktreeName); process.env.HOME = fakeHome; process.env.USERPROFILE = fakeHome; @@ -1089,8 +1099,14 @@ describe("storage", () => { await fs.mkdir(mainGitDir, { recursive: true }); await fs.mkdir(worktreeGitDir, { recursive: true }); await fs.mkdir(worktreeRepo, { recursive: true }); - await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); - await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + if (options?.pointerStyle === "windows") { + const winGitDirPointer = worktreeGitDir.replace(/\//g, "\\"); + await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${winGitDirPointer}\n`, "utf-8"); + await fs.writeFile(join(worktreeGitDir, "commondir"), "..\\..\\\n", "utf-8"); + } else { + await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); + await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + } return { fakeHome, mainRepo, worktreeRepo }; } @@ -1110,9 +1126,12 @@ describe("storage", () => { afterEach(async () => { setStoragePathDirect(null); - process.env.HOME = originalHome; - process.env.USERPROFILE = originalUserProfile; - process.env.CODEX_MULTI_AUTH_DIR = originalMultiAuthDir; + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + if (originalUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = originalUserProfile; + if (originalMultiAuthDir === undefined) delete process.env.CODEX_MULTI_AUTH_DIR; + else process.env.CODEX_MULTI_AUTH_DIR = originalMultiAuthDir; await fs.rm(testWorkDir, { recursive: true, force: true }); vi.restoreAllMocks(); }); @@ -1229,6 +1248,89 @@ describe("storage", () => { expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); expect(existsSync(legacyWorktreePath)).toBe(true); }); + + it("handles concurrent loadAccounts migration without duplicate race artifacts", async () => { + const { worktreeRepo } = await prepareWorktreeFixture({ worktreeName: "repo-pr-race" }); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const legacyWorktreePath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(worktreeRepo)), { + recursive: true, + }); + await fs.mkdir(dirname(canonicalPath), { recursive: true }); + await fs.writeFile( + canonicalPath, + JSON.stringify(buildStorage([accountFromCanonical]), null, 2), + "utf-8", + ); + await fs.writeFile( + legacyWorktreePath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + + const results = await Promise.all([ + loadAccounts(), + loadAccounts(), + loadAccounts(), + loadAccounts(), + ]); + + for (const result of results) { + expect(result).not.toBeNull(); + expect(result?.accounts).toHaveLength(2); + } + + const persistedRaw = await fs.readFile(canonicalPath, "utf-8"); + const persistedNormalized = normalizeAccountStorage(JSON.parse(persistedRaw) as unknown); + expect(persistedNormalized).not.toBeNull(); + expect(persistedNormalized?.accounts).toHaveLength(2); + expect(existsSync(legacyWorktreePath)).toBe(false); + }); + + it("migrates worktree storage with Windows-style gitdir pointer fixtures", async () => { + if (process.platform !== "win32") { + return; + } + + const { worktreeRepo } = await prepareWorktreeFixture({ + pointerStyle: "windows", + worktreeName: "repo-pr-win-ptr", + }); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const legacyWorktreePath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + expect(legacyWorktreePath).not.toBe(canonicalPath); + + await fs.mkdir(join(getConfigDir(), "projects", getProjectStorageKey(worktreeRepo)), { + recursive: true, + }); + await fs.writeFile( + legacyWorktreePath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + + const loaded = await loadAccounts(); + + expect(loaded).not.toBeNull(); + expect(loaded?.accounts).toHaveLength(1); + expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); + expect(existsSync(canonicalPath)).toBe(true); + expect(existsSync(legacyWorktreePath)).toBe(false); + }); }); describe("saveAccounts EPERM/EBUSY retry logic", () => { From 5e5c66187e146c5f652fc92f5793940f1f8dc525 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 11:15:34 +0800 Subject: [PATCH 3/6] fix(review): handle windows gitdir pointers cross-platform --- lib/storage/paths.ts | 41 +++++++++++++++++++++++++++++++++++------ test/paths.test.ts | 4 ---- test/storage.test.ts | 4 ---- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index a6f94dab7..f84935e8d 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -5,7 +5,7 @@ import { existsSync, readFileSync, statSync } from "node:fs"; import { createHash } from "node:crypto"; -import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path"; +import { basename, dirname, isAbsolute, join, relative, resolve, win32 } from "node:path"; import { homedir, tmpdir } from "node:os"; import { getCodexMultiAuthDir } from "../runtime-paths.js"; @@ -22,6 +22,37 @@ function parseGitDirPointer(pointerContent: string): string | null { return value.length > 0 ? value : null; } +function normalizePathDelimiters(pathValue: string): string { + return pathValue.replace(/\\/g, "/"); +} + +function isWindowsRootedPath(pathValue: string): boolean { + return /^[A-Za-z]:[\\/]/.test(pathValue) || /^\\\\[^\\]/.test(pathValue) || /^\/\/[^/]/.test(pathValue); +} + +function resolveGitPath(basePath: string, pointerValue: string): string { + const trimmedPointer = pointerValue.trim(); + if (!trimmedPointer) { + return basePath; + } + + if (isWindowsRootedPath(basePath) || isWindowsRootedPath(trimmedPointer)) { + const windowsBase = win32.normalize(basePath.replace(/\//g, "\\")); + const windowsPointer = win32.normalize(trimmedPointer.replace(/\//g, "\\")); + const windowsResolved = win32.isAbsolute(windowsPointer) + ? windowsPointer + : win32.resolve(windowsBase, windowsPointer); + return process.platform === "win32" + ? windowsResolved + : normalizePathDelimiters(windowsResolved); + } + + const normalizedPointer = normalizePathDelimiters(trimmedPointer); + return isAbsolute(normalizedPointer) + ? normalizedPointer + : resolve(basePath, normalizedPointer); +} + function readGitCommonDir(gitDirPath: string): string { const commonDirFile = join(gitDirPath, "commondir"); if (!existsSync(commonDirFile)) { @@ -31,14 +62,14 @@ function readGitCommonDir(gitDirPath: string): string { try { const raw = readFileSync(commonDirFile, "utf-8").trim(); if (!raw) return gitDirPath; - return isAbsolute(raw) ? raw : resolve(gitDirPath, raw); + return resolveGitPath(gitDirPath, raw); } catch { return gitDirPath; } } function isWorktreeGitDirPath(gitDirPath: string): boolean { - const normalized = normalizeProjectPath(gitDirPath); + const normalized = normalizePathDelimiters(gitDirPath).toLowerCase(); return normalized.includes("/.git/worktrees/"); } @@ -172,9 +203,7 @@ export function resolveProjectStorageIdentityRoot(projectRoot: string): string { return projectRoot; } - const gitDirPath = isAbsolute(gitDirValue) - ? gitDirValue - : resolve(projectRoot, gitDirValue); + const gitDirPath = resolveGitPath(projectRoot, gitDirValue); if (!isWorktreeGitDirPath(gitDirPath)) { return projectRoot; } diff --git a/test/paths.test.ts b/test/paths.test.ts index 8eef42997..0988243dd 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -273,10 +273,6 @@ describe("Storage Paths Module", () => { }); it("supports Windows-style backslash gitdir pointers", () => { - if (process.platform !== "win32") { - return; - } - const projectRoot = path.win32.join("C:\\repo", "worktrees", "pr-8"); const gitEntry = path.win32.join(projectRoot, ".git"); const worktreeGitDir = path.win32.join("C:\\repo", ".git", "worktrees", "pr-8"); diff --git a/test/storage.test.ts b/test/storage.test.ts index ceb0c9d88..ca68149ca 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1295,10 +1295,6 @@ describe("storage", () => { }); it("migrates worktree storage with Windows-style gitdir pointer fixtures", async () => { - if (process.platform !== "win32") { - return; - } - const { worktreeRepo } = await prepareWorktreeFixture({ pointerStyle: "windows", worktreeName: "repo-pr-win-ptr", From 7cea336afb41ab60787ac21cdf363b7b352ed09d Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 11:43:51 +0800 Subject: [PATCH 4/6] fix(security): validate worktree ownership before aliasing --- lib/storage/paths.ts | 41 ++++++++++++++++++++++++++++++++++ test/paths.test.ts | 53 ++++++++++++++++++++++++++++++++++++++++++++ test/storage.test.ts | 7 ++++++ 3 files changed, 101 insertions(+) diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index f84935e8d..6555de2e6 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -73,6 +73,44 @@ function isWorktreeGitDirPath(gitDirPath: string): boolean { return normalized.includes("/.git/worktrees/"); } +function normalizePathForIdentityCheck(pathValue: string): string { + const normalizedDelimiters = normalizePathDelimiters(pathValue.trim()); + if (!normalizedDelimiters) { + return normalizedDelimiters; + } + + if (isWindowsRootedPath(normalizedDelimiters)) { + return win32.normalize(normalizedDelimiters.replace(/\//g, "\\")).toLowerCase(); + } + + const resolvedPath = resolve(normalizedDelimiters); + const normalizedResolved = normalizePathDelimiters(resolvedPath); + return process.platform === "win32" ? normalizedResolved.toLowerCase() : normalizedResolved; +} + +function worktreeGitDirBelongsToProject(projectRoot: string, gitDirPath: string): boolean { + const gitdirBackRefPath = join(gitDirPath, "gitdir"); + if (!existsSync(gitdirBackRefPath)) { + return false; + } + + try { + const gitdirBackRefRaw = readFileSync(gitdirBackRefPath, "utf-8").trim(); + if (!gitdirBackRefRaw) { + return false; + } + + const resolvedBackRef = resolveGitPath(gitDirPath, gitdirBackRefRaw); + const expectedBackRef = join(projectRoot, ".git"); + return ( + normalizePathForIdentityCheck(resolvedBackRef) === + normalizePathForIdentityCheck(expectedBackRef) + ); + } catch { + return false; + } +} + /** * Gets the path to the global Codex multi-auth configuration directory. * @@ -207,6 +245,9 @@ export function resolveProjectStorageIdentityRoot(projectRoot: string): string { if (!isWorktreeGitDirPath(gitDirPath)) { return projectRoot; } + if (!worktreeGitDirBelongsToProject(projectRoot, gitDirPath)) { + return projectRoot; + } const commonGitDir = readGitCommonDir(gitDirPath); const candidateRepoRoot = dirname(commonGitDir); diff --git a/test/paths.test.ts b/test/paths.test.ts index 0988243dd..0a82585f1 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -195,6 +195,7 @@ describe("Storage Paths Module", () => { const gitEntry = path.join(projectRoot, ".git"); const worktreeGitDir = path.resolve("repo", ".git", "worktrees", "pr-8"); const commondirFile = path.join(worktreeGitDir, "commondir"); + const gitdirBackRefFile = path.join(worktreeGitDir, "gitdir"); const sharedRepoRoot = path.resolve("repo"); const sharedGitDir = path.join(sharedRepoRoot, ".git"); const normalize = (value: string) => @@ -205,6 +206,7 @@ describe("Storage Paths Module", () => { const normalizedCandidate = normalize(candidate); if (normalizedCandidate === normalize(gitEntry)) return true; if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(gitdirBackRefFile)) return true; if (normalizedCandidate === normalize(sharedGitDir)) return true; return false; }); @@ -223,6 +225,9 @@ describe("Storage Paths Module", () => { if (normalizedCandidate === normalize(commondirFile)) { return "../..\n"; } + if (normalizedCandidate === normalize(gitdirBackRefFile)) { + return `${path.join(projectRoot, ".git")}\n`; + } throw new Error(`Unexpected read path: ${String(candidate)}`); }); @@ -236,6 +241,7 @@ describe("Storage Paths Module", () => { const gitEntry = path.join(projectRoot, ".git"); const worktreeGitDir = path.resolve("repo", ".git", "worktrees", "pr-relative"); const commondirFile = path.join(worktreeGitDir, "commondir"); + const gitdirBackRefFile = path.join(worktreeGitDir, "gitdir"); const sharedRepoRoot = path.resolve("repo"); const sharedGitDir = path.join(sharedRepoRoot, ".git"); const normalize = (value: string) => @@ -246,6 +252,7 @@ describe("Storage Paths Module", () => { const normalizedCandidate = normalize(candidate); if (normalizedCandidate === normalize(gitEntry)) return true; if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(gitdirBackRefFile)) return true; if (normalizedCandidate === normalize(sharedGitDir)) return true; return false; }); @@ -264,6 +271,9 @@ describe("Storage Paths Module", () => { if (normalizedCandidate === normalize(commondirFile)) { return "../..\n"; } + if (normalizedCandidate === normalize(gitdirBackRefFile)) { + return `${path.join(projectRoot, ".git")}\n`; + } throw new Error(`Unexpected read path: ${String(candidate)}`); }); @@ -277,6 +287,7 @@ describe("Storage Paths Module", () => { const gitEntry = path.win32.join(projectRoot, ".git"); const worktreeGitDir = path.win32.join("C:\\repo", ".git", "worktrees", "pr-8"); const commondirFile = path.win32.join(worktreeGitDir, "commondir"); + const gitdirBackRefFile = path.win32.join(worktreeGitDir, "gitdir"); const sharedRepoRoot = "C:\\repo"; const sharedGitDir = path.win32.join(sharedRepoRoot, ".git"); const normalize = (value: string) => path.win32.normalize(value).toLowerCase(); @@ -286,6 +297,7 @@ describe("Storage Paths Module", () => { const normalizedCandidate = normalize(candidate); if (normalizedCandidate === normalize(gitEntry)) return true; if (normalizedCandidate === normalize(commondirFile)) return true; + if (normalizedCandidate === normalize(gitdirBackRefFile)) return true; if (normalizedCandidate === normalize(sharedGitDir)) return true; return false; }); @@ -304,6 +316,9 @@ describe("Storage Paths Module", () => { if (normalizedCandidate === normalize(commondirFile)) { return "..\\..\\\n"; } + if (normalizedCandidate === normalize(gitdirBackRefFile)) { + return `${path.win32.join(projectRoot, ".git")}\n`; + } throw new Error(`Unexpected read path: ${String(candidate)}`); }); @@ -312,6 +327,44 @@ describe("Storage Paths Module", () => { expect(normalize(resolved)).toBe(normalize(sharedRepoRoot)); }); + it("falls back to project root for forged worktree pointers", () => { + const projectRoot = "/repo/attacker"; + const gitEntry = path.join(projectRoot, ".git"); + const foreignWorktreeGitDir = "/repo/victim/.git/worktrees/pr-8"; + const foreignCommondir = path.join(foreignWorktreeGitDir, "commondir"); + const foreignGitdirBackRef = path.join(foreignWorktreeGitDir, "gitdir"); + const victimGitDir = path.join("/repo/victim", ".git"); + + mockedExistsSync.mockImplementation((candidate) => { + return ( + candidate === gitEntry || + candidate === foreignCommondir || + candidate === foreignGitdirBackRef || + candidate === victimGitDir + ); + }); + mockedStatSync.mockImplementation((candidate) => { + expect(candidate).toBe(gitEntry); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (candidate === gitEntry) { + return `gitdir: ${foreignWorktreeGitDir}\n`; + } + if (candidate === foreignCommondir) { + return "../..\n"; + } + if (candidate === foreignGitdirBackRef) { + return "/repo/victim/worktrees/pr-8/.git\n"; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(projectRoot); + }); + it("keeps project root when .git file does not point to worktrees", () => { const projectRoot = "/repo/submodule"; const gitEntry = path.join(projectRoot, ".git"); diff --git a/test/storage.test.ts b/test/storage.test.ts index ca68149ca..53cd7d9a0 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -786,6 +786,7 @@ describe("storage", () => { await fs.mkdir(worktreeRepo, { recursive: true }); await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + await fs.writeFile(join(worktreeGitDir, "gitdir"), `${join(worktreeRepo, ".git")}\n`, "utf-8"); setStoragePath(mainRepo); const mainPath = getStoragePath(); @@ -1103,9 +1104,15 @@ describe("storage", () => { const winGitDirPointer = worktreeGitDir.replace(/\//g, "\\"); await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${winGitDirPointer}\n`, "utf-8"); await fs.writeFile(join(worktreeGitDir, "commondir"), "..\\..\\\n", "utf-8"); + await fs.writeFile( + join(worktreeGitDir, "gitdir"), + `${join(worktreeRepo, ".git").replace(/\//g, "\\")}\n`, + "utf-8", + ); } else { await fs.writeFile(join(worktreeRepo, ".git"), `gitdir: ${worktreeGitDir}\n`, "utf-8"); await fs.writeFile(join(worktreeGitDir, "commondir"), "../..\n", "utf-8"); + await fs.writeFile(join(worktreeGitDir, "gitdir"), `${join(worktreeRepo, ".git")}\n`, "utf-8"); } return { fakeHome, mainRepo, worktreeRepo }; From 04aa86b807c508032cd74f14d597a845647998f1 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 12:40:47 +0800 Subject: [PATCH 5/6] fix(storage): block forged commondir repo aliasing Reject worktree identity aliasing when commondir does not match the worktree gitdir ancestry, and add unit/integration regressions for hostile commondir scenarios. Co-authored-by: Codex --- lib/storage/paths.ts | 19 +++++++++++++++ test/paths.test.ts | 38 +++++++++++++++++++++++++++++ test/storage.test.ts | 57 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+) diff --git a/lib/storage/paths.ts b/lib/storage/paths.ts index 6555de2e6..a85b3ce8c 100644 --- a/lib/storage/paths.ts +++ b/lib/storage/paths.ts @@ -111,6 +111,22 @@ function worktreeGitDirBelongsToProject(projectRoot: string, gitDirPath: string) } } +function isGitDirUnderCommonWorktrees(gitDirPath: string, commonGitDir: string): boolean { + const normalizedGitDir = normalizePathDelimiters( + normalizePathForIdentityCheck(gitDirPath), + ).replace(/\/+$/, ""); + const normalizedCommonGitDir = normalizePathDelimiters( + normalizePathForIdentityCheck(commonGitDir), + ).replace(/\/+$/, ""); + + if (!normalizedGitDir || !normalizedCommonGitDir) { + return false; + } + + const worktreesRoot = `${normalizedCommonGitDir}/worktrees/`; + return normalizedGitDir.startsWith(worktreesRoot); +} + /** * Gets the path to the global Codex multi-auth configuration directory. * @@ -250,6 +266,9 @@ export function resolveProjectStorageIdentityRoot(projectRoot: string): string { } const commonGitDir = readGitCommonDir(gitDirPath); + if (!isGitDirUnderCommonWorktrees(gitDirPath, commonGitDir)) { + return projectRoot; + } const candidateRepoRoot = dirname(commonGitDir); if (!existsSync(join(candidateRepoRoot, ".git"))) { return projectRoot; diff --git a/test/paths.test.ts b/test/paths.test.ts index 0a82585f1..c5192719f 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -365,6 +365,44 @@ describe("Storage Paths Module", () => { expect(resolved).toBe(projectRoot); }); + it("falls back to project root when commondir points to a foreign repository", () => { + const projectRoot = "/repo/attacker-worktree"; + const gitEntry = path.join(projectRoot, ".git"); + const worktreeGitDir = "/repo/attacker/.git/worktrees/pr-hostile"; + const forgedCommondir = path.join(worktreeGitDir, "commondir"); + const gitdirBackRefFile = path.join(worktreeGitDir, "gitdir"); + const foreignGitDir = "/repo/victim/.git"; + + mockedExistsSync.mockImplementation((candidate) => { + return ( + candidate === gitEntry || + candidate === forgedCommondir || + candidate === gitdirBackRefFile || + candidate === foreignGitDir + ); + }); + mockedStatSync.mockImplementation((candidate) => { + expect(candidate).toBe(gitEntry); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (candidate === gitEntry) { + return `gitdir: ${worktreeGitDir}\n`; + } + if (candidate === forgedCommondir) { + return `${foreignGitDir}\n`; + } + if (candidate === gitdirBackRefFile) { + return `${path.join(projectRoot, ".git")}\n`; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(resolved).toBe(projectRoot); + }); + it("keeps project root when .git file does not point to worktrees", () => { const projectRoot = "/repo/submodule"; const gitEntry = path.join(projectRoot, ".git"); diff --git a/test/storage.test.ts b/test/storage.test.ts index 53cd7d9a0..6c477fd97 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1334,6 +1334,63 @@ describe("storage", () => { expect(existsSync(canonicalPath)).toBe(true); expect(existsSync(legacyWorktreePath)).toBe(false); }); + + it("rejects forged commondir aliasing and keeps storage scoped to the current worktree", async () => { + const worktreeName = "repo-pr-hostile"; + const { mainRepo, worktreeRepo } = await prepareWorktreeFixture({ worktreeName }); + const worktreeGitDir = join(mainRepo, ".git", "worktrees", worktreeName); + const foreignRepo = join(testWorkDir, "repo-foreign"); + const foreignGitDir = join(foreignRepo, ".git"); + const foreignAccount: StoredAccountFixture = { + refreshToken: "foreign-refresh", + accountId: "foreign-account", + addedAt: now + 2, + lastUsed: now + 2, + }; + + await fs.mkdir(foreignGitDir, { recursive: true }); + await fs.writeFile(join(worktreeGitDir, "commondir"), `${foreignGitDir}\n`, "utf-8"); + + setStoragePath(worktreeRepo); + const canonicalPath = getStoragePath(); + const safeCanonicalPath = join( + getConfigDir(), + "projects", + getProjectStorageKey(worktreeRepo), + "openai-codex-accounts.json", + ); + const foreignCanonicalPath = join( + getConfigDir(), + "projects", + getProjectStorageKey(foreignRepo), + "openai-codex-accounts.json", + ); + await fs.mkdir(dirname(safeCanonicalPath), { recursive: true }); + await fs.mkdir(dirname(foreignCanonicalPath), { recursive: true }); + await fs.writeFile( + safeCanonicalPath, + JSON.stringify(buildStorage([accountFromLegacy]), null, 2), + "utf-8", + ); + await fs.writeFile( + foreignCanonicalPath, + JSON.stringify(buildStorage([foreignAccount]), null, 2), + "utf-8", + ); + + const loaded = await loadAccounts(); + + expect(canonicalPath).toBe(safeCanonicalPath); + expect(canonicalPath).not.toBe(foreignCanonicalPath); + expect(loaded).not.toBeNull(); + expect(loaded?.accounts).toHaveLength(1); + expect(loaded?.accounts[0]?.accountId).toBe("legacy-account"); + expect(existsSync(canonicalPath)).toBe(true); + + const foreignRaw = await fs.readFile(foreignCanonicalPath, "utf-8"); + const foreignStorage = normalizeAccountStorage(JSON.parse(foreignRaw) as unknown); + expect(foreignStorage?.accounts[0]?.accountId).toBe("foreign-account"); + }); }); describe("saveAccounts EPERM/EBUSY retry logic", () => { From 6d137a83b00aed655f49cc702fc72b00a2963951 Mon Sep 17 00:00:00 2001 From: ndycode Date: Sat, 28 Feb 2026 12:59:46 +0800 Subject: [PATCH 6/6] test(paths): cover UNC worktree gitdir resolution Add a deterministic Windows UNC pointer regression for storage identity root resolution. Co-authored-by: Codex --- test/paths.test.ts | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/paths.test.ts b/test/paths.test.ts index c5192719f..8b2599e6d 100644 --- a/test/paths.test.ts +++ b/test/paths.test.ts @@ -327,6 +327,52 @@ describe("Storage Paths Module", () => { expect(normalize(resolved)).toBe(normalize(sharedRepoRoot)); }); + it("supports windows UNC gitdir pointers", () => { + const sharedRepoRoot = "\\\\server\\share\\repo"; + const projectRoot = path.win32.join(sharedRepoRoot, "worktrees", "pr-unc"); + const gitEntry = path.win32.join(projectRoot, ".git"); + const worktreeGitDir = path.win32.join(sharedRepoRoot, ".git", "worktrees", "pr-unc"); + const commondirFile = path.win32.join(worktreeGitDir, "commondir"); + const gitdirBackRefFile = path.win32.join(worktreeGitDir, "gitdir"); + const sharedGitDir = path.win32.join(sharedRepoRoot, ".git"); + const normalize = (value: string) => path.win32.normalize(value).toLowerCase(); + + mockedExistsSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") return false; + const normalizedCandidate = normalize(candidate); + return ( + normalizedCandidate === normalize(gitEntry) || + normalizedCandidate === normalize(commondirFile) || + normalizedCandidate === normalize(gitdirBackRefFile) || + normalizedCandidate === normalize(sharedGitDir) + ); + }); + mockedStatSync.mockImplementation((candidate) => { + expect(normalize(String(candidate))).toBe(normalize(gitEntry)); + return buildMockStat({ isDirectory: false, isFile: true }); + }); + mockedReadFileSync.mockImplementation((candidate) => { + if (typeof candidate !== "string") { + throw new Error(`Unexpected read path: ${String(candidate)}`); + } + const normalizedCandidate = normalize(candidate); + if (normalizedCandidate === normalize(gitEntry)) { + return `gitdir: ${worktreeGitDir}\n`; + } + if (normalizedCandidate === normalize(commondirFile)) { + return "..\\..\\\n"; + } + if (normalizedCandidate === normalize(gitdirBackRefFile)) { + return `${path.win32.join(projectRoot, ".git")}\n`; + } + throw new Error(`Unexpected read path: ${String(candidate)}`); + }); + + const resolved = resolveProjectStorageIdentityRoot(projectRoot); + + expect(normalize(resolved)).toBe(normalize(sharedRepoRoot)); + }); + it("falls back to project root for forged worktree pointers", () => { const projectRoot = "/repo/attacker"; const gitEntry = path.join(projectRoot, ".git");