fix(hooks): strip interpreter flags from launcher shebang#2045
Open
nowage wants to merge 1 commit into
Open
Conversation
pipx writes launcher shebangs as `#!/path/to/venv/bin/python -E`. The shebang probe in _PYTHON_DETECT kept the trailing flag, so the path allowlist that runs right after rejected the whole value (a space is not a valid path character) and blanked GRAPHIFY_PYTHON. Detection then fell through to the last-resort `python3` probe, which on many machines is an unrelated system interpreter carrying an older graphify — every commit rebuilt the graph with a stale version and tripped the node-shrink guard. Drop everything after the first token before the allowlist check, so both `#!/path/to/python -E` (pipx) and `#!/usr/bin/env python3 -E` resolve to a usable interpreter. Flagless shebangs are unaffected. Adds a regression test that runs the generated detection snippet against a fake pipx-style launcher and asserts the venv interpreter is picked.
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
1 participant
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.
Problem
The post-commit hook's third interpreter probe parses the
graphifylauncher's shebang. pipx writes launchers with an interpreter flag:The probe keeps the whole string, including
-E. The path allowlist immediately below rejects it because of the space,GRAPHIFY_PYTHONis blanked, and the last-resort probe falls through topython3on PATH — which is a different environment that may hold an older graphify.The failure is silent and the symptom is misleading. On my machine the fallback interpreter had a stale graphify, so every commit rebuilt a smaller graph and tripped the shrink guard:
The guard was doing its job; nothing was wrong with the graph. It took a while to trace the warning back to interpreter selection rather than graph corruption, and the tempting "fix" (
--force) would have been the wrong move.The pinned-interpreter probe and
graphify-out/.graphify_pythonusually prevent this, so the shebang path is only reached when both miss — a stale pinned path after a reinstall, or a repo whosegraphify-out/has not been written yet. When it is reached, it currently fails for the most common isolated install layout.Fix
Keep the first whitespace-delimited token before the allowlist check:
GRAPHIFY_PYTHON="${GRAPHIFY_PYTHON%% *}"Shebangs without flags are unaffected — the parameter expansion is a no-op when there is no space. This also covers
-s/-I, which uv and some venv launchers emit.Tests
Two tests in
tests/test_hooks.py, both driving the generated snippet with a fake pipx-style launcher rather than asserting on the template text:test_shebang_probe_strips_interpreter_flags— flagged shebang resolves to the venv interpreter. Fails onv8without this patch.test_shebang_probe_accepts_plain_interpreter— unflagged shebang keeps working.tests/test_hooks.pypasses in full (60 passed). The 4 pre-existing failures intests/test_ollama_retry_cap.pyare unrelated (missingopenaifrom theollamaextra) and reproduce on a cleanv8.Notes
This is deliberately a minimal fix to the existing probe. I noticed #1619 argues for dropping shebang parsing in the skill templates in favour of probing uv/pipx directories; if you want the hook to move the same way, this patch is easy to drop in favour of that. It touches only
graphify/hooks.pyand does not overlap with #1962, which is confined toskill-*.md.