Fix: Resolve Matrix CI failure (Windows pipx detection)#462
Draft
curdriceaurora wants to merge 1 commit into
Draft
Fix: Resolve Matrix CI failure (Windows pipx detection)#462curdriceaurora wants to merge 1 commit into
curdriceaurora wants to merge 1 commit into
Conversation
CI Full Matrix has failed daily on the Test Windows (py3.12) job for the
last several scheduled runs. Root cause is a Windows-only mismatch in
cli/doctor._detect_install_method:
pattern = os.path.join(pipx_home, "venvs") + os.sep
On Windows, os.path.join("/custom/pipx", "venvs") produces
"/custom/pipx\\venvs" (mixed separators) and os.sep is "\\", so the
pattern becomes "/custom/pipx\\venvs\\". A sys.executable like
"/custom/pipx/venvs/fo-core/bin/python" (the value used by
test_detect_pipx_via_pipx_home_env, and a realistic env-var shape on
machines where PIPX_HOME originates from a POSIX-style config) never
satisfies startswith() against that pattern, so detection falls through
to "pip".
Normalize both the executable path and each candidate pattern with
os.path.normpath so prefix comparison is platform-agnostic. The other
detection paths (~/.local/pipx/...) get the same treatment for
consistency — they happened to pass on Windows only because both sides
of the comparison used forward slashes by coincidence.
Verified:
- 162 tests in tests/cli/test_doctor.py pass on Linux.
- Direct simulation of Windows path semantics (ntpath) confirms the
failing test now passes and the other 4 cases in
TestInstallMethodDetection still behave correctly.
Failing run: https://github.com/curdriceaurora/fo-core/actions/runs/26938361854
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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.
Summary
The scheduled
CI Full Matrixworkflow has been failing daily onmainsince at least 2026-06-02 — the Test Windows (Python 3.12) job fails one test while all other matrix variants (Linux 3.11, Linux 3.12, macOS 3.12) pass.Failing test (1 of 4922 total):
Root Cause Analysis
The test sets
PIPX_HOME=/custom/pipxandsys.executable=/custom/pipx/venvs/fo-core/bin/python, then expects_detect_install_method()to return"pipx".cli/doctor.py:117-120builds the match pattern via:On Windows:
os.path.join("/custom/pipx", "venvs")→"/custom/pipx\venvs"(mixed separators;joindoesn't rewrite the input's existing forward slashes but usesos.septo attach the new segment)os.sep→"\\""/custom/pipx\venvs\""/custom/pipx/venvs/fo-core/bin/python".startswith("/custom/pipx\venvs\")→ False → falls through to"pip".This is a real correctness bug, not just a test issue: any Windows user whose
PIPX_HOMEcarries forward slashes (env files copied from a POSIX machine, WSL setups, scripted exports) would have pipx misdetected. The other tests in this class happened to pass on Windows only because they used forward-slash paths on both sides of the comparison and didn't go through theos.path.joinbranch.Classification: Deterministic code regression (Windows path handling). Not flaky, not infrastructure.
Fix
Normalize both the executable path and each candidate pattern with
os.path.normpath()before the prefix check. The same treatment is applied to the~/.local/pipx/...fallback branch for consistency.This keeps the production logic minimal and platform-agnostic; the test was already exercising a realistic input shape and didn't need to change.
Verification
tests/cli/test_doctor.pypass on Linux.ntpath(Windows path semantics) confirms:test_detect_pipx_via_pipx_home_envnow returns"pipx"(was"pip").TestInstallMethodDetectioncases still behave correctly.ruff checkclean.Impact
src/cli/doctor.py—_detect_install_method()only (used in 4 call sites within the same file for install/upgrade flows).Trade-offs considered
pathlib.Path: rejected —os.path.startswith-style prefix matching is more straightforward than pathlib'sis_relative_to, which would require try/except and adds an import.Generated by Claude Code