Skip to content

fix: pin interpreters whose path contains a space#2186

Closed
Souptik96 wants to merge 1 commit into
Graphify-Labs:v8from
Souptik96:fix/2166-pin-interpreter-path-with-spaces
Closed

fix: pin interpreters whose path contains a space#2186
Souptik96 wants to merge 1 commit into
Graphify-Labs:v8from
Souptik96:fix/2166-pin-interpreter-path-with-spaces

Conversation

@Souptik96

Copy link
Copy Markdown
Contributor

Fixes #2166

Root cause (a bit narrower than the report)

The empty _PINNED comes from the install-time allowlist in _pinned_python(), not from the
uv-tool layout. It accepted [a-zA-Z0-9/_.@:\-] — no space — and rejects the path
wholesale on any non-matching character, so sys.executable under a profile whose name
contains a space records nothing.

Verified on Windows against v8 (0.9.26), stubbing sys.executable:

PINNED   'C:\Users\souptic\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe'  -> pinned
EMPTY!!  'C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe' -> ''
PINNED   '/home/u/.local/share/uv/tools/graphifyy/bin/python'                      -> pinned
EMPTY!!  'C:\Users\evil\py.exe; rm -rf /'                                          -> ''   (correct)
EMPTY!!  "C:\Users\ev'il\python.exe"                                               -> ''   (correct)

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 _PINNED is empty, probe 1
is 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_python at hook
install 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 already
    quotes 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 unquoted
    into merge.graphify.driver, which git runs through a shell — a spaced path would split into
    two 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,
#2126 already established that a pinned C:\... path is the expected shape, and rewriting it
would churn the existing test_install_embeds_pinned_interpreter assertion for no gain.

Testing

New tests in tests/test_hooks.py:

$ uv run pytest tests/test_hooks.py -q -k "pinned_python_accepts_paths_containing_spaces or pinned_python_still_rejects or merge_driver_quotes_interpreter or install_pins_interpreter_path_with_spaces"
11 passed, 67 deselected, 1 warning in 1.29s

Confirmed they actually catch the bug — same tests with graphify/hooks.py reverted to v8:

FAILED tests/test_hooks.py::test_pinned_python_accepts_paths_containing_spaces[C:\Users\First Last\...\python.exe]
FAILED tests/test_hooks.py::test_pinned_python_accepts_paths_containing_spaces[C:\Program Files\Python312\python.exe]
FAILED tests/test_hooks.py::test_pinned_python_accepts_paths_containing_spaces[/home/first last/...]
FAILED tests/test_hooks.py::test_merge_driver_quotes_interpreter_with_spaces - interpreter not quoted in merge driver: 'graphify merge-dri...'
FAILED tests/test_hooks.py::test_install_pins_interpreter_path_with_spaces - post-commit did not pin the spaced interpreter
5 failed, 73 deselected

Affected files plus both pre-commit gates:

$ uv run pytest tests/test_hooks.py tests/test_hook_strict.py tests/test_install.py -q
11 failed, 172 passed, 1 warning in 19.40s

$ uv run ruff check --config pyproject.toml graphify/hooks.py tests/test_hooks.py
All checks passed!

$ uv run python -m tools.skillgen --check
check OK: 134 artifact(s) match committed output and expected/.

Those 11 failures are pre-existing on Windows, not from this PR. Baseline uv run pytest tests/ -q on unmodified v8 (66d8110) is 45 failed, 3578 passed, 15 skipped, of which
exactly 11 live in these three files (test_hooks.py 8, test_hook_strict.py 1,
test_install.py 2) — the same 11, and most of the test_hooks.py ones are the
bash-dependent allowlist tests plus WinError cases. I have no Linux/macOS box to confirm
they're green there, so please read that number in that light.

`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=''`.
@safishamsi

Copy link
Copy Markdown
Collaborator

Thanks @Souptik96. Shipped in v0.9.27 (cherry-picked to v8). Closed-unmerged here, but it's in the release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hook install emits empty _PINNED on Windows uv installs, so every post-commit rebuild silently no-ops

2 participants