fix: cancel dead pids on non-English Windows (#423)

[HoneyTyagii]

Fixes #423

Problem

On non-English Windows, cancelling a background task whose worker has already died leaves the job stuck as "running" forever (and blocks --resume-last).

terminateProcessTree() runs taskkill /PID <pid> /T /F. When the process is already gone, taskkill exits 128 with a localized stderr (e.g. zh-TW 錯誤: 找不到處理程序). Two things then go wrong:

1. looksLikeMissingProcessMessage() only matches English strings, so the localized message never matches and the win32 branch falls through to throw.
2. runCommand() decodes output as utf8, but taskkill emits the console codepage (cp950, etc.), so the text is mojibake (U+FFFD runs) and could never be matched anyway. The exit code is the only locale-independent signal.

Because handleCancel() calls terminateProcessTree() before persisting the cancelled state, that throw turns a routine "already dead" case into a permanently stuck job.

Changes

• Treat taskkill exit code 128 as process-not-found in the win32 branch of terminateProcessTree(). The message check remains as a fallback.
• Invoke taskkill with shell: false so SHELL pointing at Git Bash no longer mangles /PID via MSYS path conversion, and to avoid the DEP0190 deprecation warning. runCommand() now accepts a shell override.
• Persist the cancelled state even if terminateProcessTree() throws (try/catch + log in handleCancel()), so abandoning a job can never leave the state machine stuck.
• Tests for the exit-128/mojibake case and the shell: false invocation.

Testing

node --test tests/process.test.mjs — all pass, including the two new cases:

✔ terminateProcessTree treats exit 128 as not-found even with localized/mojibake output
✔ terminateProcessTree invokes taskkill without a shell

Credit to the detailed root-cause analysis in #423.

[Copilot]

Pull request overview

Fixes Windows cancellation behavior so jobs don’t remain stuck as "running" when the worker PID is already dead—especially on non-English Windows where taskkill output is localized and often mojibake when decoded as UTF-8.

Changes:

• Treat taskkill exit code 128 as “process not found” (locale-independent) in terminateProcessTree().
• Add a shell override to runCommand() and force shell: false for taskkill to avoid MSYS/Git Bash argument mangling and DEP0190 warnings.
• Ensure /codex:cancel persists the cancelled state even if process termination throws (log + continue).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tests/process.test.mjs	Adds regression tests for exit-128 handling with localized/mojibake output and verifies taskkill is invoked with shell: false.
plugins/codex/scripts/lib/process.mjs	Adds shell override support to runCommand(), forces shell:false for taskkill, and treats exit code 128 as missing PID on Windows.
plugins/codex/scripts/codex-companion.mjs	Wraps terminateProcessTree() in try/catch during cancel to prevent stuck "running" jobs when termination fails.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rajpratham1]

This is a well-scoped Windows compatibility fix with good regression tests.

What's good
Fixes locale-dependent behavior by relying on the exit code (128) instead of parsing localized taskkill output.
Explicitly runs taskkill with:
shell: false

avoiding MSYS/Git Bash argument mangling (/PID → path conversion) and avoiding unnecessary shell invocation.

Makes cancellation more robust by ensuring a failed terminateProcessTree() call doesn't leave jobs permanently marked as running.
try {
terminateProcessTree(...);
} catch (...) {
appendLogLine(...);
}

This matches user expectations: cancelling a job should cancel it even if the process has already exited.

Test coverage

The added tests cover the important cases:

✅ localized/non-English Windows output
✅ exit code 128 handling
✅ taskkill invoked with shell: false

These directly validate the behavior introduced by the patch.

Minor observation

resolveShellOption() changes behavior for runCommand() by allowing null/undefined and validating the type. While this is a reasonable improvement, it's a small behavioral expansion beyond the Windows bug fix. It isn't problematic, but it could have been a separate cleanup commit.