fix(codex): warn or restart stale app-server after sync - #518
Closed
Wibias wants to merge 11 commits into
Closed
Conversation
Disk catalog updates do not refresh Codex's in-memory app-server list (lidge-jun#476). Warn on sync/sync-cache when matching processes are present, and offer --restart-codex for targeted SIGTERM.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds cross-platform detection and scoped restarting of stale Codex app-server processes after catalog updates, wires ChangesCodex app-server lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as ocx sync
participant Catalog as Codex catalog sync
participant Handler as afterCatalogWriteHandleAppServers
participant Detector as listCodexAppServerProcesses
participant Processes as Codex app-server processes
CLI->>Catalog: write catalog or models_cache
Catalog-->>CLI: return catalog/cache write status
CLI->>Handler: pass restart option after successful write
Handler->>Detector: detect stale processes
Detector->>Processes: inspect platform snapshots
Handler->>Processes: send SIGTERM when restart is enabled
Processes-->>Handler: stopped, surviving, or failed PIDs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Tighten app-server argv matching, scope Windows listing to the current user, add macOS ps discovery, revalidate PIDs before SIGTERM, gate CLI handling on catalog writes, keep CLI samples out of i18n, and avoid blocking /api/sync on process enumeration.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Match code-mode-host only as argv0/entrypoint, require pid+cmdline identity before SIGTERM, and add CLI//api/sync wiring regressions.
CIM instance GetOwner must go through Invoke-CimMethod; the direct call failed closed on every row and broke --restart-codex on Windows.
Match Windows Install-path CommandLines with a closing quote after codex.exe/cmd, and only warn/restart after sync-cache when models_cache was actually rewritten.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Keep upstream comboOmissions on the refresh result while preserving the boolean models_cache write gate from this branch.
Wibias
added
enhancement
Treat catalogExists as insufficient proof of a write so ocx sync --restart-codex does not warn or restart when the catalog path exists but is malformed or unreadable.
github-actions
Bot
added
bug
Only attach staleAppServerHint from POST /api/sync when a catalog or models_cache write succeeded, so the dashboard does not imply a restart after a no-op sync.
Skip supported Codex global options (enable/config/profile and related forms) when locating the app-server subcommand, while still rejecting later-argument and non-app-server invocations.
Recognize --ask-for-approval/-a, --local-provider, and --add-dir when locating the app-server subcommand so common Codex root option forms still match.
Recognize platform-baked binaries like codex-x86_64-unknown-linux-musl in argv0 and the Windows candidate prefilter without accepting arbitrary codex-* tools.
4 tasks
Wibias
commented
Jul 27, 2026
Wibias
left a comment
Wibias
left a comment
Collaborator
Author
There was a problem hiding this comment.
Security review: PASS
Reviewed tip d93b469 — src/codex/app-server-processes.ts, CLI --restart-codex, and POST /api/sync hint wiring. Excluded unrelated issue-translation/issue-quality workflow changes from this pass.
Summary
No medium+ findings. Process termination stays local, owner/UID-scoped, and narrowly matched; SIGTERM is preceded by PID+cmdline identity revalidation; PowerShell enumeration uses fixed script + Invoke-CimMethod GetOwner with fail-closed owner checks; logs/API expose PIDs and a static hint only (no cmdlines).
Checked
- Cross-user kill: Unix UID filter + macOS
ps -u+ Windows GetOwner vs current identity - Injection:
execFileargv array; regex literals only in PS-matchoperands - argv0 / subcommand matching + target-triple basenames; negatives for opencodex/hermes
/api/sync: hint only, no enumerate/kill- Privacy: no command-line logging on warn/restart paths
Verdict: Security review: PASS
This was referenced Jul 27, 2026
Owner
|
Split this into two stacked PRs so the process-termination half can be reviewed on its own:
Both are derived from this PR at The reasoning: the second half sends SIGTERM to matching processes, and process matching is where a mistake is expensive — it drew four rounds of review feedback here. Separating the signal from the action gives that half a small diff to review against. @Wibias this is your PR, so the call is yours: close this in favour of the two, or keep it and treat the split as a review aid. Nothing here was force-pushed or changed on your branch. |
Collaborator
Author
Collaborator
Author
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
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 #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.
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
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 #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.
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
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 #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.
lidge-jun
added a commit
that referenced
this pull request
Jul 27, 2026
* fix(codex): report whether a sync actually wrote the catalog or cache 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 #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. * test(codex): cover real catalog sync write signals
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ocx sync/ocx sync-cacherewrite on-disk catalogs, long-lived Codexapp-serverprocesses can keep serving a stale in-memory model list.codex … app-serverandcodex-code-mode-hostprocesses (UID-scoped on Unix; narrow cmdline match — no broad*codex*kills).ocx sync --restart-codex/ocx sync-cache --restart-codexsend SIGTERM only (no SIGKILL escalation)./api/syncreturns shared hint and optionalstaleAppServerPids.Test plan
bun test tests/codex-app-server-processes.test.tsbun run typecheckocx syncprints warning;ocx sync --restart-codexstops matching PIDs and refreshed list appearsSummary by CodeRabbit
--restart-codextoocx syncandocx sync-cacheto refresh stale long-running Codex app-server processes.