Skip to content

Add PEP 723 inline script environment creation (PEP 723 PR 5c/16) - #1656

Open
StellaHuang95 wants to merge 4 commits into
microsoft:mainfrom
StellaHuang95:pep723-pr5c-manager
Open

Add PEP 723 inline script environment creation (PEP 723 PR 5c/16)#1656
StellaHuang95 wants to merge 4 commits into
microsoft:mainfrom
StellaHuang95:pep723-pr5c-manager

Conversation

@StellaHuang95

@StellaHuang95 StellaHuang95 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Part of #1602 (PEP 723 inline script env support). Design doc: #1601.

Split for review (3 PRs). Reviewers flagged the original PR 5 as too large, so it is split into three PRs grouped by dependency layer:

#1651 and #1655 have merged and this branch has been rebased. The diff now contains only this PR's five files.

Roadmap context

This is the final slice of PR 5 of 16 — the actual create() happy path. See #1651 for the full roadmap table.

Phase 2: Manager PR Status
PR 4: InlineScriptEnvManager skeleton merged (#1610)
PR 5a: generic env-creation utilities merged (#1651)
PR 5b: inline-script cache + interpreter utilities merged (#1655)
PR 5c: create() happy path (manager + wiring) this PR (#1656)
PR 6: create() uv-install fallback not started (needs 3, 5)

Why this PR

InlineScriptEnvManager.create() was a deliberately empty no-op after PR 4. This PR implements its happy path: the case where the machine already has a base interpreter that satisfies the script's requires-python, so no uv Python install is required. Given a PEP 723 script, it builds — or reuses — a dependency-keyed virtual environment under the extension's global storage, following the pipx-style cache design from Q4 of #1601. The uv-install fallback (no compatible interpreter present) is deferred to PR 6.

It composes the primitives from 5a (#1651) and the inline-script utilities from 5b (#1655); this PR adds only the manager and its wiring.

What this PR does

Wires the manager's collaborators (extension.ts, inlineScriptMain.ts): registerInlineScriptFeatures and the InlineScriptEnvManager constructor now receive the NativePythonFinder, the PythonEnvironmentApi, the base (system) environment manager, and globalStorageUri.

Implements create(scope) (inlineScriptEnvManager.ts):

  • Accepts exactly one local file: URI (a bare Uri or single-element array). Anything else — 'global', a folder, or multiple URIs — logs a warning and returns undefined.
  • Reads PEP 723 metadata from the script; missing or invalid metadata returns undefined.
  • Merges metadata.dependencies with options.additionalPackages, trims each, and rejects empty entries.
  • Selects a base interpreter, computes the dependency + interpreter cache key, and de-duplicates concurrent in-process create() calls for the same key via a pendingCreations map.

Base-interpreter selection (selectBaseInterpreter): starts from getEnvironments('global'), keeps only true base managers (system, pyenv, conda base), and excludes derived environments by rejecting a non-absolute sysPrefix or the presence of pyvenv.cfg. It then picks the newest compatible interpreter with pickCompatibleInterpreter and resolves the executable through fs.realpath so the cache key is canonical. If a candidate cannot be resolved it falls through to the next.

Create-or-reuse under a cross-process lock (createOrReuseEnvironment): acquires a directory lock (5a), inspects the existing cache entry, and reuses / rebuilds / preserves accordingly, always releasing the lock in finally.

Fail-closed cache inspection (inspectCacheEntry) returns absent | stale | uncertain | reusable:

  • Rejects symlinks and non-directories; verifies the entry is contained under the cache root (resolveCacheEntryPath).
  • Reads and validates the .meta.json sidecar and confirms the recorded base-interpreter path and version still match the selected base.
  • Confirms the base interpreter is still present on disk (getBaseInterpreterStatus).
  • Resolves the cached venv to a real PythonEnvironment, confirms it is genuinely ours via realpath containment (inspectOwnedCacheEntry), compares Python release segments, and re-checks requires-python with the existing matchesPythonVersion.
  • Only conclusive evidence marks an entry stale (rebuild); any doubt yields uncertain, and an uncertain entry is preserved, never deleted. A reused entry has its lastUsedAt refreshed.

Environment build (buildCacheEntry): delegates to the existing createWithProgress venv flow with trackUvEnvironment set to false so cached script environments are not registered as workspace venvs. On success it writes the sidecar and re-validates that the built environment matches the requested release and is owned by this entry. On failure it removes the directory and returns empty. On cancellation it retains the lock so a half-built environment is not silently reused later.

Tests

  • inlineScriptEnvManager.unit.test.ts — 40 tests across scope/metadata validation, base-interpreter selection, cache creation, cache reuse, transaction rollback, and events/disposal.
  • inlineScriptMain.unit.test.ts — updated for the new registerInlineScriptFeatures signature.

On this rebased branch npm run compile-tests is clean and npm run unittest reports 1491 passing, 0 failing, 5 pending.

User impact

None on the default path. The manager is still registered only when the undeclared python-envs.inlineScripts.enabled flag is on, so default users see nothing.

create() is now a declared method (PR 4 omitted it), so with the flag on the inline manager can appear as a create target. But it acts only on a single local script URI and no-ops on every other scope, and nothing in the extension routes a script URI to it yet. Wiring the trigger is later work: routing in PR 9, and the "Set up env for this script" picker item and bulk command in PR 11/12.

Merge order

#1651 and #1655 have merged. This PR is the remaining final slice.

@StellaHuang95 StellaHuang95 added the feature-request Request for new features or functionality label Jul 23, 2026
StellaHuang95 added a commit that referenced this pull request Jul 27, 2026
> Part of #1602 (PEP 723 inline script env support). Design doc: #1601.

> **Split for review (3 PRs).** Reviewers flagged the original PR 5 as
too large, so it is split into three stacked PRs grouped by dependency
layer:
> - **5a — generic env-creation utilities — this PR (#1651).** Based on
`main`; independent; merges first.
> - **5b — inline-script cache + interpreter utilities — #1655.**
Stacked on 5a.
> - **5c — `create()` happy path (manager + wiring) — #1656.** Stacked
on 5b.
>
> Applied together the three PRs are byte-for-byte identical to the
original single change. **Merge order: 5a → 5b → 5c.**

### Roadmap context

This is the first slice of **PR 5 of 16** in the PEP 723 inline-script
roadmap. The full plan lives in #1602.

| Phase | PR | Status |
|---|---|---|
| **Phase 1: Foundation** | PR 1: cache key hash utility | merged
(#1634) |
| | PR 2: cache layout + `meta.json` sidecar | merged (#1635) |
| | PR 3: `requires-python` to interpreter selection | merged (#1636) |
| **Phase 2: Manager** | PR 4: `InlineScriptEnvManager` skeleton |
merged (#1610) |
| | **PR 5a: generic env-creation utilities** | **this PR (#1651)** |
| | **PR 5b: inline-script cache + interpreter utilities** | **#1655** |
| | **PR 5c: `create()` happy path (manager + wiring)** | **#1656** |
| | PR 6: `create()` uv-install fallback | not started (needs 3, 5) |
| | PR 7: persistence with `get`, `set`, and Memento | not started
(needs 4) |
| | PR 8: activation-time discovery | not started (needs 2, 4, 7) |
| **Phase 3: Routing** | PR 9: route PEP 723 scripts to the inline
manager | not started (needs 4, 7) |
| | PR 10: per-script project registration | not started (needs 9) |
| **Phase 4+: UX / lifecycle** | PRs 11-16 | not started |

### Why this PR

PR 5c implements `InlineScriptEnvManager.create()`. Before touching the
manager, this PR lands the **generic, reusable primitives** it relies on
— a cross-process file lock, a venv Python-path helper, a
cancellation-hardened process runner, and two small `createWithProgress`
options. None of this code is inline-script-specific, so it is reviewed
on its own.

### What this PR adds

**Cross-process file lock** (`src/common/lockfile.apis.ts`, new):
`acquireFileLock` uses an atomic `mkdir` of a `<path>.lock` directory
plus a per-owner marker file, returning `AcquiredFileLock { release,
retain }`. `retain()` writes a `retained` marker so a later acquirer
**fails fast with `ELOCKRETAINED`** instead of waiting out the 5-minute
timeout — used when a build is cancelled mid-flight. Distinct error
codes (`ELOCKED`, `ELOCKRETAINED`, `ELOCKORPHANED`, `ECOMPROMISED`,
`ERETAINFAILED`) separate contention from corruption.

**Shared `getVenvPythonPath`**
(`src/common/utils/virtualEnvironment.ts`, new): returns
`Scripts\python.exe` on Windows, else `bin/python`. Replaces an inline
copy in `venvUtils` and is reused by 5b/5c.

**Hardened process helper** (`src/managers/builtin/helpers.ts`): `runUV`
and `runPython` now share one `runProcess` implementation whose
cancellation guards `kill()` in `try/catch` and still emits a clean
`CancellationError` if the process errors after a cancel. Per-caller
options preserve existing behavior (`collectStderr`, `logPrefix`).

**`venvUtils.ts`:** `createWithProgress` gains
`CreateWithProgressOptions { trackUvEnvironment }`, and
`CreateEnvironmentResult` gains `pkgInstallationCancelled` so a caller
can tell cancellation apart from a real install failure. Existing
callers are unaffected (both are optional / additive).

### Tests

- **`lockfile.apis.unit.test.ts`** — 9 tests: contention,
retain/fail-fast, orphaned and compromised locks, and timeout.
- **`virtualEnvironment.unit.test.ts`** — 2 tests for
`getVenvPythonPath` on Windows and POSIX.
- **`helpers.cancellation.unit.test.ts`** — 4 tests for `runProcess`
cancellation safety.
- **`venvUtils.createWithProgress.unit.test.ts`** — 3 tests for
`trackUvEnvironment` and `pkgInstallationCancelled`.

On this branch alone `npm run compile-tests` is clean and `npm run
unittest` reports **1447 passing, 0 failing, 4 pending**.

### User impact

**None.** These are internal primitives with no new user-visible
behavior. The refactors to `helpers.ts` and `venvUtils.ts` are
behavior-preserving for existing callers.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
@eleanorjboyd

eleanorjboyd commented Jul 28, 2026

Copy link
Copy Markdown
Member

Test verification report

CI clarification: all checks are green for this commit. The TypeScript unit-test job runs only on Ubuntu and Windows; macOS CI runs smoke/E2E/integration suites, not these unit tests.

Must fix

  1. No test protects an unresolvable cached environment. Production deliberately preserves an entry when resolveVenvPythonEnvironmentPath() returns undefined in inspectCacheEntry(). Mutating this result from uncertain to stale left all 41 manager tests passing. Please add a valid-cache test with resolveVenvStub.resolves(undefined) and assert the entry is preserved and not rebuilt.

Warnings

  1. Registration does not verify that the manager itself is disposable. Replacing the manager in disposables with a duplicate registration disposable still left both registerInlineScriptFeatures tests passing. Assert that the captured manager is actually present in disposables, not only that its length is 2.

Summary

  • New tests reviewed: 43
  • CI: all checks passing
  • Local macOS unit run: 38 passing, 5 failing due to /var vs /private/var path identity
  • Critical gaps: 1
  • Warnings: 2
  • Grade: B

Trace checks confirmed the coalescing, cache-reuse, and rollback tests execute real manager paths. Mutations showed coalescing and sidecar rollback tests correctly catch regressions. All temporary repairs, traces, and mutations were reverted; the worktree is clean.

StellaHuang95 added a commit that referenced this pull request Jul 29, 2026
…1655)

> Part of #1602 (PEP 723 inline script env support). Design doc: #1601.

> **Split for review (3 PRs).** Reviewers flagged the original PR 5 as
too large, so it is split into three PRs grouped by dependency layer:
> - **5a — generic env-creation utilities — #1651.** Merged.
> - **5b — inline-script cache + interpreter utilities — this PR
(#1655).** Rebased on `main`.
> - **5c — `create()` happy path (manager + wiring) — #1656.** Stacked
on 5b.
>
> #1651 has merged and this branch has been rebased. The diff now
contains only this PR's seven files. **Remaining merge order: 5b → 5c.**

### Roadmap context

This is the second slice of **PR 5 of 16**. See #1651 for the full
roadmap table.

| Phase 2: Manager | PR | Status |
|---|---|---|
| | PR 4: `InlineScriptEnvManager` skeleton | merged (#1610) |
| | PR 5a: generic env-creation utilities | merged (#1651) |
| | **PR 5b: inline-script cache + interpreter utilities** | **this PR
(#1655)** |
| | PR 5c: `create()` happy path (manager + wiring) | #1656 |

### Why this PR

With the generic primitives from 5a in place, this PR lands the
**inline-script-specific utilities** that `create()` (5c) composes: a
normalized dependency cache key, cache-layout ownership/status checks,
and interpreter-constraint handling. These are pure functions with no
manager wiring yet, so they are reviewed on their own.

### What this PR adds

**Cache-key tail normalization** (`src/common/inlineScriptCacheKey.ts`):
adds `normalizeRequirementTail`, a quote-aware scanner that collapses
whitespace and tightens comparator spacing (`>= 1.0` → `>=1.0`) in a
requirement's version/marker tail while **preserving quoted PEP 508
marker literals verbatim** (e.g. `python_version >= "3.11"`).
Direct-reference requirements (`pkg @ https://…`) are kept verbatim
after the name and extras. The effect is that semantically identical
dependency strings normalize to the same cache key, so they reuse the
same cached environment.

**Cache-layout additions** (`src/common/inlineScriptCacheLayout.ts`):
`resolveCacheEntryPath` (containment under the cache root),
`inspectOwnedCacheEntry` (realpath ownership),
`getBaseInterpreterStatus` (`available | missing | unavailable`),
`inspectMetaJson` (typed sidecar read), and a stricter `validateMeta`.
The `.meta.json` sidecar schema is `{ schemaVersion,
baseInterpreterPath, baseInterpreterVersion, lastUsedAt }`. Uses
`getVenvPythonPath` from merged PR #1651.

**Interpreter-constraint trimming**
(`src/common/inlineScriptInterpreter.ts`): `pickCompatibleInterpreter`
now trims `requires-python`, so a whitespace-only constraint is treated
as no constraint.

**Manager ID constants** (`src/common/constants.ts`): centralizes the
conda and inline-script manager IDs used by interpreter filtering.

### Tests

- **`inlineScriptCacheKey.unit.test.ts`** — canonicalization cases
including marker literals and direct references.
- **`inlineScriptCacheLayout.unit.test.ts`** — the new containment,
ownership, base-interpreter-status, and typed sidecar-read helpers.
- **`inlineScriptInterpreter.unit.test.ts`** — constraint trimming /
selection.

On this rebased branch `npm run compile-tests` is clean and `npm run
unittest` reports **1467 passing, 0 failing, 5 pending**.

### User impact

**None.** These are pure utilities. Nothing calls the new code paths
until the manager lands in 5c (#1656).

### Merge order

#1651 has merged. Merge this PR next, then #1656.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
StellaHuang95 and others added 2 commits July 29, 2026 14:16
Implement InlineScriptEnvManager.create(): select a compatible base interpreter and build or reuse a dependency-keyed virtual environment, with cache-ownership validation, cross-process locking, and cancellation-safe creation. Wire the manager's collaborators.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
Use the merged createWithProgress boolean parameter while preserving that inline-script cache entries are not tracked as workspace uv environments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
@StellaHuang95

Copy link
Copy Markdown
Contributor Author

Will do a separate PR to put all the inline related files into a "inline" directory. Don't want to make this PR too messy and difficult to review.

@StellaHuang95
StellaHuang95 marked this pull request as ready for review July 29, 2026 22:28
@StellaHuang95

Copy link
Copy Markdown
Contributor Author

Test verification report

CI clarification: all checks are green for this commit. The TypeScript unit-test job runs only on Ubuntu and Windows; macOS CI runs smoke/E2E/integration suites, not these unit tests.

Must fix

  1. No test protects an unresolvable cached environment. Production deliberately preserves an entry when resolveVenvPythonEnvironmentPath() returns undefined in inspectCacheEntry(). Mutating this result from uncertain to stale left all 41 manager tests passing. Please add a valid-cache test with resolveVenvStub.resolves(undefined) and assert the entry is preserved and not rebuilt.

Warnings

  1. Registration does not verify that the manager itself is disposable. Replacing the manager in disposables with a duplicate registration disposable still left both registerInlineScriptFeatures tests passing. Assert that the captured manager is actually present in disposables, not only that its length is 2.

Summary

  • New tests reviewed: 43
  • CI: all checks passing
  • Local macOS unit run: 38 passing, 5 failing due to /var vs /private/var path identity
  • Critical gaps: 1
  • Warnings: 2
  • Grade: B

Trace checks confirmed the coalescing, cache-reuse, and rollback tests execute real manager paths. Mutations showed coalescing and sidecar rollback tests correctly catch regressions. All temporary repairs, traces, and mutations were reverted; the worktree is clean.

feedback addressed.

@eleanorjboyd

Copy link
Copy Markdown
Member

Low — Consolidate the duplicated file-not-found helper

InlineScriptEnvManager.isFileNotFoundError() duplicates the helper already used throughout inlineScriptCacheLayout.ts. Keeping identical errno classification in two places risks future semantic drift. Move it to an accessible common filesystem utility and reuse it from both modules.

No Critical, High, or Medium findings. Lock retention, rollback, cache ownership, symlink containment, interpreter selection, cross-platform paths, constants placement, and comments all look sound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-request Request for new features or functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants