Route PEP 723 scripts to inline environments (PEP 723 PR 9/16) - #1729
Conversation
|
🔒 Automated review in progress — Heejae Chang (@heejaechang) is auto-reviewing this PR. |
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
Heejae Chang (heejaechang)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Eleanor Boyd (eleanorjboyd)
left a comment
There was a problem hiding this comment.
Thanks for the thorough work here. I found three correctness issues that need addressing before approval:
-
InlineScriptRoutingRegistry.setMetadata()preservesvalidatedAssociationwhen the metadata routing identity changes. This leaves the old association routeable while async provenance validation for the newly saved metadata is pending, so callers can temporarily receive the previous inline environment. Please invalidate association validation synchronously whenever the metadata identity changes. -
When a non-inline script override is cleared,
setEnvironment()removes the override but continues resolving and publishing through the old manager/key. It also deletes the now-relevant inline last-known selection, so timeout-based API callers can receiveundefinedand no later manager event is guaranteed to correct it. Please re-resolve and publish through the manager that becomes effective after override removal. -
The cache creation/reuse path rejects entries when the selected base interpreter version has changed or the environment no longer satisfies
requires-python, but cached and rehydrated persisted-association validation does not apply those checks before restoring routeability. A same-path interpreter upgrade can therefore keep an association routeable that fresh cache validation would reject. Please apply the same base-version and compatibility invariants to persisted associations.
Test note: triggerSavedMetadataChange() both publishes through the registry and directly invokes the private handler. It would be valuable to exercise only the public event path so broken production wiring cannot be masked.
Written by an AI agent on behalf of Eleanor.
Add saved-metadata routing, durable association provenance, explicit override precedence, and race-safe active-environment transitions behind the internal feature gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
Cancel stale reads after edits, preserve monotonic metadata revisions, and align BOM source offsets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
cb4928c to
96f38ef
Compare
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
Invalidate changed metadata immediately, restore the effective manager after override removal, and align persisted validation with cache invariants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
|
Addressed the three correctness issues from review 4985022926 in aed8aee:
I also added a focused test that drives validation solely through Validation: compile-tests, bundle, lint, and full unit suite — 1,806 passing, 6 pending. |
Keep overrides through manager refreshes and reject malformed association schema versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
Treat non-positive and fractional schema versions as malformed rather than future-compatible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b12d843-8011-4bfc-9ba9-f75761eadee2
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/managers/builtin/inlineScript/envManager.ts:L227.
|
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
There was a problem hiding this comment.
Pull request overview
This PR introduces activation-scoped routing for PEP 723 inline scripts so that per-file environment resolution can switch to the inline-script environment manager only when both (a) current saved metadata exists and (b) a persisted association is proven valid for that metadata/cache provenance. It wires a shared routing registry into activation, detection, the inline-script manager, and the central environment-manager router.
Changes:
- Add an activation-latched inline-script feature gate that (when enabled) creates a shared
InlineScriptRoutingRegistryused across components. - Extend the lazy PEP 723 detector to publish/clear saved-metadata routing state (including dirty/edit/delete/rename handling) while keeping telemetry behavior unchanged when routing is absent.
- Add cache provenance durability and routing-aware association validation, plus central manager routing precedence that respects explicit non-inline overrides.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/managers/builtin/inlineScript/main.unit.test.ts | Updates registration tests for activation-latched gating and routing-registry requirements. |
| src/test/features/pythonApi.unit.test.ts | Adds coverage for getEnvironment() timeout fallback behavior. |
| src/test/features/inlineScript/lazyDetector.unit.test.ts | Expands detector tests for routing-aware behavior (dirty/save races, loose files, delete/rename, BOM/CRLF offsets). |
| src/test/features/envManagers.lastKnown.unit.test.ts | Adds routing-driven manager-selection precedence and override/refresh scenarios. |
| src/test/common/inlineScript/routingRegistry.unit.test.ts | New unit tests for routing registry identity/revision and routeability transitions. |
| src/test/common/inlineScript/metadata.unit.test.ts | Adds assertions for sourceRange and normalized vs source offsets. |
| src/test/common/inlineScript/cacheLayout.unit.test.ts | Expands tests for sidecar provenance, write contention recovery, and backup restoration behavior. |
| src/managers/builtin/inlineScript/main.ts | Switches manager registration to use activation-latched feature state and requires routing registry when enabled. |
| src/managers/builtin/inlineScript/envManager.ts | Adds provenance recording, persisted binding schema, routing-registry-driven validation/refresh, and delete/rename association clearing. |
| src/features/inlineScript/lazyDetector.ts | Adds routing-aware metadata publishing/invalidation logic while preserving telemetry-only behavior when routing is absent. |
| src/features/inlineScript/activation.ts | New helper to latch the feature flag once per activation and create the shared routing registry. |
| src/features/envManagers.ts | Adds routing-aware manager selection and explicit override precedence; reacts to routeability changes. |
| src/extension.ts | Wires activation-latched routing registry into extension activation, detector creation, and manager registration. |
| src/common/inlineScript/routingRegistry.ts | New routing registry tracking per-script metadata identity/revision and validated-association state with change events. |
| src/common/inlineScript/metadata.ts | Adds sourceRange offsets and mapping from normalized offsets to source offsets (BOM/CRLF-aware). |
| src/common/inlineScript/cacheLayout.ts | Adds cache provenance hashes, backup restoration under lock, and serialized in-process sidecar writes with safer replacement behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| teardown(() => { | ||
| sinon.restore(); | ||
| }); |
| const validBackups: Array<{ readonly path: string; readonly metadata: InlineScriptEnvMeta }> = []; | ||
| for (const entry of entries.filter((name) => META_JSON_BACKUP_FILENAME_RE.test(name))) { | ||
| const result = await inspectMetaJsonFile(path.join(envDir.fsPath, entry)); | ||
| if (result.kind === 'valid' && isCompatible(result.metadata)) { | ||
| validBackups.push({ path: path.join(envDir.fsPath, entry), metadata: result.metadata }); | ||
| } else if (result.kind === 'unavailable' || result.kind === 'missing') { | ||
| // A listed candidate changing or becoming unreadable is an | ||
| // uncertain scan; preserve the entry rather than rebuilding it. | ||
| return { kind: 'unavailable' }; | ||
| } | ||
| } |
a891360
into
microsoft:main
Roadmap context
This is PR 9 of 16 in the PEP 723 inline-script roadmap and the repository's intentional routing cutover. Earlier manager PRs create, discover, validate, and persist environments; this PR makes normal per-file environment lookup use them when a saved script association is proven current.
Why this PR
The inline manager can already create/reuse environments, persist a script association, rediscover cache entries after restart, clear state safely, and report lifecycle telemetry. Nothing automatically selects that manager for a Python file, however. A script continues to use its normal project/default environment unless another component directly asks the inline manager.
Automatic routing must be stricter than checking whether a file contains PEP 723 metadata or whether Memento contains an executable path. It must prove both:
If either proof is absent, dirty, stale, transiently unavailable, or superseded by an explicit choice, routing falls through to the existing project/default manager.
What this PR does
Adds an activation-scoped routing registry
shouldRoute(uri)becomes true.Turns the existing lazy detector into a routing input only when enabled
.pyfiles for routing while retaining the existing workspace-only telemetry population.When no routing registry exists, the detector retains its existing telemetry-only listeners, coalescing, workspace filter, and event behavior.
Defines a stable metadata identity
requires-python.[tool]table.Adds durable cache provenance
Makes sidecar replacement recoverable
Upgrades persisted associations from path-only values
Each current record contains:
legacy: old string association; remains retrievable but is not automatically routeable.pending: the environment is proven, but saved metadata has not yet been durably matched (for example, selection while the document is dirty).matched: saved metadata identity and cache provenance agree.Validates associations before routing
Validation requires:
Definitively stale associations are conditionally removed. Locked, transient, uncertain, or future-schema states are preserved but remain non-routeable.
Protects asynchronous validation from stale results
Adds central manager routing with explicit precedence
User flows
Previously configured script after restart
Metadata edit
New unassociated script
Opening a new PEP 723 script does not silently select a matching cache entry. It remains on normal routing until the future explicit setup action (PR 11/12) creates or reuses an environment and persists the association. After that, this PR provides automatic routing.
Routing and failure semantics
Review guide
The production changes are easiest to review in this order:
routingRegistry.tsactivation.tsmetadata.tslazyDetector.tscacheLayout.tsinlineScript/envManager.tsinlineScript/envManager.tsenvManagers.tsextension.tsinlineScript/main.tsMore than half of the diff is deterministic unit coverage for dirty/save/restart and async race behavior.
Tests
Coverage includes:
Validation on the final rebased tree:
npm run compile-testsnpm run compilenpm run lintnpm run unittest: 1,817 passing, 6 pendingPerformance
Privacy and safety
User impact
No default-path user impact.
python-envs.inlineScripts.enabledremains undeclared and defaults to false:For developers manually enabling the hidden flag, existing proven script associations route automatically and fall back conservatively when proof is absent. New scripts still require the future explicit setup UX.
Scope and follow-up
This PR intentionally does not implement:
Those later PRs can build on this guarded routing layer without changing its validation contract.