fix(win): SoniTranslate venv paths cross-platform — Scripts/ on Windows (#186)

[debpalash]

Closes #186.

Bug: sonitranslate.py hardcoded SONI_VENV/"bin"/{pip,python} (POSIX). On Windows, python -m venv creates Scripts\ (with .exe), so is_venv_ready() was always False (install loops), and start() fell back to the app's own interpreter → SoniTranslate never came up → RuntimeError("…failed to start within 30s").

Fix: add _venv_bin(name) → Scripts/{name}.exe on win32 else bin/{name} (mirrors the existing engines/indextts/bootstrap.py pattern); use it for the is_venv_ready/install/start pip+python paths. Also stop() now uses _proc.terminate() (cross-platform) instead of send_signal(SIGTERM), and drops the now-unused signal import.

Test: _venv_bin returns Scripts/pip.exe on win32 and bin/python on posix. Router smoke confirms the app still boots.

(Opt-in/experimental engine — Windows-only impact — but now functional there.)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Improvements

  • Enhanced cross-platform compatibility for virtual environment path handling, improving reliability on Windows and Linux systems.

• Tests

  • Added unit tests to verify cross-platform virtual environment binary path resolution.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3d61f16b-c18e-4b75-8a33-c072ea734075

📥 Commits

Reviewing files that changed from the base of the PR and between 7d9338b and b1c7048.

📒 Files selected for processing (2)

• backend/services/sonitranslate.py
• tests/test_sonitranslate_venv.py

---

📝 Walkthrough

Walkthrough

This PR fixes SoniTranslate venv path resolution to work on Windows. It introduces a _venv_bin() helper function to compute platform-specific executable paths (Windows Scripts/{name}.exe vs POSIX bin/{name}), refactors all venv binary path usages, replaces signal-based subprocess termination with Popen.terminate(), and adds cross-platform unit tests.

Changes

SoniTranslate Windows Venv and Subprocess Compatibility

Layer / File(s)	Summary
Venv executable path helper and integration backend/services/sonitranslate.py	Introduces _venv_bin() helper to compute platform-specific venv paths; refactors is_venv_ready(), install(), and start() to use it instead of hard-coded POSIX bin/ paths.
Cross-platform subprocess termination backend/services/sonitranslate.py	Removes signal module import and replaces _proc.send_signal(signal.SIGTERM) with _proc.terminate() for platform-agnostic child process shutdown.
Unit tests for venv path resolution tests/test_sonitranslate_venv.py	Adds parametric tests validating _venv_bin() outputs correct executable names and parent directories for Windows and POSIX platforms using sys.platform monkeypatching.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: fixing SoniTranslate venv paths for Windows cross-platform compatibility using Scripts/ instead of hardcoded bin/.
Description check	✅ Passed	The description is comprehensive and mostly complete. It details the bug, fix, testing approach, and references the issue. However, it does not follow the provided template structure with explicit sections like Summary, Changes, Type, Testing, and Checklist.
Linked Issues check	✅ Passed	The PR fully addresses all coding requirements from issue #186: adds _venv_bin() helper for cross-platform venv path resolution, uses it for pip/python paths in is_venv_ready/install/start, replaces signal.SIGTERM with terminate(), and includes tests.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #186 requirements: venv path helper, platform-aware binary paths, process termination improvement, and corresponding tests. No out-of-scope modifications detected.

_{✏️ 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/sonitranslate-windows-venv

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR fixes a Windows-only bug where sonitranslate.py hardcoded bin/pip and bin/python POSIX paths inside the venv, causing is_venv_ready() to always return False on Windows (where Python creates Scripts\ with .exe executables). The fix introduces a _venv_bin(name) helper that mirrors the existing engines/indextts/bootstrap.py pattern, and replaces send_signal(SIGTERM) with the cross-platform terminate().

• _venv_bin(name) returns Scripts/{name}.exe on win32 and bin/{name} on POSIX; used in is_venv_ready, install, and start.
• stop() now calls _proc.terminate() instead of _proc.send_signal(signal.SIGTERM), removing the dependency on the signal module.
• New unit tests cover both platform branches of _venv_bin via monkeypatch.

Confidence Score: 4/5

Safe to merge; the change is narrow, correctly mirrors the established indextts pattern, and the only feedback is a missing return-type annotation.

All three call sites (is_venv_ready, install, start) are correctly updated. The terminate() replacement is behaviourally equivalent to send_signal(SIGTERM) on Windows (both call TerminateProcess). Tests cover both platform branches. The only open item is the missing -> Path annotation on _venv_bin.

No files require special attention.

Important Files Changed

Filename	Overview
backend/services/sonitranslate.py	Adds _venv_bin() cross-platform helper and wires it into is_venv_ready/install/start; replaces send_signal(SIGTERM) with terminate(); drops signal import. Logic is correct and minimal.
tests/test_sonitranslate_venv.py	New test file covering both platform branches of _venv_bin via monkeypatch; assertions are correct and sufficient for the change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[is_venv_ready / install / start] --> B{_venv_bin name}
    B -->|sys.platform == win32| C[SONI_VENV / Scripts / name.exe]
    B -->|POSIX / macOS / Linux| D[SONI_VENV / bin / name]
    C --> E[pip.is_file / spawn pip install / Popen python]
    D --> E
    F[stop] --> G[_proc.terminate]
    G -->|cross-platform| H{timeout 5s}
    H -->|ok| I[_proc = None]
    H -->|TimeoutExpired| J[_proc.kill] --> I

Loading

_{Reviews (1): Last reviewed commit: "fix(win): SoniTranslate venv paths cross..." | Re-trigger Greptile}

[greptile-apps]

Missing return type annotation on _venv_bin. Every other function in this module is annotated, and the indextts _venv_python_path reference this mirrors also carries a -> Path annotation. Adding it keeps the file consistent and makes IDE type-checking fully effective for callers.

Suggested change

	def _venv_bin(name: str):
	def _venv_bin(name: str) -> Path:

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!