fix(codex): resolve control socket symlinks before daemon restart - #4018
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P1
|
Codex review: needs real behavior proof before merge. Reviewed September 26, 2026, 6:40 AM ET / 10:40 UTC (Revision 2). ClawSweeper reviewWhat this changesThe branch resolves both Codex control-socket paths before deciding whether to restart the background server after a System Account switch, and adds symlink tests and documentation. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 2 items remain The latest release and current main retain the asymmetric socket comparison, and this PR addresses that gap with focused regression coverage. The earlier review’s proof gap remains: the new before-and-after trace exercises a copied daemon check with an injected CLI, while the contributor reports no after-fix account switch against a live daemon. Priority: P1 Review scores
Verification
How this fits togetherCodexBar copies the selected managed account’s credentials into the system Codex home. Its daemon check then reads that home’s PID record, asks the Codex CLI about the running server, and may restart the server so it reloads the selected account. flowchart LR
A[System Account selection] --> B[Publish selected credentials]
B --> C[Check daemon PID and process]
C --> D[Ask Codex CLI for server status]
D --> E[Compare resolved socket paths]
E --> F[Restart matching daemon]
F --> G[CLI uses reloaded account]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep the symmetric socket check and focused tests, then demonstrate an after-fix System Account switch through the installed CodexBar and Codex CLI, showing the daemon restart and the CLI reflecting the selected account. Do we have a high-confidence way to reproduce the issue? Yes. The released guard compares a resolved socket target with an unresolved alias, and the reporter’s live pre-fix account-switch observations and linked socket harness exercise that mismatch. I did not execute the app in this read-only review. Is this the best way to solve the issue? Yes. Resolving both sides of the existing guard is a narrow repair that preserves its other checks; the remaining question is whether the installed app completes the intended restart after the fix. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 3c97aa79eea7. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Preserve symmetric socket resolution with an inline PID guard, keeping the production change neutral in line count. Cover symlinked homes and failed probes through dangling socket aliases, and document the home/socket boundary. Refs steipete#3990, steipete#4018. Co-authored-by: massdo <46751454+massdo@users.noreply.github.com>
|
Merged as yours, @massdo, with your commit intact; the maintainer commit on your branch only added the symlinked-home and dangling-link coverage plus the doc and changelog lines. Good catch: the #4006 guard canonicalized the reported socket but compared it against the unresolved expected path, and since Codex publishes the home socket as a symlink into its protected per-user directory, the restart was being skipped silently. Ships in 0.67.1. Thanks! |
2 participants
Summary
After a System Account switch, CodexBar can silently skip the daemon restart added in #4006 because it resolves the reported socket path but not the expected socket path. Codex's Unix app-server publishes that expected path as a symbolic link, so the two URLs compare unequal even though they identify the same socket.
This PR resolves both paths before comparing them and extends the existing promotion tests with real filesystem symlinks. It is a follow-up to #3990 and #4006.
Cause and fix
The current guard compares the resolved destination against the unresolved alias:
The mismatch returns
nilbefore the restart command. That branch also produces no recovery note. The fix applies the same symlink resolution and path standardization to the expected URL; PID/process, running-state, backend, and home checks remain in place.Source references:
Diagnostic history and anonymized evidence
All account labels and local paths below are anonymized.
CODEX_HOMEwas unset in the checked CodexBar, CLI, and daemon processes, and all resolved to the same home. Published observations.codex app-server daemon versionreportedstatus: running,backend: pid, and CLI/app-server/managed versions0.157.1. The reported socket path was the alias under the same Codex home. Evaluating the original Foundation URL comparison against that existing link returnedfalse; resolving both URLs returnedtrue. This check did not restart the daemon or read authentication contents.Regression coverage
The existing promotion test now covers both PID filenames with a plain path, a symlink alias, and the resolved symlink target. It still verifies that the selected credentials are published before exactly one restart and that the command receives the destination
CODEX_HOME.The existing rejection test also runs with symlinked paths: a different home with a different target, an unmanaged server, and a stopped server must not restart. CLI responses and process matching remain injected; the filesystem symlinks are real, with fixture files as targets.
Validation
false; symmetric resolutiontruemake test, two macOS shardsmake checkandgit diff --checkReproducible local socket harness and anonymized before/after output are preserved on a separate proof branch.
Validation workflow and retained logs. The fork-only workflow restores the original implementation for the red test, reinstates the fix for the green test, then runs the repository checks. It is not part of the proposed source changes.
Commands and environment
The local Command Line Tools installation lacks Swift's
Testingmodule, so the package test results come from the Xcode 26.6 GitHub runners. Localmake checkpassed withXCODE_DEFAULT_TOOLCHAIN_OVERRIDE=/Library/Developer/CommandLineToolsso SwiftLint could locate SourceKit. Local runtime proof uses Foundation and temporary sockets only.The automated promotion tests use isolated homes and synthetic credentials with Keychain and real Codex/session file access suppressed. The additional local harness used real temporary Unix sockets, copied
restartIfRunningunchanged from each revision, and injected the CLI/process dependencies. A post-fix UI A → B test against a live authenticated daemon has not been performed; the live evidence above establishes the failure, and the automated tests exercise the corrected promotion path.