fix: pin interpreters whose path contains a space#2186
Open
Souptik96 wants to merge 1 commit into
Open
Conversation
`graphify hook install` emitted `_PINNED=''` for some Windows uv-tool installs, so every interpreter probe failed, each commit printed "could not locate a Python with graphify installed" and the graph never rebuilt. Root cause is the install-time allowlist in `_pinned_python()`, not the uv layout: it accepted `[a-zA-Z0-9/_.@:\-]` but not a plain space, so any `sys.executable` under a profile whose name contains one -- `C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe`, or the equally common `C:\Program Files\Python312\python.exe` -- was rejected wholesale and nothing was recorded. A space-free Windows uv path pins correctly, which is why this looks layout-specific. A space is safe to allow because every consumer already quotes the value: the hook scripts embed it as `_PINNED='<path>'` and dereference `"$_PINNED"`, so a space can neither split a word nor start a command. Adding it to the allowlist therefore fixes the pin without weakening the injection guard -- `$`, backtick, `;`, `'` and `"` are all still rejected. `_register_merge_driver` did interpolate the path unquoted into the `merge.graphify.driver` command, which git runs through a shell; that would split a spaced path into two words, so it is now double-quoted. Double quotes are safe here precisely because the allowlist keeps `$` and backticks out. Tests: spaced Windows/POSIX paths are pinned; metacharacter paths are still rejected (including `'` and `"`); the merge driver quotes a spaced interpreter; and the installed post-commit/post-checkout hooks carry the real path rather than `_PINNED=''`.
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.
Fixes #2166
Root cause (a bit narrower than the report)
The empty
_PINNEDcomes from the install-time allowlist in_pinned_python(), not from theuv-tool layout. It accepted
[a-zA-Z0-9/_.@:\-]— no space — and rejects the pathwholesale on any non-matching character, so
sys.executableunder a profile whose namecontains a space records nothing.
Verified on Windows against
v8(0.9.26), stubbingsys.executable:So a space-free Windows uv path pins fine — which is why this reads as uv-layout-specific.
What actually breaks it is a space anywhere in the interpreter path:
C:\Users\First Last\...,or the equally common
C:\Program Files\Python312\python.exe. Once_PINNEDis empty, probe 1is skipped and (per the report's own table) probes 2–4 can't recover on a uv install, so every
commit warns and no-ops.
That also means the issue's other suggestion — writing
graphify-out/.graphify_pythonat hookinstall time — isn't needed to fix this; probe 1 works once it's actually populated. I left it
out to keep the change minimal.
Fix
_pinned_python(): allow a plain space. This is safe because every consumer alreadyquotes the value — the hook embeds
_PINNED='<path>'(single-quoted) and dereferences"$_PINNED", so a space can neither split a word nor start a command.$, backtick,;,'and"remain rejected, so the injection guard is unchanged._register_merge_driver(): quote the interpreter. It interpolated the path unquotedinto
merge.graphify.driver, which git runs through a shell — a spaced path would split intotwo words and the driver would never run. Now double-quoted, which is safe precisely because
the allowlist keeps
$/backticks out.I deliberately did not convert the pinned path to forward slashes (the issue suggests
sys.executable.replace("\\", "/")). Probe 1 quotes"$_PINNED"so backslashes are fine there,#2126already established that a pinnedC:\...path is the expected shape, and rewriting itwould churn the existing
test_install_embeds_pinned_interpreterassertion for no gain.Testing
New tests in
tests/test_hooks.py:Confirmed they actually catch the bug — same tests with
graphify/hooks.pyreverted tov8:Affected files plus both pre-commit gates:
Those 11 failures are pre-existing on Windows, not from this PR. Baseline
uv run pytest tests/ -qon unmodifiedv8(66d8110) is 45 failed, 3578 passed, 15 skipped, of whichexactly 11 live in these three files (
test_hooks.py8,test_hook_strict.py1,test_install.py2) — the same 11, and most of thetest_hooks.pyones are thebash-dependent allowlist tests plusWinErrorcases. I have no Linux/macOS box to confirmthey're green there, so please read that number in that light.