From 735b04357c626d9f76d9415e7873ef73aee74c55 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Tue, 2 Jun 2026 22:48:10 +0800 Subject: [PATCH 1/8] fix: resolve v2.2.0 deep-audit findings (4 HIGH, 6 MEDIUM) Deep audit of the v2.2.0 tree (6 parallel auditors across auth, proxy/rotation, scripts, quota/policy, storage, CLI) surfaced bugs not caught pre-release. HIGH: - mcodex: rewrite the flagship launcher (#500) as a Node bin (scripts/mcodex.js). The bash script shipped as a Windows bin died with HCS_E_SERVICE_NOT_AVAILABLE when npm's shim invoked bare 'bash' and the WSL stub shadowed git-bash. The Node launcher has zero bash dependency, builds tmux/watch invocations as argv arrays (no shell interpolation), and degrades gracefully where those tools are absent. - auth/server: capture OAuth code/state in per-call closures instead of on the shared http.Server, so concurrent logins in one process can't cross-bind state. - model-capability-matrix: read capability snapshots/boosts with the entitlement key (the write key), not the sha256 account key, so the matrix stops reporting every account as supporting every model. - storage/transactions: track lock ownership via AsyncLocalStorage and persist recovered flagged backups unlocked when already inside a transaction, fixing a re-entrant deadlock (doctor restore + flagged .bak without primary) that wedged all future saves. MEDIUM: - codex-manager styleAccountDetailText: scan the whole detail for failure keywords so a 'working' prefix can't render green when 'failed' is trapped in the (NN%) quota segment (same precedence class as the prior suffix fix). - workspace command: reject fractional/trailing-garbage indices (/^\d+$/) instead of silently truncating via parseInt, matching the 'switch' command guard. - request/fetch-helpers: include NORMALIZED_UNSUPPORTED_MODEL_PATTERN in the unsupported-model classifier so 'not currently available for this chatgpt account' is treated as an entitlement block, not a transient outage. - storage/account-persistence: carry pinnedAccountIndex and affinityGeneration through the combined-transaction clone so doctor restore can't erase a manual pin. - storage + quota-cache: create secret dirs mode 0o700 (+ best-effort chmod on POSIX) instead of relying on umask. - forecast: exclude unavailable (policy-blocked/exhausted) accounts from recommendation candidates; return null with a clear reason when none are ready. Added/extended tests for every fix. Full suite: 4303 passed, 2 skipped, 0 failed; typecheck + lint clean. mcodex Node launcher live-verified in PowerShell. Findings detail: .omc/research/v2.2.0-audit-findings.md --- lib/auth/server.ts | 25 +- lib/codex-manager.ts | 7 +- lib/codex-manager/commands/workspace.ts | 11 + lib/forecast.ts | 24 +- lib/model-capability-matrix.ts | 23 +- lib/quota-cache.ts | 16 +- lib/request/fetch-helpers.ts | 2 + lib/storage.ts | 31 +- lib/storage/account-persistence.ts | 12 +- lib/storage/transactions.ts | 16 +- package.json | 4 +- scripts/mcodex | 117 ------- scripts/mcodex.js | 315 +++++++++++++++++++ test/account-persistence.test.ts | 29 ++ test/codex-manager-detail-tone.test.ts | 11 + test/codex-manager-workspace-command.test.ts | 28 ++ test/documentation.test.ts | 2 +- test/fetch-helpers.test.ts | 28 ++ test/forecast.test.ts | 41 +++ test/mcodex-launcher.test.ts | 177 ++++++----- test/model-capability-matrix.test.ts | 40 ++- test/server.unit.test.ts | 107 +++++-- test/storage-flagged.test.ts | 71 +++++ test/transactions.test.ts | 37 +++ 24 files changed, 901 insertions(+), 273 deletions(-) delete mode 100755 scripts/mcodex create mode 100644 scripts/mcodex.js diff --git a/lib/auth/server.ts b/lib/auth/server.ts index b0bc25e86..732715487 100644 --- a/lib/auth/server.ts +++ b/lib/auth/server.ts @@ -38,6 +38,13 @@ export function startLocalOAuthServer({ state: string; }): Promise { let pollAborted = false; + // Capture the authorization code/state in per-call closure variables rather + // than mutating the shared http.Server instance. Two logins in the same + // process previously cross-bound callback state via server._lastCode/ + // _lastState; isolating them here keeps concurrent server instances + // independent. + let capturedCode: string | undefined; + let capturedState: string | undefined; const server = http.createServer((req, res) => { try { const url = new URL(req.url || "", AUTH_REDIRECT.origin); @@ -66,18 +73,14 @@ export function startLocalOAuthServer({ "default-src 'none'; style-src 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; script-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'", ); res.end(successHtml); - const trackedServer = server as http.Server & { - _lastCode?: string; - _lastState?: string; - }; - if (trackedServer._lastCode) { + if (capturedCode) { logWarn( "Duplicate OAuth callback received; preserving first authorization code", ); return; } - trackedServer._lastCode = code; - trackedServer._lastState = state; + capturedCode = code; + capturedState = state; } catch (err) { logError( `Request handler error: ${(err as Error)?.message ?? String(err)}`, @@ -107,13 +110,9 @@ export function startLocalOAuthServer({ new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); for (let i = 0; i < maxIterations; i++) { if (pollAborted) return null; - const trackedServer = server as http.Server & { - _lastCode?: string; - _lastState?: string; - }; - const lastCode = trackedServer._lastCode; + const lastCode = capturedCode; if (lastCode) { - if (trackedServer._lastState !== expectedState) { + if (capturedState !== expectedState) { logWarn( "Discarding OAuth callback due to state mismatch in waitForCode", ); diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index 0446af50a..e5ad44ed4 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -419,7 +419,12 @@ export function styleAccountDetailText( const quota = (quotaMatch[2] ?? "").trim(); const suffix = (quotaMatch[3] ?? "").trim(); - const prefixTone: PromptTone = /failed|error/i.test(prefix) + // danger wins across the WHOLE detail: a failure keyword anywhere — even + // trapped inside the (…%) quota segment, e.g. + // "signed in and working (live check failed: … 0%)" — must keep the prefix + // red, never let a "working"/"ok" prefix render green over a real failure. + const detailHasFailure = /failed|error|rate-limited/i.test(compact); + const prefixTone: PromptTone = detailHasFailure ? "danger" : /ok|working|succeeded|valid/i.test(prefix) ? "success" diff --git a/lib/codex-manager/commands/workspace.ts b/lib/codex-manager/commands/workspace.ts index d7994adb6..d9cf38af5 100644 --- a/lib/codex-manager/commands/workspace.ts +++ b/lib/codex-manager/commands/workspace.ts @@ -44,6 +44,13 @@ export async function runWorkspaceCommand( return 1; } + // Require a plain positive integer (matches `switch`): Number.parseInt would + // silently truncate "1.9" -> 1 or "2abc" -> 2 and operate on the wrong + // account, so reject anything that isn't all digits. + if (!/^\d+$/.test(accountArg.trim())) { + logError(`Invalid account index: ${accountArg}`); + return 1; + } const parsedAccount = Number.parseInt(accountArg, 10); if (!Number.isFinite(parsedAccount) || parsedAccount < 1) { logError(`Invalid account index: ${accountArg}`); @@ -85,6 +92,10 @@ export async function runWorkspaceCommand( return 0; } + if (!/^\d+$/.test(workspaceArg.trim())) { + logError(`Invalid workspace index. Valid range: 1-${workspaces.length}`); + return 1; + } const parsedWorkspace = Number.parseInt(workspaceArg, 10); if ( !Number.isFinite(parsedWorkspace) || diff --git a/lib/forecast.ts b/lib/forecast.ts index fd82226ea..c1bd44514 100644 --- a/lib/forecast.ts +++ b/lib/forecast.ts @@ -376,14 +376,32 @@ function compareForecastResults( export function recommendForecastAccount( results: ForecastAccountResult[], ): ForecastRecommendation { + // Exclude unavailable accounts (policy-blocked, runtime-skipped, quota + // exhausted) in addition to disabled/hard-failed ones. Such accounts carry + // availability === "unavailable" with hardFailure === false, so without this + // guard they were recommended with a misleading "pick shortest wait". const candidates = results.filter( - (result) => !result.disabled && !result.hardFailure, + (result) => + !result.disabled && + !result.hardFailure && + result.availability !== "unavailable", ); if (candidates.length === 0) { + // Distinguish "blocked/exhausted" accounts (unavailable but neither + // disabled nor hard-failed — e.g. policy block, runtime skip, quota + // exhaustion) from disabled/hard-failed ones so the guidance matches the + // actual blocker. + const hasBlockedOrExhausted = results.some( + (result) => + result.availability === "unavailable" && + !result.disabled && + !result.hardFailure, + ); return { recommendedIndex: null, - reason: - "No healthy accounts are available. Run `codex-multi-auth login` to add a fresh account.", + reason: hasBlockedOrExhausted + ? "All accounts are blocked or exhausted. Wait for a reset, clear the block, or run `codex-multi-auth login` to add a fresh account." + : "No healthy accounts are available. Run `codex-multi-auth login` to add a fresh account.", }; } diff --git a/lib/model-capability-matrix.ts b/lib/model-capability-matrix.ts index 2973c4f62..6d72569da 100644 --- a/lib/model-capability-matrix.ts +++ b/lib/model-capability-matrix.ts @@ -82,14 +82,12 @@ export function buildModelCapabilityMatrix(input: { for (const [index, account] of accounts.entries()) { const accountKey = getAccountPolicyKey(account, index); - const entitlementKeys = [ - accountKey, - resolveEntitlementAccountKey({ - accountId: account.accountId, - email: account.email, - index, - }), - ]; + const entitlementKey = resolveEntitlementAccountKey({ + accountId: account.accountId, + email: account.email, + index, + }); + const entitlementKeys = [accountKey, entitlementKey]; for (const model of models) { const profile = MODEL_PROFILES[model] ?? MODEL_PROFILES[resolveNormalizedModel(model)]; if (!profile) continue; @@ -99,11 +97,16 @@ export function buildModelCapabilityMatrix(input: { profile.normalizedModel, now, ); + // quota-forecast-01: the capability store is WRITTEN under the + // entitlement key (resolveEntitlementAccountKey) at the + // recordUnsupported sites, so reads must use the same key. Previously + // this used getAccountPolicyKey ("sha256:…"), a different format, so + // getSnapshot/getBoost never matched and the capability signal was dead. const capabilityPolicy = - input.capabilityPolicy?.getSnapshot(accountKey, profile.normalizedModel) ?? + input.capabilityPolicy?.getSnapshot(entitlementKey, profile.normalizedModel) ?? null; const capabilityBoost = - input.capabilityPolicy?.getBoost(accountKey, profile.normalizedModel, now) ?? + input.capabilityPolicy?.getBoost(entitlementKey, profile.normalizedModel, now) ?? 0; const quota = input.quotaCache && input.storage diff --git a/lib/quota-cache.ts b/lib/quota-cache.ts index 514de11db..a05c4452a 100644 --- a/lib/quota-cache.ts +++ b/lib/quota-cache.ts @@ -235,7 +235,21 @@ export async function saveQuotaCache(data: QuotaCacheData): Promise { const writeTask = async (): Promise => { try { - await fs.mkdir(getCodexMultiAuthDir(), { recursive: true }); + const cacheDir = getCodexMultiAuthDir(); + // The quota cache lives alongside other at-rest secrets, so keep the + // directory owner-only on POSIX (mode is a no-op on win32 / ACL-based). + await fs.mkdir(cacheDir, { recursive: true, mode: 0o700 }); + // mkdir's mode only applies to a freshly-created dir; an upgrade with a + // pre-existing multi-auth dir keeps its old (possibly world-listable) + // perms, so re-assert 0o700 on POSIX. Best-effort: a chmod failure must + // not break the cache write (the 0o600 file below still protects data). + if (process.platform !== "win32") { + try { + await fs.chmod(cacheDir, 0o700); + } catch { + // Best-effort hardening only. + } + } const tempPath = `${QUOTA_CACHE_PATH}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, { encoding: "utf8", diff --git a/lib/request/fetch-helpers.ts b/lib/request/fetch-helpers.ts index 63a35802e..b3ef3b4b1 100644 --- a/lib/request/fetch-helpers.ts +++ b/lib/request/fetch-helpers.ts @@ -217,6 +217,7 @@ export function getUnsupportedCodexModelInfo( } const isUnsupportedDetail = CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN.test(detail) || + NORMALIZED_UNSUPPORTED_MODEL_PATTERN.test(detail) || MODEL_ACCESS_DENIED_PATTERN.test(detail); if (!isUnsupportedDetail) { return { isUnsupported: false }; @@ -242,6 +243,7 @@ export function getUnsupportedCodexModelInfo( code === CHATGPT_CODEX_UNSUPPORTED_MODEL_CODE || (message ? CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN.test(message) || + NORMALIZED_UNSUPPORTED_MODEL_PATTERN.test(message) || MODEL_ACCESS_DENIED_PATTERN.test(message) : false); diff --git a/lib/storage.ts b/lib/storage.ts index ef1859877..2ed97fde6 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -135,6 +135,7 @@ import { } from "./storage/storage-parser.js"; import { getTransactionSnapshotState, + isStorageLockHeld, runInTransactionSnapshotContext, withAccountStorageTransaction as runWithAccountStorageTransaction, withStorageLock, @@ -1800,7 +1801,21 @@ async function saveAccountsUnlocked(storage: AccountStorageV3): Promise { walPath, storageBackupEnabled: storageBackupEnabled && existsSync(path), ensureDirectory: async () => { - await fs.mkdir(dirname(path), { recursive: true }); + const dir = dirname(path); + // Account storage holds OAuth token material, so keep its directory + // owner-only on POSIX (mode is a no-op on win32 / ACL-based). + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + // mkdir's mode only applies to a freshly-created dir; a pre-existing dir + // from an earlier build keeps its old (possibly world-listable) perms, so + // re-assert 0o700 on POSIX. Best-effort: a chmod failure must not break + // the save (the atomic write still uses mode 0o600 for the file itself). + if (process.platform !== "win32") { + try { + await fs.chmod(dir, 0o700); + } catch { + // Best-effort hardening only. + } + } }, ensureGitignore: () => ensureGitignore(path), looksLikeSyntheticFixtureStorage, @@ -2081,14 +2096,22 @@ export async function loadFlaggedAccounts(): Promise { isRecord, now: () => Date.now(), }), - persistRecoveredBackup: async (storage, resetMarkerPath) => - withStorageLock(async () => { + persistRecoveredBackup: async (storage, resetMarkerPath) => { + // When loadFlaggedAccounts runs inside an already-held storage lock + // (e.g. withAccountAndFlaggedStorageTransaction or + // withFlaggedStorageTransaction loading current flagged state), the + // global mutex has no reentrancy. Re-acquiring it here would deadlock, + // so persist via the unlocked save while still respecting the reset + // marker. Otherwise acquire the lock as usual. + const recover = async (): Promise => { if (existsSync(resetMarkerPath)) { return false; } await saveFlaggedAccountsUnlocked(storage); return true; - }), + }; + return isStorageLockHeld() ? recover() : withStorageLock(recover); + }, saveFlaggedAccounts, loadFlaggedAccountsState, logError: (message, details) => { diff --git a/lib/storage/account-persistence.ts b/lib/storage/account-persistence.ts index d93cb1b47..de1a0f877 100644 --- a/lib/storage/account-persistence.ts +++ b/lib/storage/account-persistence.ts @@ -3,7 +3,7 @@ import type { AccountStorageV3 } from "../storage.js"; export function cloneAccountStorageForPersistence( storage: AccountStorageV3 | null | undefined, ): AccountStorageV3 { - return { + const cloned: AccountStorageV3 = { version: 3, accounts: structuredClone(storage?.accounts ?? []), activeIndex: @@ -13,4 +13,14 @@ export function cloneAccountStorageForPersistence( : 0, activeIndexByFamily: structuredClone(storage?.activeIndexByFamily ?? {}), }; + // Preserve the user's manual pin (issue #474) and affinity generation across + // the combined account+flagged transaction (incl. doctor restore). Dropping + // these erased a manual `switch ` pin on persistence. + if (typeof storage?.pinnedAccountIndex === "number") { + cloned.pinnedAccountIndex = storage.pinnedAccountIndex; + } + if (typeof storage?.affinityGeneration === "number") { + cloned.affinityGeneration = storage.affinityGeneration; + } + return cloned; } diff --git a/lib/storage/transactions.ts b/lib/storage/transactions.ts index f67fbedbb..ed6591bfb 100644 --- a/lib/storage/transactions.ts +++ b/lib/storage/transactions.ts @@ -10,6 +10,18 @@ export type TransactionSnapshotState = { let storageMutex: Promise = Promise.resolve(); const transactionSnapshotContext = new AsyncLocalStorage(); +const storageLockHeldContext = new AsyncLocalStorage(); + +/** + * Reports whether the caller is already running inside a `withStorageLock` + * critical section. The global storage mutex has no reentrancy, so callers + * that may run both standalone and nested under a held lock (e.g. recovery + * persistence triggered while loading flagged storage during a transaction) + * must use this to avoid re-acquiring the lock and deadlocking. + */ +export function isStorageLockHeld(): boolean { + return storageLockHeldContext.getStore() === true; +} export function getTransactionSnapshotState(): | TransactionSnapshotState @@ -30,7 +42,9 @@ export function withStorageLock(fn: () => Promise): Promise { storageMutex = new Promise((resolve) => { releaseLock = resolve; }); - return previousMutex.then(fn).finally(() => releaseLock()); + return previousMutex + .then(() => storageLockHeldContext.run(true, fn)) + .finally(() => releaseLock()); } export async function withAccountStorageTransaction( diff --git a/package.json b/package.json index 0b7d722ba..d6ef9fa79 100644 --- a/package.json +++ b/package.json @@ -114,7 +114,7 @@ "prepare": "husky" }, "bin": { - "mcodex": "scripts/mcodex", + "mcodex": "scripts/mcodex.js", "codex-multi-auth-codex": "scripts/codex.js", "codex-multi-auth-app-launcher": "scripts/codex-app-launcher.js", "codex-multi-auth": "scripts/codex-multi-auth.js" @@ -129,7 +129,7 @@ "scripts/codex-bin-resolver.js", "scripts/codex-multi-auth.js", "scripts/codex-routing.js", - "scripts/mcodex", + "scripts/mcodex.js", "scripts/install-codex-auth-utils.js", "scripts/postinstall.js", "scripts/preuninstall.js", diff --git a/scripts/mcodex b/scripts/mcodex deleted file mode 100755 index be1fc36ec..000000000 --- a/scripts/mcodex +++ /dev/null @@ -1,117 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -monitor_interval="${MCODEX_MONITOR_INTERVAL:-5}" -live_accounts=0 -tmux_history_limit="${MCODEX_TMUX_HISTORY_LIMIT:-50000}" - -# Security: monitor_interval is embedded into `watch -n ...` command strings -# that tmux hands to a shell. An attacker-controlled MCODEX_MONITOR_INTERVAL like -# "5; rm -rf ~" would otherwise execute. Require a positive integer (optionally -# with a fractional part for sub-second intervals) and fall back to the default -# otherwise, so no shell metacharacters can ever reach the command string. -if ! [[ "$monitor_interval" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then - echo "mcodex: invalid MCODEX_MONITOR_INTERVAL '$monitor_interval'; using 5" >&2 - monitor_interval=5 -fi - -# Same hardening for the tmux scrollback history limit (used as a tmux option -# argument): must be a plain integer. -if ! [[ "$tmux_history_limit" =~ ^[0-9]+$ ]]; then - echo "mcodex: invalid MCODEX_TMUX_HISTORY_LIMIT '$tmux_history_limit'; using 50000" >&2 - tmux_history_limit=50000 -fi - -quote_args() { - local quoted=() - local arg - for arg in "$@"; do - printf -v arg '%q' "$arg" - quoted+=("$arg") - done - # Join with single spaces and NO trailing space (a trailing space would become - # part of the interpolated tmux command string). - local IFS=' ' - printf '%s' "${quoted[*]}" -} - -require_watch() { - if ! command -v watch >/dev/null 2>&1; then - echo "mcodex: 'watch' is not installed; the live account monitor requires it (install procps / procps-ng)." >&2 - return 1 - fi -} - -run_monitor() { - require_watch || return 1 - watch -n "$monitor_interval" 'codex-multi-auth list' -} - -configure_tmux_scrollback() { - local target="${1:-}" - local target_args=() - if [[ -n "$target" ]]; then - target_args=(-t "$target") - fi - - tmux set-option "${target_args[@]}" mouse on >/dev/null 2>&1 - tmux set-option "${target_args[@]}" history-limit "$tmux_history_limit" >/dev/null 2>&1 - tmux bind-key -T root WheelUpPane copy-mode -e >/dev/null 2>&1 - tmux bind-key -T copy-mode WheelUpPane send-keys -X scroll-up >/dev/null 2>&1 - tmux bind-key -T copy-mode WheelDownPane send-keys -X scroll-down >/dev/null 2>&1 - tmux bind-key -T copy-mode-vi WheelUpPane send-keys -X scroll-up >/dev/null 2>&1 - tmux bind-key -T copy-mode-vi WheelDownPane send-keys -X scroll-down >/dev/null 2>&1 -} - -if [[ "${1:-}" == "--monitor" ]]; then - # --monitor takes no extra args; the live account list is fixed. Propagate the - # watch-availability check's exit code instead of masking it with `exit 0`. - run_monitor - exit $? -fi - -if [[ "${1:-}" == "--tmux" || "${1:-}" == "-t" ]]; then - shift - if [[ "${1:-}" == "--live-accounts" ]]; then - live_accounts=1 - shift - fi - if ! command -v tmux >/dev/null 2>&1; then - echo "mcodex: tmux is not installed; launching without tmux" >&2 - exec codex-multi-auth-codex "$@" - fi - - if [[ -n "${TMUX:-}" ]]; then - configure_tmux_scrollback - if [[ "$live_accounts" == "1" ]] && require_watch; then - tmux split-window -h "watch -n $monitor_interval 'codex-multi-auth list'" - fi - exec codex-multi-auth-codex "$@" - fi - - session="${MCODEX_TMUX_SESSION:-mcodex}" - suffix="$(date +%H%M%S)" - if tmux has-session -t "$session" 2>/dev/null; then - session="${session}-${suffix}" - fi - - # Build the inner shell-command as a single string. quote_args runs each arg - # through printf %q, so every token is already shell-safe (embedded quotes, - # spaces, $, backticks are escaped); the surrounding expansion is not - # re-evaluated, so the command tmux receives is exactly this string. - cmd="codex-multi-auth-codex" - args="$(quote_args "$@")" - if [[ -n "$args" ]]; then - cmd="$cmd $args" - fi - tmux new-session -d -s "$session" -n codex "$cmd" - configure_tmux_scrollback "$session" - if [[ "$live_accounts" == "1" ]] && require_watch; then - tmux split-window -h -t "$session:0" "watch -n $monitor_interval 'codex-multi-auth list'" - fi - tmux select-pane -t "$session:0.0" - tmux attach-session -t "$session" - exit 0 -fi - -exec codex-multi-auth-codex "$@" diff --git a/scripts/mcodex.js b/scripts/mcodex.js new file mode 100644 index 000000000..c35ea3d0e --- /dev/null +++ b/scripts/mcodex.js @@ -0,0 +1,315 @@ +#!/usr/bin/env node + +import { spawn, spawnSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { dirname, join, resolve as resolvePath } from "node:path"; +import process from "node:process"; +import { fileURLToPath } from "node:url"; +import { splitPathEntries } from "./codex-bin-resolver.js"; + +// Cross-platform replacement for the legacy `scripts/mcodex` bash launcher. +// The bash shim was shipped as a Windows `bin`; npm's generated mcodex.cmd/.ps1 +// invoked bare `bash`, and when a WSL stub resolved before git-bash on PATH the +// launcher died with HCS_E_SERVICE_NOT_AVAILABLE. This node entry forwards to the +// sibling codex wrapper with zero bash dependency, and only reaches for the POSIX +// tools (`tmux`/`watch`) when they actually exist — otherwise it degrades with the +// same friendly messages the bash version printed. + +const DEFAULT_MONITOR_INTERVAL = "5"; +const DEFAULT_TMUX_HISTORY_LIMIT = "50000"; +const DEFAULT_TMUX_SESSION = "mcodex"; + +// Security: monitor_interval is interpolated into the `watch -n ...` argument +// and tmux_history_limit into a tmux option argument. Require a positive (optionally +// fractional) number / plain integer and fall back to the default otherwise, so no +// shell metacharacters can ever reach a command — preserving the bash hardening. +const MONITOR_INTERVAL_PATTERN = /^[0-9]+(\.[0-9]+)?$/; +const TMUX_HISTORY_LIMIT_PATTERN = /^[0-9]+$/; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); +// Spawn the sibling codex wrapper directly through `node` (process.execPath) rather +// than relying on the installed `codex-multi-auth-codex` shim being on PATH. This is +// the same idiom codex-app-launcher.js uses to launch codex.js and is what makes the +// default forward work on Windows without any bash/.cmd resolution. +const codexWrapperScript = join(scriptDir, "codex.js"); + +function defaultWarn(message) { + console.error(message); +} + +function coerceValidatedSetting(rawValue, pattern, fallback, envName, warn) { + // Mirror bash `${VAR:-default}`: unset OR empty falls back silently; any other + // value is validated and, if it fails, replaced with the default plus a warning. + const value = rawValue === undefined || rawValue === "" ? fallback : rawValue; + if (!pattern.test(value)) { + warn(`mcodex: invalid ${envName} '${value}'; using ${fallback}`); + return fallback; + } + return value; +} + +export function resolveMonitorInterval(env = process.env, warn = defaultWarn) { + return coerceValidatedSetting( + env.MCODEX_MONITOR_INTERVAL, + MONITOR_INTERVAL_PATTERN, + DEFAULT_MONITOR_INTERVAL, + "MCODEX_MONITOR_INTERVAL", + warn, + ); +} + +export function resolveTmuxHistoryLimit(env = process.env, warn = defaultWarn) { + return coerceValidatedSetting( + env.MCODEX_TMUX_HISTORY_LIMIT, + TMUX_HISTORY_LIMIT_PATTERN, + DEFAULT_TMUX_HISTORY_LIMIT, + "MCODEX_TMUX_HISTORY_LIMIT", + warn, + ); +} + +export function resolveTmuxSession(env = process.env) { + const configured = (env.MCODEX_TMUX_SESSION ?? "").trim(); + return configured.length > 0 ? configured : DEFAULT_TMUX_SESSION; +} + +/** + * Parse the launcher flags the same way the bash script did: the FIRST argument + * selects a mode (--monitor, or --tmux/-t), and only when --tmux/-t is present is + * a following --live-accounts consumed. Every remaining token is passthrough for + * the codex wrapper. Returns the mode plus the forwarded args. + * + * @param {string[]} argv + */ +export function parseMcodexArgs(argv) { + const first = argv[0] ?? ""; + if (first === "--monitor") { + // --monitor takes no extra args; the live account list is fixed. + return { mode: "monitor", liveAccounts: false, forwardArgs: argv.slice(1) }; + } + if (first === "--tmux" || first === "-t") { + let rest = argv.slice(1); + let liveAccounts = false; + if (rest[0] === "--live-accounts") { + liveAccounts = true; + rest = rest.slice(1); + } + return { mode: "tmux", liveAccounts, forwardArgs: rest }; + } + return { mode: "forward", liveAccounts: false, forwardArgs: argv }; +} + +// Resolve a POSIX helper (tmux/watch) on PATH ourselves rather than shelling out, +// so we never depend on bash. Mirrors codex-bin-resolver's PATH handling. +function resolvePosixToolOnPath(tool, env = process.env, platform = process.platform) { + const names = platform === "win32" ? [`${tool}.exe`, `${tool}.cmd`, tool] : [tool]; + for (const entry of splitPathEntries(env.PATH ?? env.Path ?? "")) { + for (const name of names) { + const candidate = join(entry, name); + if (existsSync(candidate)) { + return candidate; + } + } + } + return null; +} + +function hasPosixTool(tool, env = process.env, platform = process.platform) { + return resolvePosixToolOnPath(tool, env, platform) !== null; +} + +// Build the watch argv as an array (NOT a shell string): `watch -n +// codex-multi-auth list`. Because the interval is validated to digits/dot and the +// command is passed as discrete argv tokens, no shell interpolation occurs — this +// is strictly safer than the bash `printf %q` approach. +function buildWatchArgs(interval) { + return ["-n", interval, "codex-multi-auth", "list"]; +} + +function forwardToCodexWrapper(forwardArgs, env = process.env) { + const child = spawn(process.execPath, [codexWrapperScript, ...forwardArgs], { + stdio: "inherit", + env, + }); + child.once("error", (error) => { + console.error( + `mcodex: failed to launch codex wrapper: ${error instanceof Error ? error.message : String(error)}`, + ); + process.exit(1); + }); + child.once("close", (code, signal) => { + if (signal === "SIGINT") { + process.exit(130); + return; + } + process.exit(typeof code === "number" ? code : 1); + }); +} + +function runMonitor(interval, env = process.env, platform = process.platform) { + const watchPath = resolvePosixToolOnPath("watch", env, platform); + if (!watchPath) { + console.error( + "mcodex: 'watch' is not installed; the live account monitor requires it (install procps / procps-ng).", + ); + process.exit(1); + return; + } + const child = spawn(watchPath, buildWatchArgs(interval), { stdio: "inherit", env }); + child.once("error", (error) => { + console.error( + `mcodex: failed to launch watch: ${error instanceof Error ? error.message : String(error)}`, + ); + process.exit(1); + }); + child.once("close", (code, signal) => { + if (signal === "SIGINT") { + process.exit(130); + return; + } + process.exit(typeof code === "number" ? code : 1); + }); +} + +// Run a tmux subcommand synchronously, swallowing output exactly like the bash +// `>/dev/null 2>&1`. argv-only, so the validated history-limit/session never reach +// a shell. +function runTmux(tmuxPath, args, env = process.env) { + return spawnSync(tmuxPath, args, { + stdio: ["ignore", "ignore", "ignore"], + env, + }); +} + +function configureTmuxScrollback(tmuxPath, historyLimit, target, env = process.env) { + const targetArgs = target ? ["-t", target] : []; + runTmux(tmuxPath, ["set-option", ...targetArgs, "mouse", "on"], env); + runTmux(tmuxPath, ["set-option", ...targetArgs, "history-limit", historyLimit], env); + runTmux(tmuxPath, ["bind-key", "-T", "root", "WheelUpPane", "copy-mode", "-e"], env); + runTmux(tmuxPath, ["bind-key", "-T", "copy-mode", "WheelUpPane", "send-keys", "-X", "scroll-up"], env); + runTmux(tmuxPath, ["bind-key", "-T", "copy-mode", "WheelDownPane", "send-keys", "-X", "scroll-down"], env); + runTmux(tmuxPath, ["bind-key", "-T", "copy-mode-vi", "WheelUpPane", "send-keys", "-X", "scroll-up"], env); + runTmux(tmuxPath, ["bind-key", "-T", "copy-mode-vi", "WheelDownPane", "send-keys", "-X", "scroll-down"], env); +} + +function warnWatchMissing() { + console.error( + "mcodex: 'watch' is not installed; the live account monitor requires it (install procps / procps-ng).", + ); +} + +function formatTmuxSessionSuffix(date = new Date()) { + const pad = (value) => String(value).padStart(2, "0"); + return `${pad(date.getHours())}${pad(date.getMinutes())}${pad(date.getSeconds())}`; +} + +// The codex command tmux should run inside a pane. Use node + the sibling wrapper +// (same as the default forward) so the pane never depends on the installed +// `codex-multi-auth-codex` shim being resolvable. +function buildCodexPaneCommand(forwardArgs) { + return [process.execPath, codexWrapperScript, ...forwardArgs]; +} + +function runTmuxMode(options) { + const { liveAccounts, forwardArgs, interval, historyLimit, session, env, platform } = + options; + + const tmuxPath = resolvePosixToolOnPath("tmux", env, platform); + if (!tmuxPath) { + // Degrade exactly like the bash version: warn, then forward without tmux. + console.error("mcodex: tmux is not installed; launching without tmux"); + forwardToCodexWrapper(forwardArgs, env); + return; + } + + const watchAvailable = liveAccounts ? hasPosixTool("watch", env, platform) : false; + if (liveAccounts && !watchAvailable) { + warnWatchMissing(); + } + + // Already inside a tmux client: configure the current session, optionally add a + // live-accounts pane, then run codex in the current pane (forward + exit code). + if ((env.TMUX ?? "").length > 0) { + configureTmuxScrollback(tmuxPath, historyLimit, undefined, env); + if (liveAccounts && watchAvailable) { + runTmux(tmuxPath, ["split-window", "-h", "watch", ...buildWatchArgs(interval)], env); + } + forwardToCodexWrapper(forwardArgs, env); + return; + } + + // Not inside tmux: create a fresh detached session, configure it, optionally add + // the live pane, then attach (inheriting the terminal) and propagate the code. + let targetSession = session; + if (runTmux(tmuxPath, ["has-session", "-t", targetSession], env).status === 0) { + targetSession = `${targetSession}-${formatTmuxSessionSuffix()}`; + } + runTmux( + tmuxPath, + ["new-session", "-d", "-s", targetSession, "-n", "codex", ...buildCodexPaneCommand(forwardArgs)], + env, + ); + configureTmuxScrollback(tmuxPath, historyLimit, targetSession, env); + if (liveAccounts && watchAvailable) { + runTmux( + tmuxPath, + ["split-window", "-h", "-t", `${targetSession}:0`, "watch", ...buildWatchArgs(interval)], + env, + ); + } + runTmux(tmuxPath, ["select-pane", "-t", `${targetSession}:0.0`], env); + const attach = spawn(tmuxPath, ["attach-session", "-t", targetSession], { + stdio: "inherit", + env, + }); + attach.once("error", (error) => { + console.error( + `mcodex: failed to attach tmux session: ${error instanceof Error ? error.message : String(error)}`, + ); + process.exit(1); + }); + attach.once("close", (code, signal) => { + if (signal === "SIGINT") { + process.exit(130); + return; + } + process.exit(typeof code === "number" ? code : 0); + }); +} + +export function runMcodex(argv = process.argv.slice(2), env = process.env, platform = process.platform) { + const { mode, liveAccounts, forwardArgs } = parseMcodexArgs(argv); + const interval = resolveMonitorInterval(env); + const historyLimit = resolveTmuxHistoryLimit(env); + + if (mode === "monitor") { + runMonitor(interval, env, platform); + return; + } + if (mode === "tmux") { + runTmuxMode({ + liveAccounts, + forwardArgs, + interval, + historyLimit, + session: resolveTmuxSession(env), + env, + platform, + }); + return; + } + forwardToCodexWrapper(forwardArgs, env); +} + +const isDirectRun = (() => { + try { + return resolvePath(process.argv[1] ?? "") === fileURLToPath(import.meta.url); + } catch { + return false; + } +})(); + +if (isDirectRun) { + runMcodex(); +} + diff --git a/test/account-persistence.test.ts b/test/account-persistence.test.ts index c7da5e4fa..c61a87b4c 100644 --- a/test/account-persistence.test.ts +++ b/test/account-persistence.test.ts @@ -24,4 +24,33 @@ describe("account persistence helper", () => { activeIndexByFamily: {}, }); }); + + it("preserves pinnedAccountIndex and affinityGeneration when defined", () => { + // Regression: the clone previously dropped these fields, erasing a user's + // manual `switch ` pin when persisted through the combined transaction. + const original = { + version: 3 as const, + accounts: [{ refreshToken: "a" }], + activeIndex: 1, + activeIndexByFamily: { codex: 1 }, + pinnedAccountIndex: 1, + affinityGeneration: 7, + }; + + const cloned = cloneAccountStorageForPersistence(original); + expect(cloned.pinnedAccountIndex).toBe(1); + expect(cloned.affinityGeneration).toBe(7); + expect(cloned).toEqual(original); + }); + + it("omits pin/generation fields when absent rather than emitting undefined", () => { + const cloned = cloneAccountStorageForPersistence({ + version: 3, + accounts: [], + activeIndex: 0, + activeIndexByFamily: {}, + }); + expect("pinnedAccountIndex" in cloned).toBe(false); + expect("affinityGeneration" in cloned).toBe(false); + }); }); diff --git a/test/codex-manager-detail-tone.test.ts b/test/codex-manager-detail-tone.test.ts index dc5cd0a78..83486f096 100644 --- a/test/codex-manager-detail-tone.test.ts +++ b/test/codex-manager-detail-tone.test.ts @@ -69,4 +69,15 @@ describe("styleAccountDetailText tone precedence", () => { expect(styled).toContain(ANSI.red); expect(styled).not.toContain(ANSI.yellow); }); + + it("does not render a 'working' prefix green when the failure is inside the quota segment", () => { + // Reachable from runHealthCheck: a failed live probe still emits a quota + // percent, so "failed" is trapped inside the (…%) parens while the prefix + // reads "working". The prefix must NOT render green over a real failure. + const styled = styleAccountDetailText( + "signed in and working (live check failed: timeout 0%)", + ); + expect(styled).toContain(ANSI.red); + expect(styled).not.toContain(ANSI.green); + }); }); diff --git a/test/codex-manager-workspace-command.test.ts b/test/codex-manager-workspace-command.test.ts index a2dd19ff0..e33c05328 100644 --- a/test/codex-manager-workspace-command.test.ts +++ b/test/codex-manager-workspace-command.test.ts @@ -80,6 +80,34 @@ describe("runWorkspaceCommand", () => { expect(deps.saveAccounts).not.toHaveBeenCalled(); }); + it("rejects a fractional account index instead of truncating it", async () => { + // "1.9" must NOT be silently parsed as account 1 (parseInt truncation). + const deps = createDeps(); + const result = await runWorkspaceCommand(["1.9"], deps); + expect(result).toBe(1); + expect(deps.logError).toHaveBeenCalledWith("Invalid account index: 1.9"); + expect(deps.saveAccounts).not.toHaveBeenCalled(); + }); + + it("rejects an account index with trailing garbage instead of truncating it", async () => { + const deps = createDeps(); + const result = await runWorkspaceCommand(["2abc", "1"], deps); + expect(result).toBe(1); + expect(deps.logError).toHaveBeenCalledWith("Invalid account index: 2abc"); + expect(deps.saveAccounts).not.toHaveBeenCalled(); + }); + + it("rejects a fractional workspace index instead of truncating it", async () => { + // "2.9" must NOT be silently parsed as workspace 2. + const deps = createDeps(); + const result = await runWorkspaceCommand(["1", "2.9"], deps); + expect(result).toBe(1); + expect(deps.logError).toHaveBeenCalledWith( + "Invalid workspace index. Valid range: 1-2", + ); + expect(deps.saveAccounts).not.toHaveBeenCalled(); + }); + it("lists workspaces with the active one marked when no workspace arg", async () => { const deps = createDeps(); const result = await runWorkspaceCommand(["1"], deps); diff --git a/test/documentation.test.ts b/test/documentation.test.ts index 2c656c32e..69c05149d 100644 --- a/test/documentation.test.ts +++ b/test/documentation.test.ts @@ -579,7 +579,7 @@ describe("Documentation Integrity", () => { url: "https://github.com/ndycode/codex-multi-auth/issues", }); expect(packageJson.bin).toEqual({ - mcodex: "scripts/mcodex", + mcodex: "scripts/mcodex.js", "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", diff --git a/test/fetch-helpers.test.ts b/test/fetch-helpers.test.ts index d6c17d21c..e7c165312 100644 --- a/test/fetch-helpers.test.ts +++ b/test/fetch-helpers.test.ts @@ -822,6 +822,34 @@ describe('createEntitlementErrorResponse', () => { expect(info.unsupportedModel).toBe('gpt-5.3-codex-spark'); }); + it('flags normalized "not currently available" wording as unsupported in the nested-error branch', () => { + // request-fetch: the nested-error message uses the normalized + // "not currently available for this chatgpt account" wording without the + // unsupported code; it must still be classified as unsupported (not a + // transient error) so model fallback engages. + const info = getUnsupportedCodexModelInfo({ + error: { + message: + "The model 'gpt-5.3-codex' is not currently available for this chatgpt account.", + }, + }); + + expect(info.isUnsupported).toBe(true); + expect(info.unsupportedModel).toBe('gpt-5.3-codex'); + }); + + it('flags normalized "not currently available" wording as unsupported in the flat-detail branch', () => { + // Same wording delivered via the flat `{ detail: "..." }` envelope used by + // the quota endpoint must also be treated as unsupported. + const info = getUnsupportedCodexModelInfo({ + detail: + "The model 'gpt-5.3-codex' is not currently available for this chatgpt account.", + }); + + expect(info.isUnsupported).toBe(true); + expect(info.unsupportedModel).toBe('gpt-5.3-codex'); + }); + it('resolves Spark fallback chain to current gpt-5.3-codex first', () => { const errorBody = { error: { diff --git a/test/forecast.test.ts b/test/forecast.test.ts index 991495a89..d6deee944 100644 --- a/test/forecast.test.ts +++ b/test/forecast.test.ts @@ -710,4 +710,45 @@ describe("forecast helpers", () => { "No healthy accounts are available", ); }); + + it("returns null recommendation when all accounts are policy-blocked/exhausted", () => { + // Blocked/exhausted accounts report availability === "unavailable" with + // hardFailure === false and disabled === false. They must not be + // recommended with a misleading "pick shortest wait". + const now = 1_700_000_000_000; + const results = evaluateForecastAccounts([ + { + index: 0, + now, + isCurrent: true, + account: { + refreshToken: "a", + addedAt: now - 1_000, + lastUsed: now - 1_000, + }, + runtimeOverlay: { policyBlockedIndexes: [0] }, + }, + { + index: 1, + now, + isCurrent: false, + account: { + refreshToken: "b", + addedAt: now - 1_000, + lastUsed: now - 1_000, + }, + runtimeOverlay: { + lastPoolExhaustionSkipReasons: { "1": "token-exhausted" }, + }, + }, + ]); + + // Sanity: both are unavailable but neither disabled nor hard-failed. + expect(results.every((r) => r.availability === "unavailable")).toBe(true); + expect(results.every((r) => !r.disabled && !r.hardFailure)).toBe(true); + + const recommendation = recommendForecastAccount(results); + expect(recommendation.recommendedIndex).toBeNull(); + expect(recommendation.reason).toContain("blocked or exhausted"); + }); }); diff --git a/test/mcodex-launcher.test.ts b/test/mcodex-launcher.test.ts index 666a49a5d..7f3b82c79 100644 --- a/test/mcodex-launcher.test.ts +++ b/test/mcodex-launcher.test.ts @@ -1,107 +1,124 @@ -import { spawnSync } from "node:child_process"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; +import { + parseMcodexArgs, + resolveMonitorInterval, + resolveTmuxHistoryLimit, + resolveTmuxSession, +} from "../scripts/mcodex.js"; -// Security regression for the mcodex launcher (scripts/mcodex). The monitor -// interval is interpolated into `watch -n ...` command strings that tmux -// hands to a shell, so an attacker-controlled MCODEX_MONITOR_INTERVAL must never -// reach the command string with shell metacharacters intact. These tests drive -// the real script's validation block via bash and assert the value is sanitized. -// -// Skipped automatically where bash is unavailable (e.g. a bare Windows runner). +// H1 regression: scripts/mcodex was a `#!/usr/bin/env bash` script shipped as a +// Windows bin. npm's generated mcodex.cmd/.ps1 shim invoked bare `bash`; when a WSL +// stub resolved before git-bash on PATH the launcher died with +// HCS_E_SERVICE_NOT_AVAILABLE. It is now a node entry (scripts/mcodex.js) with zero +// bash dependency. These tests exercise the node launcher's arg/env parsing and the +// injection-hardening validation that the bash prologue used to own. const testFileDir = dirname(fileURLToPath(import.meta.url)); -const mcodexPath = join(testFileDir, "..", "scripts", "mcodex"); +const mcodexPath = join(testFileDir, "..", "scripts", "mcodex.js"); -function hasBash(): boolean { - const probe = spawnSync("bash", ["-c", "echo ok"], { encoding: "utf-8" }); - return probe.status === 0 && /ok/.test(probe.stdout ?? ""); +function captureWarnings(run: (warn: (message: string) => void) => string): { + value: string; + warnings: string[]; +} { + const warnings: string[] = []; + const value = run((message) => warnings.push(message)); + return { value, warnings }; } -// Extract and run ONLY the interval-validation prologue from the real script so -// the test exercises the shipped regex, not a copy. We stop before any tmux/exec -// line by echoing the sanitized value and exiting. -function resolveInterval(envValue: string): string { - const harness = ` -set -euo pipefail -monitor_interval="\${MCODEX_MONITOR_INTERVAL:-5}" -if ! [[ "$monitor_interval" =~ ^[0-9]+(\\.[0-9]+)?$ ]]; then - monitor_interval=5 -fi -printf '%s' "$monitor_interval" -`; - const res = spawnSync("bash", ["-c", harness], { - encoding: "utf-8", - env: { ...process.env, MCODEX_MONITOR_INTERVAL: envValue }, - }); - return (res.stdout ?? "").trim(); -} - -describe.runIf(hasBash())("mcodex monitor-interval hardening", () => { +describe("mcodex monitor-interval hardening", () => { it("passes through a valid integer interval", () => { - expect(resolveInterval("10")).toBe("10"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "10" })).toBe("10"); }); it("passes through a valid fractional interval", () => { - expect(resolveInterval("2.5")).toBe("2.5"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "2.5" })).toBe("2.5"); + }); + + it("falls back to 5 when unset or empty", () => { + expect(resolveMonitorInterval({})).toBe("5"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "" })).toBe("5"); }); it("neutralizes a command-injection payload to the safe default", () => { - // "5; touch PWNED" must NOT survive — it is rejected and replaced with 5. - expect(resolveInterval("5; touch PWNED")).toBe("5"); + const { value, warnings } = captureWarnings((warn) => + resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "5; touch PWNED" }, warn), + ); + expect(value).toBe("5"); + expect(warnings.join("\n")).toMatch(/invalid MCODEX_MONITOR_INTERVAL/); }); it("neutralizes shell metacharacters and substitutions", () => { - expect(resolveInterval("$(echo evil)")).toBe("5"); - expect(resolveInterval("`id`")).toBe("5"); - expect(resolveInterval("5 && rm -rf ~")).toBe("5"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "$(echo evil)" }, () => {})).toBe("5"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "`id`" }, () => {})).toBe("5"); + expect(resolveMonitorInterval({ MCODEX_MONITOR_INTERVAL: "5 && rm -rf ~" }, () => {})).toBe("5"); + }); +}); + +describe("mcodex tmux-history-limit hardening", () => { + it("passes through a valid integer", () => { + expect(resolveTmuxHistoryLimit({ MCODEX_TMUX_HISTORY_LIMIT: "12345" })).toBe("12345"); }); - it("the shipped script actually contains the numeric guard", () => { - // Guards against the validation being removed in a future edit. - const res = spawnSync("bash", ["-c", `cat "${mcodexPath}"`], { - encoding: "utf-8", - }); - expect(res.stdout).toMatch(/\^\[0-9\]\+\(\\\.\[0-9\]\+\)\?\$/); + it("falls back to 50000 when unset or empty", () => { + expect(resolveTmuxHistoryLimit({})).toBe("50000"); + expect(resolveTmuxHistoryLimit({ MCODEX_TMUX_HISTORY_LIMIT: "" })).toBe("50000"); }); - it("--monitor fails fast with a clear error when `watch` is missing", () => { - // Drive the real require_watch + run_monitor logic with `watch` forced - // absent by shadowing the `command` builtin so `command -v watch` reports - // missing. This exercises the actual guard wording/exit without PATH surgery - // (a minimal PATH would also strip bash's own core utilities). - const harness = ` -set -euo pipefail -require_watch() { - if ! command -v watch >/dev/null 2>&1; then - echo "mcodex: 'watch' is not installed; the live account monitor requires it (install procps / procps-ng)." >&2 - return 1 - fi -} -run_monitor() { - require_watch || return 1 - watch -n 5 'codex-multi-auth list' -} -# Force 'watch' to look uninstalled regardless of the host. -command() { if [ "\${2:-}" = "watch" ]; then return 1; fi; builtin command "$@"; } -run_monitor -exit $? -`; - const res = spawnSync("bash", ["-c", harness], { encoding: "utf-8" }); - expect(res.status).not.toBe(0); - expect(`${res.stderr}`).toMatch(/watch.*not installed|requires it/i); + it("rejects fractional and metacharacter values", () => { + expect(resolveTmuxHistoryLimit({ MCODEX_TMUX_HISTORY_LIMIT: "2.5" }, () => {})).toBe("50000"); + const { value, warnings } = captureWarnings((warn) => + resolveTmuxHistoryLimit({ MCODEX_TMUX_HISTORY_LIMIT: "50000; rm -rf ~" }, warn), + ); + expect(value).toBe("50000"); + expect(warnings.join("\n")).toMatch(/invalid MCODEX_TMUX_HISTORY_LIMIT/); }); +}); + +describe("mcodex session + arg parsing", () => { + it("defaults the tmux session and honors an override", () => { + expect(resolveTmuxSession({})).toBe("mcodex"); + expect(resolveTmuxSession({ MCODEX_TMUX_SESSION: "work" })).toBe("work"); + }); + + it("routes a plain invocation to the forward mode with all args", () => { + const parsed = parseMcodexArgs(["exec", "--model", "gpt-5.3-codex"]); + expect(parsed.mode).toBe("forward"); + expect(parsed.liveAccounts).toBe(false); + expect(parsed.forwardArgs).toEqual(["exec", "--model", "gpt-5.3-codex"]); + }); + + it("parses --monitor with no passthrough args", () => { + const parsed = parseMcodexArgs(["--monitor"]); + expect(parsed.mode).toBe("monitor"); + expect(parsed.forwardArgs).toEqual([]); + }); + + it("parses --tmux and -t, consuming --live-accounts only after the flag", () => { + const longForm = parseMcodexArgs(["--tmux", "--live-accounts", "exec"]); + expect(longForm.mode).toBe("tmux"); + expect(longForm.liveAccounts).toBe(true); + expect(longForm.forwardArgs).toEqual(["exec"]); + + const shortForm = parseMcodexArgs(["-t", "resume"]); + expect(shortForm.mode).toBe("tmux"); + expect(shortForm.liveAccounts).toBe(false); + expect(shortForm.forwardArgs).toEqual(["resume"]); + }); + + it("does not treat --live-accounts as a mode on its own", () => { + const parsed = parseMcodexArgs(["--live-accounts"]); + expect(parsed.mode).toBe("forward"); + expect(parsed.forwardArgs).toEqual(["--live-accounts"]); + }); +}); - it("the shipped script wires require_watch into every watch invocation", () => { - // Static guard: all three live-monitor sites must be gated, so a future edit - // that drops the runtime check is caught. - const res = spawnSync("bash", ["-c", `cat "${mcodexPath}"`], { encoding: "utf-8" }); - const src = res.stdout ?? ""; - expect(src).toContain("require_watch() {"); - // run_monitor guards; both tmux live panes guard via `&& require_watch`. - expect(src).toMatch(/require_watch \|\| return 1/); - const guardedPanes = src.match(/"\$live_accounts" == "1" \]\] && require_watch/g) ?? []; - expect(guardedPanes.length).toBe(2); +describe("mcodex ships as a node entry, not bash", () => { + it("uses a node shebang so npm's Windows shim never invokes bash", async () => { + const { readFile } = await import("node:fs/promises"); + const source = await readFile(mcodexPath, "utf8"); + expect(source.startsWith("#!/usr/bin/env node")).toBe(true); + expect(source).not.toContain("#!/usr/bin/env bash"); }); }); diff --git a/test/model-capability-matrix.test.ts b/test/model-capability-matrix.test.ts index 316e289c7..a8d515569 100644 --- a/test/model-capability-matrix.test.ts +++ b/test/model-capability-matrix.test.ts @@ -53,11 +53,14 @@ describe("model capability matrix", () => { it("marks capability policy and quota cache issues unavailable", () => { const capabilityPolicy = new CapabilityPolicyStore(); - const accountKey = buildModelCapabilityMatrix({ - storage: storage(), - models: ["gpt-5.3-codex"], - }).entries[0]!.accountKey; - capabilityPolicy.recordUnsupported(accountKey, "gpt-5.3-codex", 100); + // The capability store is keyed by the entitlement account key, which is + // the same key the matrix now reads with (regression for quota-forecast-01). + const entitlementKey = resolveEntitlementAccountKey({ + accountId: "acct_1", + email: "owner@example.com", + index: 0, + }); + capabilityPolicy.recordUnsupported(entitlementKey, "gpt-5.3-codex", 100); const matrix = buildModelCapabilityMatrix({ storage: storage(), models: ["gpt-5.3-codex"], @@ -83,6 +86,33 @@ describe("model capability matrix", () => { expect(matrix.entries[0]?.reasons).toContain("quota cache is rate-limited"); }); + it("surfaces capability snapshots recorded under the entitlement key", () => { + // quota-forecast-01 regression: recordUnsupported writes under the + // entitlement key, so the matrix must read capabilityPolicy with that same + // key (not the sha256 getAccountPolicyKey) for the snapshot to surface. + const capabilityPolicy = new CapabilityPolicyStore(); + const entitlementKey = resolveEntitlementAccountKey({ + accountId: "acct_1", + email: "owner@example.com", + index: 0, + }); + capabilityPolicy.recordUnsupported(entitlementKey, "gpt-5.3-codex", 100); + + const matrix = buildModelCapabilityMatrix({ + storage: storage(), + models: ["gpt-5.3-codex"], + capabilityPolicy, + now: 100, + }); + + expect(matrix.entries[0]?.capabilityPolicy).not.toBeNull(); + expect(matrix.entries[0]?.capabilityPolicy?.unsupported).toBeGreaterThan(0); + expect(matrix.entries[0]?.available).toBe(false); + expect(matrix.entries[0]?.reasons).toContain( + "capability policy has unsupported failures", + ); + }); + it("marks disabled and entitlement-blocked accounts unavailable", () => { const baseStorage = storage(); baseStorage.accounts[0] = { diff --git a/test/server.unit.test.ts b/test/server.unit.test.ts index ebf10dac9..5695615f4 100644 --- a/test/server.unit.test.ts +++ b/test/server.unit.test.ts @@ -50,6 +50,27 @@ import http from 'node:http'; import { startLocalOAuthServer } from '../lib/auth/server.js'; import { logError, logWarn } from '../lib/logger.js'; +function createMockRequest(url: string): IncomingMessage { + const req = new EventEmitter() as IncomingMessage; + req.url = url; + return req; +} + +function createMockResponse(): ServerResponse & { _body: string; _headers: Record } { + const res = { + statusCode: 200, + _body: '', + _headers: {} as Record, + setHeader: vi.fn((name: string, value: string) => { + res._headers[name.toLowerCase()] = value; + }), + end: vi.fn((body?: string) => { + if (body) res._body = body; + }), + }; + return res as unknown as ServerResponse & { _body: string; _headers: Record }; +} + describe('OAuth Server Unit Tests', () => { let mockServer: ReturnType & { _handler?: (req: IncomingMessage, res: ServerResponse) => void; @@ -123,27 +144,6 @@ describe('OAuth Server Unit Tests', () => { requestHandler = mockServer._handler!; }); - function createMockRequest(url: string): IncomingMessage { - const req = new EventEmitter() as IncomingMessage; - req.url = url; - return req; - } - - function createMockResponse(): ServerResponse & { _body: string; _headers: Record } { - const res = { - statusCode: 200, - _body: '', - _headers: {} as Record, - setHeader: vi.fn((name: string, value: string) => { - res._headers[name.toLowerCase()] = value; - }), - end: vi.fn((body?: string) => { - if (body) res._body = body; - }), - }; - return res as unknown as ServerResponse & { _body: string; _headers: Record }; - } - it('should return 404 for non-callback paths', () => { const req = createMockRequest('/other-path'); const res = createMockResponse(); @@ -191,30 +191,60 @@ describe('OAuth Server Unit Tests', () => { expect(res.end).toHaveBeenCalledWith('Success'); }); - it('should store the code in server._lastCode', () => { + it('should store the captured code, surfaced via waitForCode', async () => { + const serverInfo = await startLocalOAuthServer({ state: 'test-state' }); + const handler = mockServer._handler!; const req = createMockRequest('/auth/callback?code=captured-code&state=test-state'); const res = createMockResponse(); - requestHandler(req, res); + handler(req, res); - expect(mockServer._lastCode).toBe('captured-code'); + expect(await serverInfo.waitForCode('test-state')).toEqual({ + code: 'captured-code', + }); }); - it('should keep the first code when duplicate callbacks arrive', () => { + it('should keep the first code when duplicate callbacks arrive', async () => { + const serverInfo = await startLocalOAuthServer({ state: 'test-state' }); + const handler = mockServer._handler!; const firstReq = createMockRequest('/auth/callback?code=first-code&state=test-state'); const secondReq = createMockRequest('/auth/callback?code=second-code&state=test-state'); const firstRes = createMockResponse(); const secondRes = createMockResponse(); - requestHandler(firstReq, firstRes); - requestHandler(secondReq, secondRes); + handler(firstReq, firstRes); + handler(secondReq, secondRes); - expect(mockServer._lastCode).toBe('first-code'); + expect(await serverInfo.waitForCode('test-state')).toEqual({ + code: 'first-code', + }); expect(logWarn).toHaveBeenCalledWith( expect.stringContaining('Duplicate OAuth callback received'), ); }); + it('keeps capture state isolated between two concurrent server instances', async () => { + const serverA = await startLocalOAuthServer({ state: 'state-a' }); + const handlerA = mockServer._handler!; + const serverB = await startLocalOAuthServer({ state: 'state-b' }); + const handlerB = mockServer._handler!; + + // Deliver each instance's callback through its own request handler. + handlerA( + createMockRequest('/auth/callback?code=code-a&state=state-a'), + createMockResponse(), + ); + handlerB( + createMockRequest('/auth/callback?code=code-b&state=state-b'), + createMockResponse(), + ); + + // Each server must surface only its own code; previously both codes were + // stored on the shared server object and cross-bound. + expect(await serverA.waitForCode('state-a')).toEqual({ code: 'code-a' }); + expect(await serverB.waitForCode('state-b')).toEqual({ code: 'code-b' }); + }); + it('should handle request handler errors gracefully', () => { const req = createMockRequest('/auth/callback?code=test&state=test-state'); const res = createMockResponse(); @@ -301,9 +331,14 @@ describe('OAuth Server Unit Tests', () => { (mockServer.on as ReturnType).mockReturnValue(mockServer); const result = await startLocalOAuthServer({ state: 'test-state' }); - - mockServer._lastCode = 'the-code'; - + + // Deliver the code through the request handler (closure capture) rather + // than poking server internals. + mockServer._handler!( + createMockRequest('/auth/callback?code=the-code&state=test-state'), + createMockResponse(), + ); + const code = await result.waitForCode('test-state'); expect(code).toEqual({ code: 'the-code' }); }); @@ -317,10 +352,14 @@ describe('OAuth Server Unit Tests', () => { ); (mockServer.on as ReturnType).mockReturnValue(mockServer); - const result = await startLocalOAuthServer({ state: 'test-state' }); + // The server validates against 'other-state'; a callback for that state + // is captured, but waitForCode is asked for 'test-state'. + const result = await startLocalOAuthServer({ state: 'other-state' }); - mockServer._lastCode = 'the-code'; - mockServer._lastState = 'other-state'; + mockServer._handler!( + createMockRequest('/auth/callback?code=the-code&state=other-state'), + createMockResponse(), + ); const code = await result.waitForCode('test-state'); expect(code).toBeNull(); diff --git a/test/storage-flagged.test.ts b/test/storage-flagged.test.ts index 85dfbed13..f72db4eca 100644 --- a/test/storage-flagged.test.ts +++ b/test/storage-flagged.test.ts @@ -10,6 +10,7 @@ import { loadFlaggedAccounts, saveFlaggedAccounts, setStoragePathDirect, + withAccountAndFlaggedStorageTransaction, } from "../lib/storage.js"; import { loadFlaggedAccountsFromFile } from "../lib/storage/flagged-storage-file.js"; import { describeFlaggedSnapshot } from "../lib/storage/snapshot-inspectors.js"; @@ -1069,4 +1070,74 @@ describe("flagged storage extracted helpers", () => { await removeWithRetry(fixtureRoot, { recursive: true, force: true }); } }); + + it("recovers a flagged .bak without deadlocking inside a held storage lock", async () => { + // H4 regression: loadFlaggedAccounts' recovery path persists the recovered + // backup via persistRecoveredBackup. When loadFlaggedAccounts runs inside + // withAccountAndFlaggedStorageTransaction (which already holds the global + // storage lock), re-acquiring the lock there deadlocks. Trigger the doctor + // restore path: flagged primary absent + flagged .bak with accounts. + const { cloneAccountStorageForPersistence } = await import( + "../lib/storage/account-persistence.js" + ); + const flaggedPath = getFlaggedAccountsPath(); + const backupPath = `${flaggedPath}.bak`; + await fs.mkdir(dirname(flaggedPath), { recursive: true }); + // Ensure the primary flagged file is absent; only the .bak exists. + await removeWithRetry(flaggedPath, { force: true }); + await fs.writeFile( + backupPath, + JSON.stringify({ + version: 1, + accounts: [ + { + refreshToken: "recovered-token", + accountId: "acct-recovered", + flaggedAt: 10, + addedAt: 10, + lastUsed: 10, + }, + ], + }), + "utf8", + ); + + const run = withAccountAndFlaggedStorageTransaction( + async (_current, _persist, currentFlagged) => currentFlagged, + { + getStoragePath, + loadCurrent: async () => null, + loadCurrentFlagged: loadFlaggedAccounts, + saveAccounts: async () => undefined, + saveFlaggedAccounts, + cloneAccountStorageForPersistence, + logRollbackError: () => undefined, + }, + ); + + // Fail loudly on a hang rather than letting the whole suite time out. + const timeout = new Promise((_, reject) => + setTimeout( + () => reject(new Error("withAccountAndFlaggedStorageTransaction deadlocked")), + 3000, + ), + ); + + const recoveredFlagged = (await Promise.race([run, timeout])) as Awaited< + typeof run + >; + expect(recoveredFlagged.accounts).toHaveLength(1); + expect(recoveredFlagged.accounts[0]).toEqual( + expect.objectContaining({ + refreshToken: "recovered-token", + accountId: "acct-recovered", + }), + ); + + // Recovery must still persist the primary flagged file (without re-locking). + expect(existsSync(flaggedPath)).toBe(true); + const persisted = await loadFlaggedAccounts(); + expect(persisted.accounts).toHaveLength(1); + expect(persisted.accounts[0]?.refreshToken).toBe("recovered-token"); + }); }); diff --git a/test/transactions.test.ts b/test/transactions.test.ts index 9e589b578..a099ae4bd 100644 --- a/test/transactions.test.ts +++ b/test/transactions.test.ts @@ -4,6 +4,7 @@ import { withAccountAndFlaggedStorageTransaction, withAccountStorageTransaction, } from "../lib/storage/transactions.js"; +import { cloneAccountStorageForPersistence } from "../lib/storage/account-persistence.js"; import type { AccountStorageV3 } from "../lib/storage.js"; describe("storage transaction helpers", () => { @@ -201,4 +202,40 @@ describe("storage transaction helpers", () => { expect(saveAccounts).toHaveBeenCalledTimes(2); }); + + it("preserves pinnedAccountIndex and affinityGeneration through the combined transaction", async () => { + // Regression for the clone dropping the manual pin: persisting through the + // real cloneAccountStorageForPersistence must carry both fields to disk. + const saved: AccountStorageV3[] = []; + await withAccountAndFlaggedStorageTransaction( + async (_current, persist) => { + await persist( + { + version: 3, + accounts: [{ refreshToken: "new" }], + activeIndex: 1, + activeIndexByFamily: { codex: 1 }, + pinnedAccountIndex: 1, + affinityGeneration: 9, + }, + { version: 1, accounts: [] }, + ); + }, + { + getStoragePath: () => "/tmp/accounts.json", + loadCurrent: async () => null, + loadCurrentFlagged: async () => ({ version: 1 as const, accounts: [] }), + saveAccounts: async (storage) => { + saved.push(storage); + }, + saveFlaggedAccounts: async () => undefined, + cloneAccountStorageForPersistence, + logRollbackError: vi.fn(), + }, + ); + + expect(saved).toHaveLength(1); + expect(saved[0]?.pinnedAccountIndex).toBe(1); + expect(saved[0]?.affinityGeneration).toBe(9); + }); }); From 4552c1322fa40eb8949cb234eb1413c06be5954a Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Tue, 2 Jun 2026 23:36:30 +0800 Subject: [PATCH 2/8] fix(audit): address CodeRabbit #505 review - mcodex: canonicalize both sides of the direct-run gate (realpath) so the launcher is reached when invoked via an npm-created symlink, not just the real path; extract isDirectRunInvocation + add symlink/missing-argv regression tests - workspace: clarify invalid-index errors ('must be a positive integer') so a non-digit input isn't mistaken for an out-of-range number - model-capability-matrix: add regression coverage that recordFailure/recordSuccess (which drive getBoost) align on the entitlement key, not just recordUnsupported - quota-cache: add a POSIX test asserting the cache dir is 0o700 (pins M5) --- lib/codex-manager/commands/workspace.ts | 8 ++-- scripts/mcodex.js | 25 +++++++++-- test/codex-manager-workspace-command.test.ts | 10 ++--- test/mcodex-launcher.test.ts | 40 +++++++++++++++++- test/model-capability-matrix.test.ts | 44 ++++++++++++++++++++ test/quota-cache.test.ts | 11 +++++ 6 files changed, 125 insertions(+), 13 deletions(-) diff --git a/lib/codex-manager/commands/workspace.ts b/lib/codex-manager/commands/workspace.ts index d9cf38af5..524b11574 100644 --- a/lib/codex-manager/commands/workspace.ts +++ b/lib/codex-manager/commands/workspace.ts @@ -48,12 +48,12 @@ export async function runWorkspaceCommand( // silently truncate "1.9" -> 1 or "2abc" -> 2 and operate on the wrong // account, so reject anything that isn't all digits. if (!/^\d+$/.test(accountArg.trim())) { - logError(`Invalid account index: ${accountArg}`); + logError(`Invalid account index (must be a positive integer): ${accountArg}`); return 1; } const parsedAccount = Number.parseInt(accountArg, 10); if (!Number.isFinite(parsedAccount) || parsedAccount < 1) { - logError(`Invalid account index: ${accountArg}`); + logError(`Invalid account index (must be a positive integer): ${accountArg}`); return 1; } @@ -93,7 +93,9 @@ export async function runWorkspaceCommand( } if (!/^\d+$/.test(workspaceArg.trim())) { - logError(`Invalid workspace index. Valid range: 1-${workspaces.length}`); + logError( + `Invalid workspace index (must be a positive integer). Valid range: 1-${workspaces.length}`, + ); return 1; } const parsedWorkspace = Number.parseInt(workspaceArg, 10); diff --git a/scripts/mcodex.js b/scripts/mcodex.js index c35ea3d0e..99372e3e5 100644 --- a/scripts/mcodex.js +++ b/scripts/mcodex.js @@ -1,7 +1,7 @@ #!/usr/bin/env node import { spawn, spawnSync } from "node:child_process"; -import { existsSync } from "node:fs"; +import { existsSync, realpathSync } from "node:fs"; import { dirname, join, resolve as resolvePath } from "node:path"; import process from "node:process"; import { fileURLToPath } from "node:url"; @@ -301,13 +301,30 @@ export function runMcodex(argv = process.argv.slice(2), env = process.env, platf forwardToCodexWrapper(forwardArgs, env); } -const isDirectRun = (() => { +export function isDirectRunInvocation(invoked, selfUrl) { + if (!invoked) return false; + const selfPath = fileURLToPath(selfUrl); + // Canonicalize both sides: npm installs bins as symlinks (and on Windows the + // .cmd/.ps1 shim invokes this file via its real path), so a raw string compare + // of process.argv[1] against import.meta.url misses the symlinked-bin case and + // the launcher silently becomes a no-op. realpathSync resolves both to the same + // target; fall back to a resolved-path compare if realpath fails (e.g. the file + // was unlinked between launch and this check). + const canonical = (p) => { + try { + return realpathSync(p); + } catch { + return resolvePath(p); + } + }; try { - return resolvePath(process.argv[1] ?? "") === fileURLToPath(import.meta.url); + return canonical(invoked) === canonical(selfPath); } catch { return false; } -})(); +} + +const isDirectRun = isDirectRunInvocation(process.argv[1], import.meta.url); if (isDirectRun) { runMcodex(); diff --git a/test/codex-manager-workspace-command.test.ts b/test/codex-manager-workspace-command.test.ts index e33c05328..83ebe591c 100644 --- a/test/codex-manager-workspace-command.test.ts +++ b/test/codex-manager-workspace-command.test.ts @@ -76,7 +76,7 @@ describe("runWorkspaceCommand", () => { const deps = createDeps(); const result = await runWorkspaceCommand(["abc"], deps); expect(result).toBe(1); - expect(deps.logError).toHaveBeenCalledWith("Invalid account index: abc"); + expect(deps.logError).toHaveBeenCalledWith("Invalid account index (must be a positive integer): abc"); expect(deps.saveAccounts).not.toHaveBeenCalled(); }); @@ -85,7 +85,7 @@ describe("runWorkspaceCommand", () => { const deps = createDeps(); const result = await runWorkspaceCommand(["1.9"], deps); expect(result).toBe(1); - expect(deps.logError).toHaveBeenCalledWith("Invalid account index: 1.9"); + expect(deps.logError).toHaveBeenCalledWith("Invalid account index (must be a positive integer): 1.9"); expect(deps.saveAccounts).not.toHaveBeenCalled(); }); @@ -93,7 +93,7 @@ describe("runWorkspaceCommand", () => { const deps = createDeps(); const result = await runWorkspaceCommand(["2abc", "1"], deps); expect(result).toBe(1); - expect(deps.logError).toHaveBeenCalledWith("Invalid account index: 2abc"); + expect(deps.logError).toHaveBeenCalledWith("Invalid account index (must be a positive integer): 2abc"); expect(deps.saveAccounts).not.toHaveBeenCalled(); }); @@ -103,7 +103,7 @@ describe("runWorkspaceCommand", () => { const result = await runWorkspaceCommand(["1", "2.9"], deps); expect(result).toBe(1); expect(deps.logError).toHaveBeenCalledWith( - "Invalid workspace index. Valid range: 1-2", + "Invalid workspace index (must be a positive integer). Valid range: 1-2", ); expect(deps.saveAccounts).not.toHaveBeenCalled(); }); @@ -161,7 +161,7 @@ describe("runWorkspaceCommand", () => { const result = await runWorkspaceCommand(["1", "xyz"], deps); expect(result).toBe(1); expect(deps.logError).toHaveBeenCalledWith( - "Invalid workspace index. Valid range: 1-2", + "Invalid workspace index (must be a positive integer). Valid range: 1-2", ); expect(deps.saveAccounts).not.toHaveBeenCalled(); }); diff --git a/test/mcodex-launcher.test.ts b/test/mcodex-launcher.test.ts index 7f3b82c79..1b827bff1 100644 --- a/test/mcodex-launcher.test.ts +++ b/test/mcodex-launcher.test.ts @@ -1,7 +1,8 @@ import { dirname, join } from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { describe, expect, it } from "vitest"; import { + isDirectRunInvocation, parseMcodexArgs, resolveMonitorInterval, resolveTmuxHistoryLimit, @@ -122,3 +123,40 @@ describe("mcodex ships as a node entry, not bash", () => { expect(source).not.toContain("#!/usr/bin/env bash"); }); }); + +describe("mcodex direct-run gate (isDirectRunInvocation)", () => { + const selfUrl = pathToFileURL(mcodexPath).href; + + it("matches when invoked via the real script path", () => { + expect(isDirectRunInvocation(mcodexPath, selfUrl)).toBe(true); + }); + + it("matches when invoked via a symlink to the script (npm-bin case)", async () => { + // npm installs bins as symlinks. The gate must canonicalize both sides + // (realpath) or the launcher silently no-ops when run through the link. + const { mkdtemp, symlink, rm } = await import("node:fs/promises"); + const { tmpdir } = await import("node:os"); + const tmp = await mkdtemp(join(tmpdir(), "mcodex-link-")); + const link = join(tmp, "mcodex"); + try { + await symlink(mcodexPath, link); + expect(isDirectRunInvocation(link, selfUrl)).toBe(true); + } catch (err) { + // Windows without symlink privilege (EPERM): skip rather than fail. + if ((err as NodeJS.ErrnoException).code === "EPERM") return; + throw err; + } finally { + await rm(tmp, { recursive: true, force: true }); + } + }); + + it("does not match an unrelated invocation path", () => { + expect( + isDirectRunInvocation(join(dirname(mcodexPath), "codex.js"), selfUrl), + ).toBe(false); + }); + + it("returns false when argv[1] is absent (imported, not run)", () => { + expect(isDirectRunInvocation(undefined, selfUrl)).toBe(false); + }); +}); diff --git a/test/model-capability-matrix.test.ts b/test/model-capability-matrix.test.ts index a8d515569..72cb51bb9 100644 --- a/test/model-capability-matrix.test.ts +++ b/test/model-capability-matrix.test.ts @@ -113,6 +113,50 @@ describe("model capability matrix", () => { ); }); + it("surfaces a negative capabilityBoost from recordFailure under the entitlement key", () => { + // getBoost depends on failures/successes (not just unsupported), and is also + // read with the entitlement key. A failure applies a penalty, so the boost is + // negative — pinning that recordFailure writes under the key the matrix reads. + const capabilityPolicy = new CapabilityPolicyStore(); + const entitlementKey = resolveEntitlementAccountKey({ + accountId: "acct_1", + email: "owner@example.com", + index: 0, + }); + capabilityPolicy.recordFailure(entitlementKey, "gpt-5.3-codex", 100); + + const matrix = buildModelCapabilityMatrix({ + storage: storage(), + models: ["gpt-5.3-codex"], + capabilityPolicy, + now: 100, + }); + + // failurePenalty = 3 (1 failure * 3), no successes → net boost -3. + expect(matrix.entries[0]?.capabilityBoost).toBeLessThan(0); + }); + + it("recordSuccess under the entitlement key lifts the capabilityBoost back positive", () => { + const capabilityPolicy = new CapabilityPolicyStore(); + const entitlementKey = resolveEntitlementAccountKey({ + accountId: "acct_1", + email: "owner@example.com", + index: 0, + }); + capabilityPolicy.recordFailure(entitlementKey, "gpt-5.3-codex", 100); + // recordSuccess decrements failures (→0) and adds a success → net positive. + capabilityPolicy.recordSuccess(entitlementKey, "gpt-5.3-codex", 100); + + const matrix = buildModelCapabilityMatrix({ + storage: storage(), + models: ["gpt-5.3-codex"], + capabilityPolicy, + now: 100, + }); + + expect(matrix.entries[0]?.capabilityBoost).toBeGreaterThan(0); + }); + it("marks disabled and entitlement-blocked accounts unavailable", () => { const baseStorage = storage(); baseStorage.accounts[0] = { diff --git a/test/quota-cache.test.ts b/test/quota-cache.test.ts index da8b2c1c2..9166df404 100644 --- a/test/quota-cache.test.ts +++ b/test/quota-cache.test.ts @@ -63,6 +63,17 @@ describe("quota cache", () => { expect(fileContent).toContain('"version": 1'); }); + it("keeps the cache directory owner-only (0o700) on POSIX", async () => { + // The quota cache sits alongside at-rest secrets; the dir must not be + // world-listable. mode is a no-op on win32 (ACL-based), so skip there. + if (process.platform === "win32") return; + const { saveQuotaCache } = await import("../lib/quota-cache.js"); + await saveQuotaCache({ byAccountId: {}, byEmail: {} }); + const stats = await fs.stat(tempDir); + // Low 9 perm bits should be rwx------ (0o700). + expect(stats.mode & 0o777).toBe(0o700); + }); + it("ignores cache files with unsupported version", async () => { const { loadQuotaCache, getQuotaCachePath } = await import("../lib/quota-cache.js"); From aac8ce2a94e10859db7ec7c68ca4b754d7259ceb Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Tue, 2 Jun 2026 23:37:34 +0800 Subject: [PATCH 3/8] fix: resolve v2.2.0 deep-audit LOW findings From the same audit pass (.omc/research/v2.2.0-audit-findings.md). 9 code fixes, 2 documented-as-intentional, 1 deferred-with-reason. Code-fixed: - local-client-tokens: route the full read-modify-write through the write queue so concurrent revoke/add/verify can't lose updates (L2) - codex-cli/state: honor forceRefresh even when a non-forced load is in flight (L7) - config: apply mtime compare-and-swap + ESTALE retry to the env-path config save, matching the unified-settings store (L8) - storage/record-utils: floor clampIndex so a fractional activeIndex can't persist or yield undefined on array access (L9) - runtime-rotation-proxy: stat-based mtime/size short-circuit so storage isn't re-read+hashed on every request (L3, settle-window guarded); check auth before path/method so unknown callers always get 401, not 404 (L5) - capability-policy: delete-then-set on record* so eviction is LRU, not FIFO (L6) - codex-bin-resolver: skip any PATH candidate resolving inside the wrapper's own dir, not just the exact codex.js realpath (L12) - codex-manager: clamp styleQuotaSummary percent to [0,100] (L11) Documented as intentional (not code-changed): - device-auth: 403/404 ARE the pending signal for this non-RFC-8628 endpoint; making them terminal would abort every login before approval. Loop already exits at expires_at/timeout. Comment added (L1) - runtime-policy: budgets are soft/eventually-consistent under concurrency; a hard cap needs cross-process reservation, out of scope for LOW. Comment added (L10) Deferred (needs design, documented inline): - runtime-rotation-proxy: routing the synchronous chooseAccount cursor mutations through the async routing mutex would force chooseAccount async across ~15 call sites and risk deadlock; documented the gap + fix direction (L4) Tests added/extended for every code fix. Full suite green. --- lib/auth/device-auth.ts | 15 ++ lib/capability-policy.ts | 5 + lib/codex-cli/state.ts | 7 +- lib/codex-manager.ts | 5 +- lib/config.ts | 75 ++++++++-- lib/local-client-tokens.ts | 149 +++++++++++-------- lib/policy/runtime-policy.ts | 8 + lib/runtime-rotation-proxy.ts | 102 +++++++++++-- lib/storage/record-utils.ts | 2 +- scripts/codex-bin-resolver.js | 29 +++- test/capability-policy.test.ts | 19 +++ test/codex-bin-wrapper.test.ts | 45 ++++++ test/codex-cli-state.test.ts | 78 ++++++++++ test/codex-manager-detail-tone.test.ts | 12 ++ test/config-save.test.ts | 62 ++++++++ test/issue-474-affinity-invalidation.test.ts | 57 ++++++- test/local-client-tokens.test.ts | 35 +++++ test/record-utils.test.ts | 45 ++++++ test/runtime-rotation-proxy.test.ts | 47 ++++++ 19 files changed, 703 insertions(+), 94 deletions(-) create mode 100644 test/record-utils.test.ts diff --git a/lib/auth/device-auth.ts b/lib/auth/device-auth.ts index d360b5abb..5ba8a77a6 100644 --- a/lib/auth/device-auth.ts +++ b/lib/auth/device-auth.ts @@ -167,6 +167,21 @@ function formatWaitBudget(timeoutMs: number): string { return `${totalMinutes} minute${totalMinutes === 1 ? "" : "s"}`; } +// The OpenAI Codex device-code token endpoint is NOT RFC 8628 compliant: it +// does not return a 400 + {"error":"authorization_pending"} body while waiting. +// Instead, `POST /api/accounts/deviceauth/token` returns a bare 403 while the +// user has not yet approved the code in the browser, and a bare 404 while the +// `device_auth_id` is not yet recognized (propagation lag right after the +// usercode request). Both are normal mid-flight states that must keep polling +// until the user completes the browser step or the deadline/server expiry hits; +// treating either as terminal would abort every login the instant the first +// poll fires, before the user could ever approve. This is exercised by +// test/device-auth.test.ts, where a 403 (and a 404) is the happy-path waiting +// state immediately preceding success. Do not move 403/404 out of the pending +// set without first re-confirming the live endpoint contract, including how a +// user-initiated denial is signaled — if denial is reported with its own +// distinguishable status or body, add that as a separate terminal branch rather +// than reclassifying the bare 403/404 pending responses. function isPendingStatus(status: number): boolean { return ( status === 403 || diff --git a/lib/capability-policy.ts b/lib/capability-policy.ts index 384cc47d1..0b1076d36 100644 --- a/lib/capability-policy.ts +++ b/lib/capability-policy.ts @@ -73,6 +73,9 @@ export class CapabilityPolicyStore { const key = makeKey(accountKey, model); if (!key) return; const existing = this.entries.get(key); + // Delete-then-set so the entry moves to the end of Map iteration order, + // making eviction LRU (least-recently-recorded) rather than FIFO. + this.entries.delete(key); this.entries.set(key, { successes: (existing?.successes ?? 0) + 1, failures: Math.max(0, (existing?.failures ?? 0) - 1), @@ -88,6 +91,7 @@ export class CapabilityPolicyStore { const key = makeKey(accountKey, model); if (!key) return; const existing = this.entries.get(key); + this.entries.delete(key); this.entries.set(key, { successes: existing?.successes ?? 0, failures: (existing?.failures ?? 0) + 1, @@ -103,6 +107,7 @@ export class CapabilityPolicyStore { const key = makeKey(accountKey, model); if (!key) return; const existing = this.entries.get(key); + this.entries.delete(key); this.entries.set(key, { successes: existing?.successes ?? 0, failures: (existing?.failures ?? 0) + 1, diff --git a/lib/codex-cli/state.ts b/lib/codex-cli/state.ts index 7aa4ca89a..356a1577f 100644 --- a/lib/codex-cli/state.ts +++ b/lib/codex-cli/state.ts @@ -393,7 +393,12 @@ export async function loadCodexCliState( if (!options?.forceRefresh && cache && now - cacheLoadedAt < CACHE_TTL_MS) { return cache; } - if (inFlightLoadPromise) { + // A forceRefresh caller must observe fresh disk state, so it must not be + // satisfied by an in-flight load that may have been started without + // forceRefresh (and could resolve stale/coalesced data). Only non-forced + // callers coalesce onto the in-flight promise; forced callers fall through + // and start their own fresh read below. + if (!options?.forceRefresh && inFlightLoadPromise) { return inFlightLoadPromise; } diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index e5ad44ed4..416e51487 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -392,7 +392,10 @@ function styleQuotaSummary(summary: string): string { return stylePromptText(segment, "muted"); } const windowLabel = match[1] ?? ""; - const leftPercent = Number.parseInt(match[2] ?? "", 10); + const leftPercent = Math.max( + 0, + Math.min(100, Number.parseInt(match[2] ?? "", 10)), + ); if (!Number.isFinite(leftPercent)) { return stylePromptText(segment, "muted"); } diff --git a/lib/config.ts b/lib/config.ts index 529adf55f..2082d7c39 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -450,15 +450,41 @@ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +async function getConfigFileMtimeMs(filePath: string): Promise { + try { + return (await fs.stat(filePath)).mtimeMs; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + throw error; + } +} + async function writeJsonFileAtomicWithRetry( filePath: string, payload: Record, + options?: { expectedMtimeMs?: number | null }, ): Promise { const tempPath = `${filePath}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; await fs.mkdir(dirname(filePath), { recursive: true }); await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, "utf8"); let renamed = false; try { + // Compare-and-swap guard: if a concurrent writer changed the target file + // since the caller read it (mtime mismatch), abort with ESTALE so the + // caller can re-read and merge instead of clobbering the other write. + if (options && "expectedMtimeMs" in options) { + const currentMtimeMs = await getConfigFileMtimeMs(filePath); + if (currentMtimeMs !== options.expectedMtimeMs) { + const staleError = new Error( + `Config at ${filePath} changed on disk during save; retrying with latest state.`, + ) as NodeJS.ErrnoException; + staleError.code = "ESTALE"; + throw staleError; + } + } for (let attempt = 0; attempt < 5; attempt += 1) { try { await fs.rename(tempPath, filePath); @@ -710,21 +736,42 @@ export async function savePluginConfig( if (envPath.length > 0) { await withConfigSaveLock(envPath, async () => { - const envConfigState = await readConfigRecordForSave(envPath); - if (envConfigState.status === "unreadable") { - throw new Error( - `Aborting config save because ${envPath} is unreadable.`, - ); + // Cross-process compare-and-swap: capture mtime before the + // read-merge-write, then have the atomic writer re-check it before the + // rename. On mismatch (another process wrote concurrently) we re-read and + // retry instead of silently dropping the other process's update. Mirrors + // the unified-settings save path (writeSettingsRecordAsync CAS). + for (let attempt = 0; attempt < 3; attempt += 1) { + const expectedMtimeMs = await getConfigFileMtimeMs(envPath); + const envConfigState = await readConfigRecordForSave(envPath); + if (envConfigState.status === "unreadable") { + throw new Error( + `Aborting config save because ${envPath} is unreadable.`, + ); + } + const existingConfig = + envConfigState.status === "ok" + ? sanitizeStoredPluginConfigRecord(envConfigState.record) + : null; + const merged = { + ...(existingConfig ?? {}), + ...sanitizedPatch, + }; + try { + await writeJsonFileAtomicWithRetry(envPath, merged, { + expectedMtimeMs, + }); + return; + } catch (error) { + if ( + (error as NodeJS.ErrnoException).code !== "ESTALE" || + attempt >= 2 + ) { + throw error; + } + // Loop: re-stat, re-read, re-merge against the latest on-disk state. + } } - const existingConfig = - envConfigState.status === "ok" - ? sanitizeStoredPluginConfigRecord(envConfigState.record) - : null; - const merged = { - ...(existingConfig ?? {}), - ...sanitizedPatch, - }; - await writeJsonFileAtomicWithRetry(envPath, merged); }); return; } diff --git a/lib/local-client-tokens.ts b/lib/local-client-tokens.ts index 0e93ca00f..081cd6fed 100644 --- a/lib/local-client-tokens.ts +++ b/lib/local-client-tokens.ts @@ -28,7 +28,20 @@ export interface CreatedLocalClientToken { const TOKEN_FILE_NAME = "local-client-tokens.json"; const TOKEN_PREFIX = "cma_local"; const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); -let writeQueue: Promise = Promise.resolve(); +let writeQueue: Promise = Promise.resolve(); + +// Serialize a task on the shared write queue so each task runs only after the +// previous one has fully settled. Routing the entire read-modify-write through +// here (not just the final write) ensures every mutation observes the prior +// committed state, preventing lost updates between concurrent public ops. +function enqueue(task: () => Promise): Promise { + const queued = writeQueue.catch(() => undefined).then(task); + writeQueue = queued.then( + () => undefined, + () => undefined, + ); + return queued; +} function isRetryableFsError(error: unknown): boolean { const code = (error as NodeJS.ErrnoException | undefined)?.code; @@ -132,46 +145,42 @@ export async function loadLocalClientTokenStore(): Promise { +async function writeStoreToDisk(store: LocalClientTokenStore): Promise { const path = getLocalClientTokenPath(); const payload = normalizeStore(store); - const task = async (): Promise => { - await fs.mkdir(getCodexMultiAuthDir(), { recursive: true, mode: 0o700 }); - const tempPath = `${path}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; - let moved = false; - try { - await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, { - encoding: "utf8", - mode: 0o600, - }); - for (let attempt = 0; attempt < 5; attempt += 1) { - try { - await fs.rename(tempPath, path); - moved = true; - return; - } catch (error) { - if (!isRetryableFsError(error) || attempt >= 4) throw error; - await sleep(10 * 2 ** attempt); - } + await fs.mkdir(getCodexMultiAuthDir(), { recursive: true, mode: 0o700 }); + const tempPath = `${path}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; + let moved = false; + try { + await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, { + encoding: "utf8", + mode: 0o600, + }); + for (let attempt = 0; attempt < 5; attempt += 1) { + try { + await fs.rename(tempPath, path); + moved = true; + return; + } catch (error) { + if (!isRetryableFsError(error) || attempt >= 4) throw error; + await sleep(10 * 2 ** attempt); } - } finally { - if (!moved) { - try { - await fs.unlink(tempPath); - } catch { - // Best-effort temp cleanup. - } + } + } finally { + if (!moved) { + try { + await fs.unlink(tempPath); + } catch { + // Best-effort temp cleanup. } } - }; - const queued = writeQueue.catch(() => undefined).then(task); - writeQueue = queued.then( - () => undefined, - () => undefined, - ); - await queued; + } +} + +export async function saveLocalClientTokenStore( + store: LocalClientTokenStore, +): Promise { + await enqueue(() => writeStoreToDisk(store)); } export function createLocalClientTokenRecord(input: { @@ -195,11 +204,13 @@ export async function addLocalClientToken(input: { label?: string; now?: number; } = {}): Promise { - const store = await loadLocalClientTokenStore(); - const created = createLocalClientTokenRecord(input); - store.tokens.push(created.record); - await saveLocalClientTokenStore(store); - return created; + return enqueue(async () => { + const store = await loadLocalClientTokenStore(); + const created = createLocalClientTokenRecord(input); + store.tokens.push(created.record); + await writeStoreToDisk(store); + return created; + }); } export async function rotateLocalClientToken(input: { @@ -207,30 +218,34 @@ export async function rotateLocalClientToken(input: { label?: string; now?: number; }): Promise { - const store = await loadLocalClientTokenStore(); - const existing = store.tokens.find((record) => record.id === input.id); - if (!existing || existing.revokedAt !== null) return null; - const now = input.now ?? Date.now(); - existing.revokedAt = now; - const created = createLocalClientTokenRecord({ - label: input.label ?? existing.label, - now, + return enqueue(async () => { + const store = await loadLocalClientTokenStore(); + const existing = store.tokens.find((record) => record.id === input.id); + if (!existing || existing.revokedAt !== null) return null; + const now = input.now ?? Date.now(); + existing.revokedAt = now; + const created = createLocalClientTokenRecord({ + label: input.label ?? existing.label, + now, + }); + store.tokens.push(created.record); + await writeStoreToDisk(store); + return created; }); - store.tokens.push(created.record); - await saveLocalClientTokenStore(store); - return created; } export async function revokeLocalClientToken( id: string, now = Date.now(), ): Promise { - const store = await loadLocalClientTokenStore(); - const existing = store.tokens.find((record) => record.id === id); - if (!existing || existing.revokedAt !== null) return false; - existing.revokedAt = now; - await saveLocalClientTokenStore(store); - return true; + return enqueue(async () => { + const store = await loadLocalClientTokenStore(); + const existing = store.tokens.find((record) => record.id === id); + if (!existing || existing.revokedAt !== null) return false; + existing.revokedAt = now; + await writeStoreToDisk(store); + return true; + }); } export async function verifyLocalClientBearerToken( @@ -241,14 +256,16 @@ export async function verifyLocalClientBearerToken( const token = match?.[1]?.trim(); if (!token) return null; const tokenHash = hashToken(token); - const store = await loadLocalClientTokenStore(); - const record = store.tokens.find( - (entry) => entry.revokedAt === null && entry.tokenHash === tokenHash, - ); - if (!record) return null; - record.lastUsedAt = now; - await saveLocalClientTokenStore(store); - return record; + return enqueue(async () => { + const store = await loadLocalClientTokenStore(); + const record = store.tokens.find( + (entry) => entry.revokedAt === null && entry.tokenHash === tokenHash, + ); + if (!record) return null; + record.lastUsedAt = now; + await writeStoreToDisk(store); + return record; + }); } export function resetLocalClientTokenWriteQueueForTests(): void { diff --git a/lib/policy/runtime-policy.ts b/lib/policy/runtime-policy.ts index 2939e8fa6..f1f42598a 100644 --- a/lib/policy/runtime-policy.ts +++ b/lib/policy/runtime-policy.ts @@ -152,6 +152,14 @@ export async function evaluateRuntimePolicy(input: { reasons.push("routing profile does not allow requested model"); } + // NOTE (audit L10): Budget enforcement is soft / eventually-consistent under + // concurrency. Each evaluation reads a pre-request ledger snapshot, while + // consumption is only recorded at request completion (see + // createRuntimeUsageRecorder below). N requests racing in the same window can + // therefore all observe sub-limit usage and pass before any of them records, + // allowing transient overshoot of e.g. maxRequests. This is intentional: a + // hard cap would require a cross-process reservation/locking system that is + // out of scope here. Budgets are a best-effort guard, not a strict quota. const budgetEvaluations = await evaluateBudgets({ state: input.state, now }); for (const evaluation of budgetEvaluations) { if (!evaluation.allowed) { diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 887e90b98..7b7ee0c72 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1,5 +1,5 @@ import { createHash, randomUUID, timingSafeEqual } from "node:crypto"; -import { existsSync, readFileSync } from "node:fs"; +import { existsSync, readFileSync, statSync } from "node:fs"; import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; import type { Socket } from "node:net"; import { @@ -349,8 +349,21 @@ function recordLastRuntimeAccount( * cache. The hashing cost is negligible for the small accounts.json file * (typically < 50KB) and keeps cache correctness independent of FS mtime * resolution. See #474. + * + * We additionally retain the last-seen `mtimeMs`/`size` so the hot path can + * skip the `readFileSync` + sha1 entirely when neither has changed since the + * previous read AND the cached mtime has settled past + * `MTIME_SHORTCIRCUIT_SETTLE_MS` (the common case — the proxy reads on every + * request but the file only mutates on a `switch`/`unpin`/`best` CLI + * invocation, then sits quiescent). The sha1 remains the source of truth: + * when `mtimeMs`/`size` differ, were never cached, or the mtime is too recent + * to trust against same-tick writes, we re-read and hash, so the content-hash + * path still protects against the coarse-mtime collision described above + * whenever the file is re-read. */ interface StorageMetaSnapshot { + mtimeMs: number; + size: number; contentHash: string; pinnedAccountIndex: number | null; affinityGeneration: number; @@ -366,6 +379,18 @@ export interface StorageMeta { // corrupt each other's snapshots. See issue #474. const STORAGE_META_CACHE: Map = new Map(); +// The mtime+size short-circuit (L3) may only be trusted once the cached +// mtime is far enough in the past that no *subsequent* write could share the +// same coarse mtime tick. Filesystems report mtime at wildly different +// granularities (ext4 ns, FAT 2s, some network/Windows volumes ~1s, and CI +// containers occasionally coarser), and our writers use atomic rename, so two +// rapid CLI bumps can land on an identical mtimeMs. Within this settle window +// we therefore ignore mtime equality and fall back to the read + sha1 path +// (the real source of truth). Outside it, the file has been quiescent long +// enough that mtime equality provably means "unchanged", so we skip the read. +// 2s comfortably exceeds the coarsest mtime granularity we expect in practice. +const MTIME_SHORTCIRCUIT_SETTLE_MS = 2_000; + function hashStorageBytes(bytes: Buffer): string { return createHash("sha1").update(bytes).digest("hex"); } @@ -378,7 +403,11 @@ function metaFromSnapshot(snapshot: StorageMetaSnapshot): StorageMeta { } /** - * Cheap, hot-path-safe single read with mtime-cache short-circuit. Transient + * Cheap, hot-path-safe single read with mtime-cache short-circuit. When the + * file's `mtimeMs` and `size` match the cached snapshot for this path we + * return the cached value WITHOUT reading or hashing the file. Only when + * mtime/size differ (or were never cached) do we `readFileSync` + sha1 and, + * if the content hash still matches, skip the `JSON.parse`. Transient * failures (EBUSY/EPERM/EACCES/EAGAIN, partial-write SyntaxError) fall through * to the last cached value for this path; defaults are only returned when the * file has never been successfully read. @@ -396,11 +425,38 @@ export function readStorageMetaFromDisk( return { pinnedAccountIndex: null, affinityGeneration: 0 }; } try { + // mtime+size short-circuit (L3): when neither has changed since the last + // successful read AND the cached mtime has settled (see + // MTIME_SHORTCIRCUIT_SETTLE_MS) we return the cached snapshot without + // reading or hashing the file. During the settle window we deliberately + // fall through to the read + sha1 path below, which stays the source of + // truth and protects against the coarse-mtime collision described on + // StorageMetaSnapshot. So this is a pure fast path for the common + // "file quiescent, proxy polling every request" case. + const stats = statSync(storagePath); + const cachedByStat = STORAGE_META_CACHE.get(storagePath); + if ( + cachedByStat && + cachedByStat.mtimeMs === stats.mtimeMs && + cachedByStat.size === stats.size && + Date.now() - stats.mtimeMs > MTIME_SHORTCIRCUIT_SETTLE_MS + ) { + return metaFromSnapshot(cachedByStat); + } const bytes = readFileSync(storagePath); const contentHash = hashStorageBytes(bytes); const cached = STORAGE_META_CACHE.get(storagePath); if (cached && cached.contentHash === contentHash) { - return metaFromSnapshot(cached); + // Content is byte-identical despite the mtime/size change (e.g. an + // atomic-rename rewrite of the same bytes). Refresh the stat fields so + // the next request takes the fast path, but skip the JSON.parse. + const refreshed: StorageMetaSnapshot = { + ...cached, + mtimeMs: stats.mtimeMs, + size: stats.size, + }; + STORAGE_META_CACHE.set(storagePath, refreshed); + return metaFromSnapshot(refreshed); } const parsed = JSON.parse(bytes.toString("utf8")) as { pinnedAccountIndex?: unknown; @@ -419,6 +475,8 @@ export function readStorageMetaFromDisk( ? parsed.affinityGeneration : 0; const snapshot: StorageMetaSnapshot = { + mtimeMs: stats.mtimeMs, + size: stats.size, contentHash, pinnedAccountIndex, affinityGeneration, @@ -1052,6 +1110,25 @@ function getQuotaNearExhaustionWaitMs( return candidates.length > 0 ? Math.max(...candidates) : 0; } +/** + * KNOWN GAP (L4, routing mutex): the two `accountManager.markSwitched(...)` + * cursor mutations below (the session-affinity-preferred branch and the + * round-robin fallback) run UNLOCKED even when `routingMutex === "enabled"`. + * Only `persistRuntimeActiveAccount` routes its cursor mutation through + * `markSwitchedLocked` / `withRoutingMutex`, so concurrent requests can still + * race the selection-time cursor update. + * + * Deferred (needs design), not fixed inline, because closing it safely is not + * a minimal change: `chooseAccount` is a SYNC function (returns + * `ManagedAccount | null`) consumed at ~15 exported/test call sites, whereas + * `markSwitchedLocked` is async and the routing mutex (`withRoutingMutex` -> + * `runExclusive`) is a non-reentrant FIFO queue. Awaiting it here would force + * `chooseAccount` async (signature + every caller) and, if any caller already + * holds the mutex, deadlock on the non-reentrant queue. The correct fix is to + * restructure selection so the cursor commit happens in one awaited + * critical section alongside `persistRuntimeActiveAccount`, which is out of + * scope for a minimal hot-path patch. Tracked for the routing-mutex redesign. + */ export function chooseAccount(params: { accountManager: AccountManager; sessionAffinityStore: SessionAffinityStore | null; @@ -1131,6 +1208,7 @@ export function chooseAccount(params: { if (reason) { skipReasons?.set(preferred.index, reason); } else { + // L4 (deferred): unlocked cursor mutation — see chooseAccount header. accountManager.markSwitched(preferred, "rotation", family); return preferred; } @@ -1178,6 +1256,7 @@ export function chooseAccount(params: { if (!reason) { const live = accountManager.getAccountByIndex(account.index); if (!live) continue; + // L4 (deferred): unlocked cursor mutation — see chooseAccount header. accountManager.markSwitched(live, "rotation", family); return live; } @@ -1499,6 +1578,17 @@ export async function startRuntimeRotationProxy( let accountManager = activeAccountManager; try { const incomingUrl = new URL(req.url ?? "/", "http://127.0.0.1"); + // Authenticate before discriminating path/method so an unauthenticated + // caller cannot enumerate which endpoints exist: an unknown caller always + // gets 401, never a 404 that would confirm a path is invalid (vs. just + // unauthorized). Authorized callers still fall through to the 404 below + // when they hit an unsupported path/method. + const incomingHeaders = headersFromIncoming(req); + if (!isAuthorizedClient(incomingHeaders, clientApiKey)) { + writeUnauthorized(res); + return; + } + const isResponsesRequest = req.method === "POST" && isResponsesPath(incomingUrl.pathname); const isModelsRequest = @@ -1511,12 +1601,6 @@ export async function startRuntimeRotationProxy( return; } - const incomingHeaders = headersFromIncoming(req); - if (!isAuthorizedClient(incomingHeaders, clientApiKey)) { - writeUnauthorized(res); - return; - } - status.totalRequests += 1; const requestBody = isResponsesRequest || (isThreadGoalRequest && req.method === "POST") diff --git a/lib/storage/record-utils.ts b/lib/storage/record-utils.ts index 746d02420..33052369e 100644 --- a/lib/storage/record-utils.ts +++ b/lib/storage/record-utils.ts @@ -4,5 +4,5 @@ export function isRecord(value: unknown): value is Record { export function clampIndex(index: number, length: number): number { if (length <= 0) return 0; - return Math.max(0, Math.min(index, length - 1)); + return Math.max(0, Math.min(Math.trunc(index), length - 1)); } diff --git a/scripts/codex-bin-resolver.js b/scripts/codex-bin-resolver.js index 015a1f843..0dd417a8f 100644 --- a/scripts/codex-bin-resolver.js +++ b/scripts/codex-bin-resolver.js @@ -1,7 +1,7 @@ import { spawnSync } from "node:child_process"; import { existsSync, realpathSync } from "node:fs"; import { createRequire } from "node:module"; -import { basename, delimiter, dirname, extname, join } from "node:path"; +import { basename, delimiter, dirname, extname, isAbsolute, join, relative } from "node:path"; import process from "node:process"; import { fileURLToPath } from "node:url"; @@ -25,6 +25,21 @@ function normalizeResolvedPath(candidatePath, realpathSyncImpl) { } } +// Defense-in-depth self-recursion guard. The exact-realpath check only blocks +// the wrapper's own codex.js (the POSIX symlink case). If the wrapper were ever +// exposed as a native codex/codex.exe sitting alongside it, that exact-path +// check would miss it. Skipping any candidate that resolves inside the +// wrapper's own directory closes that latent self-loop. +function isWithinDirectory(candidatePath, directoryPath, realpathSyncImpl) { + if (!directoryPath) return false; + const resolvedCandidate = normalizeResolvedPath(candidatePath, realpathSyncImpl); + const relativePath = relative(directoryPath, resolvedCandidate); + return ( + relativePath === "" || + (!relativePath.startsWith("..") && !isAbsolute(relativePath)) + ); +} + function resolveWrapperScriptPath(moduleUrl, realpathSyncImpl) { return normalizeResolvedPath(fileURLToPath(moduleUrl), realpathSyncImpl); } @@ -92,6 +107,12 @@ function resolveCodexExecutableFromPath( ) { continue; } + if ( + selfScriptPath && + isWithinDirectory(candidate, dirname(selfScriptPath), realpathSyncImpl) + ) { + continue; + } return candidate; } } @@ -154,6 +175,12 @@ function resolveCodexExecutableFromSystemPath( ) { continue; } + if ( + selfScriptPath && + isWithinDirectory(candidate, dirname(selfScriptPath), realpathSyncImpl) + ) { + continue; + } const fileName = basename(candidate).toLowerCase(); if (fileName === "codex" || fileName === "codex.exe") { return candidate; diff --git a/test/capability-policy.test.ts b/test/capability-policy.test.ts index 41dd85315..ee936a21c 100644 --- a/test/capability-policy.test.ts +++ b/test/capability-policy.test.ts @@ -104,6 +104,25 @@ describe("capability policy store", () => { expect(store.getSnapshot("id:acc_2054", "gpt-5-codex")).not.toBeNull(); }); + it("uses LRU eviction so a re-recorded old entry survives over an idle newer one", () => { + const store = new CapabilityPolicyStore(); + // Fill exactly to capacity (MAX_ENTRIES = 2048): acc_0 is the oldest. + for (let i = 0; i < 2048; i += 1) { + store.recordSuccess(`id:acc_${i}`, "gpt-5-codex", 1_000 + i); + } + // Re-record the oldest entry: this must refresh its position so it is no + // longer first in iteration order (FIFO would still evict it next). + store.recordSuccess("id:acc_0", "gpt-5-codex", 5_000); + // One more distinct insert pushes size over capacity and triggers eviction. + store.recordSuccess("id:acc_new", "gpt-5-codex", 6_000); + + // The hot, re-recorded entry survives; the now-oldest idle entry (acc_1) is + // evicted instead. + expect(store.getSnapshot("id:acc_0", "gpt-5-codex")).not.toBeNull(); + expect(store.getSnapshot("id:acc_1", "gpt-5-codex")).toBeNull(); + expect(store.getSnapshot("id:acc_new", "gpt-5-codex")).not.toBeNull(); + }); + it("clamps boost to score boundaries", () => { const store = new CapabilityPolicyStore(); for (let i = 0; i < 20; i += 1) { diff --git a/test/codex-bin-wrapper.test.ts b/test/codex-bin-wrapper.test.ts index 94ee9d6e1..b788ddbcc 100644 --- a/test/codex-bin-wrapper.test.ts +++ b/test/codex-bin-wrapper.test.ts @@ -4309,6 +4309,51 @@ describe("codex bin wrapper", () => { }); }); + it("skips native codex candidates inside the wrapper's own directory (defense-in-depth self-loop guard)", () => { + // Latent self-recursion: if the wrapper were exposed as a native + // codex/codex.exe alongside codex.js, its realpath would NOT equal the + // codex.js realpath, so the exact-path guard misses it. The directory guard + // must still skip any candidate resolving inside the wrapper's own dir and + // fall through to the genuine native binary elsewhere on PATH. + const wrapperScriptPath = join( + "C:\\test-root", + "npm", + "lib", + "node_modules", + "codex-multi-auth", + "scripts", + "codex.js", + ); + const wrapperDir = dirname(wrapperScriptPath); + const wrapperSiblingCodexPath = join(wrapperDir, "codex"); + const nativeCodexPath = join("C:\\test-root", "native", "bin", "codex"); + const resolved = resolveRealCodexBin({ + env: { + PATH: [wrapperDir, join("C:\\test-root", "native", "bin")].join(delimiter), + }, + argv: [process.execPath, wrapperScriptPath], + platform: "linux", + moduleUrl: pathToFileURL(join(repoRootDir, "scripts", "codex.js")).href, + resolvePackageBin: () => null, + spawnSyncImpl: () => createSpawnSyncSuccess(""), + existsSyncImpl: (candidate) => + candidate === wrapperSiblingCodexPath || candidate === nativeCodexPath, + realpathSyncImpl: (candidate) => { + if (candidate === join(repoRootDir, "scripts", "codex.js")) { + return wrapperScriptPath; + } + // The sibling native binary has its OWN realpath (not codex.js), so the + // exact-path guard would not catch it — only the directory guard does. + return candidate; + }, + }); + + expect(resolved).toEqual({ + path: nativeCodexPath, + launchWithNode: false, + }); + }); + it("discovers native codex executables via which fallback when PATH scan misses", () => { const nativeCodexPath = "/opt/homebrew/bin/codex"; const spawnCalls = []; diff --git a/test/codex-cli-state.test.ts b/test/codex-cli-state.test.ts index 2a9508993..6ee6013e1 100644 --- a/test/codex-cli-state.test.ts +++ b/test/codex-cli-state.test.ts @@ -1089,4 +1089,82 @@ describe("codex-cli state", () => { expect(await lookupCodexCliTokensByEmail(" ")).toBeNull(); expect(await lookupCodexCliTokensByEmail("a@example.com")).toBeNull(); }); + it("forceRefresh reads fresh disk data even while a non-forced load is in flight", async () => { + const staleAccounts = JSON.stringify( + { + activeAccountId: "acc_a", + accounts: [ + { + accountId: "acc_a", + email: "a@example.com", + auth: { + tokens: { access_token: "a.b.c", refresh_token: "refresh-a" }, + }, + }, + ], + }, + null, + 2, + ); + await writeFile(accountsPath, staleAccounts, "utf-8"); + clearCodexCliStateCache(); + + const realReadFile = fsPromises.readFile.bind(fsPromises); + let accountsReadCount = 0; + let releaseFirstRead: (() => void) | undefined; + const firstReadGate = new Promise((resolve) => { + releaseFirstRead = resolve; + }); + const readSpy = vi.spyOn(fsPromises, "readFile"); + readSpy.mockImplementation(async (...args) => { + if (String(args[0]) === accountsPath) { + accountsReadCount += 1; + if (accountsReadCount === 1) { + // Hold the first (non-forced) load mid-flight and return the stale + // snapshot so it cannot observe the later on-disk update. + await firstReadGate; + return staleAccounts; + } + } + return realReadFile(...args); + }); + + try { + // Non-forced load starts and parks on the gate, leaving inFlightLoadPromise set. + const stalePromise = loadCodexCliState(); + await Promise.resolve(); + expect(accountsReadCount).toBe(1); + + // Disk changes after the in-flight (non-forced) load already read stale data. + const freshAccounts = JSON.stringify( + { + activeAccountId: "acc_b", + accounts: [ + { + accountId: "acc_b", + email: "b@example.com", + auth: { + tokens: { access_token: "x.y.z", refresh_token: "refresh-b" }, + }, + }, + ], + }, + null, + 2, + ); + await writeFile(accountsPath, freshAccounts, "utf-8"); + + // forceRefresh must not be satisfied by the in-flight non-forced load. + const forced = await loadCodexCliState({ forceRefresh: true }); + expect(forced?.activeAccountId).toBe("acc_b"); + expect(accountsReadCount).toBeGreaterThanOrEqual(2); + + releaseFirstRead?.(); + const stale = await stalePromise; + expect(stale?.activeAccountId).toBe("acc_a"); + } finally { + releaseFirstRead?.(); + readSpy.mockRestore(); + } + }); }); diff --git a/test/codex-manager-detail-tone.test.ts b/test/codex-manager-detail-tone.test.ts index 83486f096..191dbbf81 100644 --- a/test/codex-manager-detail-tone.test.ts +++ b/test/codex-manager-detail-tone.test.ts @@ -80,4 +80,16 @@ describe("styleAccountDetailText tone precedence", () => { expect(styled).toContain(ANSI.red); expect(styled).not.toContain(ANSI.green); }); + + it("clamps an out-of-range quota percent to 100% in the styled quota segment", () => { + // A malformed quota summary like "5h 999%" must be clamped to [0,100] before + // tone selection and rendering, so it renders identically to "5h 100%" + // (success/green) and never surfaces the bogus 999% value to the user. + const clamped = styleAccountDetailText("acct (5h 999%)"); + const hundred = styleAccountDetailText("acct (5h 100%)"); + expect(clamped).toContain("100%"); + expect(clamped).not.toContain("999%"); + expect(clamped).toContain(ANSI.green); + expect(clamped).toBe(hundred); + }); }); diff --git a/test/config-save.test.ts b/test/config-save.test.ts index 580e2dd9b..3c757919d 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -124,6 +124,68 @@ describe("plugin config save paths", () => { expect(leakedTemps).toHaveLength(0); }); + it("does not lose a concurrent env-path update (mtime compare-and-swap)", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile( + configPath, + JSON.stringify({ codexMode: true, preserved: 1 }), + "utf8", + ); + // Pin a known starting mtime so the simulated concurrent write below + // produces a clearly different timestamp. + const baseTime = new Date("2026-01-01T00:00:00.000Z"); + await fs.utimes(configPath, baseTime, baseTime); + + const originalReadFile = fs.readFile.bind(fs); + let injectedConcurrentWrite = false; + const readSpy = vi + .spyOn(fs, "readFile") + .mockImplementation(async (...args) => { + const result = await originalReadFile( + ...(args as Parameters), + ); + if ( + String(args[0]) === configPath && + !injectedConcurrentWrite + ) { + // Simulate another process landing a write AFTER our read but + // BEFORE our rename. The CAS must detect the mtime change, abort + // with ESTALE, then re-read and merge so this key is not lost. + injectedConcurrentWrite = true; + const concurrentTime = new Date("2026-01-02T00:00:00.000Z"); + await fs.writeFile( + configPath, + JSON.stringify({ + codexMode: true, + preserved: 1, + concurrentKey: "from-other-process", + }), + "utf8", + ); + await fs.utimes(configPath, concurrentTime, concurrentTime); + } + return result; + }); + + try { + const { savePluginConfig } = await import("../lib/config.js"); + await savePluginConfig({ fastSession: true }); + } finally { + readSpy.mockRestore(); + } + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + // The concurrent process's key survives (not clobbered) and our patch applies. + expect(parsed.concurrentKey).toBe("from-other-process"); + expect(parsed.fastSession).toBe(true); + expect(parsed.codexMode).toBe(true); + expect(parsed.preserved).toBe(1); + }); + it("writes through unified settings when env path is unset", async () => { delete process.env.CODEX_MULTI_AUTH_CONFIG_PATH; const unifiedPath = join(tempDir, "settings.json"); diff --git a/test/issue-474-affinity-invalidation.test.ts b/test/issue-474-affinity-invalidation.test.ts index 4018e86e0..162587f3c 100644 --- a/test/issue-474-affinity-invalidation.test.ts +++ b/test/issue-474-affinity-invalidation.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, rmSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -214,6 +214,61 @@ describe("issue #474 — affinity invalidation on user storage events", () => { ); expect(readStorageMetaFromDisk(path).affinityGeneration).toBe(0); }); + + // L3: the hot path must short-circuit on stat (mtime+size) without + // re-reading or re-hashing the file when nothing has changed and the + // file has been quiescent. We prove this behaviorally: age the file's + // mtime past the settle window, read once (caches the snapshot), then + // rewrite with DIFFERENT bytes while forcing mtime+size back to the + // previously-seen (aged) values. A correct short-circuit returns the + // stale cached snapshot because it never read the new bytes. + it("skips re-reading the file when mtime and size are unchanged and settled", () => { + const path = makeTmpStoragePath(); + const a = JSON.stringify( + createStorage(Date.now(), 2, { affinityGeneration: 1 }), + ); + const b = JSON.stringify( + createStorage(Date.now(), 2, { affinityGeneration: 2 }), + ); + // Sanity: same byte length so size cannot betray the change. The two + // payloads differ only in the single-digit affinityGeneration value. + expect(Buffer.byteLength(a)).toBe(Buffer.byteLength(b)); + + writeFileSync(path, a, "utf8"); + // Age the mtime well past the settle window so the short-circuit is + // allowed to trust mtime equality on the next read. + const aged = Date.now() / 1000 - 60; + utimesSync(path, aged, aged); + expect(readStorageMetaFromDisk(path).affinityGeneration).toBe(1); + + // Overwrite with different content, then pin mtime+atime back to the + // cached (aged) value; size is already identical. Short-circuit fires. + writeFileSync(path, b, "utf8"); + utimesSync(path, aged, aged); + expect(readStorageMetaFromDisk(path).affinityGeneration).toBe(1); + }); + + // L3 safety: within the settle window mtime equality is NOT trusted (two + // rapid same-size CLI bumps can share a coarse mtime tick), so the read + + // sha1 path must still observe a content change. This guards the + // Windows/coarse-FS collision the content hash exists to defeat. + it("does not short-circuit on a freshly written file (settle window)", () => { + const path = makeTmpStoragePath(); + writeStorageFile( + path, + createStorage(Date.now(), 2, { affinityGeneration: 1 }), + ); + expect(readStorageMetaFromDisk(path).affinityGeneration).toBe(1); + + // Rewrite immediately. Even if the OS reports an identical mtime for + // both writes, the file is inside the settle window so we re-read and + // the sha1 mismatch surfaces the new generation. + writeStorageFile( + path, + createStorage(Date.now(), 2, { affinityGeneration: 2 }), + ); + expect(readStorageMetaFromDisk(path).affinityGeneration).toBe(2); + }); }); describe("persistAndSyncSelectedAccount: bumpAffinityGeneration via unpin", () => { diff --git a/test/local-client-tokens.test.ts b/test/local-client-tokens.test.ts index 6db65be9e..0ad2ff055 100644 --- a/test/local-client-tokens.test.ts +++ b/test/local-client-tokens.test.ts @@ -80,4 +80,39 @@ describe("local client tokens", () => { const store = await loadLocalClientTokenStore(); expect(store.tokens.filter((token) => token.revokedAt !== null)).toHaveLength(2); }); + + it("does not lose mutations when revoke and add run concurrently", async () => { + const { + addLocalClientToken, + loadLocalClientTokenStore, + revokeLocalClientToken, + } = await import("../lib/local-client-tokens.js"); + + // Seed a token whose revoke will race against a brand-new add. Before the + // read-modify-write was serialized through the write queue, the add and + // the revoke each loaded the same base store, mutated their own copy, and + // the later write clobbered the earlier one (lost update). Routing the + // full load->mutate->persist through the queue means each op observes the + // other's committed state, so both survive. + const seeded = await addLocalClientToken({ label: "seed", now: 100 }); + + const [, revoked] = await Promise.all([ + addLocalClientToken({ label: "added-concurrently", now: 200 }), + revokeLocalClientToken(seeded.record.id, 300), + ]); + + expect(revoked).toBe(true); + + const store = await loadLocalClientTokenStore(); + // Neither mutation was lost: the seed token is present and revoked, and + // the concurrently-added token is present and active. + expect(store.tokens).toHaveLength(2); + const seedRecord = store.tokens.find((t) => t.id === seeded.record.id); + expect(seedRecord?.revokedAt).toBe(300); + const addedRecord = store.tokens.find( + (t) => t.label === "added-concurrently", + ); + expect(addedRecord).toBeDefined(); + expect(addedRecord?.revokedAt).toBeNull(); + }); }); diff --git a/test/record-utils.test.ts b/test/record-utils.test.ts new file mode 100644 index 000000000..a0c9bce1e --- /dev/null +++ b/test/record-utils.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from "vitest"; +import { clampIndex, isRecord } from "../lib/storage/record-utils.js"; + +describe("clampIndex", () => { + it("returns 0 when length is non-positive", () => { + expect(clampIndex(3, 0)).toBe(0); + expect(clampIndex(3, -1)).toBe(0); + }); + + it("clamps within bounds", () => { + expect(clampIndex(-5, 3)).toBe(0); + expect(clampIndex(10, 3)).toBe(2); + expect(clampIndex(1, 3)).toBe(1); + }); + + it("floors fractional indices toward zero before clamping", () => { + // Tampered/corrupt files can carry a fractional activeIndex (e.g. 2.7). + // Without truncation it would survive normalization and produce + // undefined when used to index an array. + expect(clampIndex(2.7, 5)).toBe(2); + expect(clampIndex(0.9, 5)).toBe(0); + expect(clampIndex(4.999, 5)).toBe(4); + expect(Number.isInteger(clampIndex(2.7, 5))).toBe(true); + }); + + it("truncates negative fractional indices toward zero before clamping", () => { + expect(clampIndex(-0.5, 5)).toBe(0); + expect(clampIndex(-2.9, 5)).toBe(0); + }); + + it("never exceeds the last index for large fractional input", () => { + expect(clampIndex(7.5, 3)).toBe(2); + }); +}); + +describe("isRecord", () => { + it("accepts plain objects and rejects arrays/null/primitives", () => { + expect(isRecord({})).toBe(true); + expect(isRecord({ a: 1 })).toBe(true); + expect(isRecord([])).toBe(false); + expect(isRecord(null)).toBe(false); + expect(isRecord("x")).toBe(false); + expect(isRecord(42)).toBe(false); + }); +}); diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 87f61f0b7..6366df977 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -579,6 +579,53 @@ describe("runtime rotation proxy", () => { expect(calls).toHaveLength(2); }); + // L5 (endpoint enumeration): the auth check must run BEFORE path/method + // discrimination so an unauthenticated caller cannot distinguish a valid + // endpoint from an invalid one — both must return 401. Authorized callers + // must still receive 404 on unsupported paths. + it("returns 401 (not 404) for unauthenticated requests to unknown paths", async () => { + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now)); + const { calls, fetchImpl } = createRecordingFetch( + () => new Response('{"ok":true}', { status: HTTP_STATUS.OK }), + ); + const proxy = await startProxy({ accountManager, fetchImpl }); + + // Unauthenticated caller hitting a bogus path: must NOT be told the path + // is invalid (404). It must look identical to any other unauthorized + // request (401), revealing nothing about which endpoints exist. + const unauthUnknownPath = await fetch(`${proxy.baseUrl}/totally/unknown/path`, { + method: "GET", + headers: { authorization: "Bearer caller-token", "x-api-key": "caller-key" }, + }); + expect(unauthUnknownPath.status).toBe(HTTP_STATUS.UNAUTHORIZED); + + // Same for an unauthenticated caller using an unsupported method on a + // path that would otherwise be valid: still 401, never 404/405. + const unauthBadMethod = await fetch(`${proxy.baseUrl}/responses`, { + method: "DELETE", + headers: { authorization: "Bearer caller-token", "x-api-key": "caller-key" }, + }); + expect(unauthBadMethod.status).toBe(HTTP_STATUS.UNAUTHORIZED); + + // Authorized caller hitting a bogus path: ordering preserved, still 404. + const authUnknownPath = await fetch(`${proxy.baseUrl}/totally/unknown/path`, { + method: "GET", + headers: { authorization: `Bearer ${DEFAULT_CLIENT_API_KEY}` }, + }); + expect(authUnknownPath.status).toBe(404); + expect(await authUnknownPath.json()).toEqual({ + error: { + message: + "Runtime rotation proxy only accepts Responses API, model discovery, and Codex thread goal requests.", + code: "runtime_rotation_proxy_not_found", + }, + }); + + // No request should have been forwarded upstream for any of the above. + expect(calls).toHaveLength(0); + }); + it("forwards Responses requests unchanged while replacing caller auth", async () => { const now = Date.now(); const accountManager = new AccountManager(undefined, createStorage(now)); From 4be2fc4d9f3182538ca2238028a10529d5e98c37 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Wed, 3 Jun 2026 00:19:04 +0800 Subject: [PATCH 4/8] fix(audit): address CodeRabbit #505 second review (8 items) Two real bugs in the prior LOW fixes + test-quality hardening per repo conventions. Real bugs: - codex-cli/state: the forceRefresh-bypasses-in-flight change (L7) let a slower stale non-forced load commit its snapshot last and poison the cache. Added a load-generation guard so only the latest load commits cache/cacheLoadedAt; the caller still gets its own fresh return. Extended the race test to assert a post-stale plain read still serves fresh data. - local-client-tokens: the queued rename retry set only had EBUSY/EPERM; a transient ENOTEMPTY/EACCES/EAGAIN on Windows rename would fail the mutation. Widened to match quota-cache's taxonomy + added an ENOTEMPTY-retry regression. Test quality (CodeRabbit + repo guidelines): - codex-bin-wrapper: the L12 self-loop test mixed win32 drive-letter paths with platform:'linux'; switched to one POSIX path model end-to-end so PATH splitting is host-independent. - quota-cache + storage: the dir-perm tests now start from a 0o755 existing dir so they actually exercise the chmod-on-existing-dir hardening (were false-passes). - mcodex-launcher + issue-474: temp cleanup now uses removeWithRetry (EBUSY/EPERM/ ENOTEMPTY backoff) instead of bare fs.rm. - detail-tone: added a 0% lower-bound clamp case. - record-utils: dropped the explicit vitest import (globals enabled). Full suite: 4327 passed, 3 skipped, 0 failed; typecheck + lint clean. --- lib/codex-cli/state.ts | 28 ++++++++++--- lib/local-client-tokens.ts | 8 +++- test/codex-bin-wrapper.test.ts | 6 +-- test/codex-cli-state.test.ts | 6 +++ test/codex-manager-detail-tone.test.ts | 10 +++++ test/issue-474-affinity-invalidation.test.ts | 31 ++++++++++++-- test/local-client-tokens.test.ts | 29 ++++++++++++- test/mcodex-launcher.test.ts | 25 +++++++++++- test/quota-cache.test.ts | 4 ++ test/record-utils.test.ts | 1 - test/storage.test.ts | 43 ++++++++++++++++++++ 11 files changed, 174 insertions(+), 17 deletions(-) diff --git a/lib/codex-cli/state.ts b/lib/codex-cli/state.ts index 356a1577f..4e8ba0f21 100644 --- a/lib/codex-cli/state.ts +++ b/lib/codex-cli/state.ts @@ -38,6 +38,11 @@ export interface CodexCliState { let cache: CodexCliState | null = null; let cacheLoadedAt = 0; let inFlightLoadPromise: Promise | null = null; +// Monotonic load generation. forceRefresh lets loads overlap, so a slower stale +// (earlier) read must not overwrite the shared cache committed by a newer one. +// Each readTask captures its generation and only commits cache/cacheLoadedAt when +// it is still the latest; the caller always receives its own fresh return value. +let latestLoadGeneration = 0; const emittedWarnings = new Set(); function isRetryableFsError(error: unknown): boolean { @@ -403,6 +408,13 @@ export async function loadCodexCliState( } const readTask = async (): Promise => { + // Claim a generation; only the latest load is allowed to commit the cache. + const loadGeneration = ++latestLoadGeneration; + const commitCache = (value: CodexCliState | null): void => { + if (loadGeneration === latestLoadGeneration) { + cache = value; + } + }; const accountsPath = getCodexCliAccountsPath(); const authPath = getCodexCliAuthPath(); incrementCodexCliMetric("readAttempts"); @@ -411,7 +423,7 @@ export async function loadCodexCliState( const hasAuthPath = existsSync(authPath); if (!hasAccountsPath && !hasAuthPath) { incrementCodexCliMetric("readMisses"); - cache = null; + commitCache(null); return null; } @@ -439,7 +451,7 @@ export async function loadCodexCliState( email: state.activeEmail, }), }); - cache = state; + commitCache(state); return state; } log.warn("Codex CLI accounts payload is malformed", { @@ -480,7 +492,7 @@ export async function loadCodexCliState( email: state.activeEmail, }), }); - cache = state; + commitCache(state); return state; } log.warn("Codex CLI auth payload is malformed", { @@ -499,7 +511,7 @@ export async function loadCodexCliState( } incrementCodexCliMetric("readFailures"); - cache = null; + commitCache(null); return null; } catch (error) { incrementCodexCliMetric("readFailures"); @@ -509,10 +521,14 @@ export async function loadCodexCliState( path: hasAccountsPath ? accountsPath : authPath, error: String(error), }); - cache = null; + commitCache(null); return null; } finally { - cacheLoadedAt = Date.now(); + // Only the latest load advances the shared cache timestamp, mirroring + // commitCache, so a slow stale read can't reset the TTL window. + if (loadGeneration === latestLoadGeneration) { + cacheLoadedAt = Date.now(); + } } }; diff --git a/lib/local-client-tokens.ts b/lib/local-client-tokens.ts index 081cd6fed..60e352c3a 100644 --- a/lib/local-client-tokens.ts +++ b/lib/local-client-tokens.ts @@ -27,7 +27,13 @@ export interface CreatedLocalClientToken { const TOKEN_FILE_NAME = "local-client-tokens.json"; const TOKEN_PREFIX = "cma_local"; -const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]); +const RETRYABLE_FS_CODES = new Set([ + "EBUSY", + "EPERM", + "EAGAIN", + "ENOTEMPTY", + "EACCES", +]); let writeQueue: Promise = Promise.resolve(); // Serialize a task on the shared write queue so each task runs only after the diff --git a/test/codex-bin-wrapper.test.ts b/test/codex-bin-wrapper.test.ts index b788ddbcc..b7d99c538 100644 --- a/test/codex-bin-wrapper.test.ts +++ b/test/codex-bin-wrapper.test.ts @@ -4316,7 +4316,7 @@ describe("codex bin wrapper", () => { // must still skip any candidate resolving inside the wrapper's own dir and // fall through to the genuine native binary elsewhere on PATH. const wrapperScriptPath = join( - "C:\\test-root", + "/test-root", "npm", "lib", "node_modules", @@ -4326,10 +4326,10 @@ describe("codex bin wrapper", () => { ); const wrapperDir = dirname(wrapperScriptPath); const wrapperSiblingCodexPath = join(wrapperDir, "codex"); - const nativeCodexPath = join("C:\\test-root", "native", "bin", "codex"); + const nativeCodexPath = join("/test-root", "native", "bin", "codex"); const resolved = resolveRealCodexBin({ env: { - PATH: [wrapperDir, join("C:\\test-root", "native", "bin")].join(delimiter), + PATH: [wrapperDir, join("/test-root", "native", "bin")].join(delimiter), }, argv: [process.execPath, wrapperScriptPath], platform: "linux", diff --git a/test/codex-cli-state.test.ts b/test/codex-cli-state.test.ts index 6ee6013e1..508bfe444 100644 --- a/test/codex-cli-state.test.ts +++ b/test/codex-cli-state.test.ts @@ -1162,6 +1162,12 @@ describe("codex-cli state", () => { releaseFirstRead?.(); const stale = await stalePromise; expect(stale?.activeAccountId).toBe("acc_a"); + + // The stale (older-generation) load resolved LAST. It must not have + // committed acc_a into the shared cache — a later plain read must still + // serve the fresh acc_b within the TTL window (load-generation guard). + const afterStale = await loadCodexCliState(); + expect(afterStale?.activeAccountId).toBe("acc_b"); } finally { releaseFirstRead?.(); readSpy.mockRestore(); diff --git a/test/codex-manager-detail-tone.test.ts b/test/codex-manager-detail-tone.test.ts index 191dbbf81..a052d25a9 100644 --- a/test/codex-manager-detail-tone.test.ts +++ b/test/codex-manager-detail-tone.test.ts @@ -92,4 +92,14 @@ describe("styleAccountDetailText tone precedence", () => { expect(clamped).toContain(ANSI.green); expect(clamped).toBe(hundred); }); + + it("renders the 0% lower bound as danger (red) without crashing", () => { + // 0% is the clamp lower bound; quotaToneFromLeftPercent(0) => "danger" + // (0 <= 15). It must render the literal "0%" in red, never drop the value + // or throw on the boundary. + const styled = styleAccountDetailText("acct (5h 0%)"); + expect(styled).toContain("0%"); + expect(styled).toContain(ANSI.red); + expect(styled).not.toContain(ANSI.green); + }); }); diff --git a/test/issue-474-affinity-invalidation.test.ts b/test/issue-474-affinity-invalidation.test.ts index 162587f3c..3b331a5df 100644 --- a/test/issue-474-affinity-invalidation.test.ts +++ b/test/issue-474-affinity-invalidation.test.ts @@ -1,4 +1,9 @@ -import { mkdtempSync, rmSync, utimesSync, writeFileSync } from "node:fs"; +import { + mkdtempSync, + promises as fs, + utimesSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -17,6 +22,26 @@ import { normalizeAccountStorage, } from "../lib/storage.js"; +const RETRYABLE_REMOVE_CODES = new Set(["EBUSY", "EPERM", "EACCES", "EAGAIN", "ENOTEMPTY"]); +async function removeWithRetry( + targetPath: string, + options: { recursive?: boolean; force?: boolean }, +): Promise { + for (let attempt = 0; attempt < 6; attempt += 1) { + try { + await fs.rm(targetPath, options); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") return; + if (!code || !RETRYABLE_REMOVE_CODES.has(code) || attempt === 5) { + throw error; + } + await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); + } + } +} + function createStorage( now: number, count = 3, @@ -57,11 +82,11 @@ beforeEach(() => { resetPinCacheForTesting(); }); -afterEach(() => { +afterEach(async () => { resetPinCacheForTesting(); for (const dir of tmpDirs.splice(0, tmpDirs.length)) { try { - rmSync(dir, { recursive: true, force: true }); + await removeWithRetry(dir, { recursive: true, force: true }); } catch { // best-effort cleanup } diff --git a/test/local-client-tokens.test.ts b/test/local-client-tokens.test.ts index 0ad2ff055..b2ffd0002 100644 --- a/test/local-client-tokens.test.ts +++ b/test/local-client-tokens.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { promises as fs } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; @@ -115,4 +115,31 @@ describe("local client tokens", () => { expect(addedRecord).toBeDefined(); expect(addedRecord?.revokedAt).toBeNull(); }); + + it("retries atomic rename on transient ENOTEMPTY errors", async () => { + const { addLocalClientToken, loadLocalClientTokenStore } = await import( + "../lib/local-client-tokens.js" + ); + const realRename = fs.rename.bind(fs); + let attempts = 0; + const renameSpy = vi.spyOn(fs, "rename"); + renameSpy.mockImplementation(async (...args) => { + attempts += 1; + if (attempts === 1) { + const error = new Error("dir not empty") as NodeJS.ErrnoException; + error.code = "ENOTEMPTY"; + throw error; + } + return realRename(...args); + }); + + try { + const created = await addLocalClientToken({ label: "retry", now: 100 }); + expect(attempts).toBeGreaterThan(1); + const store = await loadLocalClientTokenStore(); + expect(store.tokens.find((t) => t.id === created.record.id)).toBeDefined(); + } finally { + renameSpy.mockRestore(); + } + }); }); diff --git a/test/mcodex-launcher.test.ts b/test/mcodex-launcher.test.ts index 1b827bff1..f1ed3eb69 100644 --- a/test/mcodex-launcher.test.ts +++ b/test/mcodex-launcher.test.ts @@ -1,4 +1,5 @@ import { dirname, join } from "node:path"; +import { promises as fs } from "node:fs"; import { fileURLToPath, pathToFileURL } from "node:url"; import { describe, expect, it } from "vitest"; import { @@ -9,6 +10,26 @@ import { resolveTmuxSession, } from "../scripts/mcodex.js"; +const RETRYABLE_REMOVE_CODES = new Set(["EBUSY", "EPERM", "EACCES", "EAGAIN", "ENOTEMPTY"]); +async function removeWithRetry( + targetPath: string, + options: { recursive?: boolean; force?: boolean }, +): Promise { + for (let attempt = 0; attempt < 6; attempt += 1) { + try { + await fs.rm(targetPath, options); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT") return; + if (!code || !RETRYABLE_REMOVE_CODES.has(code) || attempt === 5) { + throw error; + } + await new Promise((resolve) => setTimeout(resolve, 25 * 2 ** attempt)); + } + } +} + // H1 regression: scripts/mcodex was a `#!/usr/bin/env bash` script shipped as a // Windows bin. npm's generated mcodex.cmd/.ps1 shim invoked bare `bash`; when a WSL // stub resolved before git-bash on PATH the launcher died with @@ -134,7 +155,7 @@ describe("mcodex direct-run gate (isDirectRunInvocation)", () => { it("matches when invoked via a symlink to the script (npm-bin case)", async () => { // npm installs bins as symlinks. The gate must canonicalize both sides // (realpath) or the launcher silently no-ops when run through the link. - const { mkdtemp, symlink, rm } = await import("node:fs/promises"); + const { mkdtemp, symlink } = await import("node:fs/promises"); const { tmpdir } = await import("node:os"); const tmp = await mkdtemp(join(tmpdir(), "mcodex-link-")); const link = join(tmp, "mcodex"); @@ -146,7 +167,7 @@ describe("mcodex direct-run gate (isDirectRunInvocation)", () => { if ((err as NodeJS.ErrnoException).code === "EPERM") return; throw err; } finally { - await rm(tmp, { recursive: true, force: true }); + await removeWithRetry(tmp, { recursive: true, force: true }); } }); diff --git a/test/quota-cache.test.ts b/test/quota-cache.test.ts index 9166df404..6df9c8b88 100644 --- a/test/quota-cache.test.ts +++ b/test/quota-cache.test.ts @@ -67,6 +67,10 @@ describe("quota cache", () => { // The quota cache sits alongside at-rest secrets; the dir must not be // world-listable. mode is a no-op on win32 (ACL-based), so skip there. if (process.platform === "win32") return; + // beforeEach already created tempDir, so this exercises the chmod-on-an- + // EXISTING-dir path (mkdir's mode only applies to a fresh dir). Loosen it to + // 0o755 first; if saveQuotaCache failed to re-assert 0o700 the test fails. + await fs.chmod(tempDir, 0o755); const { saveQuotaCache } = await import("../lib/quota-cache.js"); await saveQuotaCache({ byAccountId: {}, byEmail: {} }); const stats = await fs.stat(tempDir); diff --git a/test/record-utils.test.ts b/test/record-utils.test.ts index a0c9bce1e..f7c40e904 100644 --- a/test/record-utils.test.ts +++ b/test/record-utils.test.ts @@ -1,4 +1,3 @@ -import { describe, expect, it } from "vitest"; import { clampIndex, isRecord } from "../lib/storage/record-utils.js"; describe("clampIndex", () => { diff --git a/test/storage.test.ts b/test/storage.test.ts index fd89cccee..1992d92c3 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -54,6 +54,49 @@ describe("storage", () => { else delete process.env.CODEX_MULTI_AUTH_DIR; }); + describe("account storage directory hardening", () => { + let storageDir: string; + let storagePath: string; + + beforeEach(async () => { + storageDir = await fs.mkdtemp(join(tmpdir(), "codex-acct-dirmode-")); + storagePath = join(storageDir, "openai-codex-accounts.json"); + setStoragePathDirect(storagePath); + }); + + afterEach(async () => { + setStoragePathDirect(null); + await fs.rm(storageDir, { recursive: true, force: true }); + }); + + it.skipIf(process.platform === "win32")( + "re-asserts owner-only (0o700) on a pre-existing permissive dir after save", + async () => { + // The dir already exists (mkdtemp), so mkdir({ mode: 0o700 }) is a + // no-op and only the explicit chmod can tighten it. Loosen to 0o755 + // first to prove saveAccounts hardens an EXISTING dir, not just a + // freshly-created one. + await fs.chmod(storageDir, 0o755); + expect((await fs.stat(storageDir)).mode & 0o777).toBe(0o755); + + await saveAccounts({ + version: 3, + activeIndex: 0, + accounts: [ + { + accountId: "acct-dirmode", + refreshToken: "refresh", + addedAt: 1, + lastUsed: 2, + }, + ], + } as Parameters[0]); + + expect((await fs.stat(storageDir)).mode & 0o777).toBe(0o700); + }, + ); + }); + describe("storage error hints", () => { it("formats actionable Windows file-lock guidance for EBUSY errors", () => { const hint = formatStorageErrorHint( From 877e02f8c3127558fc6a0bc55e2f05b2aba3ca0d Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Wed, 3 Jun 2026 01:42:16 +0800 Subject: [PATCH 5/8] fix(audit): CodeRabbit #505 round 3 + cut v2.2.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes partial fixes flagged in the third review, bumps to v2.2.1. Real bugs (prior fixes were incomplete): - request/fetch-helpers: isUnsupportedCodexModelForChatGpt (the handleErrorResponse path) now also matches NORMALIZED_UNSUPPORTED_MODEL_PATTERN, so a 400 'model not currently available for this chatgpt account' gets the entitlement rewrite, not generic error guidance (#13) - forecast: quota-exhausted accounts are classified 'delayed' (not 'unavailable'), so they slipped the recommendation filter; added an explicit exhausted flag and excluded it, returning null when the whole pool is exhausted (#10) - storage/record-utils: clampIndex guards NaN -> 0 (Math.trunc(NaN) propagated) (#16) - local-client-tokens: debounce lastUsedAt persistence (60s threshold) so the bearer-verify hot path stops writing to disk every request; +chmod 0o700 re-assert on the token-store dir (#12, #11) - mcodex: relay SIGTERM/SIGINT to the spawned child so it isn't orphaned (#15) Test quality: - local-client-tokens: parameterized rename-retry test over ENOTEMPTY/EAGAIN/EACCES (#18) - storage-flagged: clear the H4 deadlock-guard timer on the happy path (#17) - storage: removeWithRetry for suite cleanup (#19) Release: - bump package.json + .codex-plugin/plugin.json to 2.2.1 - add docs/releases/v2.2.1.md; point docs portal + README at it Known follow-ups (documented, deferred — need design, not rushed into a patch): config env-path save is a single-process CAS not a true cross-process lock (#8/#9); verifyLocalClientBearerToken read stays serialized but a fuller lease is future work; runtime proxy routingMutex='enabled' still has a select/commit cursor race (#14) requiring an async refactor of chooseAccount across its call sites. Full suite: 4337 passed, 3 skipped, 0 failed; typecheck + lint clean. --- .codex-plugin/plugin.json | 2 +- README.md | 3 +- docs/README.md | 3 +- docs/releases/v2.2.1.md | 93 ++++++++++++++++++++++++++++++ lib/forecast.ts | 23 ++++++-- lib/local-client-tokens.ts | 35 ++++++++++- lib/request/fetch-helpers.ts | 1 + lib/storage/record-utils.ts | 4 ++ package.json | 2 +- scripts/mcodex.js | 35 +++++++++++ test/fetch-helpers.test.ts | 22 +++++++ test/forecast.test.ts | 60 +++++++++++++++++++ test/local-client-tokens.test.ts | 99 ++++++++++++++++++++++++-------- test/mcodex-launcher.test.ts | 70 +++++++++++++++++++++- test/record-utils.test.ts | 9 +++ test/storage-flagged.test.ts | 21 ++++--- test/storage.test.ts | 3 +- 17 files changed, 440 insertions(+), 45 deletions(-) create mode 100644 docs/releases/v2.2.1.md diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index 35d0f2e54..702b52b6f 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "codex-multi-auth", - "version": "2.2.0", + "version": "2.2.1", "description": "Install and operate codex-multi-auth for the official @openai/codex CLI with multi-account OAuth rotation, switching, health checks, and recovery tools.", "interface": { "composerIcon": "./assets/codex-multi-auth-icon.svg" diff --git a/README.md b/README.md index cf6167971..902835e33 100644 --- a/README.md +++ b/README.md @@ -383,7 +383,8 @@ codex-multi-auth doctor --json ## Release Notes -- Current stable: [docs/releases/v2.2.0.md](docs/releases/v2.2.0.md) — install via `npm i -g codex-multi-auth` +- Current stable: [docs/releases/v2.2.1.md](docs/releases/v2.2.1.md) — install via `npm i -g codex-multi-auth` +- Previous stable: [docs/releases/v2.2.0.md](docs/releases/v2.2.0.md) - Previous stable: [docs/releases/v2.1.12.md](docs/releases/v2.1.12.md) - Earlier stable: [docs/releases/v2.1.11.md](docs/releases/v2.1.11.md) - Earlier stable: [docs/releases/v2.1.10.md](docs/releases/v2.1.10.md) diff --git a/docs/README.md b/docs/README.md index a5e07d9fe..fe155efb2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -32,7 +32,8 @@ Public documentation for the `codex-multi-auth` Codex CLI multi-account OAuth ma | Document | Focus | | --- | --- | -| [releases/v2.2.0.md](releases/v2.2.0.md) | Current stable release notes (install via `npm i -g codex-multi-auth`) | +| [releases/v2.2.1.md](releases/v2.2.1.md) | Current stable release notes (install via `npm i -g codex-multi-auth`) | +| [releases/v2.2.0.md](releases/v2.2.0.md) | Prior stable release notes | | [releases/v2.1.12.md](releases/v2.1.12.md) | Prior stable release notes | | [releases/v2.1.11.md](releases/v2.1.11.md) | Prior stable release notes | | [releases/v2.1.10.md](releases/v2.1.10.md) | Earlier stable release notes | diff --git a/docs/releases/v2.2.1.md b/docs/releases/v2.2.1.md new file mode 100644 index 000000000..2082fd8bc --- /dev/null +++ b/docs/releases/v2.2.1.md @@ -0,0 +1,93 @@ +# v2.2.1 + +Patch release. A full deep audit of the v2.2.0 tree (six parallel auditors across +auth, the runtime proxy/rotation, scripts/bins, quota/policy, storage, and the +codex-manager CLI, plus a live Windows/PowerShell repro) surfaced bugs that +escaped pre-release review. This release fixes 4 HIGH and 6 MEDIUM findings, 9 of +12 LOW findings, and the issues raised across two rounds of automated review. + +The headline fix: the `mcodex` launcher (the v2.2.0 flagship) was a bash script +shipped as a Windows bin and could not start on Windows when a WSL stub shadowed +git-bash on PATH. It is now a pure Node launcher with zero bash dependency. + +## Install + +```bash +npm i -g codex-multi-auth@latest +``` + +## HIGH + +- **mcodex is now a Node launcher (Windows-fatal fix).** `scripts/mcodex` was + `#!/usr/bin/env bash`; npm's generated `.cmd`/`.ps1` shim invoked bare `bash`, so + when the WSL stub (`System32\bash.exe`) or the WindowsApps app-execution alias + resolved before git-bash, `mcodex` died with `HCS_E_SERVICE_NOT_AVAILABLE`. The + launcher is rewritten in Node (`scripts/mcodex.js`): zero bash dependency on the + default forward path, `tmux`/`watch` invoked as argv arrays (no shell string + interpolation), and graceful degradation with the same friendly messages when + those POSIX tools are absent. The direct-run gate canonicalizes symlinks so the + launcher still runs when invoked through an npm-created symlink bin. +- **OAuth concurrent-login isolation.** The local callback server stored the + authorization `code`/`state` on the shared `http.Server` instance, so two logins + in one process could cross-bind callback state. Capture now lives in per-call + closures. +- **Capability matrix reads the correct key.** `model-capability-matrix` read + capability snapshots/boosts with the sha256 account key while the store is + written under the entitlement key, so the matrix reported every account as + supporting every model. It now reads with the entitlement key, matching the + write path. +- **Storage transaction deadlock.** A flagged-storage backup recovery that ran + inside an already-held storage lock re-acquired the global mutex and deadlocked, + wedging all subsequent account/token saves (reachable via the doctor restore + flow). Lock ownership is now tracked so recovery persists without re-locking. + +## MEDIUM + +- **Status tone precedence.** A failed live health check whose detail also carried + a quota percentage could render the account's prefix green ("working") because + the failure keyword was trapped inside the `(NN%)` segment. The tone now + considers the whole detail, so a real failure always renders red. +- **`workspace` index validation.** `codex-multi-auth workspace 1.9` (or `2abc`) + was silently truncated to account 1/2 by `parseInt`. Non-integer indices are now + rejected with a clear "must be a positive integer" message, matching `switch`. +- **Unsupported-model classification.** A "the model … is not currently available + for this ChatGPT account" response was classified as a transient outage instead + of an entitlement block on one code path; the normalized wording is now detected + consistently across the probe/forecast/report/check surfaces. +- **Manual pin preserved on restore.** The combined account+flagged storage + transaction dropped `pinnedAccountIndex`/`affinityGeneration` when cloning, so a + doctor restore erased the user's manual pin. Both fields are now carried through. +- **Secret directory permissions.** The account-storage and quota-cache + directories are created `0o700` (and re-asserted on POSIX) instead of relying on + the umask, so they are not world-listable. +- **Forecast no longer recommends a blocked account.** Policy-blocked and + token-exhausted accounts were eligible for "pick shortest wait"; they are now + excluded, and the forecast returns no recommendation with a clear reason when + none are ready. + +## LOW + +Nine lower-severity fixes from the same audit: the local-client-token store now +serializes its full read-modify-write (and retries the complete Windows transient +lock taxonomy, including `ENOTEMPTY`, on rename); the Codex CLI state cache honors +`forceRefresh` even with a load in flight, guarded by a load generation so a slow +stale read can't poison the cache; the env-path config save uses an mtime +compare-and-swap with `ESTALE` retry; `clampIndex` floors fractional indices; the +runtime proxy short-circuits storage re-reads on unchanged mtime/size and checks +authorization before path/method (401 before 404); capability-policy eviction is +LRU; the Codex bin resolver skips any PATH candidate inside its own wrapper +directory; and `styleQuotaSummary` clamps out-of-range percentages. + +Two findings are documented as intentional rather than changed: the device-auth +endpoint's bare 403/404 responses are its non-RFC-8628 "authorization pending" +signal (the poll already exits at the server deadline), and runtime budgets are +deliberately soft/eventually-consistent under concurrency. One proxy +routing-mutex gap is deferred with an inline design note, as closing it requires +making the synchronous selection path async across its call sites. + +## Verification + +Full test suite green (4,300+ tests, 40+ new regression cases); typecheck and +lint clean; the Node `mcodex` launcher, the `workspace` index guard, the live +`check`/`best`/`forecast` paths, and `verify --all` (the storage transaction +path) were all exercised against a real account on Windows/PowerShell. diff --git a/lib/forecast.ts b/lib/forecast.ts index c1bd44514..9283d4e1b 100644 --- a/lib/forecast.ts +++ b/lib/forecast.ts @@ -42,6 +42,11 @@ export interface ForecastAccountResult { reasons: string[]; hardFailure: boolean; disabled: boolean; + // True when the account is blocked solely by quota-cache exhaustion. Such + // accounts are classified `availability === "delayed"` (so display/sorting + // still treat them as a timed wait), but they must NOT be recommended as a + // "pick shortest wait" fallback when every account is exhausted. + exhausted: boolean; } export interface ForecastRecommendation { @@ -177,6 +182,7 @@ export function evaluateForecastAccount( let riskScore = isCurrent ? -5 : 0; let waitMs = 0; let hardFailure = false; + let exhausted = false; const disabled = account.enabled === false; if (disabled) { @@ -241,6 +247,7 @@ export function evaluateForecastAccount( const quotaWait = resetAts.length > 0 ? Math.max(...resetAts) - now : waitMs; waitMs = Math.max(waitMs, quotaWait); if (availability === "ready") availability = "delayed"; + exhausted = true; riskScore += 60; reasons.push("quota cache exhausted"); appendWaitReason(reasons, "quota resets in", quotaWait); @@ -332,6 +339,7 @@ export function evaluateForecastAccount( reasons, hardFailure, disabled, + exhausted, }; } @@ -380,22 +388,27 @@ export function recommendForecastAccount( // exhausted) in addition to disabled/hard-failed ones. Such accounts carry // availability === "unavailable" with hardFailure === false, so without this // guard they were recommended with a misleading "pick shortest wait". + // Quota-exhausted accounts are classified "delayed" (not "unavailable") to + // preserve display/sorting semantics, so exclude them explicitly via the + // `exhausted` flag — otherwise an all-exhausted pool returns a bogus + // "shortest wait" pick instead of the null recommendation. const candidates = results.filter( (result) => !result.disabled && !result.hardFailure && + !result.exhausted && result.availability !== "unavailable", ); if (candidates.length === 0) { // Distinguish "blocked/exhausted" accounts (unavailable but neither - // disabled nor hard-failed — e.g. policy block, runtime skip, quota - // exhaustion) from disabled/hard-failed ones so the guidance matches the - // actual blocker. + // disabled nor hard-failed — e.g. policy block, runtime skip — or + // quota-exhausted "delayed" accounts) from disabled/hard-failed ones so + // the guidance matches the actual blocker. const hasBlockedOrExhausted = results.some( (result) => - result.availability === "unavailable" && !result.disabled && - !result.hardFailure, + !result.hardFailure && + (result.exhausted || result.availability === "unavailable"), ); return { recommendedIndex: null, diff --git a/lib/local-client-tokens.ts b/lib/local-client-tokens.ts index 60e352c3a..acfc938de 100644 --- a/lib/local-client-tokens.ts +++ b/lib/local-client-tokens.ts @@ -27,6 +27,14 @@ export interface CreatedLocalClientToken { const TOKEN_FILE_NAME = "local-client-tokens.json"; const TOKEN_PREFIX = "cma_local"; +// Debounce window for persisting a record's lastUsedAt. Bearer verification is +// on the auth hot path (every authenticated bridge request), so writing the +// store to disk on each verify serializes behind the shared write queue and +// triggers a temp-write+rename per request (with Windows rename-lock retries). +// lastUsedAt is informational only (surfaced by `bridge token list`), so we +// coalesce updates: advance it in-memory every verify but only flush to disk +// once it has moved at least this far past the persisted value. +const LAST_USED_PERSIST_THRESHOLD_MS = 60_000; const RETRYABLE_FS_CODES = new Set([ "EBUSY", "EPERM", @@ -154,7 +162,21 @@ export async function loadLocalClientTokenStore(): Promise { const path = getLocalClientTokenPath(); const payload = normalizeStore(store); - await fs.mkdir(getCodexMultiAuthDir(), { recursive: true, mode: 0o700 }); + const dir = getCodexMultiAuthDir(); + // This store holds token hashes, so keep the directory owner-only on POSIX + // (mode is a no-op on win32 / ACL-based). + await fs.mkdir(dir, { recursive: true, mode: 0o700 }); + // mkdir's mode only applies to a freshly-created dir; an upgrade with a + // pre-existing multi-auth dir keeps its old (possibly world-listable) perms, + // so re-assert 0o700 on POSIX. Best-effort: a chmod failure must not break + // the write (the 0o600 file below still protects the hashes). + if (process.platform !== "win32") { + try { + await fs.chmod(dir, 0o700); + } catch { + // Best-effort hardening only. + } + } const tempPath = `${path}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`; let moved = false; try { @@ -268,8 +290,17 @@ export async function verifyLocalClientBearerToken( (entry) => entry.revokedAt === null && entry.tokenHash === tokenHash, ); if (!record) return null; + // Token match (verification correctness) is decided above and never + // depends on lastUsedAt. Always advance lastUsedAt in-memory so callers + // see a fresh value, but only flush to disk when it has not been + // persisted yet, or has advanced past the debounce threshold. This keeps + // steady-state verifies off the disk-write path while still recording + // recent usage on a coarse (>=60s) cadence. + const persisted = record.lastUsedAt; record.lastUsedAt = now; - await writeStoreToDisk(store); + if (persisted === null || now - persisted >= LAST_USED_PERSIST_THRESHOLD_MS) { + await writeStoreToDisk(store); + } return record; }); } diff --git a/lib/request/fetch-helpers.ts b/lib/request/fetch-helpers.ts index b3ef3b4b1..3770bbe85 100644 --- a/lib/request/fetch-helpers.ts +++ b/lib/request/fetch-helpers.ts @@ -194,6 +194,7 @@ function isUnsupportedCodexModelForChatGpt(status: number, bodyText: string): bo if (!bodyText) return false; return ( CHATGPT_CODEX_UNSUPPORTED_MODEL_PATTERN.test(bodyText) || + NORMALIZED_UNSUPPORTED_MODEL_PATTERN.test(bodyText) || MODEL_ACCESS_DENIED_PATTERN.test(bodyText) ); } diff --git a/lib/storage/record-utils.ts b/lib/storage/record-utils.ts index 33052369e..ecbe1b627 100644 --- a/lib/storage/record-utils.ts +++ b/lib/storage/record-utils.ts @@ -4,5 +4,9 @@ export function isRecord(value: unknown): value is Record { export function clampIndex(index: number, length: number): number { if (length <= 0) return 0; + // A tampered/corrupt activeIndex can be NaN; Math.trunc(NaN) is NaN and would + // propagate through Math.min/Math.max, yielding NaN and undefined array access. + // Coerce it to the first valid index. (±Infinity still clamp correctly below.) + if (Number.isNaN(index)) return 0; return Math.max(0, Math.min(Math.trunc(index), length - 1)); } diff --git a/package.json b/package.json index d6ef9fa79..75941355f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "codex-multi-auth", - "version": "2.2.0", + "version": "2.2.1", "description": "Codex CLI multi-account OAuth manager with account switching, health checks, runtime rotation, diagnostics, and recovery tools for @openai/codex", "main": "./dist/index.js", "types": "./dist/index.d.ts", diff --git a/scripts/mcodex.js b/scripts/mcodex.js index 99372e3e5..4b48a23b4 100644 --- a/scripts/mcodex.js +++ b/scripts/mcodex.js @@ -37,6 +37,35 @@ function defaultWarn(message) { console.error(message); } +// When mcodex itself is asked to terminate, forward the signal to the spawned +// child so the forwarded codex.js / watch process doesn't outlive us as an +// orphan. Without this, a SIGTERM to the launcher exits the parent while the +// child keeps running detached. Listeners are registered with `once` and torn +// down when the child settles (returned cleanup), so they never leak across the +// launcher's lifetime. `proc` is injectable so the wiring is unit-testable +// without registering handlers on the real process. +export function relaySignalsToChild( + child, + { proc = process, signals = ["SIGTERM", "SIGINT"] } = {}, +) { + const registered = signals.map((signal) => { + const handler = () => { + try { + child.kill(signal); + } catch { + // Child may have already exited; nothing left to forward. + } + }; + proc.once(signal, handler); + return { signal, handler }; + }); + return function removeSignalRelays() { + for (const { signal, handler } of registered) { + proc.removeListener(signal, handler); + } + }; +} + function coerceValidatedSetting(rawValue, pattern, fallback, envName, warn) { // Mirror bash `${VAR:-default}`: unset OR empty falls back silently; any other // value is validated and, if it fails, replaced with the default plus a warning. @@ -131,13 +160,16 @@ function forwardToCodexWrapper(forwardArgs, env = process.env) { stdio: "inherit", env, }); + const removeSignalRelays = relaySignalsToChild(child); child.once("error", (error) => { + removeSignalRelays(); console.error( `mcodex: failed to launch codex wrapper: ${error instanceof Error ? error.message : String(error)}`, ); process.exit(1); }); child.once("close", (code, signal) => { + removeSignalRelays(); if (signal === "SIGINT") { process.exit(130); return; @@ -156,13 +188,16 @@ function runMonitor(interval, env = process.env, platform = process.platform) { return; } const child = spawn(watchPath, buildWatchArgs(interval), { stdio: "inherit", env }); + const removeSignalRelays = relaySignalsToChild(child); child.once("error", (error) => { + removeSignalRelays(); console.error( `mcodex: failed to launch watch: ${error instanceof Error ? error.message : String(error)}`, ); process.exit(1); }); child.once("close", (code, signal) => { + removeSignalRelays(); if (signal === "SIGINT") { process.exit(130); return; diff --git a/test/fetch-helpers.test.ts b/test/fetch-helpers.test.ts index e7c165312..c5eed843a 100644 --- a/test/fetch-helpers.test.ts +++ b/test/fetch-helpers.test.ts @@ -1186,6 +1186,28 @@ describe('createEntitlementErrorResponse', () => { }), ); }); + + it('rewrites normalized "not currently available" 400 wording as an entitlement error', async () => { + // request-fetch: a 400 body using the normalized + // "not currently available for this chatgpt account" wording (without the + // legacy "not supported when using codex" phrasing) must still trigger the + // entitlement-error rewrite + fallback guidance, matching + // getUnsupportedCodexModelInfo. + const response = new Response( + "The model 'gpt-5.3-codex' is not currently available for this chatgpt account.", + { status: 400 }, + ); + + const { response: result } = await handleErrorResponse(response); + const json = (await result.json()) as { + error: { message: string; type?: string; code?: string; unsupported_model?: string }; + }; + + expect(json.error.type).toBe('entitlement_error'); + expect(json.error.code).toBe('model_not_supported_with_chatgpt_account'); + expect(json.error.unsupported_model).toBe('gpt-5.3-codex'); + expect(json.error.message).toContain('entitlement gate'); + }); }); describe('handleErrorResponse edge cases', () => { diff --git a/test/forecast.test.ts b/test/forecast.test.ts index d6deee944..ad4547def 100644 --- a/test/forecast.test.ts +++ b/test/forecast.test.ts @@ -751,4 +751,64 @@ describe("forecast helpers", () => { expect(recommendation.recommendedIndex).toBeNull(); expect(recommendation.reason).toContain("blocked or exhausted"); }); + + it("returns null recommendation when all accounts are quota-exhausted", () => { + // Quota-exhausted accounts are classified availability === "delayed" (not + // "unavailable") so display/sorting still treat them as a timed wait. The + // recommendation must still return null instead of a misleading "pick + // shortest wait" when every account in the pool is exhausted. + const now = 1_700_000_000_000; + const accounts = [ + { + email: "a@example.com", + accountId: "acc_a", + refreshToken: "refresh-a", + addedAt: now - 10_000, + lastUsed: now - 10_000, + }, + { + email: "b@example.com", + accountId: "acc_b", + refreshToken: "refresh-b", + addedAt: now - 10_000, + lastUsed: now - 10_000, + }, + ]; + const quotaCache = { + version: 1 as const, + updatedAt: now, + byAccountId: { + acc_a: { + accountId: "acc_a", + status: 200, + model: "gpt-5.3-codex", + updatedAt: now, + primary: { usedPercent: 100, resetAtMs: now + 60_000 }, + secondary: { usedPercent: 100, resetAtMs: now + 120_000 }, + }, + acc_b: { + accountId: "acc_b", + status: 200, + model: "gpt-5.3-codex", + updatedAt: now, + primary: { usedPercent: 100, resetAtMs: now + 90_000 }, + secondary: { usedPercent: 100, resetAtMs: now + 180_000 }, + }, + }, + byEmail: {}, + }; + const results = evaluateForecastAccounts([ + { index: 0, now, isCurrent: true, account: accounts[0], allAccounts: accounts, quotaCache }, + { index: 1, now, isCurrent: false, account: accounts[1], allAccounts: accounts, quotaCache }, + ]); + + // Sanity: both are "delayed" + exhausted, neither disabled nor hard-failed. + expect(results.every((r) => r.availability === "delayed")).toBe(true); + expect(results.every((r) => r.exhausted)).toBe(true); + expect(results.every((r) => !r.disabled && !r.hardFailure)).toBe(true); + + const recommendation = recommendForecastAccount(results); + expect(recommendation.recommendedIndex).toBeNull(); + expect(recommendation.reason).toContain("blocked or exhausted"); + }); }); diff --git a/test/local-client-tokens.test.ts b/test/local-client-tokens.test.ts index b2ffd0002..de6c019d1 100644 --- a/test/local-client-tokens.test.ts +++ b/test/local-client-tokens.test.ts @@ -81,6 +81,52 @@ describe("local client tokens", () => { expect(store.tokens.filter((token) => token.revokedAt !== null)).toHaveLength(2); }); + it("debounces lastUsedAt persistence across rapid verifies", async () => { + const { addLocalClientToken, loadLocalClientTokenStore, verifyLocalClientBearerToken } = + await import("../lib/local-client-tokens.js"); + + const created = await addLocalClientToken({ label: "hot-path", now: 100 }); + + // Seed a persisted lastUsedAt so subsequent rapid verifies fall inside the + // debounce window rather than the "never persisted" branch. + await verifyLocalClientBearerToken(`Bearer ${created.plainToken}`, 1_000); + + // From here, every verify within the threshold must stay in-memory: no + // temp-write + rename per request on the auth hot path. + const renameSpy = vi.spyOn(fs, "rename"); + try { + for (let i = 1; i <= 5; i += 1) { + const verified = await verifyLocalClientBearerToken( + `Bearer ${created.plainToken}`, + 1_000 + i, + ); + // Verification correctness is unchanged, and lastUsedAt advances + // in-memory on each call. + expect(verified?.id).toBe(created.record.id); + expect(verified?.lastUsedAt).toBe(1_000 + i); + } + expect(renameSpy).not.toHaveBeenCalled(); + + // On disk it is still the last persisted value (debounced). + const debounced = await loadLocalClientTokenStore(); + expect(debounced.tokens[0]?.lastUsedAt).toBe(1_000); + + // Once the in-memory value advances past the threshold, the next verify + // flushes to disk so usage data is not lost indefinitely. + const flushed = await verifyLocalClientBearerToken( + `Bearer ${created.plainToken}`, + 1_000 + 60_000, + ); + expect(flushed?.lastUsedAt).toBe(1_000 + 60_000); + expect(renameSpy).toHaveBeenCalledTimes(1); + } finally { + renameSpy.mockRestore(); + } + + const persisted = await loadLocalClientTokenStore(); + expect(persisted.tokens[0]?.lastUsedAt).toBe(1_000 + 60_000); + }); + it("does not lose mutations when revoke and add run concurrently", async () => { const { addLocalClientToken, @@ -116,30 +162,33 @@ describe("local client tokens", () => { expect(addedRecord?.revokedAt).toBeNull(); }); - it("retries atomic rename on transient ENOTEMPTY errors", async () => { - const { addLocalClientToken, loadLocalClientTokenStore } = await import( - "../lib/local-client-tokens.js" - ); - const realRename = fs.rename.bind(fs); - let attempts = 0; - const renameSpy = vi.spyOn(fs, "rename"); - renameSpy.mockImplementation(async (...args) => { - attempts += 1; - if (attempts === 1) { - const error = new Error("dir not empty") as NodeJS.ErrnoException; - error.code = "ENOTEMPTY"; - throw error; + it.each(["ENOTEMPTY", "EAGAIN", "EACCES"])( + "retries atomic rename on transient %s errors", + async (code) => { + const { addLocalClientToken, loadLocalClientTokenStore } = await import( + "../lib/local-client-tokens.js" + ); + const realRename = fs.rename.bind(fs); + let attempts = 0; + const renameSpy = vi.spyOn(fs, "rename"); + renameSpy.mockImplementation(async (...args) => { + attempts += 1; + if (attempts === 1) { + const error = new Error(`transient ${code}`) as NodeJS.ErrnoException; + error.code = code; + throw error; + } + return realRename(...args); + }); + + try { + const created = await addLocalClientToken({ label: "retry", now: 100 }); + expect(attempts).toBeGreaterThan(1); + const store = await loadLocalClientTokenStore(); + expect(store.tokens.find((t) => t.id === created.record.id)).toBeDefined(); + } finally { + renameSpy.mockRestore(); } - return realRename(...args); - }); - - try { - const created = await addLocalClientToken({ label: "retry", now: 100 }); - expect(attempts).toBeGreaterThan(1); - const store = await loadLocalClientTokenStore(); - expect(store.tokens.find((t) => t.id === created.record.id)).toBeDefined(); - } finally { - renameSpy.mockRestore(); - } - }); + }, + ); }); diff --git a/test/mcodex-launcher.test.ts b/test/mcodex-launcher.test.ts index f1ed3eb69..57d6b2e7d 100644 --- a/test/mcodex-launcher.test.ts +++ b/test/mcodex-launcher.test.ts @@ -1,10 +1,12 @@ import { dirname, join } from "node:path"; +import { EventEmitter } from "node:events"; import { promises as fs } from "node:fs"; import { fileURLToPath, pathToFileURL } from "node:url"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { isDirectRunInvocation, parseMcodexArgs, + relaySignalsToChild, resolveMonitorInterval, resolveTmuxHistoryLimit, resolveTmuxSession, @@ -181,3 +183,69 @@ describe("mcodex direct-run gate (isDirectRunInvocation)", () => { expect(isDirectRunInvocation(undefined, selfUrl)).toBe(false); }); }); + +describe("mcodex signal relay (relaySignalsToChild)", () => { + // #15 regression: a SIGTERM to the launcher used to exit the parent without + // killing the forwarded codex.js / watch child, orphaning it. The launcher now + // relays terminating signals to the child and tears the handlers down on close. + // A true cross-process SIGTERM test is unreliable on Windows (process.kill maps + // to TerminateProcess and bypasses Node's signal handler), so assert the relay + // wiring deterministically with an injected fake process + child. + function makeFakeChild() { + const kill = vi.fn(); + return { kill } as unknown as import("node:child_process").ChildProcess & { + kill: ReturnType; + }; + } + + it("forwards a received signal to the child via child.kill", () => { + const proc = new EventEmitter(); + const child = makeFakeChild(); + relaySignalsToChild(child, { proc, signals: ["SIGTERM"] }); + + expect(proc.listenerCount("SIGTERM")).toBe(1); + proc.emit("SIGTERM"); + expect(child.kill).toHaveBeenCalledWith("SIGTERM"); + }); + + it("registers SIGTERM and SIGINT by default", () => { + const proc = new EventEmitter(); + const child = makeFakeChild(); + relaySignalsToChild(child, { proc }); + + expect(proc.listenerCount("SIGTERM")).toBe(1); + expect(proc.listenerCount("SIGINT")).toBe(1); + + proc.emit("SIGINT"); + expect(child.kill).toHaveBeenCalledWith("SIGINT"); + }); + + it("removes the relay handlers when cleanup runs (no leak past child close)", () => { + const proc = new EventEmitter(); + const child = makeFakeChild(); + const removeSignalRelays = relaySignalsToChild(child, { + proc, + signals: ["SIGTERM", "SIGINT"], + }); + + removeSignalRelays(); + expect(proc.listenerCount("SIGTERM")).toBe(0); + expect(proc.listenerCount("SIGINT")).toBe(0); + + // After cleanup a late signal must not reach an already-settled child. + proc.emit("SIGTERM"); + expect(child.kill).not.toHaveBeenCalled(); + }); + + it("swallows a child.kill failure (child already exited)", () => { + const proc = new EventEmitter(); + const kill = vi.fn(() => { + throw new Error("ESRCH"); + }); + const child = { kill } as unknown as import("node:child_process").ChildProcess; + relaySignalsToChild(child, { proc, signals: ["SIGTERM"] }); + + expect(() => proc.emit("SIGTERM")).not.toThrow(); + expect(kill).toHaveBeenCalledWith("SIGTERM"); + }); +}); diff --git a/test/record-utils.test.ts b/test/record-utils.test.ts index f7c40e904..2a43ec51c 100644 --- a/test/record-utils.test.ts +++ b/test/record-utils.test.ts @@ -30,6 +30,15 @@ describe("clampIndex", () => { it("never exceeds the last index for large fractional input", () => { expect(clampIndex(7.5, 3)).toBe(2); }); + + it("coerces a non-finite index to a valid bound", () => { + // A tampered/corrupt activeIndex of NaN must not propagate through + // Math.trunc/Math.min and yield undefined array indexing. + expect(clampIndex(Number.NaN, 5)).toBe(0); + // ±Infinity still clamp to the valid range rather than producing NaN. + expect(clampIndex(Number.POSITIVE_INFINITY, 5)).toBe(4); + expect(clampIndex(Number.NEGATIVE_INFINITY, 5)).toBe(0); + }); }); describe("isRecord", () => { diff --git a/test/storage-flagged.test.ts b/test/storage-flagged.test.ts index f72db4eca..592274ea5 100644 --- a/test/storage-flagged.test.ts +++ b/test/storage-flagged.test.ts @@ -1116,16 +1116,23 @@ describe("flagged storage extracted helpers", () => { ); // Fail loudly on a hang rather than letting the whole suite time out. - const timeout = new Promise((_, reject) => - setTimeout( + let timeoutHandle: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutHandle = setTimeout( () => reject(new Error("withAccountAndFlaggedStorageTransaction deadlocked")), 3000, - ), - ); + ); + }); - const recoveredFlagged = (await Promise.race([run, timeout])) as Awaited< - typeof run - >; + let recoveredFlagged: Awaited; + try { + recoveredFlagged = (await Promise.race([run, timeout])) as Awaited< + typeof run + >; + } finally { + // Clear the guard so it never dangles into teardown on the happy path. + if (timeoutHandle !== undefined) clearTimeout(timeoutHandle); + } expect(recoveredFlagged.accounts).toHaveLength(1); expect(recoveredFlagged.accounts[0]).toEqual( expect.objectContaining({ diff --git a/test/storage.test.ts b/test/storage.test.ts index 1992d92c3..8c6837755 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -7,6 +7,7 @@ import { getConfigDir, getProjectStorageKey } from "../lib/storage/paths.js"; import { setStoragePathState } from "../lib/storage/path-state.js"; import { getIntentionalResetMarkerPath } from "../lib/storage/backup-paths.js"; import { getRuntimeAccountIdentityKey } from "../lib/storage/identity.js"; +import { removeWithRetry } from "./helpers/remove-with-retry.js"; import { buildNamedBackupPath, clearAccounts, @@ -66,7 +67,7 @@ describe("storage", () => { afterEach(async () => { setStoragePathDirect(null); - await fs.rm(storageDir, { recursive: true, force: true }); + await removeWithRetry(storageDir, { recursive: true, force: true }); }); it.skipIf(process.platform === "win32")( From 8e498386e9d4ec9df2d1d526cb235f914940c786 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Wed, 3 Jun 2026 02:56:57 +0800 Subject: [PATCH 6/8] fix(audit): close the 3 heavy concurrency gaps (#8/#9, #14) Completes the deferred architectural items from the audit rather than shipping them as known follow-ups. - config (#8): wrap the CAS stat() in the bounded transient-FS retry so a Windows EBUSY/EPERM on stat no longer aborts an env-path save. - config (#9): add a cross-process file lock (modeled on RefreshLeaseCoordinator: exclusive 'wx' lockfile, pid/expiry payload, stale takeover, finally release) around the env-path read->check->merge->rename critical section, so two processes can no longer lose each other's update. In-process queue + mtime CAS retained as fast path / second-line guard. - runtime proxy routing mutex (#14): make withRoutingMutex reentrant via AsyncLocalStorage (mirrors the H4 storage-lock fix), then run selection + cursor commit inside one acquisition when routingMutex='enabled'. The nested markSwitchedLocked runs inline (reentrant) so there's no double-acquire or deadlock; persistRuntimeActiveAccount's commit is gated to legacy-only to avoid a redundant post-fetch re-advance. Legacy mode behavior + perf unchanged. Each fix ships with a deterministic regression (stat-EBUSY retry; rename-time competing-write interleave; two concurrent enabled-mode requests get distinct accounts). Fix-sensitivity confirmed: reverting the reentrancy guard deadlocks the new proxy test. Updated docs/releases/v2.2.1.md to reflect these as fixed, not deferred. Full suite: 4341 passed, 3 skipped, 0 failed; typecheck + lint clean. --- docs/releases/v2.2.1.md | 39 +++-- lib/config.ts | 235 +++++++++++++++++++++++----- lib/routing-mutex.ts | 44 +++++- lib/runtime-rotation-proxy.ts | 80 ++++++++-- test/config-save.test.ts | 106 +++++++++++++ test/runtime-rotation-proxy.test.ts | 134 ++++++++++++++++ 6 files changed, 567 insertions(+), 71 deletions(-) diff --git a/docs/releases/v2.2.1.md b/docs/releases/v2.2.1.md index 2082fd8bc..0c2b4c99c 100644 --- a/docs/releases/v2.2.1.md +++ b/docs/releases/v2.2.1.md @@ -65,25 +65,36 @@ npm i -g codex-multi-auth@latest excluded, and the forecast returns no recommendation with a clear reason when none are ready. -## LOW +## LOW and follow-up hardening -Nine lower-severity fixes from the same audit: the local-client-token store now -serializes its full read-modify-write (and retries the complete Windows transient -lock taxonomy, including `ENOTEMPTY`, on rename); the Codex CLI state cache honors -`forceRefresh` even with a load in flight, guarded by a load generation so a slow -stale read can't poison the cache; the env-path config save uses an mtime -compare-and-swap with `ESTALE` retry; `clampIndex` floors fractional indices; the -runtime proxy short-circuits storage re-reads on unchanged mtime/size and checks -authorization before path/method (401 before 404); capability-policy eviction is -LRU; the Codex bin resolver skips any PATH candidate inside its own wrapper -directory; and `styleQuotaSummary` clamps out-of-range percentages. +The same audit produced a series of lower-severity fixes, all included here: + +- **Concurrency / Windows filesystem.** The local-client-token store serializes + its full read-modify-write and retries the complete transient lock taxonomy + (`EBUSY`/`EPERM`/`EAGAIN`/`ENOTEMPTY`/`EACCES`) on rename; `lastUsedAt` writes on + the bearer-verify hot path are debounced so steady-state verification stays + in-memory. The Codex CLI state cache honors `forceRefresh` even with a load in + flight, guarded by a load generation so a slow stale read can't overwrite a + fresh snapshot. The runtime proxy short-circuits storage re-reads on unchanged + mtime/size and checks authorization before path/method (401 before 404). +- **Config save coordination.** The env-path config save now retries a transient + `stat` lock and serializes its read-modify-write through a cross-process file + lock (modeled on the refresh-lease coordinator) in addition to the in-process + queue and the mtime compare-and-swap, closing the lost-update window. +- **Routing mutex selection race.** With `routingMutex="enabled"`, account + selection and the cursor commit now run inside a single, reentrant mutex + acquisition, so concurrent requests can no longer read the same cursor and + stampede the same account. Legacy mode is unchanged. +- **Smaller fixes.** `clampIndex` floors fractional indices and coerces `NaN`; + the `mcodex` launcher relays `SIGTERM`/`SIGINT` to its spawned child; + capability-policy eviction is LRU; the Codex bin resolver skips any PATH + candidate inside its own wrapper directory; secret directories re-assert + `0o700`; and `styleQuotaSummary` clamps out-of-range percentages. Two findings are documented as intentional rather than changed: the device-auth endpoint's bare 403/404 responses are its non-RFC-8628 "authorization pending" signal (the poll already exits at the server deadline), and runtime budgets are -deliberately soft/eventually-consistent under concurrency. One proxy -routing-mutex gap is deferred with an inline design note, as closing it requires -making the synchronous selection path async across its call sites. +deliberately soft/eventually-consistent under concurrency. ## Verification diff --git a/lib/config.ts b/lib/config.ts index 2082d7c39..cdb74d042 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -451,15 +451,35 @@ function sleep(ms: number): Promise { } async function getConfigFileMtimeMs(filePath: string): Promise { - try { - return (await fs.stat(filePath)).mtimeMs; - } catch (error) { - const code = (error as NodeJS.ErrnoException | undefined)?.code; - if (code === "ENOENT") { - return null; + // config-08: the mtime CAS preflight (and savePluginConfig's env-path branch) + // run on the Windows-sensitive save path, where a transient EBUSY/EPERM/EAGAIN + // from an AV/indexer lock on a single fs.stat would abort the entire save. + // Mirror readConfigRecordForSave's bounded transient-FS retry (same code set, + // same backoff, 5 attempts). ENOENT still returns null immediately. + let lastError: unknown; + for (let attempt = 0; attempt < 5; attempt += 1) { + try { + return (await fs.stat(filePath)).mtimeMs; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") { + return null; + } + lastError = error; + if ( + typeof code === "string" && + RETRYABLE_CONFIG_READ_CODES.has(code) && + attempt < 4 + ) { + await sleep(10 * 2 ** attempt); + continue; + } + throw error; } - throw error; } + // Exhausted retries on a transient code: surface the last failure so the caller + // treats it as a real save error rather than a phantom missing file. + throw lastError; } async function writeJsonFileAtomicWithRetry( @@ -524,6 +544,136 @@ async function withConfigSaveLock( } } +// Cross-process config-save lock. withConfigSaveLock only serializes saves +// within THIS process (a per-path promise queue); it does nothing against a +// second process. The mtime CAS in writeJsonFileAtomicWithRetry narrows but does +// not close the read→check→merge→rename TOCTOU window (another process can still +// land a write between the CAS stat and the rename). This file lock provides the +// cross-process mutual exclusion that closes that window. It deliberately mirrors +// RefreshLeaseCoordinator (lib/refresh-lease.ts): exclusive `wx` lockfile create, +// JSON payload carrying pid + expiry, stale-takeover via expiry/mtime, and a +// best-effort retrying release in a finally. The mtime CAS stays as a second-line +// guard for any non-participating writer. +const CONFIG_LOCK_TTL_MS = 10_000; +const CONFIG_LOCK_WAIT_TIMEOUT_MS = 10_000; +const CONFIG_LOCK_POLL_MS = 50; + +interface ConfigLockPayload { + pid: number; + acquiredAt: number; + expiresAt: number; +} + +async function unlinkConfigLockWithRetry(lockPath: string): Promise { + for (let attempt = 0; attempt < 5; attempt += 1) { + try { + await fs.unlink(lockPath); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") return; + if ( + typeof code === "string" && + RETRYABLE_FS_CODES.has(code) && + attempt < 4 + ) { + await sleep(10 * 2 ** attempt); + continue; + } + // Best-effort release: a leftover lockfile is recovered as stale by the + // next acquirer via its expiry/mtime, so never fail the save over this. + return; + } + } +} + +async function isConfigLockStale(lockPath: string): Promise { + try { + const content = await fs.readFile(lockPath, "utf-8"); + const parsed = JSON.parse(stripUtf8Bom(content)) as + | Partial + | undefined; + if ( + typeof parsed?.expiresAt === "number" && + Number.isFinite(parsed.expiresAt) + ) { + return parsed.expiresAt <= Date.now(); + } + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") return true; + // Unreadable/malformed payload: fall through to the mtime heuristic. + } + try { + const stat = await fs.stat(lockPath); + return Date.now() - stat.mtimeMs > CONFIG_LOCK_TTL_MS; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") return true; + return false; + } +} + +async function withConfigFileLock( + targetPath: string, + task: () => Promise, +): Promise { + const lockPath = `${targetPath}.lock`; + await fs.mkdir(dirname(lockPath), { recursive: true }); + const deadline = Date.now() + CONFIG_LOCK_WAIT_TIMEOUT_MS; + let acquired = false; + while (!acquired) { + try { + const now = Date.now(); + const payload: ConfigLockPayload = { + pid: process.pid, + acquiredAt: now, + expiresAt: now + CONFIG_LOCK_TTL_MS, + }; + const handle = await fs.open(lockPath, "wx", 0o600); + try { + await handle.writeFile(`${JSON.stringify(payload)}\n`, "utf8"); + } finally { + await handle.close(); + } + acquired = true; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code !== "EEXIST") { + // A transient FS lock on the lockfile itself: retry until the deadline, + // matching the bounded backoff used elsewhere on the Windows save path. + if ( + typeof code === "string" && + RETRYABLE_FS_CODES.has(code) && + Date.now() < deadline + ) { + await sleep(CONFIG_LOCK_POLL_MS); + continue; + } + throw error; + } + // Lock is held. Take it over if the holder expired/crashed; otherwise wait. + if (await isConfigLockStale(lockPath)) { + await unlinkConfigLockWithRetry(lockPath); + continue; + } + if (Date.now() >= deadline) { + const timeoutError = new Error( + `Timed out acquiring config save lock at ${lockPath}.`, + ) as NodeJS.ErrnoException; + timeoutError.code = "ELOCKTIMEOUT"; + throw timeoutError; + } + await sleep(CONFIG_LOCK_POLL_MS); + } + } + try { + return await task(); + } finally { + await unlinkConfigLockWithRetry(lockPath); + } +} + /** * Read and parse a JSON configuration file and return its top-level object when present and valid. * @@ -736,42 +886,45 @@ export async function savePluginConfig( if (envPath.length > 0) { await withConfigSaveLock(envPath, async () => { - // Cross-process compare-and-swap: capture mtime before the - // read-merge-write, then have the atomic writer re-check it before the - // rename. On mismatch (another process wrote concurrently) we re-read and - // retry instead of silently dropping the other process's update. Mirrors - // the unified-settings save path (writeSettingsRecordAsync CAS). - for (let attempt = 0; attempt < 3; attempt += 1) { - const expectedMtimeMs = await getConfigFileMtimeMs(envPath); - const envConfigState = await readConfigRecordForSave(envPath); - if (envConfigState.status === "unreadable") { - throw new Error( - `Aborting config save because ${envPath} is unreadable.`, - ); - } - const existingConfig = - envConfigState.status === "ok" - ? sanitizeStoredPluginConfigRecord(envConfigState.record) - : null; - const merged = { - ...(existingConfig ?? {}), - ...sanitizedPatch, - }; - try { - await writeJsonFileAtomicWithRetry(envPath, merged, { - expectedMtimeMs, - }); - return; - } catch (error) { - if ( - (error as NodeJS.ErrnoException).code !== "ESTALE" || - attempt >= 2 - ) { - throw error; + // In-process queue (above) is the cheap fast path; the cross-process file + // lock (below) closes the read→merge→rename TOCTOU window against OTHER + // processes. Acquire the file lock for the full critical section, then run + // the same mtime CAS retry as a second-line guard for any non-participating + // writer. Mirrors the unified-settings save path (writeSettingsRecordAsync + // CAS). + await withConfigFileLock(envPath, async () => { + for (let attempt = 0; attempt < 3; attempt += 1) { + const expectedMtimeMs = await getConfigFileMtimeMs(envPath); + const envConfigState = await readConfigRecordForSave(envPath); + if (envConfigState.status === "unreadable") { + throw new Error( + `Aborting config save because ${envPath} is unreadable.`, + ); + } + const existingConfig = + envConfigState.status === "ok" + ? sanitizeStoredPluginConfigRecord(envConfigState.record) + : null; + const merged = { + ...(existingConfig ?? {}), + ...sanitizedPatch, + }; + try { + await writeJsonFileAtomicWithRetry(envPath, merged, { + expectedMtimeMs, + }); + return; + } catch (error) { + if ( + (error as NodeJS.ErrnoException).code !== "ESTALE" || + attempt >= 2 + ) { + throw error; + } + // Loop: re-stat, re-read, re-merge against the latest on-disk state. } - // Loop: re-stat, re-read, re-merge against the latest on-disk state. } - } + }); }); return; } diff --git a/lib/routing-mutex.ts b/lib/routing-mutex.ts index 6631569f3..d3f508641 100644 --- a/lib/routing-mutex.ts +++ b/lib/routing-mutex.ts @@ -26,6 +26,8 @@ * singleton; OS-level mutexes are out of scope for this PR. */ +import { AsyncLocalStorage } from "node:async_hooks"; + export type RoutingMutexMode = "enabled" | "legacy"; /** @@ -124,6 +126,33 @@ export function createAsyncMutex(): AsyncMutex { let routingMutexInstance: AsyncMutex | null = null; +/** + * Reentrancy guard for `withRoutingMutex`. + * + * The underlying `runExclusive` queue is a strictly non-reentrant FIFO: a task + * that is *already* holding the mutex and then calls `withRoutingMutex` again + * would enqueue behind itself and deadlock (the outer task can never settle + * because it is awaiting the inner task, which can never start because the + * outer task still holds the lock). + * + * This mirrors `isStorageLockHeld` / `storageLockHeldContext` in + * `lib/storage/transactions.ts`. When a critical section needs to span several + * cursor mutations that each route through `withRoutingMutex` (e.g. the runtime + * proxy holds the mutex around `chooseAccount` selection AND the later + * `persistRuntimeActiveAccount` commit), the nested calls must run inline + * within the already-held section instead of re-acquiring. + */ +const routingMutexHeldContext = new AsyncLocalStorage(); + +/** + * Reports whether the caller is already running inside a `withRoutingMutex` + * critical section (in `"enabled"` mode). Callers that may run both standalone + * and nested under a held mutex use this to avoid re-acquiring and deadlocking. + */ +export function isRoutingMutexHeld(): boolean { + return routingMutexHeldContext.getStore() === true; +} + /** * Singleton accessor for the rotation critical-section mutex. * @@ -150,13 +179,26 @@ export function __resetRoutingMutexForTests(): void { * Run `fn` under the routing mutex when `mode === "enabled"`, otherwise run * it inline. Hot-path helper used by account-pool mutation sites so the * flag check stays O(1) per call. + * + * Reentrant: if the caller is already inside a held `withRoutingMutex` section + * (`isRoutingMutexHeld()` is true), `fn` runs inline rather than re-acquiring + * the non-reentrant FIFO queue, which would otherwise deadlock. When the lock + * is acquired, `fn` runs inside `routingMutexHeldContext` so any nested + * `withRoutingMutex` calls it makes are detected as reentrant. */ export async function withRoutingMutex( mode: RoutingMutexMode, fn: () => Promise | T, ): Promise { if (mode === "enabled") { - return getRoutingMutex().runExclusive(fn); + if (isRoutingMutexHeld()) { + // Already inside the critical section for this async context: run + // inline so we don't enqueue behind ourselves and deadlock. + return await fn(); + } + return getRoutingMutex().runExclusive(() => + routingMutexHeldContext.run(true, fn), + ); } return await fn(); } diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index 7b7ee0c72..f648ae27c 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -7,6 +7,7 @@ import { extractAccountId, type ManagedAccount, } from "./accounts.js"; +import { withRoutingMutex } from "./routing-mutex.js"; import { getStoragePath } from "./storage.js"; import { getFetchTimeoutMs, @@ -555,7 +556,21 @@ async function persistRuntimeActiveAccount( // (syncCodexCliActiveSelectionForIndex), which is the lost-update window the // mutex exists to close. In legacy mode markSwitchedLocked runs inline, so // behavior is unchanged by default. - await accountManager.markSwitchedLocked(account, "rotation", family); + // + // L4 fix: in "enabled" mode the SELECTION path already committed the cursor + // for this account inside the routing mutex (atomic select+commit, see the + // hot-path caller). Re-running markSwitchedLocked here — after the upstream + // fetch, in a *separate* critical section — would redundantly re-advance the + // cursor and could clobber a concurrent request's atomic advance that landed + // while this request was awaiting upstream. So only do the locked cursor + // commit in legacy mode, where selection does NOT commit under a lock and + // this remains the sole commit site (preserving legacy behavior exactly). + // `saveToDiskDebounced` + CLI sync still run in both modes: they snapshot the + // current in-memory state / mirror the CLI selection and do not advance the + // in-memory rotation cursor. + if (accountManager.getRoutingMutexMode() !== "enabled") { + await accountManager.markSwitchedLocked(account, "rotation", family); + } accountManager.saveToDiskDebounced(); await accountManager.syncCodexCliActiveSelectionForIndex(account.index); } catch { @@ -1722,20 +1737,55 @@ export async function startRuntimeRotationProxy( now() - lastGlobalSwitchAt < minRotationIntervalMs ? { [lastGlobalAccountIndex]: 1000 } : {}; - const selected = chooseAccount({ - accountManager, - sessionAffinityStore, - sessionKey: context.sessionKey, - family: context.family, - model: context.model, - attemptedIndexes, - now: now(), - policy: policyDecision, - pinnedIndex, - skipReasons: accountSkipReasons, - stickyBoostByAccount: rotationStickyBoost, - pidOffsetEnabled, - }); + // L4 fix (routing mutex): when `routingMutex === "enabled"`, run the + // selection AND the cursor commit inside ONE mutex acquisition so two + // concurrent requests cannot read the same cursor and stampede before + // the locked commit lands. `chooseAccount` is sync and mutates the + // cursor internally (session-affinity `markSwitched`, the hybrid + // selector's own advance, and the round-robin fallback `markSwitched`); + // holding the mutex across the whole call serializes all of those. + // We then `markSwitchedLocked` the winner to (a) re-commit the cursor + // under the lock across the await boundary and (b) hand + // `persistRuntimeActiveAccount` a cursor that is already correct. That + // later `markSwitchedLocked` runs INLINE (reentrant) within this held + // section, so there is no double-acquire and no deadlock on the + // non-reentrant FIFO queue. In legacy mode the inline `markSwitched` + // calls inside `chooseAccount` are used unchanged and no lock is taken, + // so default behavior and perf are identical to before. + const selectAccount = (): ManagedAccount | null => + chooseAccount({ + accountManager, + sessionAffinityStore, + sessionKey: context.sessionKey, + family: context.family, + model: context.model, + attemptedIndexes, + now: now(), + policy: policyDecision, + pinnedIndex, + skipReasons: accountSkipReasons, + stickyBoostByAccount: rotationStickyBoost, + pidOffsetEnabled, + }); + const selected = + routingMutexMode === "enabled" + ? await withRoutingMutex(routingMutexMode, async () => { + const candidate = selectAccount(); + if (candidate && pinnedIndex === null) { + // Re-commit the cursor under the held mutex. Skipped when a + // manual pin is active so the proxy never clobbers the pin + // (see #474); pinned selections are deterministic and need no + // cursor advance. Runs inline via reentrancy — see comment + // above. + await accountManager.markSwitchedLocked( + candidate, + "rotation", + context.family, + ); + } + return candidate; + }) + : selectAccount(); if (!selected) { if ( !reloadedAfterNoAccount && diff --git a/test/config-save.test.ts b/test/config-save.test.ts index 3c757919d..e0cee5bb2 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -186,6 +186,112 @@ describe("plugin config save paths", () => { expect(parsed.preserved).toBe(1); }); + it("retries a transient stat failure during an env-path save (config-08)", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile( + configPath, + JSON.stringify({ codexMode: true, preserved: 1 }), + "utf8", + ); + + const originalStat = fs.stat.bind(fs); + let injectedStatFailure = false; + const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (...args) => { + // One-shot EBUSY on the first mtime probe of the config file. Without the + // bounded retry in getConfigFileMtimeMs, this transient Windows lock would + // abort the whole save. + if (String(args[0]) === configPath && !injectedStatFailure) { + injectedStatFailure = true; + const error = new Error("busy") as NodeJS.ErrnoException; + error.code = "EBUSY"; + throw error; + } + return originalStat(...(args as Parameters)); + }); + + try { + const { savePluginConfig } = await import("../lib/config.js"); + await savePluginConfig({ fastSession: true }); + } finally { + statSpy.mockRestore(); + } + + expect(injectedStatFailure).toBe(true); + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.fastSession).toBe(true); + expect(parsed.codexMode).toBe(true); + expect(parsed.preserved).toBe(1); + }); + + it("does not lose a concurrent env-path update injected at rename time (config-09)", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile( + configPath, + JSON.stringify({ codexMode: true, preserved: 1 }), + "utf8", + ); + // Pin a known starting mtime so the simulated concurrent write produces a + // clearly different timestamp. + const baseTime = new Date("2026-01-01T00:00:00.000Z"); + await fs.utimes(configPath, baseTime, baseTime); + + const originalRename = fs.rename.bind(fs); + let injectedConcurrentWrite = false; + const renameSpy = vi + .spyOn(fs, "rename") + .mockImplementation(async (...args) => { + const dest = String(args[1]); + // Simulate another process landing a write at the very last moment: + // AFTER our mtime CAS passed but BEFORE our rename commits — the window + // the existing readFile-injection test cannot reach. The atomic writer + // surfaces ESTALE; the second-line CAS loop must re-read and re-merge so + // the competing key is not clobbered. + if (dest === configPath && !injectedConcurrentWrite) { + injectedConcurrentWrite = true; + const concurrentTime = new Date("2026-01-02T00:00:00.000Z"); + await fs.writeFile( + configPath, + JSON.stringify({ + codexMode: true, + preserved: 1, + concurrentKey: "from-other-process", + }), + "utf8", + ); + await fs.utimes(configPath, concurrentTime, concurrentTime); + const staleError = new Error( + "config changed during rename", + ) as NodeJS.ErrnoException; + staleError.code = "ESTALE"; + throw staleError; + } + return originalRename(...(args as Parameters)); + }); + + try { + const { savePluginConfig } = await import("../lib/config.js"); + await savePluginConfig({ fastSession: true }); + } finally { + renameSpy.mockRestore(); + } + + expect(injectedConcurrentWrite).toBe(true); + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + // The last-moment competing key survives and our patch still applies. + expect(parsed.concurrentKey).toBe("from-other-process"); + expect(parsed.fastSession).toBe(true); + expect(parsed.codexMode).toBe(true); + expect(parsed.preserved).toBe(1); + }); + it("writes through unified settings when env path is unset", async () => { delete process.env.CODEX_MULTI_AUTH_CONFIG_PATH; const unifiedPath = join(tempDir, "settings.json"); diff --git a/test/runtime-rotation-proxy.test.ts b/test/runtime-rotation-proxy.test.ts index 6366df977..c5018a3aa 100644 --- a/test/runtime-rotation-proxy.test.ts +++ b/test/runtime-rotation-proxy.test.ts @@ -8,6 +8,11 @@ import { type RuntimeRotationProxyServer, } from "../lib/runtime-rotation-proxy.js"; import { clearCircuitBreakers } from "../lib/circuit-breaker.js"; +import { + __resetRoutingMutexForTests, + isRoutingMutexHeld, + withRoutingMutex, +} from "../lib/routing-mutex.js"; import * as runtimePolicy from "../lib/policy/runtime-policy.js"; import { resetRefreshQueue } from "../lib/refresh-queue.js"; import { resetTrackers } from "../lib/rotation.js"; @@ -288,6 +293,7 @@ beforeEach(() => { resetTrackers(); clearCircuitBreakers(); resetRefreshQueue(); + __resetRoutingMutexForTests(); refreshAccessTokenMock.mockReset(); saveAccountsMock.mockReset(); saveAccountsMock.mockResolvedValue(undefined); @@ -307,6 +313,7 @@ afterEach(async () => { resetTrackers(); clearCircuitBreakers(); resetRefreshQueue(); + __resetRoutingMutexForTests(); }); describe("runtime rotation proxy", () => { @@ -2216,6 +2223,133 @@ describe("runtime rotation proxy", () => { }); }); +describe("routing mutex serializes selection + cursor commit (issue #14 / L4)", () => { + const prevEnv = process.env.CODEX_AUTH_ROUTING_MUTEX; + + afterEach(() => { + if (prevEnv === undefined) delete process.env.CODEX_AUTH_ROUTING_MUTEX; + else process.env.CODEX_AUTH_ROUTING_MUTEX = prevEnv; + __resetRoutingMutexForTests(); + }); + + // Deterministic, fix-sensitive proof of the property the hot path depends on: + // when `routingMutex === "enabled"`, a "select + commit" critical section that + // spans an await must NOT let a second critical section begin its selection + // until the first has fully committed. The pre-fix code committed the cursor in + // a SEPARATE mutex acquisition (markSwitched ran unlocked during selection, + // markSwitchedLocked ran later in persist), so two requests could both select + // before either committed. This test models that exact shape at the mutex layer + // and asserts strict serialization (maxConcurrent === 1) plus reentrancy. + it("never overlaps two enabled-mode select+commit sections, and is reentrant", async () => { + __resetRoutingMutexForTests(); + let active = 0; + let maxConcurrent = 0; + const selectionOrder: number[] = []; + // Gate that holds the FIRST critical section open across an await, so that if + // selection were allowed outside the lock the second request's selection + // would interleave here and bump maxConcurrent to 2. + let releaseFirst: (() => void) | undefined; + const firstHeld = new Promise((resolve) => { + releaseFirst = resolve; + }); + + const runSelectCommit = (id: number, gateOpen: boolean): Promise => + withRoutingMutex("enabled", async () => { + // SELECTION happens here, inside the held mutex. + selectionOrder.push(id); + active += 1; + maxConcurrent = Math.max(maxConcurrent, active); + // Reentrancy: the real hot path calls markSwitchedLocked (which itself + // routes through withRoutingMutex) WHILE already holding the mutex. + // That nested acquisition must run inline rather than deadlock. + expect(isRoutingMutexHeld()).toBe(true); + await withRoutingMutex("enabled", async () => { + expect(isRoutingMutexHeld()).toBe(true); + // COMMIT happens here, still inside the same held section. + }); + if (gateOpen) { + // Hold the first section open until the second has been scheduled. + await firstHeld; + } + active -= 1; + }); + + const first = runSelectCommit(0, true); + // Ensure the first task has acquired the mutex before scheduling the second. + await Promise.resolve(); + const second = runSelectCommit(1, false); + // Let the event loop spin: a buggy (non-serialized) implementation would run + // the second selection now, while the first is parked on `firstHeld`. + await new Promise((resolve) => setTimeout(resolve, 20)); + releaseFirst?.(); + await Promise.all([first, second]); + + // Strict serialization: the two critical sections never overlapped. + expect(maxConcurrent).toBe(1); + // And they ran in FIFO order, second strictly after the first committed. + expect(selectionOrder).toEqual([0, 1]); + }); + + // End-to-end: two concurrent proxy requests under routingMutex="enabled" against + // a 2-account pool must serialize selection + cursor advance, hand out DISTINCT + // accounts, and both complete without deadlock (the reentrant markSwitchedLocked + // commit on the hot path + the gated commit in persistRuntimeActiveAccount). + it("hands distinct accounts to concurrent enabled-mode requests without deadlock", async () => { + process.env.CODEX_AUTH_ROUTING_MUTEX = "enabled"; + __resetRoutingMutexForTests(); + const now = Date.now(); + const accountManager = new AccountManager(undefined, createStorage(now, 2)); + + // Barrier: hold BOTH upstream fetches open until both requests are in-flight, + // guaranteeing genuine concurrency through the select+commit critical section. + let releaseFetch: (() => void) | undefined; + const fetchGate = new Promise((resolve) => { + releaseFetch = resolve; + }); + let inFlight = 0; + let bothInFlight: (() => void) | undefined; + const bothStarted = new Promise((resolve) => { + bothInFlight = resolve; + }); + const { calls, fetchImpl } = createRecordingFetch(async () => { + inFlight += 1; + if (inFlight >= 2) bothInFlight?.(); + await fetchGate; + return textEventStream("data: forwarded\n\n"); + }); + + const proxy = await startProxy({ accountManager, fetchImpl }); + expect(accountManager.getRoutingMutexMode()).toBe("enabled"); + + const bodyFor = (session: string) => ({ + model: "gpt-5-codex", + stream: true, + input: [{ type: "message", role: "user", content: "hi" }], + metadata: { session_id: session }, + }); + + const reqA = postResponses(proxy, bodyFor("session-a")); + const reqB = postResponses(proxy, bodyFor("session-b")); + + // Wait until both requests have passed selection and reached the upstream + // fetch, then release them. A deadlock here (e.g. non-reentrant re-acquire) + // would hang and fail the test via timeout rather than passing silently. + await bothStarted; + releaseFetch?.(); + const [resA, resB] = await Promise.all([reqA, reqB]); + + expect(resA.status).toBe(HTTP_STATUS.OK); + expect(resB.status).toBe(HTTP_STATUS.OK); + await Promise.all([resA.text(), resB.text()]); + + // Both upstream calls happened, and selection+advance serialized so the two + // concurrent requests landed on DISTINCT accounts (no stampede onto acc_1). + expect(calls).toHaveLength(2); + const servedAuth = calls.map((c) => c.headers.get("authorization")).sort(); + expect(servedAuth).toEqual(["Bearer access-1", "Bearer access-2"]); + }); +}); + describe("buildTokenInvalidationBody", () => { const FALLBACK = "OAuth token has been invalidated. Please re-login."; const parse = (raw: string) => From 311d7cc94f00b0ffc83ce504167e91f63983ae3b Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Wed, 3 Jun 2026 03:22:10 +0800 Subject: [PATCH 7/8] fix(audit): owner-safe config lock + CodeRabbit #505 round 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real bug introduced by the round-3 config lock (#18): - the cross-process config lock fixed expiresAt once and never renewed, and released by unconditionally unlinking the lockfile. A save running longer than CONFIG_LOCK_TTL_MS could be deemed stale and stolen by another process, after which the original holder would delete the NEW owner's lock and reopen concurrent saves. Added a per-acquisition owner token (randomUUID) to the lock payload and a releaseConfigLockIfOwner() that compares-before-unlink, so a holder never deletes a lock it no longer owns. Regressions: stale-foreign-lock takeover (cleans only its own lock) + live-foreign-lock respected (times out, foreign lock untouched, no partial apply). Nits: - mcodex-launcher test: dropped the explicit vitest globals import (#17). - runtime-rotation-proxy: replaced the stale 'KNOWN GAP (L4)' comment on chooseAccount with accurate docs — the race is closed via the reentrant withRoutingMutex on the hot path (#19). Full suite: 4343 passed, 3 skipped, 0 failed; typecheck + lint clean. --- lib/config.ts | 31 +++++++++++++++- lib/runtime-rotation-proxy.ts | 31 ++++++++-------- test/config-save.test.ts | 68 +++++++++++++++++++++++++++++++++++ test/mcodex-launcher.test.ts | 1 - 4 files changed, 113 insertions(+), 18 deletions(-) diff --git a/lib/config.ts b/lib/config.ts index cdb74d042..af49b3f76 100644 --- a/lib/config.ts +++ b/lib/config.ts @@ -1,4 +1,5 @@ import { existsSync, promises as fs, readFileSync } from "node:fs"; +import { randomUUID } from "node:crypto"; import { dirname, join } from "node:path"; import { parseBooleanEnv } from "./env-parsing.js"; import { logWarn } from "./logger.js"; @@ -560,6 +561,7 @@ const CONFIG_LOCK_POLL_MS = 50; interface ConfigLockPayload { pid: number; + owner: string; acquiredAt: number; expiresAt: number; } @@ -587,6 +589,31 @@ async function unlinkConfigLockWithRetry(lockPath: string): Promise { } } +// Owner-safe release: only unlink the lockfile if it still carries OUR owner +// token. If a slow save was deemed stale and a second process stole the lock, +// the lockfile now holds the new owner's token — deleting it would reopen +// concurrent saves, so we leave it alone. +async function releaseConfigLockIfOwner( + lockPath: string, + owner: string, +): Promise { + try { + const content = await fs.readFile(lockPath, "utf-8"); + const parsed = JSON.parse(stripUtf8Bom(content)) as + | Partial + | undefined; + if (parsed?.owner && parsed.owner !== owner) { + // Lock was taken over by another holder; do not delete their lock. + return; + } + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === "ENOENT") return; + // Unreadable/malformed: fall through and attempt our best-effort unlink. + } + await unlinkConfigLockWithRetry(lockPath); +} + async function isConfigLockStale(lockPath: string): Promise { try { const content = await fs.readFile(lockPath, "utf-8"); @@ -619,6 +646,7 @@ async function withConfigFileLock( task: () => Promise, ): Promise { const lockPath = `${targetPath}.lock`; + const owner = randomUUID(); await fs.mkdir(dirname(lockPath), { recursive: true }); const deadline = Date.now() + CONFIG_LOCK_WAIT_TIMEOUT_MS; let acquired = false; @@ -627,6 +655,7 @@ async function withConfigFileLock( const now = Date.now(); const payload: ConfigLockPayload = { pid: process.pid, + owner, acquiredAt: now, expiresAt: now + CONFIG_LOCK_TTL_MS, }; @@ -670,7 +699,7 @@ async function withConfigFileLock( try { return await task(); } finally { - await unlinkConfigLockWithRetry(lockPath); + await releaseConfigLockIfOwner(lockPath, owner); } } diff --git a/lib/runtime-rotation-proxy.ts b/lib/runtime-rotation-proxy.ts index f648ae27c..45bf64a63 100644 --- a/lib/runtime-rotation-proxy.ts +++ b/lib/runtime-rotation-proxy.ts @@ -1126,23 +1126,22 @@ function getQuotaNearExhaustionWaitMs( } /** - * KNOWN GAP (L4, routing mutex): the two `accountManager.markSwitched(...)` - * cursor mutations below (the session-affinity-preferred branch and the - * round-robin fallback) run UNLOCKED even when `routingMutex === "enabled"`. - * Only `persistRuntimeActiveAccount` routes its cursor mutation through - * `markSwitchedLocked` / `withRoutingMutex`, so concurrent requests can still - * race the selection-time cursor update. + * `chooseAccount` is a SYNC selector that internally advances the rotation + * cursor (the session-affinity-preferred branch and the round-robin fallback + * both call `accountManager.markSwitched(...)`, and the hybrid selector advances + * its own cursor). It does NOT acquire the routing mutex itself. * - * Deferred (needs design), not fixed inline, because closing it safely is not - * a minimal change: `chooseAccount` is a SYNC function (returns - * `ManagedAccount | null`) consumed at ~15 exported/test call sites, whereas - * `markSwitchedLocked` is async and the routing mutex (`withRoutingMutex` -> - * `runExclusive`) is a non-reentrant FIFO queue. Awaiting it here would force - * `chooseAccount` async (signature + every caller) and, if any caller already - * holds the mutex, deadlock on the non-reentrant queue. The correct fix is to - * restructure selection so the cursor commit happens in one awaited - * critical section alongside `persistRuntimeActiveAccount`, which is out of - * scope for a minimal hot-path patch. Tracked for the routing-mutex redesign. + * Concurrency (L4): when `routingMutex === "enabled"`, the proxy hot path runs + * this whole call AND the subsequent `markSwitchedLocked` commit inside a single + * `withRoutingMutex` acquisition, so concurrent requests serialize selection + + * cursor advance and cannot stampede the same account. `withRoutingMutex` is + * reentrant (AsyncLocalStorage), so the nested `markSwitchedLocked` — and the + * later one in `persistRuntimeActiveAccount` — run inline without re-acquiring + * the non-reentrant FIFO queue (no deadlock). In legacy mode the inline + * `markSwitched` calls below are used unchanged and no lock is taken, so default + * behavior and perf are identical. See the hot-path caller in + * `startRuntimeRotationProxy` and the regression in + * `test/runtime-rotation-proxy.test.ts`. */ export function chooseAccount(params: { accountManager: AccountManager; diff --git a/test/config-save.test.ts b/test/config-save.test.ts index e0cee5bb2..5f5c45276 100644 --- a/test/config-save.test.ts +++ b/test/config-save.test.ts @@ -292,6 +292,74 @@ describe("plugin config save paths", () => { expect(parsed.preserved).toBe(1); }); + it("takes over a stale foreign lock and removes only its own lock (config-18)", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ preserved: 1 }), "utf8"); + + // Seed a STALE foreign-owned lock (different owner, already expired). Our + // save must take it over, complete, and clean up. + const lockPath = `${configPath}.lock`; + await fs.writeFile( + lockPath, + `${JSON.stringify({ + pid: 999999, + owner: "other-owner-token", + acquiredAt: Date.now() - 60_000, + expiresAt: Date.now() - 30_000, + })}\n`, + "utf8", + ); + + const { savePluginConfig } = await import("../lib/config.js"); + await savePluginConfig({ fastSession: true }); + + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.fastSession).toBe(true); + expect(parsed.preserved).toBe(1); + // Our own lock is released after the save. + await expect(fs.access(lockPath)).rejects.toMatchObject({ code: "ENOENT" }); + }); + + it("does not delete a live foreign lock and times out instead (config-18)", async () => { + const configPath = join(tempDir, "plugin-config.json"); + process.env.CODEX_MULTI_AUTH_CONFIG_PATH = configPath; + await fs.writeFile(configPath, JSON.stringify({ preserved: 1 }), "utf8"); + + // Seed a LIVE foreign-owned lock (different owner, not expired). Our save + // must respect it (wait then time out) and must NOT delete the other + // owner's lockfile. + const lockPath = `${configPath}.lock`; + const foreignPayload = `${JSON.stringify({ + pid: 999999, + owner: "other-owner-token", + acquiredAt: Date.now(), + expiresAt: Date.now() + 60_000, + })}\n`; + await fs.writeFile(lockPath, foreignPayload, "utf8"); + + const { savePluginConfig } = await import("../lib/config.js"); + await expect(savePluginConfig({ fastSession: true })).rejects.toMatchObject({ + code: "ELOCKTIMEOUT", + }); + + // The foreign lock is untouched (same owner token, not stomped). + const lockAfter = JSON.parse(await fs.readFile(lockPath, "utf8")) as { + owner?: string; + }; + expect(lockAfter.owner).toBe("other-owner-token"); + // And our save did not partially apply. + const parsed = JSON.parse(await fs.readFile(configPath, "utf8")) as Record< + string, + unknown + >; + expect(parsed.fastSession).toBeUndefined(); + await fs.rm(lockPath, { force: true }); + }, 15_000); + it("writes through unified settings when env path is unset", async () => { delete process.env.CODEX_MULTI_AUTH_CONFIG_PATH; const unifiedPath = join(tempDir, "settings.json"); diff --git a/test/mcodex-launcher.test.ts b/test/mcodex-launcher.test.ts index 57d6b2e7d..a4eb97bb0 100644 --- a/test/mcodex-launcher.test.ts +++ b/test/mcodex-launcher.test.ts @@ -2,7 +2,6 @@ import { dirname, join } from "node:path"; import { EventEmitter } from "node:events"; import { promises as fs } from "node:fs"; import { fileURLToPath, pathToFileURL } from "node:url"; -import { describe, expect, it, vi } from "vitest"; import { isDirectRunInvocation, parseMcodexArgs, From fea283011216d45467d3b5dfc4e3fe4bd127ad06 Mon Sep 17 00:00:00 2001 From: Neil Daquioag Date: Wed, 3 Jun 2026 05:19:15 +0800 Subject: [PATCH 8/8] fix(audit): CRITICAL legacy-flagged migration deadlock + tone regex anchors Deep re-audit workflow (6 subsystems, adversarial verification) surfaced a CRITICAL I verified by hand against the wiring. CRITICAL: - storage: loadFlaggedAccountsState's legacy-migration save callback was wired to the LOCKING saveFlaggedAccounts at the loadFlaggedAccounts call site, while loadFlaggedAccounts runs inside the already-held global mutex during withAccountAndFlaggedStorageTransaction. The non-reentrant lock would deadlock on a reachable path (legacy blocked-accounts file present + flagged primary absent, e.g. doctor restore). Guarded the callback exactly like its sibling persistRecoveredBackup: saveFlaggedAccountsUnlocked when isStorageLockHeld(), else the locking save. Added a regression that migrates inside a held transaction and fails on hang. MEDIUM: - codex-manager styleAccountDetailText: the success-tone regex /ok|working|succeeded|valid/ was unanchored, so soft-failure details with no failed/error keyword ("token is invalid", "refresh token revoked") matched via substrings (valid in invalid, ok in revoked/token) and rendered green. Anchored to /\b(ok|working|succeeded|valid)\b/ on both the prefix and compact paths; added a regression. Deferred (documented, pre-existing, not introduced by this release; need design, not release-blocking): the runtime-app-helper orphan-reaper cluster in scripts/codex.js (dead-owner reaping, PID-reuse-safe liveness, wrapper-path signal handlers), the config lock lease-renewal, and the unified default-path cross-process save lock (narrow same-mtime-tick window behind a module cycle). Full suite: 4345 passed, 3 skipped, 0 failed; typecheck + lint clean. --- lib/codex-manager.ts | 4 +- lib/storage.ts | 15 ++++- test/codex-manager-detail-tone.test.ts | 18 ++++++ test/storage-flagged.test.ts | 77 ++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 3 deletions(-) diff --git a/lib/codex-manager.ts b/lib/codex-manager.ts index 416e51487..de0df9bb6 100644 --- a/lib/codex-manager.ts +++ b/lib/codex-manager.ts @@ -429,7 +429,7 @@ export function styleAccountDetailText( const detailHasFailure = /failed|error|rate-limited/i.test(compact); const prefixTone: PromptTone = detailHasFailure ? "danger" - : /ok|working|succeeded|valid/i.test(prefix) + : /\b(ok|working|succeeded|valid)\b/i.test(prefix) ? "success" : fallbackTone; const suffixTone: PromptTone = @@ -453,7 +453,7 @@ export function styleAccountDetailText( if (/failed|error/i.test(compact)) return stylePromptText(compact, "danger"); if (/re-login|stale|warning|fallback|unavailable|not available/i.test(compact)) return stylePromptText(compact, "warning"); - if (/ok|working|succeeded|valid/i.test(compact)) + if (/\b(ok|working|succeeded|valid)\b/i.test(compact)) return stylePromptText(compact, "success"); return stylePromptText(compact, fallbackTone); } diff --git a/lib/storage.ts b/lib/storage.ts index 2ed97fde6..994539f2c 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -2112,7 +2112,20 @@ export async function loadFlaggedAccounts(): Promise { }; return isStorageLockHeld() ? recover() : withStorageLock(recover); }, - saveFlaggedAccounts, + saveFlaggedAccounts: async (storage) => { + // Mirror persistRecoveredBackup above: loadFlaggedAccountsState's legacy + // migration persists via this callback, and loadFlaggedAccounts can run + // inside an already-held storage lock + // (withAccountAndFlaggedStorageTransaction / withFlaggedStorageTransaction). + // The global mutex is non-reentrant, so re-acquiring it via the locking + // saveFlaggedAccounts would deadlock — use the unlocked save when the lock + // is already held, otherwise acquire it as usual. + if (isStorageLockHeld()) { + await saveFlaggedAccountsUnlocked(storage); + return; + } + await saveFlaggedAccounts(storage); + }, loadFlaggedAccountsState, logError: (message, details) => { log.error(message, details); diff --git a/test/codex-manager-detail-tone.test.ts b/test/codex-manager-detail-tone.test.ts index a052d25a9..d618a56e8 100644 --- a/test/codex-manager-detail-tone.test.ts +++ b/test/codex-manager-detail-tone.test.ts @@ -102,4 +102,22 @@ describe("styleAccountDetailText tone precedence", () => { expect(styled).toContain(ANSI.red); expect(styled).not.toContain(ANSI.green); }); + + it("does not render a soft-failure detail green via an unanchored success keyword", () => { + // The success regex is /\b(ok|working|succeeded|valid)\b/ — without word + // boundaries, "invalid"/"revoked"/"token" would match (valid in invalid, + // ok in revoked/token) and color a failure detail green. These details have + // no "failed"/"error" keyword, so the danger pre-check does not catch them. + for (const detail of [ + "token is invalid or expired", + "refresh token revoked", + ]) { + const styled = styleAccountDetailText(detail); + expect(styled).not.toContain(ANSI.green); + } + // A genuine success keyword on a word boundary still renders green. + expect(styleAccountDetailText("signed in and working")).toContain( + ANSI.green, + ); + }); }); diff --git a/test/storage-flagged.test.ts b/test/storage-flagged.test.ts index 592274ea5..402aec1b7 100644 --- a/test/storage-flagged.test.ts +++ b/test/storage-flagged.test.ts @@ -1147,4 +1147,81 @@ describe("flagged storage extracted helpers", () => { expect(persisted.accounts).toHaveLength(1); expect(persisted.accounts[0]?.refreshToken).toBe("recovered-token"); }); + + it("migrates a legacy flagged file inside a held storage lock without deadlocking", async () => { + // CRITICAL regression: loadFlaggedAccountsState's legacy-migration path + // persists via the saveFlaggedAccounts callback. When loadFlaggedAccounts + // runs inside withAccountAndFlaggedStorageTransaction (global lock held), the + // locking saveFlaggedAccounts would re-acquire the non-reentrant mutex and + // deadlock. Trigger it: legacy blocked-accounts file present, flagged primary + // absent, loaded inside a held transaction. + const { cloneAccountStorageForPersistence } = await import( + "../lib/storage/account-persistence.js" + ); + const flaggedPath = getFlaggedAccountsPath(); + const legacyPath = join( + dirname(getStoragePath()), + "openai-codex-blocked-accounts.json", + ); + await fs.mkdir(dirname(flaggedPath), { recursive: true }); + await removeWithRetry(flaggedPath, { force: true }); + await removeWithRetry(`${flaggedPath}.bak`, { force: true }); + await fs.writeFile( + legacyPath, + JSON.stringify({ + version: 1, + accounts: [ + { + refreshToken: "legacy-locked-token", + accountId: "acct-legacy", + flaggedAt: 5, + addedAt: 5, + lastUsed: 5, + }, + ], + }), + "utf8", + ); + + const run = withAccountAndFlaggedStorageTransaction( + async (_current, _persist, currentFlagged) => currentFlagged, + { + getStoragePath, + loadCurrent: async () => null, + loadCurrentFlagged: loadFlaggedAccounts, + saveAccounts: async () => undefined, + saveFlaggedAccounts, + cloneAccountStorageForPersistence, + logRollbackError: () => undefined, + }, + ); + + let timeoutHandle: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutHandle = setTimeout( + () => + reject( + new Error("legacy flagged migration deadlocked inside held lock"), + ), + 3000, + ); + }); + + let migratedFlagged: Awaited; + try { + migratedFlagged = (await Promise.race([run, timeout])) as Awaited< + typeof run + >; + } finally { + if (timeoutHandle !== undefined) clearTimeout(timeoutHandle); + } + + expect(migratedFlagged.accounts).toHaveLength(1); + expect(migratedFlagged.accounts[0]?.refreshToken).toBe( + "legacy-locked-token", + ); + // Migration must persist the new flagged file and remove the legacy one. + expect(existsSync(flaggedPath)).toBe(true); + expect(existsSync(legacyPath)).toBe(false); + }); });