Fix/2.5.3 portable python discovery - #46
Merged
Merged
Conversation
A venv's Scripts\pythonw.exe is a stub that re-execs the interpreter named in pyvenv.cfg. A 3.13 -> 3.14 upgrade (PSF Python Manager) uninstalls that interpreter but leaves the stub, so ResolvePythonwPath_Impl's bare FileExist() check still passed and every hotkey spawned a dead launcher that hung on a modal "Python venv launcher is sorry to say ..." dialog. The only rung below it was the bare name pyw.exe, which Python Manager does not ship at all, so removing the stale venv would have failed a second time for a different reason. install.ps1 carried the same existence-is-health assumption and reported "venv already present" while repairing nothing. Discovery now shares one rung order between the app and the installer: GRAMMARFIX_PYTHONW -> venv (only while pyvenv.cfg's base exists) -> PEP 514 registry (3.11+, newest, HKCU before HKLM) -> pyw.exe/pythonw.exe resolved against PATH by hand. A candidate must exist and be non-zero bytes, which rejects the WindowsApps App Execution Alias stubs that shadow real installs. Venv health is a static pyvenv.cfg read, never a spawn: running the stub to discover it is dead is what raised the dialog. install.ps1 rebuilds an unhealthy venv rather than reporting success. SPEC V68 + B57. New tests/test_pythonw_discovery.ahk pins the behaviour (dead venv, zero-byte alias stub, live registry lookup); the .py companion keeps the two implementations from drifting. Co-Authored-By: Claude Opus 5 <[email protected]>
FLM 1.0.5 was installed, working, and its directory was on the machine PATH -- but the app reported it absent: doctor said `fastflowlm_cli: not found` / `flm_validate: flm CLI not in PATH`, and every hotkey failed. A process only ever sees the environment block built when its session started, and Flowkey launches at logon, so an FLM installed afterwards stays invisible until the user signs out. Nothing was broken except the lookup; `flm version` answered fine from any new shell. subprocess_util gains resolve_exe/resolve_cli: PATH, then PATH as the registry currently holds it, then the CLI's known install directories. All nine provider-CLI call sites now pass an absolute argv[0], and provider_status shares the same resolver so detection cannot disagree with execution. Passing env= with a repaired PATH is NOT sufficient on its own, which cost a round trip to discover: Windows resolves a bare argv[0] against the PARENT process's PATH, so the first fix turned `fastflowlm_cli` green while model_installed and flm_validate kept failing. argv[0] itself has to be absolute. grammar_fix's `flm validate` also gains env=flm_env(); it was the only flm call site running on the raw inherited environment, so it silently missed the B54 FLM_MODEL_PATH repair too. SPEC V69 + B58. New tests/test_cli_discovery.py (11 cases). Three existing tests asserted a bare argv[0] and are pinned to the bare name, so they test the CLI contract rather than what happens to be installed on the machine running them. Live-verified with `flm` unresolvable in the launching shell: doctor all-green (model_installed: yes, flm_validate: ready=True) and a real grammar fix end-to-end. Co-Authored-By: Claude Opus 5 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c17aa3e90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
PR #46 went red on `daemon_client.ahk: braces 54 / 55` for code AHK itself parses clean. The gate stripped comments first (`;.*$` per line) and strings second, so Loop Parse EnvGet("PATH"), ";" { was truncated at the semicolon *inside the string literal*, taking that line's opening brace with it. A false positive on valid source, and it would fire for any `;` in a literal. Swap the order: strings first, then comments. Verified across all ten .ahk files -- every pre-existing count is unchanged (grammarFix 84/84, tray 34/34, clipboard 18/18, ...) and daemon_client balances at 55/55. The step was extracted from ci.yml and executed verbatim: exit 0. Fixed the gate rather than reshaping the source to suit it. Same family as B48: a brace/comment heuristic mis-reading syntax it never parses. Co-Authored-By: Claude Opus 5 <[email protected]>
…gh the resolver Both findings from the automated review on PR #46 are correct. 1. ResolvePythonwPath_Impl cached the resolved interpreter for the whole session and never rechecked it. Flowkey autostarts at logon, so its sessions are exactly the long-lived ones where Python gets upgraded underneath it -- and a cache that never revalidates pins the dead path until AHK restarts, reintroducing B57. The cache now validates on read using file stats only (never a spawn, so a dead venv still cannot raise its own modal dialog), and walks up from <venv>\Scripts\pythonw.exe to re-check pyvenv.cfg, which also covers a venv handed to us via GRAMMARFIX_PYTHONW. 2. install.py's _has_cmd was still shutil.which, and it guards every FLM path -- so it rejected `flm` before resolve_cli could ever run, and postreboot() opened the FastFlowLM download page for an FLM that was already installed. Detection disagreeing with execution is the exact thing V69 forbids, and I introduced it by converting the argv in that file while leaving the guard alone. Caught while testing: the AHK fixture wrote a 0-byte venv stub, so the alias-stub veto fired before the pyvenv.cfg check and the new dead-venv assertions would have passed for the wrong reason. The live-venv case failing is what exposed it; the fixture now writes real content. Gates: ruff clean, 579 passed, 3 AHK suites exit 0, brace gate exit 0 (daemon_client 56/56). Co-Authored-By: Claude Opus 5 <[email protected]>
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.
No description provided.