fix(codex): warn about stale Codex app-servers after a catalog write - #576
Conversation
📝 WalkthroughWalkthroughAdds cross-platform detection and targeted restart handling for stale Codex ChangesCodex app-server refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 935a0e977b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // to explain it. Name the failure and the one command that repairs it. | ||
| console.error(`⚠️ ${grokSyncFailureMessage(err)}`); | ||
| } | ||
| } catch { /* best-effort — grok integration must never block startup */ } |
There was a problem hiding this comment.
When syncGrokConfig throws during startup—for example, because ~/.grok/config.toml is unreadable or cannot be rewritten—this catch now silently leaves the existing fence pointing at its previous proxy port. If that listener is gone, every subsequent Grok turn fails with a refused connection while ocx start reports no cause or recovery action. The same regression appears in both handleEnsure catches; preserve the best-effort behavior, but restore the error message and ocx ensure/dashboard repair hint in all three paths.
Useful? React with 👍 / 👎.
| const commandLine = readFileSync(`/proc/${pid}/cmdline`) | ||
| .toString("utf8") | ||
| .replace(/\0/g, " ") | ||
| .trim(); |
There was a problem hiding this comment.
On Linux, /proc/<pid>/cmdline is NUL-delimited argv and does not preserve the shell quoting that originally grouped arguments. Replacing every NUL with a space and then reparsing quotes therefore misses valid processes whenever an argument before app-server contains whitespace, such as codex -C "/home/Jane Doe/project" app-server, because the scanner treats Doe/project as the subcommand. Preserve the NUL-separated tokens through matching (or encode them without losing boundaries), otherwise both the warning and --restart-codex silently fail for these app-servers.
Useful? React with 👍 / 👎.
| * `x86_64-pc-windows-msvc`). Requires arch-vendor-os with an optional env | ||
| * segment — not a broad `codex-*` wildcard. | ||
| */ | ||
| const CODEX_TARGET_TRIPLE_BODY = "[a-z0-9_]+-[a-z0-9_]+-[a-z0-9_]+(?:-[a-z0-9_]+)?"; |
There was a problem hiding this comment.
The purported target-triple pattern accepts any three or four hyphen-separated lowercase components, so unrelated executable names such as codex-bridge-helper-linux or codex-not-really-codex-server pass isCodexExecutableToken. If such a same-user process is running with an app-server subcommand, ocx sync --restart-codex sends it SIGTERM despite the documented narrow-matching safety contract. Validate known architecture/vendor/OS shapes rather than treating arbitrary codex-*-*-* basenames as official binaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/index.ts`:
- Around line 299-302: Restore error logging in all three catch blocks
surrounding syncGrokConfig, including the sites near the existing changed/!r.ok
handling. Capture the thrown exception and log a detailed Grok sync failure
message using the existing logging convention or helper, while preserving
best-effort startup behavior and the current non-throwing r.message logging.
In `@src/codex/app-server-processes.ts`:
- Around line 412-419: Update restartCodexAppServers so default process
discovery occurs inside the function body after the injected io defaults are
initialized, allowing listCodexAppServerProcesses to receive the resolved io.
Rename the function’s processes references to targets, while preserving explicit
process arguments and existing restart behavior.
In `@tests/codex-app-server-processes.test.ts`:
- Around line 250-332: The tests rely on fragile raw-source slicing around CLI
case labels; replace these assertions with direct unit tests of an exported
helper that encapsulates the shared catalog/cache-write gating and app-server
handling used by the sync and sync-cache commands. Update the CLI cases to call
that helper, and retain direct assertions for restart forwarding and the
config-route behavior without parsing source text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bcf415a6-58c8-4ee0-9004-293f464a32b9
📒 Files selected for processing (15)
docs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/reference/cli.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/dashboard-overview-sections.tsxsrc/cli/help.tssrc/cli/index.tssrc/codex/app-server-processes.tssrc/server/management/config-routes.tstests/codex-app-server-processes.test.tstests/codex-models-cache-invalidate.test.ts
| const r = await syncGrokConfig(port, config, config.hostname ? { hostname: config.hostname } : {}); | ||
| if (r.changed) console.log(" + Grok Build config updated (~/.grok/config.toml)"); | ||
| else if (!r.ok) console.error(`⚠️ ${r.message}`); | ||
| } catch (err) { | ||
| // Best-effort: grok integration must never block startup. But swallowing the error | ||
| // silently is how a stale fence survives unnoticed — ~/.grok/config.toml keeps | ||
| // pointing at whatever port the LAST successful sync wrote, and if that listener is | ||
| // gone every grok turn retries against a refused connection with nothing in our log | ||
| // to explain it. Name the failure and the one command that repairs it. | ||
| console.error(`⚠️ ${grokSyncFailureMessage(err)}`); | ||
| } | ||
| } catch { /* best-effort — grok integration must never block startup */ } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Grok Build sync exceptions are now fully silent — lost diagnosability.
All three catch { /* best-effort */ } blocks (Lines 299-302, 327-330, 354-357) used to log a detailed failure message via the now-removed grokSyncFailureMessage(err) helper when syncGrokConfig threw. Now an unexpected exception (e.g. a malformed ~/.grok/config.toml, permission error) produces zero output on every ocx start/ocx ensure, leaving users with no clue why Grok routing isn't updating. Note the sibling !r.ok branch (non-throwing failure) still logs r.message — only the thrown-exception path regressed.
🛠️ Proposed fix (repeat for all three sites)
try {
const { syncGrokConfig } = await import("../grok/sync");
const r = await syncGrokConfig(port, config, config.hostname ? { hostname: config.hostname } : {});
if (r.changed) console.log(" + Grok Build config updated (~/.grok/config.toml)");
else if (!r.ok) console.error(`⚠️ ${r.message}`);
- } catch { /* best-effort — grok integration must never block startup */ }
+ } catch (err) {
+ console.error(`⚠️ Grok Build config sync failed: ${err instanceof Error ? err.message : String(err)}`);
+ }Also applies to: 327-330, 354-357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 299 - 302, Restore error logging in all three
catch blocks surrounding syncGrokConfig, including the sites near the existing
changed/!r.ok handling. Capture the thrown exception and log a detailed Grok
sync failure message using the existing logging convention or helper, while
preserving best-effort startup behavior and the current non-throwing r.message
logging.
| export function restartCodexAppServers( | ||
| processes: readonly CodexAppServerProcess[] = listCodexAppServerProcesses(), | ||
| io: CodexAppServerProcessIo = {}, | ||
| ): RestartCodexAppServersResult { | ||
| const isAlive = io.isAlive ?? isProcessAlive; | ||
| const kill = io.kill ?? ((pid, signal) => { process.kill(pid, signal); }); | ||
| const wait = io.waitExit ?? waitForExit; | ||
| const now = io.now ?? Date.now; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Default processes param silently ignores an injected io.
processes: readonly CodexAppServerProcess[] = listCodexAppServerProcesses() at Line 413 is evaluated with the function's own default io = {} — JS default-parameter expressions can't see later parameters, so listCodexAppServerProcesses() here never sees the caller-supplied io at Line 414. Every current call site (afterCatalogWriteHandleAppServers, and the tests) passes processes explicitly, so this is currently dormant, but it's a footgun for any future/test caller that does restartCodexAppServers(undefined, mockIo) expecting the default resolution to respect the injected io — it will silently hit the real OS process list instead.
🛠️ Proposed fix
export function restartCodexAppServers(
- processes: readonly CodexAppServerProcess[] = listCodexAppServerProcesses(),
+ processes?: readonly CodexAppServerProcess[],
io: CodexAppServerProcessIo = {},
): RestartCodexAppServersResult {
+ const targets = processes ?? listCodexAppServerProcesses(io);
const isAlive = io.isAlive ?? isProcessAlive;
const kill = io.kill ?? ((pid, signal) => { process.kill(pid, signal); });
const wait = io.waitExit ?? waitForExit;
const now = io.now ?? Date.now;
- const requested = processes.map(process => process.pid);
+ const requested = targets.map(process => process.pid);
...
- for (const proc of processes) {
+ for (const proc of targets) {(and rename remaining processes references in the function body to targets)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function restartCodexAppServers( | |
| processes: readonly CodexAppServerProcess[] = listCodexAppServerProcesses(), | |
| io: CodexAppServerProcessIo = {}, | |
| ): RestartCodexAppServersResult { | |
| const isAlive = io.isAlive ?? isProcessAlive; | |
| const kill = io.kill ?? ((pid, signal) => { process.kill(pid, signal); }); | |
| const wait = io.waitExit ?? waitForExit; | |
| const now = io.now ?? Date.now; | |
| export function restartCodexAppServers( | |
| processes?: readonly CodexAppServerProcess[], | |
| io: CodexAppServerProcessIo = {}, | |
| ): RestartCodexAppServersResult { | |
| const targets = processes ?? listCodexAppServerProcesses(io); | |
| const isAlive = io.isAlive ?? isProcessAlive; | |
| const kill = io.kill ?? ((pid, signal) => { process.kill(pid, signal); }); | |
| const wait = io.waitExit ?? waitForExit; | |
| const now = io.now ?? Date.now; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/app-server-processes.ts` around lines 412 - 419, Update
restartCodexAppServers so default process discovery occurs inside the function
body after the injected io defaults are initialized, allowing
listCodexAppServerProcesses to receive the resolved io. Rename the function’s
processes references to targets, while preserving explicit process arguments and
existing restart behavior.
A long-lived Codex app-server keeps serving its in-memory model list, so ocx sync could write a correct catalog and Codex would still show the old models (#476). Restarting the proxy or re-running sync did not help; only killing the app-server did, which is why rebooting appeared to fix it. Detect those processes and say so, gated on the catalogWritten/cacheSynced signal from the first half of this change so a no-op sync stays quiet. ocx sync --restart-codex and ocx sync-cache --restart-codex opt into a SIGTERM, never SIGKILL. Process matching is the risky part and is deliberately narrow: UID-scoped on Unix, exact cmdline match rather than a broad *codex* sweep, and it understands quoted paths and value-taking globals so it cannot mistake a neighbouring process for an app-server.
935a0e9 to
98142f5
Compare
Rebased onto current
|
Rebased onto current
|
…rfaced The three conflicts were the predicted logic files. What the audit caught was separate: the pre-rebase commit already deleted dev's grokSyncFailureMessage and its three handlers, and no test in the repo asserts that string, so a full green suite would not have stopped it.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/pages/dashboard-overview-sections.tsx (1)
90-95: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
DashboardInjectionPaneldroppedapiBasefrom the destructuring but kept it in the props type.Line 90 reads
({ d }: { apiBase: string; d: Dash })—apiBaseis now unused inside the component yet still required by the type, so every call site must keep threading a value that goes nowhere. Drop it from the type so the compiler flags the stale argument at the call sites instead of silently accepting it.♻️ Proposed cleanup
-export function DashboardInjectionPanel({ d }: { apiBase: string; d: Dash }) { +export function DashboardInjectionPanel({ d }: { d: Dash }) {Note this is a signature change; the caller (dashboard page) needs the now-extra
apiBaseprop removed too.#!/bin/bash # Locate call sites still passing apiBase to DashboardInjectionPanel. rg -nP -C3 '<DashboardInjectionPanel' gui/src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/dashboard-overview-sections.tsx` around lines 90 - 95, Update the DashboardInjectionPanel props type to remove the unused apiBase property, leaving only d: Dash; then update every DashboardInjectionPanel call site, especially the dashboard page, to stop passing apiBase.gui/src/i18n/ru.ts (1)
1111-1113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe the sticky counter as new-session bindings.
Line 1111 says “successful requests,” but the adjacent help text at Line 1113 and the English source define this value as successful new-session bindings. The current label can mislead users into thinking every request increments the counter.
Proposed wording
- "accountPool.stickyLimit": "Успешных запросов до ротации", + "accountPool.stickyLimit": "Успешных привязок новых сессий до ротации",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/i18n/ru.ts` around lines 1111 - 1113, Update the Russian translation for accountPool.stickyLimit and accountPool.stickyLimitAria to describe successful new-session bindings, matching accountPool.stickyLimitHelp and the English source, rather than successful requests.
♻️ Duplicate comments (1)
tests/codex-app-server-processes.test.ts (1)
257-295: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnguarded
indexOfsentinels can make these source-slicing tests pass silently on the wrong region.Separate from the brittleness already raised on this block in an earlier review: none of the slice boundaries are checked for
-1. The end sentinels are the dangerous ones, becauseString.prototype.slicetreats a negative index as an offset from the end rather than as "not found":
- Line 258: if
case "v2":is ever renamed or removed,cliSource.indexOf('case "v2":')is-1, soslice(start, -1)returns everything fromcase "sync":to one character before EOF — the whole rest of the CLI. All fourtoContainchecks (Lines 259-262) and both ordering checks (Lines 263-264) would then still pass, while verifying nothing about thesynccase in particular.- Line 272-275: same hazard with the
case "gui":sentinel.- Line 287-290: same hazard with the
url.pathname === "/api/update/check"sentinel — and here it is worse, since Lines 292-293 are negative assertions (not.toContain) that get strictly harder to satisfy over an over-wide slice, but Line 291's positive assertion would pass against any region containing the call.Net effect: a green test that no longer tests the write-gate. Asserting the sentinel positions up front costs three lines and converts every one of these into a loud failure.
🛡️ Assert slice boundaries before slicing
+ function sliceBetween(source: string, startMarker: string, endMarker: string): string { + const start = source.indexOf(startMarker); + const end = source.indexOf(endMarker); + expect(start, `missing marker: ${startMarker}`).toBeGreaterThanOrEqual(0); + expect(end, `missing marker: ${endMarker}`).toBeGreaterThan(start); + return source.slice(start, end); + } + test("ocx sync only handles app-servers after a catalog/cache write and forwards --restart-codex", () => { - const syncCase = cliSource.slice(cliSource.indexOf('case "sync":'), cliSource.indexOf('case "v2":')); + const syncCase = sliceBetween(cliSource, 'case "sync":', 'case "v2":');Apply the same helper at Lines 272-275 and Lines 287-290.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/codex-app-server-processes.test.ts` around lines 257 - 295, Validate every source-slicing sentinel before calling slice in the tests for the sync, sync-cache, and POST /api/sync regions. Add a shared assertion/helper that requires each start and end index (including the case "v2", case "gui", and /api/update/check markers) to be found, then slice only after validation so missing sentinels fail loudly instead of selecting an incorrect region.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/cli.md`:
- Around line 150-154: Update the `ocx sync` documentation sentence to state
that the warning is emitted only when the sync writes the catalog or model cache
and matching long-lived Codex processes are still running. Clarify that no
warning is expected when the sync rewrites neither file, even if app-server
processes exist; keep the `--restart-codex` behavior unchanged.
- Around line 145-159: Update the translated CLI reference sections for ocx sync
and ocx sync-cache in the ja, ko, ru, and zh-cn pages to match the English
source: document the optional --restart-codex flag, explain the stale long-lived
app-server warning, and note that the flag targets only matching current-user
Codex processes and may interrupt active turns.
In `@src/codex/app-server-processes.ts`:
- Around line 263-301: Update the user-scoped branch in listDarwinSnapshots to
invoke Darwin ps with -xU and the existing uid/output arguments, replacing the
current -u invocation. Preserve the unscoped branch, parsing logic, and
ProcessSnapshot filtering unchanged so background processes without a
controlling TTY are included.
In `@tests/codex-app-server-processes.test.ts`:
- Around line 126-161: Update the restartCodexAppServers test to record kill and
waitExit calls in one shared ordered event log, then assert that both SIGTERM
events occur before either waitExit event. Keep the existing signals, waits, and
result assertions to preserve verification of signal values, shared deadlines,
and outcomes.
- Around line 111-124: Add a focused Unix regression test in the codex
app-server process tests that exercises the default snapshot path and verifies
enumeration is scoped to the current UID, covering the getuid flow into
listUnixProcSnapshots/listDarwinSnapshots. If direct process enumeration is not
controllable in tests, introduce a narrow injectable seam and assert that other
users’ processes are excluded while matching same-user processes remain
detected.
- Around line 366-375: Replace the Atomics.wait delay in the test block with
Bun.sleepSync(250), preserving the existing 250 ms settle period before
listWindowsSnapshots() runs.
In `@tests/codex-models-cache-invalidate.test.ts`:
- Around line 57-112: Extract the inline sync write-gate into an exported
handleAppServersAfterSync helper, call it from both CLI sync cases, and have it
enumerate/restart app-servers only when catalogWritten or cacheSynced is true.
In tests/codex-models-cache-invalidate.test.ts:57-112, replace both copied gates
with direct helper calls using both flags false; at 135-149 and 179-197, pass
each syncResult directly while preserving existing assertions. In
tests/codex-app-server-processes.test.ts:257-295, replace source-slicing tests
with behavioral helper tests covering all flag combinations and restart
forwarding; leave tests/codex-app-server-processes.test.ts:297-331 unchanged.
- Around line 156-177: Remove the unused config.toml and malformed catalog file
writes from the test, and rename the test to describe propagation of
catalogExists, catalogWritten, and cacheSynced from refreshCodexModelCatalog
through syncModelsToCodex. Keep the existing stub and result assertions
unchanged.
---
Outside diff comments:
In `@gui/src/i18n/ru.ts`:
- Around line 1111-1113: Update the Russian translation for
accountPool.stickyLimit and accountPool.stickyLimitAria to describe successful
new-session bindings, matching accountPool.stickyLimitHelp and the English
source, rather than successful requests.
In `@gui/src/pages/dashboard-overview-sections.tsx`:
- Around line 90-95: Update the DashboardInjectionPanel props type to remove the
unused apiBase property, leaving only d: Dash; then update every
DashboardInjectionPanel call site, especially the dashboard page, to stop
passing apiBase.
---
Duplicate comments:
In `@tests/codex-app-server-processes.test.ts`:
- Around line 257-295: Validate every source-slicing sentinel before calling
slice in the tests for the sync, sync-cache, and POST /api/sync regions. Add a
shared assertion/helper that requires each start and end index (including the
case "v2", case "gui", and /api/update/check markers) to be found, then slice
only after validation so missing sentinels fail loudly instead of selecting an
incorrect region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fa77d67-c1d8-496c-8755-ea4ca99c282e
📒 Files selected for processing (15)
docs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/reference/cli.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/dashboard-overview-sections.tsxsrc/cli/help.tssrc/cli/index.tssrc/codex/app-server-processes.tssrc/server/management/config-routes.tstests/codex-app-server-processes.test.tstests/codex-models-cache-invalidate.test.ts
| ### `ocx sync [--restart-codex]` | ||
|
|
||
| Fetch the live model list from every configured provider and re-inject the merged catalog into Codex. | ||
| Run it after adding a provider or to refresh available models. | ||
|
|
||
| ### `ocx sync-cache` | ||
| If long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep | ||
| serving the previous in-memory model list even though `opencodex-catalog.json` / `models_cache.json` | ||
| were updated. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and | ||
| `codex-code-mode-host` processes owned by the current user (active turns may be interrupted). Broad | ||
| `pkill -f codex` matching is intentionally avoided. | ||
|
|
||
| Invalidate Codex's local model picker cache so it is rebuilt from the active opencodex catalog. | ||
| ### `ocx sync-cache [--restart-codex]` | ||
|
|
||
| Invalidate Codex's local model picker cache so it is rebuilt from the active opencodex catalog. The | ||
| same stale-`app-server` warning and optional `--restart-codex` behavior as `ocx sync` apply. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Do localized copies of the CLI reference / codex-integration guide exist?
fd -t f 'cli.md|codex-integration.md' docs-site
echo '--- localized pages that still lack --restart-codex ---'
fd -t f 'cli.md|codex-integration.md' docs-site --exec sh -c 'rg -q -- "--restart-codex" "$1" || echo "MISSING: $1"' _ {}Repository: lidge-jun/opencodex
Length of output: 1236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/reference/cli.md \
docs-site/src/content/docs/ja/reference/cli.md \
docs-site/src/content/docs/ko/reference/cli.md \
docs-site/src/content/docs/ru/reference/cli.md \
docs-site/src/content/docs/zh-cn/reference/cli.md
do
echo "===== $f ====="
wc -l "$f"
rg -n "ocx sync|ocx sync-cache|restart-codex|stale|app-server|pkill|models_cache|opencodex-catalog" "$f" || true
echo
doneRepository: lidge-jun/opencodex
Length of output: 1987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/ja/reference/cli.md \
docs-site/src/content/docs/ko/reference/cli.md \
docs-site/src/content/docs/ru/reference/cli.md \
docs-site/src/content/docs/zh-cn/reference/cli.md
do
echo "===== $f ====="
sed -n '128,145p' "$f"
echo
doneRepository: lidge-jun/opencodex
Length of output: 2616
Sync the translated CLI reference pages with the English source
docs-site/src/content/docs/ja/reference/cli.md:134-139, docs-site/src/content/docs/ko/reference/cli.md:138-143, docs-site/src/content/docs/ru/reference/cli.md:147-153, and docs-site/src/content/docs/zh-cn/reference/cli.md:134-139 still describe bare ocx sync / ocx sync-cache flows. Add --restart-codex and the stale app-server warning so these locale pages don’t contradict docs-site/src/content/docs/reference/cli.md:145-159.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/cli.md` around lines 145 - 159, Update
the translated CLI reference sections for ocx sync and ocx sync-cache in the ja,
ko, ru, and zh-cn pages to match the English source: document the optional
--restart-codex flag, explain the stale long-lived app-server warning, and note
that the flag targets only matching current-user Codex processes and may
interrupt active turns.
Source: Path instructions
| If long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep | ||
| serving the previous in-memory model list even though `opencodex-catalog.json` / `models_cache.json` | ||
| were updated. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and | ||
| `codex-code-mode-host` processes owned by the current user (active turns may be interrupted). Broad | ||
| `pkill -f codex` matching is intentionally avoided. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The warning is gated on an actual write, but the docs read as unconditional.
Lines 150-151 say ocx sync warns "if long-lived Codex app-server processes are still running". Per src/cli/index.ts Lines 821-824, detection only runs inside if (synced.catalogWritten || synced.cacheSynced) — a sync that rewrites nothing (no catalog source, external provider manager) stays silent even with app-servers alive. Tighten the sentence so users don't read the absence of a warning as "no app-server running".
📝 Suggested wording
-If long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep
-serving the previous in-memory model list even though `opencodex-catalog.json` / `models_cache.json`
-were updated. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and
+When a sync actually rewrites `opencodex-catalog.json` / `models_cache.json` and long-lived Codex
+`app-server` processes are still running, `ocx sync` warns that they may keep serving the previous
+in-memory model list. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and
`codex-code-mode-host` processes owned by the current user (active turns may be interrupted). Broad
`pkill -f codex` matching is intentionally avoided.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep | |
| serving the previous in-memory model list even though `opencodex-catalog.json` / `models_cache.json` | |
| were updated. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and | |
| `codex-code-mode-host` processes owned by the current user (active turns may be interrupted). Broad | |
| `pkill -f codex` matching is intentionally avoided. | |
| When a sync actually rewrites `opencodex-catalog.json` / `models_cache.json` and long-lived Codex `app-server` processes are still running, `ocx sync` warns that they may keep serving the previous in-memory model list. Pass `--restart-codex` to send `SIGTERM` only to matching `codex … app-server` and | |
| `codex-code-mode-host` processes owned by the current user (active turns may be interrupted). Broad | |
| `pkill -f codex` matching is intentionally avoided. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/cli.md` around lines 150 - 154, Update
the `ocx sync` documentation sentence to state that the warning is emitted only
when the sync writes the catalog or model cache and matching long-lived Codex
processes are still running. Clarify that no warning is expected when the sync
rewrites neither file, even if app-server processes exist; keep the
`--restart-codex` behavior unchanged.
Source: Path instructions
| function listDarwinSnapshots(uid: number | undefined): ProcessSnapshot[] { | ||
| const out: ProcessSnapshot[] = []; | ||
| try { | ||
| const output = uid !== undefined | ||
| ? execFileSync("ps", ["-u", String(uid), "-o", "pid=,command="], { | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| timeout: 8_000, | ||
| }) | ||
| : execFileSync("ps", ["-axo", "pid=,uid=,command="], { | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| timeout: 8_000, | ||
| }); | ||
| for (const raw of output.split(/\r?\n/)) { | ||
| const line = raw.trim(); | ||
| if (!line) continue; | ||
| if (uid !== undefined) { | ||
| const match = /^(\d+)\s+(.*)$/.exec(line); | ||
| if (!match) continue; | ||
| const pid = Number(match[1]); | ||
| const commandLine = match[2]?.trim() ?? ""; | ||
| if (!Number.isSafeInteger(pid) || pid <= 1 || !commandLine) continue; | ||
| out.push({ pid, commandLine, uid }); | ||
| continue; | ||
| } | ||
| const match = /^(\d+)\s+(\d+)\s+(.*)$/.exec(line); | ||
| if (!match) continue; | ||
| const pid = Number(match[1]); | ||
| const processUid = Number(match[2]); | ||
| const commandLine = match[3]?.trim() ?? ""; | ||
| if (!Number.isSafeInteger(pid) || pid <= 1 || !commandLine) continue; | ||
| out.push({ pid, commandLine, uid: Number.isSafeInteger(processUid) ? processUid : undefined }); | ||
| } | ||
| } catch { | ||
| return out; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no other call site already relies on the `-u <uid>` form,
# and check whether tests pin the darwin argv shape.
rg -nP --type=ts -C3 'execFileSync\(\s*"ps"'
rg -nP -C3 '"-axo"|"-u", String\(uid\)|listDarwinSnapshots' tests srcRepository: lidge-jun/opencodex
Length of output: 157
🌐 Web query:
macOS ps man page -U vs -u flag select processes by user, does -x include processes without controlling terminal
💡 Result:
On macOS, the ps command's -u flag is used to select processes by user [1][2]. According to POSIX standards and common implementations, when the -u flag is used, ps lists processes associated with the specified user [3][2]. Crucially, the -u flag selects processes based on user ID and does not inherently exclude or include processes based on their controlling terminal (TTY) status [3][2]. However, in standard behavior, ps defaults to showing only processes with the same effective user ID as the invoker and the same controlling terminal [4][5]. When you explicitly use the -u flag, you override this default selection behavior [3][6]. While -u will include processes that do not have a controlling terminal (often represented by a '?' or '??' in the TTY column on macOS) [7][8], it does not serve as a filter to exclusively show only non-terminal processes. If you need to view processes specifically for a user while including those without a controlling terminal, the -u flag will generally accomplish this because it widens the selection criteria beyond the default "current terminal" restriction [6][2]. To list all processes for a user, including those without a controlling terminal, you can typically use: ps -u If you want to see all processes on the system regardless of user or terminal, common BSD-style syntax on macOS is: ps aux In this case, the 'x' flag is the specific option that explicitly includes processes without a controlling terminal [9][10]. Note that ps options can be complex due to the mix of POSIX/UNIX-style and BSD-style flags available on macOS [10].
Citations:
- 1: https://man.archlinux.org/man/ps.1p.en
- 2: https://manpages.ubuntu.com/manpages/stonking/man1/ps.1posix.html
- 3: https://pubs.opengroup.org/onlinepubs/9699919799.orig/utilities/ps.html
- 4: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/utilities/ps.html
- 5: https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/utilities/ps.html
- 6: https://unix.stackexchange.com/questions/596782/ps-filter-by-user-and-processes-without-terminals
- 7: https://apple.stackexchange.com/questions/460394/what-does-a-question-mark-in-the-stat-column-of-ps-on-macos-indicate
- 8: https://unix.stackexchange.com/questions/242197/list-all-processes-without-controlling-terminal-only
- 9: https://www.geeksforgeeks.org/linux-unix/ps-command-in-linux-with-examples/
- 10: https://stackoverflow.com/questions/41325922/get-ps-to-show-process-not-attached-to-terminal-with-flags
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and surrounding context.
sed -n '220,330p' src/codex/app-server-processes.ts
# Find call sites and tests for this helper.
rg -n "listDarwinSnapshots|ProcessSnapshot|app-server-processes" src test testsRepository: lidge-jun/opencodex
Length of output: 6302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the app-server-processes tests for Darwin-specific assertions.
sed -n '1,260p' tests/codex-app-server-processes.test.ts
printf '\n---\n'
sed -n '260,420p' tests/codex-app-server-processes.test.tsRepository: lidge-jun/opencodex
Length of output: 17952
🌐 Web query:
Apple macOS ps man page -u option selects processes by user and -x includes processes without controlling terminal
💡 Result:
On macOS, the ps command uses the -x option to include processes without a controlling terminal [1][2]. When you use options to select processes (such as -u to filter by user), ps typically filters out processes that do not have a controlling terminal [1][2]. To include these processes in your output, you must explicitly add the -x option [1][2]. For example, to list all processes owned by a specific user including those without a controlling terminal, you would use: ps -u -x [1][2] Conversely, the -X option can be used to explicitly exclude processes without a controlling terminal if they were otherwise matched by other options [1][2]. If both -x and -X are specified, the one provided last on the command line takes precedence [1][2].
Citations:
🌐 Web query:
macOS ps man page "-u" "-U" controlling terminal "-x" BSD
💡 Result:
In the macOS ps command, the options -u, -U, and -x serve the following purposes: -u: Displays processes belonging to the specified username(s) [1][2]. -U: Displays processes belonging to the specified real user ID(s) (UIDs) [1][3]. -x: Includes processes that do not have a controlling terminal [2][3]. By default, the ps utility typically filters for processes associated with the current user that have a controlling terminal [4][5][6]. The -x option is often used to broaden this search to include "background" processes that lack a terminal connection [2][7]. Conversely, the -X option (if available and not overridden by -x) explicitly excludes processes without a controlling terminal [2][3]. If both -X and -x are specified, the option that appears last in the command line takes precedence [2][3]. Note that the -u option in modern macOS ps behavior interprets the argument as a username, distinguishing it from older legacy behavior where -u was used to select specific output formats [1][2].
Citations:
- 1: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man1/ps.1.html
- 2: https://www.unix.com/man_page/osx/1/ps/
- 3: https://ss64.com/mac/ps.html
- 4: https://man.freebsd.org/cgi/man.cgi?format=html&query=ps&sektion=1
- 5: https://man.openbsd.org/OpenBSD-current/man1/ps.1
- 6: https://man.openbsd.org/OpenBSD-7.4/ps.1
- 7: https://manned.org/man/freebsd-14.0/ps.1
Include -x in the Darwin user-scoped ps call
In src/codex/app-server-processes.ts:263-301, ps -u <uid> -o pid=,command= can miss app-server processes that have no controlling tty on macOS. Switch that branch to ps -xU <uid> -o pid=,command= (or use the unscoped -axo ... form and filter in JS) so Desktop-launched background processes are included.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 281-281: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 289-289: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/app-server-processes.ts` around lines 263 - 301, Update the
user-scoped branch in listDarwinSnapshots to invoke Darwin ps with -xU and the
existing uid/output arguments, replacing the current -u invocation. Preserve the
unscoped branch, parsing logic, and ProcessSnapshot filtering unchanged so
background processes without a controlling TTY are included.
| test("listCodexAppServerProcesses filters injected snapshots", () => { | ||
| const matched = listCodexAppServerProcesses({ | ||
| listSnapshots: () => [ | ||
| { pid: 11, commandLine: "hermes-codex-bridge-mcp" }, | ||
| { pid: 22, commandLine: "codex app-server --listen unix://x" }, | ||
| { pid: 22, commandLine: "codex app-server --listen unix://x" }, | ||
| { pid: 33, commandLine: "codex-code-mode-host" }, | ||
| { pid: 44, commandLine: "codex exec hi" }, | ||
| { pid: 55, commandLine: "codex exec \"debug app-server behavior\"" }, | ||
| { pid: 66, commandLine: "node worker.js codex-code-mode-host" }, | ||
| ], | ||
| }); | ||
| expect(matched.map(process => process.pid)).toEqual([22, 33]); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tests touching the UID-scoped snapshot enumeration path.
fd -e ts . tests --exec rg -nP -C3 '\b(defaultListSnapshots|getuid|listSnapshots\s*:)' {}
# Inspect the enumeration implementation to see what is UID-gated.
ast-grep outline src/codex/app-server-processes.ts --items allRepository: lidge-jun/opencodex
Length of output: 6174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Unix/Darwin snapshot enumeration implementation and the test file.
sed -n '220,395p' src/codex/app-server-processes.ts
printf '\n==== TEST FILE ====\\n'
sed -n '1,260p' tests/codex-app-server-processes.test.ts
# Look for any tests that mention the default snapshot path or getuid.
printf '\n==== SEARCH ====\\n'
rg -n "\b(defaultListSnapshots|getuid|parseUnixProcStatusUid|listUnixProcSnapshots|listDarwinSnapshots|listWindowsSnapshots)\b" tests srcRepository: lidge-jun/opencodex
Length of output: 21413
Add a Unix UID-scoped regression test tests/codex-app-server-processes.test.ts only covers injected snapshots and the Windows real-enumeration path; nothing exercises src/codex/app-server-processes.ts:367-382, where getuid() is threaded into listUnixProcSnapshots() / listDarwinSnapshots(). Add a focused Unix test for the default snapshot path, or split the enumerator behind an injectable seam, so a future widening of detection to other users’ processes can’t slip through CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-app-server-processes.test.ts` around lines 111 - 124, Add a
focused Unix regression test in the codex app-server process tests that
exercises the default snapshot path and verifies enumeration is scoped to the
current UID, covering the getuid flow into
listUnixProcSnapshots/listDarwinSnapshots. If direct process enumeration is not
controllable in tests, introduce a narrow injectable seam and assert that other
users’ processes are excluded while matching same-user processes remain
detected.
| test("restartCodexAppServers signals all first, shared wait deadline, no SIGKILL", () => { | ||
| const signals: Array<{ pid: number; signal: NodeJS.Signals }> = []; | ||
| const waits: number[] = []; | ||
| const alive = new Set([100, 200]); | ||
| const snapshots = [ | ||
| { pid: 100, commandLine: "codex app-server" }, | ||
| { pid: 200, commandLine: "codex-code-mode-host" }, | ||
| ]; | ||
| let now = 1_000; | ||
| const result = restartCodexAppServers( | ||
| snapshots, | ||
| { | ||
| listSnapshots: () => snapshots, | ||
| kill: (pid, signal) => { | ||
| signals.push({ pid, signal }); | ||
| if (pid === 100) alive.delete(100); | ||
| }, | ||
| isAlive: pid => alive.has(pid), | ||
| waitExit: (pid, timeoutMs) => { | ||
| waits.push(timeoutMs); | ||
| now += 500; | ||
| return !alive.has(pid); | ||
| }, | ||
| now: () => now, | ||
| }, | ||
| ); | ||
| expect(signals).toEqual([ | ||
| { pid: 100, signal: "SIGTERM" }, | ||
| { pid: 200, signal: "SIGTERM" }, | ||
| ]); | ||
| // Shared deadline: second wait gets the remaining budget, not another full 2s. | ||
| expect(waits).toEqual([2_000, 1_500]); | ||
| expect(result.stopped).toEqual([100]); | ||
| expect(result.surviving).toEqual([200]); | ||
| expect(result.failed).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The test titled "signals all first" does not actually assert that ordering.
signals (Lines 127, 152-155) only records that both PIDs got SIGTERM, and waits (Lines 128, 157) only records the timeout values. An implementation that interleaved — kill(100); waitExit(100); kill(200); waitExit(200) — produces byte-identical results: signals is still [100, 200], and waits is still [2_000, 1_500] because now only advances inside waitExit. So the signal-all-then-wait batching, which is the entire reason the shutdown overlaps instead of serializing, is currently unverified.
Recording both call kinds into one ordered log closes the gap at near-zero cost:
💚 Assert interleaving with a single ordered event log
test("restartCodexAppServers signals all first, shared wait deadline, no SIGKILL", () => {
const signals: Array<{ pid: number; signal: NodeJS.Signals }> = [];
const waits: number[] = [];
+ const events: string[] = [];
const alive = new Set([100, 200]);
const snapshots = [
{ pid: 100, commandLine: "codex app-server" },
{ pid: 200, commandLine: "codex-code-mode-host" },
];
let now = 1_000;
const result = restartCodexAppServers(
snapshots,
{
listSnapshots: () => snapshots,
kill: (pid, signal) => {
signals.push({ pid, signal });
+ events.push(`kill:${pid}`);
if (pid === 100) alive.delete(100);
},
isAlive: pid => alive.has(pid),
waitExit: (pid, timeoutMs) => {
waits.push(timeoutMs);
+ events.push(`wait:${pid}`);
now += 500;
return !alive.has(pid);
},
now: () => now,
},
);
+ // Both SIGTERMs must precede any waiting so shutdown overlaps.
+ expect(events).toEqual(["kill:100", "kill:200", "wait:100", "wait:200"]);
expect(signals).toEqual([📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("restartCodexAppServers signals all first, shared wait deadline, no SIGKILL", () => { | |
| const signals: Array<{ pid: number; signal: NodeJS.Signals }> = []; | |
| const waits: number[] = []; | |
| const alive = new Set([100, 200]); | |
| const snapshots = [ | |
| { pid: 100, commandLine: "codex app-server" }, | |
| { pid: 200, commandLine: "codex-code-mode-host" }, | |
| ]; | |
| let now = 1_000; | |
| const result = restartCodexAppServers( | |
| snapshots, | |
| { | |
| listSnapshots: () => snapshots, | |
| kill: (pid, signal) => { | |
| signals.push({ pid, signal }); | |
| if (pid === 100) alive.delete(100); | |
| }, | |
| isAlive: pid => alive.has(pid), | |
| waitExit: (pid, timeoutMs) => { | |
| waits.push(timeoutMs); | |
| now += 500; | |
| return !alive.has(pid); | |
| }, | |
| now: () => now, | |
| }, | |
| ); | |
| expect(signals).toEqual([ | |
| { pid: 100, signal: "SIGTERM" }, | |
| { pid: 200, signal: "SIGTERM" }, | |
| ]); | |
| // Shared deadline: second wait gets the remaining budget, not another full 2s. | |
| expect(waits).toEqual([2_000, 1_500]); | |
| expect(result.stopped).toEqual([100]); | |
| expect(result.surviving).toEqual([200]); | |
| expect(result.failed).toEqual([]); | |
| }); | |
| test("restartCodexAppServers signals all first, shared wait deadline, no SIGKILL", () => { | |
| const signals: Array<{ pid: number; signal: NodeJS.Signals }> = []; | |
| const waits: number[] = []; | |
| const events: string[] = []; | |
| const alive = new Set([100, 200]); | |
| const snapshots = [ | |
| { pid: 100, commandLine: "codex app-server" }, | |
| { pid: 200, commandLine: "codex-code-mode-host" }, | |
| ]; | |
| let now = 1_000; | |
| const result = restartCodexAppServers( | |
| snapshots, | |
| { | |
| listSnapshots: () => snapshots, | |
| kill: (pid, signal) => { | |
| signals.push({ pid, signal }); | |
| events.push(`kill:${pid}`); | |
| if (pid === 100) alive.delete(100); | |
| }, | |
| isAlive: pid => alive.has(pid), | |
| waitExit: (pid, timeoutMs) => { | |
| waits.push(timeoutMs); | |
| events.push(`wait:${pid}`); | |
| now += 500; | |
| return !alive.has(pid); | |
| }, | |
| now: () => now, | |
| }, | |
| ); | |
| // Both SIGTERMs must precede any waiting so shutdown overlaps. | |
| expect(events).toEqual(["kill:100", "kill:200", "wait:100", "wait:200"]); | |
| expect(signals).toEqual([ | |
| { pid: 100, signal: "SIGTERM" }, | |
| { pid: 200, signal: "SIGTERM" }, | |
| ]); | |
| // Shared deadline: second wait gets the remaining budget, not another full 2s. | |
| expect(waits).toEqual([2_000, 1_500]); | |
| expect(result.stopped).toEqual([100]); | |
| expect(result.surviving).toEqual([200]); | |
| expect(result.failed).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-app-server-processes.test.ts` around lines 126 - 161, Update the
restartCodexAppServers test to record kill and waitExit calls in one shared
ordered event log, then assert that both SIGTERM events occur before either
waitExit event. Keep the existing signals, waits, and result assertions to
preserve verification of signal values, shared deadlines, and outcomes.
| try { | ||
| expect(child.pid).toBeGreaterThan(1); | ||
| // Brief settle so Win32_Process can observe the child. | ||
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250); | ||
| const snapshots = listWindowsSnapshots(); | ||
| const match = snapshots.find(snapshot => snapshot.pid === child.pid); | ||
| expect(match).toBeDefined(); | ||
| expect(match!.owner).toMatch(/\\/); | ||
| expect(match!.commandLine.toLowerCase()).toContain("codex app-server"); | ||
| expect(snapshots.every(snapshot => (snapshot.owner?.trim().length ?? 0) > 0)).toBe(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Bun.sleepSync exist and does Atomics.wait throw on the main thread in Bun 1.3?
💡 Result:
Yes, Bun.sleepSync exists in Bun [1][2]. It is a built-in, blocking function that pauses the execution of the thread for a specified number of milliseconds by calling nanosleep(2) internally [1][3]. Regarding Atomics.wait, it throws an exception when called on the main thread [4][5]. This is consistent with the standard JavaScript specification and V8 behavior, as Atomics.wait is a blocking operation that cannot be used on the main thread to prevent it from stalling [4][5][6]. To perform non-blocking wait operations on the main thread, Atomics.waitAsync should be used instead [4][7][6].
Citations:
- 1: https://bun.com/reference/bun/sleepSync
- 2: https://bun.com/docs/runtime/utils
- 3: https://github.com/oven-sh/bun/blob/main/src/bun.js/api/BunObject.zig
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/wait
- 5: https://v8.dev/features/atomics
- 6: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/Atomics
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/waitAsync
🏁 Script executed:
sed -n '340,385p' tests/codex-app-server-processes.test.tsRepository: lidge-jun/opencodex
Length of output: 2175
Replace the Atomics.wait sleep in tests/codex-app-server-processes.test.ts:369 with Bun.sleepSync(250). Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250) is just a blocking pause here, but it can throw on the main thread in Bun, which makes this Windows-only test unnecessarily fragile. Bun.sleepSync(250) is the idiomatic synchronous sleep and avoids that caveat.
♻️ Use the Bun sync sleep helper
expect(child.pid).toBeGreaterThan(1);
// Brief settle so Win32_Process can observe the child.
- Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250);
+ Bun.sleepSync(250);
const snapshots = listWindowsSnapshots();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| expect(child.pid).toBeGreaterThan(1); | |
| // Brief settle so Win32_Process can observe the child. | |
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250); | |
| const snapshots = listWindowsSnapshots(); | |
| const match = snapshots.find(snapshot => snapshot.pid === child.pid); | |
| expect(match).toBeDefined(); | |
| expect(match!.owner).toMatch(/\\/); | |
| expect(match!.commandLine.toLowerCase()).toContain("codex app-server"); | |
| expect(snapshots.every(snapshot => (snapshot.owner?.trim().length ?? 0) > 0)).toBe(true); | |
| try { | |
| expect(child.pid).toBeGreaterThan(1); | |
| // Brief settle so Win32_Process can observe the child. | |
| Bun.sleepSync(250); | |
| const snapshots = listWindowsSnapshots(); | |
| const match = snapshots.find(snapshot => snapshot.pid === child.pid); | |
| expect(match).toBeDefined(); | |
| expect(match!.owner).toMatch(/\\/); | |
| expect(match!.commandLine.toLowerCase()).toContain("codex app-server"); | |
| expect(snapshots.every(snapshot => (snapshot.owner?.trim().length ?? 0) > 0)).toBe(true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-app-server-processes.test.ts` around lines 366 - 375, Replace the
Atomics.wait delay in the test block with Bun.sleepSync(250), preserving the
existing 250 ms settle period before listWindowsSnapshots() runs.
| test("returns false for a missing catalog and does not warn/restart app-servers", () => { | ||
| const errors: string[] = []; | ||
| const logs: string[] = []; | ||
| let listed = 0; | ||
|
|
||
| expect(invalidateCodexModelsCache()).toBe(false); | ||
| expect(existsSync(join(codexHome, "models_cache.json"))).toBe(false); | ||
|
|
||
| // Mirrors ocx sync-cache: only call the handler when invalidate wrote. | ||
| if (invalidateCodexModelsCache()) { | ||
| afterCatalogWriteHandleAppServers({ | ||
| restart: true, | ||
| log: { log: line => logs.push(String(line)), error: line => errors.push(String(line)) }, | ||
| io: { | ||
| listSnapshots: () => { | ||
| listed += 1; | ||
| return [{ pid: 7, commandLine: "codex app-server" }]; | ||
| }, | ||
| kill: () => {}, | ||
| isAlive: () => false, | ||
| waitExit: () => true, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| expect(listed).toBe(0); | ||
| expect(errors).toEqual([]); | ||
| expect(logs).toEqual([]); | ||
| }); | ||
|
|
||
| test("returns false for invalid catalog JSON and does not warn/restart app-servers", () => { | ||
| writeFileSync(join(codexHome, "opencodex-catalog.json"), "{ not-json"); | ||
| const errors: string[] = []; | ||
| const logs: string[] = []; | ||
| let listed = 0; | ||
|
|
||
| expect(invalidateCodexModelsCache()).toBe(false); | ||
| expect(existsSync(join(codexHome, "models_cache.json"))).toBe(false); | ||
|
|
||
| if (invalidateCodexModelsCache()) { | ||
| afterCatalogWriteHandleAppServers({ | ||
| restart: false, | ||
| log: { log: line => logs.push(String(line)), error: line => errors.push(String(line)) }, | ||
| io: { | ||
| listSnapshots: () => { | ||
| listed += 1; | ||
| return [{ pid: 7, commandLine: "codex app-server" }]; | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| expect(listed).toBe(0); | ||
| expect(errors).toEqual([]); | ||
| expect(logs).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
One root cause: the write-gate has no callable seam, so every test either re-implements it or greps CLI source.
The decision "only handle Codex app-servers after a real catalog or cache write" currently lives inline inside the case "sync": and case "sync-cache": blocks of src/cli/index.ts. Because there is no exported function encapsulating it, the tests in this PR cover it two ways, and neither actually executes the production gate: three sites hand-copy the if into the test body, and one parses the CLI file as text. A regression that deleted the gate from the CLI would leave all four green.
Extracting a single exported helper — for example handleAppServersAfterSync({ catalogWritten, cacheSynced, restart, log, io }) in src/codex/app-server-processes.ts, called from both CLI cases — gives every site a real seam to call. This is the same structural fix suggested in the earlier review of the source-slicing block, now with a second, independent set of evidence behind it.
tests/codex-models-cache-invalidate.test.ts#L57-L112: replace the twoif (invalidateCodexModelsCache())blocks (Lines 66 and 96) with direct calls to the extracted helper passingcatalogWritten: false, cacheSynced: false, and assert it performs no enumeration; keep the existingtoBe(false)assertions at Lines 62 and 93 as-is.tests/codex-models-cache-invalidate.test.ts#L135-L149: drop the hand-copiedif (syncResult.catalogWritten || syncResult.cacheSynced)and passsyncResultstraight into the helper, retaining the valuable Lines 126-128 assertions untouched.tests/codex-models-cache-invalidate.test.ts#L179-L197: apply the identical substitution for the third copy of the gate at Line 183.tests/codex-app-server-processes.test.ts#L257-L295: delete thecliSource/configRoutesSourceslicing tests and replace them with direct unit tests of the extracted helper covering the fourcatalogWritten/cacheSyncedcombinations plusrestartforwarding; retain Lines 297-331, which already testattachStaleAppServerHintbehaviorally.
As per path instructions, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem" — the gate is the behavior change this PR introduces, and it is the one thing currently untested by execution.
📍 Affects 2 files
tests/codex-models-cache-invalidate.test.ts#L57-L112(this comment)tests/codex-models-cache-invalidate.test.ts#L135-L149tests/codex-models-cache-invalidate.test.ts#L179-L197tests/codex-app-server-processes.test.ts#L257-L295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-models-cache-invalidate.test.ts` around lines 57 - 112, Extract
the inline sync write-gate into an exported handleAppServersAfterSync helper,
call it from both CLI sync cases, and have it enumerate/restart app-servers only
when catalogWritten or cacheSynced is true. In
tests/codex-models-cache-invalidate.test.ts:57-112, replace both copied gates
with direct helper calls using both flags false; at 135-149 and 179-197, pass
each syncResult directly while preserving existing assertions. In
tests/codex-app-server-processes.test.ts:257-295, replace source-slicing tests
with behavioral helper tests covering all flag combinations and restart
forwarding; leave tests/codex-app-server-processes.test.ts:297-331 unchanged.
Source: Path instructions
| test("ocx sync --restart-codex neither warns nor restarts when catalog JSON is malformed", async () => { | ||
| writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "broken.json"\n', "utf8"); | ||
| writeFileSync(join(codexHome, "broken.json"), "{ not-json", "utf8"); | ||
|
|
||
| // Real sync may rematerialize bundled content over a writable malformed file; the | ||
| // regression target is the CLI gate using catalogWritten, not bundled recovery. | ||
| const syncResult = await syncModelsToCodex(10100, emptyConfig, null, { | ||
| refreshCodexModelCatalog: async () => ({ | ||
| added: 0, | ||
| path: join(codexHome, "broken.json"), | ||
| catalogExists: true, | ||
| catalogWritten: false, | ||
| cacheSynced: false, | ||
| comboOmissions: [], | ||
| }), | ||
| injectCodexConfig: async () => ({ success: true, message: "injected" }), | ||
| currentExternalCodexModelProvider: () => null, | ||
| }); | ||
|
|
||
| expect(syncResult.catalogExists).toBe(true); | ||
| expect(syncResult.catalogWritten).toBe(false); | ||
| expect(syncResult.cacheSynced).toBe(false); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The malformed catalog fixture at Lines 157-158 is never read — the stub short-circuits it, so the test name overstates coverage.
refreshCodexModelCatalog is fully replaced by the canned stub at Lines 163-170, which returns catalogWritten: false without touching the filesystem. That means the config.toml and the { not-json payload written at Lines 157-158 are dead setup: no code path parses them. The test is titled "when catalog JSON is malformed", but malformed JSON is never exercised here — what is actually verified is that syncModelsToCodex propagates its deps' catalogWritten/cacheSynced through to the result (Lines 175-177), which is a legitimate but much narrower contract.
The risk is a maintainer reading this file and concluding malformed-catalog handling is covered. It is not: the only malformed-JSON case is Lines 87-112, which is tautological for the reasons noted above. Dropping the unused writes and renaming makes the actual scope self-evident.
♻️ Remove the dead fixture and align the title with what is asserted
- test("ocx sync --restart-codex neither warns nor restarts when catalog JSON is malformed", async () => {
- writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "broken.json"\n', "utf8");
- writeFileSync(join(codexHome, "broken.json"), "{ not-json", "utf8");
-
- // Real sync may rematerialize bundled content over a writable malformed file; the
- // regression target is the CLI gate using catalogWritten, not bundled recovery.
+ test("syncModelsToCodex propagates catalogWritten/cacheSynced=false from the refresh step", async () => {
+ // Stubbed refresh: real sync may rematerialize bundled content over a writable
+ // malformed file, so the regression target is the false/false propagation, not
+ // on-disk catalog parsing.
const syncResult = await syncModelsToCodex(10100, emptyConfig, null, {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("ocx sync --restart-codex neither warns nor restarts when catalog JSON is malformed", async () => { | |
| writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "broken.json"\n', "utf8"); | |
| writeFileSync(join(codexHome, "broken.json"), "{ not-json", "utf8"); | |
| // Real sync may rematerialize bundled content over a writable malformed file; the | |
| // regression target is the CLI gate using catalogWritten, not bundled recovery. | |
| const syncResult = await syncModelsToCodex(10100, emptyConfig, null, { | |
| refreshCodexModelCatalog: async () => ({ | |
| added: 0, | |
| path: join(codexHome, "broken.json"), | |
| catalogExists: true, | |
| catalogWritten: false, | |
| cacheSynced: false, | |
| comboOmissions: [], | |
| }), | |
| injectCodexConfig: async () => ({ success: true, message: "injected" }), | |
| currentExternalCodexModelProvider: () => null, | |
| }); | |
| expect(syncResult.catalogExists).toBe(true); | |
| expect(syncResult.catalogWritten).toBe(false); | |
| expect(syncResult.cacheSynced).toBe(false); | |
| test("syncModelsToCodex propagates catalogWritten/cacheSynced=false from the refresh step", async () => { | |
| // Stubbed refresh: real sync may rematerialize bundled content over a writable | |
| // malformed file, so the regression target is the false/false propagation, not | |
| // on-disk catalog parsing. | |
| const syncResult = await syncModelsToCodex(10100, emptyConfig, null, { | |
| refreshCodexModelCatalog: async () => ({ | |
| added: 0, | |
| path: join(codexHome, "broken.json"), | |
| catalogExists: true, | |
| catalogWritten: false, | |
| cacheSynced: false, | |
| comboOmissions: [], | |
| }), | |
| injectCodexConfig: async () => ({ success: true, message: "injected" }), | |
| currentExternalCodexModelProvider: () => null, | |
| }); | |
| expect(syncResult.catalogExists).toBe(true); | |
| expect(syncResult.catalogWritten).toBe(false); | |
| expect(syncResult.cacheSynced).toBe(false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-models-cache-invalidate.test.ts` around lines 156 - 177, Remove
the unused config.toml and malformed catalog file writes from the test, and
rename the test to describe propagation of catalogExists, catalogWritten, and
cacheSynced from refreshCodexModelCatalog through syncModelsToCodex. Keep the
existing stub and result assertions unchanged.
* docs(devlog): live state sync unit for 2026-07-28 (branches, PRs, issues) Measured snapshot of origin/dev=7710185c0, 15 open PRs, 27 open issues, and the delta against the 260727 owner-decision ledger and 260728 bug-bundle plan. The audit round demoted two items the first draft got wrong: #570 is only partially fixed (items 1a/2 of its six-item hardening plan; the myhost.lan alias case still 403s), and #612 is credential-handling work under MAINTAINERS.md's security-review rule rather than a decision-free patch. * docs(devlog): rebuild-unit roadmap — screen 16 open PRs, lock 4 work-phases Two audit rounds moved this a long way from the draft. The conflict set for #576 was inverted (logic files, not i18n), the proposed usage-debug size gate would have elided 0.9% of reads, and #610's P1 turned out to be author-resolved while the same defect survived on dev's own test. * docs(devlog): record the WP2 measurement — the #610 P1 does not reproduce PATH="" never reaches the child because runCodexDebugModels calls execFile without an env option, so the launcher keeps working and the catalog loads. The isolation that line intends is decorative; the test stays safe through CODEX_CLI_PATH instead. Closing WP2 as NOOP with the observation sent to #610. * test(usage): give the rolling-file bound test room on Windows The 325 appends this test needs cost ~1,954 synchronous fs calls (measured: mkdir 325, chmod 652, append 325, exists 325, read 325, write 2). On windows-latest under full-suite load that took 13.6s and tripped the 5s default, while ubuntu and macos stayed well under it. The cost is per-open rather than per-byte, so removing one of the six calls would not close a 2.7x overshoot. Reducing the append count would trade away coverage -- 325 is already the minimum that crosses the rotate threshold twice. Both assertions and the rotation contract are unchanged. * docs(devlog): record the #576 rebase outcome and the regression it surfaced The three conflicts were the predicted logic files. What the audit caught was separate: the pre-rebase commit already deleted dev's grokSyncFailureMessage and its three handlers, and no test in the repo asserts that string, so a full green suite would not have stopped it.
Replaces #527 — same change, rebased onto current
devand retargeted.Why a new PR
#527 was opened against
codex/catalog-written-signalinstead ofdev, soenforce-targetfailed by design. Retargeting alone did not fix it: the branch carried two commits, and the first one —1ba588eff"report whether a sync actually wrote the catalog or cache" — had already landed ondevthrough #526.The two are not the same commit. Measured before dropping it:
Same six files, but
dev's version is 122 lines larger — it is a rewrite that absorbed review feedback, not a rename. Cherry-picking naively and resolving conflicts toward the PR side would have silently reverted those 122 lines, which is what made the original branch dirty in the first place.So this branch takes
origin/devand cherry-picks onlya64aa5856, the stale app-server warning.devstays canonical for everything9dd3c42datouched.Verification that nothing was reverted
Deletions appear only in files this commit actually modifies (
src/cli/index.ts,src/cli/help.ts,src/server/management/config-routes.ts, the six i18n locales,dashboard-overview-sections.tsx,cli.md). Zero deleted lines undersrc/codex/or intests/codex-refresh.test.ts/tests/injection-model-api.test.ts— the two files that were conflicting — so9dd3c42da's content is intact.The change itself
Unchanged from #527: warn when a catalog write lands while Codex app-servers are still running with the old catalog, since those processes keep serving a stale model list until restarted.
Tests
Planning and evidence:
devlog/_plan/260728_bug_bundle_resolution/040_pr527_rebase.md.Summary by CodeRabbit
New Features
--restart-codextoocx syncandocx sync-cacheto refresh stale model lists in long-running Codex sessions.Documentation
Bug Fixes