Codex/app server restart - #802
Conversation
syncCatalogModels() and invalidateCodexModelsCache() both succeeded silently whether or not they wrote anything, so a caller could not tell a real catalog update from a no-op on a missing or unreadable catalog. Return that fact: syncCatalogModels() gains catalogWritten, and invalidateCodexModelsCache() returns whether it rewrote models_cache. refreshCodexModelCatalog() carries both outward. This is the signal half of lidge-jun#518, split out so it can be reviewed on its own. Nothing acts on it yet — the consumer (warning about, and optionally restarting, stale app-server processes) is the other half, and it touches process termination, which deserves its own review.
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 (lidge-jun#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.
📝 WalkthroughWalkthroughThe change detects stale Codex ChangesCodex app-server refresh flow
Grok synchronization error handling
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 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 docs-site/src/content/docs/reference/cli.md lines
150-154 to state that --restart-codex acts only after a successful catalog or
cache write. Apply the same write-status condition to the stale-process warning
and recovery steps in docs-site/src/content/docs/guides/codex-integration.md
lines 194-198. In gui/src/pages/dashboard-overview-sections.tsx line 225, ensure
the ocx sync --restart-codex command performs a write after API sync, or replace
it with a command that reliably rewrites the cache.
- Around line 145-159: Update the translated CLI pages under the locale
reference documentation for ja, ko, ru, and zh-cn to match the English `ocx
sync` and `ocx sync-cache` entries: document `--restart-codex`, the stale
long-lived `app-server` warning, and that only matching current-user Codex
processes are terminated, including Windows support. Preserve each page’s
existing language and structure.
In `@gui/src/i18n/en.ts`:
- Line 202: Update the model-page subtitle translations, including
dash.models.subtitle in English and German and the corresponding keys in all
locale files, to state that cache invalidation normally avoids a restart but a
stale long-lived app-server requires the provided restart command. Ensure every
translated locale matches the corrected English guidance and does not claim that
no restart is ever needed.
In `@src/cli/index.ts`:
- Around line 762-768: Update the sync-cache flow around
invalidateCodexModelsCache so its false result logs an error and sets
process.exitCode = 1 before break; retain the existing restart handling when it
returns true.
In `@src/codex/app-server-processes.ts`:
- Around line 246-255: Update the UID filter in the process scan around
parseUnixProcStatusUid so that when uid is requested, only processes with a
defined processUid equal to uid are retained; skip entries with an unreadable or
non-matching UID. Preserve the existing unrestricted behavior when uid is
undefined.
- Around line 412-430: Update restartCodexAppServers so the default process
snapshot is resolved inside the function body after io is available, using
listCodexAppServerProcesses(io). Store the resolved value in a targets variable
and use targets when building requested PIDs, preserving explicitly supplied
processes unchanged.
- Around line 263-301: Update both ps argument lists in listDarwinSnapshots to
include the macOS -ww option, preserving the existing UID-specific and
all-process invocations while ensuring command output is not truncated when
piped.
In `@tests/codex-app-server-processes.test.ts`:
- Around line 250-295: Harden the raw-source slice checks in the tests around
the sync, sync-cache, and /api/sync sections by asserting every start and end
marker is found before calling slice, including the “case "v2"”, “case "gui"”,
and update-check pathname markers. Keep these checks as lightweight wiring smoke
tests while relying on existing behavioral tests for gate semantics, and avoid
adding formatting-sensitive assertions beyond the required wiring markers.
In `@tests/injection-model-api.test.ts`:
- Around line 250-254: Update the mock return value in syncCatalogModels to
include the required comboOmissions property as an empty array, while preserving
the existing added, path, and catalogWritten values.
🪄 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: 8b2049c1-854a-4709-8b91-faaa95f1562b
📒 Files selected for processing (21)
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/codex/catalog/sync.tssrc/codex/refresh.tssrc/codex/sync.tssrc/server/management/config-routes.tstests/codex-app-server-processes.test.tstests/codex-models-cache-invalidate.test.tstests/codex-refresh.test.tstests/codex-sync-api.test.tstests/injection-model-api.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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'SIGTERM|process\.platform|win32|app-server|code-mode-host' \
src/codex/app-server-processes.ts src/cli/index.tsRepository: lidge-jun/opencodex
Length of output: 25366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- process discovery implementation ---'
sed -n '232,370p' src/codex/app-server-processes.ts
printf '%s\n' '--- platform-aware matching and restart flow ---'
sed -n '367,515p' src/codex/app-server-processes.ts
printf '%s\n' '--- related tests and documentation references ---'
rg -n -C 4 'listWindowsSnapshots|restartCodexAppServers|afterCatalogWriteHandleAppServers|restart-codex|sync-cache' \
test tests src docs-site 2>/dev/null || true
printf '%s\n' '--- locale CLI documentation files ---'
find docs-site/src/content/docs -type f -path '*/cli.md' -print 2>/dev/null | sortRepository: lidge-jun/opencodex
Length of output: 49661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path("docs-site/src/content/docs/reference/cli.md"),
Path("docs-site/src/content/docs/ja/reference/cli.md"),
Path("docs-site/src/content/docs/ko/reference/cli.md"),
Path("docs-site/src/content/docs/ru/reference/cli.md"),
Path("docs-site/src/content/docs/zh-cn/reference/cli.md"),
]
for path in paths:
lines = path.read_text(encoding="utf-8").splitlines()
starts = [i for i, line in enumerate(lines) if line.startswith("### `ocx sync")]
print(f"--- {path} ---")
for start in starts:
end = next((i for i in range(start + 1, len(lines))
if lines[i].startswith("### ")), len(lines))
block = lines[start:end]
print(f"lines {start + 1}-{end}:")
print("\n".join(block))
print("has_restart_flag=", any("--restart-codex" in line for line in block))
print("has_stale_warning=", any("app-server" in line or "app-server" in line.lower() for line in block))
PY
printf '%s\n' '--- platform dispatch and signal call ---'
rg -n -C 3 'defaultListSnapshots|platform === "win32"|process\.kill\(pid, signal\)|powershell\.exe|GetOwner' \
src/codex/app-server-processes.tsRepository: lidge-jun/opencodex
Length of output: 5886
🌐 Web query:
Node.js process.kill SIGTERM Windows behavior official documentation
💡 Result:
On Windows, POSIX signals are not supported, meaning there is no direct equivalent to the termination behavior triggered by signals on non-Windows platforms [1][2]. However, Node.js provides emulation for specific signals when using process.kill or subprocess.kill [1][2]. When you use process.kill(pid, 'SIGTERM') or subprocess.kill('SIGTERM') on Windows: 1. Forced Termination: The signal is treated as an instruction for unconditional and abrupt termination [1][2]. The process is killed immediately, similar to how it would be handled by 'SIGKILL' [2]. 2. Signal Reporting: Despite the forced nature of the termination, Node.js emulates the behavior for child processes, meaning the subprocess will correctly report that it was terminated by 'SIGTERM' [1][2]. 3. No Graceful Shutdown: Because Windows does not support POSIX signals, a Node.js process on Windows cannot listen for or handle 'SIGTERM' to perform a graceful shutdown [1][3]. While you can register a listener for 'SIGTERM' in your code, it will not be triggered when the process is killed in this manner [1][4]. In summary, 'SIGTERM' on Windows acts as a forced kill rather than a request for graceful exit [1][2]. This behavior applies specifically to 'SIGINT', 'SIGTERM', 'SIGQUIT', and 'SIGKILL', which are the signals supported by the Node.js emulation layer on Windows [2][5].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/process.html
- 2: https://nodejs.org/api/child_process.html
- 3: https://nodejs.org/api/process.html
- 4: https://nodejs.org/docs/latest-v25.x/api/process.html
- 5: nodejs/node@a882536596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("docs-site/src/content/docs/reference/cli.md"),
Path("docs-site/src/content/docs/ja/reference/cli.md"),
Path("docs-site/src/content/docs/ko/reference/cli.md"),
Path("docs-site/src/content/docs/ru/reference/cli.md"),
Path("docs-site/src/content/docs/zh-cn/reference/cli.md"),
]
for path in paths:
lines = path.read_text(encoding="utf-8").splitlines()
starts = [i for i, line in enumerate(lines) if line.startswith("### `ocx sync")]
print(f"--- {path} ---")
for start in starts:
end = next(
(i for i in range(start + 1, len(lines)) if lines[i].startswith("### ")),
len(lines),
)
block = lines[start:end]
print(f"lines {start + 1}-{end}:")
print("\n".join(block))
print("has_restart_flag=", any("--restart-codex" in line for line in block))
print("has_stale_warning=", any("app-server" in line.lower() for line in block))
PY
printf '%s\n' '--- platform dispatch and signal call ---'
rg -n -C 3 'defaultListSnapshots|platform === "win32"|process\.kill\(pid, signal\)|powershell\.exe|GetOwner' \
src/codex/app-server-processes.tsRepository: lidge-jun/opencodex
Length of output: 5886
Update the translated CLI pages.
src/codex/app-server-processes.ts:304-370 supports current-user process discovery on Windows through PowerShell, so no Unix-only manual fallback is required. However, docs-site/src/content/docs/{ja,ko,ru,zh-cn}/reference/cli.md still omits --restart-codex and the stale app-server warning. Add the documented restart behavior to each locale page.
🤖 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 pages under the locale reference documentation for ja, ko,
ru, and zh-cn to match the English `ocx sync` and `ocx sync-cache` entries:
document `--restart-codex`, the stale long-lived `app-server` warning, and that
only matching current-user Codex processes are terminated, including Windows
support. Preserve each page’s existing language and structure.
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make all stale-process guidance reflect the write-status gate.
Stale warnings and restarts occur only after a successful catalog or cache write. No-op syncs do not terminate matching processes.
docs-site/src/content/docs/reference/cli.md#L150-L154: state that--restart-codexacts only after a successful write.docs-site/src/content/docs/guides/codex-integration.md#L194-L198: add the same condition to the warning and recovery steps.gui/src/pages/dashboard-overview-sections.tsx#L225-L225: ensureocx sync --restart-codexperforms a write after the API sync, or show a command that reliably rewrites the cache.
📍 Affects 3 files
docs-site/src/content/docs/reference/cli.md#L150-L154(this comment)docs-site/src/content/docs/guides/codex-integration.md#L194-L198gui/src/pages/dashboard-overview-sections.tsx#L225-L225
🤖 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
docs-site/src/content/docs/reference/cli.md lines 150-154 to state that
--restart-codex acts only after a successful catalog or cache write. Apply the
same write-status condition to the stale-process warning and recovery steps in
docs-site/src/content/docs/guides/codex-integration.md lines 194-198. In
gui/src/pages/dashboard-overview-sections.tsx line 225, ensure the ocx sync
--restart-codex command performs a write after API sync, or replace it with a
command that reliably rewrites the cache.
Source: Path instructions
| "dash.syncing": "Syncing…", | ||
| "dash.syncOk": "Sync complete. {count} model(s) appended.", | ||
| "dash.syncStaleHint": "If Codex App still shows an older list, restart its long-lived app-server process.", | ||
| "dash.syncStaleHint": "If Codex still shows an older list, restart its long-lived app-server ({cmd}).", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the conflicting “no restart is needed” guidance.
This new message says that a long-lived app-server can retain an old model list. However, dash.models.subtitle at Line 343 still says that “no restart is needed”; the German equivalent at Line 329 says the same.
Update the model-page copy to state that cache invalidation normally avoids a restart, but a stale app-server requires the restart command. Apply the same correction to the other locale files.
As per path instructions, translated locale pages must not contradict the English source.
🤖 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/en.ts` at line 202, Update the model-page subtitle translations,
including dash.models.subtitle in English and German and the corresponding keys
in all locale files, to state that cache invalidation normally avoids a restart
but a stale long-lived app-server requires the provided restart command. Ensure
every translated locale matches the corrected English guidance and does not
claim that no restart is ever needed.
Source: Path instructions
| const restartCodex = args.slice(1).includes("--restart-codex"); | ||
| const { invalidateCodexModelsCache } = await import("../codex/catalog"); | ||
| invalidateCodexModelsCache(); | ||
| // Only warn/restart when models_cache was actually rewritten from a readable catalog. | ||
| if (invalidateCodexModelsCache()) { | ||
| const { afterCatalogWriteHandleAppServers } = await import("../codex/app-server-processes"); | ||
| afterCatalogWriteHandleAppServers({ restart: restartCodex, log: console }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Print the full sync-cache CLI case to check user-facing output and exit codes.
set -euo pipefail
rg -n -A16 'case "sync-cache":' src/cli/index.tsRepository: lidge-jun/opencodex
Length of output: 1058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CLI dispatch and sync-cache context ---'
sed -n '700,790p' src/cli/index.ts
printf '%s\n' '--- sync implementation ---'
sed -n '500,570p' src/codex/catalog/sync.ts
printf '%s\n' '--- command completion/error handling references ---'
rg -n -C4 'sync-cache|process\.exit|exitCode|invalidateCodexModelsCache|afterCatalogWriteHandleAppServers' src/cli src/codexRepository: lidge-jun/opencodex
Length of output: 42692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CLI file outline and entrypoint ---'
ast-grep outline src/cli/index.ts
printf '%s\n' '--- CLI tail and top-level invocation ---'
tail -n 140 src/cli/index.ts
printf '%s\n' '--- all sync-cache references ---'
rg -n -C6 'sync-cache|invalidateCodexModelsCache' .Repository: lidge-jun/opencodex
Length of output: 46857
Set a non-zero exit code when invalidateCodexModelsCache() returns false.
src/cli/index.ts:761-770 has no success output, but the sync-cache case still exits with status 0 when the catalog is missing, unreadable, or the cache write fails. Print an error and set process.exitCode = 1 before break.
🤖 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 762 - 768, Update the sync-cache flow around
invalidateCodexModelsCache so its false result logs an error and sets
process.exitCode = 1 before break; retain the existing restart handling when it
returns true.
| try { | ||
| const status = readFileSync(`/proc/${pid}/status`, "utf8"); | ||
| const processUid = parseUnixProcStatusUid(status); | ||
| if (uid !== undefined && processUid !== undefined && processUid !== uid) continue; | ||
| const commandLine = readFileSync(`/proc/${pid}/cmdline`) | ||
| .toString("utf8") | ||
| .replace(/\0/g, " ") | ||
| .trim(); | ||
| if (!commandLine) continue; | ||
| out.push({ pid, commandLine, uid: processUid }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Tighten the UID filter so an unreadable Uid: line does not defeat current-user scoping.
Line 249 skips a process only when processUid is known and differs. If /proc/<pid>/status is readable but the Uid: line does not parse, processUid is undefined and the process is kept. The PR contract states that matching is UID-scoped on Unix, so a process of another user can then appear in formatStaleCodexAppServerWarning output and in the --restart-codex target list. The kernel still rejects the SIGTERM with EPERM, so it is recorded as failed, not terminated. The result is a misleading warning and a spurious failure line rather than a foreign kill.
Fail closed instead: when a UID is requested, require a matching parsed UID.
🛡️ Proposed fix
const status = readFileSync(`/proc/${pid}/status`, "utf8");
const processUid = parseUnixProcStatusUid(status);
- if (uid !== undefined && processUid !== undefined && processUid !== uid) continue;
+ // Fail closed: an unparsable Uid line must not bypass current-user scoping.
+ if (uid !== undefined && processUid !== uid) continue;📝 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 { | |
| const status = readFileSync(`/proc/${pid}/status`, "utf8"); | |
| const processUid = parseUnixProcStatusUid(status); | |
| if (uid !== undefined && processUid !== undefined && processUid !== uid) continue; | |
| const commandLine = readFileSync(`/proc/${pid}/cmdline`) | |
| .toString("utf8") | |
| .replace(/\0/g, " ") | |
| .trim(); | |
| if (!commandLine) continue; | |
| out.push({ pid, commandLine, uid: processUid }); | |
| try { | |
| const status = readFileSync(`/proc/${pid}/status`, "utf8"); | |
| const processUid = parseUnixProcStatusUid(status); | |
| // Fail closed: an unparsable Uid line must not bypass current-user scoping. | |
| if (uid !== undefined && processUid !== uid) continue; | |
| const commandLine = readFileSync(`/proc/${pid}/cmdline`) | |
| .toString("utf8") | |
| .replace(/\0/g, " ") | |
| .trim(); | |
| if (!commandLine) continue; | |
| out.push({ pid, commandLine, uid: processUid }); |
🤖 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 246 - 255, Update the UID
filter in the process scan around parseUnixProcStatusUid so that when uid is
requested, only processes with a defined processUid equal to uid are retained;
skip entries with an unreadable or non-matching UID. Preserve the existing
unrestricted behavior when uid is undefined.
| 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.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
macOS ps -u option numeric uid selection and -ww wide output truncation behavior
💡 Result:
On macOS, the ps command's behavior regarding numeric UIDs and output truncation is defined by the following characteristics: Numeric UID Selection and Display The -u option in macOS ps serves a dual purpose [1][2]. When used for selection, it is generally intended to specify a list of UIDs (or usernames) [1][3]. However, when used as a display format flag (e.g., in combination with -f), it instructs ps to display the user name instead of the numeric UID [1][2]. If the -u option is not specified in this display context, the numeric UID is typically shown [1][3]. Wide Output and Truncation Behavior By default, the ps command on macOS truncates output—particularly the command field—based on the detected terminal width [2][4]. 1. Default Width: ps automatically determines the terminal width [2][4]. If the output is redirected or the terminal size is narrow, long commands will be truncated [2][5]. 2. Controlling Width: - -w Option: Using the -w option forces ps to use 132 columns [2][6]. Specifying -w multiple times (e.g., -ww) instructs ps to use an unlimited width, effectively disabling truncation for the command column [2][6]. - COLUMNS Environment Variable: You can override the automatic width detection by setting the COLUMNS environment variable to a desired integer value [2][4]. Setting this to a very high number can also prevent truncation [5]. Note that the behavior of the -w option and truncation can sometimes be sensitive to whether the output is sent to a terminal (tty) or a pipe, and it is standard practice in BSD-derived ps implementations to rely on the COLUMNS variable or multiple -w flags to ensure full command display [2][5][7].
Citations:
- 1: https://www.unix.com/man-page/osx/1/ps/
- 2: https://ss64.com/mac/ps.html
- 3: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man1/ps.1.html
- 4: https://www.unix.com/man_page/osx/1/ps/
- 5: https://lists.freebsd.org/pipermail/freebsd-bugs/2012-January/047334.html
- 6: https://man.bsd.lv/FreeBSD-11.2/ps.1
- 7: https://lists.freebsd.org/pipermail/freebsd-bugs/2012-January/047331.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline src/codex/app-server-processes.ts
printf '%s\n' '--- target implementation ---'
cat -n src/codex/app-server-processes.ts | sed -n '1,340p'
printf '%s\n' '--- related symbols and call sites ---'
rg -n -C 3 'listDarwinSnapshots|isCodexAppServerCommandLine|codexAppServerProcessIdentity|ProcessSnapshot' srcRepository: lidge-jun/opencodex
Length of output: 23800
🌐 Web query:
site:developer.apple.com macOS ps man page "-u userlist" "-ww"
💡 Result:
The command ps -u userlist -ww is used on macOS to display process information for specific users with wide output formatting [1]. -u userlist: This option selects processes associated with the specified user(s) [1]. It accepts a comma-separated list of usernames or user IDs (UIDs). When this flag is used, ps displays specific columns by default, including user, pid, %cpu, %mem, vsz, rss, tt, state, start, time, and command [1]. -ww: This option enables wide output [2]. When used, the ps command will not truncate the output of the command column (or other long output fields), allowing you to see the full command line arguments for each process, even if they exceed the width of your terminal window [2]. It is important to note that the -u option has a specific history on macOS. In early versions, ps -aux was common, but because of POSIX/Unix03 conformance, -u now strictly expects a user argument [1]. While ps aux (without the dash) remains supported as a convenience for backward compatibility, using ps -u without a following user list may result in errors or unintended behavior [1]. Additionally, ps may be restricted when run from within a sandboxed environment [3][4].
Citations:
- 1: https://developer.apple.com/library/archive/releasenotes/Darwin/RN-Unix03Conformance/index.html
- 2: https://developer.apple.com/forums/thread/29425
- 3: https://developer.apple.com/forums/thread/776473
- 4: https://developer.apple.com/forums/thread/655349
🌐 Web query:
Darwin ps source "-u userlist" numeric UID selection "-w" unlimited width
💡 Result:
On Darwin (macOS), the ps command provides specific options for process selection by user and output width control [1][2][3]. Process Selection by UID To select processes by a numeric user ID (UID), use the -u option followed by the numeric ID [1][2]. The command syntax is: ps -u Note that on Darwin, if you use the -u option, ps may display the username instead of the numeric UID by default [2][3]. If you specifically require numeric output for UID or other fields, you may need to use the -o option to explicitly format the output (e.g., ps -o uid,pid,args -u ) [1][4]. Unlimited Output Width To set the output width to unlimited on Darwin, use the -w option multiple times [1][2]. Specifying -w once sets the width to 132 columns; specifying it twice (-ww) enables unlimited width, causing ps to ignore the current terminal window size and display full command lines [1][2][3]. Example command: ps -u -ww When output is redirected to a file or piped to another command, Darwin's ps naturally defaults to unlimited columns [2][3]. You can also force a specific width by setting the COLUMNS environment variable before running the command [1][2]: COLUMNS=500 ps -u
Citations:
- 1: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man1/ps.1.html
- 2: https://ss64.com/mac/ps.html
- 3: https://www.unix.com/man_page/osx/1/ps/
- 4: https://man.uex.se/1/ps
🌐 Web query:
macOS Darwin ps command output width when stdout is pipe command truncation -ww
💡 Result:
On macOS, the ps command determines its output width by checking the width of the terminal window [1][2]. When you pipe the output of ps to another command, it may still truncate lines if it perceives that it should adhere to a specific width constraint [1][3]. To prevent this truncation and force full output, you can use the following methods: 1. Use the -w flag: By default, the -w option sets the output width to 132 columns [2]. If you specify the -w option twice (e.g., ps -ww), ps will use an unlimited number of columns, regardless of your window size [2][4]. 2. Override with the COLUMNS environment variable: You can manually set the output width by prefixing the command with the COLUMNS variable [1][2]. Example: COLUMNS=500 ps aux | grep process_name 3. Ensure COLUMNS is not exported: If you find that ps is unexpectedly truncating even when piped, it may be because your shell environment has the COLUMNS variable exported to a fixed value (common in some shell configurations or environments like Emacs shell-mode) [5][6]. You can check this with export | grep COLUMNS and disable it if necessary for that specific command using command substitution or by unsetting it (e.g., COLUMNS= ps aux) [6]. While some versions of ps implementations on other Unix-like systems may automatically detect pipes and disable width truncation, the BSD-based ps on macOS is known to maintain terminal-width-based truncation behavior unless explicitly overridden by the user [1][3][2].
Citations:
- 1: https://superuser.com/questions/1408774/force-wrapping-the-long-output-lines-that-arent-so-in-terminal
- 2: https://ss64.com/mac/ps.html
- 3: https://lists.freebsd.org/pipermail/freebsd-bugs/2012-January/047331.html
- 4: https://www.manpagez.com/man/1/ps/osx-10.4.php
- 5: https://lists.nongnu.org/archive/html/help-bash/2013-01/msg00001.html
- 6: https://emacs.stackexchange.com/questions/81052/why-does-shell-mode-truncate-ps-output
Pass -ww to both Darwin ps invocations
macOS accepts numeric UIDs with ps -u. The command field can still be truncated when stdout is piped, which can weaken codexAppServerProcessIdentity. Add -ww to both argument lists at src/codex/app-server-processes.ts:267 and src/codex/app-server-processes.ts:272.
🧰 Tools
🪛 OpenGrep (1.26.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 both ps
argument lists in listDarwinSnapshots to include the macOS -ww option,
preserving the existing UID-specific and all-process invocations while ensuring
command output is not truncated when piped.
| 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; | ||
| const requested = processes.map(process => process.pid); | ||
| const stopped: number[] = []; | ||
| const surviving: number[] = []; | ||
| const failed: Array<{ pid: number; error: string }> = []; | ||
|
|
||
| // Re-resolve immediately before signaling so a recycled PID is never killed. | ||
| // Require the same pid+command-line identity as the original match — a new | ||
| // Codex-shaped process that reused the PID must not receive SIGTERM. | ||
| const liveByPid = new Map( | ||
| listCodexAppServerProcesses(io).map(process => [process.pid, process] as const), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the default processes argument honor the injected io.
Line 413 evaluates the default as listCodexAppServerProcesses() with no io, while Line 429 re-resolves with listCodexAppServerProcesses(io). A caller that injects io but omits processes therefore enumerates real processes on the host through defaultListSnapshots, and only the identity re-check uses the fake snapshots. In a test that shape would run ps/PowerShell on the developer machine and could push real PIDs into requested. Today every caller passes processes explicitly, so this is latent, not live.
Resolve the default inside the body after io is known.
♻️ Proposed refactor
export function restartCodexAppServers(
- processes: readonly CodexAppServerProcess[] = listCodexAppServerProcesses(),
+ processes?: readonly CodexAppServerProcess[],
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;
- const requested = processes.map(process => process.pid);
+ const targets = processes ?? listCodexAppServerProcesses(io);
+ const requested = targets.map(target => target.pid);Then iterate targets instead of processes at Line 433.
🤖 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 - 430, Update
restartCodexAppServers so the default process snapshot is resolved inside the
function body after io is available, using listCodexAppServerProcesses(io).
Store the resolved value in a targets variable and use targets when building
requested PIDs, preserving explicitly supplied processes unchanged.
| describe("CLI /api sync wiring for stale app-servers (#476)", () => { | ||
| const cliSource = readFileSync(join(import.meta.dir, "..", "src", "cli", "index.ts"), "utf8"); | ||
| const configRoutesSource = readFileSync( | ||
| join(import.meta.dir, "..", "src", "server", "management", "config-routes.ts"), | ||
| "utf8", | ||
| ); | ||
|
|
||
| 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":')); | ||
| expect(syncCase).toContain('args.slice(1).includes("--restart-codex")'); | ||
| expect(syncCase).toContain("syncResult.catalogWritten || syncResult.cacheSynced"); | ||
| expect(syncCase).toContain("afterCatalogWriteHandleAppServers"); | ||
| expect(syncCase).toContain("restart: restartCodex"); | ||
| expect(syncCase.indexOf("catalogWritten || syncResult.cacheSynced")) | ||
| .toBeLessThan(syncCase.indexOf("afterCatalogWriteHandleAppServers")); | ||
| // No-write path must not call the handler outside the gate. | ||
| const gatedBlock = syncCase.slice(syncCase.indexOf("if (syncResult.catalogWritten")); | ||
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | ||
| expect(syncCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | ||
| }); | ||
|
|
||
| test("ocx sync-cache only handles app-servers after a successful models_cache write", () => { | ||
| const syncCacheCase = cliSource.slice( | ||
| cliSource.indexOf('case "sync-cache":'), | ||
| cliSource.indexOf('case "gui":'), | ||
| ); | ||
| expect(syncCacheCase).toContain("invalidateCodexModelsCache()"); | ||
| expect(syncCacheCase).toContain("if (invalidateCodexModelsCache())"); | ||
| expect(syncCacheCase).toContain("afterCatalogWriteHandleAppServers"); | ||
| expect(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")) | ||
| .toBeLessThan(syncCacheCase.indexOf("afterCatalogWriteHandleAppServers")); | ||
| const gatedBlock = syncCacheCase.slice(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")); | ||
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | ||
| expect(syncCacheCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | ||
| }); | ||
|
|
||
| test("POST /api/sync attaches staleAppServerHint only after a write and never enumerates processes", () => { | ||
| const syncHandler = configRoutesSource.slice( | ||
| configRoutesSource.indexOf('url.pathname === "/api/sync"'), | ||
| configRoutesSource.indexOf('url.pathname === "/api/update/check"'), | ||
| ); | ||
| expect(syncHandler).toContain("attachStaleAppServerHint(result)"); | ||
| expect(syncHandler).not.toContain("listCodexAppServerProcesses"); | ||
| expect(syncHandler).not.toContain("afterCatalogWriteHandleAppServers"); | ||
| expect(STALE_CODEX_APP_SERVER_HINT).toContain("ocx sync --restart-codex"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Guard the source-text slices, or replace them with behavioral assertions.
This block asserts on the raw text of src/cli/index.ts and src/server/management/config-routes.ts. Two failure modes are latent:
- Unfound markers produce silent nonsense. If
cliSource.indexOf('case "v2":')returns-1(someone reorders or renames that case), Line 258 becomesslice(start, -1), which yields the rest of the file minus one character. ThetoContainassertions then still pass, so the regression guard disappears without any failing test. The same applies to'case "gui":'at Line 274 and to'url.pathname === "/api/update/check"'at Line 289. - The assertions couple the test to formatting.
expect(syncCase).toContain("syncResult.catalogWritten || syncResult.cacheSynced")breaks on a harmless rewrite such as extracting the condition into a named variable, even though behavior is unchanged.
tests/codex-models-cache-invalidate.test.ts Lines 114-202 already cover the same gate behaviorally, which is the stronger guard. Keep these text checks only as a cheap wiring smoke test, and make the markers assert their own presence first.
♻️ Proposed hardening for the slice helpers
+ function sliceBetween(source: string, startMarker: string, endMarker: string): string {
+ const start = source.indexOf(startMarker);
+ const end = source.indexOf(endMarker);
+ expect(start).toBeGreaterThan(-1);
+ expect(end).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.
📝 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.
| describe("CLI /api sync wiring for stale app-servers (#476)", () => { | |
| const cliSource = readFileSync(join(import.meta.dir, "..", "src", "cli", "index.ts"), "utf8"); | |
| const configRoutesSource = readFileSync( | |
| join(import.meta.dir, "..", "src", "server", "management", "config-routes.ts"), | |
| "utf8", | |
| ); | |
| 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":')); | |
| expect(syncCase).toContain('args.slice(1).includes("--restart-codex")'); | |
| expect(syncCase).toContain("syncResult.catalogWritten || syncResult.cacheSynced"); | |
| expect(syncCase).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCase).toContain("restart: restartCodex"); | |
| expect(syncCase.indexOf("catalogWritten || syncResult.cacheSynced")) | |
| .toBeLessThan(syncCase.indexOf("afterCatalogWriteHandleAppServers")); | |
| // No-write path must not call the handler outside the gate. | |
| const gatedBlock = syncCase.slice(syncCase.indexOf("if (syncResult.catalogWritten")); | |
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | |
| }); | |
| test("ocx sync-cache only handles app-servers after a successful models_cache write", () => { | |
| const syncCacheCase = cliSource.slice( | |
| cliSource.indexOf('case "sync-cache":'), | |
| cliSource.indexOf('case "gui":'), | |
| ); | |
| expect(syncCacheCase).toContain("invalidateCodexModelsCache()"); | |
| expect(syncCacheCase).toContain("if (invalidateCodexModelsCache())"); | |
| expect(syncCacheCase).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")) | |
| .toBeLessThan(syncCacheCase.indexOf("afterCatalogWriteHandleAppServers")); | |
| const gatedBlock = syncCacheCase.slice(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")); | |
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCacheCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | |
| }); | |
| test("POST /api/sync attaches staleAppServerHint only after a write and never enumerates processes", () => { | |
| const syncHandler = configRoutesSource.slice( | |
| configRoutesSource.indexOf('url.pathname === "/api/sync"'), | |
| configRoutesSource.indexOf('url.pathname === "/api/update/check"'), | |
| ); | |
| expect(syncHandler).toContain("attachStaleAppServerHint(result)"); | |
| expect(syncHandler).not.toContain("listCodexAppServerProcesses"); | |
| expect(syncHandler).not.toContain("afterCatalogWriteHandleAppServers"); | |
| expect(STALE_CODEX_APP_SERVER_HINT).toContain("ocx sync --restart-codex"); | |
| }); | |
| describe("CLI /api sync wiring for stale app-servers (`#476`)", () => { | |
| const cliSource = readFileSync(join(import.meta.dir, "..", "src", "cli", "index.ts"), "utf8"); | |
| const configRoutesSource = readFileSync( | |
| join(import.meta.dir, "..", "src", "server", "management", "config-routes.ts"), | |
| "utf8", | |
| ); | |
| function sliceBetween(source: string, startMarker: string, endMarker: string): string { | |
| const start = source.indexOf(startMarker); | |
| const end = source.indexOf(endMarker); | |
| expect(start).toBeGreaterThan(-1); | |
| expect(end).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 = sliceBetween(cliSource, 'case "sync":', 'case "v2":'); | |
| expect(syncCase).toContain('args.slice(1).includes("--restart-codex")'); | |
| expect(syncCase).toContain("syncResult.catalogWritten || syncResult.cacheSynced"); | |
| expect(syncCase).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCase).toContain("restart: restartCodex"); | |
| expect(syncCase.indexOf("catalogWritten || syncResult.cacheSynced")) | |
| .toBeLessThan(syncCase.indexOf("afterCatalogWriteHandleAppServers")); | |
| // No-write path must not call the handler outside the gate. | |
| const gatedBlock = syncCase.slice(syncCase.indexOf("if (syncResult.catalogWritten")); | |
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | |
| }); | |
| test("ocx sync-cache only handles app-servers after a successful models_cache write", () => { | |
| const syncCacheCase = sliceBetween(cliSource, 'case "sync-cache":', 'case "gui":'); | |
| expect(syncCacheCase).toContain("invalidateCodexModelsCache()"); | |
| expect(syncCacheCase).toContain("if (invalidateCodexModelsCache())"); | |
| expect(syncCacheCase).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")) | |
| .toBeLessThan(syncCacheCase.indexOf("afterCatalogWriteHandleAppServers")); | |
| const gatedBlock = syncCacheCase.slice(syncCacheCase.indexOf("if (invalidateCodexModelsCache())")); | |
| expect(gatedBlock).toContain("afterCatalogWriteHandleAppServers"); | |
| expect(syncCacheCase.replace(gatedBlock, "")).not.toContain("afterCatalogWriteHandleAppServers"); | |
| }); | |
| test("POST /api/sync attaches staleAppServerHint only after a write and never enumerates processes", () => { | |
| const syncHandler = sliceBetween( | |
| configRoutesSource, | |
| 'url.pathname === "/api/sync"', | |
| 'url.pathname === "/api/update/check"', | |
| ); | |
| expect(syncHandler).toContain("attachStaleAppServerHint(result)"); | |
| expect(syncHandler).not.toContain("listCodexAppServerProcesses"); | |
| expect(syncHandler).not.toContain("afterCatalogWriteHandleAppServers"); | |
| expect(STALE_CODEX_APP_SERVER_HINT).toContain("ocx sync --restart-codex"); | |
| }); |
🤖 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 250 - 295, Harden the
raw-source slice checks in the tests around the sync, sync-cache, and /api/sync
sections by asserting every start and end marker is found before calling slice,
including the “case "v2"”, “case "gui"”, and update-check pathname markers. Keep
these checks as lightweight wiring smoke tests while relying on existing
behavioral tests for gate semantics, and avoid adding formatting-sensitive
assertions beyond the required wiring markers.
| syncCatalogModels: async syncedConfig => { | ||
| flagSeenBySync = syncedConfig.multiAgentGuidanceEnabled; | ||
| return { added: 0, path: join(tempHome!, "missing-catalog.json") }; | ||
| return { added: 0, path: join(tempHome!, "missing-catalog.json"), catalogWritten: false }; | ||
| }, | ||
| invalidateCodexModelsCache: () => {}, | ||
| invalidateCodexModelsCache: () => false, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the declared return type of syncCatalogModels and the RefreshDeps typing.
set -euo pipefail
ast-grep run --pattern $'export async function syncCatalogModels($_): $_ {
$$$
}' --lang typescript src/codex/catalog/sync.ts | head -20
rg -n -C4 'syncCatalogModels: typeof syncCatalogModels' src/codex/refresh.ts
# Show all test fixtures that mock syncCatalogModels, to compare shapes.
rg -n -A6 'syncCatalogModels:' tests/Repository: lidge-jun/opencodex
Length of output: 4137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/codex/refresh.ts | sed -n '1,120p'
cat -n tests/injection-model-api.test.ts | sed -n '240,260p'
cat -n src/codex/catalog/sync.ts | sed -n '450,466p'Repository: lidge-jun/opencodex
Length of output: 4207
Add comboOmissions: [] to the mock return value.
RefreshDeps.syncCatalogModels uses typeof syncCatalogModels, whose return type requires comboOmissions. The mock at tests/injection-model-api.test.ts:250-254 omits this property and fails type checking.
🤖 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/injection-model-api.test.ts` around lines 250 - 254, Update the mock
return value in syncCatalogModels to include the required comboOmissions
property as an empty array, while preserving the existing added, path, and
catalogWritten values.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a64aa58563
ℹ️ 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".
| 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 already NUL-separated argv, but this space-joins it before tokenizeCommandLine. If Codex was launched with a value-taking global option before app-server whose value contains spaces, such as codex -C "/home/me/My Project" app-server, the scanner sees -C /home/me/My Project app-server, skips only /home/me/My, treats Project as the subcommand, and misses the stale app-server, so ocx sync --restart-codex neither warns nor restarts it. Preserve the argv tokens from /proc or encode separators safely before matching.
Useful? React with 👍 / 👎.
|
Closing. This pull request contains no work by its author. The head commit here is byte-identical to a branch that already exists in this repository and was authored by the maintainer. All six of these pull requests have the same shape: fork the repository, push the upstream branches back unchanged, and open them as incoming contributions.
Every listed SHA resolves to the same commit on the upstream branch of the same name. The commit authors inside them are This is not a rebase, a resubmission of stalled work, or a fork that drifted. It burns maintainer review time and CI minutes on a diff that is already in the tree, and it presents other people's commits under a new author's pull request. Repository access is being revoked for this account. |
2 participants
Summary
Verification
Checklist
Summary by CodeRabbit
New Features
--restart-codexoption toocx syncandocx sync-cachefor targeted process restarts.Bug Fixes
Documentation