fix(@deepnote/cli): use correct venv via --python#312
Merged
dinohamzic merged 8 commits intomainfrom Feb 23, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 83.39% 83.47% +0.07%
==========================================
Files 114 114
Lines 6938 6970 +32
Branches 1913 1862 -51
==========================================
+ Hits 5786 5818 +32
Misses 1152 1152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughCLI commands (analyze, dag, lint, run) now resolve Python interpreter paths via resolvePythonExecutable and pass resolved executables to downstream calls. runtime-core adds buildPythonEnv and isBareSystemPython; buildPythonEnv detects venv roots, prepends the venv bin to PATH, and sets or clears VIRTUAL_ENV. server-starter uses buildPythonEnv to construct the environment for spawned processes. Tests and mocks updated to exercise resolver and env-building. cspell.json adds "pyproject", "pyvenv", and "venvs". Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Resolver as resolvePythonExecutable
participant Builder as buildPythonEnv
participant Runner as Analysis/Lint/Run/Server
participant Process as Spawned Process
CLI->>Resolver: resolvePythonExecutable(pythonPath)
activate Resolver
Resolver-->>CLI: resolvedPythonPath
deactivate Resolver
CLI->>Builder: buildPythonEnv(resolvedPythonPath, baseEnv)
activate Builder
Builder->>Builder: detectVenvRoot(resolvedPythonPath)
Builder-->>CLI: builtEnv (PATH, VIRTUAL_ENV)
deactivate Builder
CLI->>Runner: invoke with resolvedPythonPath + builtEnv
activate Runner
Runner->>Process: spawn/use environment (PATH, VIRTUAL_ENV)
Runner-->>CLI: result/exit
deactivate Runner
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cspell.json (1)
108-114: 🧹 Nitpick | 🔵 TrivialRemove duplicate "pyproject" entry.
The word list includes "pyproject" twice, which is redundant.
Proposed fix
- "pyproject", - "pyvenv", + "pyvenv", "pymongo", @@ - "pyproject", + "pyproject",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cspell.json` around lines 108 - 114, The cspell.json word list contains a duplicate "pyproject" entry; remove one of the two "pyproject" strings from the array to eliminate redundancy (edit the words array where "pyproject" appears, keeping a single occurrence and ensuring the JSON array remains valid and correctly comma-separated).packages/runtime-core/src/server-starter.test.ts (1)
16-28:⚠️ Potential issue | 🟡 MinorMake mock's path handling OS-agnostic to match the actual implementation.
The mock uses POSIX regex patterns and
:delimiter, while the realbuildPythonEnvinpython-env.tsis platform-aware (usinggetPathDelimiter()andnode:pathutilities). Update the mock to usedirname()anddelimiterfromnode:path, and update test assertions to usepath.join()for portable path expectations.Suggested adjustment
+import { delimiter, dirname, join } from 'node:path' ... - buildPythonEnv: vi.fn(async (resolvedPath: string, baseEnv: Record<string, string | undefined> = {}) => { + buildPythonEnv: vi.fn(async (resolvedPath: string, baseEnv: Record<string, string | undefined> = {}) => { const env = { ...baseEnv } if (resolvedPath !== 'python' && resolvedPath !== 'python3') { - const binDir = resolvedPath.replace(/\/python[^/]*$/, '') - env.PATH = `${binDir}:${env.PATH || ''}` - env.VIRTUAL_ENV = binDir.replace(/\/bin$/, '') + const binDir = dirname(resolvedPath) + env.PATH = env.PATH ? `${binDir}${delimiter}${env.PATH}` : binDir + env.VIRTUAL_ENV = dirname(binDir) } return env }),- expect(env.PATH).toContain('/path/to/venv/bin') - expect(env.VIRTUAL_ENV).toBe('/path/to/venv') + expect(env.PATH).toContain(join('/path', 'to', 'venv', 'bin')) + expect(env.VIRTUAL_ENV).toBe(join('/path', 'to', 'venv'))Also applies to: 113-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/server-starter.test.ts` around lines 16 - 28, The mock in server-starter.test.ts incorrectly assumes POSIX paths; update the vi.mock implementation for resolvePythonExecutable and buildPythonEnv to use node:path utilities: import dirname and delimiter (or getPathDelimiter) and use dirname(resolvedPath) to compute binDir and delimiter for PATH concatenation so path handling is OS-agnostic; then update the related assertions (around the spots mentioned and also lines 113-129) to construct expected paths with path.join() and expected PATH values using path.delimiter so tests pass on Windows and POSIX.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/python-env.test.ts`:
- Around line 4-6: Tests hardcode the PATH delimiter (':') causing failures on
Windows; update python-env.test.ts to import delimiter from 'node:path' and use
it when constructing expected PATH strings. Locate uses in the test file where
PATH is built/compared (see tests exercising buildPythonEnv and
resolvePythonExecutable expectations) and replace hardcoded ':' joins with
Array.join(delimiter) so expectations use the platform-specific path.delimiter;
ensure the imported symbols include delimiter alongside join/resolve.
In `@packages/runtime-core/src/python-env.ts`:
- Around line 171-188: In buildPythonEnv, the PATH manipulation uses env.PATH
which fails on Windows where the key may be "Path"; find the existing PATH key
case-insensitively (e.g., pathKey = Object.keys(env).find(k => k.toLowerCase()
=== 'path') || 'PATH') and use that pathKey when reading and writing the value;
then prepend pythonDir using getPathDelimiter() to env[pathKey] (preserving an
empty currentPath) so subprocesses see the correct Python, and keep early-return
behavior for resolvedPythonPath === 'python' or 'python3'.
---
Outside diff comments:
In `@cspell.json`:
- Around line 108-114: The cspell.json word list contains a duplicate
"pyproject" entry; remove one of the two "pyproject" strings from the array to
eliminate redundancy (edit the words array where "pyproject" appears, keeping a
single occurrence and ensuring the JSON array remains valid and correctly
comma-separated).
In `@packages/runtime-core/src/server-starter.test.ts`:
- Around line 16-28: The mock in server-starter.test.ts incorrectly assumes
POSIX paths; update the vi.mock implementation for resolvePythonExecutable and
buildPythonEnv to use node:path utilities: import dirname and delimiter (or
getPathDelimiter) and use dirname(resolvedPath) to compute binDir and delimiter
for PATH concatenation so path handling is OS-agnostic; then update the related
assertions (around the spots mentioned and also lines 113-129) to construct
expected paths with path.join() and expected PATH values using path.delimiter so
tests pass on Windows and POSIX.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cspell.json (1)
107-115:⚠️ Potential issue | 🟡 MinorRemove duplicate “pyproject” entry.
Line 114 already contains “pyproject”, so the new addition duplicates it. Keep a single entry to reduce noise.
🧩 Proposed fix
@@ - "pyproject", "PYTHONPATH",Also applies to: 147-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cspell.json` around lines 107 - 115, Remove the duplicate "pyproject" entries from the cspell word list so each spelling entry appears only once: find the array containing "pyformat", "pyproject", "pyvenv", ... (the list that currently has "pyproject" twice) and delete the extra "pyproject" string; also remove the duplicate "pyproject" occurrence in the other list referenced (the second occurrence noted around the later block) so the "pyproject" key appears exactly once across the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/server-starter.test.ts`:
- Around line 20-28: The test mock buildPythonEnv hardcodes the PATH delimiter
as ':' which breaks on Windows; update the mock in
packages/runtime-core/src/server-starter.test.ts to use Node's path.delimiter
(imported from 'path') when constructing env.PATH and when splitting/joining
PATH values, keeping the existing logic in buildPythonEnv and preserving adding
binDir and setting VIRTUAL_ENV.
---
Outside diff comments:
In `@cspell.json`:
- Around line 107-115: Remove the duplicate "pyproject" entries from the cspell
word list so each spelling entry appears only once: find the array containing
"pyformat", "pyproject", "pyvenv", ... (the list that currently has "pyproject"
twice) and delete the extra "pyproject" string; also remove the duplicate
"pyproject" occurrence in the other list referenced (the second occurrence noted
around the later block) so the "pyproject" key appears exactly once across the
file.
---
Duplicate comments:
In `@packages/runtime-core/src/python-env.test.ts`:
- Around line 250-256: The test hardcodes ':' as the PATH delimiter which fails
on Windows; update the test that calls buildPythonEnv to import/use
path.delimiter and construct expected PATH with that delimiter (use
resolve(binDir) + path.delimiter + baseEnv.PATH or join parts with
path.delimiter) instead of ':' so the assertion on env.PATH works
cross-platform; reference the buildPythonEnv call, resolvedBinDir
(resolve(binDir)), baseEnv, and path.delimiter when making the change.
In `@packages/runtime-core/src/python-env.ts`:
- Around line 186-187: The code assumes env.PATH exists but on Windows the key
may be 'Path'; update the logic to detect and use the correct casing instead of
always using 'PATH'. In the block where you build currentPath and set env.PATH,
determine the actual key first (e.g., const pathKey =
Object.prototype.hasOwnProperty.call(env, 'Path') ? 'Path' : 'PATH'), then use
env[pathKey] when reading/updating the value so pythonDir and getPathDelimiter()
are prepended to the existing PATH regardless of casing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cspell.jsonpackages/runtime-core/src/python-env.test.tspackages/runtime-core/src/python-env.tspackages/runtime-core/src/server-starter.test.ts
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/server-starter.test.ts`:
- Around line 20-29: The mock buildPythonEnv function appends the pathDelim
unconditionally, which yields a trailing separator when baseEnv.PATH is empty;
update the mock (buildPythonEnv) to mirror production logic by only prefixing
pathDelim when env.PATH is truthy (i.e., set env.PATH = binDir + pathDelim +
env.PATH if env.PATH exists, otherwise env.PATH = binDir), keeping the existing
resolvedPath check and VIRTUAL_ENV assignment unchanged.
dinohamzic
changed the title
OlegWock
requested changes
Feb 23, 2026
| } | ||
|
|
||
| const candidates = process.platform === 'win32' ? PYTHON_EXECUTABLES_WIN : PYTHON_EXECUTABLES_UNIX | ||
| const candidates = PYTHON_EXECUTABLES |
Contributor
There was a problem hiding this comment.
This variable is now obsolete
Contributor
There was a problem hiding this comment.
This fails to handle arguments like python3.11. If we support passing python/python3, I think we should support those too
| const env = { ...baseEnv } | ||
|
|
||
| // For bare commands ('python', 'python3'), inherit the current environment as-is | ||
| if (resolvedPythonPath === 'python' || resolvedPythonPath === 'python3') { |
Contributor
There was a problem hiding this comment.
Would be nice to move this into separate const array or helper function
Contributor
Author
|
@OlegWock great feedback, fixed 🙏 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/runtime-core/src/python-env.test.ts (1)
263-298:⚠️ Potential issue | 🟡 MinorUse
delimiterto build PATH strings in tests.A couple of PATH expectations still hardcode
:, which will fail on Windows. Usedelimiterto construct baseEnv and expectations.Suggested fix
- it('prepends python directory to PATH for absolute path', async () => { - const baseEnv = { PATH: '/usr/bin:/usr/local/bin' } + it('prepends python directory to PATH for absolute path', async () => { + const baseEnv = { PATH: ['/usr/bin', '/usr/local/bin'].join(delimiter) } const env = await buildPythonEnv(pythonPath, baseEnv) const resolvedBinDir = resolve(binDir) - expect(env.PATH).toBe(`${resolvedBinDir}${delimiter}/usr/bin:/usr/local/bin`) + expect(env.PATH).toBe([resolvedBinDir, '/usr/bin', '/usr/local/bin'].join(delimiter)) }) @@ - it('replaces inherited VIRTUAL_ENV with correct venv root', async () => { - const baseEnv = { PATH: '/other/venv/bin:/usr/bin', VIRTUAL_ENV: '/other/venv' } + it('replaces inherited VIRTUAL_ENV with correct venv root', async () => { + const baseEnv = { PATH: ['/other/venv/bin', '/usr/bin'].join(delimiter), VIRTUAL_ENV: '/other/venv' } const env = await buildPythonEnv(pythonPath, baseEnv)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/python-env.test.ts` around lines 263 - 298, Tests in python-env.test.ts hardcode ':' when building PATH strings which breaks on Windows; update the tests that set baseEnv and expect env.PATH to use the imported delimiter variable instead. Concretely, for the cases around buildPythonEnv (tests named "prepends python directory to PATH for absolute path", "sets VIRTUAL_ENV when python is inside a venv", "clears inherited VIRTUAL_ENV when python is NOT in a venv", and "replaces inherited VIRTUAL_ENV with correct venv root") replace literal ':' joins like '/usr/bin:/usr/local/bin' or '/other/venv/bin:/usr/bin' with template strings using delimiter (e.g. `/usr/bin${delimiter}/usr/local/bin`), and when building the RegExp for PATH in the last test ensure you join parts with delimiter rather than ':' so the expectation is platform-independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/python-env.ts`:
- Line 8: BARE_PYTHON_COMMAND currently doesn't match Windows executables like
"python.exe" or "python3.exe"; update the regex used in python-env.ts
(BARE_PYTHON_COMMAND) to accept an optional ".exe" suffix (and still allow
versions like "python3.9.exe") so that CLI values like "--python python.exe" are
recognized as commands rather than as file paths; adjust the pattern accordingly
to be Windows-aware while preserving existing behavior for Unix names (e.g.,
allow /^python[0-9.]*(\.exe)?$/i or equivalent).
---
Duplicate comments:
In `@packages/runtime-core/src/python-env.test.ts`:
- Around line 263-298: Tests in python-env.test.ts hardcode ':' when building
PATH strings which breaks on Windows; update the tests that set baseEnv and
expect env.PATH to use the imported delimiter variable instead. Concretely, for
the cases around buildPythonEnv (tests named "prepends python directory to PATH
for absolute path", "sets VIRTUAL_ENV when python is inside a venv", "clears
inherited VIRTUAL_ENV when python is NOT in a venv", and "replaces inherited
VIRTUAL_ENV with correct venv root") replace literal ':' joins like
'/usr/bin:/usr/local/bin' or '/other/venv/bin:/usr/bin' with template strings
using delimiter (e.g. `/usr/bin${delimiter}/usr/local/bin`), and when building
the RegExp for PATH in the last test ensure you join parts with delimiter rather
than ':' so the expectation is platform-independent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/runtime-core/src/python-env.test.tspackages/runtime-core/src/python-env.tspackages/runtime-core/src/server-starter.test.ts
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
2 participants
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.
Summary by CodeRabbit
New Features
Tests
Chores