[WRONG BRANCH] fix(adapters): terminate Windows coding-agent process trees - #476
[WRONG BRANCH] fix(adapters): terminate Windows coding-agent process trees#476luvs01 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7979ffe805
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const MAX_STDERR_BYTES = 8 * 1024; | ||
|
|
||
| function killWindowsProcessTree(pid: number): void { | ||
| const taskkill = `${process.env.SystemRoot ?? "C:\\Windows"}\\System32\\taskkill.exe`; |
There was a problem hiding this comment.
When OpenCodex is launched with a poisoned SystemRoot value, this constructs an absolute-looking path under the attacker-controlled directory and executes its taskkill.exe on the first abort or timeout with the proxy user's privileges and inherited environment. The repository already provides resolveTrustedWindowsTaskkillExe(), backed by GetSystemDirectoryW, specifically to avoid selecting system executables through caller-controlled environment variables; use that resolver or the same trusted-resolution logic here.
AGENTS.md reference: AGENTS.md:L357-L363
Useful? React with 👍 / 👎.
| if (platform === "win32" && child.pid !== undefined) { | ||
| try { | ||
| (deps.killWindowsProcessTree ?? killWindowsProcessTree)(child.pid); |
There was a problem hiding this comment.
When the direct child has emitted exit but close is delayed by an inherited stdio handle, child.pid remains populated even though that PID is no longer owned by this ChildProcess. The grace timer can enter this branch after Windows has recycled the PID, causing /T /F to terminate an unrelated process and its descendants; gate the PID-based kill on child.exitCode === null or an explicit exit flag before calling taskkill. Microsoft documents /T as ending the specified process and its child processes.
AGENTS.md reference: src/AGENTS.md:L17-L20
Useful? React with 👍 / 👎.
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
1 participant
Motivation
.cmd/.batshims launchcmd.exeas the direct child while the real CLI becomes a descendant, and killing only the tracked child leaves credential-bearing descendant CLIs running after cancellation or timeout.Description
killWindowsProcessTree) and a default implementation that calls the absolutetaskkill.exe /PID <pid> /T /FviaexecFileSyncto reliably kill a process tree on Windows.runCodingAgentTurn'skill()path whenplatform === "win32"andchild.pidis available, and fall back to the existingchild.killSIGTERM/SIGKILL ladder if tree termination fails or is unavailable.platformvalue consistently when producingcommandInvocationto keep Windows shim handling coherent with the termination choice.tests/providers/codebuddy-adapter.test.ts) that simulates a Windows.cmdshim and verifies the abort path calls the process-tree terminator and does not rely on a direct-child signal; minor test scaffolding was adjusted to expose a fake child'spidfor the assertion.Testing
bun test tests/providers/codebuddy-adapter.test.ts tests/providers/qoder-adapter.test.ts tests/windows/win-exec.test.ts, and the focused suite passed (47 tests passed).bun run typecheckandbun run privacy:scan, both of which passed.bun run test) in this environment; the change's focused tests remained green but the full parallel run surfaced unrelated integration failures in other areas (149 failures and 1 error) that are not caused by this adapters change.Codex Task