fix(codex): warn about stale Codex app-servers after a catalog write - #527
fix(codex): warn about stale Codex app-servers after a catalog write#527lidge-jun wants to merge 2 commits into
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 #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 (#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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
![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".
| // 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 while starting on a new or fallback port—for example because writing ~/.grok/config.toml fails—this catch now silently leaves the existing fence pointed at the previous listener. Grok requests then repeatedly target the closed port with no startup explanation; the same regression appears in both handleEnsure catches. Restore the error-bearing catch and repair hint at all three call sites while keeping the sync best-effort.
Useful? React with 👍 / 👎.
| const live = liveByPid.get(proc.pid); | ||
| if (!live || codexAppServerProcessIdentity(live) !== codexAppServerProcessIdentity(proc)) { |
There was a problem hiding this comment.
If the originally detected app-server exits and its PID is reused by a new app-server with the same command line before the re-scan, this comparison considers the replacement identical and sends SIGTERM to it. That can interrupt a newly created Codex host even though the stated safety contract is to avoid killing recycled PIDs. Record and compare a process creation/start-time identity in addition to PID and command line before signaling.
Useful? React with 👍 / 👎.
| const commandLine = readFileSync(`/proc/${pid}/cmdline`) | ||
| .toString("utf8") | ||
| .replace(/\0/g, " ") | ||
| .trim(); |
There was a problem hiding this comment.
/proc argv boundaries before matching
On Linux, replacing NUL separators with spaces loses the original argv boundaries before tokenizeCommandLine runs. A real invocation such as codex -C "/home/user/My Projects" app-server therefore becomes codex -C /home/user/My Projects app-server; the parser consumes only /home/user/My as the -C value, treats Projects as the subcommand, and fails to detect or restart the stale app-server. Parse the NUL-delimited argv directly, or serialize each argument with escaping that the tokenizer can reverse.
Useful? React with 👍 / 👎.
enforce-pr-target.yml had no test, and it is the one workflow that mutates a contributor's pull request — it rewrites the title and forces a draft, holding the base repository's write token while it does. Three characterisation tests, pinning what it does today rather than what it should do: - least-privilege shape: no checkout, no run step, pull-requests: write only, SHA-pinned action, one concurrent run per PR - the event set that can change the verdict (dropping ready_for_review silently makes the gate one-shot) - the bookkeeping that lets it undo its own title/draft edits The third also pins the ordering observed on #527: state is written before the mutation and is not reconciled when the mutation fails, so a failed convertToDraft still records autoDraftedByBot: true. Documented, not endorsed — the redesign owns fixing it, and this test should change with it. Verified by mutation: removing ready_for_review from the trigger list fails the second test.
|
Triage note on the stack ordering and the red check, since both are easy to misread. The
Retargeting to Merging #526 will not retarget this PR automatically. So: merge #526 first, then retarget this to |
1ba588e to
43d0eff
Compare
6462471 to
ce716cc
Compare
|
#527 follow-up after #526 landed: Please do not just retarget this PR to Current live state:
Requested next step: please rebuild this PR from current One rebase-specific note: please preserve the current Grok sync failure diagnostic in This is not merge approval yet. The process detection / optional SIGTERM path still needs review on a clean |
Inventory all 23 open issues and 14 open PRs on the axis of decisions only the maintainer can make, clustered into six bundles for later PABCD cycles. Live findings: - anthropics/claude-code#1124, cited as upstream evidence on #543, is CLOSED - #462/#401/#241 carry upstream-tracking with no upstream ticket at all - #92's encrypted_content body has no filed upstream issue either - #527 targets #526's head branch, so enforce-target fails - #529 is already merged; the earlier triage matrix still lists it in flight
Two read-only audit lenses returned 30 contradictions; the verified ones are applied here and recorded in 006_corrections.md. Factual corrections: - claude-code#1124 closed 2025-05-16, not 2025-08-10 - 13 open enhancement issues, not 11 - #529 is merged, so issue #42 phase 2 is done - #418 has no PR behind it; it was miscounted as in-flight - #528 does not contain #424's current head, so it needs a rebase - #527's retarget is manual and independent of #526 Framing corrections: - stale-needs-info.yml is absent from the default branch, so bundle C had no real deadline - the debug switch #543's reporter asked for already exists - whether #545 is our defect is answerable from code, not owner judgment - #491 is an OAuth credential change, so it is a security boundary rather than a warm-up; #533 and #447 keep their security classification too Structure: bundle G added for roadmap honesty, bundle F rescoped to the security boundary, #498 split out of bundle B, cycle order rebuilt, and the loop archetype corrected to decision-elicitation.
github-actions
Bot
changed the title
|
Superseded by #576. This branch was opened against That is what made this branch dirty, and it is also why a naive conflict resolution toward this side would have quietly reverted those 122 lines. #576 takes current |
WP1 docs-only cycle. Five decade docs at diff-level precision, ordered by dependency (auth gate -> response layer -> adapter -> PR cleanup) rather than effort: - 010 SSH remote proxy: isLoopbackRequestHost couples loopback identity to port equality, so ssh -L on a different local port 403s the whole /v1/* data plane. The sibling isLoopbackOriginValue already dropped its port check in e4e0612 for the same reason. - 020 issue lidge-jun#553: the same three-line 'Provider unreachable' shape repeats at core.ts 1197/1746/1788; fold into one helper and give ERR_TLS_CERT_ALTNAME_INVALID its own wording plus a verification command. - 030 issue lidge-jun#545: the anthropic adapter prepends the Claude Code identity unconditionally on OAuth, so a classifier that already carries it loses output budget it capped at 64 tokens and retries. - 040 PR lidge-jun#527: its first commit already landed on dev as 9dd3c42, which is what makes the branch dirty; rebase keeping only a64aa58. - 050 PR lidge-jun#557: no code work remains, only a security-boundary decision. No production code changed in this cycle.
Independent read-only review returned GO-WITH-FIXES with 4 blockers. Retires WP4 outright. Its premise was inverted: skipSystemPromptPrefix means Claude Code does NOT prepend its prefix, so the proposed dedup guard could never fire — while the unit tests would have passed by feeding the constant to itself. The compared strings differ anyway, and a maintainer had already ruled on issue lidge-jun#545, from an outbound capture, that removing the OAuth identity block is not a safe fix. Also folds back: - the ssh -g / devcontainer residual risk the research doc carried but the plan had dropped, now stated as accepted risk - the CODEOWNERS security-review boundary on src/server/auth-cors.ts, so WP2 is NEEDS_HUMAN-eligible rather than self-merging - OcxConfig imports from src/types, not src/config (tsc TS2459) - a tsconfig-independent error code cast - a range-diff precondition before skipping 1ba588e on PR lidge-jun#527 - server-auth.test.ts:582-602 named as WP2's must-not-break oracle
…what dev has The plan assumed 1ba588e could simply be skipped because dev already carries 9dd3c42. Measured: same six files, but dev's version is +122 lines larger (208 vs 86 insertions, 150 differing range-diff lines) — it is a rewrite that absorbed review feedback, not a rename. Skipping the commit blindly and resolving conflicts toward the PR side would have silently reverted those 122 lines. The rebase procedure now pins dev as the canonical side and requires a post-cherry-pick diff against origin/dev to prove the delta is confined to the stale app-server warning.
1 participant
Split 2 of 2 from #518. Stacked on #526 — review that one first; this PR retargets to
devautomatically once it merges.Closes #476.
What
A long-lived Codex
app-serverkeeps serving its in-memory model list, soocx synccould write a correct catalog and Codex would still show the old models. Restarting the proxy or re-running sync did not help; only killing the app-server did, which is why rebooting appeared to fix it.app-server/code-mode-hostprocesses and warn.catalogWritten/cacheSyncedsignal from fix(codex): report whether a sync actually wrote the catalog or cache #526, so a no-op sync stays quiet.ocx sync --restart-codexandocx sync-cache --restart-codexopt into a SIGTERM. Never SIGKILL.The risky part
Process matching. It is deliberately narrow:
Invoke-CimMethodfor owner lookup on Windows.*codex*sweep.Restart is opt-in and warn-only by default, because a SIGTERM mid-turn interrupts the user's work.
Test plan
bun run typecheck— exit 0.bun test tests/codex-app-server-processes.test.ts tests/codex-models-cache-invalidate.test.ts— 22 pass / 1 skip / 0 fail.Derived from #518 at
d93b469; the union of both splits matches that commit's 21 files exactly.