fix(cookbook): activate local Windows venv in bash runner - #5734
Conversation
|
CI note: the Python tests job is failing during collection before this PR's Cookbook code runs: src/agent_loop.py uses Any without importing it. I opened #5735 as a separate one-line fix for that upstream dev break. Once that lands, this PR can be rebased cleanly. |
56004fd to
cd57f07
Compare
cd57f07 to
6b48a97
Compare
o3LL
left a comment
There was a problem hiding this comment.
Reviewed at 6b48a97.
The direction is right and the guard is correctly scoped, but I don't think this
fix reaches the paths it's aiming at. Details below.
Findings
P0 issue: the conversion never fires on the main serve and download paths
-
Problem:
_local_windows_bash_env_prefixrecovers the Windows path with
shlex.split(ep, posix=True). That only works when the frontend single-quoted
the path. Three emitters quote through_psQuote, but the two primary ones
don't:static/js/cookbookRunning.js:1974in_launchServeTask— the main serve
launcher, called from the Serve panel (cookbookServe.js:3712), every retry
and auto-fix path, and the pip-reinstall shortcut.static/js/cookbookDownload.js:541in_runModelDownload— the model
Download button.
Both emit
'& ' + envPathwith no quoting at all. In posix modeshlextreats
the backslashes as escapes, soparts[1]comes back as
C:UsersdomodysseusvenvScriptsActivate.ps1, the[/\\]Activate\.ps1$guard
fails to match, and the function returnsepunchanged.Running the exact call-site expression
(_safe_env_prefix(_local_windows_bash_env_prefix(ep))) on this branch:& 'C:\Users\dom\odysseus\venv\Scripts\Activate.ps1' (_psQuote emitters) -> [ -f "/c/Users/dom/odysseus/venv/Scripts/activate" ] && source "/c/Users/dom/odysseus/venv/Scripts/activate" || true & C:\Users\dom\odysseus\venv\Scripts\Activate.ps1 (cookbookRunning.js:1974, cookbookDownload.js:541) -> & 'C:UsersdomodysseusvenvScriptsActivate.ps1' -
Impact: On local Windows, launching a serve from the Serve panel or
starting a download from the Download button still writes a PowerShell line
into the bash runner — with the path corrupted on top. That is the same
"venv never activates, packages look missing" symptom #2710 reports, so the
bug survives this PR on the two flows most people use. The three flows that
do get fixed (dep install, quick-download box, download retry) are the
secondary ones. -
Ask: Quote at the two emitters so they match the other three
('& ' + _psQuote(...)), and also make the helper tolerant of the unquoted
form rather than depending on the caller — it sits in front of the
_safe_env_prefixvalidator, so it shouldn't assume its input is well-formed.
A raw-string match on^&\s*'?(.+?\.ps1)'?$before falling back toshlex
would cover both shapes. -
Location:
routes/cookbook_helpers.py:1212,
static/js/cookbookRunning.js:1974,static/js/cookbookDownload.js:541
P1 issue: an unquoted path containing a space fails the request outright
-
Problem: Same root cause, different symptom. With
& C:\Users\dom\My Envs\venv\Scripts\Activate.ps1, posixshlexyields three
tokens, so_safe_env_prefixraisesHTTPException(400, "Invalid env_prefix")
and the launch never starts. Confirmed on this branch. -
Impact: Pre-existing rather than introduced here, but it lives on exactly
the code path this PR is fixing and any venv underC:\Users\<name>\My Documents\...or a "Program Files"-style directory hits it. Quoting at the
emitters (finding 1) removes it; leaving the emitters alone leaves a hard 400
behind after this merges. -
Location:
routes/cookbook_helpers.py:1212
P2 suggestion: this is the fourth copy of the drive-letter conversion
-
Problem:
_git_bash_path()atroutes/cookbook_helpers.py:57already does
C:\...->/c/...including the backslash rewrite, is platform-independent
(so it works under pytest on Linux/macOS), and is already used further up the
same file at line 170.core/platform_compat.py:219has another copy, which
cookbook_routes.pyalready imports from. This adds a fourth, inline. -
Impact: Four implementations of one Windows-path rule means four places to
touch if the mount-root assumption ever needs to change, and they already
disagree — the new one rewrites a UNC path to//nas/share/...while
_git_bash_pathreturns it untouched with backslashes intact. -
Ask: Reuse
_git_bash_pathand do theActivate.ps1->activaterename
around it. If UNC support is the reason for the local copy, please extend
_git_bash_pathinstead so there's one converter. -
Location:
routes/cookbook_helpers.py:1220-1224,routes/cookbook_helpers.py:57
P2 issue (test): the one test covers only the shape that already worked
-
Problem: The new test asserts the quoted, drive-lettered, space-free case.
Not covered: the unquoted form the two main emitters actually send (which
would have caught finding 1), spaces, forward-slash input, UNC, and every
pass-through case —source /home/.../activate,conda activate,
eval "$(conda shell.bash hook)" && ...,None,"". The PR body's claim
that remote Windows keeps the PowerShell path is also unasserted. -
Impact: The suite passes while the primary flow stays broken, which is how
finding 1 got this far. -
Ask: Add the unquoted-input case, plus a handful of pass-through
assertions so a future edit can't silently start rewriting POSIX or conda
prefixes. -
Location:
tests/test_cookbook_helpers.py:111
P3 nit (non-blocking): a wrong venv path now fails silently
-
Problem: After conversion,
_safe_env_prefixwraps the result as
[ -f "<path>" ] && source "<path>" || true, so a typo'd or moved venv
activates nothing and the run continues against the system interpreter with no
message in the log. -
Impact: That is indistinguishable from the bug this PR fixes, and #2710 is
a report of exactly that confusion. It matches existing POSIX behaviour, so
it's not a regression — just a missed chance to make the next report easier. -
Ask: Optional:
|| echo "venv not found at <path>"on the miss branch. -
Location:
routes/cookbook_helpers.py:1204
Open Questions
-
question (scope, non-blocking): Local Windows conda has the same problem —
cookbookRunning.js:1976andcookbookDownload.js:543emit
conda activate C:\path\to\envunquoted, and_safe_env_prefix's conda branch
passes it straight into the bash runner. Deliberately out of scope for a
venv-only fix, which is fine, but is that a follow-up you're planning? -
question (non-blocking):
/c/is the Git-for-Windows and MSYS2 mount root,
andfind_bash()normally lands on Git Bash, so this is right in practice. It
does resolveshutil.which("bash")first though, and only rejects the WSL /
Store stubs — a Cygwin bash earlier on PATH would want/cygdrive/c. Same
assumption as the existing helpers, so I'm not asking for a change; flagging it
as a known edge if Cookbook-on-Windows reports keep coming in. -
question: Did you get to run this on the Windows host? The "I actually ran
the app" box is unticked and "How to Test" reads as instructions rather than a
result. I ask because finding 1 means a launch from the Serve panel or the
Download button would still have failed, so a real end-to-end run should have
surfaced it.
Validation
-
Ran: macOS, Python 3.11.15, worktree at
refs/pull/5734/head(6b48a97).
Full suitepython -m pytest -q: 2 failed, 5037 passed, 4 skipped in
139.44s — both failures are the known environment-dependent ones
(test_workspace_confine.py::test_glob_confined_e2e,/tmpresolving to
/private/tmp;test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second,
real-socket timing) and neither touches this change. The subset from your
"How to Test": 94 passed, 1 skipped.compileallon both touched Python files:
clean. Booted the app (uvicorn app:appon 127.0.0.1:7099) —/api/health,
/and/cookbookall 200. Then drove
_safe_env_prefix(_local_windows_bash_env_prefix(ep))— the literal expression
at both call sites — over the six payload shapes the five frontend emitters
produce; that's where findings 1 and 2 come from. -
Not run: No native Windows host here, so nothing exercised
find_bash(),
_launch_local_detached, or an actual Git Bashsource. The/c/mount root
and the activation itself are unverified end-to-end — my evidence is
string-level plus reading the call sites. I also didn't run the remote-Windows
path; I confirmed by reading thatlocal_windows = IS_WINDOWS and not remote
and that remote Windows takes the separateps_linesbranch above
cookbook_routes.py:2136, so it never reaches the changed line. No JS in the
diff, so nonode --check. -
Residual risk: Low for what the diff changes — it's guarded, and POSIX and
remote Windows are provably untouched, so merging it can't break anything that
works today. The risk is that it reads as fixing #2710 without fixing it on
the two primary flows, and the issue gets closed on that basis.
PR Hygiene
-
Target/template/checks: Targets
dev, one focused change, no drive-by
edits, no UI so the screenshot requirement is correctly N/A. Title passes
Conventional Commits and the description check is green;ready for reviewis
on.Part of #2710is the right linkage — #2710 is broader than this and
shouldn't auto-close. The one gap is the unticked "I actually ran the app" box
(see Open Questions). Your CI note aboutsrc/agent_loop.pyis stale now —
#5735 merged on 2026-07-27 and the run on 2026-08-13 is green. -
Related, duplicate, or superseding context: #4894 is closed unmerged
(2026-07-23), so superseding it is accurate. Searched open PRs for
cookbook/env_prefix and cookbook/windows/venv overlap — this is the only one.
The branch is 5 commits behinddevand nothing ondevhas touched
cookbook_routes.py,cookbook_helpers.pyortests/test_cookbook_helpers.py
since the base, so it still rebases clean and GitHub reports it mergeable.
6b48a97 to
01c1192
Compare
01c1192 to
85ad875
Compare
|
Fixed in 85ad875 and rebased onto current dev. Addressed the review findings:
Validation: focused regression suite 15 passed; Python and JavaScript syntax passed; full CI, CodeQL, and Trivy are green. Ready for re-review. |
|
@o3LL Runtime and visual evidence are now posted. Native Windows validation on rebased local head
The requested emitter, parser, converter-reuse, and regression changes remain in |
|
@o3LL Additional native-Windows reproduction confirms the exact base-branch failure this PR fixes. While validating #6106 against current That is the PowerShell activation prefix reaching the Bash runner unchanged. #5734 converts this path before Bash execution. The reproduction used The requested emitter quoting, tolerant quoted/unquoted parsing, shared |
0931c38 to
f0e09d9
Compare
f0e09d9 to
a0e61a4
Compare
o3LL
left a comment
There was a problem hiding this comment.
Approving at a0e61a45, rebased onto 032967af.
All five findings from 6b48a97 are addressed, and two of them better than I asked
for: the helper now accepts the unquoted form itself rather than depending on the
emitters, and the regex I suggested was replaced with a linear parser after CodeQL
flagged the backtracking.
The Serve capture answers the one thing I couldn't check from macOS. I was unsure
whether sourcing CPython's bash activate would survive PATH="$VIRTUAL_ENV/"Scripts":$PATH"
under Git Bash, since POSIX splitting on : should break that string apart. pip ... from G:\AI\odysseus\venv\Lib\site-packages says it resolves correctly in practice,
which is the fact the whole approach rests on. Thanks for going and getting it.
Validation
- Ran: re-verified the delta end-to-end on macOS — the real
_psQuoteoutput from
the running page piped into the backend helper, round-tripping paths with spaces and
apostrophes; injection attempts (;,&&,$(...), embedded newline) all land on 400.
Full suite green apart from two known macOS environment failures. Confirmed working on a Windows host with Git Bash - Residual risk: the venv path containing a space is covered by the regression tests
but not by a native run, as you noted yourself.
a0e61a45 is patch-identical to 85ad875c — same five blobs — so the rebase doesn't
change what I reviewed.
Summary
Local Windows Cookbook serve/download tasks run through detached Git Bash, while the frontend builds PowerShell venv activation prefixes such as
& C:\path\Scripts\Activate.ps1. The primary Serve and Download emitters previously left those paths unquoted. POSIX parsing then stripped backslashes or split paths containing spaces, so the selected venv never activated.This PR:
source /c/.../Scripts/activatecommands before the existing env-prefix validator runs._git_bash_pathconverter and rejects unsupported or unsafe input shapes.Remote Windows, POSIX, and conda prefixes remain unchanged. Supersedes #4894, which was closed after
devwas force-updated without absorbing this fix.Target branch
dev, notmain. All PRs land indev;mainis curated by the maintainer at each release.Linked Issue
Part of #2710; supersedes #4894
Type of Change
Checklist
devuvicorn app:app) and verified the primary native Windows Download and Serve paths.How to Test
Native Windows runtime validation was performed on rebased local head
254f0aff. Its change patch has the same Git patch ID as PR head85ad875c.uvicorn app:appon127.0.0.1:7099.G:\AI\odysseus\venv.hf-internal-testing/tiny-random-gpt2completed and the API returned 200.python -m pip --version; pip resolved fromG:\AI\odysseus\venv\Lib\site-packagesand the process exited 0.Automated validation performed:
node --checkfor both changed JavaScript modules passed.Residual gap:
G:\AI\odysseus\.runtime-validation\Venv With Spacewas configured in the native Windows UI, but that second path was not executed end-to-end. Quoted and unquoted paths containing spaces remain covered by the focused regression suite.Visual / UI changes - REQUIRED if you touched anything that renders
No rendered layout, CSS, icon, typography, or component behavior changed.
static/js/cookbookRunning.jsandstatic/js/cookbookDownload.jsonly quote command payload values before requests are sent.Screenshots / clips
The capture shows the primary Download task completed and the primary Serve task resolving pip from the configured local Windows venv before exiting 0. GitHub-hosted evidence source.