Skip to content

refactor: install Python CLI into isolated venv instead of uv tool#1909

Merged
Saga4 merged 1 commit intomainfrom
cf-npm-isolated-venv-install
Mar 26, 2026
Merged

refactor: install Python CLI into isolated venv instead of uv tool#1909
Saga4 merged 1 commit intomainfrom
cf-npm-isolated-venv-install

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

Summary

  • Replace uv tool install (global install to ~/.local/bin) with a dedicated venv in an OS-specific cache directory (~/.cache/codeflash/venv on Linux, ~/Library/Caches/codeflash/venv on macOS, %LOCALAPPDATA%\codeflash\venv on Windows)
  • CLI entry point now invokes the codeflash binary directly from the venv instead of via uv tool run
  • Strips VIRTUAL_ENV/CONDA_PREFIX env vars from child processes to avoid interference from activated environments
  • Adds shared scripts/paths.js module for OS-specific path resolution

Test plan

  • Run npm install in packages/codeflash — verify postinstall creates venv at the expected cache path
  • Run npx codeflash --version — verify it invokes the binary from the cache venv
  • Test with an activated venv/conda env — verify it still uses the cache venv, not the activated one
  • Test on macOS/Windows if possible for path correctness

🤖 Generated with Claude Code

Replace `uv tool install` (which installs globally into ~/.local/bin) with
a dedicated venv at an OS-specific cache directory (~/.cache/codeflash/venv
on Linux, ~/Library/Caches/codeflash/venv on macOS, %LOCALAPPDATA%\codeflash\venv
on Windows). The CLI entry point now invokes the binary directly from the venv
instead of via `uv tool run`. Also strips VIRTUAL_ENV/CONDA env vars from child
processes to avoid interference from activated environments.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 26, 2026

Claude finished @mohammedahmed18's task in 2m 58s —— View job


PR Review Summary

  • Triage PR size — LARGE (267 lines across 3 JS files)
  • Run lint/typecheck checks — all changed files are JavaScript; prek skipped (Python-only), no issues
  • Resolve stale review threads — none found
  • Review code
  • Duplicate detection
  • Post summary

Prek Checks

No Python files changed; prek reported no files to check. ✓


Code Review

Bug — silent failure if verification fails

verifyInstallation() return value is discarded (postinstall.js:263). If the binary doesn't exist or --version fails, the function silently returns false but the script continues to print the success banner. The user sees "Codeflash installation complete!" even when the binary isn't working.

// Current — return value ignored
verifyInstallation();

// Should be:
if (!verifyInstallation()) {
  logWarning('Could not verify installation — run `npx codeflash --version` to test');
}

Fix this →


Issue — --clear wipes the venv on every reinstall

postinstall.js:137:

execSync(`"${uvBin}" venv --python python3.12 --clear "${venvDir}"`, ...);

--clear deletes and recreates the venv each time postinstall runs (e.g., after npm update codeflash). This forces a full Python dependency re-download on every npm package update. Consider skipping venv creation if it already exists and codeflash is already installed at the expected version:

const codeflashBin = getCodeflashBin();
if (!fs.existsSync(codeflashBin)) {
  // only create venv if binary is missing
  execSync(`"${uvBin}" venv --python python3.12 "${venvDir}"`, ...);
}

Or at minimum drop --clear so the venv is reused rather than recreated from scratch on every install.

Fix this →


Issue — uv pip install --python <venv-dir> is implicit

postinstall.js:153,170:

execSync(`"${uvBin}" pip install --python "${venvDir}" "${GITHUB_REPO}"`, ...);

Passing the venv directory to --python works in uv (it detects pyvenv.cfg) but is not the explicit/documented form. The more reliable approach is to pass the Python interpreter binary directly, which also makes cross-platform behavior explicit:

const pythonBin = os.platform() === 'win32'
  ? path.join(venvDir, 'Scripts', 'python.exe')
  : path.join(venvDir, 'bin', 'python3');

execSync(`"${uvBin}" pip install --python "${pythonBin}" "${GITHUB_REPO}"`, ...);

This avoids relying on uv's directory detection heuristic and makes it clear which Python is targeted.


Minor — step numbering is slightly off

installCodeflash is labelled [2/3], but it internally creates the venv (unlabelled logSuccess) before installing. From the user's perspective the steps are: uv check → venv create (silent/unlabelled) → install → verify. Consider either labelling the venv-creation sub-step or consolidating the count to 4 steps.


Duplicate Detection

paths.js is new and centralises path logic previously duplicated between postinstall.js and bin/codeflash.js. No duplicate logic found elsewhere in the repo. ✓


Test Coverage

JavaScript files only — Python coverage tooling does not apply. The PR test plan (manual) is clear and comprehensive.


Last updated: 2026-03-26

Copy link
Copy Markdown
Contributor

@aseembits93 aseembits93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, windows could possibly an issue, let's see

@Saga4 Saga4 merged commit 45203b5 into main Mar 26, 2026
27 of 29 checks passed
@Saga4 Saga4 deleted the cf-npm-isolated-venv-install branch March 26, 2026 18:20
aseembits93 added a commit that referenced this pull request Mar 30, 2026
Follow-up to #1909 — the npm package now installs into a dedicated venv
instead of using uv tool, but these references were missed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

3 participants