Audit remediation: security, observability, resilience (Phases 1-3)#499
Merged
Conversation
Secrets-at-rest: - refresh-lease: write token files mode 0600 under a 0700 dir (auth-01) - oc-chatgpt: atomic 0600 write of merged secret-bearing account file (chatgpt-import-01/02) - refresh-queue: log non-reversible sha256 token fingerprints, not the recoverable trailing chars (auth-05) - debug-bundle: mask account email + redact home-dir paths (cli-manager-06, errors-logging-04) - runtime proxy: mask status.lastError at the exposure boundary (errors-logging-08) Loopback / egress invariants: - runtime proxy refuses non-loopback bind unless explicitly opted in (runtime-proxy-01) - local-bridge validates runtimeBaseUrl resolves to loopback (runtime-proxy-02) Default-on correctness: - host-codex-prompt: restore live sst/opencode URLs; the rebrand-artifact URLs 404'd and re-fetched every request (prompts-01) - context-overflow: emit Responses-API SSE so the client can parse the /compact notice (was Anthropic Messages SSE); random synthetic id (recovery-01, recovery-11) - budget eval: include archived ledger rows so rotation can't bypass a budget within the active window (quota-forecast-03) - storage primary read: retry transient FS locks instead of dropping into WAL/backup recovery (storage-01) - config explain: add 3 missing live keys + parity guard test (config-01, config-07) Published type contract: - bundle @codex-ai/sdk (deps + bundleDependencies) so a consumer's tsc can resolve the types re-exported by the published .d.ts (docs-supplychain-01) Test integrity: - wrapper routing: add 3 missing auth subcommands; test imports the real ACCOUNT_MANAGER_COMMANDS instead of a hardcoded list (cli-manager-01/02) - property tests: wire setup.ts via setupFiles so fc.configureGlobal applies (tests-ci-02) - vitest: serialize fixed-port tests via fileParallelism:false (tests-ci-03) - oauth integration: await real port release in afterEach (tests-ci-03) All changes verified: tsc clean, lint clean, 4073 tests pass, pack-check ok. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ci-01) Suites that resolve storage/config paths but forget to redirect HOME / USERPROFILE / CODEX_HOME / CODEX_MULTI_AUTH_DIR could fall through to the developer's real ~/.codex and read or clobber live account state. A setupFiles sandbox now pins all four to a per-worker temp dir before any test imports application code, so the unsandboxed default is an empty throwaway dir. Tests that set these vars themselves still override and restore to the sandbox value, never the real home. Adds a guard test asserting resolved codex home + multi-auth dir land under the sandbox root. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Observability cluster: - logger: replace the process-global correlation id with AsyncLocalStorage so concurrent runtime-proxy requests cannot read each other's id; keep the set/get/clear API backward-compatible and add runWithCorrelationId for scoped use (errors-logging-02) - runtime proxy: inject a structured logger and bind a per-request trace id (distinct from sessionKey) around each request; log request failures through it with redaction instead of only stashing status.lastError (errors-logging-01/03, runtime-proxy-04) Resilience: - resolvePath: canonicalize the deepest existing ancestor via realpath and re-check containment, so a symlink inside an approved root that resolves outside it is rejected (storage-02 symlink TOCTOU) - removeAccount: clear identity-keyed health/token tracker state and the account's circuit breaker so a re-added identity does not inherit stale penalties (accounts-02); adds clearAccountKey to both trackers and removeCircuitBreaker - runtime proxy chooseAccount: thread pidOffsetEnabled into the hybrid selector so the default-on proxy spreads load like the plugin-host path instead of stampeding one account (accounts-05) Tests: AsyncLocalStorage isolation, symlink-escape accept/reject, and remove-then-fresh-health regression tests added. Verified: tsc clean, npm test 3x consecutive green (270 files / 4080 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add lib/prompts/fetch-utils.ts and route both prompt fetchers (codex.ts, host-codex-prompt.ts) through it: - fetchWithTimeout: bounded AbortSignal timeout so a hung GitHub connection cannot stall the request pipeline (prompts-02) - readBodyTextGuarded: 1 MB size cap (Content-Length + streamed enforcement) and rejection of empty/whitespace-only bodies so a bad 200 is not cached and served as instructions (prompts-04/05) - withPromptFetchHeaders: always send User-Agent + Accept, since api.github.com rejects requests without a UA (prompts-08) Also revert the earlier fileParallelism:false experiment in vitest.config.ts. Investigation (8 runs each on this branch vs pristine origin/main) showed the suite has a pre-existing, environment-level intermittent vitest worker crash on Windows (exit 1, no test failure, no summary) that reproduces on upstream main at the same rate; it is unrelated to fileParallelism, which added no protection beyond the npm --maxWorkers=1 flag. Tracked as finding tests-ci-16. Tests: 9 fetch-utils unit tests (timeout/size/empty/headers); host-prompt header assertions updated. Prompt suites: 58 tests green; tsc + lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
createUiTheme now blanks all ANSI color tokens when color should be disabled, via shouldDisableColor(): - NO_COLOR set (any value) disables color (no-color.org) - FORCE_COLOR=0/false forces off; any other FORCE_COLOR value forces on - otherwise color is off when stdout is not a TTY (piped/redirected) Glyphs are unaffected. Callers may pass disableColor explicitly. Theme/format tests that assert ANSI codes now opt into color explicitly (disableColor:false) since the test env sets NO_COLOR/FORCE_COLOR=0; adds dedicated gating tests for shouldDisableColor and the blanked theme. Verified: tsc + lint clean; full suite 271 files / 4095 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The routingMutex config flag + *Locked methods + withRoutingMutex were fully implemented and tested but had zero production callers, so the flag implied a concurrency guarantee the synchronous selector never provided. Wire it on the path that actually needs it: - apply getRoutingMutexMode(pluginConfig) to the account manager at proxy startup and on every manager-reload path - route persistRuntimeActiveAccount's commit through markSwitchedLocked, since that path spans an await (syncCodexCliActiveSelectionForIndex) and is the real lost-update window the mutex targets The synchronous chooseAccount+markSwitched sites are left as-is: with no await between read and write they are already event-loop-atomic, so a mutex there would add overhead without closing a race. Legacy mode (the default) runs inline, so behavior is unchanged unless a user sets CODEX_AUTH_ROUTING_MUTEX=enabled. Tests: assert the mode propagates for enabled + legacy-default at startup. Verified: tsc + lint clean; full suite 271 files / 4097 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- AGENTS.md: bump the stale header version 2.0.1 -> 2.1.13-beta.2 - SECURITY.md: correct the hono override rationale (4.12.14 -> 4.12.18) to match the actual package.json pin - documentation.test.ts: add a parity test asserting SECURITY.md cites the same hono override version as package.json, so this drift cannot silently recur Verified: documentation suite 25 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
readMessages/readParts previously skipped unparseable session files with a bare `continue`, so corrupt recovery data was dropped with no signal (also recovery-04). Now: - quarantineCorruptFile() renames the bad file to a `.corrupt-<ts>` sibling (preserved for inspection, not deleted), with a graceful fallback if the rename is blocked (e.g. Windows lock) - a process-level corruption count + quarantined-path list is exposed via getRecoveryCorruptionStats() so callers can report it Regression test asserts the rename + stats for a corrupt message file. Verified: tsc + lint clean; full suite 271 files / 4098 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-04) loadPluginConfig() is synchronous, so a transient EBUSY/EPERM/EAGAIN on the legacy-file readFileSync fell straight through to the catch and silently reverted to DEFAULT_PLUGIN_CONFIG, discarding the user's real settings. Add readFileSyncWithConfigRetry (sync, Atomics.wait backoff, retries only the transient lock codes; ENOENT and SyntaxError propagate unchanged) mirroring the async retry already used by the unified path. Tests: transient EBUSY is retried and the setting survives; a non-transient code falls back to defaults without retrying. Verified: tsc + lint clean; full suite 271 files / 4100 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three bespoke retry loops used divergent retryable-code subsets that all omitted ENOTEMPTY (and EACCES): - account-clear.ts isRetryableFsError (EBUSY/EPERM only) - flagged-storage-io.ts RETRYABLE_UNLINK_CODES (EBUSY/EAGAIN/EPERM) - import-export.ts renameExportFileWithRetry (EPERM/EBUSY/EAGAIN) Route all three through the shared lib/fs-retry.ts set (shouldRetryFileOperation / FILE_RETRY_CODES). Each loop keeps its own attempt count and backoff; only the retryable-code decision is unified, so the existing count-specific tests are unaffected. Tests: account-clear parametrized retry test extended to ENOTEMPTY/EACCES. Verified: tsc + lint clean; full suite 271 files / 4102 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…03/06) codex.ts cached the GitHub prompt with a non-atomic parallel writeFile of content + meta (torn-file risk on crash) and trusted the disk cache blindly (cache-poisoning -> persistent prompt-injection vector). - writeCacheAtomically: temp + rename for both files, content written before meta so the meta's sha always describes an on-disk content file (prompts-06) - CacheMetadata gains an optional sha256; the disk cache is verified against it on read and discarded + refetched on mismatch (prompts-03). Backward-compatible: caches without a sha are still accepted. Tests: sha-match serves the cache without a fetch; sha-mismatch discards and refetches. Verified: tsc + lint clean; full suite 271 files / 4104 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughsmall, factual hardening bundle. account removal clears identity-keyed trackers and circuit breakers (lib/accounts.ts:1462). logging moves to async-local storage and per-request trace ids are used by the runtime proxy (lib/logger.ts:138, lib/runtime-rotation-proxy.ts:1434). prompt fetching gains timeouts, size guards, sha256 integrity, and atomic writes (lib/prompts/fetch-utils.ts:29, lib/prompts/codex.ts:4). storage canonicalization and centralized retry logic added; recovery quarantines corrupt files and exposes stats (lib/storage/paths.ts:481, lib/recovery/storage.ts:23). many tests added/updated. Changesmulti-auth hardening, resilience, and observability
estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes possibly related PRs
suggested labels
issues to call out (explicit)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/helpers/global-sandbox.ts (1)
1-31: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuesandbox temp directory is not cleaned up after test run.
test/helpers/global-sandbox.ts:19creates a temp directory withmkdtempSyncbut there's no corresponding cleanup. repeated test runs will accumulate orphan directories inos.tmpdir().this is low priority since os temp cleanup typically handles it, but consider adding an
afterAllorglobalTeardownhook if the sandbox grows large (e.g., if tests copy account files into it).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/global-sandbox.ts` around lines 1 - 31, The temp sandbox created by mkdtempSync and stored in SANDBOX_ROOT is never removed, leading to accumulated dirs in tmpdir; add teardown logic to remove SANDBOX_ROOT after tests complete (e.g., register a globalTeardown or an afterAll/process.on('exit') cleanup that deletes the directory recursively), locate the creation in test/helpers/global-sandbox.ts (SANDBOX_ROOT, mkdtempSync) and ensure the teardown runs once per worker and safely ignores missing paths or errors.test/account-clear.test.ts (1)
20-25: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuebare
fs.rmin afterEach may fail on windows with transient locks.
test/account-clear.test.ts:21usesfs.rm({ recursive: true, force: true })which can race with antivirus scanners on windows. the comment acknowledges this but the coding guidelines requireremoveWithRetryfor windows filesystem cleanup instead of barefs.rm.consider using the shared retry helper if available, or at least wrap in a try/catch with backoff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/account-clear.test.ts` around lines 20 - 25, The afterEach cleanup uses a bare fs.rm on testTmpRoot which can fail on Windows; replace it with the shared retry helper (removeWithRetry) or wrap the removal in a retry/backoff loop: call removeWithRetry(testTmpRoot) inside the afterEach teardown (or implement a local retry wrapper named removeWithRetry and use that) to ensure transient locks from antivirus don't cause flakey test failures.lib/prompts/codex.ts (2)
170-190:⚠️ Potential issue | 🟠 Major | ⚡ Quick winsize-cap the html releases fallback too.
lib/prompts/codex.ts:190still does a rawhtmlResponse.text(), so the api-failure path bypassesreadBodyTextGuardedand can read an unbounded body on the request thread. use the same guarded reader here that you already use for prompt content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/prompts/codex.ts` around lines 170 - 190, The fallback path uses htmlResponse.text() which can read an unbounded response; replace that call with the same bounded reader used elsewhere (e.g., readBodyTextGuarded) to enforce the size cap: when handling the htmlResponse after failing to extract the tag, call the guarded reader (passing htmlResponse and the same size limit used for prompt content) instead of htmlResponse.text(), then continue using the returned string to parse the release and update latestReleaseTagCache; ensure you import or reference the same readBodyTextGuarded utility and preserve existing variables like htmlResponse, finalUrl and latestReleaseTagCache.
245-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick windo not fall back to the rejected disk cache after a sha mismatch.
lib/prompts/codex.ts:250marks the on-disk cache untrusted when the hash mismatches, butlib/prompts/codex.ts:298-lib/prompts/codex.ts:301still returns that samediskContentif the refetch fails. that serves the tampered file you just rejected. keep a separate trusted-cache variable and only reuse it when the hash matched; otherwise fall back to bundled content. please add a vitest regression forsha mismatch + fetch failure, becausetest/codex-prompts.test.ts:187-test/codex-prompts.test.ts:214only covers the successful refetch path.as per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/prompts/codex.ts` around lines 245 - 301, The code currently rejects on-disk cache when sha256 mismatches but still falls back to that same diskContent on fetch failure; fix by introducing a trustedDiskContent (or trustedCache) variable set only when integrityOk is true and use that for all cache reuse paths (e.g., in the catch fallback after fetchAndPersistInstructions and in stale-serving logic), ensuring that when integrityOk is false you never return diskContent but instead fall back to bundled content; update logic around cachedMetadata, diskContent, setCacheEntry, refreshInstructionsInBackground and fetchAndPersistInstructions accordingly. Also add a deterministic vitest regression that simulates "sha mismatch + fetch failure" (mock fetchAndPersistInstructions to throw and ensure the function does not return the rejected diskContent) to cover this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 204-207: ACCOUNT_MANAGER_COMMANDS is a test-only implementation
detail exported from lib/codex-manager.ts; move it out of the public CLI
entrypoint by creating a small internal module (e.g.,
lib/internal/command-sets.ts) that exports ACCOUNT_MANAGER_COMMANDS and import
it from both lib/codex-manager.ts and test/codex-routing.test.ts, or if you
intend it to be public, re-export it explicitly through lib/index.ts instead of
exporting directly from codex-manager.ts; update imports in codex-manager and
the test to reference the new internal module (or lib/index.ts) so the public
surface no longer exposes the registry directly.
In `@lib/codex-manager/commands/debug-bundle.ts`:
- Around line 9-15: redactHome currently uses a plain startsWith(home) which
fails on Windows case-insensitivity and matches sibling prefixes (e.g.
/users/alice2); update redactHome to compare canonicalized paths: resolve and
normalize both value and homedir() (use path.resolve/path.normalize), on Windows
perform a case-insensitive comparison (e.g. lowercasing both) and only redact
when the normalized home is an actual path boundary (either value === home or
value startsWith(home + path.sep)), and preserve the rest of the path after
replacing the exact home prefix with "~"; add regression tests covering
Windows-casing variants and a prefix-collision path like "/users/alice2/..." to
ensure no false redaction.
In `@lib/config.ts`:
- Around line 47-75: The sync retry in readFileSyncWithConfigRetry currently
sets maxAttempts = 4 which yields 4 tries and therefore doesn't actually mirror
the async logic; update readFileSyncWithConfigRetry to use the same retry count
as the async counterpart (match the async's 5 total attempts) — e.g. change
maxAttempts to 5 or adopt the same attempt gate used by the async implementation
— and update the function comment to accurately state it mirrors the async retry
behavior; reference readFileSyncWithConfigRetry and the maxAttempts variable
when making the change.
In `@lib/context-overflow.ts`:
- Around line 61-64: The responseId generation in lib/context-overflow.ts is
only using Date.now() and can collide; update responseId to include the same
extra entropy used for messageId (e.g., append a short random base36 slice or
similar) to make it collision-resistant (keep the `resp_synthetic_overflow_`
prefix and Date.now() but add the random suffix), and add a regression test in
test/context-overflow.test.ts that creates multiple overflows concurrently to
assert responseId uniqueness; touch the symbols messageId and responseId in your
change so reviewers can verify parity.
In `@lib/logger.ts`:
- Around line 165-176: The async-local store currently writes an empty string in
clearCorrelationId(), breaking the string|null contract of getCorrelationId();
update clearCorrelationId() to set the in-scope correlation id to null via
correlationStore.getStore().id = null (and adjust the store/type if needed so id
can be string|null), keep the fallback behavior unchanged (fallbackCorrelationId
= null when no store), and add a regression test in test/logger.test.ts that
calls runWithCorrelationId(...) (or the existing scope helper), invokes
clearCorrelationId() inside the active scope, and asserts getCorrelationId()
returns null.
In `@lib/prompts/codex.ts`:
- Around line 24-41: The writeCacheAtomically function currently does one-shot
fs.rename/fs.rm on the temp files (contentTmp/metaTmp) which fails
intermittently on Windows; wrap the rename and cleanup calls
(fs.rename(contentTmp, cacheFile), fs.rename(metaTmp, cacheMetaFile), and fs.rm
calls) with the shared fs-retry helper so they retry on transient errors (EBUSY,
EPERM, ENOTEMPTY and transient 429-like contention) with backoff, and ensure
writeFile remains atomic but can use the helper if desired; update tests by
adding a vitest in test/codex-prompts.test.ts that stubs/throws these transient
errors for rename/rm to assert the retry path succeeds and cleanup still runs.
In `@lib/prompts/fetch-utils.ts`:
- Around line 30-38: withPromptFetchHeaders currently spreads caller headers
last which lets callers overwrite mandatory GitHub headers; change it to
normalize the incoming headers via the Headers API (or equivalent) and then set
"User-Agent" to PROMPT_FETCH_USER_AGENT and "Accept" to json ?
"application/vnd.github+json" : "text/plain, */*" after merging so those
required values always win; update the logic in withPromptFetchHeaders to accept
the headers input, normalize/merge using new Headers(headers) (or an
object-to-headers normalization), and then call set("User-Agent", ...) and
set("Accept", ...) before returning the final Record<string,string>.
In `@lib/recovery/storage.ts`:
- Around line 266-272: The loop that reads recovery files currently quarantines
on any readFileSync/JSON.parse error (calling quarantineCorruptFile), which
wrongly quarantines healthy files for transient FS errors (EBUSY/EPERM/ENOENT);
change the logic in the recovery storage reader to: (1) detect and retry
transient read errors (EBUSY, EPERM, ENOENT) a few times with backoff before
giving up, (2) only call quarantineCorruptFile when a read succeeded but
JSON.parse or validation of the parsed object fails, and (3) preserve existing
handling for true corruption paths; update the code paths that call
readFileSync/JSON.parse (the block that invokes quarantineCorruptFile) and
ensure quarantineCorruptFile remains the sink for parse/validation failures
only; add a regression test in test/recovery-storage.test.ts that simulates a
locked-file/read-race returning EBUSY/EPERM/ENOENT to assert retries occur and
the file is not quarantined.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 2045-2052: The proxyLog.error call is logging raw error.message
which may leak emails/tokens; update the runtime-rotation-proxy logging to pass
a masked error string by calling maskString(...) on the message before logging
(use maskString(error.message ?? String(error)) ), preserving the existing
traceId and code logic (keep isRuntimeProxyHttpError(error) check for code) and
mirror the same masking approach used for status.lastError; ensure you import or
reference maskString where proxyLog.error is invoked so the logged error field
contains the redacted string.
In `@lib/storage/paths.ts`:
- Around line 471-489: Add regression tests for the symlink-escape branch
exercised by canonicalizeExistingPrefix and the conditional that throws the
"Access denied: path resolves (via symlink) outside the home, project, or temp
directory" error: extend test/paths.test.ts (the "rejects a symlink inside home
that resolves outside all approved roots" test) with Windows-specific cases
exercising drive-letter normalization and realpath/canonicalization differences,
and add a TOCTOU/concurrency stress test around exists/realpath (or the mocked
equivalents) that simulates the race where resolved and canonical paths diverge;
also make the test assertion resilient to the exact capitalization/phrasing by
matching "access denied" case-insensitively or checking the presence of "Access
denied" to ensure the thrown Error from
canonicalizeExistingPrefix/isWithinDirectory/isLookalikeSibling logic is
detected.
In `@lib/storage/storage-parser.ts`:
- Around line 55-60: Add a new unit test that mocks fs.readFile to throw a
transient Windows-lock error on the first call (set err.code = "EBUSY" or
"EPERM"/"EACCES") and then return valid JSON on the second call, then call
loadAccountsFromPath and assert it returns the parsed object; implement the mock
using a call-count closure or jest.fn().mockImplementationOnce to throw once and
then resolve, and ensure the test exercises the retry behavior provided by
withFileOperationRetry wrapping fs.readFile; optionally add a separate test that
mocks fs.readFile to throw ENOENT and assert loadAccountsFromPath
propagates/returns the missing-file behavior (no retry).
In `@scripts/codex-routing.js`:
- Around line 6-8: Add regression tests in test/codex-routing.test.ts that
assert the commands "unpin", "workspace", and "uninstall" are routed as local
auth commands (not forwarded). Specifically, add cases that exercise the routing
logic used by scripts/codex-routing.js (the arrays of auth commands and
compatibility aliases) and assert those three tokens are handled by the
local-auth branch of the router; include one test per command and reuse existing
test helpers for asserting "local auth" behavior so future changes to
scripts/codex-routing.js (the auth command list and compatibility alias
handling) will fail the test if forwarding regressions occur.
In `@test/accounts.test.ts`:
- Around line 1046-1086: Extend the test to also exercise token-bucket and
circuit-breaker state clearing: before calling removeAccount(liveTarget) consume
one token from the identity's token bucket (use the repo helper/function that
returns the bucket for an identity, e.g. getTokenBucket(identityKey) or
TokenBucket.consume on the identity) and trip/open a breaker for that same
identity (use the circuit-breaker helper, e.g. getBreaker(identityKey) or
CircuitBreaker.open/trip). After removeAccount, assert the token bucket value
for identityKey is back to its default/full value and that a fresh breaker
lookup for identityKey reports closed/not-open. Keep existing references to
AccountManager, getRuntimeTrackerKey, getHealthTracker and removeAccount when
locating where to add these steps.
In `@test/config-explain.test.ts`:
- Around line 211-217: The test currently only checks for missing explain
entries by comparing DEFAULT_PLUGIN_CONFIG keys to report.entries; make it
bidirectional by also checking for extras in the report (stale/renamed entries).
Update the assertion logic around getPluginConfigExplainReport(),
DEFAULT_PLUGIN_CONFIG and report.entries so you build both sets (explained from
report.entries and configKeys from DEFAULT_PLUGIN_CONFIG) and assert they are
equal (or assert both missing === [] and extras === []), ensuring no missing and
no extra explain rows remain.
In `@test/documentation.test.ts`:
- Around line 588-599: The test only checks pkg.overrides?.hono but not the
rollup override; add a rollupPin check by reading const rollupPin =
pkg.overrides?.rollup, assert expect(typeof rollupPin).toBe("string"), and
assert SECURITY.md contains the documented form (the SECURITY.md rationale
includes a careted pin), e.g. expect(security).toContain(`pinned to \`${'^' +
String(rollupPin)}\``) so the test verifies the rollup override in package.json
matches the form documented; place this next to the existing honoPin/assertions
in the same it block alongside pkg and security references.
In `@test/global-sandbox.test.ts`:
- Line 1: Remove the redundant explicit import of Vitest globals in the test
file by deleting the line that imports "{ describe, it, expect } from 'vitest'";
since vitest.config.ts enables globals (describe/it/expect) there’s no need to
import them in the test, so update test/global-sandbox.test.ts to rely on the
global names instead of the imported symbols.
In `@test/oauth-server.integration.test.ts`:
- Around line 21-37: The helper waitForPortFree currently returns silently on
timeout which hides flaky eaddrinuse failures; change its timeout behavior so it
throws an error when the port does not free within timeoutMs (e.g., throw new
Error indicating port and timeout) instead of returning; update the same pattern
used around the other occurrence (the similar block at lines ~43-48) so both
deterministically fail the test when the port remains occupied, referencing the
waitForPortFree function to locate the change.
In `@test/oc-chatgpt-orchestrator.test.ts`:
- Around line 232-233: Replace the bare fs.rm cleanup in the finally block of
the test (currently calling rm(dir, { recursive: true, force: true })) with the
repository's resilient helper by calling removeWithRetry(dir) (await it) so
transient Windows EBUSY/EPERM/ENOTEMPTY errors are retried; also ensure
removeWithRetry is imported or available in the test file before use and apply
the same change to the other teardown occurrence in the file
(`test/oc-chatgpt-orchestrator.test.ts`) so both cleanup sites use
removeWithRetry instead of fs.rm.
In `@test/prompt-fetch-utils.test.ts`:
- Around line 1-7: Remove the named imports of describe, it, and expect from
vitest and rely on the test globals (keep importing vi), then update the test
that currently exercises only the happy abort path to also add a
concurrency/timing regression that simulates an abort race against fetch
resolution (e.g., use a controllable/mock fetch that resolves after a delay
while calling AbortController.abort earlier or later) to exercise
fetchWithTimeout’s abort handling and ensure correct behavior when abort fires
before/after fetch resolution; keep tests related to readBodyTextGuarded,
withPromptFetchHeaders, and PROMPT_FETCH_MAX_BYTES as needed and drop any
Windows-specific EBUSY/EPERM/ENOTEMPTY cleanup logic since it’s not applicable
here.
In `@test/recovery-storage.test.ts`:
- Around line 157-165: Add a deterministic Vitest regression case that simulates
the Windows-lock transient by making fsMock.renameSync throw an EBUSY/EPERM
error on the first call and succeed on the second: update the test around the
existing quarantine assertions (referencing fsMock.renameSync and
storage.getRecoveryCorruptionStats) to set
fsMock.renameSync.mockImplementationOnce(() => { throw Object.assign(new
Error("EBUSY"), { code: "EBUSY" }) }) and then mockImplementation(() =>
undefined) so the rename is retried and eventually succeeds; assert renameSync
was called at least twice (or with the expected sequence), verify the
quarantined filename contains ".corrupt-" and that
storage.getRecoveryCorruptionStats().corruptFileCount and quarantinedPaths
reflect the file, keeping the test deterministic and using Vitest mocks.
In `@test/runtime-rotation-proxy.test.ts`:
- Around line 345-360: The test "allows a non-loopback host only with the
explicit opt-in" is not exercising the opt-in because it binds to loopback
"127.0.0.1"; update the test that calls startRuntimeRotationProxy (in
test/runtime-rotation-proxy.test.ts) to use a non-loopback bind such as
"0.0.0.0" (while keeping allowNonLoopbackHost: true and other args like
clientApiKey/upstreamBaseUrl) so the guarded branch in
lib/runtime-rotation-proxy.ts that checks allowNonLoopbackHost is actually
exercised.
---
Outside diff comments:
In `@lib/prompts/codex.ts`:
- Around line 170-190: The fallback path uses htmlResponse.text() which can read
an unbounded response; replace that call with the same bounded reader used
elsewhere (e.g., readBodyTextGuarded) to enforce the size cap: when handling the
htmlResponse after failing to extract the tag, call the guarded reader (passing
htmlResponse and the same size limit used for prompt content) instead of
htmlResponse.text(), then continue using the returned string to parse the
release and update latestReleaseTagCache; ensure you import or reference the
same readBodyTextGuarded utility and preserve existing variables like
htmlResponse, finalUrl and latestReleaseTagCache.
- Around line 245-301: The code currently rejects on-disk cache when sha256
mismatches but still falls back to that same diskContent on fetch failure; fix
by introducing a trustedDiskContent (or trustedCache) variable set only when
integrityOk is true and use that for all cache reuse paths (e.g., in the catch
fallback after fetchAndPersistInstructions and in stale-serving logic), ensuring
that when integrityOk is false you never return diskContent but instead fall
back to bundled content; update logic around cachedMetadata, diskContent,
setCacheEntry, refreshInstructionsInBackground and fetchAndPersistInstructions
accordingly. Also add a deterministic vitest regression that simulates "sha
mismatch + fetch failure" (mock fetchAndPersistInstructions to throw and ensure
the function does not return the rejected diskContent) to cover this case.
In `@test/account-clear.test.ts`:
- Around line 20-25: The afterEach cleanup uses a bare fs.rm on testTmpRoot
which can fail on Windows; replace it with the shared retry helper
(removeWithRetry) or wrap the removal in a retry/backoff loop: call
removeWithRetry(testTmpRoot) inside the afterEach teardown (or implement a local
retry wrapper named removeWithRetry and use that) to ensure transient locks from
antivirus don't cause flakey test failures.
In `@test/helpers/global-sandbox.ts`:
- Around line 1-31: The temp sandbox created by mkdtempSync and stored in
SANDBOX_ROOT is never removed, leading to accumulated dirs in tmpdir; add
teardown logic to remove SANDBOX_ROOT after tests complete (e.g., register a
globalTeardown or an afterAll/process.on('exit') cleanup that deletes the
directory recursively), locate the creation in test/helpers/global-sandbox.ts
(SANDBOX_ROOT, mkdtempSync) and ensure the teardown runs once per worker and
safely ignores missing paths or errors.
🪄 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: 60e6c9dd-0864-4829-9f51-c39a2d892a81
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (55)
AGENTS.mdSECURITY.mdlib/accounts.tslib/circuit-breaker.tslib/codex-manager.tslib/codex-manager/commands/debug-bundle.tslib/config.tslib/context-overflow.tslib/local-bridge.tslib/logger.tslib/oc-chatgpt-orchestrator.tslib/policy/runtime-policy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/prompts/host-codex-prompt.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/rotation.tslib/runtime-rotation-proxy.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/storage/paths.tslib/storage/storage-parser.tslib/types.tslib/ui/theme.tspackage.jsonscripts/codex-routing.jstest/account-clear.test.tstest/accounts.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/codex-routing.test.tstest/config-explain.test.tstest/context-overflow.test.tstest/documentation.test.tstest/global-sandbox.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/local-bridge.test.tstest/logger.test.tstest/oauth-server.integration.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/paths.test.tstest/plugin-config.test.tstest/prompt-fetch-utils.test.tstest/property/setup.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/runtime-rotation-proxy.test.tstest/ui-format.test.tstest/ui-theme.test.tsvitest.config.ts
📜 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 (23)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errorsImplement email dedup using
normalizeEmailKey()with case-insensitive trim and lowercase normalizationEnable runtime rotation by default through
codexRuntimeRotationProxywith opt-out viacodex-multi-auth rotation disableorCODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0
Files:
lib/circuit-breaker.tslib/storage/account-clear.tslib/policy/runtime-policy.tslib/storage/flagged-storage-io.tslib/types.tslib/codex-manager/commands/debug-bundle.tslib/local-bridge.tslib/oc-chatgpt-orchestrator.tslib/storage/storage-parser.tslib/rotation.tslib/storage/import-export.tslib/refresh-lease.tslib/accounts.tslib/prompts/fetch-utils.tslib/context-overflow.tslib/ui/theme.tslib/storage/paths.tslib/codex-manager.tslib/config.tslib/logger.tslib/recovery/storage.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tslib/refresh-queue.tslib/runtime-rotation-proxy.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax only with
type: modulein package.json; target Node >= 18Avoid
as any,@ts-ignore, and@ts-expect-errortype assertions
Files:
lib/circuit-breaker.tstest/config-explain.test.tstest/package-bin.test.tslib/storage/account-clear.tslib/policy/runtime-policy.tstest/ui-format.test.tsvitest.config.tslib/storage/flagged-storage-io.tstest/property/setup.test.tslib/types.tstest/global-sandbox.test.tstest/codex-routing.test.tstest/plugin-config.test.tslib/codex-manager/commands/debug-bundle.tstest/context-overflow.test.tstest/refresh-lease.test.tstest/recovery-storage.test.tstest/account-clear.test.tstest/documentation.test.tslib/local-bridge.tslib/oc-chatgpt-orchestrator.tslib/storage/storage-parser.tstest/runtime-rotation-proxy.test.tslib/rotation.tstest/host-codex-prompt.test.tstest/helpers/global-sandbox.tstest/accounts.test.tstest/logger.test.tslib/storage/import-export.tstest/oc-chatgpt-orchestrator.test.tslib/refresh-lease.tslib/accounts.tslib/prompts/fetch-utils.tstest/paths.test.tstest/oauth-server.integration.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/prompt-fetch-utils.test.tslib/context-overflow.tslib/ui/theme.tslib/storage/paths.tslib/codex-manager.tstest/ui-theme.test.tslib/config.tslib/logger.tslib/recovery/storage.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tstest/codex-prompts.test.tslib/refresh-queue.tslib/runtime-rotation-proxy.ts
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/circuit-breaker.tslib/storage/account-clear.tslib/policy/runtime-policy.tslib/storage/flagged-storage-io.tslib/types.tslib/codex-manager/commands/debug-bundle.tslib/local-bridge.tslib/oc-chatgpt-orchestrator.tslib/storage/storage-parser.tslib/rotation.tslib/storage/import-export.tslib/refresh-lease.tslib/accounts.tslib/prompts/fetch-utils.tslib/context-overflow.tslib/ui/theme.tslib/storage/paths.tslib/codex-manager.tslib/config.tslib/logger.tslib/recovery/storage.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tslib/refresh-queue.tslib/runtime-rotation-proxy.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/config-explain.test.tstest/package-bin.test.tstest/ui-format.test.tstest/property/setup.test.tstest/global-sandbox.test.tstest/codex-routing.test.tstest/plugin-config.test.tstest/context-overflow.test.tstest/refresh-lease.test.tstest/recovery-storage.test.tstest/account-clear.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/host-codex-prompt.test.tstest/accounts.test.tstest/logger.test.tstest/oc-chatgpt-orchestrator.test.tstest/paths.test.tstest/oauth-server.integration.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/prompt-fetch-utils.test.tstest/ui-theme.test.tstest/codex-prompts.test.ts
test/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for all tests with property tests and chaos tests support
Maintain docs integrity tests in the test suite
Files:
test/config-explain.test.tstest/package-bin.test.tstest/ui-format.test.tstest/property/setup.test.tstest/global-sandbox.test.tstest/codex-routing.test.tstest/plugin-config.test.tstest/context-overflow.test.tstest/refresh-lease.test.tstest/recovery-storage.test.tstest/account-clear.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/host-codex-prompt.test.tstest/helpers/global-sandbox.tstest/accounts.test.tstest/logger.test.tstest/oc-chatgpt-orchestrator.test.tstest/paths.test.tstest/oauth-server.integration.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/prompt-fetch-utils.test.tstest/ui-theme.test.tstest/codex-prompts.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/config-explain.test.tstest/package-bin.test.tstest/ui-format.test.tstest/property/setup.test.tstest/global-sandbox.test.tstest/codex-routing.test.tstest/plugin-config.test.tstest/context-overflow.test.tstest/refresh-lease.test.tstest/recovery-storage.test.tstest/account-clear.test.tstest/documentation.test.tstest/runtime-rotation-proxy.test.tstest/host-codex-prompt.test.tstest/helpers/global-sandbox.tstest/accounts.test.tstest/logger.test.tstest/oc-chatgpt-orchestrator.test.tstest/paths.test.tstest/oauth-server.integration.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/prompt-fetch-utils.test.tstest/ui-theme.test.tstest/codex-prompts.test.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/storage/import-export.tslib/storage/paths.ts
lib/storage/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Resolve project storage using
resolveProjectStorageIdentityRootinstead of keying by worktree path
Files:
lib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/storage/import-export.tslib/storage/paths.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
lib/{accounts,rotation}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use account health scoring and cooldowns in the account pool selection logic
Files:
lib/rotation.tslib/accounts.ts
scripts/codex-routing.js
📄 CodeRabbit inference engine (AGENTS.md)
Use
codex-routing.jsfor auth command and compatibility alias routing
Files:
scripts/codex-routing.js
scripts/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use Windows filesystem retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors in cleanup and write operationsDo not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling
Files:
scripts/codex-routing.js
scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex-routing.js
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
test/**/oauth-server.integration.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Bind OAuth server integration tests to port 1455; do not hardcode other ports
Files:
test/oauth-server.integration.test.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
lib/storage/paths.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement worktree resolution with repo identity root, linked-worktree detection, and commondir/gitdir validation
Files:
lib/storage/paths.ts
lib/codex-manager.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement CLI manager with command dispatcher and command modules in
lib/codex-manager.ts
Files:
lib/codex-manager.ts
lib/config.ts
📄 CodeRabbit inference engine (AGENTS.md)
Parse configuration from
pluginConfigwith environment overrides and provide config explain report
Files:
lib/config.ts
lib/prompts/codex.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement model-family prompt templates with GitHub ETag cache in
lib/prompts/codex.ts
Files:
lib/prompts/codex.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
In package.json overrides, pin hono to version 4.12.18 to avoid vulnerable versions 4.12.0-4.12.1 (GHSA-xh87-mx6m-69f3)
In package.json overrides, pin rollup to ^4.59.0 to avoid vulnerable versions <4.59.0
Files:
package.json
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesImplement loopback-only runtime rotation proxy with per-process client token, forwarding only Responses API and model discovery requests
Do not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:41.088Z
Learning: Do not edit `dist/` directory or local temp/cache directories; `dist/` is generated output
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:41.088Z
Learning: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: Use PKCE-based OAuth flow for handling OAuth credentials
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: Store sensitive credentials locally under ~/.codex/multi-auth (or CODEX_MULTI_AUTH_DIR environment variable), never in version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: Never commit auth files, logs, or cache artifacts to version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: Enable debug/body logging only for short-lived troubleshooting sessions; avoid enabling ENABLE_PLUGIN_REQUEST_LOGGING and CODEX_PLUGIN_LOG_BODIES in production
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: For PKCE OAuth implementation, ensure runtime rotation proxy is loopback-only (default), authenticated with local client key, and allow users to opt out with CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-05-31T19:03:47.576Z
Learning: Run npm run audit:ci, npm run lint, npm run typecheck, npm test, and npm run build before release and after dependency changes
🔇 Additional comments (23)
lib/runtime-rotation-proxy.ts (6)
128-136: LGTM!
468-479: LGTM!
1298-1308: LGTM!
1400-1411: LGTM!
2126-2132: LGTM!
987-1001: LGTM!Also applies to: 1065-1068
lib/accounts.ts (1)
1462-1473: ⚡ Quick winconfirm clearaccountkey exists on both trackers; cleanup path matches the tracker api
getHealthTracker().clearAccountKey(...)andgetTokenTracker().clearAccountKey(...)both exist on the tracker implementations inlib/rotation.ts:187andlib/rotation.ts:359(accountKey: TrackerKey).- the identity-keyed cleanup callsites in
lib/accounts.ts:1468-1469match that api, so the dependency concern is removed.- ensure vitest covers
accounts-02: removing an account and then re-adding the same identity (no stale health/token penalties), plus a concurrency race (remove/re-add interleaving) and any windows edge cases if identity keys are derived from filesystem paths.test/paths.test.ts (1)
808-821: ⚡ Quick winresolvepath symlink tests don’t need
realpathsync.nativestubbing
resolvepathusescanonicalizeexistingprefix, which callsrealpathsync(current)(notrealpathsync.native) inlib/storage/paths.ts:412-428, and then compares the canonical value inlib/storage/paths.ts:475-482. therealpathsync.nativepath only appears innormalizecanonicalpathforidentitycheck(lib/storage/paths.ts:116-128), whichresolvepathdoes not use.the two regressions are in
test/paths.test.ts:801-823;beforeeachalready setsmockedrealpathsyncnativeto identity (test/paths.test.ts:51-63), so leavingmockedrealsyncnativeun-overridden cannot make these tests go false-green.optional: add a win32-specific
resolvepathcontainment regression (currentresolvepathtests are platform-agnostic intest/paths.test.ts:776-923), and add concurrency coverage only where there’s shared state (none here forresolvepath).> Likely an incorrect or invalid review comment.lib/storage/account-clear.ts (2)
2-7: LGTM!
14-80: LGTM!vitest.config.ts (1)
34-47: LGTM!lib/storage/flagged-storage-io.ts (2)
6-10: LGTM!
61-77: LGTM!test/account-clear.test.ts (1)
44-50: LGTM!lib/oc-chatgpt-orchestrator.ts (1)
162-178: LGTM!lib/storage/paths.ts (1)
403-432: well-designed symlink canonicalization with proper fallback.
lib/storage/paths.ts:412-432walks up the path tree to find the deepest existing ancestor, then canonicalizes viarealpathSync. the loop limit at line 416 prevents infinite loops, and the error fallback at lines 429-431 degrades gracefully.test/ui-format.test.ts (1)
20-22: LGTM!test/property/setup.test.ts (1)
6-14: LGTM!test/global-sandbox.test.ts (1)
4-31: LGTM!test/local-bridge.test.ts (1)
133-160: LGTM!test/codex-manager-cli.test.ts (1)
58-66: LGTM!Also applies to: 1233-1242
lib/config.ts (2)
1934-1951: LGTM!
279-279: LGTM!
|
Summary
Testing
|
- recovery-02: readMessages sort comparator dereferenced a.id/b.id outside the per-file try/catch, so a parseable record missing `id` threw out of the sort and crashed the read. Guard the id access. - recovery-03: injectTextPart / prependThinkingPart / stripThinkingParts / replaceEmptyTextParts joined messageID into a filesystem path with no validatePathId (only the read path validated). Add the same guard so a crafted messageID cannot escape PART_STORAGE. - recovery-05: stripThinkingParts reported success when ANY part was removed even if a TARGETED thinking part failed to delete, so auto-resume treated the message as clean and retried forever (burning quota). Now it returns true only when every targeted thinking part was actually removed. Tests: missing-id no-throw; unsafe-messageID rejection on all four mutate helpers; strip returns false on a failed targeted delete. Verified: tsc + lint clean; full suite 271 files / 4107 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- quota-forecast-01: evaluateRuntimePolicy read the capability store under getAccountPolicyKey, but the recordUnsupported sites WRITE under resolveEntitlementAccountKey. The keys never matched, so unsupported-model suppression was dead. Read under the same entitlement key. Regression test records an unsupported model and asserts the account is blocked. - quota-forecast-02: a quota window at 100% used with no resetAtMs read as exhausted forever. When updatedAt + windowMinutes have elapsed, treat the window as rolled over (conservative synthesized expiry). Regression tests cover before/after the window boundary. Not changed (documented): quota-forecast-04 (sharing resolveNormalizedModel between entitlement-cache and the capability matrix is unsafe — the canonical normalizer collapses aliases like gpt-5-codex -> gpt-5.3-codex for routing, but entitlement identity must keep them distinct; the existing tests confirm this, so the two normalizations are intentionally separate). quota-forecast-05 (persisting in-memory capability/scheduler state) is a larger change left for a follow-up. Verified: tsc + lint clean; full suite 271 files / 4110 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…02/03) - ui-01: truncateAnsi appends a reset when the kept text contains an ANSI escape, so a color opened before the cut cannot bleed past the truncation point into the rest of the line. - ui-02: add a dependency-free display-width helper (CJK/fullwidth/hangul/ emoji = 2 cols, combining marks/ZWJ = 0) and pad+truncate table cells by display columns instead of UTF-16 code units, so CJK/emoji content stays aligned and wide glyphs are never split across the boundary. - ui-03: the quota bar honors glyphMode (Unicode block glyphs only in unicode mode, ASCII #/- otherwise) instead of hardcoding block glyphs that render as mojibake on ascii terminals. Tests: 8 display-width unit tests + CJK table-alignment/truncation regressions. Verified: tsc + lint clean; full suite 272 files / 4120 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…b-01) The experimental settings panel hardcoded proactiveRefreshIntervalMs bounds (min 60000, step 60000) that contradicted the backend settings schema (min 5000, step 5000) for the same setting, so the two panels clamped/stepped the value differently. Derive the panel's min/max/step from the single BACKEND_NUMBER_OPTION_BY_KEY schema entry so they cannot diverge. Updated the tests that encoded the old divergent 60000 step (3 in settings-hub-utils, 2 in codex-manager-cli that drive the same hotkeys through the full login flow). Note: settings-hub-02 was investigated and is NOT a bug against current code — savePluginConfig already reads-merges under withConfigSaveLock and buildBackendConfigPatch emits only scoped backend keys, so there is no full-snapshot lost-update window. Verified: tsc + lint clean; full suite 272 files / 4120 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…equest-01) RFC 8594 Deprecation/Sunset headers were logged only by handleSuccessResponse. Upstream often attaches them to error responses (e.g. a sunset endpoint returning 4xx/410), so they were silently dropped there. Extract a shared logDeprecationHeaders helper and call it from both handleSuccessResponse and handleErrorResponse. Test: an error response carrying a Sunset header logs the deprecation warning. Verified: tsc + lint clean; full suite 272 files / 4121 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- tests-ci-06: pin a deterministic fast-check seed (override via FAST_CHECK_SEED) so property-test failures are reproducible from CI logs instead of using a fresh random seed each run. - tests-ci-05: PR CI runs `npm run coverage` instead of `npm test`, so the 80% coverage threshold gates PRs and not only the post-merge push-to-main run in ci.yml. Guard tests assert the pinned seed and the PR coverage step. Verified: tsc + lint clean; full suite 272 files / 4123 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/policy/runtime-policy.ts (1)
119-126:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftincludearchives:true is correct logically, but it does a full archive scan+jsonl parse on the per-request hot path
- lib/runtime-rotation-proxy.ts:1452 calls evaluateRuntimePolicy per request, and evaluateBudgets sets
includeArchives: truewhen awaiting summarizeUsageLedger (lib/policy/runtime-policy.ts:125), multiplying this work by every budget key in the request.- summarizeUsageLedger delegates to readUsageLedgerRows (lib/usage/ledger.ts:523-526); with archives enabled, readUsageLedgerRows enumerates all matching archived ledger files via fs.readdir + regex (lib/usage/ledger.ts:402-413) and then reads+parses every file (lib/usage/ledger.ts:430-446). there’s no request-scoped caching here, so cost grows with archive history and repeats each request.
- concurrency: rotateUsageLedger renames under an append lock (lib/usage/ledger.ts:536, lib/usage/ledger.ts:553), but readUsageLedgerRows does not take that lock; archive reads that race with rotate get logged and dropped (lib/usage/ledger.ts:438-443). retry only covers ebusy/eperm (lib/usage/ledger.ts:55, lib/usage/ledger.ts:215), so rename/enoent races can still undercount.
- logging: the relevant logWarns don’t include raw tokens/emails (lib/usage/ledger.ts:393-397, lib/usage/ledger.ts:438-443).
tests
- includeArchives correctness exists for ledger rotation (test/usage-ledger.test.ts:113-155), but runtime-policy’s budget test only checks “budget exhausted => 429” without a rotated-archive window scenario (test/runtime-policy.test.ts:113-139).
recommendations
- add request-scoped memoization of summarizeUsageLedger (keyed by since/until/includeArchives + budget key) inside evaluateRuntimePolicy/evaluateBudgets, or compute once in the rotation proxy and reuse across keys.
- add a vitest regression for quota-forecast-03 (rotated archive spanning the budget window) that asserts the 429 block still happens (test/runtime-policy.test.ts:113-139).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/policy/runtime-policy.ts` around lines 119 - 126, summarizeUsageLedger with includeArchives:true is causing expensive per-request archive scans when evaluateRuntimePolicy/evaluateBudgets is called for each budget key; change evaluateRuntimePolicy (or evaluateBudgets) to memoize summarizeUsageLedger results per request (keyed by since, until, includeArchives and budget key) or compute the summary once in runtime-rotation-proxy and reuse it across budget keys, so readUsageLedgerRows is not invoked repeatedly; also add a vitest regression in test/runtime-policy.test.ts that simulates a rotated archive spanning the budget window (quota-forecast-03) and asserts the 429 behavior remains; optionally, harden readUsageLedgerRows to retry/handle rename/ENOENT races during rotateUsageLedger (or coordinate via the same append lock) and ensure logWarn entries include sufficient non-sensitive context to aid debugging (reference evaluateRuntimePolicy, evaluateBudgets, summarizeUsageLedger, readUsageLedgerRows, rotateUsageLedger).
♻️ Duplicate comments (2)
lib/recovery/storage.ts (1)
266-272:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftstill quarantining on transient read errors.
lib/recovery/storage.ts:266-272andlib/recovery/storage.ts:301-307callquarantineCorruptFileon anyreadFileSync()/JSON.parse()failure. windows filesystem locks (EBUSY/EPERM) and concurrent read races (ENOENT) are not corruption—they're transient, and renaming those files hides healthy recovery state. you haverenameSyncWithRetrywith bounded backoff for write-side races, but the read path has no retry. wrap thereadFileSyncin a similar retry loop (max 4 attempts, exponential backoff, checkingRETRYABLE_FS_CODES) before deciding the file is corrupt. only callquarantineCorruptFileafter a successful read followed by parse/validation failure, or after exhausting retries on a genuine read fault. add regression coverage intest/recovery-storage.test.tsfor a locked-file read race asserting retries occur and the file is not quarantined. as per coding guidelineslib/**: focus on auth rotation, windows filesystem io, and concurrency.Also applies to: 301-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/recovery/storage.ts` around lines 266 - 272, The read path currently calls quarantineCorruptFile on any read/parse error; instead wrap the readFileSync call in a retry loop (max 4 attempts) with exponential backoff using the same RETRYABLE_FS_CODES logic as renameSyncWithRetry to retry transient FS errors (EBUSY/EPERM/ENOENT) before treating as corrupt; only invoke quarantineCorruptFile if JSON.parse succeeds but validation fails or if all retry attempts are exhausted and the final error is non-retryable; update the code blocks around readFileSync/JSON.parse in the functions that push messages (refer to readFileSync, JSON.parse, quarantineCorruptFile) and add a regression test in test/recovery-storage.test.ts that simulates a locked-file read race, asserts retry attempts occur, and verifies the file is not quarantined.test/recovery-storage.test.ts (1)
157-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winmissing windows lock retry regression for quarantine rename.
test/recovery-storage.test.ts:157-165proves the happy-path quarantine but doesn't cover the windows-sensitive branch when the quarantinerenameSynchits a transientEBUSY/EPERMbefore succeeding. the storage hardening inlib/recovery/storage.ts:38-56is designed to handle that race with a fallback log when rename fails, but you have no deterministic test proving it works. add a test that makesfsMock.renameSync.mockImplementationOnce(() => { throw Object.assign(new Error("EBUSY"), { code: "EBUSY" }) })on the first call and thenmockImplementation(() => undefined)so the rename retries and eventually succeeds (or fails gracefully). assertrenameSyncwas called at least twice and verify the corruption stats still reflect the quarantine attempt. as per coding guidelinestest/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/recovery-storage.test.ts` around lines 157 - 165, Add a deterministic regression test to recovery-storage.test that simulates the Windows transient rename race by configuring fsMock.renameSync to throw an Error with code "EBUSY" (or "EPERM") on the first invocation and succeed thereafter, then invoke the quarantine path that triggers fsMock.renameSync (the same flow exercised by storage.quarantine or the recovery handler in lib/recovery/storage.ts) and assert renameSync was called at least twice, and that storage.getRecoveryCorruptionStats().quarantinedPaths includes "bad.json" and corruptFileCount was incremented; use vitest mockImplementationOnce + mockImplementation to keep the test deterministic and ensure the windows-sensitive retry/fallback branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/quota-readiness.ts`:
- Around line 85-98: Add regression tests in test/quota-readiness.test.ts
exercising the staleness-escape branch in lib/quota-readiness.ts for edge cases:
(1) missing updatedAt (omit updatedAt field), (2) missing/invalid
window.windowMinutes (undefined, 0, negative), and (3) updatedAt in the future
(updatedAt > now). For each case, assert that the staleness escape block (the
check using typeof window?.resetAtMs, typeof updatedAt, typeof
window?.windowMinutes and now >= updatedAt + window.windowMinutes * 60_000) does
NOT incorrectly return false — i.e., the code should treat these entries as not
eligible for the "stop treating as exhausted" shortcut and preserve the original
exhausted behavior; also keep existing happy-path tests for comparison.
In `@lib/ui/select.ts`:
- Around line 92-96: Add vitest tests that cover ANSI reset placement for the
truncation logic in lib/ui/select.ts: call the truncate/truncation function (the
code that produces output, suffix and uses the reset variable) with (1)
truncated ANSI-colored input and assert the resulting string ends with suffix +
"\x1b[0m", (2) non-truncated colored input and assert no extra "\x1b[0m" is
appended after the suffix, (3) plain (non-ANSI) input and assert no "\x1b[0m"
anywhere, and (4) input with multiple ANSI escapes and assert the final result
still ends with a single trailing "\x1b[0m"; place these tests alongside the
existing ui tests (e.g. in test/ui-format.test.ts or a new
test/ui-select.test.ts) and use the same helper/exports that produce the
output/suffix so you reference the same truncation function/variables.
In `@test/display-width.test.ts`:
- Line 1: Remove the redundant explicit import of Vitest globals by deleting the
line that imports describe, it, and expect from "vitest"; since describe, it,
and expect are enabled as globals in this project you should use them without
importing—locate the import statement that reads import { describe, it, expect }
from "vitest" in the test file and remove it, leaving the rest of the tests
unchanged.
- Around line 21-24: The test uses literal characters that may be normalized or
invisible—update the assertions to use explicit Unicode escapes so the
combining-mark and ZWJ branches of displayWidth are actually exercised: replace
the precomposed "é" with "e\u0301" (COMBINING ACUTE ACCENT) to hit the combining
range and replace the literal ZWJ in "ab" with "a\u200Db" (ZERO WIDTH JOINER);
keep the same expect(displayWidth(...)).toBe(...) assertions so the test
reliably covers displayWidth's zero-width handling.
In `@test/quota-readiness.test.ts`:
- Around line 78-85: Add a boundary test to clarify whether
isQuotaCacheEntryExhausted treats the exact window boundary as expired: create a
new case in the same test suite that uses the same entry shape (primary: {
usedPercent: 100, windowMinutes: 300 }, updatedAt) and assert the result at time
= updatedAt + windowMinutes * 60_000 (exact boundary) — expect true if the
implementation treats the boundary as expired or false if it treats it as
inclusive; name the test to document the expected behavior and reference
isQuotaCacheEntryExhausted so reviewers can see the intended boundary semantics.
- Around line 62-76: Add a symmetric test to cover secondary-only exhaustion
when resetAtMs is absent: create a new it block in test/quota-readiness.test.ts
that mirrors the existing case but with primary.usedPercent: 10 and
secondary.usedPercent: 100 (same windowMinutes values and updatedAt), then
assert isQuotaCacheEntryExhausted(entry, updatedAt + 60_000) returns true and
isQuotaCacheEntryExhausted(entry, updatedAt + windowMinutes * 60_000 + 1)
returns false; this ensures isQuotaCacheEntryExhausted handles implicit window
expiration for the secondary window as it does for primary.
---
Outside diff comments:
In `@lib/policy/runtime-policy.ts`:
- Around line 119-126: summarizeUsageLedger with includeArchives:true is causing
expensive per-request archive scans when evaluateRuntimePolicy/evaluateBudgets
is called for each budget key; change evaluateRuntimePolicy (or evaluateBudgets)
to memoize summarizeUsageLedger results per request (keyed by since, until,
includeArchives and budget key) or compute the summary once in
runtime-rotation-proxy and reuse it across budget keys, so readUsageLedgerRows
is not invoked repeatedly; also add a vitest regression in
test/runtime-policy.test.ts that simulates a rotated archive spanning the budget
window (quota-forecast-03) and asserts the 429 behavior remains; optionally,
harden readUsageLedgerRows to retry/handle rename/ENOENT races during
rotateUsageLedger (or coordinate via the same append lock) and ensure logWarn
entries include sufficient non-sensitive context to aid debugging (reference
evaluateRuntimePolicy, evaluateBudgets, summarizeUsageLedger,
readUsageLedgerRows, rotateUsageLedger).
---
Duplicate comments:
In `@lib/recovery/storage.ts`:
- Around line 266-272: The read path currently calls quarantineCorruptFile on
any read/parse error; instead wrap the readFileSync call in a retry loop (max 4
attempts) with exponential backoff using the same RETRYABLE_FS_CODES logic as
renameSyncWithRetry to retry transient FS errors (EBUSY/EPERM/ENOENT) before
treating as corrupt; only invoke quarantineCorruptFile if JSON.parse succeeds
but validation fails or if all retry attempts are exhausted and the final error
is non-retryable; update the code blocks around readFileSync/JSON.parse in the
functions that push messages (refer to readFileSync, JSON.parse,
quarantineCorruptFile) and add a regression test in
test/recovery-storage.test.ts that simulates a locked-file read race, asserts
retry attempts occur, and verifies the file is not quarantined.
In `@test/recovery-storage.test.ts`:
- Around line 157-165: Add a deterministic regression test to
recovery-storage.test that simulates the Windows transient rename race by
configuring fsMock.renameSync to throw an Error with code "EBUSY" (or "EPERM")
on the first invocation and succeed thereafter, then invoke the quarantine path
that triggers fsMock.renameSync (the same flow exercised by storage.quarantine
or the recovery handler in lib/recovery/storage.ts) and assert renameSync was
called at least twice, and that
storage.getRecoveryCorruptionStats().quarantinedPaths includes "bad.json" and
corruptFileCount was incremented; use vitest mockImplementationOnce +
mockImplementation to keep the test deterministic and ensure the
windows-sensitive retry/fallback branch is covered.
🪄 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: de5f1ace-0055-44fa-a55c-e0dd61ffff02
📒 Files selected for processing (21)
.github/workflows/pr-ci.ymllib/codex-manager/experimental-settings-prompt.tslib/policy/runtime-policy.tslib/quota-readiness.tslib/recovery/storage.tslib/request/fetch-helpers.tslib/table-formatter.tslib/ui/auth-menu.tslib/ui/display-width.tslib/ui/select.tstest/ci-workflows.test.tstest/codex-manager-cli.test.tstest/display-width.test.tstest/fetch-helpers.test.tstest/property/setup.test.tstest/property/setup.tstest/quota-readiness.test.tstest/recovery-storage.test.tstest/runtime-policy.test.tstest/settings-hub-utils.test.tstest/table-formatter.test.ts
📜 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 (10)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/runtime-policy.test.tstest/table-formatter.test.tstest/property/setup.test.tstest/display-width.test.tstest/ci-workflows.test.tstest/quota-readiness.test.tstest/settings-hub-utils.test.tstest/fetch-helpers.test.tstest/codex-manager-cli.test.tstest/recovery-storage.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions in TypeScript filesDo not hardcode OAuth ports; use existing constants/helpers for port configuration
Files:
test/runtime-policy.test.tstest/property/setup.tslib/ui/select.tstest/table-formatter.test.tstest/property/setup.test.tstest/display-width.test.tslib/ui/auth-menu.tstest/ci-workflows.test.tslib/ui/display-width.tslib/quota-readiness.tstest/quota-readiness.test.tstest/settings-hub-utils.test.tslib/table-formatter.tslib/request/fetch-helpers.tstest/fetch-helpers.test.tslib/policy/runtime-policy.tslib/codex-manager/experimental-settings-prompt.tstest/codex-manager-cli.test.tslib/recovery/storage.tstest/recovery-storage.test.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM module syntax; the project is ESM-only with
"type": "module"and Node >= 18
Files:
test/runtime-policy.test.tstest/property/setup.tslib/ui/select.tstest/table-formatter.test.tstest/property/setup.test.tstest/display-width.test.tslib/ui/auth-menu.tstest/ci-workflows.test.tslib/ui/display-width.tslib/quota-readiness.tstest/quota-readiness.test.tstest/settings-hub-utils.test.tslib/table-formatter.tslib/request/fetch-helpers.tstest/fetch-helpers.test.tslib/policy/runtime-policy.tslib/codex-manager/experimental-settings-prompt.tstest/codex-manager-cli.test.tslib/recovery/storage.tstest/recovery-storage.test.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem operations in tests must include retry handling for transient lock-related failures (
EBUSY/EPERM/ENOTEMPTY)
Files:
test/runtime-policy.test.tstest/property/setup.tstest/table-formatter.test.tstest/property/setup.test.tstest/display-width.test.tstest/ci-workflows.test.tstest/quota-readiness.test.tstest/settings-hub-utils.test.tstest/fetch-helpers.test.tstest/codex-manager-cli.test.tstest/recovery-storage.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/runtime-policy.test.tstest/property/setup.tstest/table-formatter.test.tstest/property/setup.test.tstest/display-width.test.tstest/ci-workflows.test.tstest/quota-readiness.test.tstest/settings-hub-utils.test.tstest/fetch-helpers.test.tstest/codex-manager-cli.test.tstest/recovery-storage.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/ui/select.tslib/ui/auth-menu.tslib/ui/display-width.tslib/quota-readiness.tslib/table-formatter.tslib/request/fetch-helpers.tslib/policy/runtime-policy.tslib/codex-manager/experimental-settings-prompt.tslib/recovery/storage.ts
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/ui/select.tslib/ui/auth-menu.tslib/ui/display-width.tslib/quota-readiness.tslib/table-formatter.tslib/request/fetch-helpers.tslib/policy/runtime-policy.tslib/codex-manager/experimental-settings-prompt.tslib/recovery/storage.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: The canonical package name is `codex-multi-auth` and the canonical command family is `codex-multi-auth ...`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: The package does not publish a global `codex` bin; use explicit `codex-multi-auth-codex` wrapper for forwarding official Codex commands
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: Local project-owned state defaults to `~/.codex/multi-auth`; official Codex state remains under `~/.codex` and must not be modified directly
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: Settings Q hotkey cancels without save; theme live-preview must restore baseline on cancel
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: The runtime proxy is loopback-only and uses a per-process client token; it forwards only Responses API and model discovery requests
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:34.725Z
Learning: The persistent desktop app bind is reversible and edits user config/startup metadata, not official app binaries
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: Run security audit checks before release and after dependency changes with npm run audit:ci
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: Run lint, typecheck, tests, and build verification before release and after dependency changes
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: Do not share ~/.codex/ directories
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: Review connected apps in ChatGPT settings periodically
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: For vulnerability reports, do not open a public issue; contact maintainer privately via GitHub profile contact channel within 48 hours target response time
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T06:25:44.262Z
Learning: Prepare fixes before public disclosure and coordinate timing to reduce user risk
🔇 Additional comments (17)
.github/workflows/pr-ci.yml (1)
47-50: LGTM!lib/table-formatter.ts (1)
27-41: LGTM!lib/ui/display-width.ts (1)
16-47: LGTM!the focused-range approach is fine and the zero/wide tables are documented. just noting for the record: emoji zwj sequences (e.g.
👨👩👧👦) overcount (each component +2) and a base char followed byU+FE0Fpresentation selector under-counts (base stays 1 but renders 2). both are called out in your docstring as out-of-scope, so no change requested here.test/property/setup.ts (1)
6-17: LGTM!test/table-formatter.test.ts (1)
64-79: LGTM!both cases check out against
formatCell: the pad case lands at 8 cols + 2 pad, and the truncate case keeps 4 wide glyphs (8 cols) + ellipsis since a 5th glyph won't fit in the reserved 9. solid regression coverage for the display-width path.lib/policy/runtime-policy.ts (1)
200-212: ⚡ Quick winentitlement-key read/write alignment matches, and suppression is pinned by a test
- lib/policy/runtime-policy.ts:200-212 reads the capability snapshot using
resolveEntitlementAccountKey({ accountId, email, index }).- lib/entitlement-cache.ts:65-68 shows
resolveEntitlementAccountKeybuilds the deterministic key only from trimmedaccountId/index(no filesystem dependence).- index.ts:1449-1452 computes
entitlementAccountKeyviaresolveEntitlementAccountKey({ accountId, email, refreshToken, index }), and index.ts:1830-1832 recordsrecordUnsupportedunder that sameentitlementAccountKey(therefreshTokenargument does not participate in the key).- test/runtime-policy.test.ts:84-111 already asserts that an account is blocked when its model was recorded unsupported using the aligned entitlement key.
- capabilityPolicyStore reads/writes are synchronous map operations (lib/capability-policy.ts:102-105, lib/capability-policy.ts:134-146), so in-process concurrency/reentrancy should not drop updates.
lib/codex-manager/experimental-settings-prompt.ts (1)
95-104: LGTM!Also applies to: 163-166, 173-176
lib/quota-readiness.ts (1)
104-104: LGTM!Also applies to: 109-112
lib/request/fetch-helpers.ts (1)
936-942: LGTM!Also applies to: 948-951, 1009-1010
lib/ui/auth-menu.ts (1)
291-298: LGTM!test/codex-manager-cli.test.ts (1)
10305-10310: LGTM!Also applies to: 10548-10555
test/ci-workflows.test.ts (1)
49-54: LGTM!test/fetch-helpers.test.ts (1)
1007-1023: LGTM!test/property/setup.test.ts (1)
16-22: LGTM!test/recovery-storage.test.ts (1)
182-203: LGTM!Also applies to: 755-783
test/runtime-policy.test.ts (1)
84-111: LGTM!test/settings-hub-utils.test.ts (1)
764-807: LGTM!
status and list are the primary inspection commands but only emitted human text. Add a --json/-j branch that prints a single machine-readable object (storage path/health, account count, active/pinned/recommended indices, runtime-in-use index, and a per-account array with markers) built from the same data the text path renders. The empty-storage path emits a minimal JSON object too. Tests: populated + empty storage emit exactly one JSON object with the expected shape. Verified: tsc + lint clean; full suite 272/4125 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ows (chatgpt-import-06) planOcChatgptSync let loadTargetStorage (JSON-parse of an external file) and previewMerge throw uncaught, while the sibling applyOcChatgptSync wraps everything and returns a structured result. A corrupt/unreadable target file therefore crashed planning instead of surfacing a friendly status. Add a "plan-error" result variant carrying the error + cause (load|preview); applyOcChatgptSync maps it onto its existing error variant, and the experimental settings panel surfaces the real failure message instead of a generic "unavailable". Tests: plan-error on loader throw (cause: load) and on preview throw (cause: preview). Verified: tsc + lint clean; full suite 272/4127 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
savePluginConfig writes to CODEX_MULTI_AUTH_CONFIG_PATH first when that env var is set, but loadPluginConfig preferred unified settings and only fell to the env/legacy path when unified was absent. With both an env path and a unified file present, a save went to the env path while the next load read unified — a split-brain where the saved value was invisible. loadPluginConfig now reads the env path first when it is set and exists, mirroring the save precedence (env > unified > legacy). Test: with the env path set, load returns the env file's value (not unified). Verified: tsc + lint clean; full suite 272/4128 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…me-proxy-03) The local bridge had no way to present a client token, so it could not talk to a runtime proxy that requires a per-process client key. Add a runtimeClientApiKey option; when set, the bridge replaces the inbound (already bridge-validated) Authorization with that key on the forwarded request. When unset, inbound Authorization is stripped rather than forwarded verbatim, so the caller's bridge token is never leaked upstream (reinforces runtime-proxy-02). Tests: forwarded request carries the configured runtime key; inbound auth is stripped when no key is set. Verified: tsc + lint clean; full suite 272/4130. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
scripts/preuninstall.js was shipped in files[] and unit-tested but had no "preuninstall" entry in package.json scripts, so it never ran on npm uninstall — dead cleanup. Wire it as the lifecycle hook next to postinstall. The script is already self-guarding (its top-level catch forces exitCode 0), so it cannot fail an uninstall. Test: package.json registers preuninstall -> scripts/preuninstall.js and ships the script. Verified: tsc + lint clean; full suite 272/4131 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ll-scripts-01) verify-vendor-provenance only hashed the files listed in the manifest, so a rogue/extra file added to a vendored dir passed silently — provenance proved the listed files were intact but never that the directory contained nothing else. After verifying listed files, recursively enumerate each component's root and fail if any on-disk file is not declared in vendor/provenance.json. Verified empirically: a planted vendor/codex-ai-sdk/dist/rogue.js makes the verifier exit 1 with an "Unlisted vendor file(s)" error; removing it restores the clean "2 component(s), 8 file(s) verified" pass. Lint clean; full suite 272 files / 4131 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/oc-chatgpt-orchestrator.ts (1)
183-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winretry the final rename on windows lock errors.
lib/oc-chatgpt-orchestrator.ts:197still does a singlefs.rename(tempPath, path). on windows, av scanners and file watchers commonly make that step fail with transientebusy/eperm, which turns a good merged write into a sync failure even though the temp file is already on disk. reuse the bounded backoff pattern fromlib/config.ts:447here, and add a vitest regression that forces rename to fail a few times before succeeding. as per coding guidelineslib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/oc-chatgpt-orchestrator.ts` around lines 183 - 205, persistMergedDefault currently does a single fs.rename(tempPath, path) which fails on Windows under transient EBUSY/EPERM; update persistMergedDefault to retry the final rename using the bounded backoff retry pattern used in lib/config.ts (same max retries/delay strategy), catching and retrying only transient errors (EBUSY, EPERM, EMFILE) and rethrowing others, ensure the temp unlink in the finally block still runs (and does not swallow a successful rename), and add a vitest regression that mocks fs.rename to fail the first N attempts with EBUSY/EPERM then succeed to verify retries; reference persistMergedDefault, tempPath, and the rename call in the test and ensure the new test is cited in test outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/config.ts`:
- Around line 269-284: The precedence for loading plugin config was flipped to
prefer CODEX_MULTI_AUTH_CONFIG_PATH in loadPluginConfig()/the block around
envConfigPath, but resolveStoredPluginConfigRecord() (used by
getPluginConfigExplainReport()) still checks unified first; update
resolveStoredPluginConfigRecord() to mirror the new precedence by checking the
env override (process.env.CODEX_MULTI_AUTH_CONFIG_PATH and existsSync) and
reading/parsing that file first, falling back to loadUnifiedPluginConfigSync()
only when the env path is unset or missing, and ensure it sets
storageKind/configPath/entry source consistently with loadPluginConfig(); also
add a vitest regression that sets CODEX_MULTI_AUTH_CONFIG_PATH and verifies
getPluginConfigExplainReport() reports storageKind "file" and the correct
configPath when the env file is present.
In `@lib/local-bridge.ts`:
- Around line 175-177: When initializing the bridge, fail fast if auth is
disabled but a runtime client API key is provided: detect the combination where
requireAuth === false and runtimeClientApiKey is set and throw a clear Error to
reject it; update the initialization logic that defines requireAuth,
verifyBearerToken, and runtimeClientApiKey (and the code path that forwards
`bearer ${runtimeClientApiKey}` around the request-forwarding block) so it never
proceeds with forwarding when auth is disabled; adjust tests expecting
successful startup to expect rejection instead.
In `@scripts/verify-vendor-provenance.mjs`:
- Around line 13-36: The walk loop inside listFilesUnder currently only handles
entry.isDirectory() and entry.isFile() and silently ignores symlinks/other
dirent types; update the loop in the walk function used by listFilesUnder to
explicitly detect entry.isSymbolicLink() (and any other unsupported types) and
fail closed by throwing an Error that includes the offending path (childRel) and
a clear message, rather than skipping them, so any symlink or unexpected Dirent
causes verification to abort.
In `@test/codex-manager-status-command.test.ts`:
- Around line 351-382: Add a new vitest that exercises the CLI plumbing by
invoking runCodexMultiAuthCli with the "status" and "list" aliases and the json
flags (e.g., ["status","-j"], ["list","--json"]) so the mapping code that sets
the json flag is exercised; create deps similar to createStatusDeps({ logInfo,
/* ... */ }) or reuse the existing helper, call runCodexMultiAuthCli(deps,
args), assert it returns 0, that logInfo was called once, and parse the logged
JSON to assert accountCount/storagePath/accounts as the existing
runStatusCommand tests do—repeat for both aliases to cover the routing in
runCodexMultiAuthCli and ensure the json flag plumbing is tested.
---
Outside diff comments:
In `@lib/oc-chatgpt-orchestrator.ts`:
- Around line 183-205: persistMergedDefault currently does a single
fs.rename(tempPath, path) which fails on Windows under transient EBUSY/EPERM;
update persistMergedDefault to retry the final rename using the bounded backoff
retry pattern used in lib/config.ts (same max retries/delay strategy), catching
and retrying only transient errors (EBUSY, EPERM, EMFILE) and rethrowing others,
ensure the temp unlink in the finally block still runs (and does not swallow a
successful rename), and add a vitest regression that mocks fs.rename to fail the
first N attempts with EBUSY/EPERM then succeed to verify retries; reference
persistMergedDefault, tempPath, and the rename call in the test and ensure the
new test is cited in test outputs.
🪄 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: 13ffc456-e39a-4d00-ab79-0b66413b45e4
📒 Files selected for processing (13)
lib/codex-manager.tslib/codex-manager/commands/status.tslib/codex-manager/settings-hub/experimental.tslib/config.tslib/local-bridge.tslib/oc-chatgpt-orchestrator.tspackage.jsonscripts/verify-vendor-provenance.mjstest/codex-manager-status-command.test.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/plugin-config.test.ts
📜 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 (8)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errorsOAuth callback port must remain 1455; do not hardcode OAuth ports—use existing constants/helpers instead
Do not use bare recursive delete logic in Windows-sensitive code without retry handling
Files:
lib/codex-manager/settings-hub/experimental.tslib/codex-manager.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/config.tslib/local-bridge.ts
lib/codex-manager/settings-hub/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Settings writes use queued retry for
EBUSY/EPERM/EAGAIN
Files:
lib/codex-manager/settings-hub/experimental.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), targeting Node >= 18Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/codex-manager/settings-hub/experimental.tslib/codex-manager.tstest/codex-manager-status-command.test.tstest/plugin-config.test.tstest/oc-chatgpt-orchestrator.test.tslib/oc-chatgpt-orchestrator.tstest/package-bin.test.tslib/codex-manager/commands/status.tslib/config.tslib/local-bridge.tstest/local-bridge.test.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use the canonical package name
codex-multi-authand command familycodex-multi-auth ...consistently throughout code
Files:
lib/codex-manager/settings-hub/experimental.tslib/codex-manager.tstest/codex-manager-status-command.test.tstest/plugin-config.test.tstest/oc-chatgpt-orchestrator.test.tslib/oc-chatgpt-orchestrator.tstest/package-bin.test.tslib/codex-manager/commands/status.tslib/config.tslib/local-bridge.tstest/local-bridge.test.ts
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/settings-hub/experimental.tslib/codex-manager.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/config.tslib/local-bridge.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/codex-manager-status-command.test.tstest/plugin-config.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/local-bridge.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-status-command.test.tstest/plugin-config.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/local-bridge.test.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
Pin hono dependency to 4.12.18 or higher to avoid vulnerable versions 4.12.0-4.12.1 (GHSA-xh87-mx6m-69f3)
Pin rollup dependency to ^4.59.0 or higher to keep Vite and Vitest transitive dependencies above the vulnerable range
Files:
package.json
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:03.450Z
Learning: Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper; only local account-management commands run locally, non-auth commands forward to official Codex
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:03.450Z
Learning: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:03.450Z
Learning: Source lives in root `index.ts`, `lib/`, and `scripts/`; `dist/` is generated output and must not be edited directly
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Use PKCE-based OAuth flow for handling OAuth credentials
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Store OAuth credentials locally under ~/.codex/multi-auth directory (or CODEX_MULTI_AUTH_DIR environment variable)
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Implement refresh-token lifecycle management and account health isolation
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Ensure runtime rotation proxy is loopback-only and authenticated with a local client key by default
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Never commit auth files, logs, or cache artifacts to version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Enable ENABLE_PLUGIN_REQUEST_LOGGING and CODEX_PLUGIN_LOG_BODIES environment variables only for short-lived troubleshooting sessions, not in production
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T07:51:11.819Z
Learning: Run npm run audit:ci, npm run lint, npm run typecheck, npm test, and npm run build before release and after dependency changes
🔇 Additional comments (1)
test/oc-chatgpt-orchestrator.test.ts (1)
292-293: useremovewithretryin both finally blocks.
test/oc-chatgpt-orchestrator.test.ts:293andtest/oc-chatgpt-orchestrator.test.ts:332still use barerm(...)for teardown. that keeps the windowsebusy/eperm/enotemptyflake alive for tests touching real files. switch both cleanups toawait removeWithRetry(dir). as per coding guidelinestest/**/*.test.ts: useremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff.Also applies to: 331-332
Resolve all 34 review threads plus the greptile P2 from the audit-remediation PR, split into source fixes and the regression tests reviewers asked for. Full suite: 4104 -> 4165 tests, tsc + tsc:scripts + eslint clean. Source fixes: - logger: clearCorrelationId stores null (not "") inside an ALS scope so getCorrelationId honors its string|null contract - oc-chatgpt-orchestrator: create the merged-token parent dir 0o700 (matches the 0o600 file) so it is not world-listable - context-overflow: add random entropy to responseId (was Date.now() only) - prompts/fetch-utils: mandatory User-Agent/Accept now win over caller headers; drop leftover // PLACEHOLDER_READ_BODY artifact - prompts/codex: route writeCacheAtomically through withFileOperationRetry (windows EBUSY/EPERM/ENOTEMPTY) and document the two-rename window - runtime-rotation-proxy: maskString the logged error.message (matches lastError) - recovery/storage: only quarantine on real corruption; skip transient EBUSY/EPERM/EACCES/EAGAIN/ENOENT read races; quarantine rename now retries - debug-bundle: path-aware home redaction (case-fold on win32, require a path boundary so /users/alice2 != /users/alice) - local-bridge: reject runtimeClientApiKey when requireAuth is false - config: explain report mirrors loadPluginConfig env-path precedence; sync read retry aligned to 5 attempts - verify-vendor-provenance: fail closed on symlinks / unsupported dirent types - codex-manager: move ACCOUNT_MANAGER_COMMANDS to a shared internal module Regression tests added for every behavioral change, plus the coverage-only asks: token-bucket+breaker reset on removeAccount, storage-parser EBUSY retry, unpin/workspace/uninstall routing, non-loopback (0.0.0.0) proxy opt-in, oauth port fail-loud teardown, oc-orchestrator removeWithRetry cleanup, SECURITY.md rollup pin parity, status/list -j/--json CLI plumbing, quota secondary-window + boundary + invalid-entry cases, ui truncateAnsi reset placement, display-width explicit zero-width escapes, bidirectional config-explain parity, and dropped redundant vitest global imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
Rounds 5–6 — addressed all 8 CodeRabbit threadsPushed Code fixes (
|
| Sev | File | Change |
|---|---|---|
| MAJOR | lib/recovery/storage.ts |
readMessages/readParts now validate the minimal shape (string id; parts also need string type) before pushing, and route failures through handleUnreadableFile so a parseable-but-invalid record is quarantined instead of surviving to crash a later id-based sort/index (e.g. findMessagesWithOrphanThinking's a.id.localeCompare). |
| MAJOR | lib/prompts/codex.ts |
getLatestReleaseTag wrapped response.json() / htmlResponse.text() in a new withBodyTimeout (fetch-utils) so a mid-body stall can't hang the blocking path. withBodyTimeout adds only the hang guard (no size/empty checks) so the small release-metadata reads keep their semantics. |
| MINOR | lib/prompts/codex.ts |
writeCacheAtomically temp-file cleanup now routes fs.rm through withFileOperationRetry, so a transient Windows EBUSY/EPERM/ENOTEMPTY/EACCES no longer leaks the *.tmp sibling. |
| NIT | lib/codex-manager/commands/status.ts |
Extracted a single buildAccountMarkers() used by both the json and text paths (they previously duplicated the marker sequence and could drift). Empty-storage --json now emits the same keys (activeIndex/pinnedAccountIndex/recommendedIndex/recommendationReason/runtimeInUseIndex as null) as the populated branch for a stable shape. |
Coverage (6c864fc) for the test-gap findings
rotation.ts: directHealthScoreTracker.clearAccountKey+TokenBucketTracker.clearAccountKeytests — clears every quota-key variant for one identity, leaves other identities intact, and covers numeric→string key normalization.codex-managerstatus/list--json: added theauth list/auth statuswrapper form (-j/--json) to the plumbing matrix, and assertions for the new empty-storage--jsonshape.- New regressions for every code fix above: invalid-record quarantine (messages + parts), stalled release-body timeout, temp-file rm EBUSY retry,
withBodyTimeoutunit tests.
One item I did not change: a request to assert the exact getPlanBlockedReason wrapper wording in settings-hub/experimental.ts. That string is built inside a closure passed to promptExperimentalSettings and isn't independently exported; exporting an internal closure solely to assert wording is net-negative, and the structured plan-error path it wraps is already covered (test/oc-chatgpt-orchestrator.test.ts:150-206).
Verification: typecheck clean, lint clean, full suite 4192 passed / 1 skipped (273 files).
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/oc-chatgpt-orchestrator.ts (1)
183-208: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffverify temp-file collision risk and cleanup determinism.
lib/oc-chatgpt-orchestrator.ts:195 uses
date.now() + math.random().tostring(36).slice(2, 8)for temp-file uniqueness. the 6-char random suffix (base36) provides ~2 billion combinations, but if two calls run in the same millisecond (plausible in concurrent scenarios), collision risk exists. consider adding process.pid to the temp name for stronger uniqueness guarantees.the finally block at lines 201-206 attempts best-effort temp cleanup but ignores all errors. on windows, if the temp file is still open or locked when unlink runs (race with antivirus, indexing, etc.), it will leak. the comment acknowledges this ("best-effort"), but if you want deterministic cleanup, wrap the unlink in a retry helper (like removeWithRetry used in tests).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/oc-chatgpt-orchestrator.ts` around lines 183 - 208, persistMergedDefault currently builds tempPath using Date.now() and a short Math.random() suffix which can collide under concurrent calls and uses a best-effort fs.unlink in the finally block that swallows all errors; update the temp filename generation (the tempPath creation in persistMergedDefault) to include process.pid and/or a stronger entropy source (e.g., crypto.randomBytes) to avoid same-millisecond collisions, and replace the single fs.unlink in the finally block with a deterministic cleanup using the existing removeWithRetry (or implement a small retry wrapper) to retry unlink on transient Windows/IO errors so temp files are reliably removed.lib/logger.ts (1)
243-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winstrip cr/lf from console string payloads too.
lib/logger.ts:243-258only sanitizes the message text. if a caller passes stringdata,console.warn/error/log(message, data)still emits raw newlines and can forge extra log lines whenCODEX_CONSOLE_LOG=1. please sanitize string payloads before writing them, and add a regression intest/logger.test.tsforlogError("msg", "line1\nline2").possible fix
const sanitizedMessage = maskString(message).replace(/[\r\n]+/g, " "); const sanitizedData = data === undefined ? undefined : sanitizeValue(data); + const consoleData = + typeof sanitizedData === "string" + ? sanitizedData.replace(/[\r\n]+/g, " ") + : sanitizedData; // This is the single sanctioned console sink for the whole package: every // message is mask-sanitized above before it reaches the terminal. The // no-console lint rule (request-10) intentionally points all other lib code // here, so the direct console calls below are allowed. /* eslint-disable no-console */ - if (sanitizedData !== undefined) { - if (level === "warn") console.warn(sanitizedMessage, sanitizedData); - else if (level === "error") console.error(sanitizedMessage, sanitizedData); - else console.log(sanitizedMessage, sanitizedData); + if (consoleData !== undefined) { + if (level === "warn") console.warn(sanitizedMessage, consoleData); + else if (level === "error") console.error(sanitizedMessage, consoleData); + else console.log(sanitizedMessage, consoleData); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/logger.ts` around lines 243 - 258, The console payloads are not having CR/LF stripped, so update the logging path that computes sanitizedData (the variable using sanitizeValue(data)) to also remove carriage returns/newlines from string payloads before calling console.log/warn/error (e.g. after sanitizeValue returns a string, run a .replace(/\r?\n/g, ' ') or equivalent), ensuring the console sink in lib/logger.ts always receives a newline-free string payload, and add a regression test in test/logger.test.ts that calls logError("msg", "line1\nline2") and asserts the emitted console output does not contain a line break.test/codex-prompts.test.ts (1)
1-1: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuedrop explicit vitest globals import.
test/codex-prompts.test.ts:1importsdescribe,it,expectetc. from vitest, butvitest.config.tssetsglobals: true. keep onlyvi(needed for mocking) and rely on globals for the rest. other tests in this pr already follow that pattern (e.g.,test/global-sandbox.test.ts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-prompts.test.ts` at line 1, The test file imports vitest globals explicitly; since vitest.config.ts enables globals: true you should remove exported globals from the import and only keep the mocking utility vi: update the import statement in test/codex-prompts.test.ts to stop importing describe, it, expect, beforeEach, afterEach and retain vi only so the test uses the global describe/it/expect hooks while still using vi for mocks.
♻️ Duplicate comments (2)
lib/codex-manager/settings-hub/experimental.ts (1)
106-114: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winadd assertions for the exact plan-error message formatting.
the past review (lines 106-114) correctly flagged that test/oc-chatgpt-orchestrator.test.ts checks
kind === "plan-error"andcausebut doesn't lock in the wrapper string from lib/codex-manager/settings-hub/experimental.ts:113 ("sync failed while loading the target" vs "sync failed while previewing the merge"). also, there's no coverage for the non-error → string(...) detail fallback at line 112.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.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/codex-manager/settings-hub/experimental.ts` around lines 106 - 114, Update test/oc-chatgpt-orchestrator.test.ts to assert the exact formatted message returned by the code path that handles candidate.kind === "plan-error" in lib/codex-manager/settings-hub/experimental.ts: verify both branches of candidate.cause produce the exact wrapper strings ("Sync failed while loading the target: <detail>" and "Sync failed while previewing the merge: <detail>") and add a test where candidate.error is NOT an Error (e.g., a string or undefined) to cover the String(candidate.error ?? "unknown error") fallback; reference the candidate.kind/candidate.cause/candidate.error symbols in the assertions and mark the test file(s) affected in the commit message so vitest coverage includes these new assertions.test/context-overflow.test.ts (1)
94-107: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winadd the concurrent id-collision regression here.
lib/context-overflow.ts:61-66now randomizes both synthetic ids, buttest/context-overflow.test.ts:94-107still only proves parser compatibility. please add a deterministic vitest case that creates many overflow responses and asserts uniquemessageIdandresponseId, otherwise the original concurrency bug can regress silently.As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/context-overflow.test.ts` around lines 94 - 107, Add a deterministic vitest regression that repeatedly builds many overflow responses and asserts unique messageId and responseId to prevent silent id-collision regressions: in the test file add a new test that stubs the randomness used by createContextOverflowResponse (e.g., mock Math.random or crypto.randomUUID) to a deterministic sequence, loop N times calling createContextOverflowResponse("gpt-5.1-codex"), feed each response through convertSseToJson to parse the SSE payload, extract the messageId and responseId from the parsed JSON, collect them into sets and assert their sizes equal N (no duplicates); reference createContextOverflowResponse, convertSseToJson, and the messageId/responseId fields when locating where to parse and assert uniqueness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/codex-manager/commands/debug-bundle.ts`:
- Around line 27-43: The Windows home-prefix check should normalize path
separators before comparing: update the logic around
isWindows/normalizedValue/normalizedHome to first replace all backslashes with
forward slashes (or vice versa using sep) on both value and home, then lowercase
for Windows, then perform startsWith and boundary checks using the normalized
separators; ensure the boundary is computed from the normalizedValue after
normalization and that the returned redaction still slices from the original
home length (home) to preserve original separators. Modify the block using
isWindows, normalizedValue, normalizedHome, sep to perform separator
normalization, and add a Vitest regression that passes a mixed-separator Windows
path like "C:/Users/Alice\\some\\path" to verify redaction; include the new test
in the existing test suite and mark it as covering mixed-separator Windows
paths.
In `@lib/policy/runtime-policy.ts`:
- Around line 119-126: Add an integration test that verifies
evaluateRuntimePolicy() counts archived usage when a budget window spans a
ledger rotation: create a test that uses the existing ledger rotation helpers
(similar to test/usage-ledger.test.ts rotation setup) to insert a
"before-rotate" row, rotate the ledger, insert an "after-rotate" row, then call
evaluateRuntimePolicy() with a budget whose window (use getBudgetWindowStart and
now) spans the rotation; assert the decision reflects the sum from
summarizeUsageLedger({ includeArchives: true }) (i.e., the archived row is
included so the budget is exhausted/affected), reusing the ebusy/stale-lock
rotation sequence if helpful.
In `@lib/storage/paths.ts`:
- Around line 403-432: Document the hardcoded 4096 iteration limit and
performance tradeoffs: update the doc comment for canonicalizeExistingPrefix
(and add a brief note in resolvePath where canonicalizeExistingPrefix is
invoked) to explain that 4096 is a conservative guard against
pathological/recursive inputs (references: canonicalizeExistingPrefix) and to
call out platform differences (Linux PATH_MAX vs Windows MAX_PATH/long paths)
and that existsSync/realpathSync will incur many syscalls on deep paths or on
Windows/network drives/AV scanning; state that correctness/security is
prioritized over perf and recommend callers avoid extremely deep paths or cache
results if needed.
In `@test/paths.test.ts`:
- Around line 801-876: Add a Windows drive-letter case-normalization regression
test for the symlink validation: mock mockedExistsSync to report the symlink
path (e.g. String(p) === the lower-case drive-letter link path), mock
mockedRealpathSync to return the same path but with an upper-case drive letter
and different case in the Users segment (e.g. "C:\\Users\\...") and then call
resolvePath(linkPath) asserting it does not throw; ensure the test uses
homedir()/tmpdir() or explicit Windows-style paths and references
mockedExistsSync, mockedRealpathSync, and resolvePath so the canonical-vs-raw
containment check is exercised for case-insensitive Windows drive-letter
normalization.
---
Outside diff comments:
In `@lib/logger.ts`:
- Around line 243-258: The console payloads are not having CR/LF stripped, so
update the logging path that computes sanitizedData (the variable using
sanitizeValue(data)) to also remove carriage returns/newlines from string
payloads before calling console.log/warn/error (e.g. after sanitizeValue returns
a string, run a .replace(/\r?\n/g, ' ') or equivalent), ensuring the console
sink in lib/logger.ts always receives a newline-free string payload, and add a
regression test in test/logger.test.ts that calls logError("msg",
"line1\nline2") and asserts the emitted console output does not contain a line
break.
In `@lib/oc-chatgpt-orchestrator.ts`:
- Around line 183-208: persistMergedDefault currently builds tempPath using
Date.now() and a short Math.random() suffix which can collide under concurrent
calls and uses a best-effort fs.unlink in the finally block that swallows all
errors; update the temp filename generation (the tempPath creation in
persistMergedDefault) to include process.pid and/or a stronger entropy source
(e.g., crypto.randomBytes) to avoid same-millisecond collisions, and replace the
single fs.unlink in the finally block with a deterministic cleanup using the
existing removeWithRetry (or implement a small retry wrapper) to retry unlink on
transient Windows/IO errors so temp files are reliably removed.
In `@test/codex-prompts.test.ts`:
- Line 1: The test file imports vitest globals explicitly; since
vitest.config.ts enables globals: true you should remove exported globals from
the import and only keep the mocking utility vi: update the import statement in
test/codex-prompts.test.ts to stop importing describe, it, expect, beforeEach,
afterEach and retain vi only so the test uses the global describe/it/expect
hooks while still using vi for mocks.
---
Duplicate comments:
In `@lib/codex-manager/settings-hub/experimental.ts`:
- Around line 106-114: Update test/oc-chatgpt-orchestrator.test.ts to assert the
exact formatted message returned by the code path that handles candidate.kind
=== "plan-error" in lib/codex-manager/settings-hub/experimental.ts: verify both
branches of candidate.cause produce the exact wrapper strings ("Sync failed
while loading the target: <detail>" and "Sync failed while previewing the merge:
<detail>") and add a test where candidate.error is NOT an Error (e.g., a string
or undefined) to cover the String(candidate.error ?? "unknown error") fallback;
reference the candidate.kind/candidate.cause/candidate.error symbols in the
assertions and mark the test file(s) affected in the commit message so vitest
coverage includes these new assertions.
In `@test/context-overflow.test.ts`:
- Around line 94-107: Add a deterministic vitest regression that repeatedly
builds many overflow responses and asserts unique messageId and responseId to
prevent silent id-collision regressions: in the test file add a new test that
stubs the randomness used by createContextOverflowResponse (e.g., mock
Math.random or crypto.randomUUID) to a deterministic sequence, loop N times
calling createContextOverflowResponse("gpt-5.1-codex"), feed each response
through convertSseToJson to parse the SSE payload, extract the messageId and
responseId from the parsed JSON, collect them into sets and assert their sizes
equal N (no duplicates); reference createContextOverflowResponse,
convertSseToJson, and the messageId/responseId fields when locating where to
parse and assert uniqueness.
🪄 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: eff0da9c-f5f8-4a31-8716-f70e6f4f6ac0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (81)
.github/workflows/pr-ci.ymlAGENTS.mdSECURITY.mdeslint.config.jslib/accounts.tslib/circuit-breaker.tslib/codex-manager.tslib/codex-manager/account-manager-commands.tslib/codex-manager/commands/debug-bundle.tslib/codex-manager/commands/status.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/settings-hub/experimental.tslib/config.tslib/context-overflow.tslib/local-bridge.tslib/logger.tslib/oc-chatgpt-orchestrator.tslib/policy/runtime-policy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/prompts/host-codex-prompt.tslib/quota-readiness.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/request/fetch-helpers.tslib/rotation.tslib/runtime-rotation-proxy.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/storage/paths.tslib/storage/storage-parser.tslib/table-formatter.tslib/types.tslib/ui/auth-menu.tslib/ui/display-width.tslib/ui/select.tslib/ui/theme.tspackage.jsonscripts/codex-routing.jsscripts/verify-vendor-provenance.mjstest/account-clear.test.tstest/accounts.test.tstest/ci-workflows.test.tstest/codex-manager-cli.test.tstest/codex-manager-status-command.test.tstest/codex-prompts.test.tstest/codex-routing.test.tstest/config-explain.test.tstest/context-overflow.test.tstest/debug-bundle-redact.test.tstest/display-width.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/global-sandbox.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/local-bridge.test.tstest/logger.test.tstest/oauth-server.integration.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/paths.test.tstest/plugin-config.test.tstest/prompt-fetch-utils.test.tstest/property/setup.test.tstest/property/setup.tstest/quota-readiness.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/rotation.test.tstest/runtime-policy.test.tstest/runtime-rotation-proxy.test.tstest/select.test.tstest/settings-hub-utils.test.tstest/storage-parser.test.tstest/table-formatter.test.tstest/ui-format.test.tstest/ui-theme.test.tsvitest.config.ts
📜 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 (23)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errorsDo not key project storage by worktree path; use
resolveProjectStorageIdentityRoot()helper for repo identity resolution instead
Files:
lib/storage/account-clear.tslib/storage/import-export.tslib/storage/storage-parser.tslib/accounts.tslib/types.tslib/ui/auth-menu.tslib/circuit-breaker.tslib/storage/paths.tslib/storage/flagged-storage-io.tslib/request/fetch-helpers.tslib/codex-manager/settings-hub/experimental.tslib/table-formatter.tslib/refresh-lease.tslib/rotation.tslib/policy/runtime-policy.tslib/quota-readiness.tslib/codex-manager/account-manager-commands.tslib/oc-chatgpt-orchestrator.tslib/context-overflow.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/commands/debug-bundle.tslib/prompts/host-codex-prompt.tslib/ui/select.tslib/local-bridge.tslib/ui/display-width.tslib/prompts/fetch-utils.tslib/ui/theme.tslib/codex-manager.tslib/prompts/codex.tslib/config.tslib/refresh-queue.tslib/recovery/storage.tslib/codex-manager/commands/status.tslib/logger.tslib/runtime-rotation-proxy.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handling
Files:
lib/storage/account-clear.tslib/storage/import-export.tslib/storage/storage-parser.tslib/storage/paths.tslib/storage/flagged-storage-io.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions; maintain strict TypeScript typing throughout
Files:
lib/storage/account-clear.tstest/display-width.test.tstest/table-formatter.test.tslib/storage/import-export.tslib/storage/storage-parser.tstest/runtime-policy.test.tstest/ui-format.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/settings-hub-utils.test.tslib/accounts.tstest/helpers/global-sandbox.tslib/types.tslib/ui/auth-menu.tsvitest.config.tslib/circuit-breaker.tslib/storage/paths.tslib/storage/flagged-storage-io.tslib/request/fetch-helpers.tslib/codex-manager/settings-hub/experimental.tstest/package-bin.test.tstest/accounts.test.tstest/property/setup.test.tslib/table-formatter.tslib/refresh-lease.tstest/rotation.test.tslib/rotation.tslib/policy/runtime-policy.tstest/account-clear.test.tstest/runtime-rotation-proxy.test.tslib/quota-readiness.tstest/fetch-helpers.test.tstest/documentation.test.tstest/codex-manager-status-command.test.tstest/debug-bundle-redact.test.tstest/property/setup.tstest/storage-parser.test.tstest/codex-routing.test.tstest/oauth-server.integration.test.tslib/codex-manager/account-manager-commands.tslib/oc-chatgpt-orchestrator.tslib/context-overflow.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/commands/debug-bundle.tstest/paths.test.tslib/prompts/host-codex-prompt.tstest/select.test.tslib/ui/select.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/logger.test.tstest/prompt-fetch-utils.test.tstest/local-bridge.test.tslib/local-bridge.tstest/codex-manager-cli.test.tstest/oc-chatgpt-orchestrator.test.tstest/config-explain.test.tslib/ui/display-width.tslib/prompts/fetch-utils.tstest/quota-readiness.test.tslib/ui/theme.tslib/codex-manager.tstest/plugin-config.test.tslib/prompts/codex.tstest/recovery-storage.test.tstest/codex-prompts.test.tslib/config.tslib/refresh-queue.tslib/recovery/storage.tstest/ui-theme.test.tslib/codex-manager/commands/status.tslib/logger.tstest/refresh-lease.test.tslib/runtime-rotation-proxy.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM-only syntax (
import/export); the project targets Node >= 18 with"type": "module"in package.json
Files:
lib/storage/account-clear.tstest/display-width.test.tstest/table-formatter.test.tslib/storage/import-export.tslib/storage/storage-parser.tstest/runtime-policy.test.tsscripts/codex-routing.jstest/ui-format.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/settings-hub-utils.test.tslib/accounts.tstest/helpers/global-sandbox.tslib/types.tslib/ui/auth-menu.tsvitest.config.tslib/circuit-breaker.tslib/storage/paths.tslib/storage/flagged-storage-io.tslib/request/fetch-helpers.tslib/codex-manager/settings-hub/experimental.tstest/package-bin.test.tstest/accounts.test.tstest/property/setup.test.tslib/table-formatter.tslib/refresh-lease.tstest/rotation.test.tslib/rotation.tslib/policy/runtime-policy.tstest/account-clear.test.tstest/runtime-rotation-proxy.test.tslib/quota-readiness.tstest/fetch-helpers.test.tstest/documentation.test.tstest/codex-manager-status-command.test.tstest/debug-bundle-redact.test.tstest/property/setup.tstest/storage-parser.test.tstest/codex-routing.test.tstest/oauth-server.integration.test.tslib/codex-manager/account-manager-commands.tslib/oc-chatgpt-orchestrator.tslib/context-overflow.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/commands/debug-bundle.tseslint.config.jstest/paths.test.tslib/prompts/host-codex-prompt.tstest/select.test.tslib/ui/select.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/logger.test.tstest/prompt-fetch-utils.test.tstest/local-bridge.test.tslib/local-bridge.tstest/codex-manager-cli.test.tstest/oc-chatgpt-orchestrator.test.tstest/config-explain.test.tslib/ui/display-width.tslib/prompts/fetch-utils.tstest/quota-readiness.test.tslib/ui/theme.tslib/codex-manager.tstest/plugin-config.test.tslib/prompts/codex.tstest/recovery-storage.test.tstest/codex-prompts.test.tslib/config.tslib/refresh-queue.tslib/recovery/storage.tstest/ui-theme.test.tslib/codex-manager/commands/status.tslib/logger.tstest/refresh-lease.test.tslib/runtime-rotation-proxy.ts
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/storage/account-clear.tslib/storage/import-export.tslib/storage/storage-parser.tslib/accounts.tslib/types.tslib/ui/auth-menu.tslib/circuit-breaker.tslib/storage/paths.tslib/storage/flagged-storage-io.tslib/request/fetch-helpers.tslib/codex-manager/settings-hub/experimental.tslib/table-formatter.tslib/refresh-lease.tslib/rotation.tslib/policy/runtime-policy.tslib/quota-readiness.tslib/codex-manager/account-manager-commands.tslib/oc-chatgpt-orchestrator.tslib/context-overflow.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/commands/debug-bundle.tslib/prompts/host-codex-prompt.tslib/ui/select.tslib/local-bridge.tslib/ui/display-width.tslib/prompts/fetch-utils.tslib/ui/theme.tslib/codex-manager.tslib/prompts/codex.tslib/config.tslib/refresh-queue.tslib/recovery/storage.tslib/codex-manager/commands/status.tslib/logger.tslib/runtime-rotation-proxy.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/display-width.test.tstest/table-formatter.test.tstest/runtime-policy.test.tstest/ui-format.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/settings-hub-utils.test.tstest/package-bin.test.tstest/accounts.test.tstest/property/setup.test.tstest/rotation.test.tstest/account-clear.test.tstest/runtime-rotation-proxy.test.tstest/fetch-helpers.test.tstest/documentation.test.tstest/codex-manager-status-command.test.tstest/debug-bundle-redact.test.tstest/storage-parser.test.tstest/codex-routing.test.tstest/oauth-server.integration.test.tstest/paths.test.tstest/select.test.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/logger.test.tstest/prompt-fetch-utils.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/oc-chatgpt-orchestrator.test.tstest/config-explain.test.tstest/quota-readiness.test.tstest/plugin-config.test.tstest/recovery-storage.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/refresh-lease.test.ts
test/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Windows filesystem safety: include retry handling for transient
EBUSY,EPERM, andENOTEMPTYerrors in cleanup and write operations
Files:
test/display-width.test.tstest/table-formatter.test.tstest/runtime-policy.test.tstest/ui-format.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/settings-hub-utils.test.tstest/helpers/global-sandbox.tstest/package-bin.test.tstest/accounts.test.tstest/property/setup.test.tstest/rotation.test.tstest/account-clear.test.tstest/runtime-rotation-proxy.test.tstest/fetch-helpers.test.tstest/documentation.test.tstest/codex-manager-status-command.test.tstest/debug-bundle-redact.test.tstest/property/setup.tstest/storage-parser.test.tstest/codex-routing.test.tstest/oauth-server.integration.test.tstest/paths.test.tstest/select.test.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/logger.test.tstest/prompt-fetch-utils.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/oc-chatgpt-orchestrator.test.tstest/config-explain.test.tstest/quota-readiness.test.tstest/plugin-config.test.tstest/recovery-storage.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/refresh-lease.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/display-width.test.tstest/table-formatter.test.tstest/runtime-policy.test.tstest/ui-format.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/settings-hub-utils.test.tstest/helpers/global-sandbox.tstest/package-bin.test.tstest/accounts.test.tstest/property/setup.test.tstest/rotation.test.tstest/account-clear.test.tstest/runtime-rotation-proxy.test.tstest/fetch-helpers.test.tstest/documentation.test.tstest/codex-manager-status-command.test.tstest/debug-bundle-redact.test.tstest/property/setup.tstest/storage-parser.test.tstest/codex-routing.test.tstest/oauth-server.integration.test.tstest/paths.test.tstest/select.test.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/logger.test.tstest/prompt-fetch-utils.test.tstest/local-bridge.test.tstest/codex-manager-cli.test.tstest/oc-chatgpt-orchestrator.test.tstest/config-explain.test.tstest/quota-readiness.test.tstest/plugin-config.test.tstest/recovery-storage.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/refresh-lease.test.ts
scripts/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode OAuth ports; use existing constants and helpers for port configuration instead of inline port numbers
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper; forward non-auth commands to official Codex via
codex-multi-auth-codexDo not patch official Codex app binaries; use app bind helpers (
lib/runtime/app-bind.ts,scripts/codex-app-launcher.js) or launcher routing for user-level app integrationDo not use bare recursive delete logic without retry handling for transient Windows filesystem errors (
EBUSY,EPERM,ENOTEMPTY); use retry helpers as covered in tests
Files:
scripts/codex-routing.js
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
**/*{auth,oauth,pkce}*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Use PKCE-based OAuth flow for authentication in the codex-multi-auth security model
Files:
lib/ui/auth-menu.tstest/oauth-server.integration.test.ts
**/*{auth,token,session,refresh}*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Implement refresh-token lifecycle management and account health isolation
Files:
lib/ui/auth-menu.tslib/refresh-lease.tstest/oauth-server.integration.test.tslib/refresh-queue.tstest/refresh-lease.test.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
lib/codex-manager/settings-hub/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Settings writes use queued retry for
EBUSY/EPERM/EAGAIN
Files:
lib/codex-manager/settings-hub/experimental.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
test/**/rotation*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test Windows cleanup behavior and account selection logic in rotation tests
Files:
test/rotation.test.ts
**/*{proxy,rotation,server}*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Keep runtime rotation proxy loopback-only, enabled by default, and authenticate with a local client key; allow users to opt out with CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0
Files:
test/rotation.test.tslib/rotation.tstest/runtime-rotation-proxy.test.tstest/oauth-server.integration.test.tslib/runtime-rotation-proxy.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
**/*{log,debug,trace,plugin}*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Enable debug/body logging (ENABLE_PLUGIN_REQUEST_LOGGING, CODEX_PLUGIN_LOG_BODIES) only for short-lived troubleshooting sessions, not in production
Files:
test/debug-bundle-redact.test.tslib/codex-manager/commands/debug-bundle.tstest/logger.test.tstest/plugin-config.test.tslib/logger.ts
test/**/oauth-server.integration.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Bind OAuth server integration tests to port 1455; do not hardcode other ports
Files:
test/oauth-server.integration.test.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
Pin hono to version 4.12.18 or higher to avoid vulnerable 4.12.0-4.12.1 range (GHSA-xh87-mx6m-69f3 authentication bypass)
Pin rollup to ^4.59.0 or higher to avoid vulnerable versions below 4.59.0 in the Vite and Vitest transitive dependency graph
Files:
package.json
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: Source code lives in root `index.ts`, `lib/`, and `scripts/`; maintain ESM-only module format (`import`/`export`) with Node >= 18
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: The canonical package name is `codex-multi-auth` and canonical command family is `codex-multi-auth ...`; the package does not publish a global `codex` bin
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: Runtime rotation is default-on through `codexRuntimeRotationProxy` configuration; users can opt out with `codex-multi-auth rotation disable` or `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: The runtime rotation proxy is loopback-only, uses per-process client tokens, and forwards only Responses API and model discovery requests
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: The persistent desktop app bind is reversible and edits user config/startup metadata, not official app binaries
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: OAuth callback port remains 1455; do not change this value without coordinating across auth flow, callback server, and wrapper routing components
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: Keep runtime rotation default-on behavior aligned with explicit release and migration documentation to ensure user expectation consistency
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:17.184Z
Learning: Email dedup is case-insensitive via `normalizeEmailKey()` helper (trim + lowercase); use this function for all email comparisons
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Store credentials locally under ~/.codex/multi-auth (or CODEX_MULTI_AUTH_DIR environment variable), never in version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Never commit auth files, logs, or cache artifacts to version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Do not open public issues for security vulnerabilities; contact maintainer privately via GitHub profile contact channel instead
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Run npm run audit:ci, npm run lint, npm run typecheck, npm test, and npm run build before release and after dependency changes
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Review connected apps in ChatGPT settings periodically for account security
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T19:44:25.807Z
Learning: Do not share ~/.codex/ directories to prevent unauthorized access to credentials and metadata
🔇 Additional comments (117)
.github/workflows/pr-ci.yml (1)
47-50: LGTM!AGENTS.md (1)
6-6: LGTM!lib/codex-manager/experimental-settings-prompt.ts (2)
95-104: LGTM!
160-178: LGTM!lib/oc-chatgpt-orchestrator.ts (2)
41-55: LGTM!
120-142: LGTM!lib/ui/auth-menu.ts (1)
284-310: LGTM!test/oc-chatgpt-orchestrator.test.ts (2)
148-206: LGTM!
257-335: LGTM!SECURITY.md (1)
82-82: LGTM!eslint.config.js (1)
35-40: LGTM!Also applies to: 47-72
lib/accounts.ts (1)
43-43: LGTM!Also applies to: 1462-1473
lib/circuit-breaker.ts (1)
198-205: LGTM!lib/rotation.ts (1)
182-197: LGTM!Also applies to: 354-369
test/accounts.test.ts (1)
19-19: LGTM!Also applies to: 1047-1114
test/rotation.test.ts (1)
168-198: LGTM!Also applies to: 338-364
lib/codex-manager.ts (1)
40-40: LGTM!Also applies to: 3547-3548
lib/codex-manager/account-manager-commands.ts (1)
1-42: LGTM!lib/codex-manager/commands/status.ts (1)
69-99: LGTM!Also applies to: 139-160, 216-264, 324-333
lib/quota-readiness.ts (1)
6-6: LGTM!Also applies to: 77-114
lib/ui/select.ts (1)
66-70: LGTM!Also applies to: 95-99, 419-443
test/codex-manager-cli.test.ts (1)
58-70: LGTM!Also applies to: 1237-1250, 10314-10317, 10559-10563
test/codex-manager-status-command.test.ts (2)
352-392: LGTM!
401-445: make the--jsonpath keepconsole.logexactly-once (or adjust the assertion to be resilient)
list/statusdispatch routes straight torunStatusCommand(...)inlib/codex-manager.ts:3531-3536, and theconsole.log(...)matches surfaced inlib/codex-managerare in interactive/settings flows (lib/codex-manager/settings-hub/dashboard.ts:217,257,lib/codex-manager/settings-hub/backend.ts:109) and repair flows (lib/codex-manager/repair-commands.ts:132-152), not in this dispatch block.- since these tests spy on global
console.logand requiretoHaveBeenCalledTimes(1)intest/codex-manager-status-command.test.ts:401-445, any runtime rotation/startup/deprecation logging emitted before the json branch will still make the test flaky and order-dependent; ensurerunCodexMultiAuthCli(... --json/-j)fully short-circuits incidental logs, or loosen the check to assert that the emitted line is machine-readable json rather than enforcing an exact call count.lib/context-overflow.ts (1)
52-120: LGTM!lib/logger.ts (1)
103-112: LGTM!Also applies to: 150-191, 208-208, 352-352, 487-487
lib/recovery/storage.ts (1)
23-147: LGTM!Also applies to: 339-371, 388-404, 452-455, 556-624, 698-699
lib/request/fetch-helpers.ts (1)
931-951: LGTM!Also applies to: 1001-1010
test/context-overflow.test.ts (1)
76-92: LGTM!test/fetch-helpers.test.ts (1)
998-1023: LGTM!test/logger.test.ts (1)
11-11: LGTM!Also applies to: 174-218, 543-562
lib/config.ts (4)
47-75: LGTM!
269-284: LGTM!
568-590: LGTM!
1971-1988: LGTM!test/account-clear.test.ts (1)
44-50: LGTM!test/ci-workflows.test.ts (1)
49-54: LGTM!test/config-explain.test.ts (3)
206-218: LGTM!
220-231: LGTM!
233-259: LGTM!test/debug-bundle-redact.test.ts (4)
33-59: LGTM!
61-82: LGTM!
84-88: LGTM!
90-138: LGTM!test/documentation.test.ts (1)
588-607: LGTM!test/helpers/global-sandbox.ts (1)
1-31: LGTM!test/package-bin.test.ts (2)
21-47: LGTM!
49-61: LGTM!lib/local-bridge.ts (1)
22-29: LGTM!Also applies to: 47-58, 71-87, 166-197, 238-238
lib/runtime-rotation-proxy.ts (1)
1-1: LGTM!Also applies to: 23-24, 49-49, 79-83, 127-142, 468-473, 987-1002, 1065-1068, 1291-1308, 1325-1325, 1380-1380, 1400-1411, 1481-1481, 1568-1568, 2044-2057, 2131-2137
lib/table-formatter.ts (1)
6-7: LGTM!Also applies to: 27-41
lib/ui/display-width.ts (1)
1-81: LGTM!package.json (1)
113-113: LGTM!Also applies to: 141-143, 175-175
scripts/verify-vendor-provenance.mjs (1)
3-3: LGTM!Also applies to: 13-47, 85-112
test/display-width.test.ts (1)
1-53: LGTM!test/local-bridge.test.ts (1)
47-62: LGTM!Also applies to: 149-242
lib/policy/runtime-policy.ts (1)
200-217: LGTM!test/property/setup.test.ts (1)
6-22: LGTM!test/quota-readiness.test.ts (1)
59-167: LGTM!test/runtime-policy.test.ts (1)
84-111: LGTM!test/runtime-rotation-proxy.test.ts (3)
327-363: LGTM!
367-396: LGTM!
434-457: LGTM!test/table-formatter.test.ts (1)
65-79: LGTM!test/ui-theme.test.ts (1)
5-86: LGTM!lib/prompts/codex.ts (5)
1-65: LGTM!
168-228: LGTM!
240-350: LGTM!
352-423: LGTM!
425-476: LGTM!lib/prompts/fetch-utils.ts (3)
1-48: LGTM!
49-97: LGTM!
99-176: LGTM!lib/prompts/host-codex-prompt.ts (2)
15-23: LGTM!
279-331: LGTM!lib/types.ts (1)
236-243: LGTM!test/codex-prompts.test.ts (2)
162-263: LGTM!
321-419: LGTM!test/global-sandbox.test.ts (1)
1-30: LGTM!test/prompt-fetch-utils.test.ts (2)
1-98: LGTM!
100-165: LGTM!test/property/setup.ts (1)
1-17: LGTM!scripts/codex-routing.js (1)
6-8: verify test coverage for new auth subcommands.the past review comment flagged missing regression tests for
unpin,workspace, anduninstall. marked as addressed in commit 6e04749, but need to confirmtest/codex-routing.test.ts:24-31actually covers all three commands per the duplicate-comment tag.Also applies to: 18-18
lib/refresh-lease.ts (2)
202-218: LGTM!
328-333: LGTM!lib/refresh-queue.ts (2)
20-31: LGTM!
120-120: LGTM!Also applies to: 133-134, 158-158, 170-170, 182-182, 198-198, 208-208, 256-257, 269-269, 293-293, 298-298, 309-309, 320-320, 348-348, 357-357
test/codex-routing.test.ts (2)
24-31: LGTM!
33-48: LGTM!test/refresh-lease.test.ts (2)
51-89: LGTM!Also applies to: 91-123
565-595: LGTM!Also applies to: 600-667
test/settings-hub-utils.test.ts (1)
773-775: LGTM!Also applies to: 790-791, 805-806
vitest.config.ts (1)
34-60: LGTM!lib/storage/account-clear.ts (1)
2-8: LGTM!lib/storage/flagged-storage-io.ts (1)
6-10: LGTM!lib/storage/import-export.ts (1)
4-4: LGTM!Also applies to: 20-23
lib/storage/paths.ts (1)
471-508: LGTM!test/paths.test.ts (1)
1-57: LGTM!Also applies to: 774-877
lib/storage/storage-parser.ts (1)
7-7: LGTM!Also applies to: 55-60
test/storage-parser.test.ts (1)
2-2: LGTM!Also applies to: 13-15, 62-98
lib/ui/theme.ts (3)
199-218: LGTM!
220-227: LGTM!
257-265: LGTM!test/host-codex-prompt.test.ts (2)
157-164: LGTM!
189-194: LGTM!test/oauth-server.integration.test.ts (3)
9-9: LGTM!
21-45: LGTM!
50-54: LGTM!test/plugin-config.test.ts (3)
256-277: LGTM!
279-291: LGTM!
296-318: LGTM!test/recovery-storage.test.ts (4)
157-165: LGTM!Also applies to: 168-203, 205-241, 243-274, 290-320
364-405: LGTM!
915-935: LGTM!
938-943: LGTM!test/select.test.ts (1)
132-171: LGTM!test/ui-format.test.ts (1)
20-22: LGTM!
Closes the residual items flagged but previously deferred as out-of-PR-scope. prompts/fetch-utils.ts — withBodyTimeout now actually cancels the read Previously it only rejected on timeout while the stalled body kept consuming the connection until GC/socket close. It now takes the Response and calls response.body.cancel() on timeout so the stream is torn down. For mocks / fetch impls without a streamable body the cancel is a no-op and the timeout still rejects. getLatestReleaseTag updated to pass the response. ui/display-width.ts — grapheme-cluster aware width codePointWidth summed per code point, so ZWJ emoji sequences (👨👩👧) were overcounted (6 instead of 2) and combining marks outside U+0300–036F (Arabic, Hebrew, Thai, Cyrillic, Syriac, etc.) counted as width 1. Added those zero-width ranges and a cluster walker that collapses ZWJ-joined emoji, emoji+skin-tone modifiers, and regional-indicator flag pairs to a single 2-wide glyph. ZWJ between non-emoji is still treated as a zero-width control (so the existing ab → width 2 contract holds). truncateToWidth never splits a cluster. ui/select.ts — truncateAnsi measures display columns It budgeted by UTF-16 code units (.length), so a CJK/emoji menu label could overflow `columns` and wrap to an extra physical row, desyncing the render's up-cursor line accounting and progressively corrupting the redraw. It now measures with displayWidth and advances per code point. logger.ts — drop the event-loop-blocking Atomics.wait sleep ensureLogDir slept via Atomics.wait on EBUSY/EPERM, freezing ALL in-flight requests on the concurrent proxy for up to ~30ms. Replaced with immediate (non-blocking) retries and a cached "dir ready" flag so the steady state does a single existsSync and a transient lock no longer stalls the event loop. oc-chatgpt-orchestrator.ts — rename retry + POSIX perm re-assert persistMergedDefault's atomic rename now routes through withFileOperationRetry (the destination is a live, watched store that can surface a transient EBUSY/EPERM on Windows). On POSIX it re-chmods the dir (0o700) and temp file (0o600) since mkdir/writeFile modes are ignored on an existing dir; win32 relies on the user-profile ACL like the main account store. codex-manager.ts — thread org id instead of mutating process.env runAuthLogin mutated the global CODEX_AUTH_ACCOUNT_ID for the duration of a login (racy on concurrent re-entry). The org is now passed explicitly to resolveAccountSelection (explicit arg wins, env still honored as fallback so the runtime-proxy mechanism is unchanged), and the env mutation is gone. Regression tests: ZWJ/flag/skin-tone/non-Latin-combining width + cluster-safe truncation; withBodyTimeout stream-cancel on timeout. typecheck + lint clean; full suite 4198 passed / 1 skipped.
Owner
Author
Round 7 — hardened the deferred itemsPushed
Regression tests added for the width clustering (ZWJ / flag / skin-tone / non-Latin combining + cluster-safe truncation) and the Verification: typecheck clean, lint clean, full suite 4198 passed / 1 skipped (273 files). |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/refresh-lease.ts (1)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick wintreat
eaccesas retryable in lease artifact cleanup.
lib/refresh-lease.ts:16andlib/refresh-lease.ts:117-145still stop oneacces, which is a common transient windows lock outcome for files held by av/indexers. inlib/refresh-lease.ts:259-279, that turns stale-lock cleanup into abypassafterwaitTimeoutMs, so duplicate refreshes can fan out even though the lock was only temporarily busy. please addeaccesto the retryable set and add a matching vitest regression next totest/refresh-lease.test.ts:184-236, which currently only proves theebusypath.suggested fix
-const RETRYABLE_IO_ERRORS = new Set(["EBUSY", "EPERM", "EMFILE", "ENFILE"]); +const RETRYABLE_IO_ERRORS = new Set([ + "EBUSY", + "EPERM", + "EACCES", + "EMFILE", + "ENFILE", +]);As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency.Also applies to: 117-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/refresh-lease.ts` around lines 16 - 17, The retryable IO error set RETRYABLE_IO_ERRORS in lib/refresh-lease.ts must include EACCES (Windows-exclusive transient file-locks) so the stale-lock cleanup and refresh retry logic (the code paths around the RETRYABLE_IO_ERRORS usage in lib/refresh-lease.ts, e.g., the cleanup/retry blocks referenced at lines ~117-145 and ~259-279) will treat EACCES as retryable; update that constant to include "EACCES" and ensure any comparisons use the same casing. Add a vitest regression next to the existing ebusy test in test/refresh-lease.test.ts (the block around lines ~184-236) that simulates an EACCES error during artifact cleanup and asserts the same retry/bypass behavior as the EBUSY test.lib/ui/select.ts (1)
77-115:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lifttruncateansi can still split zwj clusters.
the new loop in
lib/ui/select.ts:89-107budgets width one code point at a time, not one rendered cluster. with ansi-colored input like a family emoji sequence, this can still returnemoji + zwj + ..., which renders unpredictably and can throw off the line-counting this helper is trying to protect. please reuse the cluster-aware truncation logic fromlib/ui/display-width.tsfor plain spans, and add a vitest regression intest/select.test.tsfor ansi-colored zwj emoji truncation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/select.ts` around lines 77 - 115, truncateAnsi currently counts and slices by code points and can split ZWJ grapheme clusters; update truncateAnsi to reuse the cluster-aware truncation logic from lib/ui/display-width.ts for advancing and budgeting visible width over grapheme clusters (preserve the existing ANSI_LEADING_REGEX handling so escape sequences are still passed through unchanged), ensuring you measure each cluster with displayWidth and only append whole clusters to output; then add a vitest regression in test/select.test.ts that feeds an ANSI-colored ZWJ emoji sequence and asserts the truncated result does not break the cluster and renders the expected visible width with the reset sequence appended when needed.test/fetch-helpers.test.ts (1)
1763-1777:⚠️ Potential issue | 🟠 Major | ⚡ Quick windon't lock in the api-auth refresh bug here.
test/fetch-helpers.test.ts:1763-1777is asserting the opposite of the contract we want.lib/request/fetch-helpers.ts:540-563still routestype: "api"throughqueuedRefresh("")and persists an oauth body, so keepingexpect(client.auth.set).toHaveBeenCalledTimes(1)bakes in the bug instead of catching it. this regression should assert that api auth short-circuits before refresh and persistence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fetch-helpers.test.ts` around lines 1763 - 1777, The test currently asserts that API-key auth triggers a refresh and a client.auth.set call, which reinforces a bug; update the test for refreshAndUpdateToken to assert that when auth.type === "api" it short-circuits: mock or spy on refreshQueueModule.queuedRefresh and assert it was not called, assert client.auth.set was not called (0 times), and still assert the returned value is the original auth object; reference refreshAndUpdateToken, refreshQueueModule.queuedRefresh, and client.auth.set to locate the relevant assertions to change.lib/logger.ts (1)
236-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winsanitize console data strings before the sink too.
lib/logger.ts:241-257only strips cr/lf frommessage.datastill reachesconsole.*aftersanitizeValue(), which preserves raw newlines for plain strings, sologError("x", "a\nb")can still forge extra log lines. please normalize string payloads here as well and extendtest/logger.test.ts:761-775with a data-bearing regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/logger.ts` around lines 236 - 258, In logToConsole, sanitizedData can still contain raw CR/LF when it's a plain string because sanitizeValue preserves newlines; normalize string payloads the same way you do message (e.g., run maskString(...) and then .replace(/[\r\n]+/g, " ") or at least .replace on the string) before calling console.* inside logToConsole, and keep non-string data flow unchanged; also add a regression test in test/logger.test.ts that calls logError/logToConsole with data containing newlines (the referenced test block ~761-775) to assert no extra console lines are produced.
♻️ Duplicate comments (4)
lib/prompts/fetch-utils.ts (1)
38-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winnormalize header names before forcing the mandatory values.
lib/prompts/fetch-utils.ts:42-45only overwrites the exactUser-AgentandAcceptobject keys. a caller can still pass lowercaseuser-agent/accept, and fetch will normalize those case-insensitively, so the outgoing request can still carry caller-controlled values. normalize throughHeadersand callset()after merging. add the matching lowercase-key regression next totest/prompt-fetch-utils.test.ts:23-38.proposed fix
export function withPromptFetchHeaders( headers: Record<string, string> = {}, json = false, ): Record<string, string> { - return { - ...headers, - "User-Agent": PROMPT_FETCH_USER_AGENT, - Accept: json ? "application/vnd.github+json" : "text/plain, */*", - }; + const merged = new Headers(headers); + merged.set("User-Agent", PROMPT_FETCH_USER_AGENT); + merged.set( + "Accept", + json ? "application/vnd.github+json" : "text/plain, */*", + ); + return Object.fromEntries(merged.entries()); }run this to confirm the case-insensitive merge behavior:
#!/bin/bash set -euo pipefail node <<'NODE' const h = new Headers({ accept: 'text/evil', Accept: 'text/plain, */*', 'user-agent': 'evil-agent', 'User-Agent': 'codex-multi-auth', }); console.log('accept =', h.get('accept')); console.log('user-agent =', h.get('user-agent')); NODEAs per coding guidelines
lib/**: verify every change cites affected tests (vitest).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/prompts/fetch-utils.ts` around lines 38 - 46, withPromptFetchHeaders currently merges headers by object keys and then adds "User-Agent" and "Accept", which doesn't prevent callers from passing lowercase keys like "user-agent" or "accept"; change the implementation to build a Headers instance from the merged headers and then call headers.set('User-Agent', PROMPT_FETCH_USER_AGENT) and headers.set('Accept', json ? 'application/vnd.github+json' : 'text/plain, */*') to enforce case-insensitive normalization, returning a plain object (or the Headers) as needed; also add/adjust the regression test in test/prompt-fetch-utils.test.ts to assert that passing lowercase keys is overridden.lib/policy/runtime-policy.ts (1)
119-126:⚠️ Potential issue | 🟠 Majorthis still needs the ledger-rotation regression test.
lib/policy/runtime-policy.ts:119-126now depends onincludeArchives: true, but the supplied coverage still only shows the single-row case intest/runtime-policy.test.ts:113-139. add a vitest that writes a row, rotates the ledger, writes another row, then assertsevaluateRuntimePolicy()blocks using both rows. otherwise a future drop of archive inclusion will still pass the suite.As per coding guidelines
lib/**:verify every change cites affected tests (vitest).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/policy/runtime-policy.ts` around lines 119 - 126, Add a vitest in the existing test suite (e.g., test/runtime-policy.test.ts) that verifies ledger-rotation handling: use the helpers that write usage rows and call the ledger rotation routine to rotate the ledger between writes, then call summarizeUsageLedger (via evaluateRuntimePolicy) and assert the policy is enforced using both pre-rotation and post-rotation rows; specifically exercise summarizeUsageLedger with includeArchives:true by invoking evaluateRuntimePolicy so it must count archived rows and cause the request to be blocked. Reference summarizeUsageLedger and evaluateRuntimePolicy in the test and ensure the new test fails if archive inclusion is removed.lib/codex-manager/commands/debug-bundle.ts (1)
27-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winnormalize windows separators before the home-prefix check.
lib/codex-manager/commands/debug-bundle.ts:27only case-folds win32 paths. a captured path likec:/users/alice/.codex/config.jsonstill will not matchhomedir()returningC:\Users\Alice, so the debug bundle can leak the user's home path on windows.test/debug-bundle-redact.test.ts:61covers case-only differences but not mixed separators, so this stays untested.possible fix
const isWindows = process.platform === "win32"; - const normalizedValue = isWindows ? value.toLowerCase() : value; - const normalizedHome = isWindows ? home.toLowerCase() : home; + const normalizeForCompare = (input: string): string => + isWindows ? input.replaceAll("/", "\\").toLowerCase() : input; + const normalizedValue = normalizeForCompare(value); + const normalizedHome = normalizeForCompare(home);please add a mixed-separator vitest alongside
test/debug-bundle-redact.test.ts:61-82so this path leak cannot regress. as per coding guidelineslib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handleEBUSY/429scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/codex-manager/commands/debug-bundle.ts` around lines 27 - 43, The home-redaction logic currently only lowercases Windows paths but doesn't normalize mixed separators, so normalize separators (e.g., replace both "\\" and "/" with path.sep) on both value and home before computing normalizedValue/normalizedHome and performing the startsWith + boundary check (refer to variables isWindows, normalizedValue, normalizedHome, home, sep in debug-bundle.ts); then add a new vitest alongside test/debug-bundle-redact.test.ts that asserts a path like "C:/Users/Alice/.codex/config.json" (mixed separators and case differences) is redacted to "~/.codex/config.json" to prevent regressions.lib/recovery/storage.ts (1)
64-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winretry transient recovery reads before skipping them.
lib/recovery/storage.ts:65avoids false quarantine, but it still turns a brief windows lock into an immediate skip. that meansreadMessages()andreadParts()atlib/recovery/storage.ts:331andlib/recovery/storage.ts:379can hand partial state to mutating callers likefindEmptyMessages()or orphan-thinking recovery, which can then inject or strip data against a healthy session. please add bounded retry before the skip, and cover it with a vitest regression intest/recovery-storage.test.tsthat proves a transientebusy/epermread race is neither quarantined nor omitted after retry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/recovery/storage.ts` around lines 64 - 74, handleUnreadableFile currently skips on transient read errors; instead implement a bounded retry loop in handleUnreadableFile that, when isTransientReadError(error) is true, retries the read operation a few times with small backoff (e.g., exponential or fixed delay, max attempts ~3-5) before deciding to skip; if a retry succeeds, continue normal processing (so readMessages/readParts will receive the full content), and only call quarantineCorruptFile(filePath, error) if all retries still fail or the error is non-transient. Add a vitest regression in test/recovery-storage.test.ts that stubs the underlying read to throw EBUSY/EPERM for the first N-1 attempts and succeed on the last attempt, asserting the file is neither quarantined nor omitted and that readMessages/readParts return the expected data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/accounts.ts`:
- Around line 1462-1473: The removal path currently clears tracker state using
getRuntimeAccountIdentityKey(account) but tracker entries are written under the
stable getRuntimeTrackerKey(account), so clearAccountKey should be called with
getRuntimeTrackerKey(account) (and similarly for token tracker) to avoid leaving
behind old cached entries when identity fields are later enriched; update the
cleanup block to call
getHealthTracker().clearAccountKey(getRuntimeTrackerKey(account)) and
getTokenTracker().clearAccountKey(getRuntimeTrackerKey(account)), keep the
circuitKeyId removal as-is, and add a regression test (remove-after-enrichment)
modeled on the existing tests around accounts.test.ts:1047-1114 and 3971-4037 to
ensure re-adds do not inherit stale penalties.
In `@lib/codex-manager.ts`:
- Around line 1262-1270: Add a deterministic vitest regression that proves the
explicit org override wins and the env fallback is used: write tests under
lib/** that call the login/auth path that returns TokenSuccessWithAccount and
that exercise the override variable (the (orgOverride ??
process.env.CODEX_AUTH_ACCOUNT_ID ?? "").trim() branch referenced at the sites
around the snippets at lines ~1264 and ~3160) — one test should set
process.env.CODEX_AUTH_ACCOUNT_ID and pass an orgOverride and assert the
returned account matches the orgOverride, another should omit orgOverride and
assert the env value is used; make the tests deterministic by mocking/stubbing
the auth backend and queueing layer to simulate concurrent calls (simulate a
concurrent worker by invoking two login calls with swapped env/override) and
inject EBUSY/429 responses to verify retry/backoff behavior; finally ensure test
assertions check that logging produced during these flows does not contain
tokens or emails and reference the TokenSuccessWithAccount path in the test
names/comments so future refactors are caught.
In `@lib/codex-manager/experimental-settings-prompt.ts`:
- Around line 95-105: The label rendering currently rounds the draft refresh
interval to whole minutes causing sub-minute values to display incorrectly;
update the code that formats the refresh interval label (the logic that reads
the draft value where
refreshIntervalOption/refreshIntervalMin/refreshIntervalStep are used, around
the code that produced the "0 min"/"1 min" text) to format from the actual
millisecond value (e.g., show seconds or "XX ms"/"X.s s" derived from the ms
value) instead of converting to whole minutes, and adjust any helper formatting
function used there; then add a regression test in
test/settings-hub-utils.test.ts that asserts that a sub-minute value (e.g.,
25_000 ms) and a just-over-minute value (e.g., 65_000 ms) render the expected
textual labels in the menu.
In `@lib/config.ts`:
- Around line 573-589: The explain path currently calls
readConfigRecordFromPath(envConfigPath) which uses a single readFileSync and can
falsely mark the file as unreadable on transient Windows locks; update the block
that computes envConfigPath (and specifically getPluginConfigExplainReport flow)
to read the file using the retry helper readFileSyncWithConfigRetry (or modify
readConfigRecordFromPath to call readFileSyncWithConfigRetry internally) so the
explain path matches loadPluginConfig() behavior, return storageKind: "file"
when the retried read succeeds, and add a vitest regression in
test/config-explain.test.ts that simulates a transient EBUSY/EPERM/EAGAIN on
first attempt and succeeds on retry to assert explain reports the file as "file"
not "unreadable".
In `@lib/local-bridge.ts`:
- Around line 71-87: The forwardHeaders function currently only rewrites/deletes
Authorization but leaves any inbound x-api-key header intact; update
forwardHeaders to also remove/scrub the "x-api-key" header (e.g., call
result.delete("x-api-key") alongside result.delete("authorization") when no
runtimeClientApiKey is provided) so caller-supplied API keys aren't forwarded,
and add a regression test near test/local-bridge.test.ts:179-242 asserting that
neither Authorization nor x-api-key from the inbound request are present on the
forwarded headers.
In `@lib/oc-chatgpt-orchestrator.ts`:
- Around line 217-222: The finally block that does a single-shot await
fs.unlink(tempPath) can leave secret-bearing .tmp files on Windows when unlink
returns EBUSY/EPERM; change the cleanup to call the existing
withFileOperationRetry wrapper (use the same retry semantics used for the
fs.rename flow) to retry unlink(tempPath) on transient EBUSY/EPERM errors, and
update the surrounding function where tempPath is defined to use this wrapper
for cleanup; also add a vitest regression in oc-chatgpt-orchestrator.test that
simulates unlink failing with EBUSY/EPERM once (then succeeding) to assert the
retry path is exercised and the temp file is removed.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 128-135: The code accepts both '::1' and '[::1]' but uses
inconsistent forms for binding vs URL emission; update the normalization so the
server bind always receives the raw IPv6 literal (e.g., strip surrounding
brackets before calling server.listen in the startup path), while baseUrl
emission in the runtime rotation proxy uses the bracketed form (format baseUrl
as `http://[<ipv6>]:${port}`) so URLs parse unambiguously; modify the
isLoopbackHost function and the startup normalization used by the server bind
(references: isLoopbackHost, the server.listen call and the baseUrl emitter) to
normalize once at startup (store both bindHost = unbracketedIPv6 and urlHost =
bracketedIPv6) and apply those consistently across rotation paths to avoid
races, and add a vitest regression in test/runtime-rotation-proxy.test.ts that
starts the server with inputs '::1' and '[::1]' and asserts successful startup
and correct baseUrl emission on Windows and Unix runners.
In `@lib/storage/account-clear.ts`:
- Around line 2-7: Add a vitest in test/account-clear.test.ts that exercises
clearAccountStorageArtifacts() to prove it retries the widened Windows-style
errors (EACCES and ENOTEMPTY) via isRetryableFsError ->
shouldRetryFileOperation: mock/stub the underlying fs operations used by
clearAccountStorageArtifacts (e.g., fs.promises.unlink, fs.promises.rmdir or the
specific helper wrappers it calls) to throw an Error with code "EACCES" on the
first N attempts and "ENOTEMPTY" on another scenario, then succeed on a
subsequent attempt; assert that the function ultimately succeeds (or retries the
expected number of times) and that the mocks were invoked the expected number of
times so the test pins the retry contract change.
In `@lib/table-formatter.ts`:
- Around line 27-40: formatCell currently appends an ellipsis when width === 0
because truncateToWidth is called with Math.max(0, width - 1), causing a
one-column overflow; update formatCell to short-circuit and return an empty
string when width <= 0 (check at the top of the function before calling
displayWidth/truncateToWidth) so no glyphs are emitted for zero/negative widths,
and add a vitest regression in test/table-formatter.test.ts that asserts
formatCell("️🙂", 0) === "" (and optionally for negative widths) to prevent
regressions; reference functions displayWidth and truncateToWidth in the change.
In `@lib/ui/auth-menu.ts`:
- Around line 291-298: Add a deterministic vitest that covers glyph-mode quota
bar rendering for the three code paths controlled by ui.theme.glyphMode:
"unicode", "ascii", and the "auto" default (which should behave as ascii). In
the test, call the function or code that builds the bar (exercise the
useUnicodeBar / fillChar / emptyChar logic—e.g., by invoking the quota-bar
rendering helper or mounting the component that produces filledText/emptyText)
with fixed width and filled values and assert exact string output (not visual
snapshot) for each mode to catch mojibake; include explicit expectations for the
Unicode characters "█"/"▒" and ASCII "#" / "-" so the regression is detected if
glyphs change. Ensure the test file sits under the vitest suite (lib/**) and
runs deterministically without environment-dependent behavior.
In `@lib/ui/display-width.ts`:
- Around line 119-129: The ZWJ fast-path currently collapses any two width-2
code points around U+200D by checking width === 2, which incorrectly joins
non-emoji wide characters (e.g. "漢\u200d字"); update the logic in
lib/ui/display-width.ts inside the ZWJ handling (the block referencing nxt ===
0x200d and the local joined variable) to require both the leading code point and
the joined code point to be emoji/pictographic instead of just width === 2 —
implement or reuse a helper like isPictographic/isEmoji (e.g. test via RegExp
with \p{Extended_Pictographic} on String.fromCodePoint(cp) or an existing
Unicode helper) and use that for both sides before absorbing the joined code
point; after changing the condition, add a regression test in
test/display-width.test.ts asserting that "漢\u200d字" (and/or another non-emoji
wide-char ZWJ pair) yields width 4 and is not collapsed by
truncateToWidth()/displayWidth().
In `@package.json`:
- Line 113: Remove the "preuninstall" lifecycle entry from package.json and
replace it with an explicit npm script (e.g., "cleanup": "node
scripts/preuninstall.js") and README documentation instructing maintainers to
run npm run cleanup before removing the package; update scripts/preuninstall.js
to be idempotent, handle Windows vs POSIX paths (use path.join/path.resolve,
avoid shell-only commands), guard deletions to avoid removing shared
directories, and make file ops safe for concurrent runs (use atomic tmp files,
existence checks, and tolerate ENOENT); add regression tests covering explicit
cleanup invocation, Windows path edge cases, idempotency (repeated runs), and
concurrent execution scenarios.
In `@test/codex-manager-cli.test.ts`:
- Around line 1237-1250: Add assertions to ensure raw tokens aren't leaked by
extending the existing redaction checks that assert the emitted debug bundle
doesn't contain raw secrets; after the two expect(...) checks that verify
"codex@example.com" and "acc_codex" are absent from
String(logSpy.mock.calls[0]?.[0]), add two more assertions that the same string
does not contain "token-1" and does not contain "flagged-1" (using
expect(...).not.toContain(...)), referencing the same logSpy.mock.calls[0]?.[0]
expression so the test fails if refreshToken or flagged token values are
emitted.
- Around line 58-70: The test logger mock must match the real redaction contract
used in production: update the mock implementations for maskToken and
sanitizeValue to mirror lib/logger.ts:138 behavior (i.e., maskToken should not
leak prefix/suffix for long tokens but return the same fully redacted form the
production helper returns, and sanitizeValue should perform the same
redaction/inspection logic instead of returning the raw value); ensure maskEmail
and maskString follow the same redaction rules as the real helpers (use the same
truncation/placeholder logic), or better yet import/spy the actual redaction
helpers used by the code under test (maskEmail, maskString, maskToken,
sanitizeValue) so tests assert on deterministic redaction output rather than
permitting unredacted values.
In `@test/oauth-server.integration.test.ts`:
- Around line 50-55: The afterEach teardown currently only calls
waitForPortFree(OAUTH_PORT) when serverInfo is non-null, which skips the port
wait if a test closed the server and set serverInfo = null; update the afterEach
(referencing afterEach, serverInfo, waitForPortFree, and OAUTH_PORT) so it
always awaits waitForPortFree(OAUTH_PORT) regardless of serverInfo state — keep
serverInfo.close() and setting serverInfo = null inside the conditional, but
move or add a guaranteed await waitForPortFree(OAUTH_PORT) after the conditional
so the port is always polled before the next test.
In `@test/storage-parser.test.ts`:
- Line 2: The import line currently pulls vitest globals explicitly; remove
describe, it, and expect from the import and leave only vi and afterEach so the
test uses the enabled vitest globals for describe/it/expect; update the import
statement that references describe, it, expect, vi, afterEach to only import vi
and afterEach and run tests/linter to confirm no remaining explicit-global
imports.
- Around line 62-98: Add a regression test alongside the existing EBUSY test
that simulates a permission-lock error on the first primary read and verifies
the shared retry policy retries and then parses successfully: spy on fs.readFile
in the same test file (test/storage-parser.test.ts) and mockRejectedValueOnce an
Error with code "EPERM" (or "EACCES") then mockResolvedValueOnce the validJson
buffer, call loadAccountsFromPath with the same normalizeAccountStorage and
isRecord helpers, assert the returned normalized.version is 3 and that readFile
was called twice; this pins the widened retry contract exercised by
lib/storage/storage-parser.ts and ensures permission-lock variants are covered.
---
Outside diff comments:
In `@lib/logger.ts`:
- Around line 236-258: In logToConsole, sanitizedData can still contain raw
CR/LF when it's a plain string because sanitizeValue preserves newlines;
normalize string payloads the same way you do message (e.g., run maskString(...)
and then .replace(/[\r\n]+/g, " ") or at least .replace on the string) before
calling console.* inside logToConsole, and keep non-string data flow unchanged;
also add a regression test in test/logger.test.ts that calls
logError/logToConsole with data containing newlines (the referenced test block
~761-775) to assert no extra console lines are produced.
In `@lib/refresh-lease.ts`:
- Around line 16-17: The retryable IO error set RETRYABLE_IO_ERRORS in
lib/refresh-lease.ts must include EACCES (Windows-exclusive transient
file-locks) so the stale-lock cleanup and refresh retry logic (the code paths
around the RETRYABLE_IO_ERRORS usage in lib/refresh-lease.ts, e.g., the
cleanup/retry blocks referenced at lines ~117-145 and ~259-279) will treat
EACCES as retryable; update that constant to include "EACCES" and ensure any
comparisons use the same casing. Add a vitest regression next to the existing
ebusy test in test/refresh-lease.test.ts (the block around lines ~184-236) that
simulates an EACCES error during artifact cleanup and asserts the same
retry/bypass behavior as the EBUSY test.
In `@lib/ui/select.ts`:
- Around line 77-115: truncateAnsi currently counts and slices by code points
and can split ZWJ grapheme clusters; update truncateAnsi to reuse the
cluster-aware truncation logic from lib/ui/display-width.ts for advancing and
budgeting visible width over grapheme clusters (preserve the existing
ANSI_LEADING_REGEX handling so escape sequences are still passed through
unchanged), ensuring you measure each cluster with displayWidth and only append
whole clusters to output; then add a vitest regression in test/select.test.ts
that feeds an ANSI-colored ZWJ emoji sequence and asserts the truncated result
does not break the cluster and renders the expected visible width with the reset
sequence appended when needed.
In `@test/fetch-helpers.test.ts`:
- Around line 1763-1777: The test currently asserts that API-key auth triggers a
refresh and a client.auth.set call, which reinforces a bug; update the test for
refreshAndUpdateToken to assert that when auth.type === "api" it short-circuits:
mock or spy on refreshQueueModule.queuedRefresh and assert it was not called,
assert client.auth.set was not called (0 times), and still assert the returned
value is the original auth object; reference refreshAndUpdateToken,
refreshQueueModule.queuedRefresh, and client.auth.set to locate the relevant
assertions to change.
---
Duplicate comments:
In `@lib/codex-manager/commands/debug-bundle.ts`:
- Around line 27-43: The home-redaction logic currently only lowercases Windows
paths but doesn't normalize mixed separators, so normalize separators (e.g.,
replace both "\\" and "/" with path.sep) on both value and home before computing
normalizedValue/normalizedHome and performing the startsWith + boundary check
(refer to variables isWindows, normalizedValue, normalizedHome, home, sep in
debug-bundle.ts); then add a new vitest alongside
test/debug-bundle-redact.test.ts that asserts a path like
"C:/Users/Alice/.codex/config.json" (mixed separators and case differences) is
redacted to "~/.codex/config.json" to prevent regressions.
In `@lib/policy/runtime-policy.ts`:
- Around line 119-126: Add a vitest in the existing test suite (e.g.,
test/runtime-policy.test.ts) that verifies ledger-rotation handling: use the
helpers that write usage rows and call the ledger rotation routine to rotate the
ledger between writes, then call summarizeUsageLedger (via
evaluateRuntimePolicy) and assert the policy is enforced using both pre-rotation
and post-rotation rows; specifically exercise summarizeUsageLedger with
includeArchives:true by invoking evaluateRuntimePolicy so it must count archived
rows and cause the request to be blocked. Reference summarizeUsageLedger and
evaluateRuntimePolicy in the test and ensure the new test fails if archive
inclusion is removed.
In `@lib/prompts/fetch-utils.ts`:
- Around line 38-46: withPromptFetchHeaders currently merges headers by object
keys and then adds "User-Agent" and "Accept", which doesn't prevent callers from
passing lowercase keys like "user-agent" or "accept"; change the implementation
to build a Headers instance from the merged headers and then call
headers.set('User-Agent', PROMPT_FETCH_USER_AGENT) and headers.set('Accept',
json ? 'application/vnd.github+json' : 'text/plain, */*') to enforce
case-insensitive normalization, returning a plain object (or the Headers) as
needed; also add/adjust the regression test in test/prompt-fetch-utils.test.ts
to assert that passing lowercase keys is overridden.
In `@lib/recovery/storage.ts`:
- Around line 64-74: handleUnreadableFile currently skips on transient read
errors; instead implement a bounded retry loop in handleUnreadableFile that,
when isTransientReadError(error) is true, retries the read operation a few times
with small backoff (e.g., exponential or fixed delay, max attempts ~3-5) before
deciding to skip; if a retry succeeds, continue normal processing (so
readMessages/readParts will receive the full content), and only call
quarantineCorruptFile(filePath, error) if all retries still fail or the error is
non-transient. Add a vitest regression in test/recovery-storage.test.ts that
stubs the underlying read to throw EBUSY/EPERM for the first N-1 attempts and
succeed on the last attempt, asserting the file is neither quarantined nor
omitted and that readMessages/readParts return the expected data.
🪄 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: 6531d705-e5c4-40bc-b4aa-8eaf37a91f53
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (81)
.github/workflows/pr-ci.ymlAGENTS.mdSECURITY.mdeslint.config.jslib/accounts.tslib/circuit-breaker.tslib/codex-manager.tslib/codex-manager/account-manager-commands.tslib/codex-manager/commands/debug-bundle.tslib/codex-manager/commands/status.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/settings-hub/experimental.tslib/config.tslib/context-overflow.tslib/local-bridge.tslib/logger.tslib/oc-chatgpt-orchestrator.tslib/policy/runtime-policy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/prompts/host-codex-prompt.tslib/quota-readiness.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/request/fetch-helpers.tslib/rotation.tslib/runtime-rotation-proxy.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/storage/paths.tslib/storage/storage-parser.tslib/table-formatter.tslib/types.tslib/ui/auth-menu.tslib/ui/display-width.tslib/ui/select.tslib/ui/theme.tspackage.jsonscripts/codex-routing.jsscripts/verify-vendor-provenance.mjstest/account-clear.test.tstest/accounts.test.tstest/ci-workflows.test.tstest/codex-manager-cli.test.tstest/codex-manager-status-command.test.tstest/codex-prompts.test.tstest/codex-routing.test.tstest/config-explain.test.tstest/context-overflow.test.tstest/debug-bundle-redact.test.tstest/display-width.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/global-sandbox.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/local-bridge.test.tstest/logger.test.tstest/oauth-server.integration.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/paths.test.tstest/plugin-config.test.tstest/prompt-fetch-utils.test.tstest/property/setup.test.tstest/property/setup.tstest/quota-readiness.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/rotation.test.tstest/runtime-policy.test.tstest/runtime-rotation-proxy.test.tstest/select.test.tstest/settings-hub-utils.test.tstest/storage-parser.test.tstest/table-formatter.test.tstest/ui-format.test.tstest/ui-theme.test.tsvitest.config.ts
📜 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 (23)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/storage/flagged-storage-io.tslib/storage/account-clear.tslib/ui/auth-menu.tslib/codex-manager/account-manager-commands.tslib/circuit-breaker.tslib/codex-manager/settings-hub/experimental.tslib/types.tslib/refresh-queue.tslib/storage/storage-parser.tslib/policy/runtime-policy.tslib/storage/paths.tslib/request/fetch-helpers.tslib/ui/theme.tslib/table-formatter.tslib/context-overflow.tslib/accounts.tslib/storage/import-export.tslib/config.tslib/codex-manager.tslib/codex-manager/experimental-settings-prompt.tslib/local-bridge.tslib/refresh-lease.tslib/prompts/host-codex-prompt.tslib/rotation.tslib/prompts/fetch-utils.tslib/ui/select.tslib/recovery/storage.tslib/codex-manager/commands/debug-bundle.tslib/ui/display-width.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/prompts/codex.tslib/quota-readiness.tslib/logger.tslib/runtime-rotation-proxy.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handlingDo not key project storage by worktree path; use
resolveProjectStorageIdentityRoothelper instead
Files:
lib/storage/flagged-storage-io.tslib/storage/account-clear.tslib/storage/storage-parser.tslib/storage/paths.tslib/storage/import-export.ts
{index.ts,lib/**,scripts/**}
📄 CodeRabbit inference engine (AGENTS.md)
Source code lives in root
index.ts,lib/, andscripts/directories;dist/is generated output and should not be edited
Files:
lib/storage/flagged-storage-io.tsscripts/codex-routing.jslib/storage/account-clear.tslib/ui/auth-menu.tslib/codex-manager/account-manager-commands.tslib/circuit-breaker.tslib/codex-manager/settings-hub/experimental.tslib/types.tslib/refresh-queue.tslib/storage/storage-parser.tslib/policy/runtime-policy.tslib/storage/paths.tslib/request/fetch-helpers.tslib/ui/theme.tslib/table-formatter.tslib/context-overflow.tslib/accounts.tslib/storage/import-export.tslib/config.tslib/codex-manager.tslib/codex-manager/experimental-settings-prompt.tslib/local-bridge.tslib/refresh-lease.tslib/prompts/host-codex-prompt.tslib/rotation.tslib/prompts/fetch-utils.tslib/ui/select.tslib/recovery/storage.tslib/codex-manager/commands/debug-bundle.tslib/ui/display-width.tsscripts/verify-vendor-provenance.mjslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/prompts/codex.tslib/quota-readiness.tslib/logger.tslib/runtime-rotation-proxy.ts
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
lib/storage/flagged-storage-io.tstest/property/setup.test.tsscripts/codex-routing.jstest/global-sandbox.test.tslib/storage/account-clear.tstest/ci-workflows.test.tslib/ui/auth-menu.tstest/table-formatter.test.tstest/oauth-server.integration.test.tstest/runtime-policy.test.tstest/select.test.tslib/codex-manager/account-manager-commands.tslib/circuit-breaker.tstest/rotation.test.tstest/host-codex-prompt.test.tslib/codex-manager/settings-hub/experimental.tsvitest.config.tstest/ui-format.test.tslib/types.tstest/fetch-helpers.test.tslib/refresh-queue.tslib/storage/storage-parser.tstest/property/setup.tseslint.config.jslib/policy/runtime-policy.tstest/settings-hub-utils.test.tstest/quota-readiness.test.tstest/ui-theme.test.tslib/storage/paths.tstest/plugin-config.test.tstest/helpers/global-sandbox.tstest/refresh-lease.test.tslib/request/fetch-helpers.tslib/ui/theme.tstest/account-clear.test.tstest/storage-parser.test.tslib/table-formatter.tstest/logger.test.tslib/context-overflow.tstest/runtime-rotation-proxy.test.tstest/display-width.test.tstest/package-bin.test.tstest/codex-routing.test.tslib/accounts.tslib/storage/import-export.tstest/documentation.test.tstest/paths.test.tslib/config.tslib/codex-manager.tslib/codex-manager/experimental-settings-prompt.tslib/local-bridge.tstest/debug-bundle-redact.test.tslib/refresh-lease.tstest/prompt-fetch-utils.test.tstest/codex-manager-status-command.test.tslib/prompts/host-codex-prompt.tslib/rotation.tstest/recovery-storage.test.tslib/prompts/fetch-utils.tslib/ui/select.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/accounts.test.tstest/context-overflow.test.tslib/recovery/storage.tstest/codex-manager-cli.test.tslib/codex-manager/commands/debug-bundle.tslib/ui/display-width.tsscripts/verify-vendor-provenance.mjslib/oc-chatgpt-orchestrator.tstest/codex-prompts.test.tstest/config-explain.test.tslib/codex-manager/commands/status.tslib/prompts/codex.tslib/quota-readiness.tslib/logger.tslib/runtime-rotation-proxy.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errortype assertions
Files:
lib/storage/flagged-storage-io.tstest/property/setup.test.tstest/global-sandbox.test.tslib/storage/account-clear.tstest/ci-workflows.test.tslib/ui/auth-menu.tstest/table-formatter.test.tstest/oauth-server.integration.test.tstest/runtime-policy.test.tstest/select.test.tslib/codex-manager/account-manager-commands.tslib/circuit-breaker.tstest/rotation.test.tstest/host-codex-prompt.test.tslib/codex-manager/settings-hub/experimental.tsvitest.config.tstest/ui-format.test.tslib/types.tstest/fetch-helpers.test.tslib/refresh-queue.tslib/storage/storage-parser.tstest/property/setup.tslib/policy/runtime-policy.tstest/settings-hub-utils.test.tstest/quota-readiness.test.tstest/ui-theme.test.tslib/storage/paths.tstest/plugin-config.test.tstest/helpers/global-sandbox.tstest/refresh-lease.test.tslib/request/fetch-helpers.tslib/ui/theme.tstest/account-clear.test.tstest/storage-parser.test.tslib/table-formatter.tstest/logger.test.tslib/context-overflow.tstest/runtime-rotation-proxy.test.tstest/display-width.test.tstest/package-bin.test.tstest/codex-routing.test.tslib/accounts.tslib/storage/import-export.tstest/documentation.test.tstest/paths.test.tslib/config.tslib/codex-manager.tslib/codex-manager/experimental-settings-prompt.tslib/local-bridge.tstest/debug-bundle-redact.test.tslib/refresh-lease.tstest/prompt-fetch-utils.test.tstest/codex-manager-status-command.test.tslib/prompts/host-codex-prompt.tslib/rotation.tstest/recovery-storage.test.tslib/prompts/fetch-utils.tslib/ui/select.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/accounts.test.tstest/context-overflow.test.tslib/recovery/storage.tstest/codex-manager-cli.test.tslib/codex-manager/commands/debug-bundle.tslib/ui/display-width.tslib/oc-chatgpt-orchestrator.tstest/codex-prompts.test.tstest/config-explain.test.tslib/codex-manager/commands/status.tslib/prompts/codex.tslib/quota-readiness.tslib/logger.tslib/runtime-rotation-proxy.ts
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/storage/flagged-storage-io.tslib/storage/account-clear.tslib/ui/auth-menu.tslib/codex-manager/account-manager-commands.tslib/circuit-breaker.tslib/codex-manager/settings-hub/experimental.tslib/types.tslib/refresh-queue.tslib/storage/storage-parser.tslib/policy/runtime-policy.tslib/storage/paths.tslib/request/fetch-helpers.tslib/ui/theme.tslib/table-formatter.tslib/context-overflow.tslib/accounts.tslib/storage/import-export.tslib/config.tslib/codex-manager.tslib/codex-manager/experimental-settings-prompt.tslib/local-bridge.tslib/refresh-lease.tslib/prompts/host-codex-prompt.tslib/rotation.tslib/prompts/fetch-utils.tslib/ui/select.tslib/recovery/storage.tslib/codex-manager/commands/debug-bundle.tslib/ui/display-width.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/prompts/codex.tslib/quota-readiness.tslib/logger.tslib/runtime-rotation-proxy.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/property/setup.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/table-formatter.test.tstest/oauth-server.integration.test.tstest/runtime-policy.test.tstest/select.test.tstest/rotation.test.tstest/host-codex-prompt.test.tstest/ui-format.test.tstest/fetch-helpers.test.tstest/settings-hub-utils.test.tstest/quota-readiness.test.tstest/ui-theme.test.tstest/plugin-config.test.tstest/refresh-lease.test.tstest/account-clear.test.tstest/storage-parser.test.tstest/logger.test.tstest/runtime-rotation-proxy.test.tstest/display-width.test.tstest/package-bin.test.tstest/codex-routing.test.tstest/documentation.test.tstest/paths.test.tstest/debug-bundle-redact.test.tstest/prompt-fetch-utils.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/accounts.test.tstest/context-overflow.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/config-explain.test.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
{scripts/**/*.js,test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYfailures
Files:
test/property/setup.test.tsscripts/codex-routing.jstest/global-sandbox.test.tstest/ci-workflows.test.tstest/table-formatter.test.tstest/oauth-server.integration.test.tstest/runtime-policy.test.tstest/select.test.tstest/rotation.test.tstest/host-codex-prompt.test.tstest/ui-format.test.tstest/fetch-helpers.test.tstest/property/setup.tstest/settings-hub-utils.test.tstest/quota-readiness.test.tstest/ui-theme.test.tstest/plugin-config.test.tstest/helpers/global-sandbox.tstest/refresh-lease.test.tstest/account-clear.test.tstest/storage-parser.test.tstest/logger.test.tstest/runtime-rotation-proxy.test.tstest/display-width.test.tstest/package-bin.test.tstest/codex-routing.test.tstest/documentation.test.tstest/paths.test.tstest/debug-bundle-redact.test.tstest/prompt-fetch-utils.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/accounts.test.tstest/context-overflow.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/config-explain.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/property/setup.test.tstest/global-sandbox.test.tstest/ci-workflows.test.tstest/table-formatter.test.tstest/oauth-server.integration.test.tstest/runtime-policy.test.tstest/select.test.tstest/rotation.test.tstest/host-codex-prompt.test.tstest/ui-format.test.tstest/fetch-helpers.test.tstest/property/setup.tstest/settings-hub-utils.test.tstest/quota-readiness.test.tstest/ui-theme.test.tstest/plugin-config.test.tstest/helpers/global-sandbox.tstest/refresh-lease.test.tstest/account-clear.test.tstest/storage-parser.test.tstest/logger.test.tstest/runtime-rotation-proxy.test.tstest/display-width.test.tstest/package-bin.test.tstest/codex-routing.test.tstest/documentation.test.tstest/paths.test.tstest/debug-bundle-redact.test.tstest/prompt-fetch-utils.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/local-bridge.test.tstest/oc-chatgpt-orchestrator.test.tstest/accounts.test.tstest/context-overflow.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/config-explain.test.ts
scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper; only route auth commands locally
Files:
scripts/codex-routing.js
test/**/oauth-server.integration.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Bind OAuth server integration tests to port 1455; do not hardcode other ports
Files:
test/oauth-server.integration.test.ts
{scripts/codex-multi-auth.js,lib/codex-manager/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Canonical package name is
codex-multi-authand canonical command family iscodex-multi-auth ...
Files:
lib/codex-manager/account-manager-commands.tslib/codex-manager/settings-hub/experimental.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/commands/debug-bundle.tslib/codex-manager/commands/status.ts
test/**/rotation*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test Windows cleanup behavior and account selection logic in rotation tests
Files:
test/rotation.test.ts
lib/codex-manager/settings-hub/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Settings writes use queued retry for
EBUSY/EPERM/EAGAINSettings Q hotkey cancels without save; theme live-preview restores baseline on cancel
Files:
lib/codex-manager/settings-hub/experimental.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
lib/request/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
ChatGPT-backed Codex request compatibility requires stateless defaults (
store: false) unless explicit background-mode compatibility is enabled
Files:
lib/request/fetch-helpers.ts
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
lib/accounts.ts
📄 CodeRabbit inference engine (AGENTS.md)
Email dedup is case-insensitive via
normalizeEmailKey()(trim + lowercase)
Files:
lib/accounts.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
Pin
honoto version4.12.18to avoid the vulnerable4.12.0-4.12.1range (GHSA-xh87-mx6m-69f3 authentication bypass advisory)Pin
rollupto version^4.59.0or higher to keep Vite and Vitest transitive dependencies above the vulnerable<4.59.0range
Files:
package.json
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responsesDo not expose account emails or tokens in runtime proxy client response headers or logs
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:48:55.953Z
Learning: Do not edit `dist/` or local temp/cache directories
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Never commit auth files, logs, or cache artifacts to version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Do not share `~/.codex/` directories containing OAuth credentials and account metadata
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Enable debug/body logging (`ENABLE_PLUGIN_REQUEST_LOGGING`, `CODEX_PLUGIN_LOG_BODIES`) only for short-lived troubleshooting sessions
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Use PKCE-based OAuth flow for authentication
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Runtime rotation proxy must be loopback-only by default, authenticated with a local client key, and allow users to opt out via `codex-multi-auth rotation disable` or `CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Do not request or facilitate bypassing OpenAI terms or controls
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T20:49:04.211Z
Learning: Run `npm run audit:ci`, `npm run lint`, `npm run typecheck`, `npm test`, and `npm run build` before release and after dependency changes
🔇 Additional comments (21)
test/host-codex-prompt.test.ts (1)
157-163: LGTM!Also applies to: 189-194
test/oc-chatgpt-orchestrator.test.ts (1)
148-206: LGTM!Also applies to: 252-335
lib/oc-chatgpt-orchestrator.ts (1)
42-56: LGTM!Also applies to: 121-143, 243-245
test/property/setup.test.ts (1)
6-22: LGTM!test/table-formatter.test.ts (1)
65-79: LGTM!lib/codex-manager/settings-hub/experimental.ts (1)
103-114: LGTM!test/ui-format.test.ts (1)
20-22: LGTM!test/ui-theme.test.ts (1)
2-86: LGTM!lib/ui/theme.ts (1)
199-266: LGTM!test/codex-manager-cli.test.ts (1)
10313-10317: LGTM!Also applies to: 10556-10563
test/ci-workflows.test.ts (1)
49-54: LGTM!test/plugin-config.test.ts (1)
256-318: LGTM!test/helpers/global-sandbox.ts (1)
1-30: LGTM!test/refresh-lease.test.ts (1)
51-123: LGTM!Also applies to: 561-667
test/account-clear.test.ts (1)
47-49: LGTM!test/recovery-storage.test.ts (1)
157-165: LGTM!Also applies to: 168-274, 290-320, 364-405, 913-943
vitest.config.ts (1)
34-60: LGTM!test/package-bin.test.ts (1)
21-61: LGTM!lib/storage/import-export.ts (1)
4-4: LGTM!Also applies to: 20-23
test/documentation.test.ts (1)
588-607: LGTM!test/config-explain.test.ts (1)
205-259: LGTM!
Regressions I introduced earlier + pre-existing bugs surfaced by the re-scan, all fixed with regression tests. Regressions from my prior rounds: - debug-bundle.ts redactHome: on win32 it compared `/` and `\` literally, so a forward-slash path (c:/users/alice/...) bypassed the home-prefix redaction and leaked the username. Now folds case AND separator before comparing. - ui/display-width.ts: the ZWJ fast-path keyed off `width === 2`, so it wrongly collapsed a ZWJ between non-emoji wide chars (漢字 → 2 instead of 4). Now gated on emoji/pictographic code points only. - oc-chatgpt-orchestrator.ts: the temp-file cleanup unlink was still single-shot after the (already-retried) rename; route it through withFileOperationRetry so a transient Windows lock can't leave a secret-bearing .tmp behind. Pre-existing bugs: - local-bridge.ts: an inbound `x-api-key` was forwarded upstream alongside the stripped Authorization, leaking the local client credential across the bridge. Strip it too. - accounts.ts: account removal cleared tracker state under the recomputed identity key, not the stable runtime tracker key state is written under, so an account enriched after first-track left stale health/token entries a re-add inherited. Clear under getRuntimeTrackerKey (and the identity key when it differs). - config.ts: getPluginConfigExplainReport read via a single readFileSync, so a transient Windows lock reported storageKind:"unreadable" even when loadPluginConfig succeeded on retry. Route through readFileSyncWithConfigRetry. - runtime-rotation-proxy.ts: IPv6 loopback was inconsistent — server.listen needs the raw `::1` while the baseUrl needs bracketed `[::1]`. Normalize once at startup (bindHost raw, urlHost bracketed). - table-formatter.ts: a zero-width column still emitted `…`, overflowing the declared width by one and desyncing the row from the header. Short-circuit width<=0 to "". - experimental-settings-prompt.ts: the refresh-interval label rounded to whole minutes after the menu moved to a 5s step, so 25_000ms rendered "0 min". Use formatWaitTime for sub-minute granularity. - package.json: removed the `preuninstall` lifecycle hook — npm@7+ never fires it (the codebase already documents this in commands/uninstall.ts), so it was dead config. The script stays shipped for the explicit `uninstall` command; flipped the package-bin test to assert the hook is NOT wired. codex-manager.ts org-override: the explicit `login --org` now also treats a blank/whitespace value as absent so it falls back to CODEX_AUTH_ACCOUNT_ID (surfaced by the new regression). Coverage added (CodeRabbit-requested): org-override contract (new test/codex-manager-org-override.test.ts), glyph-mode quota bars (new test/auth-menu-quota-bar.test.ts), raw-token leak assertions + a realistic sanitizeValue mock in the debug-bundle cli test, mixed-separator redactHome, non-emoji ZWJ width, zero-width table column, x-api-key strip, tracker-key cleanup after enrichment, config-explain transient-lock retry, IPv6 proxy startup, sub-minute interval label, usage-ledger-rotation budget, windows drive-letter symlink case, storage-parser EPERM retry, and an unconditional oauth-server teardown port wait. Doc comments on paths.ts loop bound + canonical walk cost. typecheck + lint clean; full suite 4217 passed / 1 skipped.
Owner
Author
Round 8 — resolved all round-7 findings (21 threads)Pushed Bugs I introduced in earlier rounds (now fixed)
Pre-existing bugs
Coverage addedorg-override contract (new file), glyph-mode quota bars (new file), raw-token leak asserts + realistic Verification: typecheck clean, lint clean, full suite 4217 passed / 1 skipped (275 files). |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
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)
test/quota-readiness.test.ts (1)
1-1: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winuse the configured vitest globals here.
test/quota-readiness.test.ts:1importsdescribe,expect, anditeven though this repo enables vitest globals for test files. keeping the global style here avoids drift across the suite. as per coding guidelinestest/**/*.test.ts: "Vitest globals (describe,it,expect) are enabled and should be used without explicit imports."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/quota-readiness.test.ts` at line 1, The test imports explicit vitest symbols (describe, expect, it) which conflicts with the repo's configured Vitest globals; remove the import statement that brings in "describe, expect, it" from "vitest" and rely on the global variables instead so the test uses the configured globals; ensure there are no other references to the imported identifiers and run the test to confirm globals are picked up.test/codex-manager-status-command.test.ts (1)
1-1: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winremove the explicit vitest import in this test file.
test/codex-manager-status-command.test.ts:1is pulling in vitest globals that the repo already provides automatically. that makes this file diverge from the configured test style for no gain. as per coding guidelinestest/**/*.test.ts: "Vitest globals (describe,it,expect) are enabled and should be used without explicit imports."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-manager-status-command.test.ts` at line 1, Remove the explicit vitest import line (import { describe, expect, it, vi } from "vitest";) from test/codex-manager-status-command.test.ts and rely on the repository's Vitest globals (describe, it, expect, vi) provided by the test configuration; simply delete that import so the test uses the global symbols and conforms to the project's test style.lib/runtime-rotation-proxy.ts (1)
1304-1304: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueconsider masking at storage time for consistency.
lib/runtime-rotation-proxy.ts:1304(and lines 1422, 1725, 2135) setsstatus.lastErrorwith raw error text, relying ongetStatus()at line 2170 to mask on retrieval. this works but diverges from line 2083 which masks before storing.if a future refactor ever reads
status.lastErrordirectly (e.g., internal telemetry), the raw value would leak. masking at storage time everywhere would be defensive-in-depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/runtime-rotation-proxy.ts` at line 1304, Multiple places assign raw error text to status.lastError (e.g., the assignments at the sites where status.lastError is set around the current diff plus the similar assignments at lines 1422, 1725, 2135) while getStatus() masks on read; instead, mirror the approach used at the storage-time masking site around line 2083 and mask the error before storing. Update each assignment to call the same masking utility used at the line-2083 write path (use that exact function) and assign the masked string to status.lastError, leaving getStatus() masking as-is for defense-in-depth.lib/codex-manager/experimental-settings-prompt.ts (1)
161-177: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winadd a regression for the new schema-driven step and clamp path.
lib/codex-manager/experimental-settings-prompt.ts:161-177now changes the saved draft inrefreshIntervalStepincrements and clamps to the schema min/max, buttest/experimental-settings-prompt.test.ts:110-185only locks in label formatting. add one save-flow case that starts near the floor and ceiling and asserts the draft moves by5_000and clamps at the schema bounds, otherwise this behavior can drift without a vitest catching it.as per coding guidelines
test/**: "tests must stay deterministic and use vitest."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/codex-manager/experimental-settings-prompt.ts` around lines 161 - 177, Add a deterministic vitest that covers the save-flow for proactive refresh interval adjustments: in test/experimental-settings-prompt.test.ts add a case which starts with a draft whose proactiveRefreshIntervalMs is just above refreshIntervalMin and another just below refreshIntervalMax, dispatches the "decrease-refresh-interval" and "increase-refresh-interval" actions respectively, and asserts the saved draft moves by refreshIntervalStep (5_000) when inside bounds and clamps to refreshIntervalMin/refreshIntervalMax when at or beyond bounds; reference the symbols proactiveRefreshIntervalMs, refreshIntervalStep, refreshIntervalMin, refreshIntervalMax and the actionType strings ("decrease-refresh-interval"/"increase-refresh-interval") and implement using vitest assertions so the test stays deterministic.
♻️ Duplicate comments (1)
test/codex-manager-cli.test.ts (1)
58-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winuse the real redaction helpers in this mock.
test/codex-manager-cli.test.ts:58-93still hardcodes masking that diverges fromlib/logger.ts:138.maskTokenand the localmaskTokinsidesanitizeValueexpose token prefix/suffix, andmaskStringis still a pass-through. that makes this regression permissive again for the exact secret-redaction contract the runtime logger is supposed to enforce.proposed fix
-vi.mock("../lib/logger.js", () => ({ - createLogger: vi.fn(() => ({ - debug: loggerDebugMock, - info: loggerInfoMock, - warn: loggerWarnMock, - error: loggerErrorMock, - })), - logWarn: vi.fn(), - maskEmail: vi.fn((email: string) => { - const at = email.indexOf("@"); - if (at < 0) return "***@***"; - const local = email.slice(0, at); - const domain = email.slice(at + 1); - const tld = domain.split(".").pop() ?? ""; - return `${local.slice(0, Math.min(2, local.length))}***@***.${tld}`; - }), - maskString: vi.fn((value: string) => value), - maskToken: vi.fn((token: string) => - token.length <= 12 ? "***MASKED***" : `${token.slice(0, 6)}...${token.slice(-4)}`, - ), - sanitizeValue: vi.fn(function sv(value: unknown): unknown { - ... - }), -})); +vi.mock("../lib/logger.js", async () => { + const actual = await vi.importActual("../lib/logger.js"); + return { + ...(actual as Record<string, unknown>), + createLogger: vi.fn(() => ({ + debug: loggerDebugMock, + info: loggerInfoMock, + warn: loggerWarnMock, + error: loggerErrorMock, + })), + logWarn: vi.fn(), + }; +});As per coding guidelines
test/**: "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-manager-cli.test.ts` around lines 58 - 93, Replace the ad-hoc masking in the test mock with the real redaction helpers from lib/logger.ts: import the actual maskString/maskToken (or unified redact/sanitize) functions and use them for maskEmail, maskString, maskToken and inside sanitizeValue (remove the local maskTok and SENSITIVE duplication), so tokens are not revealed with prefix/suffix and strings are not passed through; update sanitizeValue to delegate to the real sanitizer for arrays/objects/strings and only fallback to the real helpers for sensitive keys to keep the test deterministic while matching the runtime logger's contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 1260-1268: The helper resolveAccountSelection is marked test-only
but is exported from lib/codex-manager.ts exposing it on the CLI module surface;
move this function into a new internal module (e.g.,
lib/internal/account-selection.ts), export it only from that file, update
lib/codex-manager.ts to import resolveAccountSelection from the new internal
module, and update test/codex-manager-org-override.test.ts to import the helper
from the internal module instead of from lib/codex-manager.ts; ensure you do not
re-export the internal module from lib/index.ts or any public package subpath so
the function remains internal-only.
In `@lib/config.ts`:
- Around line 47-75: The synchronous retry in readFileSyncWithConfigRetry uses
Atomics.wait (in the catch block for retryable codes) which blocks the Node.js
event loop; replace this blocking behavior by either removing retries and
returning/throwing immediately so loadPluginConfig falls back to
DEFAULT_PLUGIN_CONFIG, or implement a non-blocking approach: make
loadPluginConfig/readFileSyncWithConfigRetry asynchronous and use an await
sleep(retryDelay) loop, or if keeping sync semantics is required, remove
Atomics.wait and fail fast (no sleeps) after the first attempt; update
references to readFileSyncWithConfigRetry and callers (e.g., loadPluginConfig)
accordingly and ensure RETRYABLE_CONFIG_READ_CODES and maxAttempts are adjusted
to reflect the chosen strategy.
In `@lib/local-bridge.ts`:
- Around line 47-58: The isLoopbackHost check is fine but startLocalBridge still
uses the raw host for binding and URL emission; update the startLocalBridge (or
the function that binds the server and sets server.baseUrl) to normalize IPv6:
when binding, use the unbracketed "::1" for the listen address, and when
building/returning baseUrl bracket the host (e.g., "[::1]") only in the emitted
URL; keep existing behavior for IPv4 and "localhost". Also add vitest regression
tests in test/local-bridge.test.ts that start the bridge with host set to "::1"
and "[::1]" and assert startup succeeds and that server.baseUrl parses as a
valid URL for both cases.
In `@lib/logger.ts`:
- Around line 313-355: The cached flag logDirReady is never invalidated after a
successful ensureLogDir(), so if the log directory is later removed writes (in
the write path that calls writeFileSync) will fail with ENOENT while
ensureLogDir() remains a no-op; update the write path (the function that calls
writeFileSync) to catch filesystem errors, and if the caught error.code is
"ENOENT" (or other directory-missing codes) set logDirReady = false and call
ensureLogDir(path) (or let the next write attempt trigger it) before retrying or
surfacing the warning via logToConsole; also add a regression test near
logger.test.ts that deletes the log directory after a successful write and
verifies subsequent writes recover instead of permanently failing.
In `@lib/recovery/storage.ts`:
- Around line 123-129: The current type guard isValidStoredMessage only checks
id is a string and allows unsafe paths; update isValidStoredMessage (and/or the
readMessages flow) to validate StoredMessageMeta.id against the project's
safe-path pattern (the same rule readParts expects) and treat any id that fails
the safe-path test as invalid/quarantined so it never reaches readParts; modify
readMessages to filter/quarantine those records rather than returning them to
the recovery pass; add a vitest regression in test/recovery-storage.test.ts that
writes a message with an unsafe-but-string id (e.g., "../poison") and asserts it
is quarantined/ignored by readMessages.
In `@lib/storage/flagged-storage-io.ts`:
- Line 6: Add a vitest regression that verifies unlinkWithRetry retries
permission-style errors by mocking fs.unlink to throw EACCES or ENOTEMPTY once
and then succeed, then call clearFlaggedAccountsOnDisk (or
loadFlaggedAccountsState) which uses unlinkWithRetry; assert the operation
completes without error and that fs.unlink was invoked multiple times. Reference
the symbols RETRYABLE_UNLINK_CODES / FILE_RETRY_CODES (where the retry widening
is applied), the unlinkWithRetry helper (line ~61) and the public callers
clearFlaggedAccountsOnDisk and loadFlaggedAccountsState so the test targets the
actual code path and reproduces Windows-like transient failures. Ensure the test
lives under test/** (or an existing flagged-storage test file), uses vitest
mocks for fs.unlink, and explicitly asserts retry behavior (called >1).
In `@lib/storage/import-export.ts`:
- Line 4: The export path’s rename retry widening (adding ENOTEMPTY/EACCES via
shouldRetryFileOperation) lacks a regression test; add a vitest test that mocks
fs.rename to throw EACCES or ENOTEMPTY once then succeed and asserts
exportAccountsToFile completes and rename was invoked multiple times.
Specifically, in a test file for exports (or extend an existing export test
suite) stub/spy on fs.rename, call exportAccountsToFile (which uses
renameExportFileWithRetry in lib/storage/import-export.ts) and verify the retry
behavior and final success; ensure the test reproduces the windows-style
permission error window and fails if renameExportFileWithRetry does not consult
shouldRetryFileOperation/retry.
In `@lib/ui/select.ts`:
- Around line 434-453: The teardown currently calls stdin.setRawMode(wasRaw) and
stdout.write(ANSI.show) inside the same try, so if setRawMode throws the cursor
restoration (stdout.write(ANSI.show)) is skipped; change teardown to always
attempt to restore cursor visibility: clear and null refreshTimer
(refreshTimer), remove the data listener (onKey) in a best-effort try/catch,
then call stdin.setRawMode(wasRaw) in its own try/catch and regardless of its
result always attempt to write ANSI.show to stdout (stdout.write(ANSI.show)) in
a finally or a separate try so the cursor is restored even if setRawMode fails;
ensure stdin.pause() remains called when appropriate and keep best-effort
catches for non-critical failures.
In `@test/codex-manager-org-override.test.ts`:
- Line 1: Remove the explicit vitest import line that imports afterEach,
describe, expect, it, and vi and rely on the enabled Vitest globals instead;
delete the import statement referencing those symbols in the test file (so tests
use global describe, it, expect, afterEach and global vi) and run the test
linter to confirm no unused-import or namespace issues remain.
---
Outside diff comments:
In `@lib/codex-manager/experimental-settings-prompt.ts`:
- Around line 161-177: Add a deterministic vitest that covers the save-flow for
proactive refresh interval adjustments: in
test/experimental-settings-prompt.test.ts add a case which starts with a draft
whose proactiveRefreshIntervalMs is just above refreshIntervalMin and another
just below refreshIntervalMax, dispatches the "decrease-refresh-interval" and
"increase-refresh-interval" actions respectively, and asserts the saved draft
moves by refreshIntervalStep (5_000) when inside bounds and clamps to
refreshIntervalMin/refreshIntervalMax when at or beyond bounds; reference the
symbols proactiveRefreshIntervalMs, refreshIntervalStep, refreshIntervalMin,
refreshIntervalMax and the actionType strings
("decrease-refresh-interval"/"increase-refresh-interval") and implement using
vitest assertions so the test stays deterministic.
In `@lib/runtime-rotation-proxy.ts`:
- Line 1304: Multiple places assign raw error text to status.lastError (e.g.,
the assignments at the sites where status.lastError is set around the current
diff plus the similar assignments at lines 1422, 1725, 2135) while getStatus()
masks on read; instead, mirror the approach used at the storage-time masking
site around line 2083 and mask the error before storing. Update each assignment
to call the same masking utility used at the line-2083 write path (use that
exact function) and assign the masked string to status.lastError, leaving
getStatus() masking as-is for defense-in-depth.
In `@test/codex-manager-status-command.test.ts`:
- Line 1: Remove the explicit vitest import line (import { describe, expect, it,
vi } from "vitest";) from test/codex-manager-status-command.test.ts and rely on
the repository's Vitest globals (describe, it, expect, vi) provided by the test
configuration; simply delete that import so the test uses the global symbols and
conforms to the project's test style.
In `@test/quota-readiness.test.ts`:
- Line 1: The test imports explicit vitest symbols (describe, expect, it) which
conflicts with the repo's configured Vitest globals; remove the import statement
that brings in "describe, expect, it" from "vitest" and rely on the global
variables instead so the test uses the configured globals; ensure there are no
other references to the imported identifiers and run the test to confirm globals
are picked up.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 58-93: Replace the ad-hoc masking in the test mock with the real
redaction helpers from lib/logger.ts: import the actual maskString/maskToken (or
unified redact/sanitize) functions and use them for maskEmail, maskString,
maskToken and inside sanitizeValue (remove the local maskTok and SENSITIVE
duplication), so tokens are not revealed with prefix/suffix and strings are not
passed through; update sanitizeValue to delegate to the real sanitizer for
arrays/objects/strings and only fallback to the real helpers for sensitive keys
to keep the test deterministic while matching the runtime logger's contract.
🪄 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: 0b7570b6-8562-4ede-bb87-f570c58bc23c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (84)
.github/workflows/pr-ci.ymlAGENTS.mdSECURITY.mdeslint.config.jslib/accounts.tslib/circuit-breaker.tslib/codex-manager.tslib/codex-manager/account-manager-commands.tslib/codex-manager/commands/debug-bundle.tslib/codex-manager/commands/status.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/settings-hub/experimental.tslib/config.tslib/context-overflow.tslib/local-bridge.tslib/logger.tslib/oc-chatgpt-orchestrator.tslib/policy/runtime-policy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/prompts/host-codex-prompt.tslib/quota-readiness.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/request/fetch-helpers.tslib/rotation.tslib/runtime-rotation-proxy.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/storage/paths.tslib/storage/storage-parser.tslib/table-formatter.tslib/types.tslib/ui/auth-menu.tslib/ui/display-width.tslib/ui/select.tslib/ui/theme.tspackage.jsonscripts/codex-routing.jsscripts/verify-vendor-provenance.mjstest/account-clear.test.tstest/accounts.test.tstest/auth-menu-quota-bar.test.tstest/ci-workflows.test.tstest/codex-manager-cli.test.tstest/codex-manager-org-override.test.tstest/codex-manager-status-command.test.tstest/codex-prompts.test.tstest/codex-routing.test.tstest/config-explain.test.tstest/context-overflow.test.tstest/debug-bundle-redact.test.tstest/display-width.test.tstest/documentation.test.tstest/experimental-settings-prompt.test.tstest/fetch-helpers.test.tstest/global-sandbox.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/local-bridge.test.tstest/logger.test.tstest/oauth-server.integration.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/paths.test.tstest/plugin-config.test.tstest/prompt-fetch-utils.test.tstest/property/setup.test.tstest/property/setup.tstest/quota-readiness.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/rotation.test.tstest/runtime-policy.test.tstest/runtime-rotation-proxy.test.tstest/select.test.tstest/settings-hub-utils.test.tstest/storage-parser.test.tstest/table-formatter.test.tstest/ui-format.test.tstest/ui-theme.test.tsvitest.config.ts
📜 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 (21)
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/select.test.tstest/ui-format.test.tstest/ci-workflows.test.tstest/codex-manager-org-override.test.tstest/property/setup.test.tstest/account-clear.test.tstest/context-overflow.test.tstest/host-codex-prompt.test.tstest/experimental-settings-prompt.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/settings-hub-utils.test.tstest/global-sandbox.test.tstest/table-formatter.test.tstest/quota-readiness.test.tstest/auth-menu-quota-bar.test.tstest/documentation.test.tstest/oc-chatgpt-orchestrator.test.tstest/runtime-policy.test.tstest/storage-parser.test.tstest/oauth-server.integration.test.tstest/plugin-config.test.tstest/codex-manager-status-command.test.tstest/package-bin.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/accounts.test.tstest/config-explain.test.tstest/logger.test.tstest/paths.test.tstest/recovery-storage.test.tstest/display-width.test.tstest/prompt-fetch-utils.test.tstest/codex-routing.test.tstest/local-bridge.test.tstest/refresh-lease.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/debug-bundle-redact.test.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-error; maintain strict TypeScript type safetyEnsure runtime rotation is default-on through
codexRuntimeRotationProxy; users can opt out withcodex-multi-auth rotation disableorCODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0Do not expose account emails or tokens in runtime proxy client response headers or logs
Use
resolveProjectStorageIdentityRoot()to key project storage rather than worktree pathUse ESM modules exclusively with Node >= 18 compatibility
Files:
test/select.test.tstest/ui-format.test.tslib/storage/import-export.tstest/ci-workflows.test.tstest/codex-manager-org-override.test.tslib/table-formatter.tslib/types.tstest/property/setup.test.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tstest/account-clear.test.tslib/refresh-lease.tslib/storage/storage-parser.tstest/context-overflow.test.tslib/codex-manager/account-manager-commands.tstest/helpers/global-sandbox.tslib/refresh-queue.tstest/host-codex-prompt.test.tslib/circuit-breaker.tstest/experimental-settings-prompt.test.tsvitest.config.tstest/rotation.test.tstest/fetch-helpers.test.tslib/codex-manager/settings-hub/experimental.tstest/settings-hub-utils.test.tstest/global-sandbox.test.tstest/table-formatter.test.tstest/quota-readiness.test.tslib/ui/auth-menu.tstest/auth-menu-quota-bar.test.tstest/documentation.test.tslib/quota-readiness.tstest/oc-chatgpt-orchestrator.test.tstest/runtime-policy.test.tslib/request/fetch-helpers.tstest/property/setup.tstest/storage-parser.test.tslib/accounts.tstest/oauth-server.integration.test.tslib/ui/theme.tstest/plugin-config.test.tstest/codex-manager-status-command.test.tslib/storage/paths.tstest/package-bin.test.tslib/prompts/host-codex-prompt.tstest/codex-prompts.test.tstest/ui-theme.test.tslib/codex-manager.tstest/accounts.test.tslib/oc-chatgpt-orchestrator.tstest/config-explain.test.tstest/logger.test.tstest/paths.test.tslib/config.tslib/local-bridge.tstest/recovery-storage.test.tslib/context-overflow.tstest/display-width.test.tslib/rotation.tstest/prompt-fetch-utils.test.tslib/policy/runtime-policy.tstest/codex-routing.test.tslib/recovery/storage.tstest/local-bridge.test.tslib/prompts/codex.tslib/codex-manager/experimental-settings-prompt.tslib/ui/display-width.tstest/refresh-lease.test.tslib/prompts/fetch-utils.tslib/codex-manager/commands/status.tslib/logger.tstest/runtime-rotation-proxy.test.tslib/ui/select.tstest/codex-manager-cli.test.tslib/runtime-rotation-proxy.tstest/debug-bundle-redact.test.tslib/codex-manager/commands/debug-bundle.ts
test/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Include Windows filesystem retry handling in test coverage for cleanup and write failures
Files:
test/select.test.tstest/ui-format.test.tstest/ci-workflows.test.tstest/codex-manager-org-override.test.tstest/property/setup.test.tstest/account-clear.test.tstest/context-overflow.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/experimental-settings-prompt.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/settings-hub-utils.test.tstest/global-sandbox.test.tstest/table-formatter.test.tstest/quota-readiness.test.tstest/auth-menu-quota-bar.test.tstest/documentation.test.tstest/oc-chatgpt-orchestrator.test.tstest/runtime-policy.test.tstest/property/setup.tstest/storage-parser.test.tstest/oauth-server.integration.test.tstest/plugin-config.test.tstest/codex-manager-status-command.test.tstest/package-bin.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/accounts.test.tstest/config-explain.test.tstest/logger.test.tstest/paths.test.tstest/recovery-storage.test.tstest/display-width.test.tstest/prompt-fetch-utils.test.tstest/codex-routing.test.tstest/local-bridge.test.tstest/refresh-lease.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/debug-bundle-redact.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/select.test.tstest/ui-format.test.tstest/ci-workflows.test.tstest/codex-manager-org-override.test.tstest/property/setup.test.tstest/account-clear.test.tstest/context-overflow.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/experimental-settings-prompt.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/settings-hub-utils.test.tstest/global-sandbox.test.tstest/table-formatter.test.tstest/quota-readiness.test.tstest/auth-menu-quota-bar.test.tstest/documentation.test.tstest/oc-chatgpt-orchestrator.test.tstest/runtime-policy.test.tstest/property/setup.tstest/storage-parser.test.tstest/oauth-server.integration.test.tstest/plugin-config.test.tstest/codex-manager-status-command.test.tstest/package-bin.test.tstest/codex-prompts.test.tstest/ui-theme.test.tstest/accounts.test.tstest/config-explain.test.tstest/logger.test.tstest/paths.test.tstest/recovery-storage.test.tstest/display-width.test.tstest/prompt-fetch-utils.test.tstest/codex-routing.test.tstest/local-bridge.test.tstest/refresh-lease.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/debug-bundle-redact.test.ts
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errorsDo not hardcode OAuth ports; use existing constants/helpers for port configuration
Files:
lib/storage/import-export.tslib/table-formatter.tslib/types.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/refresh-lease.tslib/storage/storage-parser.tslib/codex-manager/account-manager-commands.tslib/refresh-queue.tslib/circuit-breaker.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tslib/quota-readiness.tslib/request/fetch-helpers.tslib/accounts.tslib/ui/theme.tslib/storage/paths.tslib/prompts/host-codex-prompt.tslib/codex-manager.tslib/oc-chatgpt-orchestrator.tslib/config.tslib/local-bridge.tslib/context-overflow.tslib/rotation.tslib/policy/runtime-policy.tslib/recovery/storage.tslib/prompts/codex.tslib/codex-manager/experimental-settings-prompt.tslib/ui/display-width.tslib/prompts/fetch-utils.tslib/codex-manager/commands/status.tslib/logger.tslib/ui/select.tslib/runtime-rotation-proxy.tslib/codex-manager/commands/debug-bundle.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handlingUse case-insensitive email deduplication via
normalizeEmailKey()function (trim + lowercase)
Files:
lib/storage/import-export.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/storage/paths.ts
**/{config,storage,credentials,auth}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Store OAuth credentials and account metadata locally under ~/.codex/multi-auth or via CODEX_MULTI_AUTH_DIR environment variable
Files:
lib/storage/import-export.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/storage/paths.ts
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/storage/import-export.tslib/table-formatter.tslib/types.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/refresh-lease.tslib/storage/storage-parser.tslib/codex-manager/account-manager-commands.tslib/refresh-queue.tslib/circuit-breaker.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tslib/quota-readiness.tslib/request/fetch-helpers.tslib/accounts.tslib/ui/theme.tslib/storage/paths.tslib/prompts/host-codex-prompt.tslib/codex-manager.tslib/oc-chatgpt-orchestrator.tslib/config.tslib/local-bridge.tslib/context-overflow.tslib/rotation.tslib/policy/runtime-policy.tslib/recovery/storage.tslib/prompts/codex.tslib/codex-manager/experimental-settings-prompt.tslib/ui/display-width.tslib/prompts/fetch-utils.tslib/codex-manager/commands/status.tslib/logger.tslib/ui/select.tslib/runtime-rotation-proxy.tslib/codex-manager/commands/debug-bundle.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
scripts/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Implement Windows filesystem safety with retry handling for transient
EBUSY,EPERM, andENOTEMPTYfailures in cleanup and write operationsDo not use bare recursive delete logic without retry handling in Windows-sensitive scripts
Files:
scripts/codex-routing.js
scripts/codex-routing.js
📄 CodeRabbit inference engine (AGENTS.md)
Route auth commands for local handling and forward non-auth commands to official Codex CLI
Files:
scripts/codex-routing.js
test/**/rotation*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test Windows cleanup behavior and account selection logic in rotation tests
Files:
test/rotation.test.ts
lib/codex-manager/settings-hub/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Settings writes use queued retry for
EBUSY/EPERM/EAGAIN
Files:
lib/codex-manager/settings-hub/experimental.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
test/**/oauth-server.integration.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Bind OAuth server integration tests to port 1455; do not hardcode other ports
Files:
test/oauth-server.integration.test.ts
package*.json
📄 CodeRabbit inference engine (SECURITY.md)
Pin hono to version 4.12.18 or higher to avoid the authentication bypass vulnerability GHSA-xh87-mx6m-69f3 in versions 4.12.0-4.12.1
Pin rollup to version ^4.59.0 or higher to avoid vulnerable versions in the Vite and Vitest transitive dependency graph
Files:
package.json
**/{cli,commands,config}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Allow users to opt out of runtime rotation proxy via codex-multi-auth rotation disable command or CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY=0 environment variable
Files:
lib/codex-manager/commands/status.tslib/codex-manager/commands/debug-bundle.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses
Files:
lib/runtime-rotation-proxy.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: The canonical package name is `codex-multi-auth` and canonical command family is `codex-multi-auth ...`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: The package does not publish a global `codex` bin; `codex-multi-auth-codex` is the explicit wrapper for official Codex command forwarding
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: Runtime rotation is loopback-only and uses a per-process client token, forwarding only Responses API and model discovery requests
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: The persistent desktop app bind is reversible and edits user config/startup metadata, not official app binaries
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: Local project-owned state defaults to `~/.codex/multi-auth`; official Codex state remains under `~/.codex`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: Settings Q hotkey cancels without saving; theme live-preview restores baseline on cancel
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:29.575Z
Learning: Do not edit `dist/` or local temp/cache directories; source lives in root `index.ts`, `lib/`, and `scripts/`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Run npm run audit:ci, lint, typecheck, test, and build before each release and after dependency changes
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Do not share ~/.codex/ directories between users
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Review connected apps in ChatGPT settings periodically
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Report vulnerabilities privately to the maintainer via GitHub profile contact channel rather than opening public issues
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Include vulnerability description, reproduction steps, impact assessment, and suggested mitigation when reporting security issues
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Prepare security fixes before public disclosure and coordinate timing to reduce user risk
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-01T22:35:40.837Z
Learning: Provide reporter attribution in security disclosures unless anonymity is requested
🔇 Additional comments (106)
AGENTS.md (1)
6-6: LGTM!.github/workflows/pr-ci.yml (1)
47-50: LGTM!lib/refresh-lease.ts (4)
31-35: LGTM!
202-218: LGTM!
235-235: LGTM!
328-333: LGTM!scripts/codex-routing.js (1)
6-8: LGTM!Also applies to: 18-18
lib/refresh-queue.ts (2)
11-11: LGTM!Also applies to: 20-31
120-120: LGTM!Also applies to: 133-134, 158-158, 170-170, 182-182, 198-198, 208-208, 256-257, 269-269, 293-293, 298-298, 309-309, 320-320, 348-348, 357-357
test/refresh-lease.test.ts (4)
51-89: LGTM!
91-123: LGTM!
565-595: LGTM!
600-667: LGTM!test/debug-bundle-redact.test.ts (2)
1-96: LGTM!
98-145: LGTM!lib/codex-manager/commands/debug-bundle.ts (3)
2-4: LGTM!Also applies to: 6-55
57-78: LGTM!
121-123: LGTM!Also applies to: 139-149
test/select.test.ts (1)
132-171: LGTM!test/ui-format.test.ts (1)
14-23: LGTM!test/settings-hub-utils.test.ts (1)
764-776: LGTM!Also applies to: 778-792, 794-807
lib/ui/auth-menu.ts (1)
284-310: LGTM!test/auth-menu-quota-bar.test.ts (1)
1-120: LGTM!lib/ui/theme.ts (1)
199-267: LGTM!test/ui-theme.test.ts (1)
1-87: LGTM!lib/ui/display-width.ts (2)
114-154: past zwj concern resolved (commit 14e1140).
lib/ui/display-width.ts:142now gates zwj collapsing onisEmojiBase(cp) && isEmojiBase(joined)instead of width-only checks. this correctly avoids collapsing non-emoji wide characters like漢\u200d字(which must stay width 4, not collapse to 2). the past review comment flagged this and it's marked as addressed in commit 14e1140. good fix.
1-196: ⚡ Quick winensure display-width follows the lib/ export rules**
lib/ui/display-width.ts:157,178exportsdisplayWidthandtruncateToWidth, butlib/index.tsdoes not re-export them; wire them throughlib/index.tsor document/support them as an explicit public subpath (e.g., viapackage.jsonexports).- the non-emoji zwj regression is already covered:
test/display-width.test.ts:66-70asserts漢\u200d字stays width 4.lib/storage/account-clear.ts (1)
2-7: LGTM!test/account-clear.test.ts (1)
44-82: LGTM!lib/storage/storage-parser.ts (1)
7-7: LGTM!Also applies to: 55-60
test/storage-parser.test.ts (4)
2-2: LGTM!Also applies to: 13-15
62-82: LGTM!
84-105: LGTM!
107-121: LGTM!test/codex-prompts.test.ts (5)
11-12: LGTM!Also applies to: 31-32, 38-41
163-215: LGTM!
216-263: LGTM!
321-381: LGTM!
383-419: LGTM!package.json (1)
140-142: LGTM!Also applies to: 174-174
test/ci-workflows.test.ts (1)
49-54: LGTM!test/helpers/global-sandbox.ts (1)
1-31: LGTM!test/oc-chatgpt-orchestrator.test.ts (2)
148-206: LGTM!
257-335: LGTM!lib/storage/paths.ts (2)
403-442: LGTM!
481-529: LGTM!lib/oc-chatgpt-orchestrator.ts (4)
42-56: LGTM!
121-143: LGTM!
184-228: LGTM!
247-250: LGTM!test/config-explain.test.ts (3)
206-231: LGTM!
233-259: LGTM!
275-331: LGTM!test/paths.test.ts (2)
801-876: LGTM!
886-911: LGTM!lib/table-formatter.ts (1)
27-44: LGTM!vitest.config.ts (1)
30-60: LGTM!test/documentation.test.ts (1)
588-607: LGTM!test/runtime-policy.test.ts (1)
84-111: LGTM!Also applies to: 248-309
lib/config.ts (2)
269-284: LGTM!Also applies to: 575-597
1978-1995: LGTM!test/recovery-storage.test.ts (1)
157-165: LGTM!Also applies to: 168-203, 205-241, 243-274, 915-935, 938-943
test/display-width.test.ts (1)
1-109: LGTM!lib/types.ts (1)
236-242: LGTM!test/global-sandbox.test.ts (1)
1-30: LGTM!eslint.config.js (1)
35-72: LGTM!lib/prompts/host-codex-prompt.ts (4)
13-13: LGTM!
16-23: LGTM!
281-281: LGTM!
313-326: LGTM!test/prompt-fetch-utils.test.ts (1)
1-187: LGTM!lib/prompts/codex.ts (9)
4-4: LGTM!
9-10: LGTM!
13-15: LGTM!
33-65: LGTM!
177-182: LGTM!
195-216: LGTM!
271-306: LGTM!
379-402: LGTM!
410-420: LGTM!lib/prompts/fetch-utils.ts (1)
1-190: LGTM!test/property/setup.test.ts (2)
6-14: LGTM!
16-22: LGTM!test/context-overflow.test.ts (2)
76-92: LGTM!
94-107: LGTM!test/host-codex-prompt.test.ts (2)
157-164: LGTM!
189-194: LGTM!test/oauth-server.integration.test.ts (2)
9-45: LGTM!
50-61: LGTM!test/plugin-config.test.ts (3)
256-277: LGTM!
279-291: LGTM!
293-318: LGTM!test/package-bin.test.ts (2)
21-47: LGTM!
49-65: LGTM!lib/context-overflow.ts (2)
52-66: LGTM!
69-120: LGTM!lib/runtime-rotation-proxy.ts (8)
1-1: LGTM!Also applies to: 23-24, 49-49, 79-83
128-170: LGTM!
496-507: LGTM!
1015-1030: LGTM!Also applies to: 1093-1097
1319-1343: LGTM!Also applies to: 1359-1359, 1414-1414, 1602-1602
1431-1446: LGTM!Also applies to: 1515-1515
2078-2091: LGTM!
2149-2171: LGTM!test/codex-manager-cli.test.ts (2)
1260-1277: LGTM!
10341-10343: LGTM!Also applies to: 10586-10589
CRITICAL — config.ts: drop the Atomics.wait blocking sleep on the sync path.
readFileSyncWithConfigRetry slept via Atomics.wait, and loadPluginConfig() is
synchronous + runs on startup, so a transient Windows lock froze the entire
event loop for up to ~150ms. Replaced with immediate (non-blocking) retries; a
brief exclusive lock clears within a tick and persistent failure falls through
to the existing unreadable/default handling. The async config path already used
await sleep and is unchanged.
MAJOR (security) — recovery/storage.ts: reject path-unsafe message/part ids.
The structural validators only checked `id` was a string, so `{ "id":
"../poison" }` survived readMessages/readParts and would feed a traversal into
readParts(msg.id). Both validators now also require SAFE_ID_PATTERN, so a
traversal id is quarantined like corruption.
MAJOR — local-bridge.ts: normalize IPv6 loopback for bind vs baseUrl.
isLoopbackHost accepted "[::1]" but startLocalBridge used the raw host for both
server.listen (needs "::1") and baseUrl (needs "[::1]"), so "[::1]" failed the
bind and "::1" produced "http://::1:port". Normalize once (bindHost raw, urlHost
bracketed); request URL + returned host/baseUrl all use the right form. Mirrors
the runtime-rotation-proxy fix.
MAJOR — logger.ts: invalidate the logDirReady cache on ENOENT writes.
ensureLogDir caches "ready" after first success; if LOG_DIR is later deleted,
writeFileSync fails ENOENT forever. Reset the cache on a directory-missing write
failure so the next logRequest recreates the dir.
MAJOR — codex-manager.ts: stop exporting the CLI-internal resolveAccountSelection.
Extracted the org-override precedence into lib/auth/org-override.ts
(resolveOrgOverride) so the concurrency contract is unit-testable without
widening the CLI module's public surface; resolveAccountSelection is internal
again and delegates to it.
MINOR — ui/select.ts: restore the cursor even if setRawMode throws.
stdout.write(ANSI.show) shared a try with the fragile setRawMode, so a throw
there left the terminal cursor hidden. Each teardown step now has its own try.
Tests: unsafe-id quarantine (messages + parts), config-explain transient-lock
already covered, logger ENOENT cache-reset, IPv6 bridge bind/baseUrl (::1 and
[::1]), resolveOrgOverride precedence (replacing the internal-import test),
flagged-storage + export rename retries for ENOTEMPTY/EACCES. Dropped explicit
vitest imports in the org-override test.
typecheck + lint clean; full suite 4228 passed / 1 skipped.
Owner
Author
Round 9 — resolved round-8 findings (incl. a CRITICAL)Pushed
Coverage added: unsafe-id quarantine (messages + parts), logger ENOENT cache-reset, IPv6 bridge bind/baseUrl ( Verification: typecheck clean, lint clean, full suite 4228 passed / 1 skipped. |
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/ui/theme.ts (1)
199-266: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winadd vitest coverage for color-disable precedence.
lib/ui/theme.ts:209-266changes env-driven rendering for every tty path, but none of the provided tests lock inFORCE_COLORvsNO_COLORvs non-tty precedence or the explicitdisableColoroverride. a small precedence drift here will silently flip ansi output across the cli. please add table-driven coverage intest/ui-theme.test.tsfor those cases. 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."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/theme.ts` around lines 199 - 266, Add vitest table-driven tests that assert the precedence and override behavior for color disabling: cover combinations of env.FORCE_COLOR ("0", "1", "false", "true", unset), env.NO_COLOR present/absent, and isTTY true/false, plus explicit options.disableColor true/false; exercise shouldDisableColor(...) directly and createUiTheme({disableColor}) to ensure stripColors(colors) is used when expected. Reference shouldDisableColor, createUiTheme, stripColors, and getGlyphs/getColors in the tests and assert that ANSI tokens are present or removed accordingly so any precedence drift (FORCE_COLOR > NO_COLOR > non-TTY, explicit disableColor override) is locked in.test/select.test.ts (1)
1-1: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winuse vitest globals here.
test/select.test.ts:1explicitly importsafterEach,beforeEach,describe,expect, andit, but this repo enables vitest globals for test files. drop the named imports and keep onlyviif you still need the mocking api. as per coding guidelines, "test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/select.test.ts` at line 1, The test file imports Vitest globals explicitly; remove the named imports afterEach, beforeEach, describe, expect, and it from the import statement and only import vi from "vitest" (the test globals will be available implicitly), ensuring any mocking calls still use vi (e.g., vi.spyOn/vi.mock) and no other import references remain.test/flagged-storage-io.test.ts (1)
21-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winreplace the bare teardown
rmwithremovewithretry.
test/flagged-storage-io.test.ts:25still uses barefs.rm(...), so this suite can flake on windows with transientebusy/eperm/enotemptyduring cleanup. please switch theafterEachcleanup toremoveWithRetry(...)like the other filesystem-heavy tests. as per coding guidelinestest/**/*.test.ts:Use removeWithRetry for Windows filesystem cleanup instead of bare fs.rm to handle EBUSY/EPERM/ENOTEMPTY backoff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/flagged-storage-io.test.ts` around lines 21 - 29, The afterEach teardown uses a bare fs.rm call which flakes on Windows; replace the rm logic in the afterEach callback with the shared removeWithRetry utility, calling await removeWithRetry(testTmpRoot) (or equivalent signature used elsewhere) and remove the try/catch around fs.rm; ensure the file imports/uses removeWithRetry (and not fs.rm) and keep vi.restoreAllMocks() and vi.useRealTimers() intact so afterEach still restores mocks and timers before performing removeWithRetry.
♻️ Duplicate comments (2)
lib/logger.ts (1)
391-398:⚠️ Potential issue | 🟠 Major | ⚡ Quick wininvalidate the cached log-dir state on
enotdirtoo.in
lib/logger.ts:391-398, line 396 only clearslogDirReadyforenoent. if the cached log directory gets replaced by a file, or one parent stops being a directory,writeFileSync()fails withenotdirand request logging stays dead until restart. please invalidate the cache for both codes and extend the newtest/logger.test.ts:949-978regression to cover the replacement case too.possible fix
- if (code === "ENOENT") { + if (code === "ENOENT" || code === "ENOTDIR") { logDirReady = false; }as per coding guidelines
lib/**: focus on auth rotation, windows filesystem io, and concurrency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/logger.ts` around lines 391 - 398, The code currently only clears the cached log-dir flag when write errors produce ENOENT; update the error-handling in the logger (the block that checks (e as NodeJS.ErrnoException | undefined)?.code and sets logDirReady = false) to also invalidate the cache for ENOTDIR so log rotation/replacement-by-file cases recover; adjust ensureLogDir/logging error path that wraps writeFileSync to treat code === "ENOTDIR" the same as "ENOENT". Also extend the regression test in test/logger.test.ts (around the new 949-978 test) to add a case where the log directory is replaced by a file (simulate ENOTDIR) and assert logDirReady is cleared and logging recovers.test/codex-manager-cli.test.ts (1)
58-93:⚠️ Potential issue | 🟠 Majoralign the logger mock with the real redaction contract.
this mock at
test/codex-manager-cli.test.ts:58still reimplements the redaction rules fromlib/logger.ts:138, andmaskTokenattest/codex-manager-cli.test.ts:67keeps the reversible prefix/suffix shape that phase 1 explicitly removed. that means the bundle tests can still pass whilelib/logger.ts:138regresses back to leaking token fingerprints or raw strings throughmaskString.as per coding guidelines
test/**: "reject changes that mock real secrets or skip assertions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/codex-manager-cli.test.ts` around lines 58 - 93, The test logger mock reimplements redaction and still exposes a reversible token shape; update the mock so it matches the real redaction contract in lib/logger.ts instead of reimplementing it: replace the inline implementations for sanitizeValue, maskToken, maskString (and maskEmail if necessary) by reusing or importing the real redaction utilities from lib/logger.ts (or copy the exact regex/behavior from that file), ensure maskToken no longer reveals prefix/suffix (return the same non-reversible masked value used by lib/logger), and make sanitizeValue use the exact sensitive-key detection and masking flow from lib/logger so tests will fail if lib/logger regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/config.ts`:
- Around line 279-305: The code treats an env override path
(CODEX_MULTI_AUTH_CONFIG_PATH) as present even when the file doesn't exist,
causing
resolvePluginConfigPath()/loadPluginConfig()/getPluginConfigExplainReport() to
fall back to defaults instead of the real legacy file; update the logic so any
env path is considered present only if the file actually exists (i.e., check
existsSync or equivalent before returning/using the env path), and ensure both
loadPluginConfig and getPluginConfigExplainReport rely on that existence-aware
resolvePluginConfigPath behavior; add a regression test in
test/plugin-config.test.ts that sets CODEX_MULTI_AUTH_CONFIG_PATH to a
non-existent file while a legacy config exists on disk and asserts
loadPluginConfig returns the legacy config (not defaults).
In `@lib/prompts/codex.ts`:
- Around line 277-305: The code currently treats cachedMetadata.sha256 absence
as integrityOk; change logic so a missing sha is considered unverified
(integrityOk = false) and therefore does not fast-path serve the cached body or
participate in conditional revalidation; instead force a full 200 fetch to
obtain and store the first digest while allowing the old bytes only as an
offline fallback. Concretely, update the integrity check around
cachedMetadata/sha256 (the variable integrityOk and its usage with
usableDiskContent, cachedMetadata, setCacheEntry, and
refreshInstructionsInBackground) so that "!cachedMetadata.sha256" does not make
integrityOk true, ensure you do not leave cachedMetadata populated (or send
If-None-Match) for entries without a sha, and only set cachedMetadata and call
setCacheEntry after a successful full fetch that yields a sha.
In `@lib/recovery/storage.ts`:
- Around line 123-133: The validator isValidStoredMessage currently only checks
id and allows records with a non-numeric time.created to pass, which later makes
the comparator that sorts on time.created produce NaN and break chronology;
update isValidStoredMessage to also check that if (value as
StoredMessageMeta).time?.created exists it is a finite number (or parseable to a
finite number) and reject/quarantine the record if not, and update the
sorting/comparator code that relies on time.created to assume numeric values
only; add a regression test that inserts a parseable record like { id: "msg_1",
time: { created: "oops" } } and asserts it is quarantined (not included in
chronological recovery) to prevent regressions.
In `@lib/runtime-rotation-proxy.ts`:
- Around line 79-83: Remove the non-loopback escape hatch by deleting the
allowNonLoopbackHost property and any conditional branches referencing it
(search for allowNonLoopbackHost and usages around the runtime rotation proxy
code, including the declaration and the runtime bind logic near the later
occurrences), make the proxy bind loopback-only by default (force bind to
127.0.0.1/localhost in the proxy start/bind logic), and ensure the proxy
requires the local client key for authentication when accepting connections
(update the proxy initialization/auth checks to always enforce the local client
key rather than gating enforcement behind allowNonLoopbackHost).
In `@lib/storage/import-export.ts`:
- Around line 20-23: The staged-export cleanup currently does a single-shot
fs.unlink(tempPath); update lib/storage/import-export.ts to route that unlink
through the same retry policy used for renames (use shouldRetryFileOperation and
EXPORT_RENAME_MAX_ATTEMPTS/backoff) so transient
ENOTEMPTY/EACCES/EBUSY/EAGAIN/EPERM are retried, and ensure the retry
helper/logic invoked is the shared one referenced at the rename site
(shouldRetryFileOperation and the same attempt+1 < EXPORT_RENAME_MAX_ATTEMPTS
check); then add a vitest regression in test/import-export.test.ts that
stubs/mocks fs.unlink to fail once with EACCES or EBUSY and succeed on the retry
to assert the .tmp file is cleaned up, and update any test setup to cover
Windows-style IO/concurrency scenarios as noted.
In `@lib/ui/display-width.ts`:
- Around line 80-103: The codePointWidth logic under function codePointWidth
currently treats many emoji as width 1 and lets subsequent handling (e.g., in
the zero-width follower logic around the block at lines 127-153) keep that
underestimated width; update codePointWidth to special-case common
emoji-presentation and keycap clusters (e.g., sequences involving U+FE0F
emoji-presentation selector and U+20E3 keycap combining mark, plus common emoji
ranges like Dingbats and Misc Symbols where FE0F implies emoji width 2) so that
base width returns 2 for those display-emoji code points, and ensure the
zero-width follower handling in the same module still treats variation
selectors/combining-keycap as combining (not adding width). Add regression
tests: extend test/display-width.test.ts to assert widths for "☀️" and "❤️" and
"1️⃣" (and a non-emoji control) and add a small case in
test/table-formatter.test.ts that verifies table alignment with those emoji
inputs.
In `@test/auth-menu-quota-bar.test.ts`:
- Around line 78-98: The tests overwrite process.stdin.isTTY and
process.stdout.isTTY in beforeEach but never restore them, leaking non-TTY
state; update the beforeEach to capture the original property descriptors (e.g.,
const originalStdinTTY = Object.getOwnPropertyDescriptor(process.stdin, "isTTY")
and similarly for stdout) and then restore those descriptors in afterEach before
calling resetUiRuntimeOptions()/vi.restoreAllMocks(); ensure you reference the
same symbols (beforeEach, afterEach, resetUiRuntimeOptions) and use
Object.defineProperty to put the original descriptors back to avoid affecting
other suites.
- Line 1: Replace the explicit Vitest globals import by removing afterEach,
beforeEach, describe, expect, and it from the import statement and only import
vi if you need the mocking API; use the enabled Vitest globals (describe, it,
expect, beforeEach, afterEach) directly in the test file and keep vi (symbol vi)
imported from "vitest" if mocks/spies are used.
In `@test/codex-manager-cli.test.ts`:
- Around line 10341-10344: The two tests hardcode the guardian interval step
(5000) when computing proactiveRefreshIntervalMs; instead derive that step from
the already-imported backend settings schema at the top of the test file (use
the schema variable and its guardian interval step property) and replace the
literal 5000 math with a computed expression using that property; specifically
update the object setting proactiveRefreshIntervalMs and the similar assertion
further down to compute 175_000 (and related deltas) by using the imported
schema's guardian interval step (e.g., schema.guardianIntervalStep or the actual
property name on the imported backend settings schema) rather than the literal
5000.
In `@test/host-codex-prompt.test.ts`:
- Around line 157-163: Add the Accept header to the relaxed fetch assertions:
update one of the mockFetch expect.objectContaining({ headers:
expect.objectContaining({...}) }) checks (the blocks currently matching
"If-None-Match" and "User-Agent") to also assert the presence/value of the
"Accept" header (e.g., include "Accept" alongside "If-None-Match" or
"User-Agent" using expect.objectContaining so the test guards the prompt
hardening in fetch-utils regarding Accept).
In `@test/local-bridge.test.ts`:
- Around line 207-234: The test shows we rewrite Authorization to the configured
runtimeClientApiKey but we also need to ensure incoming x-api-key is stripped
before forwarding so it cannot leak; update the proxying logic in the local
bridge (the code that performs the fetch to runtimeBaseUrl — the same block that
reads runtimeClientApiKey and sets Authorization) to explicitly delete the
incoming "x-api-key" header from the forwarded request headers (and any
duplicate casing) before calling fetch, keeping the existing behavior of
replacing Authorization with runtimeClientApiKey; reference the
runtimeClientApiKey handling and the proxy fetch logic that currently replaces
Authorization so you remove headers["x-api-key"] (and headers["X-API-KEY"]) from
the Headers/init used for the forwarded request.
In `@test/oauth-server.integration.test.ts`:
- Around line 21-31: The test's port-probing helper waitForPortFree currently
only binds probes to 127.0.0.1 which mismatches the OAuth server's actual bind
host (AUTH_REDIRECT.host / localhost) and permits dual-stack flakes; update
waitForPortFree to probe the same host used by startLocalOAuthServer (use
AUTH_REDIRECT.host or accept a host parameter) so it attempts the same address
family as the server, and adjust the afterEach teardown to call waitForPortFree
with the correct host instead of unconditionally probing 127.0.0.1; additionally
add a deterministic regression that pre-binds a throwaway IPv6 listener on
::1:1455 and assert that waitForPortFree(1455[, host='::1' or using the host
param]) does not resolve until that IPv6 listener is closed to cover the
dual-stack/windows edge case and the async server.close() timing in
startLocalOAuthServer/server.close.
In `@test/oc-chatgpt-orchestrator.test.ts`:
- Around line 252-335: The tests only validate the final file state but don't
assert the atomic staging path; update the test to assert staging behavior by
either spying on node:fs/promises calls or injecting hooks into the persister:
when calling applyOcChatgptSync in the test, attach spies to writeFile and
rename (or mock persistMergedDefault via the dependencies) and assert writeFile
was called with a temp filename ending in ".tmp" and that rename was invoked to
move that temp path to accountPath; alternatively add an injectable hook in
persistMergedDefault (the persister used by lib/oc-chatgpt-orchestrator.ts) to
expose the temp path and verify the final commit occurred via rename rather than
a direct write.
In `@test/rotation.test.ts`:
- Around line 189-197: The tests currently use numeric keys on both write and
clear paths so they don't verify normalization; update the two failing cases in
test/rotation.test.ts to use mismatched types (use tracker.recordFailure(0,
"codex") and then tracker.clearAccountKey("0") — and in the other case use the
string for record/number for clear) so one side is numeric and the other is a
string, then assert via tracker.getScore(0, "codex") (or getScore("0", "codex")
as appropriate) still returns DEFAULT_HEALTH_SCORE_CONFIG.maxScore; this ensures
the account-key normalization logic in functions like recordFailure,
clearAccountKey and getScore is actually exercised.
In `@test/storage-parser.test.ts`:
- Around line 71-74: The fs.readFile spy in test/storage-parser.test.ts should
mock successful utf-8 reads as strings, not Buffers, because storage-parser.ts
calls fs.readFile(path, "utf-8"); update the mockResolvedValueOnce calls in the
readSpy (the vi.spyOn(fs, "readFile") chain used around the tests) to return
validJson as a string (e.g., validJson) instead of validJson as unknown as
Buffer so the retry behavior matches real runtime return types.
---
Outside diff comments:
In `@lib/ui/theme.ts`:
- Around line 199-266: Add vitest table-driven tests that assert the precedence
and override behavior for color disabling: cover combinations of env.FORCE_COLOR
("0", "1", "false", "true", unset), env.NO_COLOR present/absent, and isTTY
true/false, plus explicit options.disableColor true/false; exercise
shouldDisableColor(...) directly and createUiTheme({disableColor}) to ensure
stripColors(colors) is used when expected. Reference shouldDisableColor,
createUiTheme, stripColors, and getGlyphs/getColors in the tests and assert that
ANSI tokens are present or removed accordingly so any precedence drift
(FORCE_COLOR > NO_COLOR > non-TTY, explicit disableColor override) is locked in.
In `@test/flagged-storage-io.test.ts`:
- Around line 21-29: The afterEach teardown uses a bare fs.rm call which flakes
on Windows; replace the rm logic in the afterEach callback with the shared
removeWithRetry utility, calling await removeWithRetry(testTmpRoot) (or
equivalent signature used elsewhere) and remove the try/catch around fs.rm;
ensure the file imports/uses removeWithRetry (and not fs.rm) and keep
vi.restoreAllMocks() and vi.useRealTimers() intact so afterEach still restores
mocks and timers before performing removeWithRetry.
In `@test/select.test.ts`:
- Line 1: The test file imports Vitest globals explicitly; remove the named
imports afterEach, beforeEach, describe, expect, and it from the import
statement and only import vi from "vitest" (the test globals will be available
implicitly), ensuring any mocking calls still use vi (e.g., vi.spyOn/vi.mock)
and no other import references remain.
---
Duplicate comments:
In `@lib/logger.ts`:
- Around line 391-398: The code currently only clears the cached log-dir flag
when write errors produce ENOENT; update the error-handling in the logger (the
block that checks (e as NodeJS.ErrnoException | undefined)?.code and sets
logDirReady = false) to also invalidate the cache for ENOTDIR so log
rotation/replacement-by-file cases recover; adjust ensureLogDir/logging error
path that wraps writeFileSync to treat code === "ENOTDIR" the same as "ENOENT".
Also extend the regression test in test/logger.test.ts (around the new 949-978
test) to add a case where the log directory is replaced by a file (simulate
ENOTDIR) and assert logDirReady is cleared and logging recovers.
In `@test/codex-manager-cli.test.ts`:
- Around line 58-93: The test logger mock reimplements redaction and still
exposes a reversible token shape; update the mock so it matches the real
redaction contract in lib/logger.ts instead of reimplementing it: replace the
inline implementations for sanitizeValue, maskToken, maskString (and maskEmail
if necessary) by reusing or importing the real redaction utilities from
lib/logger.ts (or copy the exact regex/behavior from that file), ensure
maskToken no longer reveals prefix/suffix (return the same non-reversible masked
value used by lib/logger), and make sanitizeValue use the exact sensitive-key
detection and masking flow from lib/logger so tests will fail if lib/logger
regresses.
🪄 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: 6120f105-f757-46dd-a2b4-8b8ab765a1db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (87)
.github/workflows/pr-ci.ymlAGENTS.mdSECURITY.mdeslint.config.jslib/accounts.tslib/auth/org-override.tslib/circuit-breaker.tslib/codex-manager.tslib/codex-manager/account-manager-commands.tslib/codex-manager/commands/debug-bundle.tslib/codex-manager/commands/status.tslib/codex-manager/experimental-settings-prompt.tslib/codex-manager/settings-hub/experimental.tslib/config.tslib/context-overflow.tslib/local-bridge.tslib/logger.tslib/oc-chatgpt-orchestrator.tslib/policy/runtime-policy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/prompts/host-codex-prompt.tslib/quota-readiness.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/request/fetch-helpers.tslib/rotation.tslib/runtime-rotation-proxy.tslib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/import-export.tslib/storage/paths.tslib/storage/storage-parser.tslib/table-formatter.tslib/types.tslib/ui/auth-menu.tslib/ui/display-width.tslib/ui/select.tslib/ui/theme.tspackage.jsonscripts/codex-routing.jsscripts/verify-vendor-provenance.mjstest/account-clear.test.tstest/accounts.test.tstest/auth-menu-quota-bar.test.tstest/ci-workflows.test.tstest/codex-manager-cli.test.tstest/codex-manager-org-override.test.tstest/codex-manager-status-command.test.tstest/codex-prompts.test.tstest/codex-routing.test.tstest/config-explain.test.tstest/context-overflow.test.tstest/debug-bundle-redact.test.tstest/display-width.test.tstest/documentation.test.tstest/experimental-settings-prompt.test.tstest/fetch-helpers.test.tstest/flagged-storage-io.test.tstest/global-sandbox.test.tstest/helpers/global-sandbox.tstest/host-codex-prompt.test.tstest/import-export.test.tstest/local-bridge.test.tstest/logger.test.tstest/oauth-server.integration.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/paths.test.tstest/plugin-config.test.tstest/prompt-fetch-utils.test.tstest/property/setup.test.tstest/property/setup.tstest/quota-readiness.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/rotation.test.tstest/runtime-policy.test.tstest/runtime-rotation-proxy.test.tstest/select.test.tstest/settings-hub-utils.test.tstest/storage-parser.test.tstest/table-formatter.test.tstest/ui-format.test.tstest/ui-theme.test.tsvitest.config.ts
📜 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 (25)
lib/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/**/*.ts: All public exports should flow throughlib/index.tsor documented package subpaths
Never import fromdist/in source tests or library code
Never suppress type errors
Files:
lib/circuit-breaker.tslib/auth/org-override.tslib/storage/account-clear.tslib/codex-manager/account-manager-commands.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/table-formatter.tslib/codex-manager/experimental-settings-prompt.tslib/policy/runtime-policy.tslib/storage/import-export.tslib/context-overflow.tslib/storage/paths.tslib/request/fetch-helpers.tslib/types.tslib/ui/theme.tslib/codex-manager/commands/debug-bundle.tslib/quota-readiness.tslib/rotation.tslib/ui/select.tslib/prompts/host-codex-prompt.tslib/refresh-lease.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/refresh-queue.tslib/accounts.tslib/logger.tslib/ui/display-width.tslib/local-bridge.tslib/codex-manager.tslib/runtime-rotation-proxy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/config.tslib/recovery/storage.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
Source code must be written in ESM only with Node >= 18 compatibility
Files:
lib/circuit-breaker.tslib/auth/org-override.tstest/ui-format.test.tseslint.config.jstest/account-clear.test.tslib/storage/account-clear.tslib/codex-manager/account-manager-commands.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tstest/codex-manager-org-override.test.tslib/storage/flagged-storage-io.tstest/ui-theme.test.tslib/storage/storage-parser.tslib/table-formatter.tstest/oauth-server.integration.test.tslib/codex-manager/experimental-settings-prompt.tstest/flagged-storage-io.test.tstest/property/setup.test.tslib/policy/runtime-policy.tslib/storage/import-export.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/property/setup.tslib/context-overflow.tstest/context-overflow.test.tslib/storage/paths.tstest/helpers/global-sandbox.tstest/storage-parser.test.tslib/request/fetch-helpers.tslib/types.tstest/quota-readiness.test.tstest/documentation.test.tsvitest.config.tslib/ui/theme.tslib/codex-manager/commands/debug-bundle.tslib/quota-readiness.tsscripts/codex-routing.jstest/select.test.tslib/rotation.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tslib/ui/select.tslib/prompts/host-codex-prompt.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tslib/refresh-lease.tstest/logger.test.tstest/runtime-policy.test.tslib/oc-chatgpt-orchestrator.tstest/codex-routing.test.tslib/codex-manager/commands/status.tslib/refresh-queue.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tslib/accounts.tstest/refresh-lease.test.tstest/local-bridge.test.tslib/logger.tslib/ui/display-width.tstest/codex-manager-status-command.test.tslib/local-bridge.tslib/codex-manager.tslib/runtime-rotation-proxy.tslib/prompts/codex.tstest/recovery-storage.test.tslib/prompts/fetch-utils.tslib/config.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.tslib/recovery/storage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript escape hatchesDo not hardcode OAuth ports; use existing constants/helpers
Files:
lib/circuit-breaker.tslib/auth/org-override.tstest/ui-format.test.tstest/account-clear.test.tslib/storage/account-clear.tslib/codex-manager/account-manager-commands.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tstest/codex-manager-org-override.test.tslib/storage/flagged-storage-io.tstest/ui-theme.test.tslib/storage/storage-parser.tslib/table-formatter.tstest/oauth-server.integration.test.tslib/codex-manager/experimental-settings-prompt.tstest/flagged-storage-io.test.tstest/property/setup.test.tslib/policy/runtime-policy.tslib/storage/import-export.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/property/setup.tslib/context-overflow.tstest/context-overflow.test.tslib/storage/paths.tstest/helpers/global-sandbox.tstest/storage-parser.test.tslib/request/fetch-helpers.tslib/types.tstest/quota-readiness.test.tstest/documentation.test.tsvitest.config.tslib/ui/theme.tslib/codex-manager/commands/debug-bundle.tslib/quota-readiness.tstest/select.test.tslib/rotation.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tslib/ui/select.tslib/prompts/host-codex-prompt.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tslib/refresh-lease.tstest/logger.test.tstest/runtime-policy.test.tslib/oc-chatgpt-orchestrator.tstest/codex-routing.test.tslib/codex-manager/commands/status.tslib/refresh-queue.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tslib/accounts.tstest/refresh-lease.test.tstest/local-bridge.test.tslib/logger.tslib/ui/display-width.tstest/codex-manager-status-command.test.tslib/local-bridge.tslib/codex-manager.tslib/runtime-rotation-proxy.tslib/prompts/codex.tstest/recovery-storage.test.tslib/prompts/fetch-utils.tslib/config.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.tslib/recovery/storage.ts
**/*.{js,ts,config.*,env*}
📄 CodeRabbit inference engine (SECURITY.md)
Store credentials locally under
~/.codex/multi-auth(orCODEX_MULTI_AUTH_DIRenvironment variable)Enable debug/body logging only for short-lived troubleshooting sessions; do not leave
ENABLE_PLUGIN_REQUEST_LOGGING=1orCODEX_PLUGIN_LOG_BODIES=1enabled in production code
Files:
lib/circuit-breaker.tslib/auth/org-override.tstest/ui-format.test.tseslint.config.jstest/account-clear.test.tslib/storage/account-clear.tslib/codex-manager/account-manager-commands.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tstest/codex-manager-org-override.test.tslib/storage/flagged-storage-io.tstest/ui-theme.test.tslib/storage/storage-parser.tslib/table-formatter.tstest/oauth-server.integration.test.tslib/codex-manager/experimental-settings-prompt.tstest/flagged-storage-io.test.tstest/property/setup.test.tslib/policy/runtime-policy.tslib/storage/import-export.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/property/setup.tslib/context-overflow.tstest/context-overflow.test.tslib/storage/paths.tstest/helpers/global-sandbox.tstest/storage-parser.test.tslib/request/fetch-helpers.tslib/types.tstest/quota-readiness.test.tstest/documentation.test.tsvitest.config.tslib/ui/theme.tslib/codex-manager/commands/debug-bundle.tslib/quota-readiness.tsscripts/codex-routing.jstest/select.test.tslib/rotation.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tslib/ui/select.tslib/prompts/host-codex-prompt.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tslib/refresh-lease.tstest/logger.test.tstest/runtime-policy.test.tslib/oc-chatgpt-orchestrator.tstest/codex-routing.test.tslib/codex-manager/commands/status.tslib/refresh-queue.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tslib/accounts.tstest/refresh-lease.test.tstest/local-bridge.test.tslib/logger.tslib/ui/display-width.tstest/codex-manager-status-command.test.tslib/local-bridge.tslib/codex-manager.tslib/runtime-rotation-proxy.tslib/prompts/codex.tstest/recovery-storage.test.tslib/prompts/fetch-utils.tslib/config.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.tslib/recovery/storage.ts
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/circuit-breaker.tslib/auth/org-override.tslib/storage/account-clear.tslib/codex-manager/account-manager-commands.tslib/codex-manager/settings-hub/experimental.tslib/ui/auth-menu.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/table-formatter.tslib/codex-manager/experimental-settings-prompt.tslib/policy/runtime-policy.tslib/storage/import-export.tslib/context-overflow.tslib/storage/paths.tslib/request/fetch-helpers.tslib/types.tslib/ui/theme.tslib/codex-manager/commands/debug-bundle.tslib/quota-readiness.tslib/rotation.tslib/ui/select.tslib/prompts/host-codex-prompt.tslib/refresh-lease.tslib/oc-chatgpt-orchestrator.tslib/codex-manager/commands/status.tslib/refresh-queue.tslib/accounts.tslib/logger.tslib/ui/display-width.tslib/local-bridge.tslib/codex-manager.tslib/runtime-rotation-proxy.tslib/prompts/codex.tslib/prompts/fetch-utils.tslib/config.tslib/recovery/storage.ts
lib/auth/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Never hardcode OAuth ports; use the existing auth constants/helpers
Files:
lib/auth/org-override.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/ui-format.test.tstest/account-clear.test.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tstest/codex-manager-org-override.test.tstest/ui-theme.test.tstest/oauth-server.integration.test.tstest/flagged-storage-io.test.tstest/property/setup.test.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/context-overflow.test.tstest/storage-parser.test.tstest/quota-readiness.test.tstest/documentation.test.tstest/select.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tstest/logger.test.tstest/runtime-policy.test.tstest/codex-routing.test.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tstest/refresh-lease.test.tstest/local-bridge.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.ts
{scripts,test}/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling
Files:
test/ui-format.test.tstest/account-clear.test.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tstest/codex-manager-org-override.test.tstest/ui-theme.test.tstest/oauth-server.integration.test.tstest/flagged-storage-io.test.tstest/property/setup.test.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/property/setup.tstest/context-overflow.test.tstest/helpers/global-sandbox.tstest/storage-parser.test.tstest/quota-readiness.test.tstest/documentation.test.tsscripts/codex-routing.jstest/select.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tstest/logger.test.tstest/runtime-policy.test.tstest/codex-routing.test.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tstest/refresh-lease.test.tstest/local-bridge.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/ui-format.test.tstest/account-clear.test.tstest/global-sandbox.test.tstest/experimental-settings-prompt.test.tstest/codex-manager-org-override.test.tstest/ui-theme.test.tstest/oauth-server.integration.test.tstest/flagged-storage-io.test.tstest/property/setup.test.tstest/import-export.test.tstest/host-codex-prompt.test.tstest/table-formatter.test.tstest/rotation.test.tstest/fetch-helpers.test.tstest/property/setup.tstest/context-overflow.test.tstest/helpers/global-sandbox.tstest/storage-parser.test.tstest/quota-readiness.test.tstest/documentation.test.tstest/select.test.tstest/oc-chatgpt-orchestrator.test.tstest/package-bin.test.tstest/debug-bundle-redact.test.tstest/auth-menu-quota-bar.test.tstest/display-width.test.tstest/settings-hub-utils.test.tstest/logger.test.tstest/runtime-policy.test.tstest/codex-routing.test.tstest/prompt-fetch-utils.test.tstest/config-explain.test.tstest/refresh-lease.test.tstest/local-bridge.test.tstest/codex-manager-status-command.test.tstest/recovery-storage.test.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/ci-workflows.test.tstest/accounts.test.ts
**/*{proxy,server,config}*.{js,ts,json}
📄 CodeRabbit inference engine (SECURITY.md)
Ensure runtime rotation proxy is loopback-only, enabled by default, and authenticated with a local client key
Files:
eslint.config.jstest/oauth-server.integration.test.tsvitest.config.tstest/config-explain.test.tslib/runtime-rotation-proxy.tslib/config.tstest/plugin-config.test.tstest/runtime-rotation-proxy.test.ts
lib/storage/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/storage/**/*.ts: Worktree storage usesresolveProjectStorageIdentityRoot; never derive project pools from raw worktree paths
Never use bare recursive cleanup in Windows-sensitive paths without retry handlingDo not key project storage by worktree path; use
resolveProjectStorageIdentityRoothelper
Files:
lib/storage/account-clear.tslib/storage/flagged-storage-io.tslib/storage/storage-parser.tslib/storage/import-export.tslib/storage/paths.ts
lib/codex-manager/settings-hub/**/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Settings writes use queued retry for
EBUSY/EPERM/EAGAIN
Files:
lib/codex-manager/settings-hub/experimental.ts
**/*{auth,oauth}*.{js,ts,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Use PKCE-based OAuth flow for handling OAuth credentials
Files:
lib/ui/auth-menu.tstest/oauth-server.integration.test.tstest/auth-menu-quota-bar.test.ts
**/*{token,refresh,auth}*.{js,ts,tsx}
📄 CodeRabbit inference engine (SECURITY.md)
Implement refresh-token lifecycle management and account health isolation
Files:
lib/ui/auth-menu.tstest/oauth-server.integration.test.tstest/auth-menu-quota-bar.test.tslib/refresh-lease.tslib/refresh-queue.tstest/refresh-lease.test.ts
test/**/oauth-server.integration.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Bind OAuth server integration tests to port 1455; do not hardcode other ports
Files:
test/oauth-server.integration.test.ts
test/**/property/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Use fast-check for property-based randomized testing in property test files
Files:
test/property/setup.test.ts
test/**/rotation*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test Windows cleanup behavior and account selection logic in rotation tests
Files:
test/rotation.test.ts
lib/request/*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Node fetch returns decoded response bytes while preserving upstream
content-encoding; do not forward stale decoded encoding metadata to local clients
Files:
lib/request/fetch-helpers.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex-routing.js
package.json
📄 CodeRabbit inference engine (SECURITY.md)
Implement security overrides in
package.jsonwith documented rationale: pinhonoto4.12.18androllupto^4.59.0to avoid vulnerable dependency versions
Files:
package.json
lib/accounts*.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
Account health is 0-100 and should be updated through the account manager APIs
Files:
lib/accounts.ts
lib/runtime-rotation-proxy.ts
📄 CodeRabbit inference engine (lib/AGENTS.md)
lib/runtime-rotation-proxy.ts: Runtime rotation code must preserve pass-through semantics except for auth/provider headers that intentionally change
Runtime proxy client-facing headers must not expose account emails or tokens
Runtime rotation should fail open to normal official Codex forwarding when startup helpers are unavailable
Never add account emails/tokens to runtime proxy client responses
Files:
lib/runtime-rotation-proxy.ts
lib/prompts/codex.ts
📄 CodeRabbit inference engine (AGENTS.md)
Prompt templates must sync from Codex CLI GitHub releases with ETag caching
Files:
lib/prompts/codex.ts
test/**/codex-manager-cli.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test CLI settings with question cancellation across all 5 panels and EBUSY/concurrent race conditions
Files:
test/codex-manager-cli.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:45.671Z
Learning: Do not edit `dist/` or local temp/cache directories in version control
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:52.854Z
Learning: Before release and after dependency changes, run: `npm run audit:ci`, `npm run lint`, `npm run typecheck`, `npm test`, and `npm run build`
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:52.854Z
Learning: Do not share `~/.codex/` directories containing credentials and sensitive data
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:52.854Z
Learning: Review connected apps in ChatGPT settings periodically
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:52.854Z
Learning: For vulnerability reports, contact the maintainer privately via GitHub profile contact channel instead of opening a public issue
Learnt from: CR
Repo: ndycode/codex-multi-auth
Timestamp: 2026-06-02T03:52:52.854Z
Learning: Prepare fixes before public disclosure of vulnerabilities and coordinate disclosure timing to reduce user risk
🔇 Additional comments (35)
lib/accounts.ts (1)
1462-1492: LGTM!lib/circuit-breaker.ts (1)
198-205: LGTM!lib/policy/runtime-policy.ts (1)
119-125: LGTM!Also applies to: 200-210
lib/rotation.ts (1)
182-197: LGTM!Also applies to: 354-369
test/accounts.test.ts (1)
19-19: LGTM!Also applies to: 1047-1195
test/runtime-rotation-proxy.test.ts (1)
327-421: LGTM!Also applies to: 459-482
lib/auth/org-override.ts (1)
1-25: LGTM!lib/codex-manager.ts (1)
22-22: LGTM!Also applies to: 41-41, 1261-1275, 2775-2781, 3164-3164, 3546-3546
lib/codex-manager/account-manager-commands.ts (1)
1-42: LGTM!lib/codex-manager/commands/status.ts (1)
50-51: LGTM!Also applies to: 69-99, 139-160, 188-199, 216-264, 324-332
lib/quota-readiness.ts (1)
6-6: LGTM!Also applies to: 80-80, 85-98, 104-112
test/codex-manager-status-command.test.ts (1)
8-8: LGTM!Also applies to: 352-445
test/codex-routing.test.ts (1)
24-48: LGTM!test/quota-readiness.test.ts (1)
58-167: LGTM!lib/ui/select.ts (1)
77-115: 🏗️ Heavy liftgrapheme-safe truncation needed in truncateansi
lib/ui/select.ts:89-107 budgets and advances one code point at a time, which can split zwj emoji/flag grapheme clusters (e.g.,👨👩👧👦) and render a broken glyph even though the full-stringdisplayWidth(...)precheck was computed on the intact label. update the truncation loop to segment visible text by grapheme cluster before applying the display-width budget, and add a regression case in test/select.test.ts for a zwj emoji (and/or a flag) truncation scenario.lib/storage/paths.ts (1)
403-442: LGTM!Also applies to: 481-529
lib/storage/storage-parser.ts (1)
55-60: LGTM!scripts/codex-routing.js (1)
6-8: LGTM!Also applies to: 18-18
test/display-width.test.ts (1)
1-109: LGTM!test/package-bin.test.ts (1)
21-65: LGTM!test/paths.test.ts (1)
801-911: LGTM!test/codex-manager-org-override.test.ts (1)
1-1: drop the explicit vitest import.same issue as the earlier thread:
test/codex-manager-org-override.test.ts:1still importsafterEachinstead of using the enabled vitest globals.test/config-explain.test.ts (1)
211-331: LGTM!test/debug-bundle-redact.test.ts (2)
17-145: LGTM!
1-1: ⚡ Quick winclarify vitest global import rule for lifecycle helpers
test/debug-bundle-redact.test.ts:1imports{ afterEach, beforeEach, vi }from "vitest". the stated guideline only calls out avoiding explicit imports fordescribe/it/expect, so this import may be allowed;test/codex-manager-org-override.test.ts:1also importsafterEach. check eslint/vitest config to confirm whetherafterEach/beforeEach/vimust be used as globals too.test/ui-format.test.ts (1)
20-22: LGTM!test/documentation.test.ts (1)
600-606: ⚡ Quick winkeep the caret synthesis for rollup docs parity check
test/documentation.test.ts:600-606should not drop the^—package.jsonusesoverrides.rollup = 4.59.0whileSECURITY.mddocuments it as pinned to^4.59.0, so asserting ``pinned to `^${String(rollupPin)}``` is consistent.> Likely an incorrect or invalid review comment.test/experimental-settings-prompt.test.ts (1)
110-185: LGTM!test/helpers/global-sandbox.ts (1)
1-30: LGTM!test/plugin-config.test.ts (1)
256-318: LGTM!test/property/setup.test.ts (1)
6-22: LGTM!test/recovery-storage.test.ts (1)
157-330: LGTM!Also applies to: 346-376, 420-461, 969-999
test/runtime-policy.test.ts (1)
11-12: LGTM!Also applies to: 84-111, 248-309
test/table-formatter.test.ts (1)
65-95: LGTM!test/ui-theme.test.ts (1)
2-2: LGTM!Also applies to: 5-9, 20-20, 26-26, 36-36, 55-86
Code: - config.ts: resolvePluginConfigPath() treated a set-but-non-existent CODEX_MULTI_AUTH_CONFIG_PATH as the path unconditionally, so loadPluginConfig() threw ENOENT in the fallback and collapsed to defaults — masking a real legacy config on disk. Treat a missing env override as absent. (savePluginConfig reads the env var directly, so save-to-new-env-path is unaffected.) - prompts/codex.ts: a legacy cache entry with NO sha256 was trusted (served as-is, and a 304 minted a digest over un-vetted bytes). Treat missing-sha as unverified: don't fast-path serve, don't send/accept conditional revalidation (require a prior sha to trust a 304); force a full 200 to mint the first digest, keeping old bytes only as an offline fallback. - recovery/storage.ts: validators now reject a path-unsafe string id (SAFE_ID_PATTERN) and a non-numeric time.created — both previously survived and caused a path-traversal read / a NaN sort that mis-ordered recovery. - runtime-rotation-proxy.ts: removed the allowNonLoopbackHost escape hatch. The proxy forwards managed OAuth tokens and is now loopback-only with no opt-out. - storage/import-export.ts: the staged-export temp cleanup was single-shot; route it through the shared retry so a transient Windows lock can't strand a secret-bearing .tmp next to the destination. - ui/display-width.ts: emoji-presentation clusters (U+FE0F) and keycaps (U+20E3) now count as width 2 (☀️ ❤️ 1️⃣), so table/menu alignment is correct. Tests: config legacy-fallback; no-sha cache forces full GET / offline fallback / no If-None-Match (and repaired a stale-while-revalidate test that relied on the old no-sha trust); unsafe-id + non-numeric-time quarantine; non-loopback proxy bind refused unconditionally; export temp-cleanup retry; emoji-presentation + keycap widths. Test-contract fixes: vitest-globals + tty-descriptor restore in auth-menu-quota-bar; x-api-key strip on the auth-enabled bridge path; real temp+rename atomicity assertion; numeric/string key-normalization coverage; storage-parser string (not Buffer) reads; Accept-header assertion; schema-derived guardian step; IPv6 teardown port probe. typecheck + lint clean; full suite 4236 passed / 1 skipped.
ndycode
added a commit
that referenced
this pull request
Jun 2, 2026
- Bump vitest / @vitest/ui / @vitest/coverage-v8 to ^4.1.8 (dev-only), clearing GHSA-5xrq-8626-4rwp so `npm run audit:ci` exits 0. audit:prod was already clean (nothing test-related ships); this unblocks the CI release gate. - Version → 2.1.13-beta.3. - Add docs/releases/v2.1.13-beta.3.md covering the Phase 1 audit (#499), the mcodex launcher + statusline (#500), and the pre-release hardening (#503). Update the docs portal + root README prerelease pointers (documentation.test.ts parity). typecheck + lint + audit:ci clean; full suite 4246 passed / 1 skipped on vitest 4.1.8; documentation parity 25/25.
ndycode
added a commit
that referenced
this pull request
Jun 2, 2026
…#503) * fix(security,resilience): pre-release hardening from deep stress test Found by a whole-tree adversarial audit + runtime fuzz of the merged main (both #499 and #500). The tree was already well-hardened — these are the residual MEDIUM/LOW gaps, all verified. Egress hardening (never forward inbound client credentials upstream): - runtime-rotation-proxy.ts + local-bridge.ts: strip inbound `cookie` and `proxy-authorization` on the outbound request, alongside the existing authorization/x-api-key stripping. A client cookie would otherwise ride upstream with the managed OAuth Bearer. - runtime-rotation-proxy.ts readErrorBody: the outbound fetch's abort timer is cleared once headers arrive, so a stalled ERROR body (429/5xx path) could hang the handler forever — the success path is per-chunk stall-bounded, the error path was not. Read it via a reader with a per-read idle timeout + a 1MB cap, cancelling the stream on stall/overflow. Windows fs resilience (close the transient-lock gaps the fuzz reproduced): - storage.ts: the secret-bearing WAL write and temp write had NO retry, and the temp cleanup was single-shot — a transient EBUSY/EPERM could fail a valid save or strand a token-bearing *.tmp. Wrap all three in withFileOperationRetry. Also align copyFileWithRetry / renameFileWithRetry to the shared shouldRetryFileOperation taxonomy (adds ENOTEMPTY/EACCES/EAGAIN). The primary commit rename keeps its existing EPERM/EBUSY contract unchanged. - quota-cache.ts: widen RETRYABLE_FS_CODES to the shared set. Least-privilege: - runtime/runtime-observability.ts: persist the snapshot (account id/label) with mode 0o600 and the dir 0o700, matching the other sensitive writers (no-op on win32, owner-only on POSIX). Tests: cookie strip on both egress paths; existing storage WAL/temp/backup retry coverage still green (rename contract unchanged). Also cleaned a stale eslint-disable + unused import in the merged mcodex statusline test. typecheck + lint clean; full suite 4246 passed / 1 skipped. * chore(release): cut v2.1.13-beta.3 + bump vitest to ^4.1.8 - Bump vitest / @vitest/ui / @vitest/coverage-v8 to ^4.1.8 (dev-only), clearing GHSA-5xrq-8626-4rwp so `npm run audit:ci` exits 0. audit:prod was already clean (nothing test-related ships); this unblocks the CI release gate. - Version → 2.1.13-beta.3. - Add docs/releases/v2.1.13-beta.3.md covering the Phase 1 audit (#499), the mcodex launcher + statusline (#500), and the pre-release hardening (#503). Update the docs portal + root README prerelease pointers (documentation.test.ts parity). typecheck + lint + audit:ci clean; full suite 4246 passed / 1 skipped on vitest 4.1.8; documentation parity 25/25. * fix: address #503 review (dir-mode + retry/strip coverage + version pins) - runtime/runtime-observability.ts: re-assert dir 0o700 via fs.chmod after mkdir — mkdir's mode is a no-op on a pre-existing multi-auth dir, so the upgrade path kept old (possibly permissive) perms. POSIX-only, best-effort. - Bump .codex-plugin/plugin.json + AGENTS.md to 2.1.13-beta.3 (plugin-manifest test asserts manifest version == package version; the beta.3 bump left them stale). Tests: - local-bridge + runtime-rotation-proxy: assert inbound `proxy-authorization` (alongside cookie) is stripped on the outbound request, so a refactor dropping the delete is caught. - quota-cache: read-retry (EACCES) + rename-retry (ENOTEMPTY) + last-write-wins queue-ordering regressions, pinning the widened retry taxonomy. - runtime-observability-dir-mode (new, POSIX-only): pre-create CODEX_MULTI_AUTH_DIR world-writable, persist a snapshot, assert the dir becomes owner-only — proves the chmod-after-mkdir hardening on the upgrade path. - runtime-observability test fs-mock now exposes chmod. typecheck + lint + audit:ci clean; full suite 4249 passed / 2 skipped. * fix: address #503 round-2 review (chmod error handling + win32/strip coverage) - runtime/runtime-observability.ts: stop swallowing every chmod failure. Only ENOENT (dir removed by a concurrent process) is ignored; any other failure is surfaced rather than silently leaving a world-readable dir holding account ids/labels. - runtime-observability.test.ts: add a win32 case asserting the snapshot persists WITHOUT calling chmod (POSIX-only branch), and reset the chmod mock per test. - local-bridge.test.ts: extend the auth-enabled runtime-proxy path test to assert cookie + proxy-authorization are stripped too (parity with the no-key path). typecheck + lint clean; full suite 4250 passed / 2 skipped. * test(observability): assert snapshot temp file is written 0o600 Addresses the #503 round-3 review: the win32 persistence test now also asserts writeFile received mode 0o600, so an owner-only-perms regression on the snapshot file fails the suite.
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
Remediation of the deep audit of
codex-multi-auth, landed as 11 focused, individually-verified commits across the audit's phased roadmap. Each commit is scoped to one finding (or a tight cluster), carries its own regression tests, and was gated ontsc+eslint+ the full vitest suite.Net: 56 files, +1642/−201. Full suite grows from 4062 → 4104 tests, all green.
What changed, by category
Security / correctness (Phase 1)
0600under a0700dir (was world-readable umask)0600sst/opencodeprompt URLs (the rebrand-artifact URLs 404'd and re-fetched every request)config explainsurfaces 3 previously-missing live settings + a parity guard test@codex-ai/sdkmoved to deps + bundleDependencies so a consumer'stsccan resolve the published.d.tstypes (verified bundled in the tarball)status.lastErrorruntimeBaseUrlObservability + resilience (Phase 2)
AsyncLocalStoragecorrelation IDs (concurrency-safe, replacing a process-global), structured proxy logging, per-request trace id distinct from sessionKeyresolvePathcanonicalizes via realpath and re-checks containment, rejecting symlink-escapes (TOCTOU)removeAccountclears identity-keyed health/token/circuit state so a re-added identity starts freshPrompt-fetch hardening (Phase 2c)
fetch-utils: AbortSignal timeout, size cap (Content-Length + streamed), empty-body rejection, mandatoryUser-Agent/AcceptCleanup, conventions, features (Phase 2d / 3)
NO_COLOR/FORCE_COLOR/ non-TTY for color outputroutingMutexflag now actually controls behavior (legacy mode unchanged by default)fs-retry.tsset (closes the missingENOTEMPTY/EACCESgap).corrupt-<ts>) + exposegetRecoveryCorruptionStats()instead of silently dropping themTesting
npm testgreen at every commit (271 files / 4104 tests, 1 POSIX-only skip on Windows)tsc --noEmitandeslintclean throughout (husky lint-staged ran on each commit)Known issue surfaced (not introduced here):
tests-ci-16The suite has a pre-existing, environment-level intermittent vitest worker crash on Windows — exit 1 with no test failure and no summary line. Controlled measurement (8 runs each) showed it reproduces on pristine
origin/mainat the same rate (~2/8) as on this branch, so it is independent of these changes. A re-run always passes. Worth a follow-up to harden the harness (e.g. vitest pool/forkssettings); flagged here for visibility.Deferred (documented, not done)
request-10globalno-consolelint rule — conflicts with ~226 intentional CLI/UI output sites; would need a broad refactor🤖 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
this pr lands the three-phase audit remediation for
codex-multi-auth: security hardening (file permissions, proxy egress guards, token fingerprinting, symlink-escape detection), observability (als correlation ids, structured proxy logging, per-request trace ids), and resilience (prompt-fetch hardening with timeout/size/integrity, atomic cache writes, recovery file quarantine, config fs-retry).0600under0700dirs, atomic secret-bearing file writes, proxy loopback-only bind enforcement, context-overflow response dialect fixed from anthropic messages sse to responses api sse, budget evaluation covers archived ledger rows, symlink-escape detection via realpath canonicalization inresolvePath.AsyncLocalStorage-based correlation ids replacing a process-global (concurrency-safe for the proxy),readBodyTextGuardedwith per-chunk idle timeouts + size cap + sha-256 cache integrity + atomic temp-rename writes,removeAccountnow clears identity-keyed health/token/circuit state, recovery corrupt-file quarantine withgetRecoveryCorruptionStats().NO_COLOR/FORCE_COLOR/non-tty color handling, routing mutex wired into the proxy's async select→commit path, three divergent fs-retry code sets unified, config load retries transient fs locks, 42 new regression tests.Confidence Score: 5/5
safe to merge — all security fixes are correctly scoped and the full suite is green
the security-critical changes (file permission hardening, proxy egress guards, ALS correlation ids, symlink-escape detection) are well-implemented, individually tested, and consistent with the existing codebase patterns. the two observations raised are both non-blocking quality nits. no regressions detected across the behavioral changes.
no files require special attention; the two commented files (lib/config.ts retry delay, lib/recovery/storage.ts docstring) have self-contained, low-impact issues
Important Files Changed
canonicalizeExistingPrefix+storage-02realpath check toresolvePath; logic mirrors the pre-existing lexical containment guard, correctly scoped to only fire whencanonical !== resolvedreadBodyTextGuardedproperly releases the reader on error/timeoutcurrentCorrelationIdwithAsyncLocalStorage;clearCorrelationIdnow correctly setsstore.id = nullinside ALS context;logToConsoleadds CR/LF stripping matchinglogToAppstripThinkingPartsnow returns false when a targeted part cannot be removed (recovery-05),validatePathIdapplied to write pathsloadPluginConfigprefers env path over unified,readFileSyncWithConfigRetryadds bounded sync retry,resolveStoredPluginConfigRecordmirrors load precedence forconfig explain, three previously-missing settings added to explain entriesrunWithCorrelationId, structured proxyLog, pid-offset and routing-mutex threaded into proxy pathwriteCacheAtomically,fetchWithTimeoutandwithBodyTimeoutapplied to GitHub API and HTML release paths0o700mode + best-effort chmod for pre-existing dirs (auth-01), lock file opened at0o600, result payload written at0o600via temp+renameremoveAccountclears health/token tracker entries for both the stable tracker key and recomputed identity key, then removes the circuit breaker by circuitKeyIdquotaWindowIsExhaustedsynthesizes a conservative rollover expiry whenresetAtMsis missing butupdatedAt+windowMinutesare present, preventing the "exhausted forever" trap after rotationSequence Diagram
sequenceDiagram participant Client participant LocalBridge participant RuntimeProxy participant AccountPool Client->>LocalBridge: request (Bearer client-token) LocalBridge->>LocalBridge: verifyBearerToken (inbound auth) LocalBridge->>LocalBridge: strip x-api-key, replace Authorization with runtimeClientApiKey LocalBridge->>LocalBridge: isLoopbackHost(runtimeBaseUrl.hostname) guard LocalBridge->>RuntimeProxy: forwarded request (runtime client key) RuntimeProxy->>RuntimeProxy: isLoopbackHost(bindHost) guard (non-loopback refused) RuntimeProxy->>RuntimeProxy: runWithCorrelationId(traceId) RuntimeProxy->>AccountPool: select account (pid-offset, routingMutex) AccountPool-->>RuntimeProxy: account + token RuntimeProxy->>RuntimeProxy: inject OAuth token, strip client headers RuntimeProxy-->>Client: proxied responseComments Outside Diff (2)
lib/prompts/codex.ts, line 329-348 (link)writeCacheAtomicallyhas a two-rename non-atomic windowthe function renames
contentTmp → cacheFileand thenmetaTmp → cacheMetaFileas two separate syscalls. if the second rename fails, the disk now has new content but the old meta (old sha256 from the previous tag). the next read will detect the sha256 mismatch and trigger a refetch — self-healing, but the function name implies stronger atomicity than it delivers. on windows this window is wider becausefs.renamecan briefly fail withEPERM/EBUSY. worth either noting the two-phase nature in the doc comment or adding a retry on the meta rename, and flagging a missing vitest coverage path for this partial-success scenario.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
lib/local-bridge.ts, line 47-50 (link)isLoopbackHostinlocal-bridge.tschecks for"::1"(without brackets), butnew URL("http://[::1]:3000/").hostnamereturns"[::1]"(WITH brackets, per the WHATWG URL spec). the new egress guard callsnew URL(runtimeBaseUrl).hostnameand passes the result straight toisLoopbackHost, so any IPv6 loopback runtimeBaseUrl will fail the guard and throw"Local bridge refuses to forward to non-loopback runtimeBaseUrl host [::1]".lib/runtime-rotation-proxy.tsavoids this by explicitly including"[::1]"in its equivalent function — the same fix is needed here.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (13): Last reviewed commit: "fix: resolve round-9 CodeRabbit findings..." | Re-trigger Greptile