Repository navigation
feat(update): gate venv removal on install provenance, not a name diff (#15067) - #15572
Merged
Merged
Conversation
#15067) #15063's reconciler already exists and is untouched here. What it carried was a KNOWN LIMITATION in its own docstring: removal decided by diffing installed names against the declared set cannot tell a package this tool installed from one an operator reinstalled under a name the tool's history once declared. That is the operator-package destruction #15067 exists to prevent. Provenance is a marker written into each package's own dist-info directory after a reconcile confirms it installed. A lock file cannot serve here: it records that a name was DECLARED, not that the on-disk install is still the one this tool put there. pip fully replaces a distribution's dist-info on install or uninstall, so marker-presence is evidence nothing has touched that install since, and absence is evidence something has -- which is what pip's own INSTALLER field cannot say. The first-run case is decided rather than left to fall out. Every deployed host has a venv with no markers, including packages this tool did install. Treating absence as "ours" destroys operator packages on the first run this ships; treating it as "never ours" leaves #15063 dead forever on every existing host. Neither is the default: unverified candidates are refused and REPORTED in the job's own steps, and removing them takes an explicit env opt-in an operator can review the reported list against and then unset. Meanwhile every still-declared package is re-marked each run, so coverage grows without a migration step. The test that used to assert an operator's reinstall IS removed now asserts it SURVIVES -- the inversion is the point of the issue, so it is the test that had to change, not be added alongside.
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.
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
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.
Thinking Path
#15063's reconciler — the thing that makes the deployed venv no longer
append-only — already exists on base and is untouched by this PR. What it
carried was a
KNOWN LIMITATIONin its own docstring, which is what #15067 is:That is a data-loss-shaped failure on someone else's machine, so the obvious fix
to #15063 is the dangerous one. This PR closes that hole.
Single-issue rationale: dispatched as a #15063 + #15067 batch; #15063 turned out
already implemented, so only #15067 has a diff. It closes #15067 alone rather
than claiming a closure with no change behind it.
What Changed
Provenance is a marker written into each package's own
dist-infodirectoryafter a reconcile run confirms it installed.
Why not the lock file — the obvious place: a lock records that a name was
declared, not that the on-disk install is still the one this tool put there.
pipfully replaces a distribution'sdist-infoon install or uninstall, somarker-presence is evidence nothing has touched that install since the tool last
confirmed it, and absence is evidence something has. That is precisely what
pip's ownINSTALLERfield cannot tell you, which the issue calls out.split_by_provenancesplits removal candidates into verified and unverified;only verified are removed.
The first-run case, decided rather than emergent
This is the part that mattered most, because every deployed host today has a
venv with no markers — including packages this tool genuinely did install:
Neither is the default. Unverified candidates are refused and reported in the
job's own
steps, and removing them takes an explicit env opt-in(
AUTOBOT_VENV_RECONCILE_ALLOW_UNVERIFIED_REMOVAL, default false) that anoperator can review the reported list against, set for one run, and unset. That
is the reversible first form the change needed.
Meanwhile every still-declared package is re-marked on every run, so marker
coverage — and therefore #15063's automatic behaviour — grows on its own with no
migration step and no flag day.
Verification
Three behaviours, three tests, each mutation-proven:
assert () == ('pkg-orphan',)assert () == ('pkg-stale-collide',)has_tool_provenance→TrueThe orphan-removal test stamps a real marker and runs against a real throwaway
venv, so the "IS removed" path is exercised rather than mocked.
A test was inverted, not added.
test_reconcile_removes_a_name_an_operator_reinstalled_after_it_was_declaredbecame
..._survives_.... That inversion is the issue — the old assertionencoded the defect, so it had to change rather than gain a sibling.
New code lives in a new top-level module rather than under
api/, becauseapi/__init__.pyimports every router and would break the standalonetest-loading pattern
test_venv_reconcile.pyalready documents.Needs host evidence — do not close #15063 on this
#15067 is closable on CI. #15063 is not, and this PR does not claim it:
confirming that the first reconcile on a real host with a pre-existing lock and
zero markers reports its candidate list rather than silently dropping it, and
that the opt-in works end to end through the real update job, requires a
deployed host.
That caution is not theoretical today — a live self-update an hour ago wrote new
code at 16:18:15 while the process performing it had been up since 16:15:14, so
the running code was three minutes older than what it deployed. Exactly the
#15323 trap.
Model Used
Claude Opus 5
Closes #15067
Refs #15063