fix: guard pid <= 0 in killProcessTree on all platforms

[chuks-qua]

What

Guards against pid <= 0 at the top of killProcessTree before the platform check, and moves killSpy.mockRestore() into finally blocks in the tests.

Why

The original guard was Unix-only (if (pid > 0) inside the else branch). On Windows, killProcessTree(0) would call taskkill /T /F /PID 0, which targets the System Idle Process. This was caught by the try/catch and logged, but it was noisy and technically wrong. Consolidating the guard at the function entry covers both platforms with a single check.

The mockRestore() placement in try meant a failing assertion would leave the spy active and potentially affect later tests.

Key Changes

• process-kill.ts: Single if (pid <= 0) return guard before the platform branch
• process-kill.test.ts: killSpy.mockRestore() moved to finally in all three affected tests; added pid=0 on Windows and pid=-1 on Unix coverage tests

Related to #206

Summary by CodeRabbit

• Bug Fixes

  • Enhanced validation for process identifiers, now properly handling zero and negative values to prevent unintended operations.

• Tests

  • Expanded test coverage to include edge cases for invalid process identifiers across different platforms.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes add a platform-agnostic guard to killProcessTree that rejects invalid PIDs (≤0), preventing unnecessary operations. Tests are restructured to improve spy lifecycle management and expanded with new assertions validating the guard behavior across Unix and Windows platforms.

Changes

Cohort / File(s)	Summary
Test Suite Updates apps/server/src/__tests__/process-kill.test.ts	Restructured spy setup to define killSpy before try blocks and consistently restore in finally blocks. Added test cases verifying killProcessTree performs no action for pid ≤ 0 (no execFile calls on Windows, no process.kill calls on Unix). Platform mocking consistently reset in finally across tests.
Process Kill Implementation apps/server/src/services/process-kill.ts	Added platform-agnostic early return guard when pid <= 0, eliminating the prior Unix-specific if (pid > 0) conditional. Best-effort error handling and logging behavior unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix: resolve EBUSY on Windows worktree cleanup #206: Introduces the initial killProcessTree module and related test suite; directly connected as this PR refines its guard logic and test structure.

Poem

🐰 A guard appears with grace so keen,
When PIDs fall to zero or between,
No signals sent, no processes fall,
Just early returns protecting all!
Tests now shine with structure bright, 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding a platform-agnostic guard for pid <= 0 in killProcessTree, which is the primary objective of the PR.
Description check	✅ Passed	The description covers all required template sections with clear explanations of what changed, why it was needed, and the specific key changes made in both files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/process-kill-pid-guard

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

apps/server/src/__tests__/process-kill.test.ts (1)

111-121: Consider adding a test for negative pid on Windows for symmetry.

For completeness and explicit documentation, you could add a test verifying killProcessTree(-1) on Windows also results in no execFile call. This would make the test coverage symmetric across platforms for both pid = 0 and pid < 0 cases.

💡 Optional test to add

it("does nothing when pid is negative on Windows", async () => {
  const originalPlatform = process.platform;
  Object.defineProperty(process, "platform", { value: "win32" });
  try {
    await killProcessTree(-1);

    expect(mockExecFile).not.toHaveBeenCalled();
  } finally {
    Object.defineProperty(process, "platform", { value: originalPlatform });
  }
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/__tests__/process-kill.test.ts` around lines 111 - 121, Add a
symmetric test case in process-kill.test.ts that verifies killProcessTree(-1) on
Windows does nothing: mirror the existing "does nothing when pid is 0 on
Windows" test by temporarily setting process.platform to "win32", calling
killProcessTree with -1, and asserting mockExecFile was not called; ensure you
restore the original platform in the finally block and reference the same mocks
used by the existing test (killProcessTree, mockExecFile).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/server/src/__tests__/process-kill.test.ts`:
- Around line 111-121: Add a symmetric test case in process-kill.test.ts that
verifies killProcessTree(-1) on Windows does nothing: mirror the existing "does
nothing when pid is 0 on Windows" test by temporarily setting process.platform
to "win32", calling killProcessTree with -1, and asserting mockExecFile was not
called; ensure you restore the original platform in the finally block and
reference the same mocks used by the existing test (killProcessTree,
mockExecFile).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a0f63d19-e57b-438a-ac35-e2cd87990678

📥 Commits

Reviewing files that changed from the base of the PR and between 892f622 and 72debc5.

📒 Files selected for processing (2)

• apps/server/src/__tests__/process-kill.test.ts
• apps/server/src/services/process-kill.ts