fix(lsp): improve Windows server detection and avoid unix 'which' in doctor

[MotorwaySouth9]

Summary

Fix Windows compatibility issues that make LSP features effectively unusable in the oh-my-opencode plugin (not opencode core). The main problems were:

• The doctor LSP check relied on unix-only which
• LSP server detection did not fully account for Windows executable resolution (npm *.cmd shims, PATHEXT, and Path casing)

Closes #832

Changes

Doctor: remove unix-only which

• Updated src/cli/doctor/checks/lsp.ts to stop spawning which and instead reuse the internal resolver (isServerInstalled).
• Added a regression test in src/cli/doctor/checks/lsp.test.ts to ensure the LSP doctor check does not spawn which.

LSP server detection: Windows-aware executable resolution

• Updated src/tools/lsp/config.ts (isServerInstalled) to:
  • honor PATHEXT on Windows
  • always include safe default extensions (.exe, .cmd, .bat, .ps1) as fallback
  • handle Path vs PATH env var casing on Windows

Tests

• Added src/tools/lsp/config.test.ts to cover Windows PATHEXT/Path behavior and non-Windows behavior.

Testing

bun test src/cli/doctor/checks/lsp.test.ts
bun test src/tools/lsp/config.test.ts
bun run typecheck

Notes

• Full bun test on Windows may currently fail due to unrelated pre-existing platform/path assumptions in other tests. The LSP-related tests listed above pass.

---

Summary by cubic

Fix Windows LSP detection and remove the Unix-only which call in the doctor, restoring LSP usability on Windows. Closes #832.

• Bug Fixes
  • Doctor: stop spawning which; reuse isServerInstalled for binary checks.
  • Windows detection: respect PATHEXT, add safe defaults (.exe, .cmd, .bat, .ps1), and handle Path vs PATH.
  • Tests: add coverage for Windows resolution and ensure doctor doesn’t spawn which.

^{Written for commit 8e02cab. Summary will update on new commits.}

[cubic-dev-ai]

2 issues found across 4 files

Confidence score: 3/5

• Restoring PATH/Path incorrectly in src/tools/lsp/config.test.ts can leave the test environment polluted for subsequent specs, making failures non-deterministic.
• The mock cleanup in src/cli/doctor/checks/lsp.test.ts isn’t exception-safe, so a thrown test leaves Bun.spawn patched for later tests, potentially hiding real regressions.
• Pay close attention to src/tools/lsp/config.test.ts, src/cli/doctor/checks/lsp.test.ts - environment and mock cleanup can leak across tests.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/cli/doctor/checks/lsp.test.ts">

<violation number="1" location="src/cli/doctor/checks/lsp.test.ts:23">
P2: Bun.spawn spy is not guaranteed to be restored if the test throws, so the mock can leak into subsequent tests.</violation>
</file>

<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: Teardown restores PATH/Path incorrectly on Windows; deleting "Path" after setting PATH wipes the restored value due to case-insensitive env vars, leaking env changes across tests.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

Thanks @cubic-dev-ai — good catches.\n\nApplied the suggested fixes:\n- Make spy restore exception-safe via in \n- Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \n\nLocal verification:\n- bun test v1.3.6 (d530ed99)\n- bun test v1.3.6 (d530ed99)\n- \n\nCould you please re-run the review?

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

Thanks @cubic-dev-ai — good catches.

Applied the suggested fixes:

• Make \�[0m\�[0m spy restore exception-safe via \�[0m\�[0m in \�[0m\�[0m
• Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \�[0m\�[0m
  ...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

[MotorwaySouth9]

Apologies — my previous comment got mangled by shell quoting.

Thanks @cubic-dev-ai — good catches (identified by cubic).

Applied the suggested fixes:

• Make Bun.spawn spy restore exception-safe via try/finally in src/cli/doctor/checks/lsp.test.ts
• Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in src/tools/lsp/config.test.ts

Local verification:

• bun test src/cli/doctor/checks/lsp.test.ts
• bun test src/tools/lsp/config.test.ts
• bun run typecheck

Could you please re-run the review?

[cubic-dev-ai]

Apologies — my previous comment got mangled by shell quoting.

Thanks @cubic-dev-ai — good catches (identified by cubic).

Applied the suggested fixes:
...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

Thanks @cubic-dev-ai — good catches.

Applied the suggested fixes:

• Make \�[0m\�[0m spy restore exception-safe via \�[0m\�[0m in \�[0m\�[0m
• Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \�[0m\�[0m
  ...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

Thanks @cubic-dev-ai — good catches.\n\nApplied the suggested fixes:\n- Make spy restore exception-safe via in \n- Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \n\nLocal verification:\n- bun test v1.3.6 (d530ed99)\n- bun test v1.3.6 (d530ed99)\n- \n\nCould you please re-run the review?

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:33">
P2: AfterEach merges PATH/Path and always sets both, introducing a new `Path` env var on POSIX instead of restoring the original state, polluting global test environment.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

1 issue found across 4 files

Confidence score: 4/5

• afterEach only copies one env variable back to both PATH and Path in src/tools/lsp/config.test.ts, so tests could leak a mutated PATH and introduce subtle platform-sensitive failures.
• Given the limited scope and test-only impact, the risk feels manageable but worth noting before merge.
• Pay close attention to src/tools/lsp/config.test.ts - ensure PATH and Path are restored independently to prevent test pollution.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach does not restore PATH and Path independently, collapsing them into one value and potentially altering the original environment state between tests</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

1 issue found across 4 files

Confidence score: 4/5

• afterEach in src/tools/lsp/config.test.ts reassigns both PATH and Path, risking environment contamination between tests and potentially hiding regressions.
• Overall change carries limited runtime impact but keeping tests deterministic would improve confidence in this merge.
• Pay close attention to src/tools/lsp/config.test.ts - afterEach merges PATH/Path and may leak env vars across tests.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach merges PATH/Path and assigns to both, potentially creating or overwriting `Path` and leaking env state across tests</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

No issues found across 4 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[cubic-dev-ai]

1 issue found across 4 files

Confidence score: 3/5

• afterEach currently rewrites both PATH and Path even on non-Windows platforms, so test runs could leave the process with the wrong env variable casing and clobbered PATH values—this is a clear regression risk across platforms.
• Risk level sits in the middle because the bug is concrete and user-facing (breaks environment state on non-Windows runs) but is limited to the test harness, so it’s tractable before merge.
• Pay close attention to src/tools/lsp/config.test.ts - ensure PATH/Path are restored independently without creating new env keys.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach does not restore PATH/Path independently, creating Path on non-Windows and potentially clobbering original env state</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

No issues found across 4 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[cubic-dev-ai]

No issues found across 5 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[kdcokenny]

Thanks for the contribution! 🚀