feat: add mcodex launcher statusline#500
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughthis pr registers Changescodex forward status and mcodex entrypoint
Sequence Diagram(s)sequenceDiagram
participant main as lib/scripts/codex.js:main
participant lockdir as quota_lock_dir
participant child as codex_forecast_child
main->>lockdir: check newest updatedAt -> decide refresh (lib/scripts/codex.js:580-683)
main->>lockdir: acquire lock with stale eviction (lib/scripts/codex.js:580-683)
main->>child: spawn detached forecast child (QUIET env) (lib/scripts/codex.js:580-683)
child->>lockdir: write/update quota-cache files (forecast child)
child->>main: exit -> triggers async lock cleanup (lib/scripts/codex.js:170-191)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes notes (lowercase, factual):
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/codex.js`:
- Around line 557-592: The lock cleanup can be missed when the parent exits
immediately because maybeRefreshQuotaCacheInBackground spawns a detached child
(spawn options detached:true + child.unref()) and relies on parent-side
child.once('close'/'error') handlers to call removeDirectoryWithRetry(lockPath);
instead make the child process take explicit responsibility for lock cleanup by
passing the lockPath to the child (e.g., add CODEX_MULTI_AUTH_LOCK or similar to
the env passed into spawn) and update codex-multi-auth.js to call
removeDirectoryWithRetry(process.env.CODEX_MULTI_AUTH_LOCK) on startup and on
exit/uncaught errors (wrap in try/catch); keep the parent handlers as
best-effort fallback.
In `@scripts/mcodex`:
- Around line 17-24: The run_monitor function currently ignores any positional
parameters but the call if [[ "${1:-}" == "--monitor" ]]; then ... run_monitor
"$@" passes them, so either remove the unused "$@" from the call or make
run_monitor forward arguments to the watch command; fix by editing the
invocation in the conditional to call run_monitor without "$@" (or alternatively
modify run_monitor to accept "$@" and use them in the watched command, e.g.,
pass through to codex-multi-auth list) and ensure the symbol run_monitor and the
watch invocation around monitor_interval/codex-multi-auth list are updated
accordingly.
- Around line 7-15: The quote_args function currently appends a trailing space
via printf '%s ' "${quoted[@]}", which can inject an extra empty token when
interpolated into the tmux command; change quote_args to output the quoted
arguments without a trailing space (for example, print them joined without a
trailing delimiter or print each with a space except after the last) so that the
output from quote_args (used at lib/scripts/mcodex:52) has no trailing
whitespace; update the function name quote_args and its loop logic accordingly
to ensure no trailing space is emitted.
- Around line 51-52: The tmux invocation wraps the pre-quoted args ($args
produced by quote_args) inside a double-quoted command which can break when
arguments contain quotes or special chars; update the startup to avoid embedding
a pre-quoted command string into tmux. Instead either (a) create the session
without quoting the whole command and pass raw argv (use codex-multi-auth-codex
and the original "$@" or export a temporary env var and call
codex-multi-auth-codex reading that), or (b) create the session and then use
tmux send-keys to deliver a properly quoted/escaped command to the window (use
tmux send-keys with the literal arguments) so codex-multi-auth-codex receives
arguments safely; modify the code around args, quote_args, and the tmux
new-session call accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2b78963-527f-4673-b5f9-978fa6c44539
📒 Files selected for processing (3)
package.jsonscripts/codex.jsscripts/mcodex
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (5)
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pin hono to version 4.12.14 or higher to avoid authentication bypass vulnerability in GHSA-xh87-mx6m-69f3
Pin rollup to version ^4.59.0 or higher to avoid vulnerable versions <4.59.0 in Vite and Vitest transitive dependencies
Files:
package.json
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
scripts/codex.js
**/scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex.js
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
scripts/codex.js
**/scripts/codex.js
📄 CodeRabbit inference engine (AGENTS.md)
The package does not publish a global
codexbin;codex-multi-auth-codexis the explicit wrapper
Files:
scripts/codex.js
🔇 Additional comments (30)
package.json (1)
117-117: LGTM!Also applies to: 132-132
scripts/mcodex (1)
38-43: LGTM!Also applies to: 61-61
scripts/codex.js (28)
84-86: LGTM!
182-189: LGTM!
191-207: LGTM!
209-213: LGTM!
215-235: LGTM!
237-245: LGTM!
247-293: LGTM!
295-302: LGTM!
304-312: LGTM!
314-326: LGTM!
328-349: LGTM!
351-358: LGTM!
360-366: LGTM!
368-376: LGTM!
378-385: LGTM!
387-394: LGTM!
396-404: LGTM!
406-418: LGTM!
420-430: LGTM!
432-437: LGTM!
439-444: LGTM!
446-455: LGTM!
457-487: LGTM!
489-494: LGTM!
496-502: LGTM!
504-524: LGTM!
526-555: LGTM!
4680-4681: LGTM!
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)
scripts/mcodex (2)
60-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOCTOU race in session name generation.
lib/scripts/mcodex:60 checks if session exists, then lib/scripts/mcodex:61 appends timestamp suffix. if two
mcodexprocesses start in the same second, both get identical$suffixfromdate +%H%M%S, leading to a session name collision when the second process tries to create the session at lib/scripts/mcodex:65.set -ewill cause the script to exit with tmux's error, but this is a poor user experience.consider appending
$$(process id) to the suffix for uniqueness.🔒 append pid to suffix
session="${MCODEX_TMUX_SESSION:-mcodex}" - suffix="$(date +%H%M%S)" + suffix="$(date +%H%M%S)-$$" if tmux has-session -t "$session" 2>/dev/null; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mcodex` around lines 60 - 62, The session name generation has a TOCTOU race because suffix is just date +%H%M%S and two processes can pick the same value; change the suffix generation to include the current PID (append $$) so suffix becomes time+pid and then use that suffix when setting session (referenced by the variables suffix and session and the tmux check using tmux has-session); ensure the code still checks tmux has-session -t "$session" and then sets session="${session}-${suffix}" with the new PID-enhanced suffix to guarantee uniqueness.
1-76: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winno test coverage for new
mcodexbash entrypoint.lib/scripts/mcodex adds 76 lines of bash logic (monitor mode, tmux session management, live-accounts split, quote_args, configure_tmux_scrollback) with zero test coverage. regression tests should validate:
--monitorinvokes watch with correct interval--tmuxcreates session and attaches when tmux available--tmux --live-accountssplits pane- session name collision handling (suffix generation)
- fallback to direct exec when tmux missing or no flags provided
- configure_tmux_scrollback sets mouse and history-limit
without tests, refactors or env changes can silently break these modes.
do you want me to generate a test scaffold using bats or shunit2, or open a new issue to track test coverage for lib/scripts/mcodex?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mcodex` around lines 1 - 76, Add regression tests covering the new mcodex bash entrypoint: exercise the script with --monitor to verify run_monitor invokes watch with $monitor_interval, with --tmux to verify a tmux session is created/attached and configure_tmux_scrollback sets mouse and history-limit, with --tmux --live-accounts to verify live_accounts triggers a split-window, a test for session name collision to validate suffix generation, and fallbacks when tmux is missing or no flags provided to ensure exec codex-multi-auth-codex runs; implement these as a test scaffold (bats or shunit2) that stubs/mocks command -v tmux, tmux subcommands (has-session, new-session, split-window, set-option, attach-session), watch, and inspects environment variables monitor_interval, tmux_history_limit, session and suffix to assert correct calls and options, and add the tests to CI so future refactors of run_monitor, quote_args, configure_tmux_scrollback, and the top-level flag handling are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mcodex`:
- Line 53: The tmux commands embed the monitor_interval variable unquoted,
allowing shell injection; update both tmux split-window invocations (the lines
building the watch command that reference $monitor_interval) to pass the
interval as a quoted argument, e.g. invoke tmux split-window with watch -n
"$monitor_interval" so the value is treated as a single safe argument (apply the
same quoting fix to the second occurrence around $monitor_interval).
- Around line 22-31: The tmux calls in configure_tmux_scrollback currently
redirect only stdout (tmux set-option ... >/dev/null) so errors still print;
update the two tmux set-option invocations (inside configure_tmux_scrollback
using target_args and tmux_history_limit) to consistently redirect both stdout
and stderr (e.g., add 2>&1 to the redirection or use >/dev/null 2>&1) or remove
redirection entirely so successful and failing output are treated consistently.
---
Outside diff comments:
In `@scripts/mcodex`:
- Around line 60-62: The session name generation has a TOCTOU race because
suffix is just date +%H%M%S and two processes can pick the same value; change
the suffix generation to include the current PID (append $$) so suffix becomes
time+pid and then use that suffix when setting session (referenced by the
variables suffix and session and the tmux check using tmux has-session); ensure
the code still checks tmux has-session -t "$session" and then sets
session="${session}-${suffix}" with the new PID-enhanced suffix to guarantee
uniqueness.
- Around line 1-76: Add regression tests covering the new mcodex bash
entrypoint: exercise the script with --monitor to verify run_monitor invokes
watch with $monitor_interval, with --tmux to verify a tmux session is
created/attached and configure_tmux_scrollback sets mouse and history-limit,
with --tmux --live-accounts to verify live_accounts triggers a split-window, a
test for session name collision to validate suffix generation, and fallbacks
when tmux is missing or no flags provided to ensure exec codex-multi-auth-codex
runs; implement these as a test scaffold (bats or shunit2) that stubs/mocks
command -v tmux, tmux subcommands (has-session, new-session, split-window,
set-option, attach-session), watch, and inspects environment variables
monitor_interval, tmux_history_limit, session and suffix to assert correct calls
and options, and add the tests to CI so future refactors of run_monitor,
quote_args, configure_tmux_scrollback, and the top-level flag handling are
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f8ed299-86d1-444b-a73e-b7e9c3d2c14c
📒 Files selected for processing (1)
scripts/mcodex
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (5)
scripts/mcodex (5)
8-16: trailing space inquote_argsoutput may cause empty arg injection.
printf '%s ' "${quoted[@]}"at lib/scripts/mcodex:15 appends a trailing space after the last argument. when interpolated into the tmux command at lib/scripts/mcodex:65, the trailing space becomes part of the command string.
18-20:run_monitorignores the"$@"passed at lib/scripts/mcodex:35.lib/scripts/mcodex:35 passes remaining args to
run_monitor, but the function doesn't use any parameters.
64-65: tmux command string may mishandle arguments with embedded quotes or special chars.lib/scripts/mcodex:64-65 pre-quotes
$argsviaquote_args, then embeds the result in double quotes for tmux. this double-quoting can break for arguments containing quotes.
1-6: LGTM!
75-75: LGTM!
Security: - scripts/mcodex: validate MCODEX_MONITOR_INTERVAL (and MCODEX_TMUX_HISTORY_LIMIT) as numeric before they are interpolated into the `watch -n <n> ...` / tmux option strings that tmux hands to a shell. A value like "5; rm -rf ~" or "$(cmd)" was previously executed verbatim; it is now rejected and replaced with the safe default. Closes the command-injection findings on lines 53/68. Correctness: - scripts/codex.js maybeRefreshQuotaCacheInBackground: the status-refresh child's close/error handlers called the Atomics.wait-backed removeDirectoryWithRetry, blocking the PARENT event loop up to ~200ms on a transient Windows lock while Codex was running. Added removeDirectoryBestEffortAsync (fsPromises.rm with internal retry, no blocking sleep) and use it in those callbacks. - scripts/codex.js formatStatusPath: the `~` abbreviation used a hardcoded "/" separator, so on Windows (backslash paths from resolvePath) the cwd was never abbreviated. Use path.sep for the boundary check and normalize the remainder to forward slashes for a stable status line. mcodex polish: - quote_args no longer emits a trailing space (avoids an empty trailing token in the interpolated tmux command). - the tmux new-session command is built in a variable from printf %q-escaped args so arguments with embedded quotes/spaces are passed safely. - --monitor no longer forwards unused "$@"; configure_tmux_scrollback redirects stderr too for consistent silence. Docs / known-race notes: - documented the bounded TOCTOU in the stale-lock recovery (a slow-but-alive refresh owner can be evicted, briefly yielding two idempotent refresh children) and the detached-child caveat (close/error cleanup may not fire if the parent exits first; 10-minute stale-lock recovery reclaims the lock). Tests: - test/documentation.test.ts: add `mcodex` to the canonical bin surface the PR introduced (the assertion was left stale by the PR and was failing). - test/mcodex-launcher.test.ts (new): regression for the interval-injection guard — valid integer/fractional pass through; injection payloads and shell substitutions collapse to the safe default; asserts the shipped script still contains the numeric guard. Skips where bash is unavailable. typecheck + lint clean; full suite 4067 passed.
Review fixes —
|
| Finding | Fix |
|---|---|
scripts/mcodex:53,68 — $monitor_interval command injection (P1 / Major) |
MCODEX_MONITOR_INTERVAL (and MCODEX_TMUX_HISTORY_LIMIT) are now validated as numeric before interpolation into the watch -n … / tmux-option strings. 5; rm -rf ~, $(cmd), backticks → rejected, replaced with the safe default. |
Correctness
| Finding | Fix |
|---|---|
scripts/codex.js:591 — blocking sleepSync in child close/error callbacks (P1) |
Those handlers fire on the parent event loop while Codex runs; the Atomics.wait backoff could freeze it ~200ms on a Windows lock. Added removeDirectoryBestEffortAsync (fsPromises.rm w/ internal retry, no blocking sleep) for the callback path. |
scripts/codex.js:395 — formatStatusPath hardcoded /, breaks ~ on Windows (P2) |
Uses path.sep for the boundary check; remainder normalized to forward slashes for a stable status line. |
mcodex polish
quote_argsdrops the trailing space (Refactor: complete codex-only migration and repair build/test #6); tmuxnew-sessioncommand built from%q-escaped args so embedded quotes/spaces are safe (Refactor CLI settings hub and make Q cancel without save #8);--monitorno longer forwards unused"$@"(chore: repo hygiene cleanup + test quality/speed uplift #7);configure_tmux_scrollbackredirects stderr too (docs: comprehensive precision documentation overhaul #9).
Known-race docs (#3, #5)
- Documented the bounded stale-lock TOCTOU (a slow-but-alive refresh owner can be evicted → two idempotent refresh children) and the detached-child caveat (cleanup may not fire if the parent exits first; 10-min stale-lock recovery reclaims it).
Tests
test/documentation.test.ts: the PR added themcodexbin but left the canonical-bin-surface assertion stale — it was failing on the PR head. Addedmcodexto the expected map.test/mcodex-launcher.test.ts(new): regression for the interval-injection guard — valid integer/fractional pass through; injection payloads + shell substitutions collapse to the default; asserts the shipped script still carries the numeric guard. Skips where bash is unavailable.
Verification: typecheck clean, lint clean, full suite 4067 passed.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/documentation.test.ts (1)
538-586:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winalso assert that
scripts/mcodexis published.
test/documentation.test.ts:581-586locks the newbinentry, but the packaging contract also depends onpackage.json filesincludingscripts/mcodex. without that assertion, npm can publish a brokenmcodexshim while this test still passes.proposed fix
const packageJson = JSON.parse(read("package.json")) as { name?: unknown; description?: unknown; keywords?: unknown; author?: unknown; license?: unknown; repository?: { url?: unknown } | unknown; homepage?: unknown; bugs?: { url?: unknown } | unknown; bin?: Record<string, unknown> | unknown; + files?: unknown; }; @@ expect(packageJson.bin).toEqual({ mcodex: "scripts/mcodex", "codex-multi-auth-app-launcher": "scripts/codex-app-launcher.js", "codex-multi-auth-codex": "scripts/codex.js", "codex-multi-auth": "scripts/codex-multi-auth.js", }); + expect(packageJson.files).toEqual( + expect.arrayContaining(["scripts/mcodex"]), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/documentation.test.ts` around lines 538 - 586, The test currently asserts packageJson.bin entries but doesn't ensure the uploaded package actually includes the mcodex script; update the "keeps package metadata aligned with the canonical owner surface" test to also assert that packageJson.files (or packageJson.files as an array) contains "scripts/mcodex" — locate the test using the packageJson variable and add an expectation like expect(packageJson.files).toEqual(expect.arrayContaining(["scripts/mcodex"])); to guarantee the mcodex shim is published.scripts/mcodex (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick wingate monitor modes on
watchavailability.
scripts/mcodex:38-40,scripts/mcodex:78, andscripts/mcodex:101invokewatchunconditionally.--monitorand--live-accountsshould fail fast with a clear error whenwatchisn’t installed (there’s atmuxcheck, but no equivalentwatchguard:scripts/mcodex:70-73).missing regression tests:
test/mcodex-launcher.test.ts:6-12covers interval injection hardening, not the missing-watchdependency failure path (test/mcodex-launcher.test.ts:41-67).windows edge cases: tests skip when bash is unavailable (
test/mcodex-launcher.test.ts:12,17-20), but git-bash/msys users still need this runtime guard.concurrency risk: every live pane spawns its own
watchprocess; failing fast prevents leaving broken panes/processes around during concurrent launches.proposed fix
+ensure_watch() { + if ! command -v watch >/dev/null 2>&1; then + echo "mcodex: watch is required for --monitor and --live-accounts" >&2 + exit 1 + fi +} + run_monitor() { + ensure_watch watch -n "$monitor_interval" 'codex-multi-auth list' } @@ if [[ "${1:-}" == "--live-accounts" ]]; then live_accounts=1 shift + ensure_watch fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mcodex` around lines 38 - 40, The script calls watch unconditionally in run_monitor() (and at other spots for --monitor and --live-accounts) so add a runtime guard that checks for watch availability (e.g., command -v watch >/dev/null) before spawning it; implement a small helper (check_watch_available or ensure_watch) and call it from run_monitor and wherever --monitor/--live-accounts create panes so the script exits with a clear error message if watch is missing; update the launcher paths that currently only check tmux (the same guard used around tmux checks at the earlier block) and add/adjust tests to cover the missing-watch failure path in test/mcodex-launcher.test.ts so the CLI fails fast when watch is not installed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mcodex`:
- Around line 49-55: The tmux wheel-bindings in configure_tmux_scrollback() (the
tmux bind-key -T ... lines) are applied server‑globally and not scoped or
reverted; change the implementation to either 1) apply bindings to an isolated
tmux server/socket for mcodex runs (use tmux -L / -S to create/use a per-run
socket) or 2) scope and restore bindings per-session by saving existing
bindings, using session-targeted commands and restoring them on exit (use the
same session identifier used for tmux set-option and add matching tmux
unbind-key calls to revert), and add a regression test that asserts bindings are
reverted or that an isolated tmux server/socket is used; update the tmux
bind-key lines in scripts/mcodex (the WheelUpPane/WheelDownPane bind-key
entries) and add matching unbind/restore logic and tests in
test/mcodex-launcher.test.ts.
In `@test/mcodex-launcher.test.ts`:
- Around line 22-38: The test currently hardcodes the launcher prologue in
resolveInterval and text-matches the file later; instead read the real
scripts/mcodex file, extract the actual prologue block (from the top until the
first tmux/exec/exec line or another clear delimiter used in scripts/mcodex),
and run that extracted prologue with spawnSync (preserving the same env
injection pattern used now) so resolveInterval executes the shipped regex rather
than a copy; also update the assertions around the later text-match (the block
referenced at lines 61-66) to compare against the extracted prologue from
scripts/mcodex rather than an in-test literal. Ensure you update references to
resolveInterval and the test assertions to use the file-read+extract logic.
- Line 4: Remove the named import of describe, expect, and it from vitest in the
test file and use the vitest globals instead: delete the line importing {
describe, expect, it } from "vitest" so the test calls to describe, expect, and
it use the configured globals (references: describe, expect, it, vitest).
---
Outside diff comments:
In `@scripts/mcodex`:
- Around line 38-40: The script calls watch unconditionally in run_monitor()
(and at other spots for --monitor and --live-accounts) so add a runtime guard
that checks for watch availability (e.g., command -v watch >/dev/null) before
spawning it; implement a small helper (check_watch_available or ensure_watch)
and call it from run_monitor and wherever --monitor/--live-accounts create panes
so the script exits with a clear error message if watch is missing; update the
launcher paths that currently only check tmux (the same guard used around tmux
checks at the earlier block) and add/adjust tests to cover the missing-watch
failure path in test/mcodex-launcher.test.ts so the CLI fails fast when watch is
not installed.
In `@test/documentation.test.ts`:
- Around line 538-586: The test currently asserts packageJson.bin entries but
doesn't ensure the uploaded package actually includes the mcodex script; update
the "keeps package metadata aligned with the canonical owner surface" test to
also assert that packageJson.files (or packageJson.files as an array) contains
"scripts/mcodex" — locate the test using the packageJson variable and add an
expectation like
expect(packageJson.files).toEqual(expect.arrayContaining(["scripts/mcodex"]));
to guarantee the mcodex shim is published.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2de4a735-bd55-4b14-86a3-82b7ca068594
📒 Files selected for processing (4)
scripts/codex.jsscripts/mcodextest/documentation.test.tstest/mcodex-launcher.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
test/documentation.test.tstest/mcodex-launcher.test.tsscripts/codex.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
test/documentation.test.tstest/mcodex-launcher.test.ts
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
test/documentation.test.tstest/mcodex-launcher.test.tsscripts/codex.js
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/documentation.test.tstest/mcodex-launcher.test.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
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/documentation.test.tstest/mcodex-launcher.test.ts
**/scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex.js
**/scripts/codex.js
📄 CodeRabbit inference engine (AGENTS.md)
The package does not publish a global
codexbin;codex-multi-auth-codexis the explicit wrapper
Files:
scripts/codex.js
🔇 Additional comments (1)
scripts/codex.js (1)
21-25: LGTM!Also applies to: 170-190, 395-404, 571-579, 602-625
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/mcodex-launcher.test.ts (3)
22-38: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeofftest harness duplicates validation logic instead of extracting from shipped script.
test/mcodex-launcher.test.ts:26-33hardcodes the interval-validation block rather than extracting it fromscripts/mcodex. if the script's regex or fallback logic changes, this test will pass against stale logic. the comment at lines 22-24 claims extraction but the implementation copies.consider reading
scripts/mcodexand extracting the prologue up toquote_args()so the test exercises the actual shipped code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/mcodex-launcher.test.ts` around lines 22 - 38, The test duplicates the interval-validation prologue instead of using the actual shipped script; update resolveInterval to read scripts/mcodex, extract the prologue up to the unique marker/function name quote_args() (or the first occurrence of that token), and then run that extracted shell snippet (via spawnSync/bash) with MCODEX_MONITOR_INTERVAL set to the test value so the test exercises the real regex/fallback logic from scripts/mcodex rather than a hardcoded copy.
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winremove the explicit vitest import; globals are enabled for
test/**/*.test.ts.
test/mcodex-launcher.test.ts:4imports{ describe, expect, it }from"vitest"butvitest.config.tssetsglobals: truefor test files. drop the import and use the globals directly.-import { describe, expect, it } from "vitest";as per coding guidelines: "Vitest globals (
describe,it,expect) are enabled and should be used without explicit imports"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/mcodex-launcher.test.ts` at line 4, Remove the explicit import of Vitest symbols and rely on enabled globals: delete the import statement that imports { describe, expect, it } from "vitest" and use the global describe, it, and expect identifiers directly in the test file (ensure no other references to the named import remain). This keeps the test consistent with the config's globals: true setting and coding guidelines.
61-66: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winuse
readFileSyncinstead of spawning bash to cat the file.
test/mcodex-launcher.test.ts:63-65spawns bash just to read the script file.readFileSync(mcodexPath, "utf-8")is simpler, faster, and doesn't require bash.+import { readFileSync } from "node:fs"; ... it("the shipped script actually contains the numeric guard", () => { - const res = spawnSync("bash", ["-c", `cat "${mcodexPath}"`], { - encoding: "utf-8", - }); - expect(res.stdout).toMatch(/\^\[0-9\]\+\(\\\.\[0-9\]\+\)\?\$/); + expect(readFileSync(mcodexPath, "utf-8")).toMatch( + /\^\[0-9\]\+\(\\\.\[0-9\]\+\)\?\$/, + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/mcodex-launcher.test.ts` around lines 61 - 66, The test currently shells out with spawnSync to cat the file; replace that with a direct file read using fs.readFileSync to avoid spawning bash: import or require readFileSync from "fs", read the file into a string using readFileSync(mcodexPath, "utf-8") (replace the spawnSync call and the res variable), and keep the same assertion against that string (the expect(...).toMatch(...) should use the read content). Ensure mcodexPath remains the same identifier and remove the spawnSync import/use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/mcodex-launcher.test.ts`:
- Around line 22-38: The test duplicates the interval-validation prologue
instead of using the actual shipped script; update resolveInterval to read
scripts/mcodex, extract the prologue up to the unique marker/function name
quote_args() (or the first occurrence of that token), and then run that
extracted shell snippet (via spawnSync/bash) with MCODEX_MONITOR_INTERVAL set to
the test value so the test exercises the real regex/fallback logic from
scripts/mcodex rather than a hardcoded copy.
- Line 4: Remove the explicit import of Vitest symbols and rely on enabled
globals: delete the import statement that imports { describe, expect, it } from
"vitest" and use the global describe, it, and expect identifiers directly in the
test file (ensure no other references to the named import remain). This keeps
the test consistent with the config's globals: true setting and coding
guidelines.
- Around line 61-66: The test currently shells out with spawnSync to cat the
file; replace that with a direct file read using fs.readFileSync to avoid
spawning bash: import or require readFileSync from "fs", read the file into a
string using readFileSync(mcodexPath, "utf-8") (replace the spawnSync call and
the res variable), and keep the same assertion against that string (the
expect(...).toMatch(...) should use the read content). Ensure mcodexPath remains
the same identifier and remove the spawnSync import/use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3ca1021-0d60-437a-96ea-2ff34ffdf20d
📒 Files selected for processing (5)
package.jsonscripts/codex.jsscripts/mcodextest/documentation.test.tstest/mcodex-launcher.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
test/documentation.test.tstest/mcodex-launcher.test.tsscripts/codex.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
test/documentation.test.tstest/mcodex-launcher.test.ts
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
test/documentation.test.tstest/mcodex-launcher.test.tsscripts/codex.js
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/documentation.test.tstest/mcodex-launcher.test.ts
test/**/documentation.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Test documentation parity including command flags, config precedence, changelog policy, and governance rules
Files:
test/documentation.test.ts
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/documentation.test.tstest/mcodex-launcher.test.ts
package.json
📄 CodeRabbit inference engine (SECURITY.md)
package.json: Pin hono to version 4.12.14 or higher to avoid authentication bypass vulnerability in GHSA-xh87-mx6m-69f3
Pin rollup to version ^4.59.0 or higher to avoid vulnerable versions <4.59.0 in Vite and Vitest transitive dependencies
Files:
package.json
**/scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex.js
**/scripts/codex.js
📄 CodeRabbit inference engine (AGENTS.md)
The package does not publish a global
codexbin;codex-multi-auth-codexis the explicit wrapper
Files:
scripts/codex.js
🔇 Additional comments (15)
scripts/codex.js (11)
595-628: lock cleanup caveat is now documented inline.the past review comment about lock cleanup handlers not firing if parent exits immediately is addressed by the comment at lines 602-605. the 10-minute stale-lock recovery in
acquireStatusRefreshLockhandles orphaned locks. this is acceptable for the use case.
21-21: LGTM!Also applies to: 24-24, 85-87
170-190: LGTM!
205-258: LGTM!
260-316: LGTM!
318-389: LGTM!
391-406: LGTM!
408-524: LGTM!
526-554: LGTM!
556-593: LGTM!
4716-4717: LGTM!scripts/mcodex (1)
1-108: LGTM!package.json (1)
117-117: LGTM!Also applies to: 132-132
test/documentation.test.ts (1)
582-582: LGTM!test/mcodex-launcher.test.ts (1)
41-59: LGTM!
P1 (Greptile): the forwarded status line resolved accounts via the global
`resolveAccountsPath` (~/.codex/multi-auth/openai-codex-accounts.json), so inside
a project with a per-project pool it either showed no status line (global pool
empty) or showed the wrong account/quota — silently contradicting the account
Codex actually routes through.
Fix: add `resolveStatusAccountsDir`, which mirrors the runtime's own account
scoping (lib/runtime/account-scope.ts) by reusing the built dist helpers (never
re-deriving project keys from raw paths, per AGENTS.md):
- perProjectAccounts enabled + Codex CLI sync OFF + cwd resolves to a project
root → per-project pool via getProjectGlobalConfigDir(
resolveProjectStorageIdentityRoot(projectRoot));
- otherwise (no project, sync on, config off, or dist unavailable) → global.
Only the ACCOUNTS pool is per-project; quota-cache.json and
runtime-observability.json stay global (lib: getCodexMultiAuthDir), so those
reads are unchanged. `maybePrintForwardStatusLine` is now async to resolve the
dir before formatting; any failure falls back to the global dir so the launcher
never breaks.
Note: the per-project pool is grounded in the dist config dir, which itself
honors CODEX_MULTI_AUTH_DIR / CODEX_HOME — so no special-casing of those envs
here, or the status line would diverge from where the runtime writes.
test/mcodex-statusline-scope.test.ts: end-to-end regressions over the real
wrapper + dist build — per-project pool is shown inside a project, global pool
outside one, and global when Codex CLI sync is enabled (matching account-scope).
typecheck + lint clean; full suite 4070 passed.
Follow-up — per-project account pool (P1)Pushed
Fix: added
Only the accounts pool is per-project —
Verification: typecheck clean, lint clean, full suite 4070 passed. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/codex.js`:
- Around line 232-280: The resolveStatusAccountsDir function currently swallows
import/resolution failures and early-returns without any visibility; update
resolveStatusAccountsDir to accept an optional cwd parameter (defaulting to
process.cwd()) and add debug logging (when env.CODEX_MULTI_AUTH_DEBUG is truthy)
at each failure point: when dynamic imports fail, when required functions are
missing (check loadPluginConfig, getPerProjectAccounts, findProjectRoot,
resolveProjectStorageIdentityRoot, getProjectGlobalConfigDir), when
getPerProjectAccounts !== true, and when isCodexCliSyncEnabled() forces
globalDir; use resolveMultiAuthDirFromEnv as the fallback value and include the
caught error details in the debug log inside the catch so developers can
diagnose missing dist helpers or wrong project roots.
In `@test/mcodex-statusline-scope.test.ts`:
- Around line 71-72: The test uses a non-existent flag "--version-noop" in
runWrapper which could collide with future real flags; update the runWrapper
invocation in test/mcodex-statusline-scope.test.ts (the runWrapper function) to
use a clearly fake arg such as "--noop-for-test" or remove the flag entirely to
exercise the TUI path, and ensure any downstream expectations still hold (i.e.,
keep spawnSync(process.execPath, [wrapperScript, ...]) but replace
"--version-noop" with the chosen safe token or omit it).
- Line 9: Remove the unused import createHash from the top of
test/mcodex-statusline-scope.test.ts: locate the import statement "import {
createHash } from 'node:crypto';" and delete it (or remove createHash from the
import list) so there are no dead imports referenced by this test file.
- Line 12: Remove the explicit Vitest imports and rely on the enabled globals:
delete the import line that brings in afterEach, describe, expect, it and use
the global identifiers directly in the test file (references: afterEach,
describe, it, expect) so the test adheres to the guideline of using Vitest
globals without importing.
- Around line 28-41: The afterEach cleanup loop uses rmSync and a tight retry
loop which lacks backoff; replace it with the recommended removeWithRetry helper
(or add a small synchronous backoff between attempts) to handle transient
EBUSY/EPERM/ENOTEMPTY on Windows: locate the afterEach block referencing
createdDirs and rmSync, import or call removeWithRetry (or, if helper not
available, implement a short sync sleep via Atomics.wait between attempts) and
ensure the cleanup uses that function instead of the bare rmSync retry loop so
tests reliably cleanup on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2aee1f99-830d-47a9-b959-1874aaafcd10
📒 Files selected for processing (2)
scripts/codex.jstest/mcodex-statusline-scope.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESM only (
"type": "module"), Node >= 18
Files:
test/mcodex-statusline-scope.test.tsscripts/codex.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
as any,@ts-ignore, or@ts-expect-errorTypeScript assertions
Files:
test/mcodex-statusline-scope.test.ts
{**/scripts/**/*.js,**/test/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use bare recursive delete logic in Windows-sensitive scripts/tests without retry handling for transient
EBUSY/EPERM/ENOTEMPTYerrors
Files:
test/mcodex-statusline-scope.test.tsscripts/codex.js
test/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
test/**/*.test.ts: Vitest globals (describe,it,expect) are enabled and should be used without explicit imports
Maintain 80% coverage threshold across statements, branches, functions, and lines
UseremoveWithRetryfor Windows filesystem cleanup instead of barefs.rmto handle EBUSY/EPERM/ENOTEMPTY backoff
Use source files in tests, not compileddist/files; test the source directly
Do not skip tests without justification; include rationale if a test must be skipped
Relax ESLint rules for test files as specified ineslint.config.js
Files:
test/mcodex-statusline-scope.test.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/mcodex-statusline-scope.test.ts
**/scripts/codex*.js
📄 CodeRabbit inference engine (AGENTS.md)
Do not bypass the official Codex CLI by reimplementing general Codex commands in the wrapper
Files:
scripts/codex.js
**/scripts/codex.js
📄 CodeRabbit inference engine (AGENTS.md)
The package does not publish a global
codexbin;codex-multi-auth-codexis the explicit wrapper
Files:
scripts/codex.js
🔇 Additional comments (8)
scripts/codex.js (5)
649-682: lock cleanup is now non-blocking—acknowledged caveat about detached child is documented.the inline comment at lines 656-659 documents the detached-child stale-lock TOCTOU risk, matching the past review comment. the 10-minute stale-lock recovery is the mitigation. this is acceptable given the idempotent, read-mostly nature of forecast refresh.
one edge case: if many parallel mcodex invocations hit a slow forecast child (>10m), the stale-lock eviction could spawn duplicate children briefly. the risk is benign (redundant cache writes) but worth noting if you ever add heavier work to this path.
220-231: LGTM!
537-578: LGTM!
179-190: LGTM!
4770-4771: LGTM!test/mcodex-statusline-scope.test.ts (3)
94-133: LGTM!
135-154: LGTM!
156-190: LGTM!
Follow-up review findings: - scripts/mcodex (Major): `watch` was invoked unconditionally in run_monitor and the two tmux live-account panes, with no equivalent of the existing `tmux` guard. Added require_watch; --monitor now fails fast with an actionable error and propagates a non-zero exit (exit $? instead of exit 0), and each live pane is gated on `&& require_watch` so a missing `watch` skips the pane instead of spawning a broken one (Codex still launches). - test/documentation.test.ts (Minor): the bin assertion locked the new `mcodex` entry but nothing verified package.json files[] publishes the shim, so npm could ship a bin pointing at a missing script while the test passed. Now every declared bin target is asserted present in files[]. Tests: test/mcodex-launcher.test.ts gains a missing-`watch` runtime guard case (drives require_watch with `watch` shadowed absent → non-zero + clear error) and a static check that all three watch sites are wired through require_watch. typecheck + lint clean; full suite 4072 passed.
Follow-up 2 —
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
- Bump vitest / @vitest/ui / @vitest/coverage-v8 to ^4.1.8 (dev-only), clearing GHSA-5xrq-8626-4rwp so `npm run audit:ci` exits 0. audit:prod was already clean (nothing test-related ships); this unblocks the CI release gate. - Version → 2.1.13-beta.3. - Add docs/releases/v2.1.13-beta.3.md covering the Phase 1 audit (#499), the mcodex launcher + statusline (#500), and the pre-release hardening (#503). Update the docs portal + root README prerelease pointers (documentation.test.ts parity). typecheck + lint + audit:ci clean; full suite 4246 passed / 1 skipped on vitest 4.1.8; documentation parity 25/25.
…#503) * fix(security,resilience): pre-release hardening from deep stress test Found by a whole-tree adversarial audit + runtime fuzz of the merged main (both #499 and #500). The tree was already well-hardened — these are the residual MEDIUM/LOW gaps, all verified. Egress hardening (never forward inbound client credentials upstream): - runtime-rotation-proxy.ts + local-bridge.ts: strip inbound `cookie` and `proxy-authorization` on the outbound request, alongside the existing authorization/x-api-key stripping. A client cookie would otherwise ride upstream with the managed OAuth Bearer. - runtime-rotation-proxy.ts readErrorBody: the outbound fetch's abort timer is cleared once headers arrive, so a stalled ERROR body (429/5xx path) could hang the handler forever — the success path is per-chunk stall-bounded, the error path was not. Read it via a reader with a per-read idle timeout + a 1MB cap, cancelling the stream on stall/overflow. Windows fs resilience (close the transient-lock gaps the fuzz reproduced): - storage.ts: the secret-bearing WAL write and temp write had NO retry, and the temp cleanup was single-shot — a transient EBUSY/EPERM could fail a valid save or strand a token-bearing *.tmp. Wrap all three in withFileOperationRetry. Also align copyFileWithRetry / renameFileWithRetry to the shared shouldRetryFileOperation taxonomy (adds ENOTEMPTY/EACCES/EAGAIN). The primary commit rename keeps its existing EPERM/EBUSY contract unchanged. - quota-cache.ts: widen RETRYABLE_FS_CODES to the shared set. Least-privilege: - runtime/runtime-observability.ts: persist the snapshot (account id/label) with mode 0o600 and the dir 0o700, matching the other sensitive writers (no-op on win32, owner-only on POSIX). Tests: cookie strip on both egress paths; existing storage WAL/temp/backup retry coverage still green (rename contract unchanged). Also cleaned a stale eslint-disable + unused import in the merged mcodex statusline test. typecheck + lint clean; full suite 4246 passed / 1 skipped. * chore(release): cut v2.1.13-beta.3 + bump vitest to ^4.1.8 - Bump vitest / @vitest/ui / @vitest/coverage-v8 to ^4.1.8 (dev-only), clearing GHSA-5xrq-8626-4rwp so `npm run audit:ci` exits 0. audit:prod was already clean (nothing test-related ships); this unblocks the CI release gate. - Version → 2.1.13-beta.3. - Add docs/releases/v2.1.13-beta.3.md covering the Phase 1 audit (#499), the mcodex launcher + statusline (#500), and the pre-release hardening (#503). Update the docs portal + root README prerelease pointers (documentation.test.ts parity). typecheck + lint + audit:ci clean; full suite 4246 passed / 1 skipped on vitest 4.1.8; documentation parity 25/25. * fix: address #503 review (dir-mode + retry/strip coverage + version pins) - runtime/runtime-observability.ts: re-assert dir 0o700 via fs.chmod after mkdir — mkdir's mode is a no-op on a pre-existing multi-auth dir, so the upgrade path kept old (possibly permissive) perms. POSIX-only, best-effort. - Bump .codex-plugin/plugin.json + AGENTS.md to 2.1.13-beta.3 (plugin-manifest test asserts manifest version == package version; the beta.3 bump left them stale). Tests: - local-bridge + runtime-rotation-proxy: assert inbound `proxy-authorization` (alongside cookie) is stripped on the outbound request, so a refactor dropping the delete is caught. - quota-cache: read-retry (EACCES) + rename-retry (ENOTEMPTY) + last-write-wins queue-ordering regressions, pinning the widened retry taxonomy. - runtime-observability-dir-mode (new, POSIX-only): pre-create CODEX_MULTI_AUTH_DIR world-writable, persist a snapshot, assert the dir becomes owner-only — proves the chmod-after-mkdir hardening on the upgrade path. - runtime-observability test fs-mock now exposes chmod. typecheck + lint + audit:ci clean; full suite 4249 passed / 2 skipped. * fix: address #503 round-2 review (chmod error handling + win32/strip coverage) - runtime/runtime-observability.ts: stop swallowing every chmod failure. Only ENOENT (dir removed by a concurrent process) is ignored; any other failure is surfaced rather than silently leaving a world-readable dir holding account ids/labels. - runtime-observability.test.ts: add a win32 case asserting the snapshot persists WITHOUT calling chmod (POSIX-only branch), and reset the chmod mock per test. - local-bridge.test.ts: extend the auth-enabled runtime-proxy path test to assert cookie + proxy-authorization are stripped too (parity with the no-key path). typecheck + lint clean; full suite 4250 passed / 2 skipped. * test(observability): assert snapshot temp file is written 0o600 Addresses the #503 round-3 review: the win32 persistence test now also asserts writeFile received mode 0o600, so an owner-only-perms regression on the snapshot file fails the suite.
2 participants
PR Summary
Add a lightweight
mcodexlauncher and cached multi-auth status display for Codex sessions.What Changed
Cache Behavior
quota-cache.json,runtime-observability.json, and account storage.Controls
Validation
mcodexsyntax check passed.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds a lightweight
mcodexbash launcher with optional tmux session management and a cached startup status line (codex-multi-auth | model | cwd | Account N | usage | email(plan) | cache age) emitted to stderr before codex launches.scripts/mcodex: new entrypoint with--tmux,--live-accounts, and--monitormodes; numeric-only validation onMCODEX_MONITOR_INTERVALcloses the shell-injection path flagged in review; args passed to codex are shell-quoted viaprintf %q.scripts/codex.js: ~480 lines of new helpers for status-line formatting, per-project account pool resolution (via dist dynamic imports), and a background quota-cache refresh child with an async lock mechanism; previous issues around unix-only path sep, global-only account reads, and blocking event-loop callbacks are resolved in this diff.mcodex-launcher.test.tsdrives the interval-validation regex directly;mcodex-statusline-scope.test.tsproves per-project vs. global pool routing end-to-end (guarded byhasDistBuild());documentation.test.tsadds a publish-integrity check that everybintarget is also infiles[].Confidence Score: 5/5
the core production paths in codex.js and mcodex are safe to merge; changes address previously-flagged injection, path-separator, and per-project account bugs, and gracefully degrade on any dist-import failure.
the status-line and background-refresh logic is well-isolated behind feature flags and TTY guards; no changed production path introduces new data loss, auth boundary crossings, or hard crashes. the remaining gaps are in the test layer and do not affect production behavior.
test/mcodex-statusline-scope.test.ts — the afterEach retry loop and dist/ import both warrant a second look before this lands on a windows CI runner
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant M as mcodex (bash) participant C as codex.js (wrapper) participant D as dist/ helpers participant FS as ~/.codex/multi-auth/ participant BG as background child (forecast) participant R as real codex binary U->>M: mcodex [args] M->>M: validate MCODEX_MONITOR_INTERVAL (numeric guard) alt --monitor M->>M: "run_monitor -> watch codex-multi-auth list" else --tmux M->>M: launch tmux session + optional live-accounts pane M->>C: exec codex-multi-auth-codex [args] else passthrough M->>C: exec codex-multi-auth-codex [args] end C->>D: resolveStatusAccountsDir() - import dist/lib/config.js, paths.js, state.js D-->>C: per-project or global accounts dir C->>FS: readJsonFileQuiet(accounts, quota-cache, runtime-observability) FS-->>C: storage, quotaCache, runtime data C->>C: "formatForwardStatusLine() -> build status string" C->>U: write status line to stderr C->>C: maybeRefreshQuotaCacheInBackground() C->>C: acquireStatusRefreshLock() - mkdirSync lock dir C->>BG: spawn codex-multi-auth forecast --live --json (detached+unref) BG-->>FS: writes updated quota-cache.json BG-->>C: "close/error -> removeDirectoryBestEffortAsync(lockDir)" C->>R: exec real codex binary [args]Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "fix(mcodex): gate monitor modes on `watc..." | Re-trigger Greptile