From a7c8bd69b134e05ce573f9d24862b5afa320025f Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Tue, 27 Oct 2026 17:16:33 -0400 Subject: [PATCH 1/3] fix: Restart Codex's background app-server on enable and disable #1518 made enable and disable warn about a running Codex app-server daemon instead of restarting it, on the belief that a restart ends Codex sessions and that Codex, unlike OpenCode, does not reconnect. Codex's source (0.163.0 and 0.164.0) says otherwise: a restart drains running turns for up to its shutdown grace (60s by default), saves its threads, and the new daemon restores them and continues an interrupted local turn; open codex windows reconnect on their own. So match OpenCode: when a daemon is running, say so before asking, restart it once after the file is written with Codex's own `codex app-server daemon restart`, then read the new daemon's environment and say whether it uses Cortex. A failed restart leaves the change made and names the command. An enable with nothing to change, and status, report the running daemon without restarting it. The restart runs with Cortex's own proxy variables removed from agentop's environment, so a restart after disable cannot put the daemon back on Cortex from the shell it ran in. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- cmd/agentop/cmd_codex.go | 42 +++- cmd/agentop/codex_daemon.go | 269 ++++++++++++++++++++-- cmd/agentop/codex_daemon_test.go | 374 +++++++++++++++++++++++++++---- docs/agents/codex.md | 51 +++-- 4 files changed, 639 insertions(+), 97 deletions(-) diff --git a/cmd/agentop/cmd_codex.go b/cmd/agentop/cmd_codex.go index 2015ca02d..d58bd166f 100644 --- a/cmd/agentop/cmd_codex.go +++ b/cmd/agentop/cmd_codex.go @@ -165,6 +165,14 @@ would quietly beat an earlier definition — including a corporate proxy someone set deliberately. The first run copies the original file to .env.bak and never overwrites that copy. disable removes only the keys it added. +Codex's background app-server, which makes the model calls for every codex +window, reads the file only when it starts. So when it is running, enable and +disable restart it once after their change (codex app-server daemon restart): +turns already running get up to its shutdown grace, a minute by default, to +finish, and open Codex windows reconnect. When it is running they say so first +and ask (--yes skips the question, not the warning). status shows whether the +running app-server uses Cortex. + Note: while enabled, Codex needs Cortex running — its requests go to the proxy address. "agentop configure codex disable" is the off switch. @@ -554,16 +562,19 @@ func codexEnable(envPath, cortexCfgPath, statePath string, yes bool, stdout, std fmt.Fprintf(stderr, "agentop: %v\n", err) return 1 } + codexHome := filepath.Dir(envPath) if len(pl.changes) == 0 { fmt.Fprintf(stdout, "Already enabled: %s routes Codex through Cortex.\n", envPath) - // Said here too: a second enable is what someone runs when Codex does not seem + // Checked here too: a second enable is what someone runs when Codex does not seem // to be going through Cortex, and a daemon older than the file is the usual - // reason. - noteCodexDaemon(envPath, true, stdout) + // reason. Reported, not restarted: there was nothing to apply. + reportCodexDaemon(codexHome, pl.want["HTTPS_PROXY"], true, false, stdout) return 0 } fmt.Fprintf(stdout, "Sets in %s:\n%s\n", envPath, strings.Join(pl.changes, "\n")) - fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak if it already existed.\n\n", envPath) + fmt.Fprintf(stdout, "Nothing else in the file changes; a copy is kept as %s.bak if it already existed.\n", envPath) + was := noteCodexDaemonRestart(codexHome, stdout) + fmt.Fprintln(stdout) if !yes && !codexConfirm(stdout) { fmt.Fprintln(stdout, "Not changed.") return exitDeclined @@ -573,7 +584,7 @@ func codexEnable(envPath, cortexCfgPath, statePath string, yes bool, stdout, std return 1 } fmt.Fprintln(stdout, "Enabled — run `codex` as usual. agentop configure codex disable undoes this.") - noteCodexDaemon(envPath, true, stdout) + finishCodexChange(codexHome, pl.want["HTTPS_PROXY"], pl.want, true, was, stdout, stderr) return 0 } @@ -673,7 +684,18 @@ func codexDisable(envPath, statePath string, yes bool, stdout, stderr io.Writer) fmt.Fprintf(stdout, "Nothing to do: none of the Cortex variables are set in %s.\n", envPath) return 0 } - fmt.Fprintf(stdout, "This will remove from %s: %s\n\n", envPath, strings.Join(pl.present, ", ")) + fmt.Fprintf(stdout, "This will remove from %s: %s\n", envPath, strings.Join(pl.present, ", ")) + codexHome := filepath.Dir(envPath) + was := noteCodexDaemonRestart(codexHome, stdout) + fmt.Fprintln(stdout) + // The proxy the restarted daemon is compared with, read before the file loses it. + // Disable has to work when Cortex is gone, so this is what the file held, not + // Cortex's config; want is nil for the same reason, and codexIsOurs falls back to + // the value's shape. + cortexProxy, _ := pl.f.get("HTTPS_PROXY") + if cortexProxy == "" { + cortexProxy, _ = pl.f.get("https_proxy") + } if !yes && !codexConfirm(stdout) { fmt.Fprintln(stdout, "Not changed.") return exitDeclined @@ -687,7 +709,7 @@ func codexDisable(envPath, statePath string, yes bool, stdout, stderr io.Writer) fmt.Fprintf(stdout, "\nRestored to the value(s) you had before: %s\n", strings.Join(restored, ", ")) } fmt.Fprintf(stdout, "\nDisabled. Codex no longer routes through Cortex.\n") - noteCodexDaemon(envPath, false, stdout) + finishCodexChange(codexHome, cortexProxy, nil, false, was, stdout, stderr) return 0 } @@ -740,10 +762,14 @@ func codexStatus(envPath, cortexCfgPath string, stdout io.Writer) int { // codexState — the table's entry point — turns a duplicate into configureUnknown, // spending its single word on the warning this command has already printed on a // line of its own above. See codexState's comment for that split. - if codexStateFrom(envPath, f, want).State == configureOn { + on := codexStateFrom(envPath, f, want).State == configureOn + if on { fmt.Fprintf(stdout, "enabled in %s\n", envPath) } else { fmt.Fprintf(stdout, "not fully enabled in %s (%d of %d set)\n", envPath, set, len(codexKeys)) } + // The file is what the next daemon reads; the running one may differ, and it is the + // one sending Codex's prompts. + reportCodexDaemon(filepath.Dir(envPath), want["HTTPS_PROXY"], on, true, stdout) return 0 } diff --git a/cmd/agentop/codex_daemon.go b/cmd/agentop/codex_daemon.go index 80a8ccf01..68bad4d13 100644 --- a/cmd/agentop/codex_daemon.go +++ b/cmd/agentop/codex_daemon.go @@ -1,13 +1,21 @@ package main import ( + "bytes" + "context" "encoding/json" "errors" "fmt" "io" + "math" "os" + "os/exec" "path/filepath" + "strings" "syscall" + "time" + + "github.com/rossoctl/cortex/core/peerproc" ) // codexDaemonPIDRel is where Codex records its app-server daemon, under CODEX_HOME. @@ -21,17 +29,29 @@ import ( // the mirror problem — the daemon keeps the proxy it started with, so it breaks if // Cortex then stops. Measured on Codex 0.163.0: a daemon started on Oct 23 sent every // prompt straight to chatgpt.com after an enable on Oct 27. +// +// So enable and disable restart a daemon they find running, as they do OpenCode's +// service. A restart interrupts Codex sessions rather than ending them (read from Codex's +// source, 0.163.0 and 0.164.0): the old daemon refuses new turns and gives running ones its shutdown +// grace to finish (60s by default), saves its threads, and the new one restores them and +// continues a local turn the grace cut off; an open codex window reconnects on its own. const codexDaemonPIDRel = "app-server-daemon/daemon.pid" -// codexDaemonRestartCmd is Codex's own command for applying a changed environment. It -// stops the daemon and starts it again, which ends any Codex work running through it. +// codexDaemonRestartCmd is Codex's own command for applying a changed environment. const codexDaemonRestartCmd = "codex app-server daemon restart" +// codexRestartTimeout bounds that command. It waits for the old daemon to drain, for up to +// its shutdown grace — at most 5 minutes — and Codex's own lock allows that plus 75s for +// the forced stop and the new start. This allows the same, plus a margin. +const codexRestartTimeout = 5*time.Minute + 90*time.Second + // codexDaemon is a running app-server daemon, as its pid file and the process table // describe it. type codexDaemon struct { PID int Started string // the pid file's processStartTime, as Codex wrote it; may be empty + Proxy string // the HTTPS proxy in its environment, as Codex reads it (codexEnvironProxy); "" when none + EnvErr error // why its environment could not be read; nil when it was } // codexDaemonAlive reports whether pid names a running process. Signal 0 delivers @@ -49,6 +69,20 @@ var codexDaemonAlive = func(pid int) bool { return err == nil || errors.Is(err, os.ErrPermission) } +// codexEnviron reads a process's environment; a package variable so tests replace it. +var codexEnviron = peerproc.Environ + +// codexRestartDaemon restarts the daemon recorded under codexHome with Codex's own command. +// want is Cortex's values for the managed keys, nil when they are not known. A package +// variable so tests replace it: nothing in a test may restart a real daemon. +var codexRestartDaemon = func(codexHome string, want map[string]string) error { + bin, err := findCodex(codexHome) + if err != nil { + return err + } + return codexRun(bin, codexHome, want, "app-server", "daemon", "restart") +} + // runningCodexDaemon returns the daemon recorded under codexHome when its process is // still alive. ok is false when there is no pid file, it cannot be read, or the process // it names has exited — a stale file Codex leaves behind after a crash or a reboot. @@ -61,33 +95,224 @@ func runningCodexDaemon(codexHome string) (codexDaemon, bool) { PID int `json:"pid"` ProcessStartTime string `json:"processStartTime"` } - if json.Unmarshal(b, &rec) != nil || !codexDaemonAlive(rec.PID) { + if json.Unmarshal(b, &rec) != nil || rec.PID > math.MaxInt32 || !codexDaemonAlive(rec.PID) { return codexDaemon{}, false } - return codexDaemon{PID: rec.PID, Started: rec.ProcessStartTime}, true + d := codexDaemon{PID: rec.PID, Started: rec.ProcessStartTime} + env, err := codexEnviron(int32(rec.PID)) + switch { + case err != nil: + d.EnvErr = fmt.Errorf("reading the environment of pid %d: %w", rec.PID, err) + case len(env) == 0: + // Unknown, not absent, for the reason probeOpenCodeService gives. + d.EnvErr = errors.New("its environment could not be read") + default: + d.Proxy = codexEnvironProxy(env) + } + return d, true } -// noteCodexDaemon tells the operator, after enable or disable has changed envPath, that -// a running daemon still has the environment it started with, and how to apply the -// change. It warns rather than restarting: a restart ends every Codex session running -// through the daemon, in any terminal or editor, and that is the operator's call. A -// daemon that is not running needs nothing — the next codex starts one with the new file. -func noteCodexDaemon(envPath string, enabled bool, stdout io.Writer) { - d, ok := runningCodexDaemon(filepath.Dir(envPath)) +// codexEnvironProxy is the proxy Codex sends HTTPS requests through, from env, a list of +// "NAME=value" strings: HTTPS_PROXY, else ALL_PROXY, each read uppercase first and then +// lowercase, and taken only when non-empty. That is Codex's own reading +// (codex-rs/http-client's proxy_env_value) and not Bun's, which is why environProxy is +// not used: Codex prefers the uppercase name, and an uppercase name set to "" hides the +// lowercase one rather than falling through to it. +func codexEnvironProxy(env []string) string { + lookup := func(name string) (string, bool) { + for _, kv := range env { + if k, v, ok := strings.Cut(kv, "="); ok && k == name { + return v, true + } + } + return "", false + } + for _, upper := range []string{"HTTPS_PROXY", "ALL_PROXY"} { + v, ok := lookup(upper) + if !ok { + v, _ = lookup(strings.ToLower(upper)) + } + if v != "" { + return v + } + } + return "" +} + +// findCodex is the codex binary: on PATH, else where Codex's standalone installer puts it +// under codexHome. +func findCodex(codexHome string) (string, error) { + if p, err := exec.LookPath("codex"); err == nil { + return p, nil + } + p := filepath.Join(codexHome, "packages", "standalone", "current", "bin", "codex") + if fi, err := os.Stat(p); err == nil && fi.Mode().IsRegular() && fi.Mode().Perm()&0o111 != 0 { + return p, nil + } + return "", fmt.Errorf("codex not found on PATH or in %s", filepath.Dir(p)) +} + +// codexRun runs the codex CLI against codexHome and returns an error carrying the first +// line of its stderr when it fails. +// +// The restarted daemon inherits this command's environment, overlaid with the dotenv file, +// so agentop's own variables reach it only when they are not Cortex's: after disable, a +// restart from a shell with Cortex's HTTPS_PROXY set would otherwise leave the daemon on +// Cortex. CODEX_HOME is set only when Codex would not find codexHome without it, so a +// daemon gains no variable it did not have before. +func codexRun(bin, codexHome string, want map[string]string, args ...string) error { + ctx, cancel := context.WithTimeout(context.Background(), codexRestartTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, bin, args...) //nolint:gosec // bin is the codex CLI, found by findCodex + env := codexCLIEnv(os.Environ(), want) + if home, err := os.UserHomeDir(); err != nil || defaultCodexEnvPath(home) != filepath.Join(codexHome, ".env") { + env = append(env, codexHomeEnv+"="+codexHome) + } + cmd.Env = env + var stderr bytes.Buffer + cmd.Stderr = &stderr + // The daemon it starts detaches with its own stdio, but bound how long Wait waits for + // the pipes anyway, as openCodeRun does. + cmd.WaitDelay = time.Second + if _, err := cmd.Output(); err != nil { + what := filepath.Base(bin) + " " + strings.Join(args, " ") + if ctx.Err() != nil { + return fmt.Errorf("%s: no answer in %s", what, codexRestartTimeout) + } + if line, _, _ := strings.Cut(strings.TrimSpace(stderr.String()), "\n"); line != "" { + return fmt.Errorf("%s: %w: %s", what, err, strings.TrimSpace(line)) + } + return fmt.Errorf("%s: %w", what, err) + } + return nil +} + +// codexCLIEnv is env, a list of "NAME=value" strings, without those of the managed keys +// (codexKeys) whose value is Cortex's, judged as disable judges them (codexIsOurs). Every +// other entry is kept. +func codexCLIEnv(env []string, want map[string]string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + if k, v, _ := strings.Cut(kv, "="); isCodexKey(k) && codexIsOurs(k, v, want) { + continue + } + out = append(out, kv) + } + return out +} + +func isCodexKey(k string) bool { + for _, c := range codexKeys { + if k == c { + return true + } + } + return false +} + +// noteCodexDaemonRestart is said before enable or disable asks, when a daemon is running: +// applying the change restarts it. It returns that daemon, nil when none runs — the next +// codex starts one with the new file, so there is nothing to restart. +func noteCodexDaemonRestart(codexHome string, stdout io.Writer) *codexDaemon { + d, ok := runningCodexDaemon(codexHome) if !ok { + return nil + } + fmt.Fprintf(stdout, "Codex's background app-server is running (pid %d%s).\n"+ + " Applying this restarts it: Codex turns already running get up to a minute to finish,\n"+ + " and open Codex windows reconnect.\n", + d.PID, startedSuffix(d)) + return &d +} + +func startedSuffix(d codexDaemon) string { + if d.Started == "" { + return "" + } + return ", started " + d.Started +} + +// finishCodexChange restarts the daemon that was running before enable (configured) or +// disable changed the file, then checks it again and says whether its proxy now matches +// the change: cortexProxy after enable, anything but cortexProxy after disable. was is +// what noteCodexDaemonRestart returned; nil leaves everything alone. A restart that fails +// is reported on stderr; the change itself succeeded, so the caller still exits 0. +func finishCodexChange(codexHome, cortexProxy string, want map[string]string, configured bool, was *codexDaemon, stdout, stderr io.Writer) { + if was == nil { return } - started := "" - if d.Started != "" { - started = ", started " + d.Started + fmt.Fprintf(stdout, "Restarting Codex's background app-server (pid %d)...\n", was.PID) + if err := codexRestartDaemon(codexHome, want); err != nil { + fmt.Fprintf(stderr, "agentop: could not restart Codex's background app-server (%v).\n"+ + " It may still be running with its old settings. To apply the change now:\n"+ + " %s\n", err, codexDaemonRestartCmd) + return + } + d, ok := runningCodexDaemon(codexHome) + switch { + case !ok: + fmt.Fprintln(stdout, "Codex's background app-server was restarted, but is not running now. "+ + "It starts with the new settings the next time you run codex.") + case d.EnvErr != nil: + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d); its environment could not be checked.\n", d.PID) + case cortexProxy == "": + // disable with no proxy of Cortex's taken out: nothing to compare with. + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d).\n", d.PID) + case sameProxy(d.Proxy, cortexProxy) == configured: + if configured { + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d) and is using Cortex.\n", d.PID) + } else { + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d) and no longer uses Cortex.\n", d.PID) + } + case configured: + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d), but its proxy is %s, not Cortex's.\n", + d.PID, proxyOrNone(d.Proxy)) + default: + fmt.Fprintf(stdout, "Codex's background app-server was restarted (pid %d), but it still uses Cortex's proxy,\n"+ + " set somewhere other than %s: in the environment you ran this from, or Codex was started from.\n", + d.PID, filepath.Join(codexHome, ".env")) + } +} + +func proxyOrNone(p string) string { + if p == "" { + return "none" + } + return p +} + +// reportCodexDaemon says how the running daemon compares with the file's routing, without +// restarting it. configured is whether the file routes Codex through Cortex, whose proxy is +// cortexProxy. status asks for a line in every case; enable, finding nothing to change, +// says only what needs doing or what it confirmed. +func reportCodexDaemon(codexHome, cortexProxy string, configured, status bool, stdout io.Writer) { + d, ok := runningCodexDaemon(codexHome) + switch { + case !ok: + if status { + fmt.Fprintln(stdout, "Codex's background app-server is not running.") + } + return + case d.EnvErr != nil && status: + fmt.Fprintf(stdout, "Could not check Codex's background app-server (pid %d): %v.\n", d.PID, d.EnvErr) + return + case d.EnvErr != nil: + fmt.Fprintf(stdout, "Could not check Codex's background app-server (pid %d): %v.\n"+ + " Restart it to be sure: %s\n", d.PID, d.EnvErr, codexDaemonRestartCmd) + return } - effect := "Until it restarts, Codex's prompts go straight to OpenAI,\n not through Cortex." - if !enabled { - effect = "Until it restarts, it still sends through Cortex, and\n stops working if Cortex does." + using := sameProxy(d.Proxy, cortexProxy) + switch { + case using && configured: + fmt.Fprintf(stdout, "Codex's background app-server (pid %d) is using Cortex.\n", d.PID) + case configured: + fmt.Fprintf(stdout, "Codex's background app-server (pid %d%s) is running with its old settings,\n"+ + " so Codex's prompts bypass Cortex. It picks up the change when it restarts:\n"+ + " %s # open Codex windows reconnect\n", d.PID, startedSuffix(d), codexDaemonRestartCmd) + case using && status: + fmt.Fprintf(stdout, "Codex's background app-server (pid %d) is using Cortex, but %s does not route it there,\n"+ + " so its next start will not use Cortex.\n", d.PID, filepath.Join(codexHome, ".env")) + case status: + fmt.Fprintf(stdout, "Codex's background app-server (pid %d) is not using Cortex.\n", d.PID) } - fmt.Fprintf(stdout, "\nCodex's background app-server is running (pid %d%s)\n"+ - " with the settings it started with. %s\n"+ - " To apply the change now:\n"+ - " %s # ends any Codex session running through it\n", - d.PID, started, effect, codexDaemonRestartCmd) } diff --git a/cmd/agentop/codex_daemon_test.go b/cmd/agentop/codex_daemon_test.go index f53e72650..93134ef99 100644 --- a/cmd/agentop/codex_daemon_test.go +++ b/cmd/agentop/codex_daemon_test.go @@ -2,13 +2,70 @@ package main import ( "bytes" + "errors" + "io" "os" "path/filepath" + "runtime" "strconv" "strings" "testing" ) +const cortexProxyURL = "http://127.0.0.1:47600" // the forward proxy in cortexCfg + +// fakeCodexDaemon is the daemon a test sees: a pid file under its CODEX_HOME, a process +// table that says that pid is alive, and an environment for it. restart, when not +// failing, replaces it with newPID running with newEnv, as Codex's restart does. +type fakeCodexDaemon struct { + pid int + env []string + envErr error + + newPID int + newEnv []string + newGone bool // the restarted daemon exits straight away + restartErr error // the restart command fails + + restarts []map[string]string // the want each restart was given +} + +// stubCodexDaemon points every daemon seam at f for one test, so nothing a test does can +// restart a real daemon or read a real process: the pid is fake, and a real process with +// that pid would otherwise be the one inspected. +func stubCodexDaemon(t *testing.T, codexHome string, f *fakeCodexDaemon) { + t.Helper() + alive, environ, restart := codexDaemonAlive, codexEnviron, codexRestartDaemon + t.Cleanup(func() { codexDaemonAlive, codexEnviron, codexRestartDaemon = alive, environ, restart }) + if f.pid != 0 { + writeCodexDaemonPID(t, codexHome, f.pid) + } + codexDaemonAlive = func(pid int) bool { return pid == f.pid } + codexEnviron = func(pid int32) ([]string, error) { + if int(pid) != f.pid { + t.Errorf("read the environment of pid %d, which is not the fake daemon's", pid) + return nil, errors.New("not the fake daemon") + } + return f.env, f.envErr + } + codexRestartDaemon = func(home string, want map[string]string) error { + if home != codexHome { + t.Errorf("restarted the daemon of %s, want %s", home, codexHome) + } + f.restarts = append(f.restarts, want) + if f.restartErr != nil { + return f.restartErr + } + f.pid, f.env, f.envErr = f.newPID, f.newEnv, nil + if f.newGone { + f.pid = 0 + } else { + writeCodexDaemonPID(t, codexHome, f.newPID) + } + return nil + } +} + // writeCodexDaemonPID writes a daemon.pid under codexHome as Codex does. func writeCodexDaemonPID(t *testing.T, codexHome string, pid int) { t.Helper() @@ -22,83 +79,219 @@ func writeCodexDaemonPID(t *testing.T, codexHome string, pid int) { } } -// withCodexDaemonAlive makes codexDaemonAlive answer for the test, so no real process -// is needed or touched. -func withCodexDaemonAlive(t *testing.T, alive func(int) bool) { +// onCortex is a daemon environment routed through Cortex, as enable's file makes it. +var onCortex = []string{"HOME=/home/someone", "HTTPS_PROXY=" + cortexProxyURL, "https_proxy=" + cortexProxyURL} + +// offCortex is one with no proxy at all. +var offCortex = []string{"HOME=/home/someone"} + +func enableCodex(t *testing.T, envPath, cfgPath string) { + t.Helper() + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d: %s", code, errb.String()) + } +} + +func wantAll(t *testing.T, what, got string, want ...string) { t.Helper() - prev := codexDaemonAlive - codexDaemonAlive = alive - t.Cleanup(func() { codexDaemonAlive = prev }) + for _, w := range want { + if !strings.Contains(got, w) { + t.Errorf("%s lacks %q:\n%s", what, w, got) + } + } } -// Enable with Codex's daemon running says the daemon still calls OpenAI directly, and -// names the command that fixes it. This is the case that looked enabled and was not: -// every prompt bypassed Cortex. -func TestCodexEnable_WarnsAboutARunningDaemon(t *testing.T) { +// Enable with a daemon running restarts it, once and after the file is written, then +// confirms the new daemon uses Cortex. This is the case that used to look enabled and was +// not: the old daemon kept sending every prompt straight to OpenAI. +func TestCodexEnable_RestartsARunningDaemon(t *testing.T) { envPath, cfgPath := codexFixture(t, "") - writeCodexDaemonPID(t, filepath.Dir(envPath), 5613) - withCodexDaemonAlive(t, func(pid int) bool { return pid == 5613 }) + f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + var wroteFirst bool + prev := codexRestartDaemon + codexRestartDaemon = func(home string, want map[string]string) error { + _, err := os.Stat(envPath) + wroteFirst = err == nil + return prev(home, want) + } var out, errb bytes.Buffer if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { t.Fatalf("enable exit %d: %s", code, errb.String()) } got := out.String() - for _, want := range []string{"pid 5613", "Fri Oct 23 09:03:25 2026", "go straight to OpenAI", codexDaemonRestartCmd} { - if !strings.Contains(got, want) { - t.Errorf("enable output lacks %q:\n%s", want, got) - } + wantAll(t, "enable output", got, "pid 5613", "Fri Oct 23 09:03:25 2026", "Applying this restarts it", + "open Codex windows reconnect", "restarted (pid 7001) and is using Cortex") + if len(f.restarts) != 1 { + t.Fatalf("restarted %d times, want once", len(f.restarts)) + } + if !wroteFirst { + t.Error("restarted before the file was written, so the new daemon read the old one") + } + if f.restarts[0]["HTTPS_PROXY"] != cortexProxyURL { + t.Errorf("restart was given want %v, which does not name Cortex's proxy", f.restarts[0]) + } + // The warning is said before the question, so it can be declined. + if strings.Index(got, "Applying this restarts it") > strings.Index(got, "Enabled —") { + t.Errorf("the restart is announced after the change:\n%s", got) + } +} + +// Declining changes nothing, so nothing is restarted, though the warning was shown. +func TestCodexEnable_DeclinedRestartsNothing(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + prev := codexConfirm + t.Cleanup(func() { codexConfirm = prev }) + codexConfirm = func(io.Writer) bool { return false } + + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), false, &out, &errb); code != exitDeclined { + t.Fatalf("exit %d, want %d", code, exitDeclined) + } + wantAll(t, "declined enable output", out.String(), "Applying this restarts it", "Not changed.") + if len(f.restarts) != 0 { + t.Errorf("a declined enable restarted the daemon %d time(s)", len(f.restarts)) } } -// Disable with the daemon running says the opposite: it still sends through Cortex, -// and breaks if Cortex stops. -func TestCodexDisable_WarnsAboutARunningDaemon(t *testing.T) { +// A restarted daemon that still has no Cortex proxy is named, not reported as fixed: +// something other than the file, such as the environment it was started from, wins. +func TestCodexEnable_SaysWhenTheRestartedDaemonStillBypassesCortex(t *testing.T) { envPath, cfgPath := codexFixture(t, "") - statePath := filepath.Join(filepath.Dir(envPath), "state.json") + f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: offCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + var out, errb bytes.Buffer - if code := codexEnable(envPath, cfgPath, statePath, true, &out, &errb); code != 0 { + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { t.Fatalf("enable exit %d: %s", code, errb.String()) } - writeCodexDaemonPID(t, filepath.Dir(envPath), 5613) - withCodexDaemonAlive(t, func(pid int) bool { return pid == 5613 }) + wantAll(t, "enable output", out.String(), "restarted (pid 7001), but its proxy is none, not Cortex's") +} + +// A restart that fails leaves the change made, so enable still exits 0, and stderr names +// the command that finishes the job. +func TestCodexEnable_RestartFailureIsReported(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{pid: 5613, env: offCortex, restartErr: errors.New("codex app-server daemon restart: exit status 1: busy")} + stubCodexDaemon(t, filepath.Dir(envPath), f) + + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d, want 0: the file was changed", code) + } + wantAll(t, "stderr", errb.String(), "could not restart", "busy", codexDaemonRestartCmd) + if readCodexEnv(t, envPath)["HTTPS_PROXY"] != cortexProxyURL { + t.Error("the file was not enabled") + } +} + +// A restarted daemon that exits, or whose environment cannot be read, is said as such +// rather than claimed to be on Cortex. +func TestCodexEnable_RestartedDaemonUnchecked(t *testing.T) { + for name, tc := range map[string]struct { + f fakeCodexDaemon + want string + }{ + "gone": {fakeCodexDaemon{pid: 5613, env: offCortex, newGone: true}, "restarted, but is not running now"}, + "unreadable": {fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001}, "restarted (pid 7001); its environment could not be checked"}, + } { + t.Run(name, func(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := tc.f + stubCodexDaemon(t, filepath.Dir(envPath), &f) + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d: %s", code, errb.String()) + } + wantAll(t, "enable output", out.String(), tc.want) + }) + } +} + +// Disable restarts the daemon too, the other way round: a daemon left on Cortex would +// stop working once Cortex stops. Cortex's config is not read, so the restart is given no +// want, and the proxy it is compared with is the one the file held. +func TestCodexDisable_RestartsARunningDaemon(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + enableCodex(t, envPath, cfgPath) + f := &fakeCodexDaemon{pid: 5613, env: onCortex, newPID: 7001, newEnv: offCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) - out.Reset() - if code := codexDisable(envPath, statePath, true, &out, &errb); code != 0 { + var out, errb bytes.Buffer + if code := codexDisable(envPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { t.Fatalf("disable exit %d: %s", code, errb.String()) } - got := out.String() - for _, want := range []string{"pid 5613", "still sends through Cortex", codexDaemonRestartCmd} { - if !strings.Contains(got, want) { - t.Errorf("disable output lacks %q:\n%s", want, got) - } + wantAll(t, "disable output", out.String(), "pid 5613", "Applying this restarts it", "restarted (pid 7001) and no longer uses Cortex") + if len(f.restarts) != 1 || f.restarts[0] != nil { + t.Errorf("restarts = %v, want one, given no want", f.restarts) } } -// Running enable again is what someone does when Codex does not seem to go through -// Cortex, and a daemon older than the file is the usual reason, so "already enabled" -// warns too. -func TestCodexEnable_AlreadyEnabledStillWarnsAboutADaemon(t *testing.T) { +// A daemon still on Cortex's proxy after disable got it from somewhere other than the +// file, and is told so. +func TestCodexDisable_SaysWhenTheRestartedDaemonStillUsesCortex(t *testing.T) { envPath, cfgPath := codexFixture(t, "") - statePath := filepath.Join(filepath.Dir(envPath), "state.json") + enableCodex(t, envPath, cfgPath) + f := &fakeCodexDaemon{pid: 5613, env: onCortex, newPID: 7001, newEnv: onCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + var out, errb bytes.Buffer - if code := codexEnable(envPath, cfgPath, statePath, true, &out, &errb); code != 0 { - t.Fatalf("enable exit %d: %s", code, errb.String()) + if code := codexDisable(envPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("disable exit %d: %s", code, errb.String()) } - writeCodexDaemonPID(t, filepath.Dir(envPath), 5613) - withCodexDaemonAlive(t, func(pid int) bool { return pid == 5613 }) - out.Reset() - if code := codexEnable(envPath, cfgPath, statePath, true, &out, &errb); code != 0 { - t.Fatalf("second enable exit %d: %s", code, errb.String()) + wantAll(t, "disable output", out.String(), "still uses Cortex's proxy") +} + +// Running enable again is what someone does when Codex does not seem to go through +// Cortex, and a daemon older than the file is the usual reason. With nothing to change it +// reports rather than restarts, and names the command. +func TestCodexEnable_AlreadyEnabledReportsTheDaemon(t *testing.T) { + for name, tc := range map[string]struct { + env []string + want string + }{ + "old settings": {offCortex, "running with its old settings"}, + "on Cortex": {onCortex, "(pid 5613) is using Cortex"}, + } { + t.Run(name, func(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + enableCodex(t, envPath, cfgPath) + f := &fakeCodexDaemon{pid: 5613, env: tc.env} + stubCodexDaemon(t, filepath.Dir(envPath), f) + + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("second enable exit %d: %s", code, errb.String()) + } + wantAll(t, "second enable output", out.String(), "Already enabled", tc.want) + if len(f.restarts) != 0 { + t.Errorf("an enable with nothing to change restarted the daemon") + } + }) } - if got := out.String(); !strings.Contains(got, "Already enabled") || !strings.Contains(got, codexDaemonRestartCmd) { - t.Errorf("second enable output = %q, want the already-enabled line and the daemon warning", got) +} + +// status says what the running daemon uses, since that, not the file, is what sends +// Codex's prompts. +func TestCodexStatus_ReportsTheDaemon(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + enableCodex(t, envPath, cfgPath) + stubCodexDaemon(t, filepath.Dir(envPath), &fakeCodexDaemon{pid: 5613, env: offCortex}) + + var out bytes.Buffer + if code := codexStatus(envPath, cfgPath, &out); code != 0 { + t.Fatalf("exit %d", code) } + wantAll(t, "status output", out.String(), "enabled in "+envPath, "running with its old settings", codexDaemonRestartCmd) } -// No daemon, or one whose pid file outlived it, needs no warning: the next codex starts -// a fresh daemon that reads the file enable just wrote. -func TestCodexEnable_NoWarningWithoutALiveDaemon(t *testing.T) { +// No daemon, or one whose pid file outlived it, needs nothing: the next codex starts a +// fresh daemon that reads the file enable just wrote. +func TestCodexEnable_NothingToRestartWithoutALiveDaemon(t *testing.T) { for name, setup := range map[string]func(t *testing.T, codexHome string){ "no pid file": func(*testing.T, string) {}, "stale pid file": func(t *testing.T, codexHome string) { @@ -112,20 +305,103 @@ func TestCodexEnable_NoWarningWithoutALiveDaemon(t *testing.T) { } { t.Run(name, func(t *testing.T) { envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{} // pid 0: nothing is alive + stubCodexDaemon(t, filepath.Dir(envPath), f) setup(t, filepath.Dir(envPath)) - withCodexDaemonAlive(t, func(int) bool { return false }) var out, errb bytes.Buffer if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { t.Fatalf("enable exit %d: %s", code, errb.String()) } if strings.Contains(out.String(), "app-server") { - t.Errorf("warned about a daemon that is not running:\n%s", out.String()) + t.Errorf("spoke of a daemon that is not running:\n%s", out.String()) + } + if len(f.restarts) != 0 { + t.Error("restarted a daemon that is not running") + } + }) + } +} + +// Codex's own reading of the proxy variables, which is not Bun's: uppercase first, an +// empty value is unset, and ALL_PROXY is the fallback. +func TestCodexEnvironProxy(t *testing.T) { + for _, tc := range []struct { + name string + env []string + want string + }{ + {"none", []string{"HOME=/h"}, ""}, + {"uppercase wins", []string{"https_proxy=http://lower:1", "HTTPS_PROXY=http://upper:2"}, "http://upper:2"}, + {"lowercase alone", []string{"https_proxy=http://lower:1"}, "http://lower:1"}, + {"empty uppercase hides lowercase", []string{"HTTPS_PROXY=", "https_proxy=http://lower:1"}, ""}, + {"ALL_PROXY fallback", []string{"ALL_PROXY=http://all:3"}, "http://all:3"}, + {"HTTP_PROXY is not for HTTPS", []string{"HTTP_PROXY=http://plain:4"}, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := codexEnvironProxy(tc.env); got != tc.want { + t.Errorf("codexEnvironProxy(%q) = %q, want %q", tc.env, got, tc.want) } }) } } +// The restart's environment loses Cortex's values and keeps everything else, a proxy of +// the user's own included, so a restart after disable cannot put the daemon back on +// Cortex from the shell it ran in. +func TestCodexCLIEnv(t *testing.T) { + env := []string{"HOME=/h", "HTTPS_PROXY=" + cortexProxyURL, "http_proxy=http://corp.example.com:3128", "SSL_CERT_FILE=/h/.cortex/ca/bundle.crt"} + got := strings.Join(codexCLIEnv(env, nil), " ") + if want := "HOME=/h http_proxy=http://corp.example.com:3128"; got != want { + t.Errorf("codexCLIEnv = %q, want %q", got, want) + } +} + +// The real runner, given /bin/sh rather than codex: a failure carries stderr's first +// line, the child gets no Cortex proxy, and CODEX_HOME names a home Codex would not find +// by itself. +func TestCodexRun(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses /bin/sh") + } + t.Setenv("HOME", t.TempDir()) + t.Setenv(codexHomeEnv, "") + t.Setenv("HTTPS_PROXY", cortexProxyURL) + home := t.TempDir() + err := codexRun("/bin/sh", home, nil, "-c", `echo "home=$CODEX_HOME proxy=$HTTPS_PROXY" >&2; echo second >&2; exit 3`) + if err == nil { + t.Fatal("a non-zero exit returned no error") + } + if msg := err.Error(); !strings.HasSuffix(msg, ": exit status 3: home="+home+" proxy=") { + t.Errorf("err = %q, want it to end with the exit status, CODEX_HOME and no proxy", msg) + } + if err := codexRun("/bin/sh", home, nil, "-c", "exit 0"); err != nil { + t.Errorf("a zero exit returned %v", err) + } +} + +// codex is found on PATH, else where its standalone installer puts it. +func TestFindCodex(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("unix permissions") + } + t.Setenv("PATH", t.TempDir()) + home := t.TempDir() + if _, err := findCodex(home); err == nil { + t.Error("found a codex that is not there") + } + bin := filepath.Join(home, "packages", "standalone", "current", "bin", "codex") + if err := os.MkdirAll(filepath.Dir(bin), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o700); err != nil { + t.Fatal(err) + } + if got, err := findCodex(home); err != nil || got != bin { + t.Errorf("findCodex = %q, %v; want %q", got, err, bin) + } +} + // The liveness check against the real process table: this process is alive, a pid no // process can have is not. func TestCodexDaemonAlive_RealProcesses(t *testing.T) { diff --git a/docs/agents/codex.md b/docs/agents/codex.md index f1b1254c3..ccc3541f7 100644 --- a/docs/agents/codex.md +++ b/docs/agents/codex.md @@ -11,7 +11,7 @@ confirmed against the real Codex CLI (writing a dead proxy address there and run fallback transport). There is no settings JSON, so the routing goes into that file directly. **Every Codex process reads it once, when it starts** — and that includes Codex's own background server, which outlives the windows you type into; see -[Restart Codex's background app-server](#restart-codexs-background-app-server). +[Codex's background app-server](#codexs-background-app-server). ## Enable and revert @@ -82,16 +82,17 @@ comparison is tracked by #1289. **What `status` says.** It prints the five variables as the file currently holds them, a warning if any is defined more than once, then `enabled` if every one matches what `enable` -would write right now, or `not fully enabled (N of 5 set)` otherwise. It changes nothing. +would write right now, or `not fully enabled (N of 5 set)` otherwise. Last, whether Codex's +running background app-server uses Cortex — see below. It changes nothing. `enable` and `disable` both show what they will change and ask before doing it. `--yes` skips the question. If you decline, or there is no terminal to ask on, they change nothing and exit 3. `--env PATH` points at a different dotenv file; `--config PATH` reads a different Cortex config. -### Restart Codex's background app-server +### Codex's background app-server -**Enabling Cortex does not reach a Codex that is already running.** Codex runs a +**The file alone does not reach a Codex that is already running.** Codex runs a long-lived background server, its app-server daemon (`codex app-server --managed-daemon`), and the `codex` you type into is only a screen that hands it each prompt: the daemon makes the model calls. It reads the dotenv file when it starts and never again, and it is shared @@ -101,23 +102,37 @@ prompts do not, and agentop shows Codex only as a small `pending:codex@…` buck **Other**, with no tokens. `disable` has the mirror problem: the daemon keeps routing through Cortex, and Codex stops working if Cortex is then stopped. -`enable` and `disable` say so when they find a running daemon, naming its pid and when it -started. To apply the change, restart it: +**So `enable` and `disable` restart a running daemon**, once, after writing the file, with +Codex's own `codex app-server daemon restart`. They say so before asking, naming its pid and +when it started, and `--yes` skips the question but not that line. Then they check the new +daemon's environment and say whether it uses Cortex — or, if its proxy is still not what the +file says, where else it could be coming from. With no daemon running there is nothing to +restart: the next `codex` starts one that reads the new file. An `enable` that finds nothing +to change does not restart; it says whether the running daemon uses Cortex, and how to +restart it if not. + +**What a restart does to Codex sessions**, from Codex's source (0.163.0 and 0.164.0), not +measured here: the old daemon stops taking new turns, gives running ones its shutdown grace — 60 +seconds by default — to finish, and saves its threads. The new daemon restores them and +continues a local turn the grace cut off, saying "Resuming interrupted work". An open +`codex` window shows that it is reconnecting and rejoins its thread on its own. So a restart +interrupts Codex rather than ending it, and `enable` or `disable` can take up to that minute +when a turn is running. + +If the restart fails, the file is still changed and the command still exits 0; it prints the +error and the command to run yourself: ```sh -codex app-server daemon restart # ends any Codex session running through it +codex app-server daemon restart ``` -Then start `codex` again. To confirm the daemon now goes through Cortex, its connections -should be to `127.0.0.1:47600` rather than to OpenAI: +To check the daemon by hand, its connections should be to `127.0.0.1:47600` rather than to +OpenAI: ```sh lsof -nP -a -p "$(pgrep -f 'codex app-server --listen')" -iTCP -sTCP:ESTABLISHED ``` -`agentop` does not restart the daemon itself, because that would end Codex work running in -any other terminal or editor. - ### Undoing it by hand Without `agentop`: open `$CODEX_HOME/.env` (or `~/.codex/.env`) and remove the lines for the five variables above, @@ -382,12 +397,12 @@ Not tested: ## Known issues -- **A running Codex keeps its old routing until its background server restarts.** - `enable` and `disable` change a file Codex's app-server daemon reads only when it - starts, so the change does not reach a daemon already running — prompts keep bypassing - Cortex after an enable, and keep needing it after a disable. Both commands warn when - they find a live daemon; `codex app-server daemon restart` applies the change. See - [Restart Codex's background app-server](#restart-codexs-background-app-server). +- **Changing the routing restarts Codex's background server.** `enable` and `disable` + change a file Codex's app-server daemon reads only when it starts, so they restart a + daemon that is running, which interrupts running Codex turns for up to a minute. Changing + the file any other way — by hand, or with an agentop older than this — does not reach a + running daemon until `codex app-server daemon restart`. See + [Codex's background app-server](#codexs-background-app-server). - **Cost is blank where no list price reaches the model.** Two cases, and neither is Codex-specific: a **sidecar** keeps the compiled-in table and makes no download, so a model newer than the pinned commit has no rate there; and `bundled: false` turns the From eed72126f606450ca36e2b9c6431aea45ff60102 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Wed, 28 Oct 2026 11:52:41 -0400 Subject: [PATCH 2/3] fix: Restart Codex's auto-updater with its app-server An auto-updatable Codex runs a second background process, its updater (`codex app-server daemon pid-update-loop`). Like the app-server, it reads ~/.codex/.env only when it starts, and the app-server it starts after an update inherits its environment. `daemon restart` keeps a running updater, so after disable the app-server was clean but the updater still had Cortex's proxy, and the next Codex update put the app-server back on Cortex (reproduced in review on 0.164.0 -> 0.165.0). enable and disable now stop the updater named in daemon-updater.pid before restarting, as Codex stops it (SIGTERM to its process group), so the restart starts a fresh one. After the restart, an updater whose proxy still disagrees with the change is named with the command that replaces it. Also from review: - Run inside a Codex turn (CODEX_THREAD_ID set), restart nothing: the restart would wait for the turn that is waiting on agentop. Print the command to run from a terminal instead. - Say the drain is up to the shutdown grace, a minute by default, rather than "up to a minute". - Discard the restart command's stdout instead of piping it, and do not count ErrWaitDelay (Go returns it only after a successful exit) as a failed restart, so a daemon holding an inherited pipe open cannot turn a successful restart into a reported failure. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- cmd/agentop/cmd_codex.go | 14 +- cmd/agentop/codex_daemon.go | 119 +++++++++++++-- cmd/agentop/codex_daemon_test.go | 240 +++++++++++++++++++++++++++++-- docs/agents/codex.md | 23 ++- 4 files changed, 363 insertions(+), 33 deletions(-) diff --git a/cmd/agentop/cmd_codex.go b/cmd/agentop/cmd_codex.go index d58bd166f..3bb65ae57 100644 --- a/cmd/agentop/cmd_codex.go +++ b/cmd/agentop/cmd_codex.go @@ -166,12 +166,14 @@ someone set deliberately. The first run copies the original file to .env.bak and never overwrites that copy. disable removes only the keys it added. Codex's background app-server, which makes the model calls for every codex -window, reads the file only when it starts. So when it is running, enable and -disable restart it once after their change (codex app-server daemon restart): -turns already running get up to its shutdown grace, a minute by default, to -finish, and open Codex windows reconnect. When it is running they say so first -and ask (--yes skips the question, not the warning). status shows whether the -running app-server uses Cortex. +window, reads the file only when it starts, and so does its auto-updater. So +when the app-server is running, enable and disable stop the updater and restart +the app-server once after their change (codex app-server daemon restart, which +starts a fresh updater): turns already running get up to its shutdown grace, a +minute by default, to finish, and open Codex windows reconnect. When it is +running they say so first and ask (--yes skips the question, not the warning). +Run from inside a Codex turn they restart nothing and print the command to run +from a terminal. status shows whether the running app-server uses Cortex. Note: while enabled, Codex needs Cortex running — its requests go to the proxy address. "agentop configure codex disable" is the off switch. diff --git a/cmd/agentop/codex_daemon.go b/cmd/agentop/codex_daemon.go index 68bad4d13..ef0fe6fc7 100644 --- a/cmd/agentop/codex_daemon.go +++ b/cmd/agentop/codex_daemon.go @@ -37,6 +37,25 @@ import ( // continues a local turn the grace cut off; an open codex window reconnects on its own. const codexDaemonPIDRel = "app-server-daemon/daemon.pid" +// codexUpdaterPIDRel is where Codex records its auto-updater, the second long-lived +// process an auto-updatable install runs (`codex app-server daemon pid-update-loop`). +// +// IT IS WHY RESTARTING THE DAEMON ALONE IS NOT ENOUGH. The updater also reads the dotenv +// file only when it starts, and the daemon it starts after installing an update inherits +// the updater's environment. `daemon restart` replaces the daemon but keeps a running +// updater: Codex starts one only when its pid file names no live process. So after +// disable the daemon was clean and the updater still had Cortex's proxy, and the next +// update put the daemon back on Cortex (reproduced on 0.164.0 updating to 0.165.0). +// Enable does not need it, since a daemon's own dotenv load adds a proxy it did not +// inherit; it is stopped on both so the two processes never disagree. Stopping it first +// lets the restart start a fresh one, which reads the new file. +const codexUpdaterPIDRel = "app-server-daemon/daemon-updater.pid" + +// codexThreadIDEnv is set by Codex in the environment of every command it runs for a +// turn. Seeing it means agentop is a tool call inside Codex, and that Codex turn is one +// the restart would have to drain. +const codexThreadIDEnv = "CODEX_THREAD_ID" + // codexDaemonRestartCmd is Codex's own command for applying a changed environment. const codexDaemonRestartCmd = "codex app-server daemon restart" @@ -83,11 +102,48 @@ var codexRestartDaemon = func(codexHome string, want map[string]string) error { return codexRun(bin, codexHome, want, "app-server", "daemon", "restart") } +// codexStopUpdater ends the updater recorded under codexHome, if it runs, and waits for it +// to exit. Codex stops its updater the same way: SIGTERM to its process group, which the +// updater handles by shutting down. A package variable so tests replace it. +var codexStopUpdater = func(codexHome string) error { + u, ok := runningCodexProc(codexHome, codexUpdaterPIDRel) + if !ok { + return nil + } + if err := syscall.Kill(-u.PID, syscall.SIGTERM); err != nil && !errors.Is(err, syscall.ESRCH) { + return fmt.Errorf("stopping Codex's updater (pid %d): %w", u.PID, err) + } + deadline := time.Now().Add(codexUpdaterStopTimeout) + for codexDaemonAlive(u.PID) { + if time.Now().After(deadline) { + return fmt.Errorf("Codex's updater (pid %d) did not exit within %s", u.PID, codexUpdaterStopTimeout) + } + time.Sleep(50 * time.Millisecond) + } + return nil +} + +// codexUpdaterStopTimeout bounds the wait for the updater to exit. It has no turns to +// drain, so it exits at once unless it is mid-install; a timeout leaves the restart to go +// ahead and the check after it to name the updater if it kept its old settings. +const codexUpdaterStopTimeout = 10 * time.Second + // runningCodexDaemon returns the daemon recorded under codexHome when its process is // still alive. ok is false when there is no pid file, it cannot be read, or the process // it names has exited — a stale file Codex leaves behind after a crash or a reboot. func runningCodexDaemon(codexHome string) (codexDaemon, bool) { - b, err := os.ReadFile(filepath.Join(codexHome, codexDaemonPIDRel)) + return runningCodexProc(codexHome, codexDaemonPIDRel) +} + +// runningCodexUpdater is runningCodexDaemon for the auto-updater. +func runningCodexUpdater(codexHome string) (codexDaemon, bool) { + return runningCodexProc(codexHome, codexUpdaterPIDRel) +} + +// runningCodexProc reads one of Codex's pid files under codexHome, and the environment of +// the process it names when that process is alive. +func runningCodexProc(codexHome, pidRel string) (codexDaemon, bool) { + b, err := os.ReadFile(filepath.Join(codexHome, pidRel)) if err != nil { return codexDaemon{}, false } @@ -169,12 +225,15 @@ func codexRun(bin, codexHome string, want map[string]string, args ...string) err env = append(env, codexHomeEnv+"="+codexHome) } cmd.Env = env + // Only stderr is read. stdout goes nowhere rather than to a pipe, so a daemon that kept + // an inherited stdout open could not hold Wait. stderr is a pipe and might be held the + // same way: WaitDelay bounds that, and ErrWaitDelay, which Go returns only after a + // successful exit, is not a failed restart. var stderr bytes.Buffer + cmd.Stdout = io.Discard cmd.Stderr = &stderr - // The daemon it starts detaches with its own stdio, but bound how long Wait waits for - // the pipes anyway, as openCodeRun does. cmd.WaitDelay = time.Second - if _, err := cmd.Output(); err != nil { + if err := cmd.Run(); err != nil && !errors.Is(err, exec.ErrWaitDelay) { what := filepath.Base(bin) + " " + strings.Join(args, " ") if ctx.Err() != nil { return fmt.Errorf("%s: no answer in %s", what, codexRestartTimeout) @@ -219,8 +278,8 @@ func noteCodexDaemonRestart(codexHome string, stdout io.Writer) *codexDaemon { return nil } fmt.Fprintf(stdout, "Codex's background app-server is running (pid %d%s).\n"+ - " Applying this restarts it: Codex turns already running get up to a minute to finish,\n"+ - " and open Codex windows reconnect.\n", + " Applying this restarts it: Codex turns already running get up to its shutdown grace\n"+ + " (a minute by default) to finish, and open Codex windows reconnect.\n", d.PID, startedSuffix(d)) return &d } @@ -235,12 +294,28 @@ func startedSuffix(d codexDaemon) string { // finishCodexChange restarts the daemon that was running before enable (configured) or // disable changed the file, then checks it again and says whether its proxy now matches // the change: cortexProxy after enable, anything but cortexProxy after disable. was is -// what noteCodexDaemonRestart returned; nil leaves everything alone. A restart that fails -// is reported on stderr; the change itself succeeded, so the caller still exits 0. +// what noteCodexDaemonRestart returned; nil leaves everything alone. The auto-updater is +// stopped first, so the restart starts a fresh one (see codexUpdaterPIDRel), and checked +// after. A restart that fails is reported on stderr; the change itself succeeded, so the +// caller still exits 0. Inside a Codex turn nothing is restarted (insideCodexTurn). func finishCodexChange(codexHome, cortexProxy string, want map[string]string, configured bool, was *codexDaemon, stdout, stderr io.Writer) { if was == nil { return } + if insideCodexTurn() { + // The restart would drain the turn waiting on this command: it blocks for the + // whole grace, and the forced stop at its end can take this command down with it, + // leaving no daemon. The file is changed; the restart is left to a terminal. + fmt.Fprintf(stdout, "Not restarting Codex's background app-server (pid %d): this is running inside a Codex turn,\n"+ + " which the restart would have to wait for. Run this from a terminal outside Codex:\n"+ + " %s\n", was.PID, codexDaemonRestartCmd) + return + } + if err := codexStopUpdater(codexHome); err != nil { + // Not fatal: the restart still applies the change to the daemon, and the check + // below names an updater still on the old settings. + fmt.Fprintf(stderr, "agentop: %v\n", err) + } fmt.Fprintf(stdout, "Restarting Codex's background app-server (pid %d)...\n", was.PID) if err := codexRestartDaemon(codexHome, want); err != nil { fmt.Fprintf(stderr, "agentop: could not restart Codex's background app-server (%v).\n"+ @@ -248,6 +323,34 @@ func finishCodexChange(codexHome, cortexProxy string, want map[string]string, co " %s\n", err, codexDaemonRestartCmd) return } + reportRestartedDaemon(codexHome, cortexProxy, configured, stdout) + reportRestartedUpdater(codexHome, cortexProxy, configured, stdout) +} + +// insideCodexTurn reports whether this process runs as a tool call of a Codex turn. +func insideCodexTurn() bool { + return strings.TrimSpace(os.Getenv(codexThreadIDEnv)) != "" +} + +// reportRestartedUpdater names an updater whose proxy disagrees with the change, since +// the daemon it starts after its next update inherits that proxy. No updater, one whose +// environment cannot be read, or one that agrees, needs no line. +func reportRestartedUpdater(codexHome, cortexProxy string, configured bool, stdout io.Writer) { + u, ok := runningCodexUpdater(codexHome) + if !ok || u.EnvErr != nil || cortexProxy == "" || sameProxy(u.Proxy, cortexProxy) == configured { + return + } + stale := "still uses Cortex's proxy" + if configured { + stale = "does not use Cortex's proxy" + } + fmt.Fprintf(stdout, "Codex's auto-updater (pid %d) %s, and the app-server it starts after an update\n"+ + " inherits that. Stop it, then restart the app-server so a new updater starts:\n"+ + " kill %d && %s\n", u.PID, stale, u.PID, codexDaemonRestartCmd) +} + +// reportRestartedDaemon says whether the restarted daemon's proxy matches the change. +func reportRestartedDaemon(codexHome, cortexProxy string, configured bool, stdout io.Writer) { d, ok := runningCodexDaemon(codexHome) switch { case !ok: diff --git a/cmd/agentop/codex_daemon_test.go b/cmd/agentop/codex_daemon_test.go index 93134ef99..1838234af 100644 --- a/cmd/agentop/codex_daemon_test.go +++ b/cmd/agentop/codex_daemon_test.go @@ -5,18 +5,24 @@ import ( "errors" "io" "os" + "os/exec" "path/filepath" "runtime" "strconv" "strings" + "syscall" "testing" + "time" ) const cortexProxyURL = "http://127.0.0.1:47600" // the forward proxy in cortexCfg -// fakeCodexDaemon is the daemon a test sees: a pid file under its CODEX_HOME, a process -// table that says that pid is alive, and an environment for it. restart, when not -// failing, replaces it with newPID running with newEnv, as Codex's restart does. +// fakeCodexDaemon is the Codex a test sees: a daemon and, when updaterPID is set, an +// auto-updater, each with a pid file under its CODEX_HOME, a process table that says the +// pid is alive, and an environment. restart, when not failing, replaces the daemon with +// newPID running with newEnv and, as Codex's restart does, starts an updater only when +// none is running — a running one keeps its pid and its old environment. stopUpdater ends +// the updater unless stopErr says it refused. type fakeCodexDaemon struct { pid int env []string @@ -27,31 +33,59 @@ type fakeCodexDaemon struct { newGone bool // the restarted daemon exits straight away restartErr error // the restart command fails + updaterPID int + updaterEnv []string + newUpdaterPID int // the updater a restart starts when none is running + stopErr error // stopping the updater fails, and it keeps running + restarts []map[string]string // the want each restart was given + steps []string // "stop-updater" and "restart", in the order they ran } // stubCodexDaemon points every daemon seam at f for one test, so nothing a test does can -// restart a real daemon or read a real process: the pid is fake, and a real process with -// that pid would otherwise be the one inspected. +// restart a real daemon, stop a real updater or read a real process: the pids are fake, +// and a real process with one of them would otherwise be the one inspected. CODEX_THREAD_ID +// is cleared, so a test run from inside a Codex turn behaves like one run from a terminal. func stubCodexDaemon(t *testing.T, codexHome string, f *fakeCodexDaemon) { t.Helper() - alive, environ, restart := codexDaemonAlive, codexEnviron, codexRestartDaemon - t.Cleanup(func() { codexDaemonAlive, codexEnviron, codexRestartDaemon = alive, environ, restart }) + t.Setenv(codexThreadIDEnv, "") + alive, environ, restart, stop := codexDaemonAlive, codexEnviron, codexRestartDaemon, codexStopUpdater + t.Cleanup(func() { + codexDaemonAlive, codexEnviron, codexRestartDaemon, codexStopUpdater = alive, environ, restart, stop + }) if f.pid != 0 { writeCodexDaemonPID(t, codexHome, f.pid) } - codexDaemonAlive = func(pid int) bool { return pid == f.pid } + if f.updaterPID != 0 { + writeCodexPID(t, codexHome, codexUpdaterPIDRel, f.updaterPID) + } + codexDaemonAlive = func(pid int) bool { return pid != 0 && (pid == f.pid || pid == f.updaterPID) } codexEnviron = func(pid int32) ([]string, error) { - if int(pid) != f.pid { - t.Errorf("read the environment of pid %d, which is not the fake daemon's", pid) - return nil, errors.New("not the fake daemon") + switch int(pid) { + case f.pid: + return f.env, f.envErr + case f.updaterPID: + return f.updaterEnv, nil + } + t.Errorf("read the environment of pid %d, which is neither fake process", pid) + return nil, errors.New("not a fake process") + } + codexStopUpdater = func(home string) error { + if home != codexHome { + t.Errorf("stopped the updater of %s, want %s", home, codexHome) } - return f.env, f.envErr + f.steps = append(f.steps, "stop-updater") + if f.stopErr != nil { + return f.stopErr + } + f.updaterPID = 0 + return nil } codexRestartDaemon = func(home string, want map[string]string) error { if home != codexHome { t.Errorf("restarted the daemon of %s, want %s", home, codexHome) } + f.steps = append(f.steps, "restart") f.restarts = append(f.restarts, want) if f.restartErr != nil { return f.restartErr @@ -62,6 +96,11 @@ func stubCodexDaemon(t *testing.T, codexHome string, f *fakeCodexDaemon) { } else { writeCodexDaemonPID(t, codexHome, f.newPID) } + if f.updaterPID == 0 && f.newUpdaterPID != 0 { + // A fresh updater reads the file the change just wrote, as the daemon does. + f.updaterPID, f.updaterEnv = f.newUpdaterPID, f.newEnv + writeCodexPID(t, codexHome, codexUpdaterPIDRel, f.newUpdaterPID) + } return nil } } @@ -69,12 +108,18 @@ func stubCodexDaemon(t *testing.T, codexHome string, f *fakeCodexDaemon) { // writeCodexDaemonPID writes a daemon.pid under codexHome as Codex does. func writeCodexDaemonPID(t *testing.T, codexHome string, pid int) { t.Helper() - dir := filepath.Join(codexHome, filepath.Dir(codexDaemonPIDRel)) + writeCodexPID(t, codexHome, codexDaemonPIDRel, pid) +} + +// writeCodexPID writes one of Codex's pid files, rel under codexHome. +func writeCodexPID(t *testing.T, codexHome, rel string, pid int) { + t.Helper() + dir := filepath.Join(codexHome, filepath.Dir(rel)) if err := os.MkdirAll(dir, 0o700); err != nil { t.Fatal(err) } body := `{"pid":` + strconv.Itoa(pid) + `,"processStartTime":"Fri Oct 23 09:03:25 2026"}` - if err := os.WriteFile(filepath.Join(codexHome, codexDaemonPIDRel), []byte(body), 0o600); err != nil { + if err := os.WriteFile(filepath.Join(codexHome, rel), []byte(body), 0o600); err != nil { t.Fatal(err) } } @@ -123,7 +168,8 @@ func TestCodexEnable_RestartsARunningDaemon(t *testing.T) { } got := out.String() wantAll(t, "enable output", got, "pid 5613", "Fri Oct 23 09:03:25 2026", "Applying this restarts it", - "open Codex windows reconnect", "restarted (pid 7001) and is using Cortex") + "up to its shutdown grace", "a minute by default", "open Codex windows reconnect", + "restarted (pid 7001) and is using Cortex") if len(f.restarts) != 1 { t.Fatalf("restarted %d times, want once", len(f.restarts)) } @@ -139,6 +185,107 @@ func TestCodexEnable_RestartsARunningDaemon(t *testing.T) { } } +// Disable stops a running auto-updater before restarting, so the restart starts a fresh +// one that read the new file. Without that the old updater kept Cortex's proxy, and the +// daemon it started after the next update was back on Cortex (#1526 review, reproduced +// on Codex 0.164.0 → 0.165.0). +func TestCodexDisable_StopsTheUpdaterBeforeRestarting(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + enableCodex(t, envPath, cfgPath) + f := &fakeCodexDaemon{ + pid: 5613, env: onCortex, newPID: 7001, newEnv: offCortex, + updaterPID: 5600, updaterEnv: onCortex, newUpdaterPID: 7002, + } + stubCodexDaemon(t, filepath.Dir(envPath), f) + + var out, errb bytes.Buffer + if code := codexDisable(envPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("disable exit %d: %s", code, errb.String()) + } + if got := strings.Join(f.steps, ","); got != "stop-updater,restart" { + t.Errorf("steps = %s, want the updater stopped before the restart", got) + } + if f.updaterPID != 7002 { + t.Errorf("updater pid = %d, want the fresh one the restart started", f.updaterPID) + } + if strings.Contains(out.String(), "auto-updater") { + t.Errorf("named the fresh updater, which agrees with the change:\n%s", out.String()) + } +} + +// An updater that refuses to stop keeps its old environment through the restart, and is +// named with the command that fixes it rather than reported as fixed. +func TestCodexDisable_NamesAnUpdaterThatKeptCortex(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + enableCodex(t, envPath, cfgPath) + f := &fakeCodexDaemon{ + pid: 5613, env: onCortex, newPID: 7001, newEnv: offCortex, + updaterPID: 5600, updaterEnv: onCortex, stopErr: errors.New("Codex's updater (pid 5600) did not exit within 10s"), + } + stubCodexDaemon(t, filepath.Dir(envPath), f) + + var out, errb bytes.Buffer + if code := codexDisable(envPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("disable exit %d: %s", code, errb.String()) + } + wantAll(t, "stderr", errb.String(), "did not exit") + wantAll(t, "disable output", out.String(), "no longer uses Cortex", + "auto-updater (pid 5600) still uses Cortex's proxy", "kill 5600 && "+codexDaemonRestartCmd) +} + +// Enable stops the updater too, so the two processes never disagree, and an install with +// no updater (auto-update off) stops nothing and names nothing. +func TestCodexEnable_UpdaterHandling(t *testing.T) { + t.Run("running updater is replaced", func(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{ + pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex, + updaterPID: 5600, updaterEnv: offCortex, newUpdaterPID: 7002, + } + stubCodexDaemon(t, filepath.Dir(envPath), f) + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d: %s", code, errb.String()) + } + if got := strings.Join(f.steps, ","); got != "stop-updater,restart" || f.updaterPID != 7002 { + t.Errorf("steps = %s, updater = %d; want stop-updater,restart and the fresh updater", got, f.updaterPID) + } + }) + t.Run("no updater", func(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d: %s", code, errb.String()) + } + if strings.Contains(out.String(), "auto-updater") || errb.Len() != 0 { + t.Errorf("spoke of an updater that is not running:\nstdout %s\nstderr %s", out.String(), errb.String()) + } + }) +} + +// Run as a tool call inside a Codex turn, enable changes the file but restarts nothing: +// the restart would wait for the very turn waiting on it. It says what to run instead. +func TestCodexEnable_InsideACodexTurnDoesNotRestart(t *testing.T) { + envPath, cfgPath := codexFixture(t, "") + f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex, updaterPID: 5600, updaterEnv: offCortex} + stubCodexDaemon(t, filepath.Dir(envPath), f) + t.Setenv(codexThreadIDEnv, "019a0000-0000-7000-8000-000000000000") + + var out, errb bytes.Buffer + if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { + t.Fatalf("enable exit %d: %s", code, errb.String()) + } + if len(f.steps) != 0 { + t.Errorf("steps = %v, want nothing stopped or restarted from inside a Codex turn", f.steps) + } + wantAll(t, "enable output", out.String(), "Enabled —", "inside a Codex turn", "outside Codex", codexDaemonRestartCmd) + if readCodexEnv(t, envPath)["HTTPS_PROXY"] != cortexProxyURL { + t.Error("the file was not enabled") + } +} + // Declining changes nothing, so nothing is restarted, though the warning was shown. func TestCodexEnable_DeclinedRestartsNothing(t *testing.T) { envPath, cfgPath := codexFixture(t, "") @@ -378,6 +525,69 @@ func TestCodexRun(t *testing.T) { if err := codexRun("/bin/sh", home, nil, "-c", "exit 0"); err != nil { t.Errorf("a zero exit returned %v", err) } + // A restart leaves a daemon running, and a daemon that kept the command's stdout or + // stderr open must not turn a successful restart into a failure (#1526 review). The + // background sleep holds both for longer than WaitDelay. + if err := codexRun("/bin/sh", home, nil, "-c", "sleep 5 & exit 0"); err != nil { + t.Errorf("a successful command whose child kept its output open returned %v", err) + } + // The same child behind a failed command is still a failure. + if err := codexRun("/bin/sh", home, nil, "-c", "sleep 5 & exit 4"); err == nil || !strings.Contains(err.Error(), "exit status 4") { + t.Errorf("a failed command whose child kept its output open returned %v, want exit status 4", err) + } +} + +// The real updater stop, against a process this test starts as its own process-group +// leader, as Codex starts its updater: SIGTERM to the group, then a wait for it to exit. +// An updater that is not running, or a stale pid file, needs nothing. +func TestCodexStopUpdater_RealProcess(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("process groups") + } + home := t.TempDir() + if err := codexStopUpdater(home); err != nil { + t.Errorf("no pid file: %v", err) + } + cmd := exec.Command("/bin/sh", "-c", "sleep 30") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + exited := make(chan struct{}) + go func() { _ = cmd.Wait(); close(exited) }() + t.Cleanup(func() { _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); <-exited }) + writeCodexPID(t, home, codexUpdaterPIDRel, cmd.Process.Pid) + + // codexDaemonAlive counts a zombie as alive, and only Wait reaps this child, so + // liveness here is whether Wait has returned. + prev := codexDaemonAlive + t.Cleanup(func() { codexDaemonAlive = prev }) + codexDaemonAlive = func(pid int) bool { + if pid != cmd.Process.Pid { + return false + } + select { + case <-exited: + return false + default: + return true + } + } + start := time.Now() + if err := codexStopUpdater(home); err != nil { + t.Fatalf("stopping a live updater: %v", err) + } + select { + case <-exited: + default: + t.Fatal("codexStopUpdater returned with the updater still running") + } + if d := time.Since(start); d > 5*time.Second { + t.Errorf("stop took %s; SIGTERM should end sleep at once", d) + } + if err := codexStopUpdater(home); err != nil { + t.Errorf("stale pid file: %v", err) + } } // codex is found on PATH, else where its standalone installer puts it. diff --git a/docs/agents/codex.md b/docs/agents/codex.md index ccc3541f7..e076b8f6c 100644 --- a/docs/agents/codex.md +++ b/docs/agents/codex.md @@ -111,12 +111,26 @@ restart: the next `codex` starts one that reads the new file. An `enable` that f to change does not restart; it says whether the running daemon uses Cortex, and how to restart it if not. +**The auto-updater is restarted too.** An auto-updatable install runs a second background +process, `codex app-server daemon pid-update-loop`, which also reads the dotenv file only when +it starts. After installing an update it starts a new daemon, and that daemon inherits the +updater's environment, but `codex app-server daemon restart` leaves a running updater alone. +So `enable` and `disable` stop the updater first, and the restart starts a fresh one that has +read the new file. Comparing `ps` before and after, both processes change. Without this, a +`disable` came undone at the next Codex update, with the daemon back on Cortex's proxy. If the +updater will not stop, they name it and the command that replaces it. + +**Not from inside Codex.** Run as a command inside a Codex turn — asking Codex to run `agentop +configure codex enable` — they change the file but do not restart anything: the restart would +wait for the very turn that is waiting for it. They print the restart command to run from a +terminal instead. Codex marks such commands with `CODEX_THREAD_ID`, which is how they tell. + **What a restart does to Codex sessions**, from Codex's source (0.163.0 and 0.164.0), not measured here: the old daemon stops taking new turns, gives running ones its shutdown grace — 60 -seconds by default — to finish, and saves its threads. The new daemon restores them and +seconds by default, configurable up to 5 minutes — to finish, and saves its threads. The new daemon restores them and continues a local turn the grace cut off, saying "Resuming interrupted work". An open `codex` window shows that it is reconnecting and rejoins its thread on its own. So a restart -interrupts Codex rather than ending it, and `enable` or `disable` can take up to that minute +interrupts Codex rather than ending it, and `enable` or `disable` can take as long as that grace when a turn is running. If the restart fails, the file is still changed and the command still exits 0; it prints the @@ -398,8 +412,9 @@ Not tested: ## Known issues - **Changing the routing restarts Codex's background server.** `enable` and `disable` - change a file Codex's app-server daemon reads only when it starts, so they restart a - daemon that is running, which interrupts running Codex turns for up to a minute. Changing + change a file Codex's app-server daemon and auto-updater read only when they start, so + they restart both when they are running, which interrupts running Codex turns for up to + the daemon's shutdown grace (a minute by default). Changing the file any other way — by hand, or with an agentop older than this — does not reach a running daemon until `codex app-server daemon restart`. See [Codex's background app-server](#codexs-background-app-server). From 2a4a82659360a812a55ef74b6e99c78c99b9a472 Mon Sep 17 00:00:00 2001 From: Alan Cha Date: Wed, 28 Oct 2026 12:53:37 -0400 Subject: [PATCH 3/3] fix: Leave the run-from-inside-Codex case to #1530 The CODEX_THREAD_ID check skipped the restart when agentop was run as a Codex tool call. OpenCode's restart has the same exposure and no such check, so the two are handled together in #1530 instead of Codex alone here. Assisted-By: Claude (Anthropic AI) Signed-off-by: Alan Cha --- cmd/agentop/cmd_codex.go | 3 +-- cmd/agentop/codex_daemon.go | 21 +-------------------- cmd/agentop/codex_daemon_test.go | 25 +------------------------ docs/agents/codex.md | 5 ----- 4 files changed, 3 insertions(+), 51 deletions(-) diff --git a/cmd/agentop/cmd_codex.go b/cmd/agentop/cmd_codex.go index 3bb65ae57..24e084189 100644 --- a/cmd/agentop/cmd_codex.go +++ b/cmd/agentop/cmd_codex.go @@ -172,8 +172,7 @@ the app-server once after their change (codex app-server daemon restart, which starts a fresh updater): turns already running get up to its shutdown grace, a minute by default, to finish, and open Codex windows reconnect. When it is running they say so first and ask (--yes skips the question, not the warning). -Run from inside a Codex turn they restart nothing and print the command to run -from a terminal. status shows whether the running app-server uses Cortex. +status shows whether the running app-server uses Cortex. Note: while enabled, Codex needs Cortex running — its requests go to the proxy address. "agentop configure codex disable" is the off switch. diff --git a/cmd/agentop/codex_daemon.go b/cmd/agentop/codex_daemon.go index ef0fe6fc7..7b300a25a 100644 --- a/cmd/agentop/codex_daemon.go +++ b/cmd/agentop/codex_daemon.go @@ -51,11 +51,6 @@ const codexDaemonPIDRel = "app-server-daemon/daemon.pid" // lets the restart start a fresh one, which reads the new file. const codexUpdaterPIDRel = "app-server-daemon/daemon-updater.pid" -// codexThreadIDEnv is set by Codex in the environment of every command it runs for a -// turn. Seeing it means agentop is a tool call inside Codex, and that Codex turn is one -// the restart would have to drain. -const codexThreadIDEnv = "CODEX_THREAD_ID" - // codexDaemonRestartCmd is Codex's own command for applying a changed environment. const codexDaemonRestartCmd = "codex app-server daemon restart" @@ -297,20 +292,11 @@ func startedSuffix(d codexDaemon) string { // what noteCodexDaemonRestart returned; nil leaves everything alone. The auto-updater is // stopped first, so the restart starts a fresh one (see codexUpdaterPIDRel), and checked // after. A restart that fails is reported on stderr; the change itself succeeded, so the -// caller still exits 0. Inside a Codex turn nothing is restarted (insideCodexTurn). +// caller still exits 0. func finishCodexChange(codexHome, cortexProxy string, want map[string]string, configured bool, was *codexDaemon, stdout, stderr io.Writer) { if was == nil { return } - if insideCodexTurn() { - // The restart would drain the turn waiting on this command: it blocks for the - // whole grace, and the forced stop at its end can take this command down with it, - // leaving no daemon. The file is changed; the restart is left to a terminal. - fmt.Fprintf(stdout, "Not restarting Codex's background app-server (pid %d): this is running inside a Codex turn,\n"+ - " which the restart would have to wait for. Run this from a terminal outside Codex:\n"+ - " %s\n", was.PID, codexDaemonRestartCmd) - return - } if err := codexStopUpdater(codexHome); err != nil { // Not fatal: the restart still applies the change to the daemon, and the check // below names an updater still on the old settings. @@ -327,11 +313,6 @@ func finishCodexChange(codexHome, cortexProxy string, want map[string]string, co reportRestartedUpdater(codexHome, cortexProxy, configured, stdout) } -// insideCodexTurn reports whether this process runs as a tool call of a Codex turn. -func insideCodexTurn() bool { - return strings.TrimSpace(os.Getenv(codexThreadIDEnv)) != "" -} - // reportRestartedUpdater names an updater whose proxy disagrees with the change, since // the daemon it starts after its next update inherits that proxy. No updater, one whose // environment cannot be read, or one that agrees, needs no line. diff --git a/cmd/agentop/codex_daemon_test.go b/cmd/agentop/codex_daemon_test.go index 1838234af..82a185399 100644 --- a/cmd/agentop/codex_daemon_test.go +++ b/cmd/agentop/codex_daemon_test.go @@ -44,11 +44,9 @@ type fakeCodexDaemon struct { // stubCodexDaemon points every daemon seam at f for one test, so nothing a test does can // restart a real daemon, stop a real updater or read a real process: the pids are fake, -// and a real process with one of them would otherwise be the one inspected. CODEX_THREAD_ID -// is cleared, so a test run from inside a Codex turn behaves like one run from a terminal. +// and a real process with one of them would otherwise be the one inspected. func stubCodexDaemon(t *testing.T, codexHome string, f *fakeCodexDaemon) { t.Helper() - t.Setenv(codexThreadIDEnv, "") alive, environ, restart, stop := codexDaemonAlive, codexEnviron, codexRestartDaemon, codexStopUpdater t.Cleanup(func() { codexDaemonAlive, codexEnviron, codexRestartDaemon, codexStopUpdater = alive, environ, restart, stop @@ -265,27 +263,6 @@ func TestCodexEnable_UpdaterHandling(t *testing.T) { }) } -// Run as a tool call inside a Codex turn, enable changes the file but restarts nothing: -// the restart would wait for the very turn waiting on it. It says what to run instead. -func TestCodexEnable_InsideACodexTurnDoesNotRestart(t *testing.T) { - envPath, cfgPath := codexFixture(t, "") - f := &fakeCodexDaemon{pid: 5613, env: offCortex, newPID: 7001, newEnv: onCortex, updaterPID: 5600, updaterEnv: offCortex} - stubCodexDaemon(t, filepath.Dir(envPath), f) - t.Setenv(codexThreadIDEnv, "019a0000-0000-7000-8000-000000000000") - - var out, errb bytes.Buffer - if code := codexEnable(envPath, cfgPath, filepath.Join(filepath.Dir(envPath), "state.json"), true, &out, &errb); code != 0 { - t.Fatalf("enable exit %d: %s", code, errb.String()) - } - if len(f.steps) != 0 { - t.Errorf("steps = %v, want nothing stopped or restarted from inside a Codex turn", f.steps) - } - wantAll(t, "enable output", out.String(), "Enabled —", "inside a Codex turn", "outside Codex", codexDaemonRestartCmd) - if readCodexEnv(t, envPath)["HTTPS_PROXY"] != cortexProxyURL { - t.Error("the file was not enabled") - } -} - // Declining changes nothing, so nothing is restarted, though the warning was shown. func TestCodexEnable_DeclinedRestartsNothing(t *testing.T) { envPath, cfgPath := codexFixture(t, "") diff --git a/docs/agents/codex.md b/docs/agents/codex.md index e076b8f6c..e166fb459 100644 --- a/docs/agents/codex.md +++ b/docs/agents/codex.md @@ -120,11 +120,6 @@ read the new file. Comparing `ps` before and after, both processes change. Witho `disable` came undone at the next Codex update, with the daemon back on Cortex's proxy. If the updater will not stop, they name it and the command that replaces it. -**Not from inside Codex.** Run as a command inside a Codex turn — asking Codex to run `agentop -configure codex enable` — they change the file but do not restart anything: the restart would -wait for the very turn that is waiting for it. They print the restart command to run from a -terminal instead. Codex marks such commands with `CODEX_THREAD_ID`, which is how they tell. - **What a restart does to Codex sessions**, from Codex's source (0.163.0 and 0.164.0), not measured here: the old daemon stops taking new turns, gives running ones its shutdown grace — 60 seconds by default, configurable up to 5 minutes — to finish, and saves its threads. The new daemon restores them and