Make the plugin actually run on Windows: import #725 and #656, then fix what the audit found - #8
Merged
Merged
Conversation
…e updating job state on a partial kill failure Bug 1: runCommand and SpawnedCodexAppServerClient.initialize both consulted process.env.SHELL when deciding the shell: option on Windows. SHELL is a POSIX convention with no meaning for native Windows process creation -- on a machine where it's set to a POSIX shell path (e.g. Git Bash, which Claude Code's own Bash tool sets), spawn/spawnSync routes commands through that shell instead of cmd.exe, and MSYS's automatic POSIX-path conversion mangles Windows-style flags like taskkill's /PID. Fixed by always using true on win32, never consulting SHELL. Bug 2: terminateProcessTree throws whenever taskkill exits non-zero for a reason other than "process not found". handleCancel called it unguarded, so a partial /T tree-kill failure on Windows aborted the whole cancel before the job's on-disk status was ever updated, leaving it stuck at running/finalizing forever even though the turn interrupt had already succeeded. Fixed by wrapping the call in try/catch and logging-but-continuing, matching terminateProcessTree's own best-effort semantics for the already-gone case. Fixes openai#647
…termination succeeded handleCancel's catch around terminateProcessTree unconditionally continued and reported the job as cancelled, clearing its pid, even when the turn interrupt was also unavailable/failed. terminateProcessTree already treats "process already gone" as non-fatal without throwing, so reaching the catch means the outcome is genuinely unknown, not just "already stopped." In that case, with the interrupt also not confirmed, nothing had actually proven the worker stopped -- reporting cancelled and clearing pid would let a write-capable task keep modifying the workspace unsupervised, with its later completion able to overwrite the fabricated cancelled status. Extracted the decision (wasCancellationConfirmed) into job-control.mjs so it's directly unit-testable, since codex-companion.mjs's handlers aren't exported and forcing terminateProcessTree to genuinely throw via a real subprocess integration test isn't reliably engineerable. When neither path confirms the stop, the job's status/pid are left unchanged and the command throws instead of reporting a cancellation that may not have happened. Found via Codex Review on the PR.
Upstream PR openai#725 (fscfede-beep), four conflicts resolved. runCommand() defaulted to `shell: process.env.SHELL || true` on Windows, so every command ran through whatever shell happened to be configured. Under Git Bash that means MSYS path conversion rewrites switch arguments: `taskkill /PID 1234 /T /F` becomes `bash -c "taskkill /PID …"` and `/PID` is expanded to `C:/Program Files/Git/PID`, failing every kill with "Invalid argument/option" — so a background worker could not be terminated at all. Commands are now spawned directly (`shell: false`), and the one case that genuinely needs a shell — a Windows `.cmd` shim such as `codex` or `npm` — goes through an explicit `cmd.exe /d /s /c call`, which takes its arguments as an array and performs no path rewriting. binaryAvailable() and the app-server spawn both use it; `taskkill` is direct. Conflicts: the app-server import (union with this fork's isBrokerEndpointReady) and three in tests/process.test.mjs, where the PR's two new tests were written against upstream's shorter file. The fork's file is taken whole and those two tests appended, rather than merged hunk by hunk — the conflict interleaved their bodies with the fork's own Windows identity tests. Audited every launch site for the new default, since a `.cmd` reached without the shim would now fail with ENOENT: `node`, `npm` and `codex` go through binaryAvailable() and are shimmed; `codex app-server` is shimmed at its spawn; `git` already passed `shell: false` explicitly; `taskkill` and `powershell.exe` are real executables; the worker and broker are spawned as process.execPath. This also makes openai#735 redundant — it fixed `taskkill` alone, which this covers. Verified: tests/process.test.mjs, tests/runtime.test.mjs and tests/commands.test.mjs 152/152. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Upstream PR openai#656 (mittalpk), five conflicts resolved. Two halves, and this fork already had one of them. The Windows half — taskkill mangled by the user's shell — is fixed better by openai#725, merged just before: where this PR keeps `shell: true` on Windows and lets Node pick cmd.exe, openai#725 spawns directly and wraps only a real `.cmd` shim in an explicit cmd.exe call. Those two conflicts keep openai#725's code and take this PR's explanation of why SHELL must never be consulted for Windows process creation. The other half is not Windows-specific and is the reason to take this PR: `/codex:cancel` reported success whenever it reached the end, even if the turn interrupt had failed *and* terminating the worker tree had thrown — a partial `taskkill /T` refusing part of the tree is the real case. Nothing had then proven the job stopped, so a write-capable task could go on editing the workspace behind a record that says cancelled. Grafted rather than adopted, because the flows differ: the PR throws before writing the record, while this fork is record-first (the terminal record is persisted before the turn is touched, so a crash mid-cancel cannot leave an interrupted turn with no outcome). The cancellation is therefore not rewound. Instead `wasCancellationConfirmed()` decides a new `cancellationConfirmed` field in the payload, the report is printed, and the command then exits non-zero naming what was not confirmed. The caller gets both the record and an unmistakable failure signal. Also pinned what openai#725 actually fixes: the taskkill test now asserts `shell: false`, and fails if that argument is dropped. Verified: full npm test 318/318; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Both Windows branches in app-server.mjs justified the tree kill with "with shell: true the direct child is cmd.exe". Since openai#725 nothing is spawned through a shell, so that reasoning reads as obsolete — and the natural conclusion from it (drop the tree kill, there is no shell child) is wrong: `codex` is a .cmd shim, so commandWithWindowsShim() still puts a real cmd.exe between us and the app-server. Say that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Requirements now name Git Bash (the hooks launch scripts/run-node.sh through it) and spell out that nothing else goes through a shell, with the taskkill failure as the reason it matters. Differences From Upstream gains the two imports of this batch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
`process.kill(-pid, "SIGKILL")` addresses a process group, which only POSIX has. On Windows a negative pid is an invalid handle: OpenProcess fails, the call throws, and the catch-and-retry underneath it quietly degrades to killing the single process — leaving the worker's app-server, and every MCP server under it, running. That is the leak the SessionEnd hook exists to prevent, and it happened on exactly the stragglers that had already ignored SIGTERM. forceKillProcessTree() makes the platform choice once: SIGKILL the group on POSIX (with the non-leader fallback), taskkill's own tree walk on Windows, where /F is already the hardest stop available. The SessionEnd hook and the escalation inside terminateProcessTreeAndExit() both go through it. A guard test keeps the rule: no plugin script calls process.kill() with a negative pid, and group signalling stays inside process.mjs's platform-guarded helpers, which take an injectable killImpl. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Replacing a file through rename can fail on Windows while something else holds the target open — an on-access scanner or the search indexer is enough — and it surfaces as EPERM/EACCES/EBUSY. Every job and state write goes through writeJsonFileAtomic(), so one unlucky moment threw the record away instead of recording it. renameReplacing() waits the sharing violation out: four short waits, then the error is real and is thrown. POSIX has no such failure mode, so the retry is Windows-only and any other error code fails on the first attempt. A guard test keeps bare fs.renameSync() out of the plugin scripts, except in locking.mjs, where EPERM already means "someone else holds the lock". README documents both Windows fixes and what running on Windows requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
They were described in the fork-fix list, which is for defects the imports surfaced — not for the imports themselves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third import batch, this one about Windows. Two upstream PRs merged one commit each (revertable individually, authorship and hashes preserved), then an audit of every launch, kill, pipe and rename site — which turned up two real defects the imports did not cover.
328/328 tests, full suite run twice.
npx tsc -p tsconfig.app-server.jsonclean.Imported
/codex:cancelno longer reports a cancellation nothing confirmedopenai#725.
shell: process.env.SHELL || truewas the root cause: under Git Bash, MSYS path conversion rewrites/PIDinto a filesystem path, sotaskkill /PID … /T /Ffailed every single time and background workers were unkillable. Nowshell: options.shell ?? falseeverywhere, andcodex/npm— which are.cmdshims on Windows — go through a newcommandWithWindowsShim()that spawns an explicitcmd.exe /d /s /c call …. This supersedes #735, which only addressed a symptom; #669 competes with it and was not imported.openai#656.
wasCancellationConfirmed()grafted onto this fork's record-first kill site, with a newcancellationConfirmedfield in the payload and a non-zero exit: when neither the turn interrupt nor the worker kill confirmed the job stopped,/codex:cancelsays so instead of claiming a cancellation it cannot prove. The PR's Windows half was superseded by openai#725 and dropped in the merge.Found by the audit
Both are real, both are Windows-only, both have a test that fails when only that fix is reverted.
1. Force-kill through a negative pid.
process.kill(-pid, "SIGKILL")addresses a process group — POSIX-only. On Windows a negative pid is an invalid handle:OpenProcessfails, the call throws, and the catch-and-retry underneath quietly degraded to killing the worker alone, leaving its app-server and every MCP server under it running. That is precisely the leak the SessionEnd hook exists to prevent, and it happened on the stragglers that had already ignored SIGTERM. NewforceKillProcessTree()makes the platform choice once: SIGKILL the group on POSIX (with the non-leader fallback),taskkill /T /Fon Windows. Used by the SessionEnd hook and by the escalation insideterminateProcessTreeAndExit().2. Atomic write refused by the filesystem. Every job and state write goes through
writeJsonFileAtomic(). On Windows the replacing rename can fail withEPERM/EACCES/EBUSYwhile something else holds the target open — an on-access scanner or the search indexer is enough — and one unlucky moment threw the record away.renameReplacing()waits it out over four short retries, then the error is real and is thrown. POSIX has no such failure mode, so no retry there, and any other error code fails on the first attempt.Also corrected two comments in
app-server.mjsthat justified the Windows tree kill with "withshell: truethe direct child is cmd.exe". After openai#725 that reasoning reads as obsolete, and the natural conclusion from it — drop the tree kill, there is no shell child — is wrong:codexis still a.cmdshim, so a realcmd.exeremains between us and the app-server.Regression guards
Two structural tests keep the rules from being undone by a later change:
process.kill()with a negative pid; group signalling stays insideprocess.mjs's platform-guarded helpers, which take an injectablekillImplfs.renameSync(), exceptlocking.mjs, whereEPERMalready means "someone else holds the lock"Docs
README now states the Windows requirement (Git Bash, because the hooks launch
scripts/run-node.shthrough it —hooks.jsonalready handlescygpath), spells out what is spawned and how, notes that the broker listens on a named pipe with no filesystem artifact to clean up, and lists both imports in the imports table.Caveat, stated plainly
There is no Windows machine in this environment. Windows coverage here is by injected
platform: "win32"— rigorous about what each function does, but none of these paths has been executed on real Windows. A run on an actual machine is the only definitive check.🤖 Generated with Claude Code
https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Generated by Claude Code