feat!: rename oc-chatgpt-multi-auth to oc-codex-multi-auth#104
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe pull request rebands the package from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function migrateLegacyGlobalStorageIfNeeded(): Promise<AccountStorageV3 | null> { | ||
| const nextPath = getGlobalAccountsStoragePath(); | ||
| const persistGlobalStorage = async (storage: AccountStorageV3): Promise<void> => { | ||
| await writeAccountsToPathUnlocked(nextPath, storage); | ||
| }; | ||
|
|
||
| return migrateStorageFileIfNeeded( | ||
| getLegacyGlobalAccountsStoragePath(), | ||
| nextPath, | ||
| persistGlobalStorage, | ||
| "global account storage", | ||
| ); | ||
| } |
There was a problem hiding this comment.
writeAccountsToPathUnlocked documents "callers must already be inside withStorageLock" (see its JSDoc at line ~891), but migrateLegacyGlobalStorageIfNeeded() calls it for the global path while holding only the PROJECT storage lock (when invoked via loadGlobalAccountsFallback()). on windows, two concurrent plugin processes — e.g. one running opencode while a terminal relaunch fires another — can both pass the existsSync(legacyPath) check, both read the old file, and race to rename their temp files over the new path. the atomic write prevents a torn file, but the second rename silently wins and there is no guard against double-migration. consider checking whether nextPath already exists immediately before calling persist, or guard the global path with the same mutex that protects it elsewhere.
Rule Used: What: Every code change must explain how it defend... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 729-741
Comment:
**global path written outside its intended lock scope**
`writeAccountsToPathUnlocked` documents "callers must already be inside withStorageLock" (see its JSDoc at line ~891), but `migrateLegacyGlobalStorageIfNeeded()` calls it for the global path while holding only the PROJECT storage lock (when invoked via `loadGlobalAccountsFallback()`). on windows, two concurrent plugin processes — e.g. one running opencode while a terminal relaunch fires another — can both pass the `existsSync(legacyPath)` check, both read the old file, and race to rename their temp files over the new path. the atomic write prevents a torn file, but the second rename silently wins and there is no guard against double-migration. consider checking whether `nextPath` already exists immediately before calling `persist`, or guard the global path with the same mutex that protects it elsewhere.
**Rule Used:** What: Every code change must explain how it defend... ([source](https://app.greptile.com/review/custom-context?memory=637a42e6-7a78-40d6-9ef8-6a45e02e73b6))
How can I resolve this? If you propose a fix, please make it concise.
…rand feat!: rename oc-chatgpt-multi-auth to oc-codex-multi-auth
…rand feat!: rename oc-chatgpt-multi-auth to oc-codex-multi-auth
1 participant
Summary
oc-chatgpt-multi-authtooc-codex-multi-authTesting
npm run lintnpm run buildnpm testCompliance Confirmation
Notes
npm run typecheckalso passes. This is an intentional breaking rename for6.0.0; post-merge cutover still needs the GitHub repo rename, npm publish foroc-codex-multi-auth, and npm deprecation foroc-chatgpt-multi-auth.Summary by CodeRabbit
New Features
Chores
oc-codex-multi-auth(v6.0.0 breaking change).note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this PR renames the package from
oc-chatgpt-multi-authtooc-codex-multi-auth(v6.0.0), centralises storage filename constants inlib/constants.ts, consolidates legacy migration into a genericmigrateStorageFileIfNeededhelper, adds a new global-storage migration path, ships a rewritten installer with windows atomic-write retry logic, and updates all docs, config examples, and tests consistently.lib/storage.ts): the rename changes the active per-project storage filename fromopenai-codex-accounts.jsontooc-codex-multi-auth-accounts.jsonin the same~/.opencode/projects/<key>/directory, but there is no migration from the old name in that directory.currentLegacyProjectStoragePathpoints at<projectRoot>/.opencode/openai-codex-accounts.json(the pre-v5.x local path), andgetLegacyGlobalAccountsStoragePath()points at~/.opencode/openai-codex-accounts.json— neither covers the v5.x primary path~/.opencode/projects/<key>/openai-codex-accounts.json. any user upgrading from v5.x with project-scoped storage silently loses all accounts and oauth tokens.writeAccountsToPathUnlockedis called for the global path while holding only the project storage lock; concurrent processes migrating global storage simultaneously race on the atomic rename without a guard.migrateLegacyGlobalStorageIfNeeded()branch insideloadAccountsInternal(non-project-fallback ENOENT path, lines 842-848) has no test; no test exists for the v5.x per-project-global upgrade scenario either.scripts/install-oc-codex-multi-auth-core.jsis clean: atomic writes, exponential-backoff windows retry onEPERM/EBUSY, backup-before-overwrite, and legacy package removal from cache are all well-tested.Confidence Score: 1/5
not safe to merge — v5.x users with per-project storage will silently lose all stored accounts and oauth tokens on upgrade to v6.0.0
one critical P0 data-loss bug (missing migration for the exact path v5.x wrote to) blocks merge; a P1 windows concurrency race in global migration and missing vitest coverage add further risk
lib/storage.ts (missing per-project-global migration path) and test/storage.test.ts (missing regression test for v5.x upgrade scenario)
Important Files Changed
~/.opencode/projects/<key>/openai-codex-accounts.json) — existing users lose accounts on upgradeFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["loadAccountsInternal ENOENT"] --> B["migrateProjectLocal?\ngetProjectConfigDir / LEGACY_FILE\n<projectRoot>/.opencode/openai-codex-accounts.json"] B -- found --> C["Return migrated project-local data ✅"] B -- not found --> D{"shouldUseProjectGlobalFallback?"} D -- false --> E["migrateLegacyGlobal?\ngetConfigDir / LEGACY_FILE\n~/.opencode/openai-codex-accounts.json"] E -- found --> F["Return migrated global data ✅"] E -- not found --> G["Return null"] D -- true --> H["loadGlobalAccountsFallback"] H --> I["migrateLegacyGlobal?\ngetConfigDir / LEGACY_FILE"] I -- found --> J["Return migrated global data ✅"] I -- not found --> K["Read getConfigDir / ACCOUNTS_FILE"] K -- found --> L["Return global data seeded to project ✅"] K -- not found --> G M["❌ MISSING PATH\ngetProjectGlobalConfigDir / LEGACY_FILE\n~/.opencode/projects/key/openai-codex-accounts.json\nwhere v5.x actually wrote per-project storage"] -. never checked .-> AComments Outside Diff (1)
lib/storage.ts, line 284-294 (link)v5.x stored project-scoped accounts at
~/.opencode/projects/<key>/openai-codex-accounts.json— that was the value ofcurrentStoragePathin v5.x (line 288 usedgetProjectGlobalConfigDir(projectRoot)+ the oldACCOUNTS_FILE_NAME). v6.0.0 changes the filename but keeps the same dir. the two migration paths added by this PR cover:<projectRoot>/.opencode/openai-codex-accounts.json→currentLegacyProjectStoragePath~/.opencode/openai-codex-accounts.json→getLegacyGlobalAccountsStoragePath()but neither covers
~/.opencode/projects/<key>/openai-codex-accounts.json. any user who ran v5.x in project mode hits ENOENT on the new filename, both migration checks fail,loadGlobalAccountsFallback()also won't find their data, andloadAccountsInternalreturnsnull— silently discarding all stored accounts and OAuth tokens.a third migration path is needed:
this should be called in the ENOENT handler before
migrateLegacyProjectStorageIfNeeded. a regression test covering the v5.x → v6.0.0 upgrade path is also missing fromtest/storage.test.ts.windows risk: if the legacy file is locked by antivirus during
fs.unlink, the stale token file persists on disk — the warn log should redact therefreshTokenfield.Rule Used: What: Every code change must explain how it defend... (source)
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "docs: refresh codex-first rebrand and cu..." | Re-trigger Greptile
Context used: