From d2162a1f77aa8a624e8fbc92ed5a2d15589b342b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 17:07:19 +0900 Subject: [PATCH 1/2] fix(codex): match the shim's launcher backups in --restart-codex Completes contributor PR #2884, which reported this with ps output from an affected host. When the autostart shim installs, backupPathFor renames the original launcher by inserting .opencodex-real before its extension, so a shimmed host runs codex.opencodex-real app-server. isCodexExecutableToken never admitted that name, so ocx sync --restart-codex reported zero processes stopped and left app-servers alive on stale in-memory catalogs. Two corrections to #2884. It omits codex.opencodex-real.ps1, and findWindowsCodexTargets shims codex.ps1 alongside codex.cmd, so that backup runs too. Its Windows prefilter puts the optional suffix before the target triple, which admits codex.opencodex-real-x86_64-pc-windows-msvc.exe. backupPathFor writes the suffix after the stem and before the extension, so that name is one nothing produces and it pays GetOwner for it. The basename set is exact and stays separate from the triple pattern. An audit rejected my first plan, which would have stripped the suffix before the triple test so any stem matched: that turns codex-report-generator-worker.opencodex-real into a valid triple and makes an unrelated process a kill target. The same audit disproved my premise that triple backups exist -- Unix discovery accepts only a PATH entry named codex and Windows refuses a real codex.exe, so nothing hands a triple-named binary to backupPathFor. .exe stays as breadth rather than an observed shape, and the comment says so. Mutations: not admitting backups turns the reported command line red; reversing the suffix ordering turns the prefilter negative red. 72 pass / 0 fail across the process-matching and core-lab-boundary suites; tsc clean. --- .../140_pr2884_shim_backup_matcher.md | 71 +++++++++++++++++++ src/codex/app-server-processes.ts | 46 ++++++++++-- tests/codex-app-server-processes.test.ts | 40 +++++++++++ 3 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 devlog/_plan/260829_bugpr_lane_h_residual_issues/140_pr2884_shim_backup_matcher.md 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..07597a4cd4 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/140_pr2884_shim_backup_matcher.md @@ -0,0 +1,71 @@ +# 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. `.exe` is kept as breadth, not as an observed shape: +current installation refuses to rename a native `codex.exe`, so matching that name costs +nothing while missing one leaves a process alive. + +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. + +## 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. + +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. diff --git a/src/codex/app-server-processes.ts b/src/codex/app-server-processes.ts index fbce428278..634fa843b6 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,37 @@ 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` is here because `findWindowsCodexTargets` shims `codex.ps1` alongside + * `codex.cmd`. `.exe` is breadth rather than an observed shape: current installation + * refuses to rename a native `codex.exe`, and matching a name nothing produces costs + * nothing, while missing one leaves a process alive. + */ +const CODEX_LAUNCHER_BASENAMES = new Set([ + "codex", "codex.exe", "codex.cmd", + "codex.opencodex-real", + "codex.opencodex-real.exe", + "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 +194,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); } @@ -418,9 +454,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..96d0eda157 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -397,6 +397,30 @@ 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); + }); + 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 +520,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); From e2f2830895cafc8cfcc8f7077914268bf87b5955 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 17:20:36 +0900 Subject: [PATCH 2/2] fix(codex): stop matching a TUI prompt, and drop the impossible backup Two blockers from an independent review of the shim-backup matcher. codex -- app-server matched, and matched before this branch too: the option loop consumed -- like any other --prefixed token. But -- ends option parsing, so the next word is a prompt for the interactive TUI. That command opens a session whose first prompt word is "app-server", and --restart-codex sent it SIGTERM. The scanner now stops at --. That bug is not #2884's and not caused by the backup names -- it applies to every launcher spelling. It is fixed here because this change widens which processes reach the scanner, and shipping a broader matcher over a known false positive is the wrong order. Second: codex.opencodex-real.exe is no longer matched. #2884 included it and so did my first version, on the reasoning that matching a name nothing produces is free breadth. It is not free here. This set decides what receives SIGTERM, and Windows installation refuses to rename a native codex.exe, so that backup cannot exist. Mutations: restoring -- as an ordinary option turns the TUI-prompt case red; readmitting the .exe backup turns its negative red. 56 pass / 0 fail; tsc clean. --- .../140_pr2884_shim_backup_matcher.md | 29 +++++++++++++++++-- src/codex/app-server-processes.ts | 15 ++++++---- tests/codex-app-server-processes.test.ts | 20 +++++++++++++ 3 files changed, 56 insertions(+), 8 deletions(-) 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 index 07597a4cd4..71ffe76685 100644 --- 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 @@ -49,9 +49,13 @@ path whose job is sending SIGTERM, widening the matcher to be tidy is the wrong 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. `.exe` is kept as breadth, not as an observed shape: -current installation refuses to rename a native `codex.exe`, so matching that name costs -nothing while missing one leaves a process alive. +`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 @@ -59,13 +63,32 @@ stem, before the extension. #2884 placed it before the triple, which admits 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 634fa843b6..f0e2bbbe8b 100644 --- a/src/codex/app-server-processes.ts +++ b/src/codex/app-server-processes.ts @@ -80,15 +80,16 @@ const CODEX_TARGET_TRIPLE_BASENAME_RE = new RegExp( * refuses a real `codex.exe` outright — so the combination is unreachable, not merely * unlisted. * - * `.ps1` is here because `findWindowsCodexTargets` shims `codex.ps1` alongside - * `codex.cmd`. `.exe` is breadth rather than an observed shape: current installation - * refuses to rename a native `codex.exe`, and matching a name nothing produces costs - * nothing, while missing one leaves a process alive. + * `.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.exe", "codex.opencodex-real.cmd", "codex.opencodex-real.ps1", ]); @@ -318,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; diff --git a/tests/codex-app-server-processes.test.ts b/tests/codex-app-server-processes.test.ts index 96d0eda157..cb51e3abbc 100644 --- a/tests/codex-app-server-processes.test.ts +++ b/tests/codex-app-server-processes.test.ts @@ -419,6 +419,26 @@ describe("Codex app-server process matching (#476)", () => { // 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); });