diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/140_pr2884_shim_backup_matcher.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/140_pr2884_shim_backup_matcher.md new file mode 100644 index 0000000000..71ffe76685 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/140_pr2884_shim_backup_matcher.md @@ -0,0 +1,94 @@ +# 140 — #2884: match the shim's launcher backups, and only those + +Contributor PR #2884 reported a real defect with `ps` output from an affected host. This +completes it. The plan below is the second version: an independent audit rejected the +first, and the rejection was correct. + +## Scope + +IN: `src/codex/app-server-processes.ts` (`isCodexExecutableToken`, +`WINDOWS_CODEX_BASENAME_CANDIDATE_RE`), `tests/codex-app-server-processes.test.ts`. + +OUT: the shim itself, and `.ps1` process-shape support beyond admitting the basename — +proving a PowerShell launcher's real command line needs a Win32 reproduction. + +## The defect + +`backupPathFor` (`src/codex/shim.ts`) renames the original launcher when the autostart +shim installs, inserting `.opencodex-real` before the extension. On a shimmed host the +running process is + +```text +/home/ubuntu/.local/bin/codex.opencodex-real -c features.code_mode_host=true app-server --listen unix:// +``` + +`isCodexExecutableToken` admitted `codex`, `codex.exe`, `codex.cmd` and target triples, +so `ocx sync --restart-codex` matched nothing, reported zero processes stopped, and left +app-servers alive holding stale in-memory model catalogs. + +## What the first plan got wrong + +It claimed `backupPathFor` also produces target-triple backups such as +`codex-x86_64-unknown-linux-gnu.opencodex-real`, and proposed stripping an optional +`.opencodex-real` segment before the existing checks so any stem would match. + +Both halves were wrong. + +**The triple case is unreachable.** Unix discovery accepts only a PATH entry named +`codex`. Windows discovery refuses a real `codex.exe` outright and targets `codex.cmd`, +`codex.ps1` and the extensionless Git-Bash launcher. Nothing hands a triple-named binary +to `backupPathFor`. + +**The normalisation would have been unsafe.** Stripping the suffix before +`CODEX_TARGET_TRIPLE_BASENAME_RE` turns `codex-report-generator-worker.opencodex-real` +into a syntactically valid triple, making an unrelated process a kill target. In a code +path whose job is sending SIGTERM, widening the matcher to be tidy is the wrong trade. + +## The fix + +An exact basename set, kept separate from the triple pattern rather than folded into it. + +`.ps1` is added because `findWindowsCodexTargets` shims `codex.ps1` alongside +`codex.cmd`, and #2884 omitted it. + +`.opencodex-real.exe` is deliberately NOT matched. Both #2884 and my first draft included +it, reasoning that matching a name nothing produces is free breadth. A review round +rejected that and was right: this set decides what receives SIGTERM, and Windows +installation refuses to rename a native `codex.exe`, so the backup cannot exist. Breadth +in a matcher is not free when the matcher's output is a signal. + +The Windows prefilter's optional suffix goes where `backupPathFor` writes it — after the +stem, before the extension. #2884 placed it before the triple, which admits +`codex.opencodex-real-x86_64-pc-windows-msvc.exe`: a name nothing produces, paying +GetOwner for it. The regex source is embedded into PowerShell, so every addition stays +within plain character classes that .NET reads identically. + +## A pre-existing kill-target bug, found on the way + +The same review round found that `codex -- app-server` matched, and still matched before +any of this work: `--` was consumed by the option-skipping loop like any other +`-`-prefixed token. But `--` ends option parsing, so the word after it is a prompt for +the interactive TUI. `codex -- app-server` opens a session whose first prompt word is +"app-server", and `--restart-codex` sent SIGTERM to it. + +That is not #2884's defect and it is not caused by the backup names — it applies to every +launcher spelling. It is fixed here because the shim-backup change widens which processes +reach this scanner, and shipping a broader matcher over a known false positive would be +the wrong order. The scanner now stops at `--`. + +## Verification + +Named mutations, each observed red: + +- Backups not admitted at all — the reported command line fails. +- Reversed suffix/triple ordering — the prefilter negative assertion fails. +- `--` treated as an ordinary option again — the TUI-prompt case fails. +- `.opencodex-real.exe` readmitted — the impossible-backup negative fails. + +Positive coverage uses the exact command line from the report. Negative coverage holds +the line the fix is at risk of crossing: a subcommand, an argument position, and +`codex-report-generator-worker.opencodex-real`, which must never match. + +One limit worth stating: `.ps1` is basename admission only. A real PowerShell launcher +runs as `powershell.exe -File `, which this scanner does not match, and proving +that shape needs a Win32 reproduction rather than another synthetic token test. diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index fbce428278..f0e2bbbe8b 100644 --- a/src/codex/app-server-processes.ts +++ b/src/codex/app-server-processes.ts @@ -44,9 +44,14 @@ const CODEX_TARGET_TRIPLE_BODY = "[a-z0-9_]+-[a-z0-9_]+-[a-z0-9_]+(?:-[a-z0-9_]+ * `"C:\Program Files\...\codex.exe" app-server` still reach GetOwner. * Also admits official target-triple basenames such as * `codex-x86_64-pc-windows-msvc.exe`. + * + * The optional `.opencodex-real` sits where `backupPathFor` actually puts it — after + * the stem and BEFORE the extension — and deliberately not before the triple. Written + * the other way it admits `codex.opencodex-real-x86_64-pc-windows-msvc.exe`, a name + * nothing produces, and pays GetOwner for it. */ export const WINDOWS_CODEX_BASENAME_CANDIDATE_RE = new RegExp( - `(^|[/\\\\\\s'"=])codex(-${CODEX_TARGET_TRIPLE_BODY})?([.]exe|[.]cmd)?['"]?(\\s|$)`, + `(^|[/\\\\\\s'"=])codex(-${CODEX_TARGET_TRIPLE_BODY})?([.]opencodex-real)?([.]exe|[.]cmd|[.]ps1)?['"]?(\\s|$)`, "i", ); @@ -57,6 +62,38 @@ const CODEX_TARGET_TRIPLE_BASENAME_RE = new RegExp( `^codex-${CODEX_TARGET_TRIPLE_BODY}(?:\\.exe|\\.cmd)?$`, ); +/** + * Launcher basenames a Codex app-server can be started through, including the + * `.opencodex-real` backups the autostart shim creates. + * + * When the shim installs, `backupPathFor` (`src/codex/shim.ts`) renames the original + * launcher by inserting `.opencodex-real` before its extension, so a shimmed host runs + * `~/.local/bin/codex.opencodex-real app-server`. Reported by a contributor (#2884) with + * `ps` output from an affected host: `--restart-codex` matched nothing and left + * app-servers alive holding stale in-memory catalogs. + * + * An EXACT set, kept separate from the target-triple pattern above rather than folded + * into it by stripping the suffix first. That shortcut is unsafe: normalising + * `codex-report-generator-worker.opencodex-real` yields a syntactically valid triple + * and would make an unrelated process a kill target. A triple binary cannot be a shim + * target anyway — Unix discovery accepts only a PATH entry named `codex`, and Windows + * refuses a real `codex.exe` outright — so the combination is unreachable, not merely + * unlisted. + * + * `.ps1` and `.cmd` are here because `findWindowsCodexTargets` shims both, and the + * extensionless form because Unix discovery and the Git-Bash launcher use it. There is + * deliberately no `.opencodex-real.exe`: Windows installation REFUSES to rename a native + * `codex.exe`, so that backup cannot exist. Matching it looked like free breadth until a + * review round put it plainly — this set decides what receives SIGTERM, and a name no + * installation can produce only widens what a coincidence can hit. + */ +const CODEX_LAUNCHER_BASENAMES = new Set([ + "codex", "codex.exe", "codex.cmd", + "codex.opencodex-real", + "codex.opencodex-real.cmd", + "codex.opencodex-real.ps1", +]); + /** True when a Windows CommandLine is worth paying GetOwner for (current-user scoped later). */ export function isWindowsCodexCandidateCommandLine(commandLine: string): boolean { return WINDOWS_CODEX_BASENAME_CANDIDATE_RE.test(commandLine) @@ -158,7 +195,7 @@ function tokenBasename(token: string): string { function isCodexExecutableToken(token: string): boolean { const base = tokenBasename(token); - return base === "codex" || base === "codex.exe" || base === "codex.cmd" + return CODEX_LAUNCHER_BASENAMES.has(base) || CODEX_TARGET_TRIPLE_BASENAME_RE.test(base); } @@ -282,6 +319,10 @@ export function isCodexAppServerCommandLine(commandLine: string, executable?: st let i = 1; while (i < tokens.length) { const token = tokens[i]!; + // `--` ends option parsing, so what follows is a prompt for the interactive TUI, not + // a subcommand. `codex -- app-server` starts a session whose first prompt word is + // "app-server"; treating it as a match sends SIGTERM to somebody's live session. + if (token === "--") return false; if (token.startsWith("-")) { i = advancePastCodexGlobalOption(tokens, i); continue; @@ -418,9 +459,9 @@ function windowsSnapshotPowerShellCommand(): string { // Newlines keep -Command as a real script (space-joined statements need ';'). // Double-quoted format string so `t expands to a real tab. // Codex candidates only: basename token codex / codex.exe / codex.cmd / - // official target-triple binaries (optional closing quote after the - // basename), or code-mode-host — not incidental substrings like a repo - // path with "opencodex". + // codex.ps1, their .opencodex-real shim backups, official target-triple + // binaries (optional closing quote after the basename), or code-mode-host — + // not incidental substrings like a repo path with "opencodex". const basenameMatch = powerShellSingleQuotedIgnoreCaseMatch(WINDOWS_CODEX_BASENAME_CANDIDATE_RE.source); const codeModeMatch = powerShellSingleQuotedIgnoreCaseMatch(WINDOWS_CODEX_CODE_MODE_HOST_CANDIDATE_RE.source); return [ diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index da8d0b522a..cb51e3abbc 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -397,6 +397,50 @@ describe("Codex app-server process matching (#476)", () => { expect(isCodexAppServerCommandLine("node /opt/codex-code-mode-host --session 1")).toBe(true); }); + /** + * Reported by a contributor in #2884 with `ps` output from an affected host: once the + * autostart shim renames the original launcher to `codex.opencodex-real`, + * `--restart-codex` matched nothing and left app-servers alive on stale catalogs. + */ + test("matches the .opencodex-real launcher backups the shim creates", () => { + expect(isCodexAppServerCommandLine("/home/ubuntu/.local/bin/codex.opencodex-real app-server proxy")).toBe(true); + // The exact command line from the report. + expect(isCodexAppServerCommandLine( + "/home/ubuntu/.local/bin/codex.opencodex-real -c features.code_mode_host=true app-server --listen unix://", + )).toBe(true); + expect(isCodexAppServerCommandLine("\"C:\\Program Files\\nodejs\\codex.opencodex-real.cmd\" app-server")).toBe(true); + // findWindowsCodexTargets shims codex.ps1 alongside codex.cmd, so its backup runs too. + expect(isCodexAppServerCommandLine("\"C:\\Program Files\\nodejs\\codex.opencodex-real.ps1\" app-server")).toBe(true); + expect(isCodexAppServerCommandLine("node /usr/local/bin/codex.opencodex-real app-server proxy")).toBe(true); + + // Still narrow: the suffix does not turn a subcommand or an argument into a match. + expect(isCodexAppServerCommandLine("codex.opencodex-real exec 'hello'")).toBe(false); + expect(isCodexAppServerCommandLine("node worker.js codex.opencodex-real app-server")).toBe(false); + // A backup name must not be normalised into the target-triple pattern. Stripping the + // suffix before that test would make this unrelated binary a kill target. + expect(isCodexAppServerCommandLine("/opt/tools/codex-report-generator-worker.opencodex-real app-server")).toBe(false); + // No shim installation can produce a .exe backup: Windows refuses to rename a native + // codex.exe. Matching a name nothing writes only widens what SIGTERM can reach. + expect(isCodexAppServerCommandLine("C:\\tools\\codex.opencodex-real.exe app-server")).toBe(false); + }); + + /** + * `--` ends option parsing, so the next word is a TUI prompt rather than a subcommand. + * `codex -- app-server` opens an interactive session whose first prompt word happens to + * be "app-server"; matching it sent SIGTERM to a live session. Predates the shim-backup + * work and applies to every launcher name. + */ + test("a prompt after -- is not the app-server subcommand", () => { + expect(isCodexAppServerCommandLine("codex -- app-server")).toBe(false); + expect(isCodexAppServerCommandLine("/usr/local/bin/codex -- app-server --listen unix://")).toBe(false); + expect(isCodexAppServerCommandLine("codex.opencodex-real -- app-server")).toBe(false); + expect(isCodexAppServerCommandLine("codex -c features.x=true -- app-server")).toBe(false); + expect(isCodexAppServerCommandLine("node /usr/local/bin/codex -- app-server")).toBe(false); + // The real invocations still match: a global option before the subcommand is ordinary. + expect(isCodexAppServerCommandLine("codex app-server")).toBe(true); + expect(isCodexAppServerCommandLine("codex -c features.x=true app-server")).toBe(true); + }); + test("matches the npm wrapper that supervises the native app-server", () => { // The shape that made `ocx sync --restart-codex` report a survivor on Linux. An @@ -496,6 +540,22 @@ describe("Codex app-server process matching (#476)", () => { expect(isWindowsCodexCandidateCommandLine( "node C:\\Users\\a\\opencodex\\src\\cli\\index.ts start", )).toBe(false); + // Shim backups reach GetOwner, in the shape backupPathFor actually writes: the + // suffix goes after the stem and before the extension. + expect(isWindowsCodexCandidateCommandLine( + "\"C:\\Program Files\\nodejs\\codex.opencodex-real.cmd\" app-server", + )).toBe(true); + expect(isWindowsCodexCandidateCommandLine( + "\"C:\\Program Files\\nodejs\\codex.opencodex-real.ps1\" app-server", + )).toBe(true); + expect(isWindowsCodexCandidateCommandLine( + "C:\\Users\\a\\.local\\bin\\codex.opencodex-real app-server", + )).toBe(true); + // The reverse ordering is a name nothing produces. Admitting it would pay GetOwner + // on a process that cannot be a shim backup. + expect(isWindowsCodexCandidateCommandLine( + "C:\\x\\codex.opencodex-real-x86_64-pc-windows-msvc.exe app-server", + )).toBe(false); expect(isWindowsCodexCandidateCommandLine("opencodex app-server")).toBe(false); expect(isWindowsCodexCandidateCommandLine("hermes-codex-bridge-mcp")).toBe(false); expect(isWindowsCodexCandidateCommandLine("hermes-codex-x86_64-pc-windows-msvc.exe")).toBe(false);