fix: support Windows hosts for provider CLI resolution and process termination - #6
Open
inahus99 wants to merge 2 commits into
Open
fix: support Windows hosts for provider CLI resolution and process termination#6inahus99 wants to merge 2 commits into
inahus99 wants to merge 2 commits into
Conversation
Codex/Claude readiness used bare create_subprocess_exec names, which miss npm .CMD shims on Windows and left hung auth probes uncleared. Resolve executables via shutil.which, kill process trees with taskkill, and keep provider path serialization POSIX-stable.
Follow-up review of the Windows shim fix. kill_process_tree only signalled the single pid on POSIX despite promising descendants, orphaning every child of a session leader. It now signals the child's process group, and skips the group kill when the target shares this process's group so a caller can never signal itself. resolve_executable treated any command naming an existing file as a path, including bare names, so a file in the working directory could shadow a real PATH entry while Yoke operated on an untrusted checkout. Bare names now always go through PATH. Path-like commands are returned verbatim, because normalising through Path strips a leading "./" and turns an explicit relative path back into a PATH lookup. Also collapses three spellings of the Windows check onto one IS_WINDOWS constant, replaces a hand-rolled context manager with contextlib.suppress, and drops a misleading comment on readiness.__all__ that claimed to control direct imports. Adds a Windows CI matrix, since the code this branch changes is exactly the code that behaves differently there, and a changelog for the 0.1.8 release.
|
Hi @inahus99, thanks for your detailed note and for highlighting the overlap! We have updated PR #3 and PR #5 to include the complete set of Windows support enhancements:
Both PRs have been updated, pushed, and are green and ready for maintainer review! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Yoke currently cannot drive Codex or Claude on a Windows host. Provider CLIs installed through npm are reported as "not found on PATH" on every Windows machine, and
claude auth statuscan hang indefinitely instead of timing out. This PR makes the provider process layer work on Windows.Executable resolution. npm installs CLIs as
.CMD/.BATshims.asyncio.create_subprocess_exec("codex", ...)cannot launch those by bare name, so every readiness check reported the CLI as missing even when it was installed and working. Commands are now resolved throughshutil.which, which honoursPATHEXTand finds the shim.Process termination. Killing only the wrapper PID leaves the npm shim's child tree running. Windows now terminates via
taskkill /T /F; POSIX signals the child's process group.claude auth statusis additionally bounded by a timeout so a stalled CLI reports a repair hint rather than blocking the caller forever.Path serialization. Paths sent to provider APIs are serialized with
as_posix(), so Windows drive paths round-trip through providers that expect POSIX separators.The new
yoke.processmodule holds this logic in one place; it depends only on the standard library and adds no new requirements.Relationship to #3 and #5
This PR overlaps with two of @Adityakk9031's open PRs. That overlap was not deliberate — I found both only after the work was written — and I want to be explicit about it rather than quietly talk over them.
#5 (
scriptPathPOSIX serialization) — this branch contains the identical one-line change. I tried removing it to defer to #5, andtests/test_workflows.py::test_file_workflow_requires_native_workflow_featurefails on Windows without it, so it is a hard prerequisite for a green Windows suite rather than something I can drop. I'm happy to rebase this branch on top of #5 and drop the line the moment #5 lands — that ordering is better and the credit for the fix is theirs.#3 (
WinError 87inprocess_is_alive) — same underlying bug, different fix. #3 addsexcept OSError: return Falseto the existing helper. This branch instead movesprocess_is_aliveintoyoke.processand, on Windows, queries the process throughOpenProcess/GetExitCodeProcessrather than callingos.killat all, soWinError 87cannot arise in the first place. If maintainers would rather land the smaller, more targeted #3 first, I'll rebase on it.I have no preference about ordering and would rather both of their PRs merge first than have this one supersede them. Ordering is entirely the maintainers' call.
CI
Yoke's CI ran on
ubuntu-latestonly, so code whose whole purpose is Windows behaviour had no Windows coverage. This adds awindows-latestleg across Python 3.11/3.12/3.13 withfail-fast: false. That matrix is the only thing that will exercise the new POSIX process-group test on Linux and the shim-resolution tests on Windows.Testing
507 passed, 1 skippedlocally on Windows 11 / Python 3.13 (1 skippedis the POSIX-only process-group test, which runs on the Ubuntu legs).ruff check .clean.PATHEXT, the readiness timeout path, working-directory shadowing, POSIX descendant termination, and the self-kill guard.Two behaviours worth reviewer attention, both covered by tests:
PATH. Previously any command naming an existing file was treated as a path, so a file calledcodexin the working directory could shadow the real executable while Yoke operated on an untrusted checkout. Commands written as paths are returned verbatim rather than normalized, becausestr(Path("./codex"))strips the./and turns an explicit relative path back into aPATHlookup.start_new_session=Truelead their own group.Note on release sequencing
CodeAlmanac's Windows support depends on this landing and being published as
0.1.8— its pin isalmanac-yoke[claude]>=0.1.8. The version bump is included here, but cutting the tag and release is obviously a maintainer action. If this merges, a 0.1.8 release would unblock the CodeAlmanac side; happy to open that PR whenever it's useful.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.