Skip to content

Add inline-script cache and interpreter utilities (PEP 723 PR 5b/16) - #1655

Merged
StellaHuang95 merged 2 commits into
microsoft:mainfrom
StellaHuang95:pep723-pr5b-cache-utils
Jul 29, 2026
Merged

Add inline-script cache and interpreter utilities (PEP 723 PR 5b/16)#1655
StellaHuang95 merged 2 commits into
microsoft:mainfrom
StellaHuang95:pep723-pr5b-cache-utils

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 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.

@StellaHuang95 StellaHuang95 added the feature-request Request for new features or functionality label Jul 23, 2026
@StellaHuang95
StellaHuang95 marked this pull request as ready for review July 23, 2026 23:36
@StellaHuang95
StellaHuang95 marked this pull request as draft July 23, 2026 23:41
@StellaHuang95
StellaHuang95 force-pushed the pep723-pr5b-cache-utils branch from 09b7664 to 7363231 Compare July 24, 2026 00:13
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
@StellaHuang95
StellaHuang95 force-pushed the pep723-pr5b-cache-utils branch from 7363231 to 2bbc6a5 Compare July 27, 2026 19:25
Comment thread src/common/inlineScriptCacheKey.ts
@StellaHuang95
StellaHuang95 marked this pull request as ready for review July 27, 2026 20:09
@StellaHuang95
StellaHuang95 force-pushed the pep723-pr5b-cache-utils branch from 2bbc6a5 to a98295d Compare July 27, 2026 22:33
Comment thread src/common/inlineScriptCacheKey.ts
Comment thread src/common/inlineScriptCacheKey.ts
Comment thread src/common/inlineScriptInterpreter.ts
Comment thread src/common/inlineScriptCacheLayout.ts
Comment thread src/common/inlineScriptCacheLayout.ts
Comment thread src/common/inlineScriptCacheLayout.ts
@eleanorjboyd

Copy link
Copy Markdown
Member

Test verification report

Must fix

  1. Ownership checks are not independently protected. In inspectOwnedCacheEntry tests, the stale case changes both sysPrefix and environmentPath. Mutations removing either production comparison still left all 4 ownership tests passing. Please add separate stale tests where only sysPrefix differs and where only environmentPath differs.

  2. Direct-child enforcement is untested. The resolveCacheEntryPath tests cover a direct entry, the root, an outside path, and a missing path—but not a nested descendant. Removing the exact direct-child comparison while retaining descendant containment left all 3 tests passing. Please add a nested path such as cacheRoot/nested/key and assert it is rejected.

Summary

  • Suites reviewed: 3
  • Added tests reviewed: 27
  • Critical gaps: 2
  • Always-passing assertions / swallowed failures: 0
  • Flakiness or over-mocking findings: 0
  • Grade: B

Verification evidence

  • Full baseline: 1,466 passing, 6 pending.
  • Affected suites: 136 passing.
  • Trace logging confirmed the ownership and containment tests execute their real production functions.
  • Mutation checks exposed the two gaps above.
  • All temporary traces and mutations were reverted; the worktree is clean.

StellaHuang95 and others added 2 commits July 28, 2026 13:50
Cache-key tail normalization, cache-layout ownership/status checks with typed sidecar reads, and interpreter-constraint trimming that inline-script environment creation builds on.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27
@StellaHuang95
StellaHuang95 force-pushed the pep723-pr5b-cache-utils branch from d9a6511 to a64dbff Compare July 28, 2026 20:50
@StellaHuang95
StellaHuang95 requested a review from edvilme July 28, 2026 21:34

@edvilme edvilme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 comments/questions but otherwise, looks good to me ;)

}

/** ` >= 2 ; python_version < "3.13"` becomes `>=2 ; python_version<"3.13"`. */
function normalizeRequirementTail(value: string): string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not entirely sure, but I think this could be further simplified with some regexes rather than iterating over the characters. If not, this looks good to me :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking to address the feedback while getting 5c pr ready. I considered this, but the loop is intended to be quotes aware. It normalizes whitespace outside marker literals while preserving it inside quoted values, and include quotes. It's difficult to get regex to do that. I'd keep it this way for now.

Comment on lines +280 to +285
function isDescendantPath(rootPath: string, candidatePath: string): boolean {
const relative = path.relative(rootPath, candidatePath);
return (
relative.length > 0 && relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps we could see if there are other places we could use this in, and put it on an utils file or something similar?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

venv and pip utils have something similar, but for different purposes and different behavior. This one rejects the root itself on purpose because we don't want to accidentally touch the root of the cached envs and modify every cached envs.

@StellaHuang95
StellaHuang95 merged commit 2cef37a into microsoft:main Jul 29, 2026
45 checks passed
@StellaHuang95
StellaHuang95 deleted the pep723-pr5b-cache-utils branch July 29, 2026 21:07
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.

3 participants