Skip to content

Python: Fix venv isolation from user-site and base-install site-packages [STUD-81085] - #596

Open
viogroza wants to merge 4 commits into
developfrom
fix/python_venv
Open

Python: Fix venv isolation from user-site and base-install site-packages [STUD-81085]#596
viogroza wants to merge 4 commits into
developfrom
fix/python_venv

Conversation

@viogroza

@viogroza viogroza commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Python Scope's embedded interpreter never goes through CPython's own venv activation path (site.py's venv() function only triggers for a normally-launched <venv>/Scripts/python.exe), so pointing Path at a venv left both the PEP-370 user-site directory and the base install's own site-packages on sys.path unfiltered.
  • A native package (e.g. pywin32) installed with a different build in either location could resolve its Python module from one and its native DLL dependency from the other, mismatched, copy — surfacing as DLL load failed while importing win32api: The specified procedure could not be found.
  • Fixes both leak paths via PythonEngine.SetNoSiteFlag() (gated on whether the venv requests --system-site-packages). For a --system-site-packages venv, an ambient PYTHONNOUSERSITE in the host's own environment is now honored exactly as a natively-activated interpreter would (no override).
  • Also fixes PythonHome being pointed at the venv root instead of its declared base install (breaks stdlib resolution entirely) — falling back to LibraryPath's own directory when the declared base install doesn't actually carry a stdlib (e.g. a Microsoft Store Python) — tightens venv detection against false positives and a trailing-separator miss, and adds a version cross-check between a venv and the Python library actually being loaded.
  • Fixes sitecustomize.py/usercustomize.py precedence for --system-site-packages venvs so the venv's own copy gets a chance to run, without re-running a base/user copy's side effects a second time when the venv has none of its own.

Behaviour changes

  • EngineProvider.ValidateInstallation (called from PythonScope.ExecuteAsync/PythonService) now throws NotSupportedException when a venv's declared Python version (pyvenv.cfg) doesn't match the version actually loaded from LibraryPath. Previously this pairing would proceed and fail later with a confusing, version-specific stdlib error.

Test plan

  • UiPath.Python.Tests, UiPath.Python.Activities.Tests, UiPath.Python.Activities.API.Tests all pass (98 total)
  • Regression coverage added for: user-site isolation (default + --system-site-packages), base-install site-packages leak, sitecustomize.py/usercustomize.py preservation and precedence (including the no-double-execution case), PythonHome resolution and its Microsoft-Store/no-stdlib fallback, venv detection (root/Scripts/bin/trailing-separator/false-positive/unreadable-file), venv/library version mismatch (including version_info-only configs), ambient PYTHONNOUSERSITE handling, and the Windows Lib vs POSIX lib case-sensitive path segments
  • Each new/changed behavior verified to actually catch its regression by temporarily reverting the fix and confirming the test fails, then restoring it

🤖 Generated with Claude Code

…ges [STUD-81085]

Python Scope's embedded interpreter never goes through CPython's own venv
activation path (site.py's venv() function, which only triggers for a
normally-launched <venv>/Scripts/python.exe), so pointing Path at a venv left
both the PEP-370 user-site directory and the base install's own site-packages
on sys.path unfiltered. A native package (e.g. pywin32) installed with a
different build in either location could resolve its Python module from one
and its native DLL dependency from the other, mismatched, copy, surfacing as
"DLL load failed while importing win32api: The specified procedure could not
be found."

Fixes user-site and base-install leakage via PythonEngine.SetNoSiteFlag()
(gated on whether the venv requests --system-site-packages), with
Controller.ClearUserSiteEnvironmentOverride guarding against an ambient
PYTHONNOUSERSITE leaking in from the host's own environment. Also fixes
PythonHome being pointed at the venv root instead of its declared base
install (breaks stdlib resolution entirely), tightens venv detection against
false positives, and adds a version cross-check between a venv and the
Python library actually being loaded.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the Python activity pack’s handling of virtual environments (venvs) when using an embedded interpreter, ensuring sys.path is correctly isolated from PEP-370 user-site and (when appropriate) the base install’s site-packages, and surfacing clearer errors for misconfigured venv/library pairings.

Changes:

  • Adds shared venv detection (pyvenv.cfg) and uses it to drive runtime initialization behavior.
  • Updates engine initialization to set an appropriate PythonHome, gate Py_NoSiteFlag / SetNoSiteFlag() based on venv flags, and preserve sitecustomize.py execution.
  • Adds validation and regression tests for venv detection, version mismatch, user-site isolation, base-site leakage, and ambient PYTHONNOUSERSITE behavior.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
Activities/Python/UiPath.Python/Properties/UiPath.Python.resx Adds localized error text for venv/library version mismatch.
Activities/Python/UiPath.Python/Properties/UiPath.Python.Designer.cs Adds generated accessor for the new localized resource.
Activities/Python/UiPath.Python/Properties/AssemblyInfo.cs Grants internals visibility to UiPath.Python.Tests for new regression coverage.
Activities/Python/UiPath.Python/Impl/VenvDetection.cs Introduces shared, bounded venv detection + config parsing (pyvenv.cfg).
Activities/Python/UiPath.Python/Impl/OutOfProcessEngine.cs Plumbs venv flag into host spawning to clear ambient PYTHONNOUSERSITE when needed.
Activities/Python/UiPath.Python/Impl/Engine.cs Fixes PythonHome resolution for venvs, applies SetNoSiteFlag() for default venvs, and preserves sitecustomize.py.
Activities/Python/UiPath.Python/EngineProvider.cs Adds venv-vs-library version cross-check during installation validation.
Activities/Python/UiPath.Python.Tests/VenvVersionValidationTests.cs Regression tests for upfront venv/library version mismatch handling.
Activities/Python/UiPath.Python.Tests/VenvUserSiteIsolationTests.cs Regression coverage for user-site suppression, base-site leakage, sitecustomize preservation, and ambient env-var behavior.
Activities/Python/UiPath.Python.Tests/VenvDetectionTests.cs Fast unit tests for venv detection shapes and false-positive avoidance.
Activities/Python/Shared/UiPath.Shared.Service/Client/Controller.cs Adds child-process environment scrubbing for PYTHONNOUSERSITE when requested.
Files not reviewed (1)
  • Activities/Python/UiPath.Python/Properties/UiPath.Python.Designer.cs: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@alexandru-petre alexandru-petre left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code review — findings

Verdict: NEEDS CHANGES (1 HIGH, 8 MEDIUM, 2 LOW). Reviewed all 11 changed files against STUD=81085 (description + all 7 comments) and the PR description. 12 inline findings below.

Highlights

  • HIGH — the PythonHome fix does not cover Microsoft Store Python venvs. pyvenv.cfg's home there points at the app-execution-alias folder, which contains only reparse-point .exe stubs — no Lib, no Lib\encodings. Verified locally against a stdlib-venv config with home = ...\WindowsApps\PythonSoftwareFoundation.Python.3.13_qbz5n2kfra8p0. Not a regression (the old PythonHome = _path was equally broken), but the headline fix does not close a mainstream Windows install shape.
  • The new venv version cross-check silently no-ops for virtualenv/uv venvs, which write version_info rather than version.
  • ClearUserSiteEnvironmentOverride contradicts its own native-parity rationale — a natively-launched interpreter does honour PYTHONNOUSERSITE=1.

On the failed quality gate

Pulled the 34 issues from the SonarCloud API for this PR. D Security and D Reliability are driven entirely by pre-existing code in the touched files, not by this diff: S4790 (MD5 in Engine.Hash) and S6444 (regex without timeout in EngineProvider) for Security; S2551 (lock(this) x3) and S2445 for Reliability. The only genuinely PR-caused Sonar issue is S3776 on Engine.Initialize (complexity 29 vs. limit 15), commented inline. Suggest a separate PR or a gate exception rather than scope creep here.

Evidence gap

The ticket's reproduction package (Test-Pywin32VenvRepro (2).zip, attached 2026-08-20) postdates this branch's only commit (3b2a9d61, 2026-08-19). Nothing in the PR records a run against it — see the inline comment on VenvUserSiteIsolationTests.cs.

Done well

  • The SetNoSiteFlag block documents why the env-var approach was abandoned and why call ordering matters — exactly the reasoning that otherwise gets re-litigated six months later.
  • Replacing the unbounded GetVenvPath(maxLevels: 3) ancestor walk with root-or-Scripts/bin detection is the right narrowing, and both false-positive shapes are pinned by tests.
  • Both sides of the --system-site-packages fork assert opposite outcomes from the same code path — the pairing that actually proves the flag is load-bearing.
  • Catching the second-order sitecustomize.py loss and pinning it with Venv_SiteCustomize_Still_Runs was sharp.
  • Clean migration hygiene: zero surviving references to the removed IsVenv/GetVenvPath. Localization convention followed — only the canonical English resx was touched.

Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/VenvDetection.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/OutOfProcessEngine.cs Outdated
Comment thread Activities/Python/UiPath.Python.Tests/VenvVersionValidationTests.cs Outdated
Comment thread Activities/Python/UiPath.Python.Tests/VenvUserSiteIsolationTests.cs
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python.Tests/VenvUserSiteIsolationTests.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.

Files not reviewed (1)
  • Activities/Python/UiPath.Python/Properties/UiPath.Python.Designer.cs: Generated file
Suppressed comments (1)

Activities/Python/UiPath.Python/Impl/VenvDetection.cs:81

  • A Path ending in a separator breaks launcher-folder detection. folderName is computed from the trimmed value, but Path.GetDirectoryName(path) is not; for .../venv/bin/ it returns .../venv/bin, so pyvenv.cfg is searched in the launcher folder and the venv isolation/PythonHome fix is skipped. Derive the parent from the same trimmed path (and add a trailing-separator case to the detection tests).
            return TryReadVenvConfig(Path.GetDirectoryName(path));

Comment thread Activities/Python/UiPath.Python/Impl/VenvDetection.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/OutOfProcessEngine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/VenvDetection.cs Outdated
viogroza and others added 2 commits August 25, 2026 17:59
Fixes findings from PR #596 review:

- PythonHome falls back to LibraryPath's directory when the venv's
  declared home (pyvenv.cfg) doesn't actually carry a stdlib (e.g. a
  Microsoft Store Python's app-execution-alias folder).
- VenvDetection.GetVenvInfo no longer misses the venv root for a Path
  ending in a separator (Path.GetDirectoryName on a separator-terminated
  path only strips the separator, it doesn't walk up).
- An unreadable pyvenv.cfg (locked ACLs, AV, concurrent pip rewrite) now
  returns null instead of throwing out of the public ValidateInstallation.
- pyvenv.cfg's "version_info" (virtualenv/uv) is read as a fallback for
  "version" (stdlib venv), so the venv/library version cross-check isn't
  silently skipped for venvs created by those tools.
- sitecustomize.py/usercustomize.py precedence fixed for
  --system-site-packages venvs: the venv's own copy now actually gets a
  chance to run instead of being permanently shadowed by whatever
  site.main() already cached before the venv's site-packages was added.
- Removed Controller.ClearUserSiteEnvironmentOverride entirely: an
  ambient PYTHONNOUSERSITE is now honored for --system-site-packages
  venvs exactly like a natively-activated interpreter would, instead of
  being silently stripped.
- Extracted Engine.ConfigureRuntime out of Initialize (addresses the
  Sonar S3776 cognitive-complexity finding, the only genuinely
  PR-caused issue behind the failing quality gate — the rest is
  pre-existing debt in touched files, confirmed via diff, left alone).
- Made PostInitializationVenvSetup static and added GC.SuppressFinalize
  to three tests' Dispose() (CA1816/CA1822/S2325 — all genuinely new,
  cheap, zero-behavior-change).
- Added regression tests for all of the above, plus direct tests for the
  Windows "Lib" vs POSIX "lib" case-sensitive path segments, each
  verified to actually fail without its corresponding fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
alexandru-petre caught (PR #596, discussion_r3854255951) that the
previous fix — unconditionally popping "sitecustomize" from
sys.modules and re-importing for --system-site-packages venvs — was
worse than the bug it fixed: when the venv has no sitecustomize.py of
its own, the re-import still resolves to the same base/user copy,
running its side effects a second time.

Gate the pop+reimport on the venv's own site-packages actually
containing a sitecustomize.py/usercustomize.py. When it does, the
venv's copy (now first on sys.path) wins as intended. When it
doesn't, the module site.main() already cached — already the correct,
highest-priority one — is left untouched, so it runs exactly once.

Added Venv_With_SystemSitePackages_And_No_Own_SiteCustomize_Does_Not_Rerun_UserSite_Copy,
verified to fail (2 executions instead of 1) against the previous,
unconditional version of the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@viogroza

Copy link
Copy Markdown
Collaborator Author

Review comments addressed

Two follow-up commits: 359f5fc (first pass) and 49dde87 (a regression @alexandru-petre caught in the first pass's sitecustomize fix — see below).

Fixed:

  • PythonHome now falls back to LibraryPath's directory when the venv's declared home doesn't carry a real stdlib (Microsoft Store Python, or a pyvenv.cfg missing home entirely) — HIGH + one of the LOW findings.
  • version_info read as a fallback for version (virtualenv/uv venvs) — both the venv-detection and version-cross-check sides.
  • pyvenv.cfg read errors (ACLs, AV, concurrent pip) now return null instead of throwing out of the public ValidateInstallation.
  • Trailing-separator bug in launcher-folder detection (Scripts\/bin\) — fixed and covered.
  • ClearUserSiteEnvironmentOverride removed entirely — an ambient PYTHONNOUSERSITE is now honored for --system-site-packages venvs exactly like native CPython, no override.
  • sitecustomize.py/usercustomize.py precedence for --system-site-packages venvs, and a real double-execution regression in that same fix that @alexandru-petre caught afterward (49dde87) — the pop+reimport is now gated on the venv actually having its own copy, so a base/user copy's side effects never run twice.
  • Engine.Initialize extracted into ConfigureRuntime (Sonar S3776, the one genuinely PR-caused finding behind the failing quality gate — the rest is confirmed pre-existing debt in touched files, left alone).
  • The misleading EngineProvider-validated-home comment, corrected.
  • Hardcoded embedded-runtime version duplicated across two test files, now derived from one source.
  • PostInitializationVenvSetup made static, GC.SuppressFinalize added to three tests' Dispose() — the remaining genuinely-new Sonar findings.
  • PR description updated with a ## Behaviour changes section for the new NotSupportedException.

Not fixed, called out explicitly rather than silently skipped:

  • No live run against a real installer-based Python 3.10 + genuinely mismatched pywin32 builds — no such environment available here. Added direct, synthetic-fixture unit tests for the HasStdlib logic instead (including the Microsoft-Store shape), each verified to fail without its fix — but that's not the same as a live repro.
  • Py_NoSiteFlag's CPython 3.12 deprecation/3.15 removal — documented as a checklist item in ConfigureRuntime's XML doc rather than acted on, since nothing needs to change until 3.15+ support is actually added.

Every fix in both commits was verified the same way: temporarily revert it, confirm the corresponding test fails, restore it, confirm green. Full suite: 98/98 passing.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • Activities/Python/UiPath.Python/Properties/UiPath.Python.Designer.cs: Generated file
Suppressed comments (3)

Activities/Python/UiPath.Python/Impl/Engine.cs:170

  • SetNoSiteFlag() suppresses all of site.main(), not only user/base site-package discovery. The manual post-initialization path restores .pth processing and customization imports, but not site.main()'s other observable setup (quit/exit, help, copyright helpers, and readline completion). These built-ins therefore disappear for default venv scopes even though native venv startup and the previous implementation provide them. Restore the non-path portions of site initialization explicitly.
            if (venv != null && venv.ShouldDisableUserSite)
                PythonEngine.SetNoSiteFlag();

Activities/Python/UiPath.Python/Impl/Engine.cs:423

  • Assigning sys.prefix does not update site.PREFIXES, which was captured when site was imported while PythonHome still pointed at the base install. Consequently site.getsitepackages() continues to report base-install locations and omits the venv (including for default venvs), diverging from native activation and potentially directing package tooling to the wrong environment. Synchronize site.PREFIXES with the venv and, only for --system-site-packages, the base prefixes.
                    sys.prefix = venv.Root;
                    sys.exec_prefix = venv.Root;

Activities/Python/UiPath.Python/Impl/Engine.cs:461

  • This guard does not actually mirror site.main()'s ENABLE_USER_SITE check. In a --system-site-packages venv with ambient PYTHONNOUSERSITE=1, initialization sets site.ENABLE_USER_SITE to false, but a venv-local usercustomize.py still satisfies this condition and is executed explicitly. Check the runtime site.ENABLE_USER_SITE value before calling execusercustomize() so the ambient suppression applies to customization as well as user-site .pth files.
                    // execusercustomize() mirrors site.main()'s own "if ENABLE_USER_SITE:" guard —
                    // only called when user-site isn't suppressed, consistent with not calling it at
                    // all for the default (user-site-disabled) case. Same re-import gating as above.
                    if (!venv.ShouldDisableUserSite && File.Exists(Path.Combine(sitePackagesPath, "usercustomize.py")))

Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python.Tests/VenvUserSiteIsolationTests.cs

@alexandru-petre alexandru-petre left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review of 359f5fc3 + 49dde878

Thanks — every finding from the first round is either fixed or consciously deferred with a stated reason, and the deferrals are the right calls. Re-reviewed the two new commits and also validated Copilot's latest review, including its three suppressed comments, since those are collapsed and easy to miss.

Verified correct

  • Controller.cs reverted to the exact pre-PR blob (a1a7d1a9e7510269) — no residue. OutOfProcessEngine also drops its now-redundant GetVenvInfo call, removing a duplicate pyvenv.cfg read in the parent.
  • The HasStdlib fallback goes further than asked, in a good way: since both call sites pass Version.Auto, _path always contains a Python exe, so the only non-venv case where the fallback fires is one that was already broken (e.g. Path pointed at a Store alias folder). Strict improvement on Windows.
  • The try/catch wraps the foreach, not just the File.ReadLines() call — correct placement for a lazy enumerator, and the usual place people get it wrong.
  • Tests are strong: UnreadablePyvenvCfg_... takes a real FileShare.None lock rather than mocking; VersionInfoOnly_CrossChecksJustLikeVersion asserts the throw, proving the cross-check engages rather than merely parsing; Venv_With_SystemSitePackages_And_No_Own_SiteCustomize_Does_Not_Rerun_UserSite_Copy uses Assert.Single to pin "exactly once", which is precisely the double-execution regression. The PYTHONNOUSERSITE test was correctly inverted and renamed to ..._Honors_.... The WindowsStdlibLandmark/PosixStdlibDirectory casing tests, justified by NTFS case-folding on Windows-only CI, are a genuinely thoughtful bit of design.

New findings (4 inline below)

All four live in the 49dde878 sitecustomize work or in what SetNoSiteFlag silently removes. Two are mine, two are Copilot suppressed comments I verified and am surfacing.

Copilot's latest review — validated

Copilot finding Verdict
POSIX Path.GetDirectoryName(_libraryPath) is not a valid prefix (Engine.cs:164) Valid — endorsed in-thread with a minimal fix
Tests inherit ambient PYTHONNOUSERSITE (VenvUserSiteIsolationTests.cs:45) Valid — endorsed in-thread; now load-bearing since the fix deliberately honours the ambient value
Suppressed: SetNoSiteFlag also drops quit/exit/help builtins (Engine.cs:170) Valid — surfaced below, the strongest of the three
Suppressed: sys.prefix assignment leaves site.PREFIXES stale (Engine.cs:423) Valid — surfaced below as LOW
Suppressed: usercustomize guard doesn't mirror ENABLE_USER_SITE (Engine.cs:461) Valid — merged into my finding on the same block

One recurring defect in Copilot's output worth knowing about: its "This issue also appears in the following locations" lists are unreliable. On Engine.cs:164 it points at lines 169, 422 and 458 — which are its own three suppressed findings, not other instances of the POSIX problem. It did the same thing last round. Read those cross-references with suspicion.

Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs Outdated
Comment thread Activities/Python/UiPath.Python/Impl/Engine.cs

sys.prefix = venvPath;
sys.exec_prefix = venvPath;
sys.prefix = venv.Root;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Correctness/LOW] Assigning sys.prefix leaves site.PREFIXES pointing at the base install

Also from Copilot's suppressed section (Engine.cs:423); verified and agreed, at low severity.

site.PREFIXES is initialised at module-import time from [sys.prefix, sys.exec_prefix]. Py.Import("site") on the line above runs while PythonHome still points at the base install, and the sys.prefix assignment here does not retroactively update it. So site.getsitepackages() keeps reporting base-install locations and omits the venv — for default venvs too, not just --system-site-packages.

This does not affect script execution or the STUD=81085 fix, which is why LOW: it is a fidelity gap that shows up only for code introspecting the environment (packaging helpers, pip-adjacent tooling, diagnostics that print getsitepackages()).

Suggested fix — one line, right after the sys.exec_prefix assignment:

// site.PREFIXES was captured at import time, while PythonHome still pointed at the base
// install; resync it so site.getsitepackages() reports the venv like native activation does.
site.PREFIXES = venv.IncludeSystemSitePackages
    ? new[] { venv.Root, basePrefix }
    : new[] { venv.Root };

Reasonable to defer to a follow-up if you would rather keep this patch tight — but it is cheap, and it is the kind of thing that is much harder to justify revisiting later.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Deferring this one rather than folding it in now, per your own "reasonable to defer to a follow-up" note. Implementing it cleanly needs the resolved base prefix threaded into PostInitializationVenvSetup (it currently only receives venv), which is a bigger plumbing change than the snippet implies for a LOW-severity, introspection-only gap — doesn't affect script execution or the STUD-81085 fix itself. Given how much ground the last two rounds already covered (including two rounds of my own fixes needing a fix), I'd rather not rush a fourth change into this same method without it getting the same scrutiny. Happy to open a follow-up ticket for it, or fold it in here if you'd rather have it now — your call.

…nVenvSetup regressions

Addresses further PR #596 review findings from Copilot and alexandru-petre:

- ResolvePrefixFromLibraryPath now walks up ancestors testing HasStdlib
  at each level, instead of trusting the library's immediate parent
  directory as the prefix. On POSIX the shared library sits one or more
  levels below the real prefix (plain lib/, or a multiarch triplet like
  lib/x86_64-linux-gnu/), so the previous one-level fallback could set
  PythonHome to a directory with no stdlib either.
- HasStdlib now also recognizes a *._pth file as valid evidence of a
  home: CPython's ._pth-based isolation (the official Windows
  embeddable distribution, but not Windows-exclusive) resolves its
  stdlib from a bundled zip rather than an unpacked Lib folder, which
  the folder-based checks alone would wrongly treat as "no stdlib".
- Fixed a regression in the previous commit's sitecustomize fix: the
  File.Exists gate meant to stop double-execution for
  --system-site-packages venvs was also applied to the default-venv
  case, where SetNoSiteFlag means nothing was ever cached — so a
  default venv with no venv-local sitecustomize.py silently stopped
  picking up a base-install Lib\sitecustomize.py (e.g. corporate
  proxy/logging setup). Split into two branches: default venv calls
  execsitecustomize() unconditionally again; --system-site-packages
  keeps the gated pop+reimport.
- Removed the usercustomize block entirely: it checked the venv's own
  site-packages, but usercustomize.py only ever lives in the per-user
  site directory, so it was dead code — and on the rare path it could
  fire, its guard (a static config flag) didn't match the runtime
  ENABLE_USER_SITE value, so it could fire exactly when user-site
  customization should be suppressed. Restored the original one-line
  rationale for not calling it at all here.
- SetNoSiteFlag also skips site.main()'s setquit()/setcopyright()/
  sethelper(), silently dropping the exit/quit/help/copyright/credits/
  license builtins from a default-venv scope. Restored them explicitly
  (not enablerlcompleter(), which only matters interactively).
- VenvUserSiteIsolationTests' constructor now clears PYTHONUSERBASE/
  PYTHONNOUSERSITE after saving them, so tests don't inherit whatever
  the host machine's own ambient environment happens to have.

Added regression tests for all of the above (._pth recognition,
ancestor-walk success/exhaustion, default-venv sitecustomize from a
base install, exit/help builtins), each verified to fail without its
corresponding fix and pass with it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@viogroza

Copy link
Copy Markdown
Collaborator Author

Second round of review comments addressed

Commit 0a1e7ba, on top of the previous 359f5fc/49dde87.

Fixed — new findings from this round:

  • My own File.Exists gate (from 49dde87) over-applied to the default-venv case, silently dropping execsitecustomize()'s unconditional call for a venv with no local sitecustomize.py but a base-install Lib\sitecustomize.py (corporate proxy/logging setups). Split into two branches by venv.ShouldDisableUserSite.
  • The usercustomize block checked the venv's own site-packages — usercustomize.py only ever lives in the per-user site directory, so it was dead code, and wrong on the rare path it could fire. Removed entirely, restored the original one-line rationale.
  • SetNoSiteFlag silently drops exit/quit/help/copyright/credits/license builtins from a default-venv scope (Copilot's suppressed finding, verified and surfaced by @alexandru-petre). Restored setquit()/setcopyright()/sethelper().
  • ResolvePrefixFromLibraryPath's one-level fallback was wrong on POSIX (library sits below the real prefix, not at it) — now walks up ancestors testing HasStdlib. Also closed a gap this exposed once tested: HasStdlib didn't recognize a legitimate ._pth-based home (the embeddable distribution's own layout), now fixed.
  • VenvUserSiteIsolationTests's constructor now clears PYTHONUSERBASE/PYTHONNOUSERSITE after saving them, so tests don't inherit whatever the host machine's ambient environment has.

Deferred, with the reviewer's own suggestion to do so:

  • site.PREFIXES staying stale after the sys.prefix reassignment (LOW, introspection-only — doesn't affect script execution or the fix itself). Needs threading the resolved base prefix into PostInitializationVenvSetup, which didn't seem worth rushing in on top of everything else this round.

Every fix verified the same way as before: temporarily revert, confirm the corresponding test fails, restore, confirm green. Full suite: 104/104 passing.

🤖 Generated with Claude Code

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
D Security Rating on New Code (required ≥ A)
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • Activities/Python/UiPath.Python/Properties/UiPath.Python.Designer.cs: Generated file

else
return GetVenvPath(Path.GetDirectoryName(venvPath), maxLevels - 1);

var dir = Path.GetDirectoryName(libraryPath);
// like a natively-activated default venv does.
site.execsitecustomize();
}
else if (File.Exists(Path.Combine(sitePackagesPath, "sitecustomize.py")))

@alexandru-petre alexandru-petre left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving ✅

Four rounds, 24 threads, 21 resolved. Every MEDIUM and the one HIGH are fixed and verified against 0a1e7ba1 — not just marked done. What stands out is that the fixes were checked rather than assumed: the merged sitecustomize gate was confirmed by restoring it and watching a new test fail, and the double-execution regression in 49dde878 was caught and corrected in the same session it was introduced.

The ._pth recognition in HasStdlib deserves a specific mention — it was the author's own catch, and it is the sharpest thing in the PR. Without it, commits 2–3 were passing their venv tests through the fallback path, since the embeddable runtime has no unpacked Lib. The primary home-based resolution was silently untested and nobody had flagged it.

Nits — all non-blocking, all one-to-three lines

None of these justify another round on their own; batch them into a follow-up if you prefer.

1. [Correctness/LOW] Stdlib landmark is satisfied by an empty directoryEngine.cs:382, same shape at :401

HasStdlib accepts any folder containing Lib\encodings (POSIX: lib/python*/encodings) regardless of contents, so an empty directory of that name passes and PythonHome gets set to a home that still cannot bootstrap. Venv_SiteCustomize_Still_Runs_From_BaseInstall_When_Venv_Has_None shows how cheap that is to satisfy — it creates fakebase\Lib\encodings empty purely to get past the check. Testing for Lib\encodings\__init__.py instead would tighten a check the whole fallback now hinges on.

2. [Bugs/LOW] The ancestor walk aborts on one unreadable ancestorEngine.cs:378, reached from :417

ResolvePrefixFromLibraryPath calls HasStdlib at each level, and HasStdlib now does Directory.EnumerateFiles(pythonHome, "*._pth"), which throws UnauthorizedAccessException on a directory the robot account cannot enumerate. One denied ancestor aborts the whole walk instead of continuing upward, failing init even when a higher ancestor would have resolved. Before 0a1e7ba1 the fallback did no I/O at all. A try/catch { continue; } around the per-level probe matches what TryReadVenvConfig and the EngineProvider validators already do.

3–4. The two open Copilot findings — both valid, both LOW, both still unaddressed:

  • Bare relative LibraryPath (Engine.cs:422). One correction to that comment: Path.GetDirectoryName("python314.dll") returns "", not null — but the effect is exactly as described, the while guard fails immediately and the fallback silently no-ops in the case it exists to repair. Path.GetFullPath first.
  • sitecustomize as a package or bytecode module (Engine.cs:498). Narrow — needs --system-site-packages and a non-source form and a cached base/user copy.

Two deferrals that need a home after merge

Both were deferred for good reasons, and I agree with both calls — but once this merges, the threads carrying them stop being somewhere anyone looks:

  • site.PREFIXES left stale — fidelity only, no execution impact; needs the resolved base prefix threaded through to fix cleanly.
  • No regression coverage against an installer-based Python. This is the one I would still like closed, and it is the reason I am calling it out here rather than letting it disappear. The suite runs entirely against the embeddable distribution, whose ._pth bootstrap resolves the stdlib independently of PythonHome — structurally the opposite of the reported configuration (3.10, installer-based, real pywin32). Declining to claim coverage you cannot run was the right instinct. The ticket's own Test-Pywin32VenvRepro (2).zip exists; one manual run recorded in the PR body would close it, and for a customer-facing patch that is worth the twenty minutes.

Suggest a follow-up ticket carrying both, linked from this PR.

On the quality gate

Still red, still not this PR's doing. Verified against the SonarCloud API at this head: all seven issues driving the D Security / D Reliability ratings are pre-existing code in touched files — S4790 (MD5 in Engine.Hash), S6444 (regex without timeout, ×2), S2551 (lock(this), ×3), S2445. The one genuinely PR-caused issue, S3776 on Engine.Initialize, is CLOSED / FIXED after the ConfigureRuntime extraction. Merge will need either a separate cleanup PR or a gate exception — please do not absorb those into this one.

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