fix(hooks): strip shebang argument so pipx launchers resolve (#2629) - #2687
fix(hooks): strip shebang argument so pipx launchers resolve (#2629)#2687ousamabenyounes wants to merge 3 commits into
Conversation
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR fixes shebang parsing in the git post-commit hook and the interpreter-detection probes across the various agent skill files (graphify/hooks.py, skill-*.md, and their generated tools/skillgen expected fixtures). It adds a step that strips a trailing interpreter argument (e.g., the -E in pipx's #!/.../python -E) so the leading path word is kept before the character allowlist check. The change spans a CHANGELOG entry and the corresponding hook test file (tests/test_hooks.py), touching many parallel skill-template copies that share the same snippet.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1133 functions depend on the 987 functions this change touches.
Health — grade B; 7 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
dispatch_command()— 2 callers, 117 callees (high)install()— 25 callers, 6 callees (high)dispatch_install_cli()— 2 callers, 31 callees (high)uninstall()— 9 callers, 5 callees (high)status()— 6 callers, 5 callees (medium)uninstall_all()— 2 callers, 13 callees (high)test_poisoned_manifest_is_healed()— 0 callers, 6 callees (medium)
Verification — 1133 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 994 function(s) in the blast radius were not formally verified this run
43eff52 to
cd98290
Compare
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR fixes handling of Python interpreter shebangs in the git post-commit hook (graphify/hooks.py) and across the generated per-agent skill files. The change strips any trailing interpreter argument (e.g. pipx's #!/.../python -E) from the parsed shebang before it's checked against the path allowlist, so the valid interpreter path is retained rather than falling back to python3. The same logic is applied consistently across all the skill-*.md files, the skillgen tool fragments/expected outputs, and a CHANGELOG entry (#2629) is added. Tests in test_hooks.py are also touched, presumably to cover the new shebang-argument stripping behavior. Reviewers should check that the shell parameter-expansion edits are duplicated identically across every skill file and that the generated/expected skillgen fixtures stay in sync with the source templates.
No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1166 functions depend on the 1017 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 117 callees - new:
install()— 33 callers, 7 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 1166 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1024 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
cd98290 to
ff8aa7a
Compare
|
Updated this PR at What changed since the previous version:
Validation:
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Fixes the git post-commit hook and skill interpreter probes to strip a shebang argument (#!/.../python -E from pipx) before treating it as a path, so the valid interpreter no longer trips the character allowlist and silently falls back to a python3 without graphify. The in-repo .graphify_python resolution now also verifies the chosen interpreter can import graphify and drops to python3 only when it can't, applied consistently across graphify/hooks.py and every generated skill file.
Worth a look
- env -S shebang can be accepted as interpreter path —
graphify/hooks.py:88· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- env -S shebangs are parsed as the env option instead of the interpreter —
graphify/hooks.py:90· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- tr -d '#!' strips all '#' and '!' chars from shebang, corrupting env-based paths —
graphify/skill-copilot.md:78· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Valid env -S shebangs are parsed as the interpreter name '-S' —
graphify/skill-copilot.md:79· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Graphify python guard verifies PATH-controlled interpreter —
tools/skillgen/fragments/shell/interpreter-guard-posix.md:12· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1580 functions depend on the 1352 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 122 callees - new:
install()— 33 callers, 7 callees - new:
render()— 13 callers, 5 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
audit_coverage()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
main()— 3 callers, 11 callees - …and 4 more — each is listed as a finding
Verification — 1580 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1424 function(s) in the blast radius were not formally verified this run
· 12 more finding(s) on lines outside this diff (see the check run).
…y-Labs#2629) pipx writes the graphify launcher shebang with an argument (`#!/.../python -E`). The interpreter probes used the whole shebang as a path; the space tripped the character allowlist, the valid interpreter was discarded, and the probe silently fell back to a `python3` without graphify — printing "could not locate a Python with graphify installed" after every commit and leaving the graph stale. Strip any interpreter argument (and resolve `/usr/bin/env python`) before using the shebang as a path, in the post-commit hook probe and both skill interpreter probes (regenerated via skillgen).
Extend the pipx shebang-argument fix to aider/devin monolith sources and the Devin subcommand guard, verify stripped guard candidates before writing .graphify_python, and move the changelog entry to Unreleased. RED->GREEN: red-devin-second-guard.log fails when the Devin guard fix is removed; green-review-blockers6.log passes 6 targeted tests. Full run-ci final5 matches baseline failure identities with 5110 passes vs 5106 baseline.
Handle /usr/bin/env -S shebang launchers, strip only the leading shebang marker, and reject env-derived bare interpreter names before verification. Regenerate skill artifacts and tighten the monolith drift guard coverage.
ff8aa7a to
d3b92f9
Compare
|
Updated this PR at What changed since the previous version:
Validation:
|
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Fixes the git post-commit hook and every skill interpreter probe to derive the Python path from a graphify shebang correctly: they strip a shebang argument (so pipx's #!/.../python -E resolves instead of tripping the path allowlist), parse /usr/bin/env -S ... launchers alongside plain /usr/bin/env, and reject bare PATH-controlled env names like python so an unverified interpreter can't be trusted. When the shebang yields no usable path, the skills fall back to python3 and verify import graphify before caching it in .graphify_python, ending the "could not locate a Python with graphify installed" message printed after every commit (#2629).
No blocking issues surfaced. 15 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1674 functions depend on the 1503 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 123 callees - new:
install()— 33 callers, 7 callees - new:
render()— 13 callers, 5 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
audit_coverage()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
main()— 3 callers, 11 callees - …and 4 more — each is listed as a finding
Verification — 1674 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 1510 function(s) in the blast radius were not formally verified this run
· 12 more finding(s) on lines outside this diff (see the check run).
1 participant
Summary
Fixes #2629.
pipx launchers can put an interpreter argument in the shebang, for example
#!/Users/me/.local/pipx/venvs/graphifyy/bin/python -E. The hook and skill interpreter probes were reading the whole shebang as if it were only a path. The trailing-Eintroduced a space, tripped the allowlist, and made the probes fall back to a barepython3that often does not have graphify installed.That produced the reporter's post-commit warning:
Fix
#!marker and then strip shebang arguments before validating launcher interpreter paths./usr/bin/env -S ...and/usr/bin/env ...launchers.pythonbefore verification, avoiding PATH-controlled interpreter execution; explicit fallback probing still handlespython3/python.esac/fidrift.graphify/skill*.mdandtools/skillgen/expected/*from the skillgen sources.Unreleased.Test verification (RED -> GREEN)
Original PR regression proof for #2629:
Additional RED proof for Graphify review advisories:
Focused GREEN validation on the fixed tree:
Targeted commands run:
Coverage:
Note: an earlier ad-hoc line coverage helper (
diff-coverage-python-coverable-3.log) was superseded because it treated shell lines inside Python string constants and docstring text as Python executable lines. The AST-aware gate above is the retained Python coverage check.Final local full replay (
./run-ci.sh) compared against an isolatedorigin/v8baseline:The four local failures are unchanged from baseline:
Install smoke and all skillgen validators pass (
--check,--audit-coverage,--schema-singleton,--monolith-roundtrip,--always-on-roundtrip).